Re: [PATCH RESEND 14/15] ppc: spapr: Implement nested PAPR hcall - H_GUEST_DELETE

[Harsh Prateek Bora]

On 9/7/23 08:01, Nicholas Piggin wrote:
> On Wed Sep 6, 2023 at 2:33 PM AEST, Harsh Prateek Bora wrote:
> > This hcall is used by L1 to delete a guest entry in L0 or can also be
> > used to delete all guests if needed (usually in shutdown scenarios).
>
> I'd squash with at least the create hcall.

Done.

> > Signed-off-by: Michael Neuling <mikey@neuling.org>
> > Signed-off-by: Harsh Prateek Bora <harshpb@linux.ibm.com>
> > ---
> >   hw/ppc/spapr_nested.c         | 32 ++++++++++++++++++++++++++++++++
> >   include/hw/ppc/spapr_nested.h |  1 +
> >   2 files changed, 33 insertions(+)
> >
> > diff --git a/hw/ppc/spapr_nested.c b/hw/ppc/spapr_nested.c
> > index 3605f27115..5afdad4990 100644
> > --- a/hw/ppc/spapr_nested.c
> > +++ b/hw/ppc/spapr_nested.c
> > @@ -1692,6 +1692,37 @@ static void exit_process_output_buffer(PowerPCCPU *cpu,
> >       return;
> >   }
> >   
> > +static target_ulong h_guest_delete(PowerPCCPU *cpu,
> > +                                   SpaprMachineState *spapr,
> > +                                   target_ulong opcode,
> > +                                   target_ulong *args)
> > +{
> > +    target_ulong flags = args[0];
> > +    target_ulong lpid = args[1];
> > +    struct SpaprMachineStateNestedGuest *guest;
> > +
> > +    if (!spapr_get_cap(spapr, SPAPR_CAP_NESTED_PAPR)) {
> > +        return H_FUNCTION;
> > +    }
>
> If you only register these hcalls when you apply the cap, then you
> don't need to test it, right?
Yes, cleaned up now.

> Open question as to whether it's better to register hcalls when
> enabling such caps, or do the tests for them here. I guess the
> former makes sense.

Yeh, I am inclined towards former as well.

> > +
> > +    /* handle flag deleteAllGuests, remaining bits reserved */
>
> This comment is confusing. What is flag deleteAllGuests?
This flag, as per spec, if set, should delete all guests and the
provided guestID is ignored. Updating comment to mention the same.

> H_GUEST_DELETE_ALL_MASK? Is that a mask, or a flag?

Flag, Updating it to H_GUEST_DELETE_ALL_FLAG.

> > +    if (flags & ~H_GUEST_DELETE_ALL_MASK) {
> > +        return H_UNSUPPORTED_FLAG;
> > +    } else if (flags & H_GUEST_DELETE_ALL_MASK) {
> > +        g_hash_table_destroy(spapr->nested.guests);
> > +        return H_SUCCESS;
> > +    }
> > +
> > +    guest = g_hash_table_lookup(spapr->nested.guests, GINT_TO_POINTER(lpid));
> > +    if (!guest) {
> > +        return H_P2;
> > +    }
> > +
> > +    g_hash_table_remove(spapr->nested.guests, GINT_TO_POINTER(lpid));
> > +
> > +    return H_SUCCESS;
> > +}
> > +
> >   void spapr_register_nested(void)
> >   {
> >       spapr_register_hypercall(KVMPPC_H_SET_PARTITION_TABLE, h_set_ptbl);
> > @@ -1709,6 +1740,7 @@ void spapr_register_nested_phyp(void)
> >       spapr_register_hypercall(H_GUEST_SET_STATE       , h_guest_set_state);
> >       spapr_register_hypercall(H_GUEST_GET_STATE       , h_guest_get_state);
> >       spapr_register_hypercall(H_GUEST_RUN_VCPU        , h_guest_run_vcpu);
> > +    spapr_register_hypercall(H_GUEST_DELETE          , h_guest_delete);
> >   }
> >   
> >   #else
> > diff --git a/include/hw/ppc/spapr_nested.h b/include/hw/ppc/spapr_nested.h
> > index ca5d28c06e..9eb43778ad 100644
> > --- a/include/hw/ppc/spapr_nested.h
> > +++ b/include/hw/ppc/spapr_nested.h
> > @@ -209,6 +209,7 @@
> >   #define H_GUEST_GETSET_STATE_FLAG_GUEST_WIDE 0x8000000000000000 /* BE in GSB 
> > */
> >   #define GUEST_STATE_REQUEST_GUEST_WIDE       0x1
> >   #define GUEST_STATE_REQUEST_SET              0x2
> > +#define H_GUEST_DELETE_ALL_MASK              0x8000000000000000ULL
> >   
> >   #define GUEST_STATE_ELEMENT(i, sz, s, f, ptr, c) { \
> >       .id = (i),                                     \