[PATCH v2 0/2] x86/spec: misc fixes for XSA-456

[PATCH v2 0/2] x86/spec: misc fixes for XSA-456

[Roger Pau Monne]

Hello,

Fix patch fixing the printing of whether BHB-entry is used for PV or
HVM, second patch refines a bit the logic to decide whether the lfence
on entry can be elided for the entry points.

Thanks, Roger.

Roger Pau Monne (2):
  x86/spec: fix reporting of BHB clearing usage from guest entry points
  x86/spec: adjust logic to logic that elides lfence

 xen/arch/x86/include/asm/cpufeature.h |  3 ---
 xen/arch/x86/spec_ctrl.c              | 14 +++++++-------
 2 files changed, 7 insertions(+), 10 deletions(-)

-- 
2.44.0

[PATCH v2 1/2] x86/spec: fix reporting of BHB clearing usage from guest entry points

[Roger Pau Monne]

Reporting whether the BHB clearing on entry is done for the different domains
types based on cpu_has_bhb_seq is incorrect, as that variable signals whether
there's a BHB clearing sequence selected, but that alone doesn't imply that
such sequence is used from the PV and/or HVM entry points.

Instead use opt_bhb_entry_{pv,hvm} which do signal whether BHB clearing is
performed on entry from PV/HVM.

Fixes: 689ad48ce9cf ('x86/spec-ctrl: Wire up the Native-BHI software sequences')
Signed-off-by: Roger Pau Monné <roger.pau@citrix.com>
---
Changes since v1:
 - Also fix usage of cpu_has_bhb_seq when deciding whether to print "None".
---
 xen/arch/x86/spec_ctrl.c | 8 ++++----
 1 file changed, 4 insertions(+), 4 deletions(-)

diff --git a/xen/arch/x86/spec_ctrl.c b/xen/arch/x86/spec_ctrl.c
index dd01e30844a1..1e831c1c108e 100644
--- a/xen/arch/x86/spec_ctrl.c
+++ b/xen/arch/x86/spec_ctrl.c
@@ -634,7 +634,7 @@ static void __init print_details(enum ind_thunk thunk)
            (boot_cpu_has(X86_FEATURE_SC_MSR_HVM) ||
             boot_cpu_has(X86_FEATURE_SC_RSB_HVM) ||
             boot_cpu_has(X86_FEATURE_IBPB_ENTRY_HVM) ||
-            cpu_has_bhb_seq || amd_virt_spec_ctrl ||
+            opt_bhb_entry_hvm || amd_virt_spec_ctrl ||
             opt_eager_fpu || opt_verw_hvm)           ? ""               : " None",
            boot_cpu_has(X86_FEATURE_SC_MSR_HVM)      ? " MSR_SPEC_CTRL" : "",
            (boot_cpu_has(X86_FEATURE_SC_MSR_HVM) ||
@@ -643,7 +643,7 @@ static void __init print_details(enum ind_thunk thunk)
            opt_eager_fpu                             ? " EAGER_FPU"     : "",
            opt_verw_hvm                              ? " VERW"          : "",
            boot_cpu_has(X86_FEATURE_IBPB_ENTRY_HVM)  ? " IBPB-entry"    : "",
-           cpu_has_bhb_seq                           ? " BHB-entry"     : "");
+           opt_bhb_entry_hvm                         ? " BHB-entry"     : "");
 
 #endif
 #ifdef CONFIG_PV
@@ -651,14 +651,14 @@ static void __init print_details(enum ind_thunk thunk)
            (boot_cpu_has(X86_FEATURE_SC_MSR_PV) ||
             boot_cpu_has(X86_FEATURE_SC_RSB_PV) ||
             boot_cpu_has(X86_FEATURE_IBPB_ENTRY_PV) ||
-            cpu_has_bhb_seq ||
+            opt_bhb_entry_pv ||
             opt_eager_fpu || opt_verw_pv)            ? ""               : " None",
            boot_cpu_has(X86_FEATURE_SC_MSR_PV)       ? " MSR_SPEC_CTRL" : "",
            boot_cpu_has(X86_FEATURE_SC_RSB_PV)       ? " RSB"           : "",
            opt_eager_fpu                             ? " EAGER_FPU"     : "",
            opt_verw_pv                               ? " VERW"          : "",
            boot_cpu_has(X86_FEATURE_IBPB_ENTRY_PV)   ? " IBPB-entry"    : "",
-           cpu_has_bhb_seq                           ? " BHB-entry"     : "");
+           opt_bhb_entry_pv                          ? " BHB-entry"     : "");
 
     printk("  XPTI (64-bit PV only): Dom0 %s, DomU %s (with%s PCID)\n",
            opt_xpti_hwdom ? "enabled" : "disabled",
-- 
2.44.0

Re: [PATCH v2 1/2] x86/spec: fix reporting of BHB clearing usage from guest entry points

[Jan Beulich]

On 18.04.2024 17:52, Roger Pau Monne wrote:
> Reporting whether the BHB clearing on entry is done for the different domains
> types based on cpu_has_bhb_seq is incorrect, as that variable signals whether
> there's a BHB clearing sequence selected, but that alone doesn't imply that
> such sequence is used from the PV and/or HVM entry points.
> 
> Instead use opt_bhb_entry_{pv,hvm} which do signal whether BHB clearing is
> performed on entry from PV/HVM.
> 
> Fixes: 689ad48ce9cf ('x86/spec-ctrl: Wire up the Native-BHI software sequences')
> Signed-off-by: Roger Pau Monné <roger.pau@citrix.com>

This looks correct to me, so:
Reviewed-by: Jan Beulich <jbeulich@suse.com>
But since Andrew indicated concerns, I won't mark it (locally) for being
ready to go in.

Jan

Re: [PATCH v2 1/2] x86/spec: fix reporting of BHB clearing usage from guest entry points

[Andrew Cooper]

On 18/04/2024 4:52 pm, Roger Pau Monne wrote:
> Reporting whether the BHB clearing on entry is done for the different domains
> types based on cpu_has_bhb_seq is incorrect, as that variable signals whether

I'd prefer s/incorrect/unhelpful/ here.

As pointed out, it's currently like IBPB-entry in this regard,
identifying whether the block is in place; not whether it's used by the
condition.

> there's a BHB clearing sequence selected, but that alone doesn't imply that
> such sequence is used from the PV and/or HVM entry points.
>
> Instead use opt_bhb_entry_{pv,hvm} which do signal whether BHB clearing is
> performed on entry from PV/HVM.

I was going to ask for a note about dom0, but instead I need to make a
correction to the cmdline docs.  I'll do that in a separate patch.


> Fixes: 689ad48ce9cf ('x86/spec-ctrl: Wire up the Native-BHI software sequences')
> Signed-off-by: Roger Pau Monné <roger.pau@citrix.com>

Reviewed-by: Andrew Cooper <andrew.cooper3@citrix.com>

Can make the one tweak on commit.

[PATCH v2 2/2] x86/spec: adjust logic to logic that elides lfence

[Roger Pau Monne]

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(-)

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

Re: [PATCH v2 2/2] x86/spec: adjust logic to logic that elides lfence

[Jan Beulich]

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

Re: [PATCH v2 2/2] x86/spec: adjust logic to logic that elides lfence

[Roger Pau Monné]

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.

Re: [PATCH v2 2/2] x86/spec: adjust logic to logic that elides lfence

[Jan Beulich]

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

Re: [PATCH v2 2/2] x86/spec: adjust logic to logic that elides lfence

[Andrew Cooper]

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>