Re: [PATCH v8 2/2] ppc: spapr: Enable 2nd DAWR on Power10 pSeries machine

[Harsh Prateek Bora <harshpb@linux.ibm.com>]

Hi Shiva,

On 2/1/24 20:16, Shivaprasad G Bhat wrote:
> As per the PAPR, bit 0 of byte 64 in pa-features property
> indicates availability of 2nd DAWR registers. i.e. If this bit is set, 2nd
> DAWR is present, otherwise not. Use KVM_CAP_PPC_DAWR1 capability to find
> whether kvm supports 2nd DAWR or not. If it's supported, allow user to set
> the pa-feature bit in guest DT using cap-dawr1 machine capability.
> 
> Signed-off-by: Ravi Bangoria <ravi.bangoria@linux.ibm.com>
> Signed-off-by: Shivaprasad G Bhat <sbhat@linux.ibm.com>
> ---
>   hw/ppc/spapr.c         |    7 ++++++-
>   hw/ppc/spapr_caps.c    |   36 ++++++++++++++++++++++++++++++++++++
>   hw/ppc/spapr_hcall.c   |   25 ++++++++++++++++---------
>   include/hw/ppc/spapr.h |    6 +++++-
>   target/ppc/kvm.c       |   12 ++++++++++++
>   target/ppc/kvm_ppc.h   |   12 ++++++++++++
>   6 files changed, 87 insertions(+), 11 deletions(-)
> 

<snipped>

> diff --git a/hw/ppc/spapr_hcall.c b/hw/ppc/spapr_hcall.c
> index fcefd1d1c7..34c1c77c95 100644
> --- a/hw/ppc/spapr_hcall.c
> +++ b/hw/ppc/spapr_hcall.c
> @@ -814,11 +814,12 @@ static target_ulong h_set_mode_resource_set_ciabr(PowerPCCPU *cpu,
>       return H_SUCCESS;
>   }
>   
> -static target_ulong h_set_mode_resource_set_dawr0(PowerPCCPU *cpu,
> -                                                  SpaprMachineState *spapr,
> -                                                  target_ulong mflags,
> -                                                  target_ulong value1,
> -                                                  target_ulong value2)
> +static target_ulong h_set_mode_resource_set_dawr(PowerPCCPU *cpu,
> +                                                     SpaprMachineState *spapr,
> +                                                     target_ulong mflags,
> +                                                     target_ulong resource,
> +                                                     target_ulong value1,
> +                                                     target_ulong value2)
>   {
>       CPUPPCState *env = &cpu->env;
>   
> @@ -831,8 +832,13 @@ static target_ulong h_set_mode_resource_set_dawr0(PowerPCCPU *cpu,
>           return H_P4;
>       }
>   
> -    ppc_store_dawr0(env, value1);
> -    ppc_store_dawrx0(env, value2);
> +    if (resource == H_SET_MODE_RESOURCE_SET_DAWR0) {
> +        ppc_store_dawr0(env, value1);
> +        ppc_store_dawrx0(env, value2);
> +    } else {

Should we explicitly check here for resource == 
H_SET_MODE_RESOURCE_SET_DAWR0 ?

> +        ppc_store_dawr1(env, value1);
> +        ppc_store_dawrx1(env, value2);

and then do an else g_assert_not_reached() ?

With suggested changes after addressing other review comments from 
Nick/David:

Reviewed-by: Harsh Prateek Bora <harshpb@linux.ibm.com>

> +    }
>   
>       return H_SUCCESS;
>   }
> @@ -911,8 +917,9 @@ static target_ulong h_set_mode(PowerPCCPU *cpu, SpaprMachineState *spapr,
>                                               args[3]);
>           break;
>       case H_SET_MODE_RESOURCE_SET_DAWR0:
> -        ret = h_set_mode_resource_set_dawr0(cpu, spapr, args[0], args[2],
> -                                            args[3]);
> +    case H_SET_MODE_RESOURCE_SET_DAWR1:
> +        ret = h_set_mode_resource_set_dawr(cpu, spapr, args[0], args[1],
> +                                           args[2], args[3]);
>           break;
>       case H_SET_MODE_RESOURCE_LE:
>           ret = h_set_mode_resource_le(cpu, spapr, args[0], args[2], args[3]);
> diff --git a/include/hw/ppc/spapr.h b/include/hw/ppc/spapr.h
> index e91791a1a9..2b13c9a00e 100644
> --- a/include/hw/ppc/spapr.h
> +++ b/include/hw/ppc/spapr.h
> @@ -80,8 +80,10 @@ typedef enum {
>   #define SPAPR_CAP_RPT_INVALIDATE        0x0B
>   /* Support for AIL modes */
>   #define SPAPR_CAP_AIL_MODE_3            0x0C
> +/* DAWR1 */
> +#define SPAPR_CAP_DAWR1                 0x0D
>   /* Num Caps */
> -#define SPAPR_CAP_NUM                   (SPAPR_CAP_AIL_MODE_3 + 1)
> +#define SPAPR_CAP_NUM                   (SPAPR_CAP_DAWR1 + 1)
>   
>   /*
>    * Capability Values
> @@ -403,6 +405,7 @@ struct SpaprMachineState {
>   #define H_SET_MODE_RESOURCE_SET_DAWR0           2
>   #define H_SET_MODE_RESOURCE_ADDR_TRANS_MODE     3
>   #define H_SET_MODE_RESOURCE_LE                  4
> +#define H_SET_MODE_RESOURCE_SET_DAWR1           5
>   
>   /* Flags for H_SET_MODE_RESOURCE_LE */
>   #define H_SET_MODE_ENDIAN_BIG    0
> @@ -986,6 +989,7 @@ extern const VMStateDescription vmstate_spapr_cap_ccf_assist;
>   extern const VMStateDescription vmstate_spapr_cap_fwnmi;
>   extern const VMStateDescription vmstate_spapr_cap_rpt_invalidate;
>   extern const VMStateDescription vmstate_spapr_wdt;
> +extern const VMStateDescription vmstate_spapr_cap_dawr1;
>   
>   static inline uint8_t spapr_get_cap(SpaprMachineState *spapr, int cap)
>   {
> diff --git a/target/ppc/kvm.c b/target/ppc/kvm.c
> index 26fa9d0575..3d8a8f35b6 100644
> --- a/target/ppc/kvm.c
> +++ b/target/ppc/kvm.c
> @@ -89,6 +89,7 @@ static int cap_large_decr;
>   static int cap_fwnmi;
>   static int cap_rpt_invalidate;
>   static int cap_ail_mode_3;
> +static int cap_dawr1;
>   
>   static uint32_t debug_inst_opcode;
>   
> @@ -143,6 +144,7 @@ int kvm_arch_init(MachineState *ms, KVMState *s)
>       cap_ppc_nested_kvm_hv = kvm_vm_check_extension(s, KVM_CAP_PPC_NESTED_HV);
>       cap_large_decr = kvmppc_get_dec_bits();
>       cap_fwnmi = kvm_vm_check_extension(s, KVM_CAP_PPC_FWNMI);
> +    cap_dawr1 = kvm_vm_check_extension(s, KVM_CAP_PPC_DAWR1);
>       /*
>        * Note: setting it to false because there is not such capability
>        * in KVM at this moment.
> @@ -2109,6 +2111,16 @@ int kvmppc_set_fwnmi(PowerPCCPU *cpu)
>       return kvm_vcpu_enable_cap(cs, KVM_CAP_PPC_FWNMI, 0);
>   }
>   
> +bool kvmppc_has_cap_dawr1(void)
> +{
> +    return !!cap_dawr1;
> +}
> +
> +int kvmppc_set_cap_dawr1(int enable)
> +{
> +    return kvm_vm_enable_cap(kvm_state, KVM_CAP_PPC_DAWR1, 0, enable);
> +}
> +
>   int kvmppc_smt_threads(void)
>   {
>       return cap_ppc_smt ? cap_ppc_smt : 1;
> diff --git a/target/ppc/kvm_ppc.h b/target/ppc/kvm_ppc.h
> index 1975fb5ee6..493d6bb477 100644
> --- a/target/ppc/kvm_ppc.h
> +++ b/target/ppc/kvm_ppc.h
> @@ -68,6 +68,8 @@ bool kvmppc_has_cap_htm(void);
>   bool kvmppc_has_cap_mmu_radix(void);
>   bool kvmppc_has_cap_mmu_hash_v3(void);
>   bool kvmppc_has_cap_xive(void);
> +bool kvmppc_has_cap_dawr1(void);
> +int kvmppc_set_cap_dawr1(int enable);
>   int kvmppc_get_cap_safe_cache(void);
>   int kvmppc_get_cap_safe_bounds_check(void);
>   int kvmppc_get_cap_safe_indirect_branch(void);
> @@ -377,6 +379,16 @@ static inline bool kvmppc_has_cap_xive(void)
>       return false;
>   }
>   
> +static inline bool kvmppc_has_cap_dawr1(void)
> +{
> +    return false;
> +}
> +
> +static inline int kvmppc_set_cap_dawr1(int enable)
> +{
> +    abort();
> +}
> +
>   static inline int kvmppc_get_cap_safe_cache(void)
>   {
>       return 0;
> 
> 
>