|
[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index] [Xen-devel] [PATCH v2] 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@xxxxxxxxxx>
---
CC: Jan Beulich <JBeulich@xxxxxxxx>
CC: Wei Liu <wei.liu2@xxxxxxxxxx>
CC: Roger Pau Monné <roger.pau@xxxxxxxxxx>
v2:
* Split reserved bit checking out into a separate conditional
* Adjust printk wording
---
xen/arch/x86/hvm/hvm.c | 4 +--
xen/arch/x86/hvm/vlapic.c | 65 ++++++++++++++++++++++++++++++++++++----
xen/include/asm-x86/hvm/hvm.h | 3 --
xen/include/asm-x86/hvm/vlapic.h | 5 +++-
4 files changed, 64 insertions(+), 13 deletions(-)
diff --git a/xen/arch/x86/hvm/hvm.c b/xen/arch/x86/hvm/hvm.c
index d64b6b6..97fcaad 100644
--- 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 d3a5fb5..a1a43cd 100644
--- 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 d686041..95581ce 100644
--- 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 5938be2..dde66b4 100644
--- 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);
--
2.1.4
_______________________________________________
Xen-devel mailing list
Xen-devel@xxxxxxxxxxxxxxxxxxxx
https://lists.xenproject.org/mailman/listinfo/xen-devel
|
 |
Lists.xenproject.org is hosted with RackSpace, monitoring our |