Re: [PATCH] spapr: Enable DD2.3 accelerated count cache flush in pseries-5.0 machine

[David Gibson]

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