[PATCH v2] xen/x86: On x2APIC mode, derive LDR from APIC_ID

[Alejandro Vallejo <alejandro.vallejo@xxxxxxxxx>]

Both Intel and AMD manuals agree that on x2APIC mode, the APIC LDR and ID
registers are derivable from each other through a fixed formula.

Xen uses that formula, but applies it to vCPU IDs (which are sequential)
rather than x2APIC_IDs (which are not, at the moment). As I understand it,
this is an attempt to tightly pack vCPUs into clusters so each cluster has
16 vCPUs rather than 8, but this is problematic for OSs that might read the
x2APIC_ID and internally derive LDR (or the other way around)

This patch fixes the implementation so we follow the rules in the x2APIC
spec(s).

The patch also covers migrations from broken hypervisors, so LDRs are
preserved even for hotppluggable CPUs and across APIC resets.

Fixes: f9e0cccf7b35 ("x86/HVM: fix ID handling of x2APIC emulation")
Signed-off-by: Alejandro Vallejo <alejandro.vallejo@xxxxxxxxx>
---
I tested this by creating 3 checkpoints.
 1. One with pre-4.4 broken state (every LDR=1, by hacking save_regs)
 2. One with 4.4 onwards broken state (LDRs packed in their clusters)
 3. One with correct LDR values

(1) and (3) restores to the same thing. Consistent APIC_ID+LDR
(2) restores to what it previously had and hotplugs follow the same logic
---
 xen/arch/x86/hvm/vlapic.c             | 81 +++++++++++++++++++--------
 xen/arch/x86/include/asm/hvm/domain.h | 13 +++++
 2 files changed, 72 insertions(+), 22 deletions(-)

diff --git a/xen/arch/x86/hvm/vlapic.c b/xen/arch/x86/hvm/vlapic.c
index a8e87c4446..7f169f1e5f 100644
--- a/xen/arch/x86/hvm/vlapic.c
+++ b/xen/arch/x86/hvm/vlapic.c
@@ -1061,13 +1061,23 @@ 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)
 {
-    u32 id = vlapic_vcpu(vlapic)->vcpu_id;
-    u32 ldr = ((id & ~0xf) << 12) | (1 << (id & 0xf));
+    uint32_t vcpu_id = vlapic_vcpu(vlapic)->vcpu_id;
+    uint32_t apic_id = vcpu_id * 2;
+    uint32_t apic_ldr = x2apic_ldr_from_id(apic_id);
 
-    vlapic_set_reg(vlapic, APIC_ID, id * 2);
-    vlapic_set_reg(vlapic, APIC_LDR, ldr);
+    /* This is a migration bug workaround. See wall of text in 
lapic_load_fixup() */
+    if ( vlapic_domain(vlapic)->arch.hvm.has_inconsistent_x2apic_ldr_bug )
+        apic_ldr = x2apic_ldr_from_id(vcpu_id);
+
+    vlapic_set_reg(vlapic, APIC_ID, apic_id);
+    vlapic_set_reg(vlapic, APIC_LDR, apic_ldr);
 }
 
 int guest_wrmsr_apic_base(struct vcpu *v, uint64_t val)
@@ -1495,30 +1505,57 @@ static int cf_check lapic_save_regs(struct vcpu *v, 
hvm_domain_context_t *h)
 /*
  * Following lapic_load_hidden()/lapic_load_regs() we may need to
  * correct ID and LDR when they come from an old, broken hypervisor.
+ *
+ * Xen <= 4.4 had a bug by which all the APICs configured in x2APIC mode
+ * got LDR = 1. This was fixed back then, but another bug was introduced
+ * causing APIC ID and LDR to break the consistency they are meant to have
+ * according to the specs (LDR was derived from vCPU ID, rather than APIC
+ * ID)
+ *
+ * Long story short, we can detect both cases here. For the LDR=1 case we
+ * want to fix it up on migrate, as IPIs just don't work on non-physical
+ * mode otherwise. For the other case we actually want to preserve previous
+ * behaviour so that existing running instances that may have already read
+ * the LDR at the source host aren't surprised when IPIs stop working as
+ * they did at the other end.
+ *
+ * Note that "x2apic_id == 0" has always been correct and can't be used to
+ * discriminate these cases.
+ *
+ * Yuck!
  */
 static void lapic_load_fixup(struct vlapic *vlapic)
 {
-    uint32_t id = vlapic->loaded.id;
+    /*
+     * This LDR would be present in broken versions of Xen 4.4 through 4.18.
+     * It's correct for the cpu with x2apic_id=0 (vcpu0), but invalid for
+     * any other.
+     */
+    uint32_t bad_ldr = x2apic_ldr_from_id(vlapic_vcpu(vlapic)->vcpu_id);
 
-    if ( vlapic_x2apic_mode(vlapic) && id && vlapic->loaded.ldr == 1 )
-    {
+    /*
+     * No need to perform fixups in non-x2apic mode, and x2apic_id == 0 has
+     * always been correct.
+     */
+    if ( !vlapic_x2apic_mode(vlapic) || !vlapic->loaded.id )
+        return;
+
+    if ( vlapic->loaded.ldr == 1 )
+       /*
+        * Migration from a broken Xen 4.4 or earlier. We can't leave it
+        * as-is because it assigned the same LDR to every CPU. We'll fix
+        * the bug now and assign LDR values consistent with the APIC ID.
+        */
+        set_x2apic_id(vlapic);
+    else if ( bad_ldr == vlapic->loaded.ldr )
         /*
-         * This is optional: ID != 0 contradicts LDR == 1. It's being added
-         * to aid in eventual debugging of issues arising from the fixup done
-         * here, but can be dropped as soon as it is found to conflict with
-         * other (future) changes.
+         * This is a migration from a broken Xen between 4.4 and 4.18 and we
+         * must _PRESERVE_ LDRs so new vCPUs use consistent derivations. In
+         * this case we set this domain boolean so future CPU hotplugs
+         * derive an LDR consistent with the older Xen's broken idea of
+         * consistency.
          */
-        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.has_inconsistent_x2apic_ldr_bug = true;
 }
 
 static int cf_check lapic_load_hidden(struct domain *d, hvm_domain_context_t 
*h)
diff --git a/xen/arch/x86/include/asm/hvm/domain.h 
b/xen/arch/x86/include/asm/hvm/domain.h
index 6e53ce4449..a42a6e99bb 100644
--- a/xen/arch/x86/include/asm/hvm/domain.h
+++ b/xen/arch/x86/include/asm/hvm/domain.h
@@ -61,6 +61,19 @@ struct hvm_domain {
     /* Cached CF8 for guest PCI config cycles */
     uint32_t                pci_cf8;
 
+    /*
+     * Xen had a bug between 4.4 and 4.18 by which the x2APIC LDR was
+     * derived from the vcpu_id rather than the x2APIC ID. This is wrong,
+     * but changing this behaviour is tricky because guests might have
+     * already read the LDR and used it accordingly. In the interest of not
+     * breaking migrations from those hypervisors we track here whether
+     * this domain suffers from this or not so a hotplugged vCPU or an APIC
+     * reset can recover the same LDR it would've had on the older host.
+     *
+     * Yuck!
+     */
+    bool has_inconsistent_x2apic_ldr_bug;
+
     struct pl_time         *pl_time;
 
     struct hvm_io_handler *io_handler;
-- 
2.34.1