[v1] KVM: x86: AVIC and local APIC fixes+cleanups

[PATCH 19/19] Revert "KVM: SVM: Do not throw warning when calling avic_vcpu_load on a running vcpu"

Posted by Sean Christopherson 3 years, 7 months ago

Turns out that some warnings exist for good reasons.  Restore the warning
in avic_vcpu_load() that guards against calling avic_vcpu_load() on a
running vCPU now that KVM avoids doing so when switching between x2APIC
and xAPIC.  The entire point of the WARN is to highlight that KVM should
not be reloading an AVIC.

Opportunistically convert the WARN_ON() to WARN_ON_ONCE() to avoid
spamming the kernel if it does fire.

This reverts commit c0caeee65af3944b7b8abbf566e7cc1fae15c775.

Signed-off-by: Sean Christopherson <seanjc@google.com>
---
 arch/x86/kvm/svm/avic.c | 1 +
 1 file changed, 1 insertion(+)

diff --git a/arch/x86/kvm/svm/avic.c b/arch/x86/kvm/svm/avic.c
index b2033a56010c..3c300113d40b 100644
--- a/arch/x86/kvm/svm/avic.c
+++ b/arch/x86/kvm/svm/avic.c
@@ -1080,6 +1080,7 @@ void avic_vcpu_load(struct kvm_vcpu *vcpu, int cpu)
 		return;
 
 	entry = READ_ONCE(*(svm->avic_physical_id_cache));
+	WARN_ON_ONCE(entry & AVIC_PHYSICAL_ID_ENTRY_IS_RUNNING_MASK);
 
 	entry &= ~AVIC_PHYSICAL_ID_ENTRY_HOST_PHYSICAL_ID_MASK;
 	entry |= (h_physical_id & AVIC_PHYSICAL_ID_ENTRY_HOST_PHYSICAL_ID_MASK);
-- 
2.37.2.672.g94769d06f0-goog

Re: [PATCH 19/19] Revert "KVM: SVM: Do not throw warning when calling avic_vcpu_load on a running vcpu"

Posted by Maxim Levitsky 3 years, 7 months ago

On Wed, 2022-08-31 at 00:35 +0000, Sean Christopherson wrote:
> Turns out that some warnings exist for good reasons.  Restore the warning
> in avic_vcpu_load() that guards against calling avic_vcpu_load() on a
> running vCPU now that KVM avoids doing so when switching between x2APIC
> and xAPIC.  The entire point of the WARN is to highlight that KVM should
> not be reloading an AVIC.
> 
> Opportunistically convert the WARN_ON() to WARN_ON_ONCE() to avoid
> spamming the kernel if it does fire.
> 
> This reverts commit c0caeee65af3944b7b8abbf566e7cc1fae15c775.
> 
> Signed-off-by: Sean Christopherson <seanjc@google.com>
> ---
>  arch/x86/kvm/svm/avic.c | 1 +
>  1 file changed, 1 insertion(+)
> 
> diff --git a/arch/x86/kvm/svm/avic.c b/arch/x86/kvm/svm/avic.c
> index b2033a56010c..3c300113d40b 100644
> --- a/arch/x86/kvm/svm/avic.c
> +++ b/arch/x86/kvm/svm/avic.c
> @@ -1080,6 +1080,7 @@ void avic_vcpu_load(struct kvm_vcpu *vcpu, int cpu)
>  		return;
>  
>  	entry = READ_ONCE(*(svm->avic_physical_id_cache));
> +	WARN_ON_ONCE(entry & AVIC_PHYSICAL_ID_ENTRY_IS_RUNNING_MASK);
>  
>  	entry &= ~AVIC_PHYSICAL_ID_ENTRY_HOST_PHYSICAL_ID_MASK;
>  	entry |= (h_physical_id & AVIC_PHYSICAL_ID_ENTRY_HOST_PHYSICAL_ID_MASK);

Note that this warning was removed because it would trigger wheh x2avic code would switch
between xapic and x2apic.

I do agree 100% that this warning is useful.

Best regards,
	Maxim Levitsky

Re: [PATCH 19/19] Revert "KVM: SVM: Do not throw warning when calling avic_vcpu_load on a running vcpu"

Posted by Maxim Levitsky 3 years, 7 months ago

On Wed, 2022-08-31 at 09:07 +0300, Maxim Levitsky wrote:
> On Wed, 2022-08-31 at 00:35 +0000, Sean Christopherson wrote:
> > Turns out that some warnings exist for good reasons.  Restore the warning
> > in avic_vcpu_load() that guards against calling avic_vcpu_load() on a
> > running vCPU now that KVM avoids doing so when switching between x2APIC
> > and xAPIC.  The entire point of the WARN is to highlight that KVM should
> > not be reloading an AVIC.
> > 
> > Opportunistically convert the WARN_ON() to WARN_ON_ONCE() to avoid
> > spamming the kernel if it does fire.
> > 
> > This reverts commit c0caeee65af3944b7b8abbf566e7cc1fae15c775.
> > 
> > Signed-off-by: Sean Christopherson <seanjc@google.com>
> > ---
> >  arch/x86/kvm/svm/avic.c | 1 +
> >  1 file changed, 1 insertion(+)
> > 
> > diff --git a/arch/x86/kvm/svm/avic.c b/arch/x86/kvm/svm/avic.c
> > index b2033a56010c..3c300113d40b 100644
> > --- a/arch/x86/kvm/svm/avic.c
> > +++ b/arch/x86/kvm/svm/avic.c
> > @@ -1080,6 +1080,7 @@ void avic_vcpu_load(struct kvm_vcpu *vcpu, int cpu)
> >  		return;
> >  
> >  	entry = READ_ONCE(*(svm->avic_physical_id_cache));
> > +	WARN_ON_ONCE(entry & AVIC_PHYSICAL_ID_ENTRY_IS_RUNNING_MASK);
> >  
> >  	entry &= ~AVIC_PHYSICAL_ID_ENTRY_HOST_PHYSICAL_ID_MASK;
> >  	entry |= (h_physical_id & AVIC_PHYSICAL_ID_ENTRY_HOST_PHYSICAL_ID_MASK);
> 
> Note that this warning was removed because it would trigger wheh x2avic code would switch
> between xapic and x2apic.
> 
> I do agree 100% that this warning is useful.

Ah I see that you fixed the apic reloading to not trigger this code,
need more coffee - in this case this patch makes lot of sense.

I'll review it again when I review rest of the patches in the patch series.

Best regards,
	Maxim Levitsky
> 
> Best regards,
> 	Maxim Levitsky