Re: [PATCH RESEND 09/15] ppc: spapr: Implement nested PAPR hcall - H_GUEST_CREATE_VCPU

[Harsh Prateek Bora]

On 9/7/23 08:19, Nicholas Piggin wrote:
> On Wed Sep 6, 2023 at 2:33 PM AEST, Harsh Prateek Bora wrote:
> > This patch implements support for hcall H_GUEST_CREATE_VCPU which is
> > used to instantiate a new VCPU for a previously created nested guest.
> > The L1 provide the guest-id (returned by L0 during call to
> > H_GUEST_CREATE) and an associated unique vcpu-id to refer to this
> > instance in future calls. It is assumed that vcpu-ids are being
> > allocated in a sequential manner and max vcpu limit is 2048.
> >
> > 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         | 110 ++++++++++++++++++++++++++++++++++
> >   include/hw/ppc/spapr.h        |   1 +
> >   include/hw/ppc/spapr_nested.h |   1 +
> >   3 files changed, 112 insertions(+)
> >
> > diff --git a/hw/ppc/spapr_nested.c b/hw/ppc/spapr_nested.c
> > index 09bbbfb341..e7956685af 100644
> > --- a/hw/ppc/spapr_nested.c
> > +++ b/hw/ppc/spapr_nested.c
> > @@ -376,6 +376,47 @@ void spapr_exit_nested(PowerPCCPU *cpu, int excp)
> >       address_space_unmap(CPU(cpu)->as, regs, len, len, true);
> >   }
> >   
> > +static
> > +SpaprMachineStateNestedGuest *spapr_get_nested_guest(SpaprMachineState *spapr,
> > +                                                     target_ulong lpid)
> > +{
> > +    SpaprMachineStateNestedGuest *guest;
> > +
> > +    guest = g_hash_table_lookup(spapr->nested.guests, GINT_TO_POINTER(lpid));
> > +    return guest;
> > +}
>
> Are you namespacing the new API stuff with papr or no? Might be good to
> reduce confusion.
I guess you were referring to vcpu_check below.
Renaming vcpu_check to spapr_nested_vcpu_check().

> > +
> > +static bool vcpu_check(SpaprMachineStateNestedGuest *guest,
> > +                       target_ulong vcpuid,
> > +                       bool inoutbuf)
>
> What's it checking? That the id is valid? Allocated? Enabled?

This is being introduced to do sanity checks for the provided vcpuid of 
a guest. It should check if the vcpuid is valid, allocated and enabled 
before using further.

> > +{
> > +    struct SpaprMachineStateNestedGuestVcpu *vcpu;
> > +
> > +    if (vcpuid >= NESTED_GUEST_VCPU_MAX) {
> > +        return false;
> > +    }
> > +
> > +    if (!(vcpuid < guest->vcpus)) {
> > +        return false;
> > +    }
> > +
> > +    vcpu = &guest->vcpu[vcpuid];
> > +    if (!vcpu->enabled) {
> > +        return false;
> > +    }
> > +
> > +    if (!inoutbuf) {
> > +        return true;
> > +    }
> > +
> > +    /* Check to see if the in/out buffers are registered */
> > +    if (vcpu->runbufin.addr && vcpu->runbufout.addr) {
> > +        return true;
> > +    }
> > +

I think I shall move in/out buf related checks to vcpu_run patch.

> > +    return false;
> > +}
> > +
> >   static target_ulong h_guest_get_capabilities(PowerPCCPU *cpu,
> >                                                SpaprMachineState *spapr,
> >                                                target_ulong opcode,
> > @@ -448,6 +489,11 @@ static void
> >   destroy_guest_helper(gpointer value)
> >   {
> >       struct SpaprMachineStateNestedGuest *guest = value;
> > +    int i = 0;
>
> Don't need to set i = 0 twice. A newline would be good though.

Yeh, declaring with for loop and removing above init.

> > +    for (i = 0; i < guest->vcpus; i++) {
> > +        cpu_ppc_tb_free(&guest->vcpu[i].env);
> > +    }
> > +    g_free(guest->vcpu);
> >       g_free(guest);
> >   }
> >   
> > @@ -518,6 +564,69 @@ static target_ulong h_guest_create(PowerPCCPU *cpu,
> >       return H_SUCCESS;
> >   }
> >   
> > +static target_ulong h_guest_create_vcpu(PowerPCCPU *cpu,
> > +                                        SpaprMachineState *spapr,
> > +                                        target_ulong opcode,
> > +                                        target_ulong *args)
> > +{
> > +    CPUPPCState *env = &cpu->env, *l2env;
> > +    target_ulong flags = args[0];
> > +    target_ulong lpid = args[1];
> > +    target_ulong vcpuid = args[2];
> > +    SpaprMachineStateNestedGuest *guest;
> > +
> > +    if (flags) { /* don't handle any flags for now */
> > +        return H_UNSUPPORTED_FLAG;
> > +    }
> > +
> > +    guest = spapr_get_nested_guest(spapr, lpid);
> > +    if (!guest) {
> > +        return H_P2;
> > +    }
> > +
> > +    if (vcpuid < guest->vcpus) {
> > +        return H_IN_USE;
> > +    }
> > +
> > +    if (guest->vcpus >= NESTED_GUEST_VCPU_MAX) {
> > +        return H_P3;
> > +    }
> > +
> > +    if (guest->vcpus) {
> > +        struct SpaprMachineStateNestedGuestVcpu *vcpus;
>
> Ditto for using typedefs. Do a sweep for this.
Sure, done.

> > +        vcpus = g_try_renew(struct SpaprMachineStateNestedGuestVcpu,
> > +                            guest->vcpu,
> > +                            guest->vcpus + 1);
>
> g_try_renew doesn't work with NULL mem? That's unfortunate.

Hmm, behaviour with NULL is undefined, so keeping as is.

> > +        if (!vcpus) {
> > +            return H_NO_MEM;
> > +        }
> > +        memset(&vcpus[guest->vcpus], 0,
> > +               sizeof(struct SpaprMachineStateNestedGuestVcpu));
> > +        guest->vcpu = vcpus;
> > +        l2env = &vcpus[guest->vcpus].env;
> > +    } else {
> > +        guest->vcpu = g_try_new0(struct SpaprMachineStateNestedGuestVcpu, 1);
> > +        if (guest->vcpu == NULL) {
> > +            return H_NO_MEM;
> > +        }
> > +        l2env = &guest->vcpu->env;
> > +    }
>
> These two legs seem to be doing the same thing in different
> ways wrt l2env. Just assign guest->vcpu in the branches and
> get the l2env from guest->vcpu[guest->vcpus] afterward, no?
Sure, that seems better.

> > +    /* need to memset to zero otherwise we leak L1 state to L2 */
> > +    memset(l2env, 0, sizeof(CPUPPCState));
>
> AFAIKS you just zeroed it above.

Yeh, cleaning up the redundant memset.

> > +    /* Copy L1 PVR to L2 */
> > +    l2env->spr[SPR_PVR] = env->spr[SPR_PVR];
> > +    cpu_ppc_tb_init(l2env, SPAPR_TIMEBASE_FREQ);
>
> I would move this down to the end, because it's setting up the
> vcpu...

Make sense to re-order above and below chunks.

> > +
> > +    guest->vcpus++;
> > +    assert(vcpuid < guest->vcpus); /* linear vcpuid allocation only */
> > +    guest->vcpu[vcpuid].enabled = true;
> > +
>
> ... This is still allocating the vcpu so move it up.
>
> > +    if (!vcpu_check(guest, vcpuid, false)) {
> > +        return H_PARAMETER;
> > +    }
> > +    return H_SUCCESS;
> > +}
> > +
> >   void spapr_register_nested(void)
> >   {
> >       spapr_register_hypercall(KVMPPC_H_SET_PARTITION_TABLE, h_set_ptbl);
> > @@ -531,6 +640,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);
> > +    spapr_register_hypercall(H_GUEST_CREATE_VCPU     , h_guest_create_vcpu);
> >   }
> >   
> >   #else
> > diff --git a/include/hw/ppc/spapr.h b/include/hw/ppc/spapr.h
> > index 8a6e9ce929..c9f9682a46 100644
> > --- a/include/hw/ppc/spapr.h
> > +++ b/include/hw/ppc/spapr.h
> > @@ -371,6 +371,7 @@ struct SpaprMachineState {
> >   #define H_UNSUPPORTED     -67
> >   #define H_OVERLAP         -68
> >   #define H_STATE           -75
> > +#define H_IN_USE          -77
>
> Why add it here and not in the first patch?

Yeh, it was a miss for initial patch, but I guess, we want it here only 
for patch v2. Introducing stuff where they are used first.

> >   #define H_INVALID_ELEMENT_ID               -79
> >   #define H_INVALID_ELEMENT_SIZE             -80
> >   #define H_INVALID_ELEMENT_VALUE            -81
> > diff --git a/include/hw/ppc/spapr_nested.h b/include/hw/ppc/spapr_nested.h
> > index 7841027df8..2e8c6ba1ca 100644
> > --- a/include/hw/ppc/spapr_nested.h
> > +++ b/include/hw/ppc/spapr_nested.h
> > @@ -199,6 +199,7 @@
> >   
> >   /* Nested PAPR API macros */
> >   #define NESTED_GUEST_MAX 4096
> > +#define NESTED_GUEST_VCPU_MAX 2048
> >   
>
> PAPR_ prefix?
Done.

Thanks
Harsh
> >   typedef struct SpaprMachineStateNestedGuest {
> >       unsigned long vcpus;