Re: [PATCH] KVM: X86: avoid meaningless kvm_apicv_activated() check

[linmiaohe]

Vitaly Kuznetsov <vkuznets@xxxxxxxxxx> writes:
>linmiaohe <linmiaohe@xxxxxxxxxx> writes:
>
>> From: Miaohe Lin <linmiaohe@xxxxxxxxxx>
>>
>> After test_and_set_bit() for kvm->arch.apicv_inhibit_reasons, we will 
>> always get false when calling kvm_apicv_activated() because it's sure 
>> apicv_inhibit_reasons do not equal to 0.
>>
>> Signed-off-by: Miaohe Lin <linmiaohe@xxxxxxxxxx>
>> ---
>>  arch/x86/kvm/x86.c | 3 +--
>>  1 file changed, 1 insertion(+), 2 deletions(-)
>>
>> diff --git a/arch/x86/kvm/x86.c b/arch/x86/kvm/x86.c index 
>> ddcc51b89e2c..fa62dcb0ed0c 100644
>> --- a/arch/x86/kvm/x86.c
>> +++ b/arch/x86/kvm/x86.c
>> @@ -8018,8 +8018,7 @@ void kvm_request_apicv_update(struct kvm *kvm, bool activate, ulong bit)
>>  		    !kvm_apicv_activated(kvm))
>>  			return;
>>  	} else {
>> -		if (test_and_set_bit(bit, &kvm->arch.apicv_inhibit_reasons) ||
>> -		    kvm_apicv_activated(kvm))
>> +		if (test_and_set_bit(bit, &kvm->arch.apicv_inhibit_reasons))
>>  			return;
>>  	}
>
>This seems to be correct in a sense that we are not really protected against concurrent modifications of 'apicv_inhibit_reasons' (like what if 'apicv_inhibit_reasons' gets modified right after we've checked 'kvm_apicv_activated(kvm)').

Yes, there might be a race window. But this looks benign as we recalculate kvm_apicv_activated() when we proceed with KVM_REQ_APICV_UPDATE.

>
>The function, however, still gives a flase impression it is somewhat protected against concurent modifications. Like what are these
>test_and_{set,clear}_bit() for?

Yes, I think so too. And also test_and_{set,clear}_bit() checks wheather the requested bit is {set,clear} to the requested state.

>
>If I'm not mistaken, the logic this function was supposed to implement
>is: change the requested bit to the requested state and, if
>kvm_apicv_activated() changed (we set the first bit or cleared the last), proceed with KVM_REQ_APICV_UPDATE. What if we re-write it like
>
>diff --git a/arch/x86/kvm/x86.c b/arch/x86/kvm/x86.c index 2103101eca78..b97b8ff4a789 100644
>--- a/arch/x86/kvm/x86.c
>+++ b/arch/x86/kvm/x86.c
>@@ -8027,19 +8027,19 @@ EXPORT_SYMBOL_GPL(kvm_vcpu_update_apicv);
>  */
> void kvm_request_apicv_update(struct kvm *kvm, bool activate, ulong bit)  {
>+       bool apicv_was_activated = kvm_apicv_activated(kvm);
>+
>        if (!kvm_x86_ops->check_apicv_inhibit_reasons ||
>            !kvm_x86_ops->check_apicv_inhibit_reasons(bit))
>                return;
> 
>-       if (activate) {
>-               if (!test_and_clear_bit(bit, &kvm->arch.apicv_inhibit_reasons) ||
>-                   !kvm_apicv_activated(kvm))
>-                       return;
>-       } else {
>-               if (test_and_set_bit(bit, &kvm->arch.apicv_inhibit_reasons) ||
>-                   kvm_apicv_activated(kvm))
>-                       return;
>-       }
>+       if (activate)
>+               clear_bit(bit, &kvm->arch.apicv_inhibit_reasons);
>+       else
>+               set_bit(bit, &kvm->arch.apicv_inhibit_reasons);
>+
>+       if (kvm_apicv_activated(kvm) == apicv_was_activated)
>+               return;
> 
>        trace_kvm_apicv_update_request(activate, bit);
>        if (kvm_x86_ops->pre_update_apicv_exec_ctrl)
>
>Is this equal?
>

Looks good. I think this version also improves the readability. Many thanks for your advice and review!