Re: [PATCH v3 3/3] ppc: Enable 2nd DAWR support on p10

[Ravi Bangoria]

On 3/31/21 5:06 AM, David Gibson wrote:
> On Tue, Mar 30, 2021 at 06:48:38PM +0200, Greg Kurz wrote:
> > On Tue, 30 Mar 2021 15:23:50 +0530
> > Ravi Bangoria <ravi.bangoria@linux.ibm.com> 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. Though, watchpoint on powerpc TCG guest is not supported
> > > and thus 2nd DAWR is not enabled for TCG mode.
> > >
> > > Signed-off-by: Ravi Bangoria <ravi.bangoria@linux.ibm.com>
> > > ---
> >
> > LGTM. A couple of remarks, see below.
> >
> > >   hw/ppc/spapr.c                  | 11 ++++++++++-
> > >   hw/ppc/spapr_caps.c             | 32 ++++++++++++++++++++++++++++++++
> > >   include/hw/ppc/spapr.h          |  6 +++++-
> > >   target/ppc/cpu.h                |  2 ++
> > >   target/ppc/kvm.c                | 12 ++++++++++++
> > >   target/ppc/kvm_ppc.h            |  7 +++++++
> > >   target/ppc/translate_init.c.inc | 15 +++++++++++++++
> > >   7 files changed, 83 insertions(+), 2 deletions(-)
> > >
> > > diff --git a/hw/ppc/spapr.c b/hw/ppc/spapr.c
> > > index d56418ca29..4660ff9e6b 100644
> > > --- a/hw/ppc/spapr.c
> > > +++ b/hw/ppc/spapr.c
> > > @@ -238,7 +238,7 @@ static void spapr_dt_pa_features(SpaprMachineState *spapr,
> > >           0x80, 0x00, 0x80, 0x00, 0x80, 0x00, /* 48 - 53 */
> > >           /* 54: DecFP, 56: DecI, 58: SHA */
> > >           0x80, 0x00, 0x80, 0x00, 0x80, 0x00, /* 54 - 59 */
> > > -        /* 60: NM atomic, 62: RNG */
> > > +        /* 60: NM atomic, 62: RNG, 64: DAWR1 (ISA 3.1) */
> > >           0x80, 0x00, 0x80, 0x00, 0x00, 0x00, /* 60 - 65 */
> > >       };
> > >       uint8_t *pa_features = NULL;
> > > @@ -256,6 +256,10 @@ static void spapr_dt_pa_features(SpaprMachineState *spapr,
> > >           pa_features = pa_features_300;
> > >           pa_size = sizeof(pa_features_300);
> > >       }
> > > +    if (ppc_check_compat(cpu, CPU_POWERPC_LOGICAL_3_10, 0, cpu->compat_pvr)) {
> > > +        pa_features = pa_features_300;
> > > +        pa_size = sizeof(pa_features_300);
> > > +    }
> >
> > This isn't strictly needed right now because a POWER10 processor has
> > PCR_COMPAT_3_00, so the previous ppc_check_compat() block sets
> > pa_features to pa_features300 already. I guess this will make sense
> > when/if POWER10 has its own pa_features_310 one day.
>
> This should be removed for now.  We're definitely too late for
> qemu-6.0 at this point, so might as well polish this.
>
> The rest of Greg's comments look like they're good, too.

Sure. Will respin with these changes.

Thanks for the review,
Ravi