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@xxxxxxxxxxxxx>
> > Signed-off-by: Shivaprasad G Bhat <sbhat@xxxxxxxxxxxxx>
> > ---
> > 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?
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.
> Should the dawr1 cap be enabled by default for POWER10 machines?
>
> > +
> > + 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");
> > + }
> > + }
> > +}
> > +
> > SpaprCapabilityInfo capability_table[SPAPR_CAP_NUM] = {
> > [SPAPR_CAP_HTM] = {
> > .name = "htm",
> > @@ -781,6 +807,15 @@ SpaprCapabilityInfo capability_table[SPAPR_CAP_NUM] = {
> > .type = "bool",
> > .apply = cap_ail_mode_3_apply,
> > },
> > + [SPAPR_CAP_DAWR1] = {
> > + .name = "dawr1",
> > + .description = "Allow 2nd Data Address Watchpoint Register (DAWR1)",
> > + .index = SPAPR_CAP_DAWR1,
> > + .get = spapr_cap_get_bool,
> > + .set = spapr_cap_set_bool,
> > + .type = "bool",
> > + .apply = cap_dawr1_apply,
> > + },
> > };
> >
> > static SpaprCapabilities default_caps_with_cpu(SpaprMachineState *spapr,
> > @@ -923,6 +958,7 @@ SPAPR_CAP_MIG_STATE(large_decr, SPAPR_CAP_LARGE_DECREMENTER);
> > SPAPR_CAP_MIG_STATE(ccf_assist, SPAPR_CAP_CCF_ASSIST);
> > SPAPR_CAP_MIG_STATE(fwnmi, SPAPR_CAP_FWNMI);
> > SPAPR_CAP_MIG_STATE(rpt_invalidate, SPAPR_CAP_RPT_INVALIDATE);
> > +SPAPR_CAP_MIG_STATE(dawr1, SPAPR_CAP_DAWR1);
> >
> > void spapr_caps_init(SpaprMachineState *spapr)
> > {
> > 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?
>
> Aside from those things,
>
> Reviewed-by: Nicholas Piggin <npiggin@xxxxxxxxx>
>
> Thanks,
> Nick
>
--
David Gibson | I'll have my music baroque, and my code
david AT gibson.dropbear.id.au | minimalist, thank you. NOT _the_ _other_
| _way_ _around_!
http://www.ozlabs.org/~dgibson
Attachment:
signature.asc
Description: PGP signature
