On 25/02/20 13:43, Vitaly Kuznetsov wrote:
> 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;
Yes, I got to the same conclusion before seeing you message. Another
possibility is to use cmpxchg, which I slightly prefer because if there
are multiple concurrent updates it has some possibilities of avoiding
the atomic operation and consequent cacheline bouncing. I've sent a patch.
Paolo
