[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]
Re: [PATCH RESEND 09/15] ppc: spapr: Implement nested PAPR hcall - H_GUE
From: |
Nicholas Piggin |
Subject: |
Re: [PATCH RESEND 09/15] ppc: spapr: Implement nested PAPR hcall - H_GUEST_CREATE_VCPU |
Date: |
Thu, 07 Sep 2023 12:49:33 +1000 |
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;
- [PATCH RESEND 02/15] ppc: spapr: Add new/extend structs to support Nested PAPR API, (continued)
- [PATCH RESEND 02/15] ppc: spapr: Add new/extend structs to support Nested PAPR API, Harsh Prateek Bora, 2023/09/06
- [PATCH RESEND 13/15] ppc: spapr: Implement nested PAPR hcall - H_GUEST_RUN_VCPU, Harsh Prateek Bora, 2023/09/06
- [PATCH RESEND 05/15] ppc: spapr: Introduce cap-nested-papr for nested PAPR API, Harsh Prateek Bora, 2023/09/06
- [PATCH RESEND 09/15] ppc: spapr: Implement nested PAPR hcall - H_GUEST_CREATE_VCPU, Harsh Prateek Bora, 2023/09/06
- Re: [PATCH RESEND 09/15] ppc: spapr: Implement nested PAPR hcall - H_GUEST_CREATE_VCPU,
Nicholas Piggin <=
- [PATCH RESEND 15/15] ppc: spapr: Document Nested PAPR API, Harsh Prateek Bora, 2023/09/06
- [PATCH RESEND 04/15] ppc: spapr: Start using nested.api for nested kvm-hv api, Harsh Prateek Bora, 2023/09/06
- [PATCH RESEND 11/15] ppc: spapr: Implement nested PAPR hcall - H_GUEST_[GET|SET]_STATE, Harsh Prateek Bora, 2023/09/06
- [PATCH RESEND 10/15] ppc: spapr: Initialize the GSB Elements lookup table., Harsh Prateek Bora, 2023/09/06