Re: [PATCH RESEND 07/15] ppc: spapr: Implement nested PAPR hcall - H_GUEST_SET_CAPABILITIES

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

On Wed Sep 6, 2023 at 2:33 PM AEST, Harsh Prateek Bora wrote:
> This patch implements nested PAPR hcall H_GUEST_SET_CAPABILITIES.
> This is used by L1 to set capabilities of the nested guest being
> created. The capabilities being set are subset of the capabilities
> returned from the previous call to H_GUEST_GET_CAPABILITIES hcall.
> Currently, it only supports P9/P10 capability check through PVR.
>
> Signed-off-by: Michael Neuling <mikey@neuling.org>
> Signed-off-by: Harsh Prateek Bora <harshpb@linux.ibm.com>
> ---
>  hw/ppc/spapr.c                |  1 +
>  hw/ppc/spapr_nested.c         | 46 +++++++++++++++++++++++++++++++++++
>  include/hw/ppc/spapr_nested.h |  3 +++
>  3 files changed, 50 insertions(+)
>
> diff --git a/hw/ppc/spapr.c b/hw/ppc/spapr.c
> index cbab7a825f..7c6f6ee25d 100644
> --- a/hw/ppc/spapr.c
> +++ b/hw/ppc/spapr.c
> @@ -3443,6 +3443,7 @@ static void spapr_instance_init(Object *obj)
>          "Host serial number to advertise in guest device tree");
>      /* Nested */
>      spapr->nested.api = 0;
> +    spapr->nested.capabilities_set = false;

I would actually think about moving spapr->nested init into
spapr_nested.c.

>  }
>  
>  static void spapr_machine_finalizefn(Object *obj)
> diff --git a/hw/ppc/spapr_nested.c b/hw/ppc/spapr_nested.c
> index 37f3a49be2..9af65f257f 100644
> --- a/hw/ppc/spapr_nested.c
> +++ b/hw/ppc/spapr_nested.c
> @@ -399,6 +399,51 @@ static target_ulong h_guest_get_capabilities(PowerPCCPU *cpu,
>      return H_SUCCESS;
>  }
>  
> +static target_ulong h_guest_set_capabilities(PowerPCCPU *cpu,
> +                                             SpaprMachineState *spapr,
> +                                             target_ulong opcode,
> +                                              target_ulong *args)
> +{
> +    CPUPPCState *env = &cpu->env;
> +    target_ulong flags = args[0];
> +    target_ulong capabilities = args[1];
> +
> +    if (flags) { /* don't handle any flags capabilities for now */
> +        return H_PARAMETER;
> +    }
> +
> +

May need to do a pass over whitespace.

> +    /* isn't supported */
> +    if (capabilities & H_GUEST_CAPABILITIES_COPY_MEM) {
> +        env->gpr[4] = 0;
> +        return H_P2;
> +    }
> +
> +    if ((env->spr[SPR_PVR] & CPU_POWERPC_POWER_SERVER_MASK) ==
> +        (CPU_POWERPC_POWER9_BASE)) {
> +        /* We are a P9 */
> +        if (!(capabilities & H_GUEST_CAPABILITIES_P9_MODE)) {
> +            env->gpr[4] = 1;
> +            return H_P2;
> +        }
> +    }
> +
> +    if ((env->spr[SPR_PVR] & CPU_POWERPC_POWER_SERVER_MASK) ==
> +        (CPU_POWERPC_POWER10_BASE)) {
> +        /* We are a P10 */

The 2 comments above aren't helpful. Just remove them.

> +        if (!(capabilities & H_GUEST_CAPABILITIES_P10_MODE)) {
> +            env->gpr[4] = 2;
> +            return H_P2;
> +        }
> +    }
> +
> +    spapr->nested.capabilities_set = true;

Is it okay to set twice? If not, add a check. If yes, remove
capabilities_set until it's needed.

> +
> +    spapr->nested.pvr_base = env->spr[SPR_PVR];
> +
> +    return H_SUCCESS;
> +}
> +
>  void spapr_register_nested(void)
>  {
>      spapr_register_hypercall(KVMPPC_H_SET_PARTITION_TABLE, h_set_ptbl);
> @@ -410,6 +455,7 @@ void spapr_register_nested(void)
>  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);
>  }
>  
>  #else
> diff --git a/include/hw/ppc/spapr_nested.h b/include/hw/ppc/spapr_nested.h
> index ce198e9f70..a7996251cb 100644
> --- a/include/hw/ppc/spapr_nested.h
> +++ b/include/hw/ppc/spapr_nested.h
> @@ -193,6 +193,9 @@
>  #define H_GUEST_CAPABILITIES_COPY_MEM 0x8000000000000000
>  #define H_GUEST_CAPABILITIES_P9_MODE  0x4000000000000000
>  #define H_GUEST_CAPABILITIES_P10_MODE 0x2000000000000000
> +#define H_GUEST_CAP_COPY_MEM_BMAP   0
> +#define H_GUEST_CAP_P9_MODE_BMAP    1
> +#define H_GUEST_CAP_P10_MODE_BMAP   2
>  
>  typedef struct SpaprMachineStateNestedGuest {
>      unsigned long vcpus;