* [PATCH] spapr: Enable DD2.3 accelerated count cache flush in pseries-5.0 machine
@ 2020-01-30 1:26 David Gibson
2020-01-30 8:09 ` Laurent Vivier
` (2 more replies)
0 siblings, 3 replies; 6+ messages in thread
From: David Gibson @ 2020-01-30 1:26 UTC (permalink / raw)
To: mpe, groug, clg; +Cc: lvivier, aik, qemu-devel, paulus, qemu-ppc, David Gibson
For POWER9 DD2.2 cpus, the best current Spectre v2 indirect branch
mitigation is "count cache disabled", which is configured with:
-machine cap-ibs=fixed-ccd
However, this option isn't available on DD2.3 CPUs with KVM, because they
don't have the count cache disabled.
For POWER9 DD2.3 cpus, it is "count cache flush with assist", configured
with:
-machine cap-ibs=workaround,cap-ccf-assist=on
However this option isn't available on DD2.2 CPUs with KVM, because they
don't have the special CCF assist instruction this relies on.
On current machine types, we default to "count cache flush w/o assist",
that is:
-machine cap-ibs=workaround,cap-ccf-assist=off
This runs, with mitigation on both DD2.2 and DD2.3 host cpus, but has a
fairly significant performance impact.
It turns out we can do better. The special instruction that CCF assist
uses to trigger a count cache flush is a no-op on earlier CPUs, rather than
trapping or causing other badness. It doesn't, of itself, implement the
mitigation, but *if* we have count-cache-disabled, then the count cache
flush is unnecessary, and so using the count cache flush mitigation is
harmless.
Therefore for the new pseries-5.0 machine type, enable cap-ccf-assist by
default. Along with that, suppress throwing an error if cap-ccf-assist
is selected but KVM doesn't support it, as long as KVM *is* giving us
count-cache-disabled. To allow TCG to work out of the box, even though it
doesn't implement the ccf flush assist, downgrade the error in that case to
a warning. This matches several Spectre mitigations where we allow TCG
to operate for debugging, since we don't really make guarantees about TCG
security properties anyway.
While we're there, make the TCG warning for this case match that for other
mitigations.
Signed-off-by: David Gibson <david@gibson.dropbear.id.au>
---
hw/ppc/spapr.c | 5 ++++-
hw/ppc/spapr_caps.c | 26 ++++++++++++++++++++++----
2 files changed, 26 insertions(+), 5 deletions(-)
I have put this into my ppc-for-5.0 tree already, and hope to send a
pull request tomorrow (Jan 31).
diff --git a/hw/ppc/spapr.c b/hw/ppc/spapr.c
index 02cf53fc5b..deaa44f1ab 100644
--- a/hw/ppc/spapr.c
+++ b/hw/ppc/spapr.c
@@ -4397,7 +4397,7 @@ static void spapr_machine_class_init(ObjectClass *oc, void *data)
smc->default_caps.caps[SPAPR_CAP_HPT_MAXPAGESIZE] = 16; /* 64kiB */
smc->default_caps.caps[SPAPR_CAP_NESTED_KVM_HV] = SPAPR_CAP_OFF;
smc->default_caps.caps[SPAPR_CAP_LARGE_DECREMENTER] = SPAPR_CAP_ON;
- smc->default_caps.caps[SPAPR_CAP_CCF_ASSIST] = SPAPR_CAP_OFF;
+ smc->default_caps.caps[SPAPR_CAP_CCF_ASSIST] = SPAPR_CAP_ON;
spapr_caps_add_properties(smc, &error_abort);
smc->irq = &spapr_irq_dual;
smc->dr_phb_enabled = true;
@@ -4465,8 +4465,11 @@ DEFINE_SPAPR_MACHINE(5_0, "5.0", true);
*/
static void spapr_machine_4_2_class_options(MachineClass *mc)
{
+ SpaprMachineClass *smc = SPAPR_MACHINE_CLASS(mc);
+
spapr_machine_5_0_class_options(mc);
compat_props_add(mc->compat_props, hw_compat_4_2, hw_compat_4_2_len);
+ smc->default_caps.caps[SPAPR_CAP_CCF_ASSIST] = SPAPR_CAP_OFF;
}
DEFINE_SPAPR_MACHINE(4_2, "4.2", false);
diff --git a/hw/ppc/spapr_caps.c b/hw/ppc/spapr_caps.c
index 481dfd2a27..d0d4b32a40 100644
--- a/hw/ppc/spapr_caps.c
+++ b/hw/ppc/spapr_caps.c
@@ -482,18 +482,36 @@ static void cap_large_decr_cpu_apply(SpaprMachineState *spapr,
static void cap_ccf_assist_apply(SpaprMachineState *spapr, uint8_t val,
Error **errp)
{
+ Error *local_err = NULL;
uint8_t kvm_val = kvmppc_get_cap_count_cache_flush_assist();
if (tcg_enabled() && val) {
- /* TODO - for now only allow broken for TCG */
- error_setg(errp,
-"Requested count cache flush assist capability level not supported by tcg,"
- " try appending -machine cap-ccf-assist=off");
+ /* TCG doesn't implement anything here, but allow with a warning */
+ error_setg(&local_err,
+ "TCG doesn't support requested feature, cap-ccf-assist=on");
} else if (kvm_enabled() && (val > kvm_val)) {
+ uint8_t kvm_ibs = kvmppc_get_cap_safe_indirect_branch();
+
+ if (kvm_ibs == SPAPR_CAP_FIXED_CCD) {
+ /*
+ * If we don't have CCF assist on the host, the assist
+ * instruction is a harmless no-op. It won't correctly
+ * implement the cache count flush *but* if we have
+ * count-cache-disabled in the host, that flush is
+ * unnnecessary. So, specifically allow this case. This
+ * allows us to have better performance on POWER9 DD2.3,
+ * while still working on POWER9 DD2.2 and POWER8 host
+ * cpus.
+ */
+ return;
+ }
error_setg(errp,
"Requested count cache flush assist capability level not supported by kvm,"
" try appending -machine cap-ccf-assist=off");
}
+
+ if (local_err != NULL)
+ warn_report_err(local_err);
}
SpaprCapabilityInfo capability_table[SPAPR_CAP_NUM] = {
--
2.24.1
^ permalink raw reply related [flat|nested] 6+ messages in thread
* Re: [PATCH] spapr: Enable DD2.3 accelerated count cache flush in pseries-5.0 machine
2020-01-30 1:26 [PATCH] spapr: Enable DD2.3 accelerated count cache flush in pseries-5.0 machine David Gibson
@ 2020-01-30 8:09 ` Laurent Vivier
2020-01-30 8:54 ` Greg Kurz
2020-01-30 8:48 ` Greg Kurz
2020-01-31 10:28 ` Michael Ellerman
2 siblings, 1 reply; 6+ messages in thread
From: Laurent Vivier @ 2020-01-30 8:09 UTC (permalink / raw)
To: David Gibson, mpe, groug, clg; +Cc: aik, paulus, qemu-ppc, qemu-devel
On 30/01/2020 02:26, David Gibson wrote:
> For POWER9 DD2.2 cpus, the best current Spectre v2 indirect branch
> mitigation is "count cache disabled", which is configured with:
> -machine cap-ibs=fixed-ccd
> However, this option isn't available on DD2.3 CPUs with KVM, because they
> don't have the count cache disabled.
>
> For POWER9 DD2.3 cpus, it is "count cache flush with assist", configured
> with:
> -machine cap-ibs=workaround,cap-ccf-assist=on
> However this option isn't available on DD2.2 CPUs with KVM, because they
> don't have the special CCF assist instruction this relies on.
>
> On current machine types, we default to "count cache flush w/o assist",
> that is:
> -machine cap-ibs=workaround,cap-ccf-assist=off
> This runs, with mitigation on both DD2.2 and DD2.3 host cpus, but has a
> fairly significant performance impact.
>
> It turns out we can do better. The special instruction that CCF assist
> uses to trigger a count cache flush is a no-op on earlier CPUs, rather than
> trapping or causing other badness. It doesn't, of itself, implement the
> mitigation, but *if* we have count-cache-disabled, then the count cache
> flush is unnecessary, and so using the count cache flush mitigation is
> harmless.
>
> Therefore for the new pseries-5.0 machine type, enable cap-ccf-assist by
> default. Along with that, suppress throwing an error if cap-ccf-assist
> is selected but KVM doesn't support it, as long as KVM *is* giving us
> count-cache-disabled. To allow TCG to work out of the box, even though it
> doesn't implement the ccf flush assist, downgrade the error in that case to
> a warning. This matches several Spectre mitigations where we allow TCG
> to operate for debugging, since we don't really make guarantees about TCG
> security properties anyway.
>
> While we're there, make the TCG warning for this case match that for other
> mitigations.
>
> Signed-off-by: David Gibson <david@gibson.dropbear.id.au>
> ---
> hw/ppc/spapr.c | 5 ++++-
> hw/ppc/spapr_caps.c | 26 ++++++++++++++++++++++----
> 2 files changed, 26 insertions(+), 5 deletions(-)
>
> I have put this into my ppc-for-5.0 tree already, and hope to send a
> pull request tomorrow (Jan 31).
>
> diff --git a/hw/ppc/spapr.c b/hw/ppc/spapr.c
> index 02cf53fc5b..deaa44f1ab 100644
> --- a/hw/ppc/spapr.c
> +++ b/hw/ppc/spapr.c
> @@ -4397,7 +4397,7 @@ static void spapr_machine_class_init(ObjectClass *oc, void *data)
> smc->default_caps.caps[SPAPR_CAP_HPT_MAXPAGESIZE] = 16; /* 64kiB */
> smc->default_caps.caps[SPAPR_CAP_NESTED_KVM_HV] = SPAPR_CAP_OFF;
> smc->default_caps.caps[SPAPR_CAP_LARGE_DECREMENTER] = SPAPR_CAP_ON;
> - smc->default_caps.caps[SPAPR_CAP_CCF_ASSIST] = SPAPR_CAP_OFF;
> + smc->default_caps.caps[SPAPR_CAP_CCF_ASSIST] = SPAPR_CAP_ON;
> spapr_caps_add_properties(smc, &error_abort);
> smc->irq = &spapr_irq_dual;
> smc->dr_phb_enabled = true;
> @@ -4465,8 +4465,11 @@ DEFINE_SPAPR_MACHINE(5_0, "5.0", true);
> */
> static void spapr_machine_4_2_class_options(MachineClass *mc)
> {
> + SpaprMachineClass *smc = SPAPR_MACHINE_CLASS(mc);
> +
> spapr_machine_5_0_class_options(mc);
> compat_props_add(mc->compat_props, hw_compat_4_2, hw_compat_4_2_len);
> + smc->default_caps.caps[SPAPR_CAP_CCF_ASSIST] = SPAPR_CAP_OFF;
> }
>
> DEFINE_SPAPR_MACHINE(4_2, "4.2", false);
> diff --git a/hw/ppc/spapr_caps.c b/hw/ppc/spapr_caps.c
> index 481dfd2a27..d0d4b32a40 100644
> --- a/hw/ppc/spapr_caps.c
> +++ b/hw/ppc/spapr_caps.c
> @@ -482,18 +482,36 @@ static void cap_large_decr_cpu_apply(SpaprMachineState *spapr,
> static void cap_ccf_assist_apply(SpaprMachineState *spapr, uint8_t val,
> Error **errp)
> {
> + Error *local_err = NULL;
> uint8_t kvm_val = kvmppc_get_cap_count_cache_flush_assist();
>
> if (tcg_enabled() && val) {
> - /* TODO - for now only allow broken for TCG */
> - error_setg(errp,
> -"Requested count cache flush assist capability level not supported by tcg,"
> - " try appending -machine cap-ccf-assist=off");
> + /* TCG doesn't implement anything here, but allow with a warning */
> + error_setg(&local_err,
> + "TCG doesn't support requested feature, cap-ccf-assist=on");
> } else if (kvm_enabled() && (val > kvm_val)) {
> + uint8_t kvm_ibs = kvmppc_get_cap_safe_indirect_branch();
> +
> + if (kvm_ibs == SPAPR_CAP_FIXED_CCD) {
> + /*
> + * If we don't have CCF assist on the host, the assist
> + * instruction is a harmless no-op. It won't correctly
> + * implement the cache count flush *but* if we have
> + * count-cache-disabled in the host, that flush is
> + * unnnecessary. So, specifically allow this case. This
> + * allows us to have better performance on POWER9 DD2.3,
> + * while still working on POWER9 DD2.2 and POWER8 host
> + * cpus.
> + */
> + return;
> + }
> error_setg(errp,
local_err? ...
> "Requested count cache flush assist capability level not supported by kvm,"
> " try appending -machine cap-ccf-assist=off");
> }
> +
> + if (local_err != NULL)
> + warn_report_err(local_err);
... or why don't you put warn_report_err() in the first part of the "if"
as this is the only place where it is used?
Thanks,
Laurent
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH] spapr: Enable DD2.3 accelerated count cache flush in pseries-5.0 machine
2020-01-30 1:26 [PATCH] spapr: Enable DD2.3 accelerated count cache flush in pseries-5.0 machine David Gibson
2020-01-30 8:09 ` Laurent Vivier
@ 2020-01-30 8:48 ` Greg Kurz
2020-01-30 23:47 ` David Gibson
2020-01-31 10:28 ` Michael Ellerman
2 siblings, 1 reply; 6+ messages in thread
From: Greg Kurz @ 2020-01-30 8:48 UTC (permalink / raw)
To: David Gibson; +Cc: lvivier, aik, mpe, qemu-devel, paulus, qemu-ppc, clg
On Thu, 30 Jan 2020 12:26:22 +1100
David Gibson <david@gibson.dropbear.id.au> wrote:
> For POWER9 DD2.2 cpus, the best current Spectre v2 indirect branch
> mitigation is "count cache disabled", which is configured with:
> -machine cap-ibs=fixed-ccd
> However, this option isn't available on DD2.3 CPUs with KVM, because they
> don't have the count cache disabled.
>
> For POWER9 DD2.3 cpus, it is "count cache flush with assist", configured
> with:
> -machine cap-ibs=workaround,cap-ccf-assist=on
> However this option isn't available on DD2.2 CPUs with KVM, because they
> don't have the special CCF assist instruction this relies on.
>
> On current machine types, we default to "count cache flush w/o assist",
> that is:
> -machine cap-ibs=workaround,cap-ccf-assist=off
> This runs, with mitigation on both DD2.2 and DD2.3 host cpus, but has a
> fairly significant performance impact.
>
> It turns out we can do better. The special instruction that CCF assist
> uses to trigger a count cache flush is a no-op on earlier CPUs, rather than
> trapping or causing other badness. It doesn't, of itself, implement the
> mitigation, but *if* we have count-cache-disabled, then the count cache
> flush is unnecessary, and so using the count cache flush mitigation is
> harmless.
>
> Therefore for the new pseries-5.0 machine type, enable cap-ccf-assist by
> default. Along with that, suppress throwing an error if cap-ccf-assist
> is selected but KVM doesn't support it, as long as KVM *is* giving us
> count-cache-disabled. To allow TCG to work out of the box, even though it
> doesn't implement the ccf flush assist, downgrade the error in that case to
> a warning. This matches several Spectre mitigations where we allow TCG
> to operate for debugging, since we don't really make guarantees about TCG
> security properties anyway.
>
> While we're there, make the TCG warning for this case match that for other
> mitigations.
>
> Signed-off-by: David Gibson <david@gibson.dropbear.id.au>
> ---
> hw/ppc/spapr.c | 5 ++++-
> hw/ppc/spapr_caps.c | 26 ++++++++++++++++++++++----
> 2 files changed, 26 insertions(+), 5 deletions(-)
>
> I have put this into my ppc-for-5.0 tree already, and hope to send a
> pull request tomorrow (Jan 31).
>
> diff --git a/hw/ppc/spapr.c b/hw/ppc/spapr.c
> index 02cf53fc5b..deaa44f1ab 100644
> --- a/hw/ppc/spapr.c
> +++ b/hw/ppc/spapr.c
> @@ -4397,7 +4397,7 @@ static void spapr_machine_class_init(ObjectClass *oc, void *data)
> smc->default_caps.caps[SPAPR_CAP_HPT_MAXPAGESIZE] = 16; /* 64kiB */
> smc->default_caps.caps[SPAPR_CAP_NESTED_KVM_HV] = SPAPR_CAP_OFF;
> smc->default_caps.caps[SPAPR_CAP_LARGE_DECREMENTER] = SPAPR_CAP_ON;
> - smc->default_caps.caps[SPAPR_CAP_CCF_ASSIST] = SPAPR_CAP_OFF;
> + smc->default_caps.caps[SPAPR_CAP_CCF_ASSIST] = SPAPR_CAP_ON;
> spapr_caps_add_properties(smc, &error_abort);
> smc->irq = &spapr_irq_dual;
> smc->dr_phb_enabled = true;
> @@ -4465,8 +4465,11 @@ DEFINE_SPAPR_MACHINE(5_0, "5.0", true);
> */
> static void spapr_machine_4_2_class_options(MachineClass *mc)
> {
> + SpaprMachineClass *smc = SPAPR_MACHINE_CLASS(mc);
> +
> spapr_machine_5_0_class_options(mc);
> compat_props_add(mc->compat_props, hw_compat_4_2, hw_compat_4_2_len);
> + smc->default_caps.caps[SPAPR_CAP_CCF_ASSIST] = SPAPR_CAP_OFF;
> }
>
> DEFINE_SPAPR_MACHINE(4_2, "4.2", false);
> diff --git a/hw/ppc/spapr_caps.c b/hw/ppc/spapr_caps.c
> index 481dfd2a27..d0d4b32a40 100644
> --- a/hw/ppc/spapr_caps.c
> +++ b/hw/ppc/spapr_caps.c
> @@ -482,18 +482,36 @@ static void cap_large_decr_cpu_apply(SpaprMachineState *spapr,
> static void cap_ccf_assist_apply(SpaprMachineState *spapr, uint8_t val,
> Error **errp)
> {
> + Error *local_err = NULL;
> uint8_t kvm_val = kvmppc_get_cap_count_cache_flush_assist();
>
> if (tcg_enabled() && val) {
> - /* TODO - for now only allow broken for TCG */
> - error_setg(errp,
> -"Requested count cache flush assist capability level not supported by tcg,"
> - " try appending -machine cap-ccf-assist=off");
> + /* TCG doesn't implement anything here, but allow with a warning */
> + error_setg(&local_err,
> + "TCG doesn't support requested feature, cap-ccf-assist=on");
The only user for local_err is warn_report_err() below. It would be simpler to
simply call warn_report() here.
> } else if (kvm_enabled() && (val > kvm_val)) {
> + uint8_t kvm_ibs = kvmppc_get_cap_safe_indirect_branch();
> +
> + if (kvm_ibs == SPAPR_CAP_FIXED_CCD) {
> + /*
> + * If we don't have CCF assist on the host, the assist
> + * instruction is a harmless no-op. It won't correctly
> + * implement the cache count flush *but* if we have
> + * count-cache-disabled in the host, that flush is
> + * unnnecessary. So, specifically allow this case. This
> + * allows us to have better performance on POWER9 DD2.3,
> + * while still working on POWER9 DD2.2 and POWER8 host
> + * cpus.
> + */
> + return;
> + }
> error_setg(errp,
> "Requested count cache flush assist capability level not supported by kvm,"
> " try appending -machine cap-ccf-assist=off");
> }
> +
> + if (local_err != NULL)
> + warn_report_err(local_err);
> }
>
> SpaprCapabilityInfo capability_table[SPAPR_CAP_NUM] = {
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH] spapr: Enable DD2.3 accelerated count cache flush in pseries-5.0 machine
2020-01-30 8:09 ` Laurent Vivier
@ 2020-01-30 8:54 ` Greg Kurz
0 siblings, 0 replies; 6+ messages in thread
From: Greg Kurz @ 2020-01-30 8:54 UTC (permalink / raw)
To: Laurent Vivier; +Cc: aik, mpe, qemu-devel, qemu-ppc, clg, paulus, David Gibson
On Thu, 30 Jan 2020 09:09:18 +0100
Laurent Vivier <lvivier@redhat.com> wrote:
> On 30/01/2020 02:26, David Gibson wrote:
> > For POWER9 DD2.2 cpus, the best current Spectre v2 indirect branch
> > mitigation is "count cache disabled", which is configured with:
> > -machine cap-ibs=fixed-ccd
> > However, this option isn't available on DD2.3 CPUs with KVM, because they
> > don't have the count cache disabled.
> >
> > For POWER9 DD2.3 cpus, it is "count cache flush with assist", configured
> > with:
> > -machine cap-ibs=workaround,cap-ccf-assist=on
> > However this option isn't available on DD2.2 CPUs with KVM, because they
> > don't have the special CCF assist instruction this relies on.
> >
> > On current machine types, we default to "count cache flush w/o assist",
> > that is:
> > -machine cap-ibs=workaround,cap-ccf-assist=off
> > This runs, with mitigation on both DD2.2 and DD2.3 host cpus, but has a
> > fairly significant performance impact.
> >
> > It turns out we can do better. The special instruction that CCF assist
> > uses to trigger a count cache flush is a no-op on earlier CPUs, rather than
> > trapping or causing other badness. It doesn't, of itself, implement the
> > mitigation, but *if* we have count-cache-disabled, then the count cache
> > flush is unnecessary, and so using the count cache flush mitigation is
> > harmless.
> >
> > Therefore for the new pseries-5.0 machine type, enable cap-ccf-assist by
> > default. Along with that, suppress throwing an error if cap-ccf-assist
> > is selected but KVM doesn't support it, as long as KVM *is* giving us
> > count-cache-disabled. To allow TCG to work out of the box, even though it
> > doesn't implement the ccf flush assist, downgrade the error in that case to
> > a warning. This matches several Spectre mitigations where we allow TCG
> > to operate for debugging, since we don't really make guarantees about TCG
> > security properties anyway.
> >
> > While we're there, make the TCG warning for this case match that for other
> > mitigations.
> >
> > Signed-off-by: David Gibson <david@gibson.dropbear.id.au>
> > ---
> > hw/ppc/spapr.c | 5 ++++-
> > hw/ppc/spapr_caps.c | 26 ++++++++++++++++++++++----
> > 2 files changed, 26 insertions(+), 5 deletions(-)
> >
> > I have put this into my ppc-for-5.0 tree already, and hope to send a
> > pull request tomorrow (Jan 31).
> >
> > diff --git a/hw/ppc/spapr.c b/hw/ppc/spapr.c
> > index 02cf53fc5b..deaa44f1ab 100644
> > --- a/hw/ppc/spapr.c
> > +++ b/hw/ppc/spapr.c
> > @@ -4397,7 +4397,7 @@ static void spapr_machine_class_init(ObjectClass *oc, void *data)
> > smc->default_caps.caps[SPAPR_CAP_HPT_MAXPAGESIZE] = 16; /* 64kiB */
> > smc->default_caps.caps[SPAPR_CAP_NESTED_KVM_HV] = SPAPR_CAP_OFF;
> > smc->default_caps.caps[SPAPR_CAP_LARGE_DECREMENTER] = SPAPR_CAP_ON;
> > - smc->default_caps.caps[SPAPR_CAP_CCF_ASSIST] = SPAPR_CAP_OFF;
> > + smc->default_caps.caps[SPAPR_CAP_CCF_ASSIST] = SPAPR_CAP_ON;
> > spapr_caps_add_properties(smc, &error_abort);
> > smc->irq = &spapr_irq_dual;
> > smc->dr_phb_enabled = true;
> > @@ -4465,8 +4465,11 @@ DEFINE_SPAPR_MACHINE(5_0, "5.0", true);
> > */
> > static void spapr_machine_4_2_class_options(MachineClass *mc)
> > {
> > + SpaprMachineClass *smc = SPAPR_MACHINE_CLASS(mc);
> > +
> > spapr_machine_5_0_class_options(mc);
> > compat_props_add(mc->compat_props, hw_compat_4_2, hw_compat_4_2_len);
> > + smc->default_caps.caps[SPAPR_CAP_CCF_ASSIST] = SPAPR_CAP_OFF;
> > }
> >
> > DEFINE_SPAPR_MACHINE(4_2, "4.2", false);
> > diff --git a/hw/ppc/spapr_caps.c b/hw/ppc/spapr_caps.c
> > index 481dfd2a27..d0d4b32a40 100644
> > --- a/hw/ppc/spapr_caps.c
> > +++ b/hw/ppc/spapr_caps.c
> > @@ -482,18 +482,36 @@ static void cap_large_decr_cpu_apply(SpaprMachineState *spapr,
> > static void cap_ccf_assist_apply(SpaprMachineState *spapr, uint8_t val,
> > Error **errp)
> > {
> > + Error *local_err = NULL;
> > uint8_t kvm_val = kvmppc_get_cap_count_cache_flush_assist();
> >
> > if (tcg_enabled() && val) {
> > - /* TODO - for now only allow broken for TCG */
> > - error_setg(errp,
> > -"Requested count cache flush assist capability level not supported by tcg,"
> > - " try appending -machine cap-ccf-assist=off");
> > + /* TCG doesn't implement anything here, but allow with a warning */
> > + error_setg(&local_err,
> > + "TCG doesn't support requested feature, cap-ccf-assist=on");
> > } else if (kvm_enabled() && (val > kvm_val)) {
> > + uint8_t kvm_ibs = kvmppc_get_cap_safe_indirect_branch();
> > +
> > + if (kvm_ibs == SPAPR_CAP_FIXED_CCD) {
> > + /*
> > + * If we don't have CCF assist on the host, the assist
> > + * instruction is a harmless no-op. It won't correctly
> > + * implement the cache count flush *but* if we have
> > + * count-cache-disabled in the host, that flush is
> > + * unnnecessary. So, specifically allow this case. This
> > + * allows us to have better performance on POWER9 DD2.3,
> > + * while still working on POWER9 DD2.2 and POWER8 host
> > + * cpus.
> > + */
> > + return;
> > + }
> > error_setg(errp,
>
> local_err? ...
>
No. This is an error we do want to propagate to the caller, certainly
not turning it into a warning.
> > "Requested count cache flush assist capability level not supported by kvm,"
> > " try appending -machine cap-ccf-assist=off");
> > }
> > +
> > + if (local_err != NULL)
> > + warn_report_err(local_err);
>
> ... or why don't you put warn_report_err() in the first part of the "if"
> as this is the only place where it is used?
>
Yes and there you see that local_err isn't even needed in the first place.
We should simply call warn_report().
> Thanks,
> Laurent
>
>
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH] spapr: Enable DD2.3 accelerated count cache flush in pseries-5.0 machine
2020-01-30 8:48 ` Greg Kurz
@ 2020-01-30 23:47 ` David Gibson
0 siblings, 0 replies; 6+ messages in thread
From: David Gibson @ 2020-01-30 23:47 UTC (permalink / raw)
To: Greg Kurz; +Cc: lvivier, aik, mpe, qemu-devel, paulus, qemu-ppc, clg
[-- Attachment #1: Type: text/plain, Size: 6360 bytes --]
On Thu, Jan 30, 2020 at 09:48:49AM +0100, Greg Kurz wrote:
> On Thu, 30 Jan 2020 12:26:22 +1100
> David Gibson <david@gibson.dropbear.id.au> wrote:
>
> > For POWER9 DD2.2 cpus, the best current Spectre v2 indirect branch
> > mitigation is "count cache disabled", which is configured with:
> > -machine cap-ibs=fixed-ccd
> > However, this option isn't available on DD2.3 CPUs with KVM, because they
> > don't have the count cache disabled.
> >
> > For POWER9 DD2.3 cpus, it is "count cache flush with assist", configured
> > with:
> > -machine cap-ibs=workaround,cap-ccf-assist=on
> > However this option isn't available on DD2.2 CPUs with KVM, because they
> > don't have the special CCF assist instruction this relies on.
> >
> > On current machine types, we default to "count cache flush w/o assist",
> > that is:
> > -machine cap-ibs=workaround,cap-ccf-assist=off
> > This runs, with mitigation on both DD2.2 and DD2.3 host cpus, but has a
> > fairly significant performance impact.
> >
> > It turns out we can do better. The special instruction that CCF assist
> > uses to trigger a count cache flush is a no-op on earlier CPUs, rather than
> > trapping or causing other badness. It doesn't, of itself, implement the
> > mitigation, but *if* we have count-cache-disabled, then the count cache
> > flush is unnecessary, and so using the count cache flush mitigation is
> > harmless.
> >
> > Therefore for the new pseries-5.0 machine type, enable cap-ccf-assist by
> > default. Along with that, suppress throwing an error if cap-ccf-assist
> > is selected but KVM doesn't support it, as long as KVM *is* giving us
> > count-cache-disabled. To allow TCG to work out of the box, even though it
> > doesn't implement the ccf flush assist, downgrade the error in that case to
> > a warning. This matches several Spectre mitigations where we allow TCG
> > to operate for debugging, since we don't really make guarantees about TCG
> > security properties anyway.
> >
> > While we're there, make the TCG warning for this case match that for other
> > mitigations.
> >
> > Signed-off-by: David Gibson <david@gibson.dropbear.id.au>
> > ---
> > hw/ppc/spapr.c | 5 ++++-
> > hw/ppc/spapr_caps.c | 26 ++++++++++++++++++++++----
> > 2 files changed, 26 insertions(+), 5 deletions(-)
> >
> > I have put this into my ppc-for-5.0 tree already, and hope to send a
> > pull request tomorrow (Jan 31).
> >
> > diff --git a/hw/ppc/spapr.c b/hw/ppc/spapr.c
> > index 02cf53fc5b..deaa44f1ab 100644
> > --- a/hw/ppc/spapr.c
> > +++ b/hw/ppc/spapr.c
> > @@ -4397,7 +4397,7 @@ static void spapr_machine_class_init(ObjectClass *oc, void *data)
> > smc->default_caps.caps[SPAPR_CAP_HPT_MAXPAGESIZE] = 16; /* 64kiB */
> > smc->default_caps.caps[SPAPR_CAP_NESTED_KVM_HV] = SPAPR_CAP_OFF;
> > smc->default_caps.caps[SPAPR_CAP_LARGE_DECREMENTER] = SPAPR_CAP_ON;
> > - smc->default_caps.caps[SPAPR_CAP_CCF_ASSIST] = SPAPR_CAP_OFF;
> > + smc->default_caps.caps[SPAPR_CAP_CCF_ASSIST] = SPAPR_CAP_ON;
> > spapr_caps_add_properties(smc, &error_abort);
> > smc->irq = &spapr_irq_dual;
> > smc->dr_phb_enabled = true;
> > @@ -4465,8 +4465,11 @@ DEFINE_SPAPR_MACHINE(5_0, "5.0", true);
> > */
> > static void spapr_machine_4_2_class_options(MachineClass *mc)
> > {
> > + SpaprMachineClass *smc = SPAPR_MACHINE_CLASS(mc);
> > +
> > spapr_machine_5_0_class_options(mc);
> > compat_props_add(mc->compat_props, hw_compat_4_2, hw_compat_4_2_len);
> > + smc->default_caps.caps[SPAPR_CAP_CCF_ASSIST] = SPAPR_CAP_OFF;
> > }
> >
> > DEFINE_SPAPR_MACHINE(4_2, "4.2", false);
> > diff --git a/hw/ppc/spapr_caps.c b/hw/ppc/spapr_caps.c
> > index 481dfd2a27..d0d4b32a40 100644
> > --- a/hw/ppc/spapr_caps.c
> > +++ b/hw/ppc/spapr_caps.c
> > @@ -482,18 +482,36 @@ static void cap_large_decr_cpu_apply(SpaprMachineState *spapr,
> > static void cap_ccf_assist_apply(SpaprMachineState *spapr, uint8_t val,
> > Error **errp)
> > {
> > + Error *local_err = NULL;
> > uint8_t kvm_val = kvmppc_get_cap_count_cache_flush_assist();
> >
> > if (tcg_enabled() && val) {
> > - /* TODO - for now only allow broken for TCG */
> > - error_setg(errp,
> > -"Requested count cache flush assist capability level not supported by tcg,"
> > - " try appending -machine cap-ccf-assist=off");
> > + /* TCG doesn't implement anything here, but allow with a warning */
> > + error_setg(&local_err,
> > + "TCG doesn't support requested feature, cap-ccf-assist=on");
>
> The only user for local_err is warn_report_err() below. It would be simpler to
> simply call warn_report() here.
Yeah, fair enough. I was doing it this way for consistency with some
of the other cases where we warn only on TCG, but there's not really
any point. Revised in my tree.
>
> > } else if (kvm_enabled() && (val > kvm_val)) {
> > + uint8_t kvm_ibs = kvmppc_get_cap_safe_indirect_branch();
> > +
> > + if (kvm_ibs == SPAPR_CAP_FIXED_CCD) {
> > + /*
> > + * If we don't have CCF assist on the host, the assist
> > + * instruction is a harmless no-op. It won't correctly
> > + * implement the cache count flush *but* if we have
> > + * count-cache-disabled in the host, that flush is
> > + * unnnecessary. So, specifically allow this case. This
> > + * allows us to have better performance on POWER9 DD2.3,
> > + * while still working on POWER9 DD2.2 and POWER8 host
> > + * cpus.
> > + */
> > + return;
> > + }
> > error_setg(errp,
> > "Requested count cache flush assist capability level not supported by kvm,"
> > " try appending -machine cap-ccf-assist=off");
> > }
> > +
> > + if (local_err != NULL)
> > + warn_report_err(local_err);
> > }
> >
> > SpaprCapabilityInfo capability_table[SPAPR_CAP_NUM] = {
>
--
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 #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH] spapr: Enable DD2.3 accelerated count cache flush in pseries-5.0 machine
2020-01-30 1:26 [PATCH] spapr: Enable DD2.3 accelerated count cache flush in pseries-5.0 machine David Gibson
2020-01-30 8:09 ` Laurent Vivier
2020-01-30 8:48 ` Greg Kurz
@ 2020-01-31 10:28 ` Michael Ellerman
2 siblings, 0 replies; 6+ messages in thread
From: Michael Ellerman @ 2020-01-31 10:28 UTC (permalink / raw)
To: David Gibson, groug, clg
Cc: lvivier, aik, qemu-devel, paulus, qemu-ppc, David Gibson
David Gibson <david@gibson.dropbear.id.au> writes:
> For POWER9 DD2.2 cpus, the best current Spectre v2 indirect branch
> mitigation is "count cache disabled", which is configured with:
> -machine cap-ibs=fixed-ccd
> However, this option isn't available on DD2.3 CPUs with KVM, because they
> don't have the count cache disabled.
>
> For POWER9 DD2.3 cpus, it is "count cache flush with assist", configured
> with:
> -machine cap-ibs=workaround,cap-ccf-assist=on
> However this option isn't available on DD2.2 CPUs with KVM, because they
> don't have the special CCF assist instruction this relies on.
>
> On current machine types, we default to "count cache flush w/o assist",
> that is:
> -machine cap-ibs=workaround,cap-ccf-assist=off
> This runs, with mitigation on both DD2.2 and DD2.3 host cpus, but has a
> fairly significant performance impact.
>
> It turns out we can do better. The special instruction that CCF assist
> uses to trigger a count cache flush is a no-op on earlier CPUs, rather than
> trapping or causing other badness. It doesn't, of itself, implement the
> mitigation, but *if* we have count-cache-disabled, then the count cache
> flush is unnecessary, and so using the count cache flush mitigation is
> harmless.
>
> Therefore for the new pseries-5.0 machine type, enable cap-ccf-assist by
> default. Along with that, suppress throwing an error if cap-ccf-assist
> is selected but KVM doesn't support it, as long as KVM *is* giving us
> count-cache-disabled. To allow TCG to work out of the box, even though it
> doesn't implement the ccf flush assist, downgrade the error in that case to
> a warning. This matches several Spectre mitigations where we allow TCG
> to operate for debugging, since we don't really make guarantees about TCG
> security properties anyway.
>
> While we're there, make the TCG warning for this case match that for other
> mitigations.
>
> Signed-off-by: David Gibson <david@gibson.dropbear.id.au>
> ---
> hw/ppc/spapr.c | 5 ++++-
> hw/ppc/spapr_caps.c | 26 ++++++++++++++++++++++----
> 2 files changed, 26 insertions(+), 5 deletions(-)
>
> I have put this into my ppc-for-5.0 tree already, and hope to send a
> pull request tomorrow (Jan 31).
I tested this on a DD 2.2 machine, seems to work as designed.
I had to modify our spectre_v2 test slightly because it is confused
about the sysfs reported mitigation not reflecting reality, but that is
an existing issue.
cheers
# uname -r
5.5.0-gcc-8.2.0
# cat /proc/cpuinfo
processor : 0
cpu : POWER9 (architected), altivec supported
clock : 3000.000000MHz
revision : 2.2 (pvr 004e 1202)
timebase : 512000000
platform : pSeries
model : IBM pSeries (emulated by qemu)
machine : CHRP IBM pSeries (emulated by qemu)
MMU : Radix
# grep . /sys/devices/system/cpu/vulnerabilities/*
/sys/devices/system/cpu/vulnerabilities/itlb_multihit:Not affected
/sys/devices/system/cpu/vulnerabilities/l1tf:Mitigation: RFI Flush, L1D private per thread
/sys/devices/system/cpu/vulnerabilities/mds:Not affected
/sys/devices/system/cpu/vulnerabilities/meltdown:Mitigation: RFI Flush, L1D private per thread
/sys/devices/system/cpu/vulnerabilities/spec_store_bypass:Mitigation: Kernel entry/exit barrier (eieio)
/sys/devices/system/cpu/vulnerabilities/tsx_async_abort:Not affected
/sys/devices/system/cpu/vulnerabilities/spectre_v1:Mitigation: __user pointer sanitization, ori31 speculation barrier enabled
/sys/devices/system/cpu/vulnerabilities/spectre_v2:Mitigation: Software count cache flush (hardware accelerated), Software link stack flush
# /spectre_v2
test: spectre_v2
tags: git_version:v5.5-1-g0b2cecc02bf1
sysfs reports: 'Mitigation: Software count cache flush (hardware accelerated), Software link stack flush'
HW accelerated count cache flush on P9N DD2.2 => actually count cache disabled.
PM_BR_PRED_CCACHE: result 2147483649 running/enabled 19406588726
PM_BR_MPRED_CCACHE: result 2147483649 running/enabled 19406580624
PM_BR_PRED_PCACHE: result 0 running/enabled 19406574530
PM_BR_MPRED_PCACHE: result 0 running/enabled 19406571036
Miss percent 100 %
OK - Measured branch prediction rates match reported spectre v2 mitigation.
success: spectre_v2
# [ 47.183779][ C0] sysrq: Entering xmon
0:mon> di $_switch
c00000000040e280 7c0802a6 mflr r0
c00000000040e284 f8010010 std r0,16(r1)
c00000000040e288 f821fe21 stdu r1,-480(r1)
c00000000040e28c f9c100e0 std r14,224(r1)
c00000000040e290 f9e100e8 std r15,232(r1)
c00000000040e294 fa0100f0 std r16,240(r1)
c00000000040e298 fa2100f8 std r17,248(r1)
c00000000040e29c fa410100 std r18,256(r1)
c00000000040e2a0 fa610108 std r19,264(r1)
c00000000040e2a4 fa810110 std r20,272(r1)
c00000000040e2a8 faa10118 std r21,280(r1)
c00000000040e2ac fac10120 std r22,288(r1)
c00000000040e2b0 fae10128 std r23,296(r1)
c00000000040e2b4 fb010130 std r24,304(r1)
c00000000040e2b8 fb210138 std r25,312(r1)
c00000000040e2bc fb410140 std r26,320(r1)
c00000000040e2c0 fb610148 std r27,328(r1)
c00000000040e2c4 fb810150 std r28,336(r1)
c00000000040e2c8 fba10158 std r29,344(r1)
c00000000040e2cc fbc10160 std r30,352(r1)
c00000000040e2d0 fbe10168 std r31,360(r1)
c00000000040e2d4 f8010170 std r0,368(r1)
c00000000040e2d8 7ee00026 mfcr r23
c00000000040e2dc fae101a0 std r23,416(r1)
c00000000040e2e0 f8230000 std r1,0(r3)
c00000000040e2e4 4bffdafd bl c00000000040bde0 # flush_count_cache+0x0/0x24a0
0:mon> di $flush_count_cache
c00000000040bde0 7d2802a6 mflr r9
# This is the link stack flush ...
c00000000040bde4 48000005 bl c00000000040bde8 # flush_count_cache+0x8/0x24a0
...
0:mon>
c00000000040be20 48000005 bl c00000000040be24 # flush_count_cache+0x44/0x24a0
...
0:mon>
c00000000040be60 48000005 bl c00000000040be64 # flush_count_cache+0x84/0x24a0
...
0:mon>
c00000000040bea0 48000005 bl c00000000040bea4 # flush_count_cache+0xc4/0x24a0
...
0:mon>
c00000000040bee0 48000005 bl c00000000040bee4 # flush_count_cache+0x104/0x24a0
c00000000040bee4 4800001c b c00000000040bf00 # flush_count_cache+0x120/0x24a0
c00000000040bee8 60000000 nop
...
c00000000040bf00 7d2803a6 mtlr r9
c00000000040bf04 60000000 nop
c00000000040bf08 39207fff li r9,32767
c00000000040bf0c 7d2903a6 mtctr r9
# This is the HW accelerated count cache flush
c00000000040bf10 4c400420 bcctr- 2,lt
c00000000040bf14 4e800020 blr
^ permalink raw reply [flat|nested] 6+ messages in thread
end of thread, other threads:[~2020-01-31 10:29 UTC | newest]
Thread overview: 6+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2020-01-30 1:26 [PATCH] spapr: Enable DD2.3 accelerated count cache flush in pseries-5.0 machine David Gibson
2020-01-30 8:09 ` Laurent Vivier
2020-01-30 8:54 ` Greg Kurz
2020-01-30 8:48 ` Greg Kurz
2020-01-30 23:47 ` David Gibson
2020-01-31 10:28 ` Michael Ellerman
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).