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

[Laurent Vivier]

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 <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");
>      } 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