Re: [PATCH v4 08/15] spapr: nested: Introduce H_GUEST_CREATE_VCPU hcall.

["Nicholas Piggin" <npiggin@gmail.com>]

On Tue Feb 20, 2024 at 6:36 PM AEST, Harsh Prateek Bora wrote:
> Introduce the nested PAPR hcall H_GUEST_CREATE_VCPU which is used to
> create and initialize the specified VCPU resource for the previously
> created guest. Each guest can have multiple VCPUs upto max 2048.
> All VCPUs for a guest gets deallocated on guest delete.
>
> Signed-off-by: Michael Neuling <mikey@neuling.org>
> Signed-off-by: Harsh Prateek Bora <harshpb@linux.ibm.com>
> ---
>  include/hw/ppc/spapr.h        |  2 +
>  include/hw/ppc/spapr_nested.h | 10 ++++
>  hw/ppc/spapr_nested.c         | 96 +++++++++++++++++++++++++++++++++++
>  3 files changed, 108 insertions(+)
>
> diff --git a/include/hw/ppc/spapr.h b/include/hw/ppc/spapr.h
> index c4a79a1785..82b077bdd2 100644
> --- a/include/hw/ppc/spapr.h
> +++ b/include/hw/ppc/spapr.h
> @@ -365,6 +365,7 @@ struct SpaprMachineState {
>  #define H_UNSUPPORTED     -67
>  #define H_OVERLAP         -68
>  #define H_STATE           -75
> +#define H_IN_USE          -77
>  #define H_UNSUPPORTED_FLAG -256
>  #define H_MULTI_THREADS_ACTIVE -9005
>  
> @@ -587,6 +588,7 @@ struct SpaprMachineState {
>  #define H_GUEST_GET_CAPABILITIES 0x460
>  #define H_GUEST_SET_CAPABILITIES 0x464
>  #define H_GUEST_CREATE           0x470
> +#define H_GUEST_CREATE_VCPU      0x474
>  #define H_GUEST_DELETE           0x488
>  
>  #define MAX_HCALL_OPCODE         H_GUEST_DELETE
> diff --git a/include/hw/ppc/spapr_nested.h b/include/hw/ppc/spapr_nested.h
> index f282479275..24e87bca08 100644
> --- a/include/hw/ppc/spapr_nested.h
> +++ b/include/hw/ppc/spapr_nested.h
> @@ -14,6 +14,8 @@ typedef struct SpaprMachineStateNested {
>  
>  typedef struct SpaprMachineStateNestedGuest {
>      uint32_t pvr_logical;
> +    unsigned long vcpus;
> +    struct SpaprMachineStateNestedGuestVcpu *vcpu;
>  } SpaprMachineStateNestedGuest;
>  
>  /* Nested PAPR API related macros */
> @@ -27,6 +29,7 @@ typedef struct SpaprMachineStateNestedGuest {
>  #define H_GUEST_CAP_P10_MODE_BMAP     2
>  #define PAPR_NESTED_GUEST_MAX         4096
>  #define H_GUEST_DELETE_ALL_FLAG       0x8000000000000000ULL
> +#define PAPR_NESTED_GUEST_VCPU_MAX    2048
>  
>  /*
>   * Register state for entering a nested guest with H_ENTER_NESTED.
> @@ -118,8 +121,15 @@ struct nested_ppc_state {
>      uint64_t ppr;
>  
>      int64_t tb_offset;
> +    /* Nested PAPR API */
> +    uint64_t pvr;
>  };
>  
> +typedef struct SpaprMachineStateNestedGuestVcpu {
> +    bool enabled;
> +    struct nested_ppc_state state;
> +} SpaprMachineStateNestedGuestVcpu;
> +
>  void spapr_exit_nested(PowerPCCPU *cpu, int excp);
>  typedef struct SpaprMachineState SpaprMachineState;
>  bool spapr_get_pate_nested_hv(SpaprMachineState *spapr, PowerPCCPU *cpu,
> diff --git a/hw/ppc/spapr_nested.c b/hw/ppc/spapr_nested.c
> index 09c4a35908..3cc704adda 100644
> --- a/hw/ppc/spapr_nested.c
> +++ b/hw/ppc/spapr_nested.c
> @@ -428,6 +428,41 @@ void spapr_exit_nested(PowerPCCPU *cpu, int excp)
>      }
>  }
>  
> +static
> +SpaprMachineStateNestedGuest *spapr_get_nested_guest(SpaprMachineState *spapr,
> +                                                     target_ulong guestid)
> +{
> +    SpaprMachineStateNestedGuest *guest;
> +
> +    guest = g_hash_table_lookup(spapr->nested.guests, GINT_TO_POINTER(guestid));
> +    return guest;
> +}
> +
> +static bool spapr_nested_vcpu_check(SpaprMachineStateNestedGuest *guest,
> +                                    target_ulong vcpuid)
> +{
> +    struct SpaprMachineStateNestedGuestVcpu *vcpu;
> +    /*
> +     * Perform sanity checks for the provided vcpuid of a guest.
> +     * For now, ensure its valid, allocated and enabled for use.
> +     */
> +
> +    if (vcpuid >= PAPR_NESTED_GUEST_VCPU_MAX) {
> +        return false;
> +    }
> +
> +    if (!(vcpuid < guest->vcpus)) {
> +        return false;
> +    }
> +
> +    vcpu = &guest->vcpu[vcpuid];
> +    if (!vcpu->enabled) {
> +        return false;
> +    }
> +
> +    return true;
> +}
> +
>  static target_ulong h_guest_get_capabilities(PowerPCCPU *cpu,
>                                               SpaprMachineState *spapr,
>                                               target_ulong opcode,
> @@ -518,6 +553,7 @@ static void
>  destroy_guest_helper(gpointer value)
>  {
>      struct SpaprMachineStateNestedGuest *guest = value;
> +    g_free(guest->vcpu);
>      g_free(guest);
>  }
>  
> @@ -613,6 +649,65 @@ static target_ulong h_guest_delete(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;
> +    struct nested_ppc_state *l2_state;
> +    target_ulong flags = args[0];
> +    target_ulong guestid = 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, guestid);
> +    if (!guest) {
> +        return H_P2;
> +    }
> +
> +    if (vcpuid < guest->vcpus) {
> +        return H_IN_USE;
> +    }

Linear allocation isn't really a constraint of the API right? I
would add an UNIMP log message to say what the problem is otherwise
hypervisor developer might struggle to understand the problem.

> +
> +    if (guest->vcpus >= PAPR_NESTED_GUEST_VCPU_MAX) {
> +        return H_P3;
> +    }
> +
> +    if (guest->vcpus) {
> +        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(SpaprMachineStateNestedGuestVcpu));
> +        guest->vcpu = vcpus;
> +    } else {
> +        guest->vcpu = g_try_new0(SpaprMachineStateNestedGuestVcpu, 1);
> +        if (guest->vcpu == NULL) {
> +            return H_NO_MEM;
> +        }
> +    }

g_try_renew works with NULL AFAIKS, so no need for the branch. I would
also create a local variable for the new nested guest vcpu created
here so you only have to index it once.

> +    l2_state = &guest->vcpu[guest->vcpus].state;
> +    guest->vcpus++;
> +    assert(vcpuid < guest->vcpus); /* linear vcpuid allocation only */

Target can trigger this assert if using a smaller vcpu id number than
already allocated? I would check above that it is exactly equal to
vcpus, and remove it from here.

> +    /* Set L1 PVR as L2 default */
> +    l2_state->pvr = env->spr[SPR_PVR];

Why is this here and not in H_GUEST_CREATE? I think you can use pcc->pvr
for this?

> +    guest->vcpu[vcpuid].enabled = true;
> +
> +    if (!spapr_nested_vcpu_check(guest, vcpuid)) {
> +        return H_PARAMETER;
> +    }

This doesn't clean up on failure. Just remove "sanity" checks if they
are already checked in the same function. Any useful ones should be
properly hanlded or done before cleanup is needed.

If you're respinning could you call vcpus nr_vcpus, then call vcpu
(the array of vcpus) vcpus.

Thanks,
Nick