Re: [RFC PATCH v1 4/4] KVM: selftests: Add bus lock exit test

[Manali Shukla <manali.shukla@amd.com>]

Hi Sean,

Thank you for reviewing my patches.

On 8/26/2024 9:36 PM, Sean Christopherson wrote:
> On Mon, Aug 26, 2024, Manali Shukla wrote:
>>>> +struct buslock_test {
>>>> +	unsigned char pad[126];
>>>> +	atomic_long_t val;
>>>> +} __packed;
>>>> +
>>>> +struct buslock_test test __cacheline_aligned;
>>>> +
>>>> +static __always_inline void buslock_atomic_add(int i, atomic_long_t *v)
>>>> +{
>>>> +	asm volatile(LOCK_PREFIX "addl %1,%0"
>>>> +		     : "+m" (v->counter)
>>>> +		     : "ir" (i) : "memory");
>>>> +}
>>>> +
>>>> +static void buslock_add(void)
>>>> +{
>>>> +	/*
>>>> +	 * Increment a cache unaligned variable atomically.
>>>> +	 * This should generate a bus lock exit.
>>>
>>> So... this test doesn't actually verify that a bus lock exit occurs.  The userspace
>>> side will eat an exit if one occurs, but there's literally not a single TEST_ASSERT()
>>> in here.
>>
>> Agreed, How about doing following?
>>
>> +       for (;;) {
>> +               struct ucall uc;
>> +
>> +               vcpu_run(vcpu);
>> +
>> +               if (run->exit_reason == KVM_EXIT_IO) {
>> +                       switch (get_ucall(vcpu, &uc)) {
>> +                       case UCALL_ABORT:
>> +                               REPORT_GUEST_ASSERT(uc);
>> +                               /* NOT REACHED */
>> +                       case UCALL_SYNC:
>> +                               break;
>> +                       case UCALL_DONE:
>> +                               goto done;
>> +                       default:
>> +                               TEST_FAIL("Unknown ucall 0x%lx.", uc.cmd);
>> +                       }
>> +               }
>> +
>> +               TEST_ASSERT_KVM_EXIT_REASON(vcpu, KVM_EXIT_X86_BUS_LOCK);
> 
> I doubt this works, the UCALL_SYNC above will fallthrough to this assert.  I
> assume run->exit_reason needs a continue for UCALL_SYNC.
>

I agree, there should be a continue for UCALL_SYNC in place of break. I will
correct it in V2. 

I didn't observe this issue because UCALL_SYNC is invoked, when GUEST_SYNC() is
called from the guest code. Since GUEST_SYNC() is not present in the guest
code used in bus lock test case, UCALL_SYNC was never triggered.
 
>> +               TEST_ASSERT_EQ(run->flags, KVM_RUN_X86_BUS_LOCK);
>> +               run->flags &= ~KVM_RUN_X86_BUS_LOCK;
>
> No need, KVM should clear the flag if the exit isn't due to a bus lock.

Sure I will remove this.

> 
>> +               run->exit_reason = 0;
> 
> Again, no need, KVM should take care of resetting exit_reason.

Ack.

> 
>> +       }
>>

- Manali