Re: [PATCH] KVM: LAPIC: Fix a spelling mistake in comments

[Sean Christopherson]

On Fri, Jul 01, 2022, Zhang Jiaming wrote:
> There is a typo (writeable) in kvm_apic_match_physical_addr's comments.
> Fix it.
> 
> Signed-off-by: Zhang Jiaming <jiaming@xxxxxxxxxxxx>
> ---
>  arch/x86/kvm/lapic.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/arch/x86/kvm/lapic.c b/arch/x86/kvm/lapic.c
> index 0e68b4c937fc..ace161bf3744 100644
> --- a/arch/x86/kvm/lapic.c
> +++ b/arch/x86/kvm/lapic.c
> @@ -808,7 +808,7 @@ static bool kvm_apic_match_physical_addr(struct kvm_lapic *apic, u32 mda)
>  	 * Hotplug hack: Make LAPIC in xAPIC mode also accept interrupts as if
>  	 * it were in x2APIC mode.  Hotplugged VCPUs start in xAPIC mode and
>  	 * this allows unique addressing of VCPUs with APIC ID over 0xff.
> -	 * The 0xff condition is needed because writeable xAPIC ID.
> +	 * The 0xff condition is needed because writable xAPIC ID.


Oof, that comment isn't exactly overflowing with information about why writable
xAPIC IDs are problematic.

>  	 */
>  	if (kvm_x2apic_id(apic) > 0xff && mda == kvm_x2apic_id(apic))

IMO checking @mda for > 0xff is more intuitive and easier to document.  Checking
the x2APID ID is functionally equivalent when combined with the "== mda" check, but
in isolation depends on the x2APIC ID being read-only.

Aha!  And checking @mda would allow dropping "fallthrough" logic, as the xAPIC
_can't_ match if @mda > 0xff.  

So this?

---
From: Sean Christopherson <seanjc@xxxxxxxxxx>
Date: Tue, 12 Jul 2022 11:46:53 -0700
Subject: [PATCH] KVM: x86: Check target, not vCPU's x2APIC ID, when applying
 hotplug hack

When applying the hotplug hack to match x2APIC IDs for vCPUs in xAPIC
mode, check the target APID ID for being unaddressable in xAPIC mode
instead of checking the vCPU's x2APIC ID.  Functionally, the two checks
yield identical behavior when combined with the "mda == x2apid_id" check.
But in isolation, checking the x2APIC ID takes an unnecessary dependency
on the x2APIC ID being read-only (which isn't strictly true on AMD CPUs,
and is difficult to document as well), and requires KVM to fallthrough
and check the xAPIC ID as well to deal with a writable xAPIC ID, whereas
the xAPIC ID _can't_ match a target ID greater than 0xff.

Opportunistically reword the comment to call out the various subtleties,
and to fix a typo reported by Zhang Jiaming.

No functional change intended.

Cc: Zhang Jiaming <jiaming@xxxxxxxxxxxx>
Signed-off-by: Sean Christopherson <seanjc@xxxxxxxxxx>
---
 arch/x86/kvm/lapic.c | 15 +++++++++------
 1 file changed, 9 insertions(+), 6 deletions(-)

diff --git a/arch/x86/kvm/lapic.c b/arch/x86/kvm/lapic.c
index 48740a235dee..ef5417d3ce95 100644
--- a/arch/x86/kvm/lapic.c
+++ b/arch/x86/kvm/lapic.c
@@ -830,13 +830,16 @@ static bool kvm_apic_match_physical_addr(struct kvm_lapic *apic, u32 mda)
 		return mda == kvm_x2apic_id(apic);

 	/*
-	 * Hotplug hack: Make LAPIC in xAPIC mode also accept interrupts as if
-	 * it were in x2APIC mode.  Hotplugged VCPUs start in xAPIC mode and
-	 * this allows unique addressing of VCPUs with APIC ID over 0xff.
-	 * The 0xff condition is needed because writable xAPIC ID.
+	 * Hotplug hack: Accept interrupts for vCPUs in xAPIC mode as if they
+	 * were in x2APIC mode if the target APIC ID can't be encoded as an
+	 * xAPIC ID.  This allows unique addressing of hotplugged vCPUs (which
+	 * start in xAPIC mode) with an APIC ID that is unaddressable in xAPIC
+	 * mode.  Match the x2APIC ID if and only if the target APIC ID can't
+	 * be encoded in xAPIC to avoid spurious matches against a vCPU that
+	 * changed its (addressable) xAPIC ID (which is writable).
 	 */
-	if (kvm_x2apic_id(apic) > 0xff && mda == kvm_x2apic_id(apic))
-		return true;
+	if (mda > 0xff)
+		return mda == kvm_x2apic_id(apic);

 	return mda == kvm_xapic_id(apic);
 }

base-commit: ba0d159dd8844469d4e4defff4985a7b80f956e9
--