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 = >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(>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 = >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([guest->vcpus], 0,
+ sizeof(struct SpaprMachineStateNestedGuestVcpu));
+guest->vcpu = vcpus;
+l2env = [guest->vcpus].env;
+} else {
+guest->vcpu = g_try_new0(struct SpaprMachineStateNestedGuestVcpu, 1);
+if (guest->vcpu == NULL) {
+return H_NO_MEM;
+}
+l2env = >vcpu->env;
+}
These two legs seem to be doing the same thing in
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 = >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(>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 = >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([guest->vcpus], 0,
> + sizeof(struct SpaprMachineStateNestedGuestVcpu));
> +guest->vcpu = vcpus;
> +l2env = [guest->vcpus].env;
> +} else {
> +guest->vcpu = g_try_new0(struct SpaprMachineStateNestedGuestVcpu, 1);
> +if (guest->vcpu == NULL) {
> +return H_NO_MEM;
> +}
> +l2env = >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.
> +/*
[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 = >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(>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 = >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([guest->vcpus], 0,
+ sizeof(struct SpaprMachineStateNestedGuestVcpu));
+guest->vcpu = vcpus;
+l2env = [guest->vcpus].env;
+} else {
+guest->vcpu = g_try_new0(struct SpaprMachineStateNestedGuestVcpu, 1);
+if (guest->vcpu == NULL) {
+return H_NO_MEM;
+}
+l2env = >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
