[Xen-devel] [PATCH RFC] x86/HVM: fix various aspects of x2APIC emulation

["Jan Beulich" <JBeulich@xxxxxxxx>]

- properly change ID when switching into x2APIC mode (instead of
  mimicking necessary behavior in hvm_x2apic_msr_read())
- correctly (meaningfully) set LDR (so far it ended up being 1 on all
  vCPU-s)
- fail reads from the self-IPI register (which is write-only)
- handle self-IPI writes and the ICR2 half of ICR writes largely in
  hvm_x2apic_msr_write()
- even if we don't support more than 128 vCPU-s in a HVM guest for now,
  we should properly handle IDs as 32-bit values (i.e. not ignore the
  top 24 bits)
- with that, properly do cluster ID and bit mask check in
  vlapic_match_logical_addr()
- slightly adjust other parameter types of vlapic_match_dest() and
  vlapic_lowest_prio() (and related local variable ones)

Signed-off-by: Jan Beulich <jbeulich@xxxxxxxx>
---
RFC reason: The previously broken APIC ID handling would seem to cause
problems on migration; so far I didn't have a good idea how to
transparently and correctly convert the bogus old ID (and derived LDR)
to the correct one. Since for a single vCPU guest this doesn't matter
(the only vCPU has ID zero), perhaps checking for LDR being wrongly 1
on all vCPU-s might be a usable approach. I further hope that dealing
with migration _to_ an old, broken hypervisor isn't going to be
necessary.

--- a/xen/arch/x86/hvm/vlapic.c
+++ b/xen/arch/x86/hvm/vlapic.c
@@ -173,18 +173,17 @@ uint32_t vlapic_set_ppr(struct vlapic *v
    return ppr;
 }
 
-static int vlapic_match_logical_addr(struct vlapic *vlapic, uint8_t mda)
+static int vlapic_match_logical_addr(struct vlapic *vlapic, uint32_t mda)
 {
     int result = 0;
-    uint32_t logical_id;
+    uint32_t logical_id = vlapic_get_reg(vlapic, APIC_LDR);
 
     if ( vlapic_x2apic_mode(vlapic) )
-    {
-        logical_id = vlapic_get_reg(vlapic, APIC_LDR);
-        return !!(logical_id & mda);
-    }
+        return ((logical_id >> 16) == (mda >> 16)) &&
+               (uint16_t)(logical_id & mda);
 
-    logical_id = GET_xAPIC_LOGICAL_ID(vlapic_get_reg(vlapic, APIC_LDR));
+    logical_id = GET_xAPIC_LOGICAL_ID(logical_id);
+    mda = (uint8_t)mda;
 
     switch ( vlapic_get_reg(vlapic, APIC_DFR) )
     {
@@ -207,8 +206,8 @@ static int vlapic_match_logical_addr(str
 }
 
 bool_t vlapic_match_dest(
-    struct vlapic *target, struct vlapic *source,
-    int short_hand, uint8_t dest, uint8_t dest_mode)
+    struct vlapic *target, const struct vlapic *source,
+    int short_hand, uint32_t dest, bool_t dest_mode)
 {
     HVM_DBG_LOG(DBG_LEVEL_VLAPIC, "target %p, source %p, dest %#x, "
                 "dest_mode %#x, short_hand %#x",
@@ -219,7 +218,8 @@ bool_t vlapic_match_dest(
     case APIC_DEST_NOSHORT:
         if ( dest_mode )
             return vlapic_match_logical_addr(target, dest);
-        return ((dest == 0xFF) || (dest == VLAPIC_ID(target)));
+        return (dest == _VLAPIC_ID(target, 0xffffffff)) ||
+               (dest == VLAPIC_ID(target));
 
     case APIC_DEST_SELF:
         return (target == source);
@@ -286,7 +286,7 @@ static void vlapic_init_sipi_action(unsi
     uint32_t icr = vcpu_vlapic(origin)->init_sipi.icr;
     uint32_t dest = vcpu_vlapic(origin)->init_sipi.dest;
     uint32_t short_hand = icr & APIC_SHORT_MASK;
-    uint32_t dest_mode  = !!(icr & APIC_DEST_MASK);
+    bool_t dest_mode  = !!(icr & APIC_DEST_MASK);
     struct vcpu *v;
 
     if ( icr == 0 )
@@ -352,8 +352,8 @@ static void vlapic_accept_irq(struct vcp
 }
 
 struct vlapic *vlapic_lowest_prio(
-    struct domain *d, struct vlapic *source,
-    int short_hand, uint8_t dest, uint8_t dest_mode)
+    struct domain *d, const struct vlapic *source,
+    int short_hand, uint32_t dest, bool_t dest_mode)
 {
     int old = d->arch.hvm_domain.irq.round_robin_prev_vcpu;
     uint32_t ppr, target_ppr = UINT_MAX;
@@ -414,13 +414,11 @@ void vlapic_ipi(
 {
     unsigned int dest;
     unsigned int short_hand = icr_low & APIC_SHORT_MASK;
-    unsigned int dest_mode  = !!(icr_low & APIC_DEST_MASK);
+    bool_t dest_mode = !!(icr_low & APIC_DEST_MASK);
 
     HVM_DBG_LOG(DBG_LEVEL_VLAPIC, "icr = 0x%08x:%08x", icr_high, icr_low);
 
-    dest = (vlapic_x2apic_mode(vlapic)
-            ? icr_high
-            : GET_xAPIC_DEST_FIELD(icr_high));
+    dest = _VLAPIC_ID(vlapic, icr_high);
 
     switch ( icr_low & APIC_MODE_MASK )
     {
@@ -611,15 +609,12 @@ int hvm_x2apic_msr_read(struct vcpu *v, 
     vlapic_read_aligned(vlapic, offset, &low);
     switch ( offset )
     {
-    case APIC_ID:
-        low = GET_xAPIC_ID(low);
-        break;
-
     case APIC_ICR:
         vlapic_read_aligned(vlapic, APIC_ICR2, &high);
         break;
 
     case APIC_ICR2:
+    case APIC_SELF_IPI:
         return 1;
     }
 
@@ -710,9 +705,7 @@ static int vlapic_reg_write(struct vcpu 
         break;
 
     case APIC_SELF_IPI:
-        rc = vlapic_x2apic_mode(vlapic)
-            ? vlapic_reg_write(v, APIC_ICR, 0x40000 | (val & 0xff))
-            : X86EMUL_UNHANDLEABLE;
+        rc = X86EMUL_UNHANDLEABLE;
         break;
 
     case APIC_ICR:
@@ -722,9 +715,7 @@ static int vlapic_reg_write(struct vcpu 
         break;
 
     case APIC_ICR2:
-        if ( !vlapic_x2apic_mode(vlapic) )
-            val &= 0xff000000;
-        vlapic_set_reg(vlapic, APIC_ICR2, val);
+        vlapic_set_reg(vlapic, APIC_ICR2, val & 0xff000000);
         break;
 
     case APIC_LVTT:         /* LVT Timer Reg */
@@ -883,16 +874,17 @@ int hvm_x2apic_msr_write(struct vcpu *v,
 
     switch ( offset )
     {
-        int rc;
-
     case APIC_ICR:
-        rc = vlapic_reg_write(v, APIC_ICR2, (uint32_t)(msr_content >> 32));
-        if ( rc )
-            return rc;
+        vlapic_set_reg(vlapic, APIC_ICR2, msr_content >> 32);
         break;
 
     case APIC_ICR2:
         return X86EMUL_UNHANDLEABLE;
+
+    case APIC_SELF_IPI:
+        offset = APIC_ICR;
+        msr_content = APIC_DEST_SELF | (uint8_t)msr_content;
+        break;
     }
 
     return vlapic_reg_write(v, offset, (uint32_t)msr_content);
@@ -932,8 +924,10 @@ void vlapic_msr_set(struct vlapic *vlapi
 
     if ( vlapic_x2apic_mode(vlapic) )
     {
-        u32 id = vlapic_get_reg(vlapic, APIC_ID);
-        u32 ldr = ((id & ~0xf) << 16) | (1 << (id & 0xf));
+        u32 id = vlapic_vcpu(vlapic)->vcpu_id;
+        u32 ldr = ((id & ~0xf) << 12) | (1 << (id & 0xf));
+
+        vlapic_set_reg(vlapic, APIC_ID, id * 2);
         vlapic_set_reg(vlapic, APIC_LDR, ldr);
     }
 
--- a/xen/include/asm-x86/hvm/vlapic.h
+++ b/xen/include/asm-x86/hvm/vlapic.h
@@ -30,8 +30,9 @@
 #define vlapic_vcpu(x)   (container_of((x), struct vcpu, arch.hvm_vcpu.vlapic))
 #define vlapic_domain(x) (vlapic_vcpu(x)->domain)
 
-#define VLAPIC_ID(vlapic)   \
-    (GET_xAPIC_ID(vlapic_get_reg((vlapic), APIC_ID)))
+#define _VLAPIC_ID(vlapic, id) (vlapic_x2apic_mode(vlapic) \
+                                ? (id) : GET_xAPIC_ID(id))
+#define VLAPIC_ID(vlapic) _VLAPIC_ID(vlapic, vlapic_get_reg(vlapic, APIC_ID))
 
 /*
  * APIC can be disabled in two ways:
@@ -123,11 +124,11 @@ void vlapic_ipi(struct vlapic *vlapic, u
 int vlapic_apicv_write(struct vcpu *v, unsigned int offset);
 
 struct vlapic *vlapic_lowest_prio(
-    struct domain *d, struct vlapic *source,
-    int short_hand, uint8_t dest, uint8_t dest_mode);
+    struct domain *d, const struct vlapic *source,
+    int short_hand, uint32_t dest, bool_t dest_mode);
 
 bool_t vlapic_match_dest(
-    struct vlapic *target, struct vlapic *source,
-    int short_hand, uint8_t dest, uint8_t dest_mode);
+    struct vlapic *target, const struct vlapic *source,
+    int short_hand, uint32_t dest, bool_t dest_mode);
 
 #endif /* __ASM_X86_HVM_VLAPIC_H__ */

Attachment: x86-HVM-x2APIC.patch
Description: Text document

_______________________________________________
Xen-devel mailing list
Xen-devel@xxxxxxxxxxxxx
http://lists.xen.org/xen-devel