| Message ID | 20230906043333.448244-10-harshpb@linux.ibm.com |
|---|---|
| State | New |
| Headers | show |
| Series | Nested PAPR API (KVM on PowerVM) | expand |
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. > + > +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 you just zeroed it above. > + /* 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... > + > + 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? > #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? > typedef struct SpaprMachineStateNestedGuest { > unsigned long vcpus;
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; >
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_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 #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 typedef struct SpaprMachineStateNestedGuest { unsigned long vcpus;