Message-ID: <20251215210553.5ab5b674@pumpkin>
Date: Mon, 15 Dec 2025 21:05:53 +0000
From: David Laight <david.laight.linux@...il.com>
To: Pawan Gupta <pawan.kumar.gupta@...ux.intel.com>
Cc: Nikolay Borisov <nik.borisov@...e.com>, x86@...nel.org, David Kaplan
 <david.kaplan@....com>, "H. Peter Anvin" <hpa@...or.com>, Josh Poimboeuf
 <jpoimboe@...nel.org>, Sean Christopherson <seanjc@...gle.com>, Paolo
 Bonzini <pbonzini@...hat.com>, Borislav Petkov <bp@...en8.de>, Dave Hansen
 <dave.hansen@...ux.intel.com>, linux-kernel@...r.kernel.org,
 kvm@...r.kernel.org, Asit Mallick <asit.k.mallick@...el.com>, Tao Zhang
 <tao1.zhang@...el.com>
Subject: Re: [PATCH v6 2/9] x86/bhi: Make clear_bhb_loop() effective on
 newer CPUs

On Mon, 15 Dec 2025 10:01:36 -0800
Pawan Gupta <pawan.kumar.gupta@...ux.intel.com> wrote:

> On Sun, Dec 14, 2025 at 07:02:33PM +0000, David Laight wrote:
> > On Sun, 14 Dec 2025 10:38:27 -0800
> > Pawan Gupta <pawan.kumar.gupta@...ux.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@...e.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@...ux.intel.com>
> > > > > > Reviewed-by: Nikolay Borisov <nik.borisov@...e.com>
> > > > > > Signed-off-by: Pawan Gupta <pawan.kumar.gupta@...ux.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...  
> 
> The macro CLEAR_BRANCH_HISTORY calls clear_bhb_loop() at system call entry.

I didn't look very hard :-)

> 
> > > > 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.  
> 
> BHB clear sequence is executed at the end of the BPF JITted code, and %rax
> is likely the return value of the BPF program. So, saving/restoring %rax
> around the sequence makes sense to me.
> 
> > 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.)  
> 
> %rbx is a callee-saved register so it felt more intuitive to save/restore
> it in clear_bhb_loop(). But, I can use %ax if you feel strongly.

If you are going to save a register it might as well be %ax.
Otherwise someone will wonder why you picked a different one.

> 
> > Definitely worth a comment that it must save all resisters.  
> 
> Yes, will add a comment.
> 
> > I also wonder if it needs to setup a stack frame?  
> 
> I don't know if thats necessary, objtool doesn't complain because
> clear_bhb_loop() is marked STACK_FRAME_NON_STANDARD.

In some senses it is a leaf functions - and the compiler doesn't create
stack frames for those (by default).

Provided objtool isn't confused by all the call instructions it probably
doesn't matter.

	David

> 
> > Again, the code is so slow it won't matter.
> > 
> > 	David  

Powered by blists - more mailing lists