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

[Harsh Prateek Bora]

Typo corrected below.

On 12/20/24 15:27, Harsh Prateek Bora wrote:
> 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_DAWR1 ? 

Copy paste error corrected. Was referring to _DAWR1 here.

> > +        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;