Re: [PATCH v6 2/9] x86/bhi: Make clear_bhb_loop() effective on newer CPUs

[David Laight <david.laight.linux@gmail.com>]

On Sun, 14 Dec 2025 10:38:27 -0800
Pawan Gupta <pawan.kumar.gupta@linux.intel.com> wrote:

> On Wed, Dec 10, 2025 at 01:35:42PM +0000, David Laight wrote:
> > On Wed, 10 Dec 2025 14:31:31 +0200
> > Nikolay Borisov <nik.borisov@suse.com> wrote:
> >   
> > > On 2.12.25 г. 8:19 ч., Pawan Gupta wrote:  
> > > > As a mitigation for BHI, clear_bhb_loop() executes branches that overwrites
> > > > the Branch History Buffer (BHB). On Alder Lake and newer parts this
> > > > sequence is not sufficient because it doesn't clear enough entries. This
> > > > was not an issue because these CPUs have a hardware control (BHI_DIS_S)
> > > > that mitigates BHI in kernel.
> > > > 
> > > > BHI variant of VMSCAPE requires isolating branch history between guests and
> > > > userspace. Note that there is no equivalent hardware control for userspace.
> > > > To effectively isolate branch history on newer CPUs, clear_bhb_loop()
> > > > should execute sufficient number of branches to clear a larger BHB.
> > > > 
> > > > Dynamically set the loop count of clear_bhb_loop() such that it is
> > > > effective on newer CPUs too. Use the hardware control enumeration
> > > > X86_FEATURE_BHI_CTRL to select the appropriate loop count.
> > > > 
> > > > Suggested-by: Dave Hansen <dave.hansen@linux.intel.com>
> > > > Reviewed-by: Nikolay Borisov <nik.borisov@suse.com>
> > > > Signed-off-by: Pawan Gupta <pawan.kumar.gupta@linux.intel.com>    
> > > 
> > > nit: My RB tag is incorrect, while I did agree with Dave's suggestion to 
> > > have global variables for the loop counts I haven't' really seen the 
> > > code so I couldn't have given my RB on something which I haven't seen 
> > > but did agree with in principle.  
> > 
> > I thought the plan was to use global variables rather than ALTERNATIVE.
> > The performance of this code is dominated by the loop.  
> 
> Using globals was much more involved, requiring changes in atleast 3 files.
> The current ALTERNATIVE approach is much simpler and avoids additional
> handling to make sure that globals are set correctly for all mitigation
> modes of BHI and VMSCAPE.
> 
> [ BTW, I am travelling on a vacation and will be intermittently checking my
>   emails. ]
> 
> > I also found this code in arch/x86/net/bpf_jit_comp.c:
> > 	if (cpu_feature_enabled(X86_FEATURE_CLEAR_BHB_LOOP)) {
> > 		/* The clearing sequence clobbers eax and ecx. */
> > 		EMIT1(0x50); /* push rax */
> > 		EMIT1(0x51); /* push rcx */
> > 		ip += 2;
> > 
> > 		func = (u8 *)clear_bhb_loop;
> > 		ip += x86_call_depth_emit_accounting(&prog, func, ip);
> > 
> > 		if (emit_call(&prog, func, ip))
> > 			return -EINVAL;
> > 		EMIT1(0x59); /* pop rcx */
> > 		EMIT1(0x58); /* pop rax */
> > 	}
> > which appears to assume that only rax and rcx are changed.
> > Since all the counts are small, there is nothing stopping the code
> > using the 8-bit registers %al, %ah, %cl and %ch.  
> 
> Thanks for catching this.

I was trying to find where it was called from.
Failed to find the one on system call entry...

> > There are probably some schemes that only need one register.
> > eg two separate ALTERNATIVE blocks.  
> 
> Also, I think it is better to use a callee-saved register like rbx to avoid
> callers having to save/restore registers. Something like below:

I'm not sure.
%ax is the return value so can be 'trashed' by a normal function call.
But if the bpf code is saving %ax then it isn't expecting a normal call.
OTOH if you are going to save the register in clear_bhb_loop you might
as well use %ax to get the slightly shorter instructions for %al.
(I think 'movb' comes out shorter - as if it really matters.)

Definitely worth a comment that it must save all resisters.

I also wonder if it needs to setup a stack frame?
Again, the code is so slow it won't matter.

	David


> 
> diff --git a/arch/x86/entry/entry_64.S b/arch/x86/entry/entry_64.S
> index 9f6f4a7c5baf..ca4a34ce314a 100644
> --- a/arch/x86/entry/entry_64.S
> +++ b/arch/x86/entry/entry_64.S
> @@ -1535,11 +1535,12 @@ SYM_CODE_END(rewind_stack_and_make_dead)
>  SYM_FUNC_START(clear_bhb_loop)
>  	ANNOTATE_NOENDBR
>  	push	%rbp
> +	push	%rbx
>  	mov	%rsp, %rbp
>  
>  	/* loop count differs based on BHI_CTRL, see Intel's BHI guidance */
> -	ALTERNATIVE "movl $5,  %ecx; movl $5, %edx",	\
> -		    "movl $12, %ecx; movl $7, %edx", X86_FEATURE_BHI_CTRL
> +	ALTERNATIVE "movb $5,  %bl",	\
> +		    "movb $12, %bl", X86_FEATURE_BHI_CTRL
>  
>  	ANNOTATE_INTRA_FUNCTION_CALL
>  	call	1f
> @@ -1561,15 +1562,17 @@ SYM_FUNC_START(clear_bhb_loop)
>  	 * but some Clang versions (e.g. 18) don't like this.
>  	 */
>  	.skip 32 - 18, 0xcc
> -2:	movl	%edx, %eax
> +2:	ALTERNATIVE "movb $5, %bh",	\
> +		    "movb $7, %bh", X86_FEATURE_BHI_CTRL
>  3:	jmp	4f
>  	nop
> -4:	sub	$1, %eax
> +4:	sub	$1, %bh
>  	jnz	3b
> -	sub	$1, %ecx
> +	sub	$1, %bl
>  	jnz	1b
>  .Lret2:	RET
>  5:
> +	pop	%rbx
>  	pop	%rbp
>  	RET
>  SYM_FUNC_END(clear_bhb_loop)
> diff --git a/arch/x86/net/bpf_jit_comp.c b/arch/x86/net/bpf_jit_comp.c
> index c1ec14c55911..823b3f613774 100644
> --- a/arch/x86/net/bpf_jit_comp.c
> +++ b/arch/x86/net/bpf_jit_comp.c
> @@ -1593,11 +1593,6 @@ static int emit_spectre_bhb_barrier(u8 **pprog, u8 *ip,
>  	u8 *func;
>  
>  	if (cpu_feature_enabled(X86_FEATURE_CLEAR_BHB_LOOP)) {
> -		/* The clearing sequence clobbers eax and ecx. */
> -		EMIT1(0x50); /* push rax */
> -		EMIT1(0x51); /* push rcx */
> -		ip += 2;
> -
>  		func = (u8 *)clear_bhb_loop;
>  		ip += x86_call_depth_emit_accounting(&prog, func, ip);
>  
> @@ -1605,8 +1600,6 @@ static int emit_spectre_bhb_barrier(u8 **pprog, u8 *ip,
>  			return -EINVAL;
>  		/* Don't speculate past this until BHB is cleared */
>  		EMIT_LFENCE();
> -		EMIT1(0x59); /* pop rcx */
> -		EMIT1(0x58); /* pop rax */
>  	}
>  	/* Insert IBHF instruction */
>  	if ((cpu_feature_enabled(X86_FEATURE_CLEAR_BHB_LOOP) &&