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!
