Re: [PATCH v4 12/15] spapr: nested: Use correct source for parttbl info for nested PAPR API.

[Harsh Prateek Bora]

Hi Nick,

On 2/27/24 15:46, Nicholas Piggin wrote:
> On Tue Feb 20, 2024 at 6:36 PM AEST, Harsh Prateek Bora wrote:
> > For nested PAPR API, we use SpaprMachineStateNestedGuest struct to store
> > partition table info, use the same in spapr_get_pate_nested() via
> > helper.
> >
> > Signed-off-by: Michael Neuling <mikey@neuling.org>
> > Signed-off-by: Harsh Prateek Bora <harshpb@linux.ibm.com>
> > ---
> >   include/hw/ppc/spapr_nested.h |  4 ++++
> >   hw/ppc/spapr.c                |  2 ++
> >   hw/ppc/spapr_nested.c         | 20 +++++++++++++++++++-
> >   3 files changed, 25 insertions(+), 1 deletion(-)
> >
> > diff --git a/include/hw/ppc/spapr_nested.h b/include/hw/ppc/spapr_nested.h
> > index 1b7e55f12a..da918d2dd0 100644
> > --- a/include/hw/ppc/spapr_nested.h
> > +++ b/include/hw/ppc/spapr_nested.h
> > @@ -511,4 +511,8 @@ bool spapr_get_pate_nested_hv(SpaprMachineState *spapr, 
> > PowerPCCPU *cpu,
> >   void spapr_nested_init(SpaprMachineState *spapr);
> >   uint8_t spapr_nested_api(SpaprMachineState *spapr);
> >   void spapr_nested_gsb_init(void);
> > +bool spapr_get_pate_nested_papr(SpaprMachineState *spapr, PowerPCCPU *cpu,
> > +                                target_ulong lpid, ppc_v3_pate_t *entry);
> > +SpaprMachineStateNestedGuest *spapr_get_nested_guest(SpaprMachineState *spapr,
> > +                                                     target_ulong lpid);
> >   #endif /* HW_SPAPR_NESTED_H */
> > diff --git a/hw/ppc/spapr.c b/hw/ppc/spapr.c
> > index 51a1be027a..3453b30a57 100644
> > --- a/hw/ppc/spapr.c
> > +++ b/hw/ppc/spapr.c
> > @@ -1379,6 +1379,8 @@ static bool spapr_get_pate(PPCVirtualHypervisor *vhyp, 
> > PowerPCCPU *cpu,
> >           assert(spapr_nested_api(spapr));
> >           if (spapr_nested_api(spapr) == NESTED_API_KVM_HV) {
> >               return spapr_get_pate_nested_hv(spapr, cpu, lpid, entry);
> > +        } else if (spapr_nested_api(spapr) == NESTED_API_PAPR) {
> > +            return spapr_get_pate_nested_papr(spapr, cpu, lpid, entry);
> >           }
> >           return false;
> >       }
>
> BTW. I would change these asserts to } else { g_assert_not_reached(); }

Sure, updating as suggested.

> > diff --git a/hw/ppc/spapr_nested.c b/hw/ppc/spapr_nested.c
> > index aba4b25da6..0edb362709 100644
> > --- a/hw/ppc/spapr_nested.c
> > +++ b/hw/ppc/spapr_nested.c
> > @@ -52,6 +52,19 @@ bool spapr_get_pate_nested_hv(SpaprMachineState *spapr, 
> > PowerPCCPU *cpu,
> >       return true;
> >   }
> >   
> > +bool spapr_get_pate_nested_papr(SpaprMachineState *spapr, PowerPCCPU *cpu,
> > +                                target_ulong lpid, ppc_v3_pate_t *entry)
> > +{
> > +    SpaprMachineStateNestedGuest *guest;
> > +    assert(lpid != 0);
> > +    guest = spapr_get_nested_guest(spapr, lpid);
> > +    assert(guest != NULL);
> > +
> > +    entry->dw0 = guest->parttbl[0];
> > +    entry->dw1 = guest->parttbl[1];
> > +    return true;
> > +}
>
> Asserts should not need to be changed to proper error handling, right?

Hmm, changing !guest check to return false as more appropriate.
lpid check shall remain an assert.

regards,
Harsh

> Thanks,
> Nick