[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: |
Greg Kurz |
Subject: |
Re: [PATCH] spapr: Enable DD2.3 accelerated count cache flush in pseries-5.0 machine |
Date: |
Thu, 30 Jan 2020 09:48:49 +0100 |
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.
> } 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] = {