x86/hvm: Disallow moving the APIC MMIO window

See the code comment for a full discussion, but in short: guests which
currently run under Xen don't move the window, because moving it has never
worked properly.  Implementing support for moving the window is never going to
work architecturally unless we switch to per-vcpu P2Ms (which seems very
unlikely), and would still be a substantial quantity of work for a feature
which is unused in practice.

Take the opportunity to rename vlapic_msr_set() to be consistent with the
other MSR handling functions, and return X86EMUL_* constants.  Add logic to
check for reserved bits, including refusing x2APIC mode if it has not been
offered to the guest.  Move the guest_{rd,wr}msr_x2apic() declarations into
vlapic.h which is a more appropriate place for them to live.

Signed-off-by: Andrew Cooper <andrew.cooper3@citrix.com>
Reviewed-by: Roger Pau Monné <roger.pau@citrix.com>
Reviewed-by: Jan Beulich <jbeulich@suse.com>

diff --git a/xen/arch/x86/hvm/hvm.c b/xen/arch/x86/hvm/hvm.c
index d64b6b6c20bd0cb6af9ef520950db00f80f47e48..97fcaadb0be8a51584e5da1895e3f910b0664988 100644 (file)
--- a/xen/arch/x86/hvm/hvm.c
+++ b/xen/arch/x86/hvm/hvm.c
@@ -3565,9 +3565,7 @@ int hvm_msr_write_intercept(unsigned int msr, uint64_t msr_content,
         break;
 
     case MSR_APIC_BASE:
-        if ( !vlapic_msr_set(vcpu_vlapic(v), msr_content) )
-            goto gp_fault;
-        break;
+        return guest_wrmsr_apic_base(v, msr_content);
 
     case MSR_IA32_TSC_DEADLINE:
         vlapic_tdt_msr_set(vcpu_vlapic(v), msr_content);

diff --git a/xen/arch/x86/hvm/vlapic.c b/xen/arch/x86/hvm/vlapic.c
index d3a5fb5d3f32b4b4814061ee2792f548e97ac709..a1a43cd792ab9404a0de354f2e5b311e0abd514d 100644 (file)
--- a/xen/arch/x86/hvm/vlapic.c
+++ b/xen/arch/x86/hvm/vlapic.c
@@ -1072,15 +1072,68 @@ static void set_x2apic_id(struct vlapic *vlapic)
     vlapic_set_reg(vlapic, APIC_LDR, ldr);
 }
 
-bool_t vlapic_msr_set(struct vlapic *vlapic, uint64_t value)
+int guest_wrmsr_apic_base(struct vcpu *v, uint64_t value)
 {
-    if ( !has_vlapic(vlapic_domain(vlapic)) )
-        return 0;
+    const struct cpuid_policy *cp = v->domain->arch.cpuid;
+    struct vlapic *vlapic = vcpu_vlapic(v);
+
+    if ( !has_vlapic(v->domain) )
+        return X86EMUL_EXCEPTION;
+
+    /* Attempting to set reserved bits? */
+    if ( value & ~(APIC_BASE_ADDR_MASK | APIC_BASE_ENABLE | APIC_BASE_BSP |
+                   (cp->basic.x2apic ? APIC_BASE_EXTD : 0)) )
+        return X86EMUL_EXCEPTION;
+
+    /*
+     * Architecturally speaking, we should allow a guest to move the xAPIC
+     * MMIO window (within reason - not even hardware allows arbitrary
+     * positions).  However, virtualising the behaviour for multi-vcpu guests
+     * is problematic.
+     *
+     * The ability to move the MMIO window was introduced with the Pentium Pro
+     * processor, to deconflict the window with other MMIO in the system.  The
+     * need to move the MMIO window was obsoleted by the Netburst architecture
+     * which reserved the space in physical address space for MSIs.
+     *
+     * As such, it appears to be a rarely used feature before the turn of the
+     * millennium, and entirely unused after.
+     *
+     * Xen uses a per-domain P2M, but MSR_APIC_BASE is per-vcpu.  In
+     * principle, we could emulate the MMIO windows being in different
+     * locations by ensuring that all windows are unmapped in the P2M and trap
+     * for emulation.  Xen has never had code to modify the P2M in response to
+     * APIC_BASE updates, so guests which actually try this are likely to end
+     * up without a working APIC.
+     *
+     * Things are more complicated with hardware APIC acceleration, where Xen
+     * has to map a sink-page into the P2M for APIC accesses to be recognised
+     * and accelerated by microcode.  Again, this could in principle be
+     * emulated, but the visible result in the guest would be multiple working
+     * APIC MMIO windows.  Moving the APIC window has never caused the
+     * sink-page to move in the P2M, meaning that on all modern hardware, the
+     * APIC definitely ceases working if the guest tries to move the window.
+     *
+     * As such, when the APIC is configured in xAPIC mode, require the MMIO
+     * window to be in its default location.  We don't expect any guests which
+     * currently run on Xen to be impacted by this restriction, and the #GP
+     * fault will be far more obvious to debug than a malfunctioning MMIO
+     * window.
+     */
+    if ( ((value & (APIC_BASE_EXTD | APIC_BASE_ENABLE)) == APIC_BASE_ENABLE) &&
+         ((value & APIC_BASE_ADDR_MASK) != APIC_DEFAULT_PHYS_BASE) )
+    {
+        printk(XENLOG_G_INFO
+               "%pv tried to move the APIC MMIO window: val 0x%08"PRIx64"\n",
+               v, value);
+        return X86EMUL_EXCEPTION;
+    }
 
     if ( (vlapic->hw.apic_base_msr ^ value) & APIC_BASE_ENABLE )
     {
         if ( unlikely(value & APIC_BASE_EXTD) )
-            return 0;
+            return X86EMUL_EXCEPTION;
+
         if ( value & APIC_BASE_ENABLE )
         {
             vlapic_reset(vlapic);
@@ -1095,7 +1148,7 @@ bool_t vlapic_msr_set(struct vlapic *vlapic, uint64_t value)
     }
     else if ( ((vlapic->hw.apic_base_msr ^ value) & APIC_BASE_EXTD) &&
               unlikely(!vlapic_xapic_mode(vlapic)) )
-        return 0;
+        return X86EMUL_EXCEPTION;
 
     vlapic->hw.apic_base_msr = value;
     memset(&vlapic->loaded, 0, sizeof(vlapic->loaded));
@@ -1108,7 +1161,7 @@ bool_t vlapic_msr_set(struct vlapic *vlapic, uint64_t value)
     HVM_DBG_LOG(DBG_LEVEL_VLAPIC,
                 "apic base msr is 0x%016"PRIx64, vlapic->hw.apic_base_msr);
 
-    return 1;
+    return X86EMUL_OKAY;
 }
 
 uint64_t  vlapic_tdt_msr_get(struct vlapic *vlapic)

diff --git a/xen/include/asm-x86/hvm/hvm.h b/xen/include/asm-x86/hvm/hvm.h
index d68604127f06cf6ca720ecd5675a2d545faa8bab..95581ce6cb9f83640b994ff3627b40db4735ccf0 100644 (file)
--- a/xen/include/asm-x86/hvm/hvm.h
+++ b/xen/include/asm-x86/hvm/hvm.h
@@ -333,9 +333,6 @@ void hvm_toggle_singlestep(struct vcpu *v);
 int hvm_hap_nested_page_fault(paddr_t gpa, unsigned long gla,
                               struct npfec npfec);
 
-int guest_rdmsr_x2apic(const struct vcpu *v, uint32_t msr, uint64_t *val);
-int guest_wrmsr_x2apic(struct vcpu *v, uint32_t msr, uint64_t val);
-
 /* Check CR4/EFER values */
 const char *hvm_efer_valid(const struct vcpu *v, uint64_t value,
                            signed int cr0_pg);

diff --git a/xen/include/asm-x86/hvm/vlapic.h b/xen/include/asm-x86/hvm/vlapic.h
index 5938be2523be51e5d629a42bfec281c3f2f17f67..dde66b4f0fc446c0ebd8239f0af86ce07274b68e 100644 (file)
--- a/xen/include/asm-x86/hvm/vlapic.h
+++ b/xen/include/asm-x86/hvm/vlapic.h
@@ -123,7 +123,10 @@ void vlapic_destroy(struct vcpu *v);
 
 void vlapic_reset(struct vlapic *vlapic);
 
-bool_t vlapic_msr_set(struct vlapic *vlapic, uint64_t value);
+int guest_wrmsr_apic_base(struct vcpu *v, uint64_t val);
+int guest_rdmsr_x2apic(const struct vcpu *v, uint32_t msr, uint64_t *val);
+int guest_wrmsr_x2apic(struct vcpu *v, uint32_t msr, uint64_t val);
+
 void vlapic_tdt_msr_set(struct vlapic *vlapic, uint64_t value);
 uint64_t vlapic_tdt_msr_get(struct vlapic *vlapic);