| Message ID | 20240422181434.3463252-7-andrew.cooper3@citrix.com (mailing list archive) |
|---|---|
| State | New |
| Headers | show |
| Series | x86/alternatives: Adjust all insn-relative fields | expand |
On 22.04.2024 20:14, Andrew Cooper wrote: > --- a/xen/arch/x86/hvm/vmx/entry.S > +++ b/xen/arch/x86/hvm/vmx/entry.S > @@ -62,12 +62,12 @@ ENTRY(vmx_asm_vmexit_handler) > * Clear the BHB to mitigate BHI. Used on eIBRS parts, and uses RETs > * itself so must be after we've perfomed all the RET-safety we can. > */ > - testb $SCF_entry_bhb, CPUINFO_scf(%rsp) > - jz .L_skip_bhb > - ALTERNATIVE_2 "", \ > - "call clear_bhb_loops", X86_SPEC_BHB_LOOPS, \ > - "call clear_bhb_tsx", X86_SPEC_BHB_TSX > -.L_skip_bhb: > + .macro VMX_BHB_SEQ fn:req > + DO_COND_BHB_SEQ \fn scf=CPUINFO_scf(%rsp) While the assembler permits this, can we please avoid mixing positional and named macro arguments? Instead, ... > + .endm > + ALTERNATIVE_2 "", \ > + "VMX_BHB_SEQ fn=clear_bhb_loops", X86_SPEC_BHB_LOOPS, \ > + "VMX_BHB_SEQ fn=clear_bhb_tsx", X86_SPEC_BHB_TSX ... as done further down, named arguments don't need using here. With the former addressed and preferably with the latter in consistent shape with what's below (which way round I don't care as much, albeit with a slight preference for "shorter"): Reviewed-by: Jan Beulich <jbeulich@suse.com> Jan > --- a/xen/arch/x86/include/asm/spec_ctrl_asm.h > +++ b/xen/arch/x86/include/asm/spec_ctrl_asm.h > @@ -92,6 +92,21 @@ > .L\@_skip: > .endm > > +.macro DO_COND_BHB_SEQ fn:req, scf=%bl > +/* > + * Requires SCF (defaults to %rbx), fn=clear_bhb_{loops,tsx} > + * Clobbers %rax, %rcx > + * > + * Conditionally use a BHB clearing software sequence. > + */ > + testb $SCF_entry_bhb, \scf > + jz .L\@_skip_bhb > + > + call \fn > + > +.L\@_skip_bhb: > +.endm > + > .macro DO_OVERWRITE_RSB tmp=rax, xu > /* > * Requires nothing > @@ -277,12 +292,9 @@ > * Clear the BHB to mitigate BHI. Used on eIBRS parts, and uses RETs > * itself so must be after we've perfomed all the RET-safety we can. > */ > - testb $SCF_entry_bhb, %bl > - jz .L\@_skip_bhb > - ALTERNATIVE_2 "", \ > - "call clear_bhb_loops", X86_SPEC_BHB_LOOPS, \ > - "call clear_bhb_tsx", X86_SPEC_BHB_TSX > -.L\@_skip_bhb: > + ALTERNATIVE_2 "", \ > + "DO_COND_BHB_SEQ clear_bhb_loops", X86_SPEC_BHB_LOOPS, \ > + "DO_COND_BHB_SEQ clear_bhb_tsx", X86_SPEC_BHB_TSX > > ALTERNATIVE "lfence", "", X86_SPEC_NO_LFENCE_ENTRY_PV > .endm > @@ -322,12 +334,9 @@ > ALTERNATIVE "", __stringify(DO_SPEC_CTRL_ENTRY maybexen=1), \ > X86_FEATURE_SC_MSR_PV > > - testb $SCF_entry_bhb, %bl > - jz .L\@_skip_bhb > - ALTERNATIVE_2 "", \ > - "call clear_bhb_loops", X86_SPEC_BHB_LOOPS, \ > - "call clear_bhb_tsx", X86_SPEC_BHB_TSX > -.L\@_skip_bhb: > + ALTERNATIVE_2 "", \ > + "DO_COND_BHB_SEQ clear_bhb_loops", X86_SPEC_BHB_LOOPS, \ > + "DO_COND_BHB_SEQ clear_bhb_tsx", X86_SPEC_BHB_TSX > > ALTERNATIVE "lfence", "", X86_SPEC_NO_LFENCE_ENTRY_INTR > .endm > @@ -433,13 +442,9 @@ > * Clear the BHB to mitigate BHI. Used on eIBRS parts, and uses RETs > * itself so must be after we've perfomed all the RET-safety we can. > */ > - testb $SCF_entry_bhb, %bl > - jz .L\@_skip_bhb > - > - ALTERNATIVE_2 "", \ > - "call clear_bhb_loops", X86_SPEC_BHB_LOOPS, \ > - "call clear_bhb_tsx", X86_SPEC_BHB_TSX > -.L\@_skip_bhb: > + ALTERNATIVE_2 "", \ > + "DO_COND_BHB_SEQ clear_bhb_loops", X86_SPEC_BHB_LOOPS, \ > + "DO_COND_BHB_SEQ clear_bhb_tsx", X86_SPEC_BHB_TSX > > lfence > .endm
On 22.04.2024 20:14, Andrew Cooper wrote: > --- a/xen/arch/x86/hvm/vmx/entry.S > +++ b/xen/arch/x86/hvm/vmx/entry.S > @@ -62,12 +62,12 @@ ENTRY(vmx_asm_vmexit_handler) > * Clear the BHB to mitigate BHI. Used on eIBRS parts, and uses RETs > * itself so must be after we've perfomed all the RET-safety we can. > */ > - testb $SCF_entry_bhb, CPUINFO_scf(%rsp) > - jz .L_skip_bhb > - ALTERNATIVE_2 "", \ > - "call clear_bhb_loops", X86_SPEC_BHB_LOOPS, \ > - "call clear_bhb_tsx", X86_SPEC_BHB_TSX > -.L_skip_bhb: > + .macro VMX_BHB_SEQ fn:req > + DO_COND_BHB_SEQ \fn scf=CPUINFO_scf(%rsp) > + .endm > + ALTERNATIVE_2 "", \ > + "VMX_BHB_SEQ fn=clear_bhb_loops", X86_SPEC_BHB_LOOPS, \ > + "VMX_BHB_SEQ fn=clear_bhb_tsx", X86_SPEC_BHB_TSX Oh, and just to mention it since we were discussing this before: The variant of this that I had been thinking of without decode-lite would have been to transform this (readable) testb $SCF_entry_bhb, CPUINFO_scf(%rsp) ALTERNATIVE_2 "jmp .L_skip_bhb", \ "jz .L_skip_bhb", X86_SPEC_BHB_LOOPS, \ "jz .L_skip_bhb", X86_SPEC_BHB_TSX ALTERNATIVE_2 "", \ "call clear_bhb_loops", X86_SPEC_BHB_LOOPS, \ "call clear_bhb_tsx", X86_SPEC_BHB_TSX .L_skip_bhb: into (untested, and hence perhaps slightly off) the (less readable) testb $SCF_entry_bhb, CPUINFO_scf(%rsp) ALTERNATIVE_2 ".byte 0xeb" /* jmp */, \ ".byte 0x74" /* jz */, X86_SPEC_BHB_LOOPS, \ ".byte 0x74" /* jz */, X86_SPEC_BHB_TSX .byte .L_skip_bhb - (. + 1) ALTERNATIVE_2 "", \ "call clear_bhb_loops", X86_SPEC_BHB_LOOPS, \ "call clear_bhb_tsx", X86_SPEC_BHB_TSX .L_skip_bhb: Of course yours (dropping the branch altogether) is better, but also comes at a higher price. Jan
On 22.04.2024 20:14, Andrew Cooper wrote: > @@ -322,12 +334,9 @@ > ALTERNATIVE "", __stringify(DO_SPEC_CTRL_ENTRY maybexen=1), \ > X86_FEATURE_SC_MSR_PV > > - testb $SCF_entry_bhb, %bl > - jz .L\@_skip_bhb > - ALTERNATIVE_2 "", \ > - "call clear_bhb_loops", X86_SPEC_BHB_LOOPS, \ > - "call clear_bhb_tsx", X86_SPEC_BHB_TSX > -.L\@_skip_bhb: > + ALTERNATIVE_2 "", \ > + "DO_COND_BHB_SEQ clear_bhb_loops", X86_SPEC_BHB_LOOPS, \ > + "DO_COND_BHB_SEQ clear_bhb_tsx", X86_SPEC_BHB_TSX Only spotting this while doing the 456 backport to 4.9: While this is all usual and fine, ... > @@ -433,13 +442,9 @@ > * Clear the BHB to mitigate BHI. Used on eIBRS parts, and uses RETs > * itself so must be after we've perfomed all the RET-safety we can. > */ > - testb $SCF_entry_bhb, %bl > - jz .L\@_skip_bhb > - > - ALTERNATIVE_2 "", \ > - "call clear_bhb_loops", X86_SPEC_BHB_LOOPS, \ > - "call clear_bhb_tsx", X86_SPEC_BHB_TSX > -.L\@_skip_bhb: > + ALTERNATIVE_2 "", \ > + "DO_COND_BHB_SEQ clear_bhb_loops", X86_SPEC_BHB_LOOPS, \ > + "DO_COND_BHB_SEQ clear_bhb_tsx", X86_SPEC_BHB_TSX > > lfence ... the original use of ALTERNATIVE_2 here is safe only because for the idle domain SCF_entry_bhb will not be set, and hence any patching done here will not affect NMI or #MC taken while doing the patching. Therefore I think this hunk needs dropping. Jan
diff --git a/xen/arch/x86/hvm/vmx/entry.S b/xen/arch/x86/hvm/vmx/entry.S index 7233e771d88a..77bf9ea564ea 100644 --- a/xen/arch/x86/hvm/vmx/entry.S +++ b/xen/arch/x86/hvm/vmx/entry.S @@ -62,12 +62,12 @@ ENTRY(vmx_asm_vmexit_handler) * Clear the BHB to mitigate BHI. Used on eIBRS parts, and uses RETs * itself so must be after we've perfomed all the RET-safety we can. */ - testb $SCF_entry_bhb, CPUINFO_scf(%rsp) - jz .L_skip_bhb - ALTERNATIVE_2 "", \ - "call clear_bhb_loops", X86_SPEC_BHB_LOOPS, \ - "call clear_bhb_tsx", X86_SPEC_BHB_TSX -.L_skip_bhb: + .macro VMX_BHB_SEQ fn:req + DO_COND_BHB_SEQ \fn scf=CPUINFO_scf(%rsp) + .endm + ALTERNATIVE_2 "", \ + "VMX_BHB_SEQ fn=clear_bhb_loops", X86_SPEC_BHB_LOOPS, \ + "VMX_BHB_SEQ fn=clear_bhb_tsx", X86_SPEC_BHB_TSX ALTERNATIVE "lfence", "", X86_SPEC_NO_LFENCE_ENTRY_VMX /* WARNING! `ret`, `call *`, `jmp *` not safe before this point. */ diff --git a/xen/arch/x86/include/asm/spec_ctrl_asm.h b/xen/arch/x86/include/asm/spec_ctrl_asm.h index 729a830411eb..559dad88f967 100644 --- a/xen/arch/x86/include/asm/spec_ctrl_asm.h +++ b/xen/arch/x86/include/asm/spec_ctrl_asm.h @@ -92,6 +92,21 @@ .L\@_skip: .endm +.macro DO_COND_BHB_SEQ fn:req, scf=%bl +/* + * Requires SCF (defaults to %rbx), fn=clear_bhb_{loops,tsx} + * Clobbers %rax, %rcx + * + * Conditionally use a BHB clearing software sequence. + */ + testb $SCF_entry_bhb, \scf + jz .L\@_skip_bhb + + call \fn + +.L\@_skip_bhb: +.endm + .macro DO_OVERWRITE_RSB tmp=rax, xu /* * Requires nothing @@ -277,12 +292,9 @@ * Clear the BHB to mitigate BHI. Used on eIBRS parts, and uses RETs * itself so must be after we've perfomed all the RET-safety we can. */ - testb $SCF_entry_bhb, %bl - jz .L\@_skip_bhb - ALTERNATIVE_2 "", \ - "call clear_bhb_loops", X86_SPEC_BHB_LOOPS, \ - "call clear_bhb_tsx", X86_SPEC_BHB_TSX -.L\@_skip_bhb: + ALTERNATIVE_2 "", \ + "DO_COND_BHB_SEQ clear_bhb_loops", X86_SPEC_BHB_LOOPS, \ + "DO_COND_BHB_SEQ clear_bhb_tsx", X86_SPEC_BHB_TSX ALTERNATIVE "lfence", "", X86_SPEC_NO_LFENCE_ENTRY_PV .endm @@ -322,12 +334,9 @@ ALTERNATIVE "", __stringify(DO_SPEC_CTRL_ENTRY maybexen=1), \ X86_FEATURE_SC_MSR_PV - testb $SCF_entry_bhb, %bl - jz .L\@_skip_bhb - ALTERNATIVE_2 "", \ - "call clear_bhb_loops", X86_SPEC_BHB_LOOPS, \ - "call clear_bhb_tsx", X86_SPEC_BHB_TSX -.L\@_skip_bhb: + ALTERNATIVE_2 "", \ + "DO_COND_BHB_SEQ clear_bhb_loops", X86_SPEC_BHB_LOOPS, \ + "DO_COND_BHB_SEQ clear_bhb_tsx", X86_SPEC_BHB_TSX ALTERNATIVE "lfence", "", X86_SPEC_NO_LFENCE_ENTRY_INTR .endm @@ -433,13 +442,9 @@ * Clear the BHB to mitigate BHI. Used on eIBRS parts, and uses RETs * itself so must be after we've perfomed all the RET-safety we can. */ - testb $SCF_entry_bhb, %bl - jz .L\@_skip_bhb - - ALTERNATIVE_2 "", \ - "call clear_bhb_loops", X86_SPEC_BHB_LOOPS, \ - "call clear_bhb_tsx", X86_SPEC_BHB_TSX -.L\@_skip_bhb: + ALTERNATIVE_2 "", \ + "DO_COND_BHB_SEQ clear_bhb_loops", X86_SPEC_BHB_LOOPS, \ + "DO_COND_BHB_SEQ clear_bhb_tsx", X86_SPEC_BHB_TSX lfence .endm
Now that alternatives can fix up call displacements even when they're not the first instruction of the replacement, move the SCF_entry_bhb conditional inside the replacement block. This removes a conditional branch from the fastpaths of BHI-unaffected hardware. Signed-off-by: Andrew Cooper <andrew.cooper3@citrix.com> --- CC: Jan Beulich <JBeulich@suse.com> CC: Roger Pau Monné <roger.pau@citrix.com> --- xen/arch/x86/hvm/vmx/entry.S | 12 +++---- xen/arch/x86/include/asm/spec_ctrl_asm.h | 43 +++++++++++++----------- 2 files changed, 30 insertions(+), 25 deletions(-)