From: Maxim Levitsky <mlevitsk@redhat.com>
To: Paolo Bonzini <pbonzini@redhat.com>, kvm@vger.kernel.org
Cc: "open list:X86 ARCHITECTURE (32-BIT AND 64-BIT)"
<linux-kernel@vger.kernel.org>, Jim Mattson <jmattson@google.com>,
Joerg Roedel <joro@8bytes.org>, Borislav Petkov <bp@alien8.de>,
Vitaly Kuznetsov <vkuznets@redhat.com>,
Wanpeng Li <wanpengli@tencent.com>,
Thomas Gleixner <tglx@linutronix.de>,
"H. Peter Anvin" <hpa@zytor.com>, Ingo Molnar <mingo@redhat.com>,
"maintainer:X86 ARCHITECTURE (32-BIT AND 64-BIT)"
<x86@kernel.org>, Sean Christopherson <seanjc@google.com>
Subject: Re: [PATCH v2 5/8] KVM: x86: APICv: fix race in kvm_request_apicv_update on SVM
Date: Tue, 27 Jul 2021 16:22:40 +0300 [thread overview]
Message-ID: <654e0b6aa25c15ef8907813b6ab17681c7f12f5f.camel@redhat.com> (raw)
In-Reply-To: <e8acf99c-0e3b-f0cc-c8ad-53074420d734@redhat.com>
On Tue, 2021-07-27 at 00:34 +0200, Paolo Bonzini wrote:
> On 13/07/21 16:20, Maxim Levitsky wrote:
> > + mutex_lock(&vcpu->kvm->apicv_update_lock);
> > +
> > vcpu->arch.apicv_active = kvm_apicv_activated(vcpu->kvm);
> > kvm_apic_update_apicv(vcpu);
> > static_call(kvm_x86_refresh_apicv_exec_ctrl)(vcpu);
> > @@ -9246,6 +9248,8 @@ void kvm_vcpu_update_apicv(struct kvm_vcpu *vcpu)
> > */
> > if (!vcpu->arch.apicv_active)
> > kvm_make_request(KVM_REQ_EVENT, vcpu);
> > +
> > + mutex_unlock(&vcpu->kvm->apicv_update_lock);
>
> Does this whole piece of code need the lock/unlock? Does it work and/or
> make sense to do the unlock immediately after mutex_lock()? This makes
> it clearer that the mutex is being to synchronize against the requestor.
Yes, I do need to hold the mutex for the whole duration.
The requester does the following:
1. Take the mutex
2. Kick all the vCPUs out of the guest mode with KVM_REQ_EVENT
At that point all these vCPUs will be (or soon will be) stuck on the mutex
and guaranteed to be outside of the guest mode.
which is exactly what I need to avoid them entering the guest
mode as long as the AVIC's memslot state is not up to date.
3. Update kvm->arch.apicv_inhibit_reasons. I removed the cmpchg loop
since it is now protected under the lock anyway.
This step doesn't have to be done at this point, but should be done while mutex is held
so that there is no need to cmpchg and such.
This itself isn't the justification for the mutex.
4. Update the memslot
5. Release the mutex.
Only now all other vCPUs are permitted to enter the guest mode again
(since only now the memslot is up to date)
and they will also update their per-vcpu AVIC enablement prior to entering it.
I think it might be possible to avoid the mutex, but I am not sure if this is worth it:
First of all, the above sync sequence is only really needed when we enable AVIC.
(Because from the moment we enable the memslot and to the moment the vCPU enables the AVIC,
it must not be in guest mode as otherwise it will access the dummy page in the memslot
without VMexit, instead of going through AVIC vmexit/acceleration.
The other way around is OK. IF we disable the memslot, and a vCPU still has a enabled AVIC,
it will just get a page fault which will be correctly emulated as APIC read/write by
the MMIO page fault.
If I had a guarantee that when I enable the memslot (either as it done today or
using the kvm_zap_gfn_range (which I strongly prefer), would always raise a TLB
flush request on all vCPUs, then I could (ab)use that request to update local
AVIC state.
Or I can just always check if local AVIC state matches the memslot and update
if it doesn't prior to guest mode entry.
I still think I would prefer a mutex to be 100% sure that there are no races,
since the whole AVIC disablement isn't something that is done often anyway.
Best regards,
Maxim Levitsky
>
> > diff --git a/virt/kvm/kvm_main.c b/virt/kvm/kvm_main.c
> > index ed4d1581d502..ba5d5d9ebc64 100644
> > --- a/virt/kvm/kvm_main.c
> > +++ b/virt/kvm/kvm_main.c
> > @@ -943,6 +943,7 @@ static struct kvm *kvm_create_vm(unsigned long type)
> > mutex_init(&kvm->irq_lock);
> > mutex_init(&kvm->slots_lock);
> > mutex_init(&kvm->slots_arch_lock);
> > + mutex_init(&kvm->apicv_update_lock);
> > INIT_LIST_HEAD(&kvm->devices);
> >
> > BUILD_BUG_ON(KVM_MEM_SLOTS_NUM > SHRT_MAX);
> >
>
> Please add comments above fields that are protected by this lock
> (anything but apicv_inhibit_reasons?), and especially move it to kvm->arch.
I agree, I will do this.
>
> Paolo
next prev parent reply other threads:[~2021-07-27 13:27 UTC|newest]
Thread overview: 32+ messages / expand[flat|nested] mbox.gz Atom feed top
2021-07-13 14:20 [PATCH v2 0/8] My AVIC patch queue Maxim Levitsky
2021-07-13 14:20 ` [PATCH v2 1/8] KVM: SVM: svm_set_vintr don't warn if AVIC is active but is about to be deactivated Maxim Levitsky
2021-07-13 14:20 ` [PATCH v2 2/8] KVM: SVM: tweak warning about enabled AVIC on nested entry Maxim Levitsky
2021-07-13 14:20 ` [PATCH v2 3/8] KVM: SVM: use vmcb01 in svm_refresh_apicv_exec_ctrl Maxim Levitsky
2021-07-13 14:20 ` [PATCH v2 4/8] KVM: x86: APICv: drop immediate APICv disablement on current vCPU Maxim Levitsky
2021-07-13 14:20 ` [PATCH v2 5/8] KVM: x86: APICv: fix race in kvm_request_apicv_update on SVM Maxim Levitsky
2021-07-26 22:34 ` Paolo Bonzini
2021-07-27 13:22 ` Maxim Levitsky [this message]
2021-07-13 14:20 ` [PATCH v2 6/8] KVM: SVM: add warning for mistmatch between AVIC state and AVIC access page state Maxim Levitsky
2021-07-13 14:20 ` [PATCH v2 7/8] KVM: SVM: call avic_vcpu_load/avic_vcpu_put when enabling/disabling AVIC Maxim Levitsky
2021-07-13 14:20 ` [PATCH v2 8/8] KVM: x86: hyper-v: Deactivate APICv only when AutoEOI feature is in use Maxim Levitsky
2021-07-18 12:13 ` Maxim Levitsky
2021-07-19 7:47 ` Vitaly Kuznetsov
2021-07-19 9:00 ` Maxim Levitsky
2021-07-19 9:23 ` Vitaly Kuznetsov
2021-07-19 9:58 ` Maxim Levitsky
2021-07-19 18:49 ` Sean Christopherson
2021-07-20 9:40 ` Maxim Levitsky
2021-07-22 9:12 ` KVM's support for non default APIC base Maxim Levitsky
2021-08-02 9:20 ` Maxim Levitsky
2021-08-06 21:55 ` Sean Christopherson
2021-08-09 9:40 ` Maxim Levitsky
2021-08-09 15:57 ` Sean Christopherson
2021-08-09 16:47 ` Jim Mattson
2021-08-10 20:42 ` Maxim Levitsky
2021-07-22 17:35 ` [PATCH v2 8/8] KVM: x86: hyper-v: Deactivate APICv only when AutoEOI feature is in use Maxim Levitsky
2021-07-22 19:06 ` Sean Christopherson
2021-07-27 13:05 ` Maxim Levitsky
2021-07-27 17:48 ` Ben Gardon
2021-07-27 18:17 ` Sean Christopherson
2021-07-29 14:10 ` Maxim Levitsky
2021-07-26 17:24 ` [PATCH v2 0/8] My AVIC patch queue Paolo Bonzini
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=654e0b6aa25c15ef8907813b6ab17681c7f12f5f.camel@redhat.com \
--to=mlevitsk@redhat.com \
--cc=bp@alien8.de \
--cc=hpa@zytor.com \
--cc=jmattson@google.com \
--cc=joro@8bytes.org \
--cc=kvm@vger.kernel.org \
--cc=linux-kernel@vger.kernel.org \
--cc=mingo@redhat.com \
--cc=pbonzini@redhat.com \
--cc=seanjc@google.com \
--cc=tglx@linutronix.de \
--cc=vkuznets@redhat.com \
--cc=wanpengli@tencent.com \
--cc=x86@kernel.org \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.