Re: [PATCH v3 12/14] x86/svm: Cleanup LBRV tests

[Shivansh Dhiman <shivansh.dhiman@amd.com>]

Hi Yosry,

On 13-11-2025 20:29, Yosry Ahmed wrote:
> On Thu, Nov 13, 2025 at 05:28:11PM +0530, Shivansh Dhiman wrote:
>> Hi Yosry,
>>
>> I tested this on EPYC-Turin and found that some tests seem to be a bit flaky.
>> See below.
> 
> Which ones? I was also running the tests on EPYC-Turin.

Most of the nested LBRV tests had this issue. I checked your other patch to fix
this. I tested it and it does fixes it for me. Thanks.

>>
>> On 11-11-2025 04:56, Yosry Ahmed wrote:
>>> @@ -3058,55 +3041,64 @@ u64 dbgctl;
>>>  
>>>  static void svm_lbrv_test_guest1(void)
>>>  {
>>> +	u64 from_ip, to_ip;
>>> +
>>>  	/*
>>>  	 * This guest expects the LBR to be already enabled when it starts,
>>>  	 * it does a branch, and then disables the LBR and then checks.
>>>  	 */
>>> +	dbgctl = rdmsr(MSR_IA32_DEBUGCTLMSR);
>>> +	TEST_EXPECT_EQ(dbgctl, DEBUGCTLMSR_LBR);
>>
>> This TEST_EXPECT_EQ is run when LBR is enabled, causing it to change last
>> branch. I tried to move it below wrmsr(MSR_IA32_DEBUGCTLMSR, 0) and it works
>> fine that way.
> 
> It shouldn't matter though because we execute the branch we care about
> after TEST_EXPECT_EQ(), it's DO_BRANCH(guest_branch0) below. Is it
> possible that the compiler reordered them for some reason?
> 
> I liked having the check here because it's easier to follow when the
> checks are done at their logical place rather than delayed after
> wrmsr().

Correct, that should be the natural order.

>>
>>>  
>>>  	DO_BRANCH(guest_branch0);
>>>  
>>> -	dbgctl = rdmsr(MSR_IA32_DEBUGCTLMSR);
>>> +	/* Disable LBR before the checks to avoid changing the last branch */
>>>  	wrmsr(MSR_IA32_DEBUGCTLMSR, 0);> +	dbgctl = rdmsr(MSR_IA32_DEBUGCTLMSR);
>>> +	TEST_EXPECT_EQ(dbgctl, 0);
>>>  
>>> -	if (dbgctl != DEBUGCTLMSR_LBR)
>>> -		asm volatile("ud2\n");
>>> -	if (rdmsr(MSR_IA32_DEBUGCTLMSR) != 0)
>>> -		asm volatile("ud2\n");
>>> +	get_lbr_ips(&from_ip, &to_ip);
>>> +	TEST_EXPECT_EQ((u64)&guest_branch0_from, from_ip);
>>> +	TEST_EXPECT_EQ((u64)&guest_branch0_to, to_ip);
>>>  
>>> -	GUEST_CHECK_LBR(&guest_branch0_from, &guest_branch0_to);
>>>  	asm volatile ("vmmcall\n");
>>>  }
>>>  
>>>  static void svm_lbrv_test_guest2(void)
>>>  {
>>> +	u64 from_ip, to_ip;
>>> +
>>>  	/*
>>>  	 * This guest expects the LBR to be disabled when it starts,
>>>  	 * enables it, does a branch, disables it and then checks.
>>>  	 */
>>> -
>>> -	DO_BRANCH(guest_branch1);
>>>  	dbgctl = rdmsr(MSR_IA32_DEBUGCTLMSR);
>>> +	TEST_EXPECT_EQ(dbgctl, 0);
>>>  
>>> -	if (dbgctl != 0)
>>> -		asm volatile("ud2\n");
>>> +	DO_BRANCH(guest_branch1);
>>>  
>>> -	GUEST_CHECK_LBR(&host_branch2_from, &host_branch2_to);
>>> +	get_lbr_ips(&from_ip, &to_ip);
>>> +	TEST_EXPECT_EQ((u64)&host_branch2_from, from_ip);
>>> +	TEST_EXPECT_EQ((u64)&host_branch2_to, to_ip);
>>>  
>>>  	wrmsr(MSR_IA32_DEBUGCTLMSR, DEBUGCTLMSR_LBR);
>>>  	dbgctl = rdmsr(MSR_IA32_DEBUGCTLMSR);
>>> +	TEST_EXPECT_EQ(dbgctl, DEBUGCTLMSR_LBR);
>>
>> Same thing here as well.
>>
>>> +
>>>  	DO_BRANCH(guest_branch2);
>>>  	wrmsr(MSR_IA32_DEBUGCTLMSR, 0);
>>>  
>>> -	if (dbgctl != DEBUGCTLMSR_LBR)
>>> -		asm volatile("ud2\n");
>>> -	GUEST_CHECK_LBR(&guest_branch2_from, &guest_branch2_to);
>>> +	get_lbr_ips(&from_ip, &to_ip);
>>> +	TEST_EXPECT_EQ((u64)&guest_branch2_from, from_ip);
>>> +	TEST_EXPECT_EQ((u64)&guest_branch2_to, to_ip);
>>>  
>>>  	asm volatile ("vmmcall\n");
>>>  }
>> Reviewed-by: Shivansh Dhiman <shivansh.dhiman@amd.com>
>>
>> Other tests look good to me, and work fine.
>>
>> Tested-by: Shivansh Dhiman <shivansh.dhiman@amd.com>
> 
> Thanks!