Re: [PATCH RESEND 09/15] ppc: spapr: Implement nested PAPR hcall - H_GUEST_CREATE_VCPU
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 Signed-off-by: Shivaprasad G Bhat Signed-off-by: Harsh Prateek Bora --- 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 s
Re: [PATCH RESEND 09/15] ppc: spapr: Implement nested PAPR hcall - H_GUEST_CREATE_VCPU
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 > Signed-off-by: Shivaprasad G Bhat > Signed-off-by: Harsh Prateek Bora > --- > 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. > + > +static bool vcpu_check(SpaprMachineStateNestedGuest *guest, > + target_ulong vcpuid, > + bool inoutbuf) What's it checking? That the id is valid? Allocated? Enabled? > +{ > +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; > +} > + > +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. > +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. > +vcpus = g_try_renew(struct SpaprMachineStateNestedGuestVcpu, > +guest->vcpu, > +guest->vcpus + 1); g_try_renew doesn't work with NULL mem? That's unfortunate. > +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? > +/* need to memset to zero otherwise we leak L1 state to L2 */ > +memset(l2env, 0, sizeof(CPUPPCState)); AFAIKS
[PATCH RESEND 09/15] ppc: spapr: Implement nested PAPR hcall - H_GUEST_CREATE_VCPU
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 Signed-off-by: Shivaprasad G Bhat Signed-off-by: Harsh Prateek Bora --- 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; +} + +static bool vcpu_check(SpaprMachineStateNestedGuest *guest, + target_ulong vcpuid, + bool inoutbuf) +{ +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; +} + +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; +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; +vcpus = g_try_renew(struct SpaprMachineStateNestedGuestVcpu, +guest->vcpu, +guest->vcpus + 1); +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; +} +/* need to memset to zero otherwise we leak L1 state to L2 */ +memset(l2env, 0, sizeof(CPUPPCState)); +/* Copy L1 PVR to L2 */ +l2env->spr[SPR_PVR] = env->spr[SPR_PVR]; +cpu_ppc_tb_init(l2env, SPAPR_TIMEBASE_FREQ); + +guest->vcpus++; +assert(vcpuid < guest->vcpus); /* linear vcpuid allocation only */ +guest->vcpu[vcpuid].enabled = true; + +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_creat