All of lore.kernel.org
 help / color / mirror / Atom feed
From: Maxim Levitsky <mlevitsk@redhat.com>
To: Sean Christopherson <seanjc@google.com>
Cc: Paolo Bonzini <pbonzini@redhat.com>,
	kvm@vger.kernel.org, linux-kernel@vger.kernel.org,
	Alejandro Jimenez <alejandro.j.jimenez@oracle.com>,
	Suravee Suthikulpanit <suravee.suthikulpanit@amd.com>,
	Li RongQing <lirongqing@baidu.com>
Subject: Re: [PATCH v3 07/28] KVM: x86: Inhibit APIC memslot if x2APIC and AVIC are enabled
Date: Wed, 28 Sep 2022 20:40:37 +0300	[thread overview]
Message-ID: <2ca1b7c59e2abe661dd03309ad13eca7d692ff05.camel@redhat.com> (raw)
In-Reply-To: <YzR3PaZokwIPDoXb@google.com>

On Wed, 2022-09-28 at 16:33 +0000, Sean Christopherson wrote:
> On Wed, Sep 28, 2022, Maxim Levitsky wrote:
> > On Mon, 2022-09-26 at 17:00 +0000, Sean Christopherson wrote:
> > > Given the SRCU problem, I'd prefer to keep the management of the memslot in common
> > > code, even though I agree it's a bit silly.  And KVM_REQ_UNBLOCK is a perfect fit
> > > for dealing with the SRCU issue, i.e. handling this in AVIC code would require
> > > another hook on top of spreading the memslot management across x86 and SVM code.
> > 
> > OK, I am not going to argue about this. But what about at least not using an inhibit
> > bit for that but something else like a boolean, or maybe really add 'I am AVIC bit'
> > or rather something like vcpu->arch.apicv_type enum?
> > 
> > Or we can make SVM code just call a common function - just put these in a
> > function and call it from avic_set_virtual_apic_mode?
> 
> The issue is that kvm_vcpu_update_apicv() is called from kvm_lapic_set_base(),
> which is the one that may or may not hold SRCU.

Makes sense now.

> 
> > > > You are about to remove the KVM_REQ_UNBLOCK in other patch series.
> > > 
> > > No, KVM_REQ_UNHALT is being removed.  KVM_REQ_UNBLOCK needs to stay, although it
> > > has a rather weird name, e.g. KVM_REQ_WORK would probably be better.
> > 
> > Roger that!
> > And I guess lets rename it while we are at it.
> 
> I'll prep a patch.
> 
> > > > How about just raising KVM_REQ_APICV_UPDATE on current vCPU
> > > > and having a special case in kvm_vcpu_update_apicv of 
> > > > 
> > > > if (apic_access_memslot_enabled == false && apic_access_memslot_allocaed == true) {
> > > > 	drop srcu lock
> > > 
> > > This was my initial thought as well, but the issue is that SRCU may or may not be
> > > held, and so the unlock+lock would need to be conditional.  That's technically a
> > > solvable problem, as it's possible to detect if SRCU is held, but I really don't
> > > want to rely on kvm_vcpu.srcu_depth for anything other than proving that KVM doesn't
> > > screw up SRCU.
> > 
> > Why though? the KVM_REQ_APICV_UPDATE is only handled AFAIK in vcpu_enter_guest
> > which drops the srcu lock few lines afterwards, and therefore the
> > kvm_vcpu_update_apicv is always called with the lock held and it means that it
> > can drop it for the duration of slot update.
> > 
> > The original issue we had was that we tried to drop the srcu lock in 
> > 'kvm_set_apicv_inhibit' which indeed is called from various places,
> > with, or without the lock held.
> > 
> > Moving the memslot disable code to kvm_vcpu_update_apicv would actually solve
> > that, but it was not possible because kvm_vcpu_update_apicv is called
> > simultaneously on all vCPUs, and created various races, including toggling
> > the memslot twice.
> 
> As above, kvm_vcpu_update_apicv() can be called without SRCU held.  Oh, but that
> was a recent addition, commit 8fc9c7a3079e ("KVM: x86: Deactivate APICv on vCPU
> with APIC disabled").  I was wary of using KVM_REQ_APICV_UPDATE in kvm_lapic_set_base(),
> e.g. in case there was some dependency on updating _immediately, but since that's
> such a new addition I have no objection to switching to the request.
> 
> Similarly, is there a good reason for having nested_svm_vmexit() invoke
> kvm_vcpu_update_apicv() directly?  I'm confused by the "so that other vCPUs can
> start to benefit from it right away".  The nested inhibit is per-vCPU and so
> should only affect the current vCPU, no?  I.e. for all intents and purposes, using
> a request should be functionally equivalent.

It is kind of the other way around:

The mere fact of switching to vmcb02 *inhibits* the AVIC on the current vCPU,
but the AVIC inhibit is there only to set the is_running bits in the physid table
and in IOMMU to prevent its *peers* to try and send interrupts to it via AVIC.

It is the reason why APICv doesn't need it - the posted interrupts still work
just fine when a vCPU doens't use APICv, or uses a different posted interrupt vector
when it uses the nested APICv.

So it makes sense to remove that inhibit as soon as possible that the peers
could stop getting 'unaccellerated IPI' vmexits for nothing.


However back to the discussion, I don't think this is a problem.

We can just call both the kvm_vcpu_update_apicv() and a new function that
does the memslot disable from KVM_REQ_APICV_UPDATE, then 
plain kvm_vcpu_update_apicv() won't need to drop the srcu lock.

It is pretty much the same that you proposed, just instead of piggybacking on 
KVM_REQ_UNBLOCK, I proposed to piggyback on KVM_REQ_APICV_UPDATE.


Best regards,
	Maxim Levitsky


> 
> 	/*
> 	 * Un-inhibit the AVIC right away, so that other vCPUs can start
> 	 * to benefit from it right away.
> 	 */
> 	if (kvm_apicv_activated(vcpu->kvm))
> 		kvm_vcpu_update_apicv(vcpu);
> 



  reply	other threads:[~2022-09-28 17:40 UTC|newest]

Thread overview: 43+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2022-09-20 23:31 [PATCH v3 00/28] KVM: x86: AVIC and local APIC fixes+cleanups Sean Christopherson
2022-09-20 23:31 ` [PATCH v3 01/28] KVM: x86: Blindly get current x2APIC reg value on "nodecode write" traps Sean Christopherson
2022-09-20 23:31 ` [PATCH v3 02/28] KVM: x86: Purge "highest ISR" cache when updating APICv state Sean Christopherson
2022-09-20 23:31 ` [PATCH v3 03/28] KVM: SVM: Flush the "current" TLB when activating AVIC Sean Christopherson
2022-09-20 23:31 ` [PATCH v3 04/28] KVM: SVM: Process ICR on AVIC IPI delivery failure due to invalid target Sean Christopherson
2022-09-20 23:31 ` [PATCH v3 05/28] KVM: x86: Don't inhibit APICv/AVIC if xAPIC ID mismatch is due to 32-bit ID Sean Christopherson
2022-09-28  3:15   ` Alejandro Jimenez
2022-09-28  5:55     ` Maxim Levitsky
2022-09-28 16:51       ` Sean Christopherson
2022-09-28 17:51         ` Maxim Levitsky
2022-09-28 18:03           ` Sean Christopherson
2022-09-28 18:16             ` Maxim Levitsky
2022-09-28 20:44               ` Sean Christopherson
2022-09-28 20:50         ` Alejandro Jimenez
2022-09-20 23:31 ` [PATCH v3 06/28] KVM: x86: Move APIC access page helper to common x86 code Sean Christopherson
2022-09-20 23:31 ` [PATCH v3 07/28] KVM: x86: Inhibit APIC memslot if x2APIC and AVIC are enabled Sean Christopherson
2022-09-23 10:27   ` Maxim Levitsky
2022-09-26 17:00     ` Sean Christopherson
2022-09-28  6:21       ` Maxim Levitsky
2022-09-28 16:33         ` Sean Christopherson
2022-09-28 17:40           ` Maxim Levitsky [this message]
2022-09-28 22:35             ` Sean Christopherson
2022-09-20 23:31 ` [PATCH v3 08/28] KVM: SVM: Don't put/load AVIC when setting virtual APIC mode Sean Christopherson
2022-09-20 23:31 ` [PATCH v3 09/28] KVM: SVM: Replace "avic_mode" enum with "x2avic_enabled" boolean Sean Christopherson
2022-09-20 23:31 ` [PATCH v3 10/28] KVM: SVM: Compute dest based on sender's x2APIC status for AVIC kick Sean Christopherson
2022-09-20 23:31 ` [PATCH v3 11/28] KVM: SVM: Fix x2APIC Logical ID calculation for avic_kick_target_vcpus_fast Sean Christopherson
2022-09-20 23:31 ` [PATCH v3 12/28] Revert "KVM: SVM: Use target APIC ID to complete x2AVIC IRQs when possible" Sean Christopherson
2022-09-20 23:31 ` [PATCH v3 13/28] KVM: SVM: Document that vCPU ID == APIC ID in AVIC kick fastpatch Sean Christopherson
2022-09-20 23:31 ` [PATCH v3 14/28] KVM: SVM: Add helper to perform final AVIC "kick" of single vCPU Sean Christopherson
2022-09-20 23:31 ` [PATCH v3 15/28] KVM: x86: Explicitly skip optimized logical map setup if vCPU's LDR==0 Sean Christopherson
2022-09-20 23:31 ` [PATCH v3 16/28] KVM: x86: Explicitly track all possibilities for APIC map's logical modes Sean Christopherson
2022-09-20 23:31 ` [PATCH v3 17/28] KVM: x86: Skip redundant x2APIC logical mode optimized cluster setup Sean Christopherson
2022-09-20 23:31 ` [PATCH v3 18/28] KVM: x86: Disable APIC logical map if logical ID covers multiple MDAs Sean Christopherson
2022-09-20 23:31 ` [PATCH v3 19/28] KVM: x86: Disable APIC logical map if vCPUs are aliased in logical mode Sean Christopherson
2022-09-20 23:31 ` [PATCH v3 20/28] KVM: x86: Honor architectural behavior for aliased 8-bit APIC IDs Sean Christopherson
2022-09-20 23:31 ` [PATCH v3 21/28] KVM: x86: Inhibit APICv/AVIC if the optimized physical map is disabled Sean Christopherson
2022-09-20 23:31 ` [PATCH v3 22/28] KVM: SVM: Inhibit AVIC if vCPUs are aliased in logical mode Sean Christopherson
2022-09-20 23:31 ` [PATCH v3 23/28] KVM: SVM: Always update local APIC on writes to logical dest register Sean Christopherson
2022-09-20 23:31 ` [PATCH v3 24/28] KVM: SVM: Update svm->ldr_reg cache even if LDR is "bad" Sean Christopherson
2022-09-20 23:31 ` [PATCH v3 25/28] KVM: SVM: Require logical ID to be power-of-2 for AVIC entry Sean Christopherson
2022-09-20 23:31 ` [PATCH v3 26/28] KVM: SVM: Handle multiple logical targets in AVIC kick fastpath Sean Christopherson
2022-09-20 23:31 ` [PATCH v3 27/28] KVM: SVM: Ignore writes to Remote Read Data on AVIC write traps Sean Christopherson
2022-09-20 23:31 ` [PATCH v3 28/28] Revert "KVM: SVM: Do not throw warning when calling avic_vcpu_load on a running vcpu" Sean Christopherson

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=2ca1b7c59e2abe661dd03309ad13eca7d692ff05.camel@redhat.com \
    --to=mlevitsk@redhat.com \
    --cc=alejandro.j.jimenez@oracle.com \
    --cc=kvm@vger.kernel.org \
    --cc=linux-kernel@vger.kernel.org \
    --cc=lirongqing@baidu.com \
    --cc=pbonzini@redhat.com \
    --cc=seanjc@google.com \
    --cc=suravee.suthikulpanit@amd.com \
    /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.