spapr: Enable DD2.3 accelerated count cache flush in pseries-5.0 machine
diff mbox series

Message ID 20200130012622.8564-1-david@gibson.dropbear.id.au
State New
Headers show
Series
  • spapr: Enable DD2.3 accelerated count cache flush in pseries-5.0 machine
Related show

Commit Message

David Gibson Jan. 30, 2020, 1:26 a.m. UTC
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).

Comments

Laurent Vivier Jan. 30, 2020, 8:09 a.m. UTC | #1
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
Greg Kurz Jan. 30, 2020, 8:48 a.m. UTC | #2
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] = {
Greg Kurz Jan. 30, 2020, 8:54 a.m. UTC | #3
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
> 
>
David Gibson Jan. 30, 2020, 11:47 p.m. UTC | #4
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] = {
>
Michael Ellerman Jan. 31, 2020, 10:28 a.m. UTC | #5
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

Patch
diff mbox series

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] = {