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

[Ravi Bangoria <ravi.bangoria@linux.ibm.com>]

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