[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

Re: [PATCH v4] xen/x86: On x2APIC mode, derive LDR from APIC ID



On 24/11/2023 22:05, Andrew Cooper wrote:
diff --git a/xen/arch/x86/hvm/vlapic.c b/xen/arch/x86/hvm/vlapic.c
index 5cb87f8649..cd4701c5a2 100644
--- a/xen/arch/x86/hvm/vlapic.c
+++ b/xen/arch/x86/hvm/vlapic.c
@@ -1061,13 +1061,26 @@ static const struct hvm_mmio_ops vlapic_mmio_ops = {
      .write = vlapic_mmio_write,
  };
+static uint32_t x2apic_ldr_from_id(uint32_t id)
+{
+    return ((id & ~0xf) << 12) | (1 << (id & 0xf));
+}
+
  static void set_x2apic_id(struct vlapic *vlapic)
  {

const struct vcpu *v = vlapic_vcpu(vlapic);

seeing as you've got the expression used twice already.

With that, the following logic is shorter too; you can get away with
dropping the vcpu_id variable as v->vcpu_id is the more common form to
use in Xen.

Twice? I can see a vague raison-d'etre in lapic_load_fixup(), but
there's a single occurence here.


  We must preserve LDRs so new vCPUs use consistent
+         * derivations and existing guests, which may have already read the
+         * LDR at the source host, aren't surprised when interrupts stop
+         * working the way they did at the other end.
           */
-        if ( GET_xAPIC_ID(id) != vlapic_vcpu(vlapic)->vcpu_id * 2 ||
-             id != SET_xAPIC_ID(GET_xAPIC_ID(id)) )
-            printk(XENLOG_G_WARNING "%pv: bogus APIC ID %#x loaded\n",
-                   vlapic_vcpu(vlapic), id);
-        set_x2apic_id(vlapic);
-    }
-    else /* Undo an eventual earlier fixup. */
-    {
-        vlapic_set_reg(vlapic, APIC_ID, id);
-        vlapic_set_reg(vlapic, APIC_LDR, vlapic->loaded.ldr);
-    }
+        vlapic_domain(vlapic)->arch.hvm.bug_x2apic_ldr_vcpu_id = true;
+    else
+        printk(XENLOG_G_WARNING
+               "%pv: bogus x2APIC loaded id=%#x ldr=%#x (expected ldr=%#x)\n",

%pv: bogus loaded x2APIC ID %#x, LDR %#x, expected LDR %#x\n

If you properly capitalise x2APIC, you should capitalise ID and LDR.
The other changes are matter of taste, but make for a less cluttered log
message IMO.


"bogus x2APIC loaded" is meant to be a sentence followed by key-value pairs. Uppercasing the keys is fine (albeit unnecessary, IMO), but you choice of word order feels backwards.


This is a long list of minor niggles, but they're all style/comment
issues, and nothing to do with the logic itself.  I'm happy to fix them
all up on commit, and here is the result with them merged:

https://xenbits.xen.org/gitweb/?p=people/andrewcoop/xen.git;a=commitdiff;h=953dcb0317d20959ffee14e404595cfbb66c607a

for you to glance over.

~Andrew

Except for the 2 bits pointed out, the others are cosmetic changes
I don't particularly mind. Jan spotted a typo in the second comment block in lapic_load_fixup() that should be corrected too.
(s/fix fix/fix/)

Cheers,
Alejandro



 


Rackspace

Lists.xenproject.org is hosted with RackSpace, monitoring our
servers 24x7x365 and backed by RackSpace's Fanatical Support®.