[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]
Re: [PATCH] spapr: Enable DD2.3 accelerated count cache flush in pseries
From: |
David Gibson |
Subject: |
Re: [PATCH] spapr: Enable DD2.3 accelerated count cache flush in pseries-5.0 machine |
Date: |
Fri, 31 Jan 2020 10:47:05 +1100 |
On Thu, Jan 30, 2020 at 09:48:49AM +0100, Greg Kurz wrote:
> On Thu, 30 Jan 2020 12:26:22 +1100
> David Gibson <address@hidden> 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 <address@hidden>
> > ---
> > 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
signature.asc
Description: PGP signature