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

[Nicholas Piggin]

On Tue Feb 27, 2024 at 7:45 PM AEST, Harsh Prateek Bora wrote:
>
>
> 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?

Not unregistering here, I mean make it a spapr_nested_reset() call and
call it from spapr_machine_reset().

If you call it after spapr_caps_apply() then you don't need to do the
hcall registering in the caps functions, just do it in the
reset.

Thanks,
Nick