| Message ID | 20220128132927.14997-5-andrew.cooper3@citrix.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | x86: MSR_SPEC_CTRL support for SVM guests | expand |
On 28/01/2022 13:29, Andrew Cooper wrote: > 'idle' here refers to hlt/mwait. The S3 path isn't an idle path - it is a > platform reset. > > Conditionally clearing IBRS and flushing the store buffers on the way down is > a waste of time. > > Furthermore, we want to load default_xen_mcu_opt_ctrl unilaterally on the way > back up. Currently it happens as a side effect of X86_FEATURE_SC_MSR_IDLE or > the next return-to-guest, but that's fragile behaviour. > > Signed-off-by: Andrew Cooper <andrew.cooper3@citrix.com> > --- > CC: Jan Beulich <JBeulich@suse.com> > CC: Roger Pau Monné <roger.pau@citrix.com> > CC: Wei Liu <wl@xen.org> > > v2: > * New > --- > xen/arch/x86/acpi/power.c | 5 +++-- > 1 file changed, 3 insertions(+), 2 deletions(-) > > diff --git a/xen/arch/x86/acpi/power.c b/xen/arch/x86/acpi/power.c > index 31a56f02d083..ea2bd8bbfe93 100644 > --- a/xen/arch/x86/acpi/power.c > +++ b/xen/arch/x86/acpi/power.c > @@ -248,7 +248,6 @@ static int enter_state(u32 state) > error = 0; > > ci = get_cpu_info(); > - spec_ctrl_enter_idle(ci); > /* Avoid NMI/#MC using MSR_SPEC_CTRL until we've reloaded microcode. */ > ci->spec_ctrl_flags &= ~SCF_ist_wrmsr; > > @@ -295,7 +294,9 @@ static int enter_state(u32 state) > > /* Re-enabled default NMI/#MC use of MSR_SPEC_CTRL. */ > ci->spec_ctrl_flags |= (default_spec_ctrl_flags & SCF_ist_wrmsr); > - spec_ctrl_exit_idle(ci); > + > + if ( boot_cpu_has(X86_FEATURE_IBRSB) ) > + wrmsrl(MSR_SPEC_CTRL, default_xen_mcu_opt_ctrl); This logic works rather better when it gets the right variable. default_xen_spec_ctrl. ~Andrew > > if ( boot_cpu_has(X86_FEATURE_SRBDS_CTRL) ) > wrmsrl(MSR_MCU_OPT_CTRL, default_xen_mcu_opt_ctrl);
On 28.01.2022 14:29, Andrew Cooper wrote: > 'idle' here refers to hlt/mwait. The S3 path isn't an idle path - it is a > platform reset. > > Conditionally clearing IBRS and flushing the store buffers on the way down is > a waste of time. I can buy this for the flushing aspect; I'm less certain about the clearing of IBRS: Whether the act of clearing is slower than the performance price of running with it enabled is unknown, I suppose? > Furthermore, we want to load default_xen_mcu_opt_ctrl unilaterally on the way > back up. Currently it happens as a side effect of X86_FEATURE_SC_MSR_IDLE or > the next return-to-guest, but that's fragile behaviour. I'll assume from your reply that you've adjusted the description as well. > Signed-off-by: Andrew Cooper <andrew.cooper3@citrix.com> Preferably with the statement above softened a little: Reviewed-by: Jan Beulich <jbeulich@suse.com> Jan
On 31/01/2022 10:15, Jan Beulich wrote: > On 28.01.2022 14:29, Andrew Cooper wrote: >> 'idle' here refers to hlt/mwait. The S3 path isn't an idle path - it is a >> platform reset. >> >> Conditionally clearing IBRS and flushing the store buffers on the way down is >> a waste of time. > I can buy this for the flushing aspect; I'm less certain about the clearing > of IBRS: Whether the act of clearing is slower than the performance price > of running with it enabled is unknown, I suppose? There are a handful of instructions from now until the core is powered down. The cost of the WRMSR is going to dominate everything else, but we're still only talking microseconds. But honestly, the perf aspect isn't relevant. It's wrong to use the enter/exit idle helpers here. >> Furthermore, we want to load default_xen_mcu_opt_ctrl unilaterally on the way >> back up. Currently it happens as a side effect of X86_FEATURE_SC_MSR_IDLE or >> the next return-to-guest, but that's fragile behaviour. > I'll assume from your reply that you've adjusted the description as well. I have, yes. > >> Signed-off-by: Andrew Cooper <andrew.cooper3@citrix.com> > Preferably with the statement above softened a little: > Reviewed-by: Jan Beulich <jbeulich@suse.com> Thanks. ~Andrew
On 31/01/2022 11:23, Andrew Cooper wrote: > On 31/01/2022 10:15, Jan Beulich wrote: >> On 28.01.2022 14:29, Andrew Cooper wrote: >>> Signed-off-by: Andrew Cooper <andrew.cooper3@citrix.com> >> Preferably with the statement above softened a little: >> Reviewed-by: Jan Beulich <jbeulich@suse.com> > Thanks. Commit message now reads: ---8<--- 'idle' here refers to hlt/mwait. The S3 path isn't an idle path - it is a platform reset. We need to load default_xen_spec_ctrl unilaterally on the way back up. Currently it happens as a side effect of X86_FEATURE_SC_MSR_IDLE or the next return-to-guest, but that's fragile behaviour. Conversely, there is no need to clear IBRS and flush the store buffers on the way down; we're microseconds away from cutting power. ---8<--- ~Andrew
diff --git a/xen/arch/x86/acpi/power.c b/xen/arch/x86/acpi/power.c index 31a56f02d083..ea2bd8bbfe93 100644 --- a/xen/arch/x86/acpi/power.c +++ b/xen/arch/x86/acpi/power.c @@ -248,7 +248,6 @@ static int enter_state(u32 state) error = 0; ci = get_cpu_info(); - spec_ctrl_enter_idle(ci); /* Avoid NMI/#MC using MSR_SPEC_CTRL until we've reloaded microcode. */ ci->spec_ctrl_flags &= ~SCF_ist_wrmsr; @@ -295,7 +294,9 @@ static int enter_state(u32 state) /* Re-enabled default NMI/#MC use of MSR_SPEC_CTRL. */ ci->spec_ctrl_flags |= (default_spec_ctrl_flags & SCF_ist_wrmsr); - spec_ctrl_exit_idle(ci); + + if ( boot_cpu_has(X86_FEATURE_IBRSB) ) + wrmsrl(MSR_SPEC_CTRL, default_xen_mcu_opt_ctrl); if ( boot_cpu_has(X86_FEATURE_SRBDS_CTRL) ) wrmsrl(MSR_MCU_OPT_CTRL, default_xen_mcu_opt_ctrl);
'idle' here refers to hlt/mwait. The S3 path isn't an idle path - it is a platform reset. Conditionally clearing IBRS and flushing the store buffers on the way down is a waste of time. Furthermore, we want to load default_xen_mcu_opt_ctrl unilaterally on the way back up. Currently it happens as a side effect of X86_FEATURE_SC_MSR_IDLE or the next return-to-guest, but that's fragile behaviour. Signed-off-by: Andrew Cooper <andrew.cooper3@citrix.com> --- CC: Jan Beulich <JBeulich@suse.com> CC: Roger Pau Monné <roger.pau@citrix.com> CC: Wei Liu <wl@xen.org> v2: * New --- xen/arch/x86/acpi/power.c | 5 +++-- 1 file changed, 3 insertions(+), 2 deletions(-)