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
