Re: [PATCH 07/10] KVM: SVM: use vmcb01 in svm_refresh_apicv_exec_ctrl

From: Maxim Levitsky <mlevitsk@redhat.com>
To: Paolo Bonzini <pbonzini@redhat.com>, kvm@vger.kernel.org
Cc: Thomas Gleixner <tglx@linutronix.de>,
	Sean Christopherson <seanjc@google.com>,
	Wanpeng Li <wanpengli@tencent.com>,
	Vitaly Kuznetsov <vkuznets@redhat.com>,
	Joerg Roedel <joro@8bytes.org>, Borislav Petkov <bp@alien8.de>,
	"H. Peter Anvin" <hpa@zytor.com>, Ingo Molnar <mingo@redhat.com>,
	"open list:X86 ARCHITECTURE (32-BIT AND 64-BIT)" 
	<linux-kernel@vger.kernel.org>,
	"maintainer:X86 ARCHITECTURE (32-BIT AND 64-BIT)"
	<x86@kernel.org>, Jim Mattson <jmattson@google.com>
Subject: Re: [PATCH 07/10] KVM: SVM: use vmcb01 in svm_refresh_apicv_exec_ctrl
Date: Thu, 24 Jun 2021 11:16:00 +0300	[thread overview]
Message-ID: <d77c4792681bfc8cb7f13793b651b355ef6684de.camel@redhat.com> (raw)
In-Reply-To: <bd20da0f-eb20-48f7-3258-cd5949f12227@redhat.com>

On Wed, 2021-06-23 at 23:54 +0200, Paolo Bonzini wrote:
> On 23/06/21 13:29, Maxim Levitsky wrote:
> > AVIC is not supported for nesting but in some corner
> > cases it is possible to have it still be enabled,
> > after we entered nesting, and use vmcb02.
> > 
> > Fix this by always using vmcb01 in svm_refresh_apicv_exec_ctrl
> 
> Please be more verbose about the corner case (and then the second 
> paragraph should not be necessary anymore).

I will do it.
The issue can happen only after patch 8 is applied, because then AVIC disable on
the current vCPU is always deferred.

I think that currently the problem in this patch can't happen because 
kvm_request_apicv_update(..., APICV_INHIBIT_REASON_NESTED) is called on each vCPU
from svm_vcpu_after_set_cpuid, and since it disables it on current vCPU, the
AVIC is fully disabled on all vCPUs when we get to the first guest entry, even if nested
(after a migration the first guest entry can be already nested)

After patch 8, the AVIC disable is done at guest entry where we already are in
L2, thus we should toggle it in vmcb01, while vmcb02 shouldn't have AVIC enabled
in the first place.

Best regards,
	Maxim Levitsky

> 
> Paolo
> 
> > Signed-off-by: Maxim Levitsky <mlevitsk@redhat.com>
> > ---
> >   arch/x86/kvm/svm/avic.c | 2 +-
> >   1 file changed, 1 insertion(+), 1 deletion(-)
> > 
> > diff --git a/arch/x86/kvm/svm/avic.c b/arch/x86/kvm/svm/avic.c
> > index 1d01da64c333..a8ad78a2faa1 100644
> > --- a/arch/x86/kvm/svm/avic.c
> > +++ b/arch/x86/kvm/svm/avic.c
> > @@ -646,7 +646,7 @@ static int svm_set_pi_irte_mode(struct kvm_vcpu *vcpu, bool activate)
> >   void svm_refresh_apicv_exec_ctrl(struct kvm_vcpu *vcpu)
> >   {
> >   	struct vcpu_svm *svm = to_svm(vcpu);
> > -	struct vmcb *vmcb = svm->vmcb;
> > +	struct vmcb *vmcb = svm->vmcb01.ptr;
> >   	bool activated = kvm_vcpu_apicv_active(vcpu);
> >   
> >   	if (!enable_apicv)
> > 