Re: [PATCH RESEND 08/15] ppc: spapr: Implement nested PAPR hcall - H_GUEST_CREATE

[Harsh Prateek Bora]

On 9/7/23 07:58, Nicholas Piggin wrote:
> On Wed Sep 6, 2023 at 2:33 PM AEST, Harsh Prateek Bora wrote:
> > This hcall is used by L1 to indicate to L0 that a new nested guest needs
> > to be created and therefore necessary resource allocation shall be made.
> > The L0 uses a hash table for nested guest specific resource management.
> > This data structure is further utilized by other hcalls to operate on
> > related members during entire life cycle of the nested guest.
>
> Similar comment for changelog re detail. Detailed specification of API
> and implementation could go in comments or documentation if useful.
Sure, squashing guest create/delete together and updating commit log to 
be abstract as needed.

> > Signed-off-by: Michael Neuling <mikey@neuling.org>
> > Signed-off-by: Shivaprasad G Bhat <sbhat@linux.ibm.com>
> > Signed-off-by: Harsh Prateek Bora <harshpb@linux.ibm.com>
> > ---
> >   hw/ppc/spapr_nested.c         | 75 +++++++++++++++++++++++++++++++++++
> >   include/hw/ppc/spapr_nested.h |  3 ++
> >   2 files changed, 78 insertions(+)
> >
> > diff --git a/hw/ppc/spapr_nested.c b/hw/ppc/spapr_nested.c
> > index 9af65f257f..09bbbfb341 100644
> > --- a/hw/ppc/spapr_nested.c
> > +++ b/hw/ppc/spapr_nested.c
> > @@ -444,6 +444,80 @@ static target_ulong h_guest_set_capabilities(PowerPCCPU 
> > *cpu,
> >       return H_SUCCESS;
> >   }
> >   
> > +static void
> > +destroy_guest_helper(gpointer value)
> > +{
> > +    struct SpaprMachineStateNestedGuest *guest = value;
> > +    g_free(guest);
> > +}
> > +
> > +static target_ulong h_guest_create(PowerPCCPU *cpu,
> > +                                   SpaprMachineState *spapr,
> > +                                   target_ulong opcode,
> > +                                   target_ulong *args)
> > +{
> > +    CPUPPCState *env = &cpu->env;
> > +    target_ulong flags = args[0];
> > +    target_ulong continue_token = args[1];
> > +    uint64_t lpid;
> > +    int nguests = 0;
> > +    struct SpaprMachineStateNestedGuest *guest;
> > +
> > +    if (flags) { /* don't handle any flags for now */
> > +        return H_UNSUPPORTED_FLAG;
> > +    }
> > +
> > +    if (continue_token != -1) {
> > +        return H_P2;
> > +    }
> > +
> > +    if (!spapr_get_cap(spapr, SPAPR_CAP_NESTED_PAPR)) {
> > +        return H_FUNCTION;
> > +    }
> > +
> > +    if (!spapr->nested.capabilities_set) {
> > +        return H_STATE;
> > +    }
> > +
> > +    if (!spapr->nested.guests) {
> > +        spapr->nested.lpid_max = NESTED_GUEST_MAX;
> > +        spapr->nested.guests = g_hash_table_new_full(NULL,
> > +                                                     NULL,
> > +                                                     NULL,
> > +                                                     destroy_guest_helper);
>
> Is lpid_max only used by create? Probably no need to have it in spapr
> then->nested then. Also, do we even need to have a limit?

Yes, as of now, it is being used only by create and doesnt need to part
of spapr->nested. We can simply use the macro for max guests. Keeping it
to emulate a finite resource model.
For all practical purposes, nested guests in an TCG emulated L0
shouldn't reach that limit.

> > +    }
> > +
> > +    nguests = g_hash_table_size(spapr->nested.guests);
> > +
> > +    if (nguests == spapr->nested.lpid_max) {
> > +        return H_NO_MEM;
> > +    }
> > +
> > +    /* Lookup for available lpid */
> > +    for (lpid = 1; lpid < spapr->nested.lpid_max; lpid++) {
>
> PAPR API calls it "guest ID" I think. Should change all references to
> lpid to that.

Changing it to "guestid".

> > +        if (!(g_hash_table_lookup(spapr->nested.guests,
> > +                                  GINT_TO_POINTER(lpid)))) {
> > +            break;
> > +        }
> > +    }
> > +    if (lpid == spapr->nested.lpid_max) {
> > +        return H_NO_MEM;
> > +    }
> > +
> > +    guest = g_try_new0(struct SpaprMachineStateNestedGuest, 1);
> > +    if (!guest) {
> > +        return H_NO_MEM;
> > +    }
> > +
> > +    guest->pvr_logical = spapr->nested.pvr_base;
> > +
> > +    g_hash_table_insert(spapr->nested.guests, GINT_TO_POINTER(lpid), guest);
> > +    printf("%s: lpid: %lu (MAX: %i)\n", __func__, lpid, 
> > spapr->nested.lpid_max);
>
> Remove printf.
Done.

> > +
> > +    env->gpr[4] = lpid;
> > +    return H_SUCCESS;
> > +}
> > +
> >   void spapr_register_nested(void)
> >   {
> >       spapr_register_hypercall(KVMPPC_H_SET_PARTITION_TABLE, h_set_ptbl);
> > @@ -456,6 +530,7 @@ void spapr_register_nested_phyp(void)
> >   {
> >       spapr_register_hypercall(H_GUEST_GET_CAPABILITIES, 
> > h_guest_get_capabilities);
> >       spapr_register_hypercall(H_GUEST_SET_CAPABILITIES, 
> > h_guest_set_capabilities);
> > +    spapr_register_hypercall(H_GUEST_CREATE          , h_guest_create);
> >   }
> >   
> >   #else
> > diff --git a/include/hw/ppc/spapr_nested.h b/include/hw/ppc/spapr_nested.h
> > index a7996251cb..7841027df8 100644
> > --- a/include/hw/ppc/spapr_nested.h
> > +++ b/include/hw/ppc/spapr_nested.h
> > @@ -197,6 +197,9 @@
> >   #define H_GUEST_CAP_P9_MODE_BMAP    1
> >   #define H_GUEST_CAP_P10_MODE_BMAP   2
> >   
> > +/* Nested PAPR API macros */
> > +#define NESTED_GUEST_MAX 4096
>
> Prefix with PAPR_?

Done.

Thanks
Harsh
> Thanks,
> Nick
>
> > +
> >   typedef struct SpaprMachineStateNestedGuest {
> >       unsigned long vcpus;
> >       struct SpaprMachineStateNestedGuestVcpu *vcpu;