| Message ID | 20200130012622.8564-1-david@gibson.dropbear.id.au (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | spapr: Enable DD2.3 accelerated count cache flush in pseries-5.0 machine | expand |
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
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] = {
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 > >
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] = { >
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
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] = {
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).