|
[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index] [Xen-devel] [PATCH v2 4/5] x86/hvm: Handle x2apic MSRs via the new guest_{rd, wr}msr() infrastructure
Dispatch from the guest_{rd,wr}msr() functions. The read side should be safe
outside of current context, but the write side is definitely not. As the
toolstack has no legitimate reason to access the APIC registers via this
interface (not least because whether they are accessible at all depends on
guest settings), unilaterally reject access attempts outside of current
context.
Rename to guest_{rd,wr}msr_x2apic() for consistency, and alter the functions
to use X86EMUL_EXCEPTION rather than X86EMUL_UNHANDLEABLE. The previous
callers turned UNHANDLEABLE into EXCEPTION, but using UNHANDLEABLE will now
interfere with the fallback to legacy MSR handling.
While altering guest_rdmsr_x2apic() make a couple of minor improvements.
Reformat the initialiser for readable[] so it indents in a more natural way,
and alter high to be a 64bit integer to avoid shifting 0 by 32 in the common
path.
Observant people might notice that we now don't let PV guests read the x2apic
MSRs. They should never have been able to in the first place.
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>
CC: Sergey Dyasli <sergey.dyasli@xxxxxxxxxx>
v2:
* Rebase over APIC MSR naming changes
* Drop gp_fault label in guest_wrmsr_x2apic()
---
xen/arch/x86/hvm/hvm.c | 10 -------
xen/arch/x86/hvm/vlapic.c | 65 +++++++++++++++++++++++++------------------
xen/arch/x86/msr.c | 15 ++++++++++
xen/include/asm-x86/hvm/hvm.h | 4 +--
4 files changed, 55 insertions(+), 39 deletions(-)
diff --git a/xen/arch/x86/hvm/hvm.c b/xen/arch/x86/hvm/hvm.c
index 403ffd5..a8d7e19 100644
--- a/xen/arch/x86/hvm/hvm.c
+++ b/xen/arch/x86/hvm/hvm.c
@@ -3439,11 +3439,6 @@ int hvm_msr_read_intercept(unsigned int msr, uint64_t
*msr_content)
*msr_content = vcpu_vlapic(v)->hw.apic_base_msr;
break;
- case MSR_X2APIC_BASE ... MSR_X2APIC_LAST:
- if ( hvm_x2apic_msr_read(v, msr, msr_content) )
- goto gp_fault;
- break;
-
case MSR_IA32_TSC_DEADLINE:
*msr_content = vlapic_tdt_msr_get(vcpu_vlapic(v));
break;
@@ -3598,11 +3593,6 @@ int hvm_msr_write_intercept(unsigned int msr, uint64_t
msr_content,
vlapic_tdt_msr_set(vcpu_vlapic(v), msr_content);
break;
- case MSR_X2APIC_BASE ... MSR_X2APIC_LAST:
- if ( hvm_x2apic_msr_write(v, msr, msr_content) )
- goto gp_fault;
- break;
-
case MSR_IA32_CR_PAT:
if ( !hvm_set_guest_pat(v, msr_content) )
goto gp_fault;
diff --git a/xen/arch/x86/hvm/vlapic.c b/xen/arch/x86/hvm/vlapic.c
index fbb57f8..8817476 100644
--- a/xen/arch/x86/hvm/vlapic.c
+++ b/xen/arch/x86/hvm/vlapic.c
@@ -666,33 +666,39 @@ static int vlapic_read(
return X86EMUL_OKAY;
}
-int hvm_x2apic_msr_read(struct vcpu *v, unsigned int msr, uint64_t
*msr_content)
+int guest_rdmsr_x2apic(const struct vcpu *v, uint32_t msr, uint64_t *val)
{
- static const unsigned long readable[] =
- {
+ static const unsigned long readable[] = {
#define REG(x) (1UL << (APIC_ ## x >> 4))
- REG(ID) | REG(LVR) | REG(TASKPRI) | REG(PROCPRI) |
- REG(LDR) | REG(SPIV) | REG(ESR) | REG(ICR) |
- REG(CMCI) | REG(LVTT) | REG(LVTTHMR) | REG(LVTPC) |
- REG(LVT0) | REG(LVT1) | REG(LVTERR) | REG(TMICT) |
- REG(TMCCT) | REG(TDCR) |
+ REG(ID) | REG(LVR) | REG(TASKPRI) | REG(PROCPRI) |
+ REG(LDR) | REG(SPIV) | REG(ESR) | REG(ICR) |
+ REG(CMCI) | REG(LVTT) | REG(LVTTHMR) | REG(LVTPC) |
+ REG(LVT0) | REG(LVT1) | REG(LVTERR) | REG(TMICT) |
+ REG(TMCCT) | REG(TDCR) |
#undef REG
#define REGBLOCK(x) (((1UL << (NR_VECTORS / 32)) - 1) << (APIC_ ## x >> 4))
- REGBLOCK(ISR) | REGBLOCK(TMR) | REGBLOCK(IRR)
+ REGBLOCK(ISR) | REGBLOCK(TMR) | REGBLOCK(IRR)
#undef REGBLOCK
- };
+ };
const struct vlapic *vlapic = vcpu_vlapic(v);
- uint32_t high = 0, reg = msr - MSR_X2APIC_BASE, offset = reg << 4;
+ uint64_t high = 0;
+ uint32_t reg = msr - MSR_X2APIC_BASE, offset = reg << 4;
+
+ /*
+ * The read side looks as if it might be safe to use outside of current
+ * context, but the write side is most certainly not. As we don't need
+ * any non-current access, enforce symmetry with the write side.
+ */
+ ASSERT(v == current);
if ( !vlapic_x2apic_mode(vlapic) ||
(reg >= sizeof(readable) * 8) || !test_bit(reg, readable) )
- return X86EMUL_UNHANDLEABLE;
+ return X86EMUL_EXCEPTION;
if ( offset == APIC_ICR )
- high = vlapic_read_aligned(vlapic, APIC_ICR2);
+ high = (uint64_t)vlapic_read_aligned(vlapic, APIC_ICR2) << 32;
- *msr_content = ((uint64_t)high << 32) |
- vlapic_read_aligned(vlapic, offset);
+ *val = high | vlapic_read_aligned(vlapic, offset);
return X86EMUL_OKAY;
}
@@ -983,49 +989,52 @@ int vlapic_apicv_write(struct vcpu *v, unsigned int
offset)
return X86EMUL_OKAY;
}
-int hvm_x2apic_msr_write(struct vcpu *v, unsigned int msr, uint64_t
msr_content)
+int guest_wrmsr_x2apic(struct vcpu *v, uint32_t msr, uint64_t msr_content)
{
struct vlapic *vlapic = vcpu_vlapic(v);
uint32_t offset = (msr - MSR_X2APIC_BASE) << 4;
+ /* The timer handling at least is unsafe outside of current context. */
+ ASSERT(v == current);
+
if ( !vlapic_x2apic_mode(vlapic) )
- return X86EMUL_UNHANDLEABLE;
+ return X86EMUL_EXCEPTION;
switch ( offset )
{
case APIC_TASKPRI:
if ( msr_content & ~APIC_TPRI_MASK )
- return X86EMUL_UNHANDLEABLE;
+ return X86EMUL_EXCEPTION;
break;
case APIC_SPIV:
if ( msr_content & ~(APIC_VECTOR_MASK | APIC_SPIV_APIC_ENABLED |
(VLAPIC_VERSION & APIC_LVR_DIRECTED_EOI
? APIC_SPIV_DIRECTED_EOI : 0)) )
- return X86EMUL_UNHANDLEABLE;
+ return X86EMUL_EXCEPTION;
break;
case APIC_LVTT:
if ( msr_content & ~(LVT_MASK | APIC_TIMER_MODE_MASK) )
- return X86EMUL_UNHANDLEABLE;
+ return X86EMUL_EXCEPTION;
break;
case APIC_LVTTHMR:
case APIC_LVTPC:
case APIC_CMCI:
if ( msr_content & ~(LVT_MASK | APIC_MODE_MASK) )
- return X86EMUL_UNHANDLEABLE;
+ return X86EMUL_EXCEPTION;
break;
case APIC_LVT0:
case APIC_LVT1:
if ( msr_content & ~LINT_MASK )
- return X86EMUL_UNHANDLEABLE;
+ return X86EMUL_EXCEPTION;
break;
case APIC_LVTERR:
if ( msr_content & ~LVT_MASK )
- return X86EMUL_UNHANDLEABLE;
+ return X86EMUL_EXCEPTION;
break;
case APIC_TMICT:
@@ -1033,20 +1042,20 @@ int hvm_x2apic_msr_write(struct vcpu *v, unsigned int
msr, uint64_t msr_content)
case APIC_TDCR:
if ( msr_content & ~APIC_TDR_DIV_1 )
- return X86EMUL_UNHANDLEABLE;
+ return X86EMUL_EXCEPTION;
break;
case APIC_ICR:
if ( (uint32_t)msr_content & ~(APIC_VECTOR_MASK | APIC_MODE_MASK |
APIC_DEST_MASK | APIC_INT_ASSERT |
APIC_INT_LEVELTRIG | APIC_SHORT_MASK) )
- return X86EMUL_UNHANDLEABLE;
+ return X86EMUL_EXCEPTION;
vlapic_set_reg(vlapic, APIC_ICR2, msr_content >> 32);
break;
case APIC_SELF_IPI:
if ( msr_content & ~APIC_VECTOR_MASK )
- return X86EMUL_UNHANDLEABLE;
+ return X86EMUL_EXCEPTION;
offset = APIC_ICR;
msr_content = APIC_DEST_SELF | (msr_content & APIC_VECTOR_MASK);
break;
@@ -1054,8 +1063,10 @@ int hvm_x2apic_msr_write(struct vcpu *v, unsigned int
msr, uint64_t msr_content)
case APIC_EOI:
case APIC_ESR:
if ( msr_content )
+ {
default:
- return X86EMUL_UNHANDLEABLE;
+ return X86EMUL_EXCEPTION;
+ }
}
vlapic_reg_write(v, offset, msr_content);
diff --git a/xen/arch/x86/msr.c b/xen/arch/x86/msr.c
index 62079bb..fa2552a 100644
--- a/xen/arch/x86/msr.c
+++ b/xen/arch/x86/msr.c
@@ -141,6 +141,7 @@ int init_vcpu_msr_policy(struct vcpu *v)
int guest_rdmsr(const struct vcpu *v, uint32_t msr, uint64_t *val)
{
+ const struct vcpu *curr = current;
const struct domain *d = v->domain;
const struct cpuid_policy *cp = d->arch.cpuid;
const struct msr_domain_policy *dp = d->arch.msr;
@@ -177,6 +178,13 @@ int guest_rdmsr(const struct vcpu *v, uint32_t msr,
uint64_t *val)
_MSR_MISC_FEATURES_CPUID_FAULTING;
break;
+ case MSR_X2APIC_BASE ... MSR_X2APIC_LAST:
+ if ( !is_hvm_domain(d) || v != curr )
+ goto gp_fault;
+
+ ret = guest_rdmsr_x2apic(v, msr, val);
+ goto out;
+
case MSR_HYPERVISOR_START ... MSR_HYPERVISOR_START + NR_VIRIDIAN_MSRS - 1:
if ( is_viridian_domain(d) )
{
@@ -272,6 +280,13 @@ int guest_wrmsr(struct vcpu *v, uint32_t msr, uint64_t val)
break;
}
+ case MSR_X2APIC_BASE ... MSR_X2APIC_LAST:
+ if ( !is_hvm_domain(d) || v != curr )
+ goto gp_fault;
+
+ ret = guest_wrmsr_x2apic(v, msr, val);
+ goto out;
+
case MSR_HYPERVISOR_START ... MSR_HYPERVISOR_START + NR_VIRIDIAN_MSRS - 1:
if ( is_viridian_domain(d) )
{
diff --git a/xen/include/asm-x86/hvm/hvm.h b/xen/include/asm-x86/hvm/hvm.h
index 9aa6c72..988b896 100644
--- a/xen/include/asm-x86/hvm/hvm.h
+++ b/xen/include/asm-x86/hvm/hvm.h
@@ -528,8 +528,8 @@ int hvm_hap_nested_page_fault(paddr_t gpa, unsigned long
gla,
? (u32)__d->arch.incarnation : (u32)(v)->arch.hvm_vcpu.msr_tsc_aux; \
})
-int hvm_x2apic_msr_read(struct vcpu *v, unsigned int msr, uint64_t
*msr_content);
-int hvm_x2apic_msr_write(struct vcpu *v, unsigned int msr, uint64_t
msr_content);
+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);
/*
* Nested HVM
--
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 |