| Message ID | 20240418155208.7771-3-roger.pau@citrix.com (mailing list archive) |
|---|---|
| State | New |
| Headers | show |
| Series | x86/spec: misc fixes for XSA-456 | expand |
On 18.04.2024 17:52, Roger Pau Monne wrote: > It's currently too restrictive by just checking whether there's a BHB clearing > sequence selected. It should instead check whether BHB clearing is used on > entry from PV or HVM specifically. > > Switch to use opt_bhb_entry_{pv,hvm} instead, and then remove cpu_has_bhb_seq > since it no longer has any users. > > Reported-by: Jan Beulich <jbeulich@suse.com> > Fixes: 954c983abcee ('x86/spec-ctrl: Software BHB-clearing sequences') > Signed-off-by: Roger Pau Monné <roger.pau@citrix.com> Except for the odd double "logic" in the title: Reviewed-by: Jan Beulich <jbeulich@suse.com> I can't really guess what is meant instead, so in order to possibly adjust while committing I'll need a hint. But committing will want to wait until Andrew has taken a look anyway, just like for patch 1. > There (possibly) still a bit of overhead for dom0 if BHB clearing is not used > for dom0, as Xen would still add the lfence if domUs require it. Right, but what do you do. > --- a/xen/arch/x86/include/asm/cpufeature.h > +++ b/xen/arch/x86/include/asm/cpufeature.h > @@ -235,9 +235,6 @@ static inline bool boot_cpu_has(unsigned int feat) > #define cpu_bug_fpu_ptrs boot_cpu_has(X86_BUG_FPU_PTRS) > #define cpu_bug_null_seg boot_cpu_has(X86_BUG_NULL_SEG) > > -#define cpu_has_bhb_seq (boot_cpu_has(X86_SPEC_BHB_TSX) || \ > - boot_cpu_has(X86_SPEC_BHB_LOOPS)) Might be worth also mentioning in the description that this construct was lacking use of X86_SPEC_BHB_LOOPS_LONG (might even warrant a 2nd Fixes: tag). Jan
On Fri, Apr 19, 2024 at 08:25:00AM +0200, Jan Beulich wrote: > On 18.04.2024 17:52, Roger Pau Monne wrote: > > It's currently too restrictive by just checking whether there's a BHB clearing > > sequence selected. It should instead check whether BHB clearing is used on > > entry from PV or HVM specifically. > > > > Switch to use opt_bhb_entry_{pv,hvm} instead, and then remove cpu_has_bhb_seq > > since it no longer has any users. > > > > Reported-by: Jan Beulich <jbeulich@suse.com> > > Fixes: 954c983abcee ('x86/spec-ctrl: Software BHB-clearing sequences') > > Signed-off-by: Roger Pau Monné <roger.pau@citrix.com> > > Except for the odd double "logic" in the title: > Reviewed-by: Jan Beulich <jbeulich@suse.com> Thanks, title should be: "adjust logic that elides lfence" It was just a typo, I didn't intended to express anything additional. > I can't really guess what is meant instead, so in order to possibly adjust > while committing I'll need a hint. But committing will want to wait until > Andrew has taken a look anyway, just like for patch 1. > > > There (possibly) still a bit of overhead for dom0 if BHB clearing is not used > > for dom0, as Xen would still add the lfence if domUs require it. > > Right, but what do you do. > > > --- a/xen/arch/x86/include/asm/cpufeature.h > > +++ b/xen/arch/x86/include/asm/cpufeature.h > > @@ -235,9 +235,6 @@ static inline bool boot_cpu_has(unsigned int feat) > > #define cpu_bug_fpu_ptrs boot_cpu_has(X86_BUG_FPU_PTRS) > > #define cpu_bug_null_seg boot_cpu_has(X86_BUG_NULL_SEG) > > > > -#define cpu_has_bhb_seq (boot_cpu_has(X86_SPEC_BHB_TSX) || \ > > - boot_cpu_has(X86_SPEC_BHB_LOOPS)) > > Might be worth also mentioning in the description that this construct was > lacking use of X86_SPEC_BHB_LOOPS_LONG (might even warrant a 2nd Fixes: > tag). Heh, no, X86_SPEC_BHB_LOOPS_LONG is added in addition to X86_SPEC_BHB_LOOPS. When using long loops we have both X86_SPEC_BHB_LOOPS and X86_SPEC_BHB_LOOPS_LONG set (I know it's confusing, I was also confused the first time and asked Andrew the same question). See the fallthrough in bhi_calculations(). Thanks, Roger.
On 22.04.2024 15:35, Roger Pau Monné wrote: > On Fri, Apr 19, 2024 at 08:25:00AM +0200, Jan Beulich wrote: >> On 18.04.2024 17:52, Roger Pau Monne wrote: >>> --- a/xen/arch/x86/include/asm/cpufeature.h >>> +++ b/xen/arch/x86/include/asm/cpufeature.h >>> @@ -235,9 +235,6 @@ static inline bool boot_cpu_has(unsigned int feat) >>> #define cpu_bug_fpu_ptrs boot_cpu_has(X86_BUG_FPU_PTRS) >>> #define cpu_bug_null_seg boot_cpu_has(X86_BUG_NULL_SEG) >>> >>> -#define cpu_has_bhb_seq (boot_cpu_has(X86_SPEC_BHB_TSX) || \ >>> - boot_cpu_has(X86_SPEC_BHB_LOOPS)) >> >> Might be worth also mentioning in the description that this construct was >> lacking use of X86_SPEC_BHB_LOOPS_LONG (might even warrant a 2nd Fixes: >> tag). > > Heh, no, X86_SPEC_BHB_LOOPS_LONG is added in addition to > X86_SPEC_BHB_LOOPS. When using long loops we have both > X86_SPEC_BHB_LOOPS and X86_SPEC_BHB_LOOPS_LONG set (I know it's > confusing, I was also confused the first time and asked Andrew the > same question). See the fallthrough in bhi_calculations(). Oh, I see. Andrew: This is a very good example of the separating blank line being misleading when fall-through is intended. Jan
On 18/04/2024 4:52 pm, Roger Pau Monne wrote: > It's currently too restrictive by just checking whether there's a BHB clearing > sequence selected. It should instead check whether BHB clearing is used on > entry from PV or HVM specifically. > > Switch to use opt_bhb_entry_{pv,hvm} instead, and then remove cpu_has_bhb_seq > since it no longer has any users. > > Reported-by: Jan Beulich <jbeulich@suse.com> > Fixes: 954c983abcee ('x86/spec-ctrl: Software BHB-clearing sequences') > Signed-off-by: Roger Pau Monné <roger.pau@citrix.com> > --- > Changes since v1: > - New in this version. > > There (possibly) still a bit of overhead for dom0 if BHB clearing is not used > for dom0, as Xen would still add the lfence if domUs require it. This is the note about dom0 that I made on the previous patch. "protect dom0" only has any effect if the appropriate foo_pv or foo_hvm is also selected. It's not possible to express "protect dom0 but not domU of $TYPE". This early on boot we have no idea whether dom0 is going to be PV or HVM. We could in principle figure it out by peeking at dom0's ELF notes, but that needs a lot of rearranging of __start_xen() to do safely. Reviewed-by: Andrew Cooper <andrew.cooper3@citrix.com>
diff --git a/xen/arch/x86/include/asm/cpufeature.h b/xen/arch/x86/include/asm/cpufeature.h index 743f11f98940..9bc553681f4a 100644 --- a/xen/arch/x86/include/asm/cpufeature.h +++ b/xen/arch/x86/include/asm/cpufeature.h @@ -235,9 +235,6 @@ static inline bool boot_cpu_has(unsigned int feat) #define cpu_bug_fpu_ptrs boot_cpu_has(X86_BUG_FPU_PTRS) #define cpu_bug_null_seg boot_cpu_has(X86_BUG_NULL_SEG) -#define cpu_has_bhb_seq (boot_cpu_has(X86_SPEC_BHB_TSX) || \ - boot_cpu_has(X86_SPEC_BHB_LOOPS)) - enum _cache_type { CACHE_TYPE_NULL = 0, CACHE_TYPE_DATA = 1, diff --git a/xen/arch/x86/spec_ctrl.c b/xen/arch/x86/spec_ctrl.c index 1e831c1c108e..40f6ae017010 100644 --- a/xen/arch/x86/spec_ctrl.c +++ b/xen/arch/x86/spec_ctrl.c @@ -2328,7 +2328,7 @@ void __init init_speculation_mitigations(void) * unconditional WRMSR. If we do have it, or we're not using any * prior conditional block, then it's safe to drop the LFENCE. */ - if ( !cpu_has_bhb_seq && + if ( !opt_bhb_entry_pv && (boot_cpu_has(X86_FEATURE_SC_MSR_PV) || !boot_cpu_has(X86_FEATURE_IBPB_ENTRY_PV)) ) setup_force_cpu_cap(X86_SPEC_NO_LFENCE_ENTRY_PV); @@ -2344,7 +2344,7 @@ void __init init_speculation_mitigations(void) * active in the block that is skipped when interrupting guest * context, then it's safe to drop the LFENCE. */ - if ( !cpu_has_bhb_seq && + if ( !opt_bhb_entry_pv && (boot_cpu_has(X86_FEATURE_SC_MSR_PV) || (!boot_cpu_has(X86_FEATURE_IBPB_ENTRY_PV) && !boot_cpu_has(X86_FEATURE_SC_RSB_PV))) ) @@ -2356,7 +2356,7 @@ void __init init_speculation_mitigations(void) * A BHB sequence, if used, is the only conditional action, so if we * don't have it, we don't need the safety LFENCE. */ - if ( !cpu_has_bhb_seq ) + if ( !opt_bhb_entry_hvm ) setup_force_cpu_cap(X86_SPEC_NO_LFENCE_ENTRY_VMX); }
It's currently too restrictive by just checking whether there's a BHB clearing sequence selected. It should instead check whether BHB clearing is used on entry from PV or HVM specifically. Switch to use opt_bhb_entry_{pv,hvm} instead, and then remove cpu_has_bhb_seq since it no longer has any users. Reported-by: Jan Beulich <jbeulich@suse.com> Fixes: 954c983abcee ('x86/spec-ctrl: Software BHB-clearing sequences') Signed-off-by: Roger Pau Monné <roger.pau@citrix.com> --- Changes since v1: - New in this version. There (possibly) still a bit of overhead for dom0 if BHB clearing is not used for dom0, as Xen would still add the lfence if domUs require it. --- xen/arch/x86/include/asm/cpufeature.h | 3 --- xen/arch/x86/spec_ctrl.c | 6 +++--- 2 files changed, 3 insertions(+), 6 deletions(-)