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:   Wed, 16 Nov 2022 21:23:12 +0000
From:   Sean Christopherson <seanjc@google.com>
To:     Greg Edwards <gedwards@ddn.com>
Cc:     kvm@vger.kernel.org, linux-kernel@vger.kernel.org,
        Paolo Bonzini <pbonzini@redhat.com>,
        Maxim Levitsky <mlevitsk@redhat.com>
Subject: Re: [PATCH v2] KVM: x86: Allow APICv APIC ID inhibit to be cleared
Message-ID: <Y3VUwKJlnu+sNnV+@google.com>
References: <20221114202037.254176-1-gedwards@ddn.com>
 <20221116205123.18737-1-gedwards@ddn.com>
MIME-Version: 1.0
Content-Type: text/plain; charset=us-ascii
Content-Disposition: inline
In-Reply-To: <20221116205123.18737-1-gedwards@ddn.com>
Precedence: bulk

On Wed, Nov 16, 2022, Greg Edwards wrote:
> Legacy kernels prior to commit 4399c03c6780 ("x86/apic: Remove
> verify_local_APIC()") write the APIC ID of the boot CPU twice to verify
> a functioning local APIC.  This results in APIC acceleration inhibited
> on these kernels for reason APICV_INHIBIT_REASON_APIC_ID_MODIFIED.
> 
> Allow the APICV_INHIBIT_REASON_APIC_ID_MODIFIED inhibit reason to be
> cleared if/when all APICs in xAPIC mode set their APIC ID back to the
> expected vcpu_id value.
> 
> Fold the functionality previously in kvm_lapic_xapic_id_updated() into
> kvm_recalculate_apic_map(), as this allows us examine all APICs in one
> pass.
> 
> Fixes: 3743c2f02517 ("KVM: x86: inhibit APICv/AVIC on changes to APIC ID or APIC base")
> Signed-off-by: Greg Edwards <gedwards@ddn.com>
> ---
> Changes from v1:
>   * Fold kvm_lapic_xapic_id_updated() into kvm_recalculate_apic_map() and
>     verify no APICs in xAPIC mode have a modified APIC ID before clearing
>     APICV_INHIBIT_REASON_APIC_ID_MODIFIED.  [Sean]
>   * Rebase on top of Sean's APIC fixes+cleanups series.  [Sean]
>     https://lore.kernel.org/all/20221001005915.2041642-1-seanjc@google.com/
> 
>  arch/x86/kvm/lapic.c | 45 +++++++++++++++++++-------------------------
>  1 file changed, 19 insertions(+), 26 deletions(-)
> 
> diff --git a/arch/x86/kvm/lapic.c b/arch/x86/kvm/lapic.c
> index 9b3af49d2524..362472da6e7f 100644
> --- a/arch/x86/kvm/lapic.c
> +++ b/arch/x86/kvm/lapic.c
> @@ -236,6 +236,7 @@ void kvm_recalculate_apic_map(struct kvm *kvm)
>  	struct kvm_vcpu *vcpu;
>  	unsigned long i;
>  	u32 max_id = 255; /* enough space for any xAPIC ID */
> +	bool xapic_id_modified = false;

Maybe "xapic_id_mismatch"?  E.g. if KVM ends up back because the xAPIC ID was
modified back to be vcpu_id, then this is somewhat misleading from super pedantic
point of view.  "modified" was ok when the inhibit was a one-way street.

>  	/* Read kvm->arch.apic_map_dirty before kvm->arch.apic_map.  */
>  	if (atomic_read_acquire(&kvm->arch.apic_map_dirty) == CLEAN)
> @@ -285,6 +286,19 @@ void kvm_recalculate_apic_map(struct kvm *kvm)
>  		xapic_id = kvm_xapic_id(apic);
>  		x2apic_id = kvm_x2apic_id(apic);
>  
> +		if (!apic_x2apic_mode(apic)) {
> +			/*
> +			 * Deliberately truncate the vCPU ID when detecting a
> +			 * modified APIC ID to avoid false positives if the
> +			 * vCPU ID, i.e. x2APIC ID, is a 32-bit value.  If the
> +			 * wrap/truncation results in unwanted aliasing, APICv
> +			 * will be inhibited as part of updating KVM's
> +			 * optimized APIC maps.

Heh, the last sentence is stale since this _is_ the flow updates the optimized
maps.

> +			 */
> +			if (xapic_id != (u8)x2apic_id)

It's more than a bit silly, but I would rather this use vcpu->vcpu_id instead of
x2apic_id.  KVM's requirement is that the xAPIC ID must match the vCPU ID.  The
reason I called out x2APIC in the comment was to hint at why KVM even supports
32-bit vCPU IDs.  The fact that KVM also makes the x2APIC ID immutable is orthogonal,
which makes the above check somewhat confusing (even though they're identical under
the hood).

And we should fold the two if-statements together, then the block comment can be
placed outside of the if and have more less indentation to deal with.

How about:
		/*
		 * Deliberately truncate the vCPU ID when detecting a mismatched
		 * APIC ID to avoid false positives if the vCPU ID, i.e. x2APIC
		 * ID, is a 32-bit value.  Any unwanted aliasing due to
		 * truncation results will be detected below.
		 */
		if (!apic_x2apic_mode(apic) && xapic_id != vcpu->vcpu_id)
			xapic_id_mismatch = true;