Re: [PATCH v3 3/3] ppc: Enable 2nd DAWR support on p10
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 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
---
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
Re: [PATCH v3 3/3] ppc: Enable 2nd DAWR support on p10
On Tue, Mar 30, 2021 at 06:48:38PM +0200, Greg Kurz wrote:
> On Tue, 30 Mar 2021 15:23:50 +0530
> Ravi Bangoria 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
> > ---
>
> 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.
>
> > if (!pa_features) {
> > return;
> > }
> > @@ -279,6 +283,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)));
> > }
> > @@ -2003,6 +2010,7 @@ static const VMStateDescription vmstate_spapr = {
> > _spapr_cap_ccf_assist,
> > _spapr_cap_fwnmi,
> > _spapr_fwnmi,
> > +_spapr_cap_dawr1,
> > NULL
> > }
> > };
> > @@ -4539,6 +4547,7 @@ static void spapr_machine_class_init(ObjectClass *oc,
> > void *data)
> > smc->default_caps.caps[SPAPR_CAP_LARGE_DECREMENTER] = SPAPR_CAP_ON;
> > 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_DAWR1] = SPAPR_CAP_OFF;
> > spapr_caps_add_properties(smc);
> > smc->irq = _irq_dual;
> > smc->dr_phb_enabled = true;
> > diff --git a/hw/ppc/spapr_caps.c b/hw/ppc/spapr_caps.c
> > index 9ea7ddd1e9..9c39a211fd 100644
> > --- a/hw/ppc/spapr_caps.c
> > +++ b/hw/ppc/spapr_caps.c
> > @@ -523,6 +523,27 @@ static void cap_fwnmi_apply(SpaprMachineState *spapr,
> > uint8_t val,
> > }
> > }
> >
> > +static void cap_dawr1_apply(SpaprMachineState *spapr, uint8_t val,
> > + Error **errp)
> > +{
> > +if (!val) {
> > +return; /* Disable by default */
> > +}
> > +
> > +if (tcg_enabled()) {
> > +error_setg(errp,
> > +"DAWR1 not supported in TCG. Try appending -machine
> > cap-dawr1=off");
>
> Hints are best added with error_append_hint() because we don't want them
> in QMP. Note that you'll need to use the ERRP_GUARD() macro.
>
> See cap_htm_apply() for an example.
>
> > +} else if (kvm_enabled()) {
> > +if (!kvmppc_has_cap_dawr1()) {
> > +error_setg(errp,
> > +"DAWR1 not supported by KVM. Try appending -machine
> > cap-dawr1=off");
> > +} else if (kvmppc_set_cap_dawr1(val) < 0) {
> > +error_setg(errp,
> > +"DAWR1 not supported by KVM. Try appending
Re: [PATCH v3 3/3] ppc: Enable 2nd DAWR support on p10
On Tue, 30 Mar 2021 15:23:50 +0530
Ravi Bangoria 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
> ---
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.
> if (!pa_features) {
> return;
> }
> @@ -279,6 +283,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)));
> }
> @@ -2003,6 +2010,7 @@ static const VMStateDescription vmstate_spapr = {
> _spapr_cap_ccf_assist,
> _spapr_cap_fwnmi,
> _spapr_fwnmi,
> +_spapr_cap_dawr1,
> NULL
> }
> };
> @@ -4539,6 +4547,7 @@ static void spapr_machine_class_init(ObjectClass *oc,
> void *data)
> smc->default_caps.caps[SPAPR_CAP_LARGE_DECREMENTER] = SPAPR_CAP_ON;
> 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_DAWR1] = SPAPR_CAP_OFF;
> spapr_caps_add_properties(smc);
> smc->irq = _irq_dual;
> smc->dr_phb_enabled = true;
> diff --git a/hw/ppc/spapr_caps.c b/hw/ppc/spapr_caps.c
> index 9ea7ddd1e9..9c39a211fd 100644
> --- a/hw/ppc/spapr_caps.c
> +++ b/hw/ppc/spapr_caps.c
> @@ -523,6 +523,27 @@ static void cap_fwnmi_apply(SpaprMachineState *spapr,
> uint8_t val,
> }
> }
>
> +static void cap_dawr1_apply(SpaprMachineState *spapr, uint8_t val,
> + Error **errp)
> +{
> +if (!val) {
> +return; /* Disable by default */
> +}
> +
> +if (tcg_enabled()) {
> +error_setg(errp,
> +"DAWR1 not supported in TCG. Try appending -machine
> cap-dawr1=off");
Hints are best added with error_append_hint() because we don't want them
in QMP. Note that you'll need to use the ERRP_GUARD() macro.
See cap_htm_apply() for an example.
> +} else if (kvm_enabled()) {
> +if (!kvmppc_has_cap_dawr1()) {
> +error_setg(errp,
> +"DAWR1 not supported by KVM. Try appending -machine
> cap-dawr1=off");
> +} else if (kvmppc_set_cap_dawr1(val) < 0) {
> +error_setg(errp,
> +"DAWR1 not supported by KVM. Try appending -machine
> cap-dawr1=off");
> +}
> +}
> +}
> +
> SpaprCapabilityInfo capability_table[SPAPR_CAP_NUM] = {
> [SPAPR_CAP_HTM] = {
> .name = "htm",
> @@ -631,6 +652,16 @@ SpaprCapabilityInfo capability_table[SPAPR_CAP_NUM] = {
> .type = "bool",
> .apply = cap_fwnmi_apply,
> },
> +[SPAPR_CAP_DAWR1] = {
> +.name = "dawr1",
> +.description = "Allow DAWR1",
Maybe expand to "Allow 2nd Data Address Watchpoint Register (DAWR1)"
[PATCH v3 3/3] ppc: Enable 2nd DAWR support on p10
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
---
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);
+}
if (!pa_features) {
return;
}
@@ -279,6 +283,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)));
}
@@ -2003,6 +2010,7 @@ static const VMStateDescription vmstate_spapr = {
_spapr_cap_ccf_assist,
_spapr_cap_fwnmi,
_spapr_fwnmi,
+_spapr_cap_dawr1,
NULL
}
};
@@ -4539,6 +4547,7 @@ static void spapr_machine_class_init(ObjectClass *oc,
void *data)
smc->default_caps.caps[SPAPR_CAP_LARGE_DECREMENTER] = SPAPR_CAP_ON;
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_DAWR1] = SPAPR_CAP_OFF;
spapr_caps_add_properties(smc);
smc->irq = _irq_dual;
smc->dr_phb_enabled = true;
diff --git a/hw/ppc/spapr_caps.c b/hw/ppc/spapr_caps.c
index 9ea7ddd1e9..9c39a211fd 100644
--- a/hw/ppc/spapr_caps.c
+++ b/hw/ppc/spapr_caps.c
@@ -523,6 +523,27 @@ static void cap_fwnmi_apply(SpaprMachineState *spapr,
uint8_t val,
}
}
+static void cap_dawr1_apply(SpaprMachineState *spapr, uint8_t val,
+ Error **errp)
+{
+if (!val) {
+return; /* Disable by default */
+}
+
+if (tcg_enabled()) {
+error_setg(errp,
+"DAWR1 not supported in TCG. Try appending -machine
cap-dawr1=off");
+} else if (kvm_enabled()) {
+if (!kvmppc_has_cap_dawr1()) {
+error_setg(errp,
+"DAWR1 not supported by KVM. Try appending -machine
cap-dawr1=off");
+} else if (kvmppc_set_cap_dawr1(val) < 0) {
+error_setg(errp,
+"DAWR1 not supported by KVM. Try appending -machine
cap-dawr1=off");
+}
+}
+}
+
SpaprCapabilityInfo capability_table[SPAPR_CAP_NUM] = {
[SPAPR_CAP_HTM] = {
.name = "htm",
@@ -631,6 +652,16 @@ SpaprCapabilityInfo capability_table[SPAPR_CAP_NUM] = {
.type = "bool",
.apply = cap_fwnmi_apply,
},
+[SPAPR_CAP_DAWR1] = {
+.name = "dawr1",
+.description = "Allow 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,
@@ -771,6 +802,7 @@ SPAPR_CAP_MIG_STATE(nested_kvm_hv, SPAPR_CAP_NESTED_KVM_HV);
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(dawr1, SPAPR_CAP_DAWR1);
void spapr_caps_init(SpaprMachineState *spapr)
{
diff --git a/include/hw/ppc/spapr.h b/include/hw/ppc/spapr.h
index b8985fab5b..00c8341acf 100644
--- a/include/hw/ppc/spapr.h
+++ b/include/hw/ppc/spapr.h
@@ -74,8 +74,10 @@ typedef enum {
#define SPAPR_CAP_CCF_ASSIST
