Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 2620:137:e000::1:20 as permitted sender) client-ip=2620:137:e000::1:20;
Date:   Fri, 16 Sep 2022 17:47:54 +0000
From:   Sean Christopherson <seanjc@google.com>
To:     "Suthikulpanit, Suravee" <suravee.suthikulpanit@amd.com>
Cc:     Paolo Bonzini <pbonzini@redhat.com>, kvm@vger.kernel.org,
        linux-kernel@vger.kernel.org, Maxim Levitsky <mlevitsk@redhat.com>,
        Li RongQing <lirongqing@baidu.com>
Subject: Re: [PATCH v2 04/23] KVM: x86: Inhibit AVIC SPTEs if any vCPU
 enables x2APIC
Message-ID: <YyS2ymYa+yojnYGp@google.com>
References: <20220903002254.2411750-1-seanjc@google.com>
 <20220903002254.2411750-5-seanjc@google.com>
 <b6fcb487-56fc-12ea-6f67-b14b0b156ee0@amd.com>
 <YyGFGXsbgr6WV0B8@google.com>
 <951f2be3-9830-ad71-0140-e5bbf4b78f96@amd.com>
MIME-Version: 1.0
Content-Type: text/plain; charset=us-ascii
Content-Disposition: inline
In-Reply-To: <951f2be3-9830-ad71-0140-e5bbf4b78f96@amd.com>
Precedence: bulk

On Wed, Sep 14, 2022, Suthikulpanit, Suravee wrote:
> Sean
> 
> On 9/14/2022 2:39 AM, Sean Christopherson wrote:
> > > > diff --git a/arch/x86/kvm/lapic.c b/arch/x86/kvm/lapic.c
> > > > index 38e9b8e5278c..d956cd37908e 100644
> > > > --- a/arch/x86/kvm/lapic.c
> > > > +++ b/arch/x86/kvm/lapic.c
> > > > @@ -2394,8 +2394,10 @@ void kvm_lapic_set_base(struct kvm_vcpu *vcpu, u64 value)
> > > >    		}
> > > >    	}
> > > > -	if (((old_value ^ value) & X2APIC_ENABLE) && (value & X2APIC_ENABLE))
> > > > +	if (((old_value ^ value) & X2APIC_ENABLE) && (value & X2APIC_ENABLE)) {
> > > >    		kvm_apic_set_x2apic_id(apic, vcpu->vcpu_id);
> > > > +		kvm_set_apicv_inhibit(vcpu->kvm, APICV_INHIBIT_REASON_X2APIC);
> > > > +	}
> > > .... Here, since we do not want to inhibit APICV/AVIC on system that can
> > > support x2AVIC, this should be set in the vendor-specific call-back
> > > function, where appropriate checks can be made.
> > No, again the intent is to inhibit only the MMIO page.  The x2APIC inhibit is
> > ignored when determining whether or not APICv is inhibited, but is included when
> > checking if the memslot is inhibited.
> > 
> > bool kvm_apicv_memslot_activated(struct kvm *kvm)
> > {
> > 	return (READ_ONCE(kvm->arch.apicv_inhibit_reasons) == 0);
> > }
> > 
> > static unsigned long kvm_apicv_get_inhibit_reasons(struct kvm *kvm)
> > {
> > 	/*
> > 	 * x2APIC only needs to "inhibit" the MMIO region, all other aspects of
> > 	 * APICv can continue to be utilized.
> > 	 */
> > 	return READ_ONCE(kvm->arch.apicv_inhibit_reasons) & ~APICV_INHIBIT_REASON_X2APIC;
> 
> Also, this should be:
> 
> return READ_ONCE(kvm->arch.apicv_inhibit_reasons) & ~(1UL <<
> APICV_INHIBIT_REASON_X2APIC);

Ugh, I found and fixed this locally when testing, but lost the change when shuffling
code between systems.

Thanks!