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

[Shivaprasad G Bhat <sbhat@linux.ibm.com>]

Hi David, Nick,

Sorry about not getting back on this for long!

On 2/28/24 2:22 AM, David Gibson wrote:
> On Tue, Feb 27, 2024 at 10:21:23PM +1000, Nicholas Piggin wrote:
>> On Fri Feb 2, 2024 at 12:46 AM AEST, 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(-)
>>>
>>> diff --git a/hw/ppc/spapr.c b/hw/ppc/spapr.c
>>> index e8dabc8614..91a97d72e7 100644
>>> --- a/hw/ppc/spapr.c
>>> +++ b/hw/ppc/spapr.c
>>> @@ -262,7 +262,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;
>>> @@ -303,6 +303,9 @@ static void spapr_dt_pa_features(SpaprMachineState *spapr,
>>>            * in pa-features. So hide it from them. */
>>>           pa_features[40 + 2] &= ~0x80; /* Radix MMU */
>>>       }
>>> +    if (spapr_get_cap(spapr, SPAPR_CAP_DAWR1)) {
>>> +        pa_features[66] |= 0x80;
>>> +    }
>>>   
>>>       _FDT((fdt_setprop(fdt, offset, "ibm,pa-features", pa_features, pa_size)));
>>>   }
>>> @@ -2138,6 +2141,7 @@ static const VMStateDescription vmstate_spapr = {
>>>           &vmstate_spapr_cap_fwnmi,
>>>           &vmstate_spapr_fwnmi,
>>>           &vmstate_spapr_cap_rpt_invalidate,
>>> +        &vmstate_spapr_cap_dawr1,
>>>           NULL
>>>       }
>>>   };
>>> @@ -4717,6 +4721,7 @@ static void spapr_machine_class_init(ObjectClass *oc, void *data)
>>>       smc->default_caps.caps[SPAPR_CAP_CCF_ASSIST] = SPAPR_CAP_ON;
>>>       smc->default_caps.caps[SPAPR_CAP_FWNMI] = SPAPR_CAP_ON;
>>>       smc->default_caps.caps[SPAPR_CAP_RPT_INVALIDATE] = SPAPR_CAP_OFF;
>>> +    smc->default_caps.caps[SPAPR_CAP_DAWR1] = SPAPR_CAP_OFF;
>>>   
>>>       /*
>>>        * This cap specifies whether the AIL 3 mode for
>>> diff --git a/hw/ppc/spapr_caps.c b/hw/ppc/spapr_caps.c
>>> index e889244e52..677f17cea6 100644
>>> --- a/hw/ppc/spapr_caps.c
>>> +++ b/hw/ppc/spapr_caps.c
>>> @@ -655,6 +655,32 @@ static void cap_ail_mode_3_apply(SpaprMachineState *spapr,
>>>       }
>>>   }
>>>   
>>> +static void cap_dawr1_apply(SpaprMachineState *spapr, uint8_t val,
>>> +                               Error **errp)
>>> +{
>>> +    ERRP_GUARD();
>>> +
>>> +    if (!val) {
>>> +        return; /* Disable by default */
>>> +    }
>>> +
>>> +    if (!ppc_type_check_compat(MACHINE(spapr)->cpu_type,
>>> +                               CPU_POWERPC_LOGICAL_3_10, 0,
>>> +                               spapr->max_compat_pvr)) {
>>> +        warn_report("DAWR1 supported only on POWER10 and later CPUs");
>>> +    }
>> Should this be an error?

Changing this to error_report().

> Yes, it should.  If you can't supply the cap requested, you *must*
> fail to start.  Near enough is not good enough when it comes to the
> guest visible properties of the virtual machine, or you'll end up with
> no end of migration headaches.
This was made to warn_report() as suggested[1] by Greg to avoid "make test"

failures once we enable dawr1 caps ON by default with POWER9 being the 
default

cpu then.


However, now that we have moved to P10 as the default CPU, I tested with 
default

ON on P10, and OFF otherwise, "make test" passes. The problem was only 
applicable

before as the default cpu was P9 back then.

>> Should the dawr1 cap be enabled by default for POWER10 machines?

Yes. Made it default enabled on Power10 cpu, and disable for Power9 and 
below.

>>> +
>>> +    if (kvm_enabled()) {
>>> +        if (!kvmppc_has_cap_dawr1()) {
>>> +            error_setg(errp, "DAWR1 not supported by KVM.");
>>> +            error_append_hint(errp, "Try appending -machine cap-dawr1=off");
>>> +        } else if (kvmppc_set_cap_dawr1(val) < 0) {
>>> +            error_setg(errp, "Error enabling cap-dawr1 with KVM.");
>>> +            error_append_hint(errp, "Try appending -machine cap-dawr1=off");
>>> +        }
>>> +    }
>>> +}
>>> +
<snip>
>>> 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)
>> Did the text alignment go wrong here?
Yes. Fixed that.
>> Aside from those things,
>>
>> Reviewed-by: Nicholas Piggin<npiggin@gmail.com>

Since its been a while, I am not sure if the Reviewed-by is valid anymore.


I am keeping it anyway, please let me know if you have any further

comments in the v9 posted here [2]


I have addressed the comments from Harsh as well in the v9.


References:

[1] - https://lore.kernel.org/qemu-devel/20230708082536.73a2bfd8@bahia/

[2] - 
https://lore.kernel.org/all/173708679976.1678.10844458987521427074.stgit@linux.ibm.com/

Thanks,

Shiva