linux-kernel - Re: [PATCH] KVM: X86: avoid meaningless kvm_apicv

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [day] [month] [year] [list]

Message-ID: <7c0aedea-36dc-bd84-b7ba-1aa6d1cceb11@redhat.com>
Date:   Sat, 14 Mar 2020 12:31:28 +0100
From:   Paolo Bonzini <pbonzini@...hat.com>
To:     Vitaly Kuznetsov <vkuznets@...hat.com>,
        linmiaohe <linmiaohe@...wei.com>
Cc:     kvm@...r.kernel.org, linux-kernel@...r.kernel.org, x86@...nel.org,
        rkrcmar@...hat.com, sean.j.christopherson@...el.com,
        wanpengli@...cent.com, jmattson@...gle.com, joro@...tes.org,
        tglx@...utronix.de, mingo@...hat.com, bp@...en8.de, hpa@...or.com
Subject: Re: [PATCH] KVM: X86: avoid meaningless kvm_apicv_activated() check

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