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 Dec 2022 19:03:59 +0000
From:   Sean Christopherson <seanjc@google.com>
To:     Maxim Levitsky <mlevitsk@redhat.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 v4 11/32] KVM: x86: Inhibit APIC memslot if x2APIC and
 AVIC are enabled
Message-ID: <Y5zBH+2VuPJi4yYV@google.com>
References: <20221001005915.2041642-1-seanjc@google.com>
 <20221001005915.2041642-12-seanjc@google.com>
 <90d4a2a1733cdb21e7c00843ddafee78ce52bbdc.camel@redhat.com>
MIME-Version: 1.0
Content-Type: text/plain; charset=us-ascii
Content-Disposition: inline
In-Reply-To: <90d4a2a1733cdb21e7c00843ddafee78ce52bbdc.camel@redhat.com>
Precedence: bulk

On Thu, Dec 08, 2022, Maxim Levitsky wrote:
> On Sat, 2022-10-01 at 00:58 +0000, Sean Christopherson wrote:
> > diff --git a/arch/x86/include/asm/kvm_host.h b/arch/x86/include/asm/kvm_host.h
> > index d40206b16d6c..062758135c86 100644
> > --- a/arch/x86/include/asm/kvm_host.h
> > +++ b/arch/x86/include/asm/kvm_host.h
> > @@ -1139,6 +1139,17 @@ enum kvm_apicv_inhibit {
> >  	 * AVIC is disabled because SEV doesn't support it.
> >  	 */
> >  	APICV_INHIBIT_REASON_SEV,
> > +
> > +	/*
> > +	 * Due to sharing page tables across vCPUs, the xAPIC memslot must be
> > +	 * deleted if any vCPU has x2APIC enabled as SVM doesn't provide fully
> > +	 * independent controls for AVIC vs. x2AVIC, and also because SVM
> > +	 * supports a "hybrid" AVIC mode for CPUs that support AVIC but not
> > +	 * x2AVIC.  Note, this isn't a "full" inhibit and is tracked separately.
> > +	 * AVIC can still be activated, but KVM must not create SPTEs for the
> > +	 * APIC base.  For simplicity, this is sticky.
> > +	 */
> > +	APICV_INHIBIT_REASON_X2APIC,
> 
> I still don't understand why do you want this to be an inhibit bit.

Because in my mental model, it's an inhibit, but with special properties.  But I
totally get why that's confusing.

> Now this 'inhibit' is not even set/clear.
> 
> I prefer to just have a boolean 'is_avic' or,
> '.needs_x2apic_memslot_inhibition' in the vendor ops, and check it in
> 'kvm_vcpu_update_apicv' with the above comment on top of it.
> 
> need_x2apic_memslot_inhibition can even be set to false when x2avic is
> supported at the initalization time, because then AVIC behaves just like
> APICv (when x2avic bit is enabled, AVIC mmio is no longer decoded).

Oh, so SVM does effectively have independent controls, it's only the "hybrid" mode
that's affected?  In that case, how about this?

	/*
	 * Due to sharing page tables across vCPUs, the xAPIC memslot must be
	 * deleted if any vCPU has x2APIC enabled and hardware doesn't support
	 * x2APIC virtualization.  E.g. some AMD CPUs support AVIC but not
	 * x2AVIC.  KVM still allows enabling AVIC in this case so that KVM can
	 * the AVIC doorbell to inject interrupts to running vCPUs, but KVM
	 * mustn't create SPTEs for the APIC base as the vCPU would incorrectly
	 * be able to access the vAPIC page via MMIO despite being in x2APIC
	 * mode.  For simplicity, inhibiting the APIC access page is sticky.
	 */
	if (apic_x2apic_mode(vcpu->arch.apic) &&
	    !kvm_x86_ops.has_hardware_x2apic_virtualization)
		kvm_inhibit_apic_access_page(vcpu)