Re: [PATCH v4 04/15] spapr: nested: keep nested-hv related code restricted to its API.

[Harsh Prateek Bora]

On 2/27/24 14:24, Nicholas Piggin wrote:
> On Tue Feb 20, 2024 at 6:35 PM AEST, Harsh Prateek Bora wrote:
> > spapr_exit_nested and spapr_get_pate_nested_hv contains code which
> > is specific to nested-hv API. Isolating code flows based on API
> > helps extending it to be used with different API as well.
> >
> > Signed-off-by: Harsh Prateek Bora <harshpb@linux.ibm.com>
> > Suggested-by: Nicholas Piggin <npiggin@gmail.com>
> > ---
> >   include/hw/ppc/spapr_nested.h |  4 ++++
> >   hw/ppc/spapr.c                |  7 ++++++-
> >   hw/ppc/spapr_caps.c           |  1 +
> >   hw/ppc/spapr_nested.c         | 27 ++++++++++++++++++++++++---
> >   4 files changed, 35 insertions(+), 4 deletions(-)
> >
> > diff --git a/include/hw/ppc/spapr_nested.h b/include/hw/ppc/spapr_nested.h
> > index 2488ea98da..3f07c81c3d 100644
> > --- a/include/hw/ppc/spapr_nested.h
> > +++ b/include/hw/ppc/spapr_nested.h
> > @@ -5,6 +5,8 @@
> >   
> >   typedef struct SpaprMachineStateNested {
> >       uint64_t ptcr;
> > +    uint8_t api;
> > +#define NESTED_API_KVM_HV  1
> >   } SpaprMachineStateNested;
> >   
> >   /*
> > @@ -103,4 +105,6 @@ void spapr_exit_nested(PowerPCCPU *cpu, int excp);
> >   typedef struct SpaprMachineState SpaprMachineState;
> >   bool spapr_get_pate_nested_hv(SpaprMachineState *spapr, PowerPCCPU *cpu,
> >                                 target_ulong lpid, ppc_v3_pate_t *entry);
> > +void spapr_nested_init(SpaprMachineState *spapr);
> > +uint8_t spapr_nested_api(SpaprMachineState *spapr);
> >   #endif /* HW_SPAPR_NESTED_H */
> > diff --git a/hw/ppc/spapr.c b/hw/ppc/spapr.c
> > index 97b69c0e42..51a1be027a 100644
> > --- a/hw/ppc/spapr.c
> > +++ b/hw/ppc/spapr.c
> > @@ -1376,7 +1376,11 @@ static bool spapr_get_pate(PPCVirtualHypervisor *vhyp, 
> > PowerPCCPU *cpu,
> >           entry->dw1 = spapr->patb_entry;
> >           return true;
> >       } else {
> > -        return spapr_get_pate_nested_hv(spapr, cpu, lpid, entry);
> > +        assert(spapr_nested_api(spapr));
> > +        if (spapr_nested_api(spapr) == NESTED_API_KVM_HV) {
> > +            return spapr_get_pate_nested_hv(spapr, cpu, lpid, entry);
> > +        }
> > +        return false;
> >       }
> >   }
> >   
> > @@ -3443,6 +3447,7 @@ static void spapr_instance_init(Object *obj)
> >           spapr_get_host_serial, spapr_set_host_serial);
> >       object_property_set_description(obj, "host-serial",
> >           "Host serial number to advertise in guest device tree");
> > +    spapr_nested_init(spapr);
>
> I would maybe make this init a reset instead, and then it could do
> the hypercall unregistering as well? You could rework that part of
> it into patch 1 (or reorder the patches).

If we do unregistering here, we still hit the assert during
spapr_machine_reset which tries to reapply the caps and thus re-register
hcalls. Also, We cant register hcalls in this since the caps havent been
applied when init is called here. So we can do as you have previously
suggested, reset in spapr_machine_reset based on caps applied.
Let me know if you think otherwise?

regards,
Harsh


> Thanks,
> Nick