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 --
