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?
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
