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

Mathias Krause
2025-11-15T05:09:40Z

On 14.11.25 21:46, Sean Christopherson wrote:
> On Fri, Nov 14, 2025, Mathias Krause wrote:
>> [...]
>>
>> 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.

Well, it doesn't rely on IBT_TRACKER_WAIT_FOR_ENDBRANCH being a single
bit, but a value with all ones.

Essentially what I wanted to do was (in pseudo-code):

if cet_u.tracker_state == WAIT_FOR_ENDBRANCH:
cet_u.tracker_state = IDLE
wrmsr(MSR_IA32_U_CET, cet_u)

I tried to make clear that it's the IBT tracker state, the code is
interested in. But yeah, it's a single bit with only two states so just
testing and toggling that single bit is fine.

Initially I thought that setting CET_IBT_SUPPRESS is needed was well,
but it's not. It's related to the "legacy handling" which is yet another
can of worms we haven't opened yet ;)

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