Re: [kvm-unit-tests PATCH v3 01/17] x86/run_in_user: Add an "end branch" marker on the user_mode destination

[Sean Christopherson <seanjc@google.com>]

On Fri, Nov 14, 2025, Mathias Krause wrote:
> On 14.11.25 01:12, Sean Christopherson wrote:
> > Add an endbr64 at the user_mode "entry point" so that run_in_user() can be
> > used when CET's Indirect Branch Tracking is enabled.
> 
> I don't think that's needed, as 'user_mode' is branched to via IRETQ and
> that isn't covered by IBT -- nor is any other RET instruction.
> 
> > 
> > Signed-off-by: Sean Christopherson <seanjc@google.com>
> > ---
> >  lib/x86/usermode.c | 3 +++
> >  1 file changed, 3 insertions(+)
> > 
> > diff --git a/lib/x86/usermode.c b/lib/x86/usermode.c
> > index c3ec0ad7..f4ba0af4 100644
> > --- a/lib/x86/usermode.c
> > +++ b/lib/x86/usermode.c
> > @@ -68,6 +68,9 @@ uint64_t run_in_user(usermode_func func, unsigned int fault_vector,
> >  			"iretq\n"
> >  
> >  			"user_mode:\n\t"
> > +#ifdef __x86_64__
> > +			"endbr64\n\t"
> > +#endif
> 
> The thing is, this ENDBR64 is actually masking a missing cleanup step in
> the IBT tests. The first failing IBT test will make the CET_U IBT
> tracker state get stuck in the WAIT_FOR_ENDBRANCH state. This means,
> every time we return to userland (and thereby implicitly switching to
> the CET_U state again), it wants to see an ENDBR64 first or it will
> directly trigger a #CP(3) again. This ENDBR64 will please that demand
> and make it transition to IDLE and allow executing the test. However,
> it's really the old test that should have fixed the tracker state and
> not a blanket ENDBR64 when entering usermode.
> 
> Attached is a patch on top of [1] that does that but is, admitted, a
> little hacky and evolved. However, it shows that above ENDBR64 is, in
> fact, not needed.

You say hacky, I say clever and correct. :-)

> diff --git a/x86/cet.c b/x86/cet.c
> index 801d8da6e929..bcf1ca6d740a 100644
> --- a/x86/cet.c
> +++ b/x86/cet.c
> @@ -1,4 +1,3 @@
> -
>  #include "libcflat.h"
>  #include "x86/desc.h"
>  #include "x86/processor.h"
> @@ -192,6 +191,10 @@ static uint64_t cet_ibt_emulation(void)
>  #define CET_ENABLE_SHSTK	BIT(0)
>  #define CET_ENABLE_IBT		BIT(2)
>  #define CET_ENABLE_NOTRACK	BIT(4)
> +#define CET_IBT_SUPPRESS	BIT(10)
> +#define CET_IBT_TRACKER_STATE	BIT(11)
> +#define     IBT_TRACKER_IDLE			0
> +#define     IBT_TRACKER_WAIT_FOR_ENDBRANCH	BIT(11)

For this, I think it makes sense to diverge slightly from the SDM and just do

  #define CET_IBT_TRACKER_WAIT_FOR_ENDBRANCH	BIT(11)

because...

>  static void test_shstk(void)
>  {
> @@ -244,6 +247,22 @@ static void test_shstk(void)
>  	report(vector == GP_VECTOR, "MSR_IA32_PL3_SSP alignment test.");
>  }
>  
> +static void ibt_tracker_fixup(struct ex_regs *regs)
> +{
> +	u64 cet_u = rdmsr(MSR_IA32_U_CET);
> +
> +	/*
> +	 * Switch the IBT tracker state to IDLE to have a clean state for
> +	 * following tests.
> +	 */
> +	if ((cet_u & CET_IBT_TRACKER_STATE) == IBT_TRACKER_WAIT_FOR_ENDBRANCH) {
> +		cet_u &= ~IBT_TRACKER_WAIT_FOR_ENDBRANCH;

...this is quite weird/confusing.  It relies on CET_IBT_TRACKER_STATE being a
single bit, and "(x & y) == z)" is very un-idiomatic for a single bit.

> +		printf("CET: suppressing IBT WAIT_FOR_ENDBRANCH state at RIP: %lx\n",
> +		       regs->rip);
> +		wrmsr(MSR_IA32_U_CET, cet_u);
> +	}
> +}