* [PATCH v5 0/4] x86/HVM: fix various aspects of APIC emulation
@ 2014-09-24 15:28 Jan Beulich
2014-09-24 15:34 ` [PATCH v5 1/4] x86/HVM: fix miscellaneous aspects of x2APIC emulation Jan Beulich
` (4 more replies)
0 siblings, 5 replies; 11+ messages in thread
From: Jan Beulich @ 2014-09-24 15:28 UTC (permalink / raw)
To: xen-devel; +Cc: Andrew Cooper, Keir Fraser
1: fix miscellaneous aspects of x2APIC emulation
2: fix ID handling of x2APIC emulation
3: a few type adjustments
4: don't silently accept bad vectors
Signed-off-by: Jan Beulich <jbeulich@suse.com>
---
v5: 1st patch extended, parts of 2nd patch moved to 3rd. See
individual patches for specifics.
v4: 2nd patch partly re-done (see patch for details), previously
separate 4th patch now integrated.
v3: 1st patch extended (see patch for details), new 3rd patch.
v2: Monolithic patch broken up. Generate #GP on invalid APIC base
MSR transitions.Correct ID and LDR after domain restore (if
necessary); as stated previously the only compatibility problem this
creates is when migrating a VM _to_ an unfixed (i.e. old) hypervisor,
a scenario which supposedly isn't supported.
^ permalink raw reply [flat|nested] 11+ messages in thread
* [PATCH v5 1/4] x86/HVM: fix miscellaneous aspects of x2APIC emulation
2014-09-24 15:28 [PATCH v5 0/4] x86/HVM: fix various aspects of APIC emulation Jan Beulich
@ 2014-09-24 15:34 ` Jan Beulich
2014-09-24 15:57 ` Andrew Cooper
2014-09-24 15:35 ` [PATCH v5 2/4] x86/HVM: fix ID handling " Jan Beulich
` (3 subsequent siblings)
4 siblings, 1 reply; 11+ messages in thread
From: Jan Beulich @ 2014-09-24 15:34 UTC (permalink / raw)
To: xen-devel; +Cc: Andrew Cooper, Keir Fraser
[-- Attachment #1: Type: text/plain, Size: 11078 bytes --]
- generate #GP on invalid APIC base MSR transitions
- fail reads from the EOI and self-IPI registers (which are write-only)
- handle self-IPI writes and the ICR2 half of ICR writes largely in
hvm_x2apic_msr_write() and (for self-IPI only) vlapic_apicv_write()
- don't permit MMIO-based access in x2APIC mode
- filter writes to read-only registers in hvm_x2apic_msr_write(),
allowing conditionals to be dropped from vlapic_reg_write()
- don't ignore upper half of MSR-based write to ESR being non-zero
- don't ignore other writes to reserved bits
- VMX's EXIT_REASON_APIC_WRITE must not result in #GP (this exit being
trap-like, this exception would get raised on the wrong RIP)
- make hvm_x2apic_msr_read() produce X86EMUL_* return codes just like
hvm_x2apic_msr_write() does (benign to the only caller)
Signed-off-by: Jan Beulich <jbeulich@suse.com>
---
v5: Force #GP to be raised on reserved bit writes. Drop APIC_TMCCT from
set of writable registers. Drop message previously issued on x2APIC
ESR writes.
v3: Also handle APIC_EOI in hvm_x2apic_msr_read() as pointed out by
Andrew. Filter MMIO-based accesses in vlapic_range() when in x2APIC
mode. Move x2APIC special casing from vlapic_reg_write() to
hvm_x2apic_msr_write(). Don't open-code vlapic_x2apic_mode().
v2: Split from main patch.
--- a/xen/arch/x86/hvm/hvm.c
+++ b/xen/arch/x86/hvm/hvm.c
@@ -4499,7 +4499,8 @@ int hvm_msr_write_intercept(unsigned int
break;
case MSR_IA32_APICBASE:
- vlapic_msr_set(vcpu_vlapic(v), msr_content);
+ if ( !vlapic_msr_set(vcpu_vlapic(v), msr_content) )
+ goto gp_fault;
break;
case MSR_IA32_TSC_DEADLINE:
--- a/xen/arch/x86/hvm/vlapic.c
+++ b/xen/arch/x86/hvm/vlapic.c
@@ -45,11 +45,11 @@
#define VLAPIC_LVT_NUM 6
#define LVT_MASK \
- APIC_LVT_MASKED | APIC_SEND_PENDING | APIC_VECTOR_MASK
+ (APIC_LVT_MASKED | APIC_SEND_PENDING | APIC_VECTOR_MASK)
#define LINT_MASK \
- LVT_MASK | APIC_MODE_MASK | APIC_INPUT_POLARITY |\
- APIC_LVT_REMOTE_IRR | APIC_LVT_LEVEL_TRIGGER
+ (LVT_MASK | APIC_MODE_MASK | APIC_INPUT_POLARITY |\
+ APIC_LVT_REMOTE_IRR | APIC_LVT_LEVEL_TRIGGER)
static const unsigned int vlapic_lvt_mask[VLAPIC_LVT_NUM] =
{
@@ -614,7 +614,7 @@ int hvm_x2apic_msr_read(struct vcpu *v,
uint32_t low, high = 0, offset = (msr - MSR_IA32_APICBASE_MSR) << 4;
if ( !vlapic_x2apic_mode(vlapic) )
- return 1;
+ return X86EMUL_UNHANDLEABLE;
vlapic_read_aligned(vlapic, offset, &low);
switch ( offset )
@@ -627,12 +627,15 @@ int hvm_x2apic_msr_read(struct vcpu *v,
vlapic_read_aligned(vlapic, APIC_ICR2, &high);
break;
+ case APIC_EOI:
case APIC_ICR2:
- return 1;
+ case APIC_SELF_IPI:
+ return X86EMUL_UNHANDLEABLE;
}
*msr_content = (((uint64_t)high) << 32) | low;
- return 0;
+
+ return X86EMUL_OKAY;
}
static void vlapic_pt_cb(struct vcpu *v, void *data)
@@ -656,10 +659,7 @@ static int vlapic_reg_write(struct vcpu
switch ( offset )
{
case APIC_ID:
- if ( !vlapic_x2apic_mode(vlapic) )
- vlapic_set_reg(vlapic, APIC_ID, val);
- else
- rc = X86EMUL_UNHANDLEABLE;
+ vlapic_set_reg(vlapic, APIC_ID, val);
break;
case APIC_TASKPRI:
@@ -671,17 +671,11 @@ static int vlapic_reg_write(struct vcpu
break;
case APIC_LDR:
- if ( !vlapic_x2apic_mode(vlapic) )
- vlapic_set_reg(vlapic, APIC_LDR, val & APIC_LDR_MASK);
- else
- rc = X86EMUL_UNHANDLEABLE;
+ vlapic_set_reg(vlapic, APIC_LDR, val & APIC_LDR_MASK);
break;
case APIC_DFR:
- if ( !vlapic_x2apic_mode(vlapic) )
- vlapic_set_reg(vlapic, APIC_DFR, val | 0x0FFFFFFF);
- else
- rc = X86EMUL_UNHANDLEABLE;
+ vlapic_set_reg(vlapic, APIC_DFR, val | 0x0FFFFFFF);
break;
case APIC_SPIV:
@@ -708,21 +702,6 @@ static int vlapic_reg_write(struct vcpu
}
break;
- case APIC_ESR:
- if ( vlapic_x2apic_mode(vlapic) && (val != 0) )
- {
- gdprintk(XENLOG_ERR, "Local APIC write ESR with non-zero %lx\n",
- val);
- rc = X86EMUL_UNHANDLEABLE;
- }
- break;
-
- case APIC_SELF_IPI:
- rc = vlapic_x2apic_mode(vlapic)
- ? vlapic_reg_write(v, APIC_ICR, 0x40000 | (val & 0xff))
- : X86EMUL_UNHANDLEABLE;
- break;
-
case APIC_ICR:
val &= ~(1 << 12); /* always clear the pending bit */
vlapic_ipi(vlapic, val, vlapic_get_reg(vlapic, APIC_ICR2));
@@ -730,9 +709,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 */
@@ -877,8 +854,16 @@ static int vlapic_write(struct vcpu *v,
int vlapic_apicv_write(struct vcpu *v, unsigned int offset)
{
- uint32_t val = vlapic_get_reg(vcpu_vlapic(v), offset);
- return vlapic_reg_write(v, offset, val);
+ struct vlapic *vlapic = vcpu_vlapic(v);
+ uint32_t val = vlapic_get_reg(vlapic, offset);
+
+ if ( !vlapic_x2apic_mode(vlapic) )
+ return vlapic_reg_write(v, offset, val);
+
+ if ( offset != APIC_SELF_IPI )
+ return X86EMUL_UNHANDLEABLE;
+
+ return vlapic_reg_write(v, APIC_ICR, APIC_DEST_SELF | (uint8_t)val);
}
int hvm_x2apic_msr_write(struct vcpu *v, unsigned int msr, uint64_t msr_content)
@@ -891,16 +876,69 @@ int hvm_x2apic_msr_write(struct vcpu *v,
switch ( offset )
{
- int rc;
+ case APIC_TASKPRI:
+ if ( msr_content & ~APIC_TPRI_MASK )
+ return X86EMUL_UNHANDLEABLE;
+ 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;
+ break;
+
+ case APIC_LVTT:
+ if ( msr_content & ~(LVT_MASK | APIC_TIMER_MODE_MASK) )
+ return X86EMUL_UNHANDLEABLE;
+ break;
+
+ case APIC_LVTTHMR:
+ case APIC_LVTPC:
+ case APIC_CMCI:
+ if ( msr_content & ~(LVT_MASK | APIC_MODE_MASK) )
+ return X86EMUL_UNHANDLEABLE;
+ break;
+
+ case APIC_LVT0:
+ case APIC_LVT1:
+ if ( msr_content & ~LINT_MASK )
+ return X86EMUL_UNHANDLEABLE;
+ break;
+
+ case APIC_LVTERR:
+ if ( msr_content & ~LVT_MASK )
+ return X86EMUL_UNHANDLEABLE;
+ break;
+
+ case APIC_TMICT:
+ break;
+
+ case APIC_TDCR:
+ if ( msr_content & ~APIC_TDR_DIV_1 )
+ return X86EMUL_UNHANDLEABLE;
+ break;
case APIC_ICR:
- rc = vlapic_reg_write(v, APIC_ICR2, (uint32_t)(msr_content >> 32));
- if ( rc )
- return rc;
+ 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;
+ vlapic_set_reg(vlapic, APIC_ICR2, msr_content >> 32);
break;
- case APIC_ICR2:
- return X86EMUL_UNHANDLEABLE;
+ case APIC_SELF_IPI:
+ if ( msr_content & ~APIC_VECTOR_MASK )
+ return X86EMUL_UNHANDLEABLE;
+ offset = APIC_ICR;
+ msr_content = APIC_DEST_SELF | (msr_content & APIC_VECTOR_MASK);
+ break;
+
+ case APIC_EOI:
+ case APIC_ESR:
+ if ( msr_content )
+ default:
+ return X86EMUL_UNHANDLEABLE;
}
return vlapic_reg_write(v, offset, (uint32_t)msr_content);
@@ -910,7 +948,10 @@ static int vlapic_range(struct vcpu *v,
{
struct vlapic *vlapic = vcpu_vlapic(v);
unsigned long offset = addr - vlapic_base_address(vlapic);
- return (!vlapic_hw_disabled(vlapic) && (offset < PAGE_SIZE));
+
+ return !vlapic_hw_disabled(vlapic) &&
+ !vlapic_x2apic_mode(vlapic) &&
+ (offset < PAGE_SIZE);
}
const struct hvm_mmio_handler vlapic_mmio_handler = {
@@ -919,10 +960,12 @@ const struct hvm_mmio_handler vlapic_mmi
.write_handler = vlapic_write
};
-void vlapic_msr_set(struct vlapic *vlapic, uint64_t value)
+bool_t vlapic_msr_set(struct vlapic *vlapic, uint64_t value)
{
if ( (vlapic->hw.apic_base_msr ^ value) & MSR_IA32_APICBASE_ENABLE )
{
+ if ( unlikely(value & MSR_IA32_APICBASE_EXTD) )
+ return 0;
if ( value & MSR_IA32_APICBASE_ENABLE )
{
vlapic_reset(vlapic);
@@ -931,10 +974,15 @@ void vlapic_msr_set(struct vlapic *vlapi
}
else
{
+ if ( unlikely(vlapic_x2apic_mode(vlapic)) )
+ return 0;
vlapic->hw.disabled |= VLAPIC_HW_DISABLED;
pt_may_unmask_irq(vlapic_domain(vlapic), NULL);
}
}
+ else if ( !(value & MSR_IA32_APICBASE_ENABLE) &&
+ unlikely(value & MSR_IA32_APICBASE_EXTD) )
+ return 0;
vlapic->hw.apic_base_msr = value;
@@ -949,6 +997,8 @@ void vlapic_msr_set(struct vlapic *vlapi
HVM_DBG_LOG(DBG_LEVEL_VLAPIC,
"apic base msr is 0x%016"PRIx64, vlapic->hw.apic_base_msr);
+
+ return 1;
}
uint64_t vlapic_tdt_msr_get(struct vlapic *vlapic)
@@ -1232,6 +1282,10 @@ static int lapic_load_hidden(struct doma
if ( hvm_load_entry_zeroextend(LAPIC, h, &s->hw) != 0 )
return -EINVAL;
+ if ( !(s->hw.apic_base_msr & MSR_IA32_APICBASE_ENABLE) &&
+ unlikely(vlapic_x2apic_mode(s)) )
+ return -EINVAL;
+
vmx_vlapic_msr_changed(v);
return 0;
--- a/xen/arch/x86/hvm/vmx/vmx.c
+++ b/xen/arch/x86/hvm/vmx/vmx.c
@@ -3110,8 +3110,7 @@ void vmx_vmexit_handler(struct cpu_user_
break;
case EXIT_REASON_APIC_WRITE:
- if ( vmx_handle_apic_write() )
- hvm_inject_hw_exception(TRAP_gp_fault, 0);
+ vmx_handle_apic_write();
break;
case EXIT_REASON_ACCESS_GDTR_OR_IDTR:
--- a/xen/include/asm-x86/hvm/vlapic.h
+++ b/xen/include/asm-x86/hvm/vlapic.h
@@ -106,7 +106,7 @@ void vlapic_destroy(struct vcpu *v);
void vlapic_reset(struct vlapic *vlapic);
-void vlapic_msr_set(struct vlapic *vlapic, uint64_t value);
+bool_t vlapic_msr_set(struct vlapic *vlapic, uint64_t value);
void vlapic_tdt_msr_set(struct vlapic *vlapic, uint64_t value);
uint64_t vlapic_tdt_msr_get(struct vlapic *vlapic);
[-- Attachment #2: x86-HVM-x2APIC-misc.patch --]
[-- Type: text/plain, Size: 11132 bytes --]
x86/HVM: fix miscellaneous aspects of x2APIC emulation
- generate #GP on invalid APIC base MSR transitions
- fail reads from the EOI and self-IPI registers (which are write-only)
- handle self-IPI writes and the ICR2 half of ICR writes largely in
hvm_x2apic_msr_write() and (for self-IPI only) vlapic_apicv_write()
- don't permit MMIO-based access in x2APIC mode
- filter writes to read-only registers in hvm_x2apic_msr_write(),
allowing conditionals to be dropped from vlapic_reg_write()
- don't ignore upper half of MSR-based write to ESR being non-zero
- don't ignore other writes to reserved bits
- VMX's EXIT_REASON_APIC_WRITE must not result in #GP (this exit being
trap-like, this exception would get raised on the wrong RIP)
- make hvm_x2apic_msr_read() produce X86EMUL_* return codes just like
hvm_x2apic_msr_write() does (benign to the only caller)
Signed-off-by: Jan Beulich <jbeulich@suse.com>
---
v5: Force #GP to be raised on reserved bit writes. Drop APIC_TMCCT from
set of writable registers. Drop message previously issued on x2APIC
ESR writes.
v3: Also handle APIC_EOI in hvm_x2apic_msr_read() as pointed out by
Andrew. Filter MMIO-based accesses in vlapic_range() when in x2APIC
mode. Move x2APIC special casing from vlapic_reg_write() to
hvm_x2apic_msr_write(). Don't open-code vlapic_x2apic_mode().
v2: Split from main patch.
--- a/xen/arch/x86/hvm/hvm.c
+++ b/xen/arch/x86/hvm/hvm.c
@@ -4499,7 +4499,8 @@ int hvm_msr_write_intercept(unsigned int
break;
case MSR_IA32_APICBASE:
- vlapic_msr_set(vcpu_vlapic(v), msr_content);
+ if ( !vlapic_msr_set(vcpu_vlapic(v), msr_content) )
+ goto gp_fault;
break;
case MSR_IA32_TSC_DEADLINE:
--- a/xen/arch/x86/hvm/vlapic.c
+++ b/xen/arch/x86/hvm/vlapic.c
@@ -45,11 +45,11 @@
#define VLAPIC_LVT_NUM 6
#define LVT_MASK \
- APIC_LVT_MASKED | APIC_SEND_PENDING | APIC_VECTOR_MASK
+ (APIC_LVT_MASKED | APIC_SEND_PENDING | APIC_VECTOR_MASK)
#define LINT_MASK \
- LVT_MASK | APIC_MODE_MASK | APIC_INPUT_POLARITY |\
- APIC_LVT_REMOTE_IRR | APIC_LVT_LEVEL_TRIGGER
+ (LVT_MASK | APIC_MODE_MASK | APIC_INPUT_POLARITY |\
+ APIC_LVT_REMOTE_IRR | APIC_LVT_LEVEL_TRIGGER)
static const unsigned int vlapic_lvt_mask[VLAPIC_LVT_NUM] =
{
@@ -614,7 +614,7 @@ int hvm_x2apic_msr_read(struct vcpu *v,
uint32_t low, high = 0, offset = (msr - MSR_IA32_APICBASE_MSR) << 4;
if ( !vlapic_x2apic_mode(vlapic) )
- return 1;
+ return X86EMUL_UNHANDLEABLE;
vlapic_read_aligned(vlapic, offset, &low);
switch ( offset )
@@ -627,12 +627,15 @@ int hvm_x2apic_msr_read(struct vcpu *v,
vlapic_read_aligned(vlapic, APIC_ICR2, &high);
break;
+ case APIC_EOI:
case APIC_ICR2:
- return 1;
+ case APIC_SELF_IPI:
+ return X86EMUL_UNHANDLEABLE;
}
*msr_content = (((uint64_t)high) << 32) | low;
- return 0;
+
+ return X86EMUL_OKAY;
}
static void vlapic_pt_cb(struct vcpu *v, void *data)
@@ -656,10 +659,7 @@ static int vlapic_reg_write(struct vcpu
switch ( offset )
{
case APIC_ID:
- if ( !vlapic_x2apic_mode(vlapic) )
- vlapic_set_reg(vlapic, APIC_ID, val);
- else
- rc = X86EMUL_UNHANDLEABLE;
+ vlapic_set_reg(vlapic, APIC_ID, val);
break;
case APIC_TASKPRI:
@@ -671,17 +671,11 @@ static int vlapic_reg_write(struct vcpu
break;
case APIC_LDR:
- if ( !vlapic_x2apic_mode(vlapic) )
- vlapic_set_reg(vlapic, APIC_LDR, val & APIC_LDR_MASK);
- else
- rc = X86EMUL_UNHANDLEABLE;
+ vlapic_set_reg(vlapic, APIC_LDR, val & APIC_LDR_MASK);
break;
case APIC_DFR:
- if ( !vlapic_x2apic_mode(vlapic) )
- vlapic_set_reg(vlapic, APIC_DFR, val | 0x0FFFFFFF);
- else
- rc = X86EMUL_UNHANDLEABLE;
+ vlapic_set_reg(vlapic, APIC_DFR, val | 0x0FFFFFFF);
break;
case APIC_SPIV:
@@ -708,21 +702,6 @@ static int vlapic_reg_write(struct vcpu
}
break;
- case APIC_ESR:
- if ( vlapic_x2apic_mode(vlapic) && (val != 0) )
- {
- gdprintk(XENLOG_ERR, "Local APIC write ESR with non-zero %lx\n",
- val);
- rc = X86EMUL_UNHANDLEABLE;
- }
- break;
-
- case APIC_SELF_IPI:
- rc = vlapic_x2apic_mode(vlapic)
- ? vlapic_reg_write(v, APIC_ICR, 0x40000 | (val & 0xff))
- : X86EMUL_UNHANDLEABLE;
- break;
-
case APIC_ICR:
val &= ~(1 << 12); /* always clear the pending bit */
vlapic_ipi(vlapic, val, vlapic_get_reg(vlapic, APIC_ICR2));
@@ -730,9 +709,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 */
@@ -877,8 +854,16 @@ static int vlapic_write(struct vcpu *v,
int vlapic_apicv_write(struct vcpu *v, unsigned int offset)
{
- uint32_t val = vlapic_get_reg(vcpu_vlapic(v), offset);
- return vlapic_reg_write(v, offset, val);
+ struct vlapic *vlapic = vcpu_vlapic(v);
+ uint32_t val = vlapic_get_reg(vlapic, offset);
+
+ if ( !vlapic_x2apic_mode(vlapic) )
+ return vlapic_reg_write(v, offset, val);
+
+ if ( offset != APIC_SELF_IPI )
+ return X86EMUL_UNHANDLEABLE;
+
+ return vlapic_reg_write(v, APIC_ICR, APIC_DEST_SELF | (uint8_t)val);
}
int hvm_x2apic_msr_write(struct vcpu *v, unsigned int msr, uint64_t msr_content)
@@ -891,16 +876,69 @@ int hvm_x2apic_msr_write(struct vcpu *v,
switch ( offset )
{
- int rc;
+ case APIC_TASKPRI:
+ if ( msr_content & ~APIC_TPRI_MASK )
+ return X86EMUL_UNHANDLEABLE;
+ 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;
+ break;
+
+ case APIC_LVTT:
+ if ( msr_content & ~(LVT_MASK | APIC_TIMER_MODE_MASK) )
+ return X86EMUL_UNHANDLEABLE;
+ break;
+
+ case APIC_LVTTHMR:
+ case APIC_LVTPC:
+ case APIC_CMCI:
+ if ( msr_content & ~(LVT_MASK | APIC_MODE_MASK) )
+ return X86EMUL_UNHANDLEABLE;
+ break;
+
+ case APIC_LVT0:
+ case APIC_LVT1:
+ if ( msr_content & ~LINT_MASK )
+ return X86EMUL_UNHANDLEABLE;
+ break;
+
+ case APIC_LVTERR:
+ if ( msr_content & ~LVT_MASK )
+ return X86EMUL_UNHANDLEABLE;
+ break;
+
+ case APIC_TMICT:
+ break;
+
+ case APIC_TDCR:
+ if ( msr_content & ~APIC_TDR_DIV_1 )
+ return X86EMUL_UNHANDLEABLE;
+ break;
case APIC_ICR:
- rc = vlapic_reg_write(v, APIC_ICR2, (uint32_t)(msr_content >> 32));
- if ( rc )
- return rc;
+ 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;
+ vlapic_set_reg(vlapic, APIC_ICR2, msr_content >> 32);
break;
- case APIC_ICR2:
- return X86EMUL_UNHANDLEABLE;
+ case APIC_SELF_IPI:
+ if ( msr_content & ~APIC_VECTOR_MASK )
+ return X86EMUL_UNHANDLEABLE;
+ offset = APIC_ICR;
+ msr_content = APIC_DEST_SELF | (msr_content & APIC_VECTOR_MASK);
+ break;
+
+ case APIC_EOI:
+ case APIC_ESR:
+ if ( msr_content )
+ default:
+ return X86EMUL_UNHANDLEABLE;
}
return vlapic_reg_write(v, offset, (uint32_t)msr_content);
@@ -910,7 +948,10 @@ static int vlapic_range(struct vcpu *v,
{
struct vlapic *vlapic = vcpu_vlapic(v);
unsigned long offset = addr - vlapic_base_address(vlapic);
- return (!vlapic_hw_disabled(vlapic) && (offset < PAGE_SIZE));
+
+ return !vlapic_hw_disabled(vlapic) &&
+ !vlapic_x2apic_mode(vlapic) &&
+ (offset < PAGE_SIZE);
}
const struct hvm_mmio_handler vlapic_mmio_handler = {
@@ -919,10 +960,12 @@ const struct hvm_mmio_handler vlapic_mmi
.write_handler = vlapic_write
};
-void vlapic_msr_set(struct vlapic *vlapic, uint64_t value)
+bool_t vlapic_msr_set(struct vlapic *vlapic, uint64_t value)
{
if ( (vlapic->hw.apic_base_msr ^ value) & MSR_IA32_APICBASE_ENABLE )
{
+ if ( unlikely(value & MSR_IA32_APICBASE_EXTD) )
+ return 0;
if ( value & MSR_IA32_APICBASE_ENABLE )
{
vlapic_reset(vlapic);
@@ -931,10 +974,15 @@ void vlapic_msr_set(struct vlapic *vlapi
}
else
{
+ if ( unlikely(vlapic_x2apic_mode(vlapic)) )
+ return 0;
vlapic->hw.disabled |= VLAPIC_HW_DISABLED;
pt_may_unmask_irq(vlapic_domain(vlapic), NULL);
}
}
+ else if ( !(value & MSR_IA32_APICBASE_ENABLE) &&
+ unlikely(value & MSR_IA32_APICBASE_EXTD) )
+ return 0;
vlapic->hw.apic_base_msr = value;
@@ -949,6 +997,8 @@ void vlapic_msr_set(struct vlapic *vlapi
HVM_DBG_LOG(DBG_LEVEL_VLAPIC,
"apic base msr is 0x%016"PRIx64, vlapic->hw.apic_base_msr);
+
+ return 1;
}
uint64_t vlapic_tdt_msr_get(struct vlapic *vlapic)
@@ -1232,6 +1282,10 @@ static int lapic_load_hidden(struct doma
if ( hvm_load_entry_zeroextend(LAPIC, h, &s->hw) != 0 )
return -EINVAL;
+ if ( !(s->hw.apic_base_msr & MSR_IA32_APICBASE_ENABLE) &&
+ unlikely(vlapic_x2apic_mode(s)) )
+ return -EINVAL;
+
vmx_vlapic_msr_changed(v);
return 0;
--- a/xen/arch/x86/hvm/vmx/vmx.c
+++ b/xen/arch/x86/hvm/vmx/vmx.c
@@ -3110,8 +3110,7 @@ void vmx_vmexit_handler(struct cpu_user_
break;
case EXIT_REASON_APIC_WRITE:
- if ( vmx_handle_apic_write() )
- hvm_inject_hw_exception(TRAP_gp_fault, 0);
+ vmx_handle_apic_write();
break;
case EXIT_REASON_ACCESS_GDTR_OR_IDTR:
--- a/xen/include/asm-x86/hvm/vlapic.h
+++ b/xen/include/asm-x86/hvm/vlapic.h
@@ -106,7 +106,7 @@ void vlapic_destroy(struct vcpu *v);
void vlapic_reset(struct vlapic *vlapic);
-void vlapic_msr_set(struct vlapic *vlapic, uint64_t value);
+bool_t vlapic_msr_set(struct vlapic *vlapic, uint64_t value);
void vlapic_tdt_msr_set(struct vlapic *vlapic, uint64_t value);
uint64_t vlapic_tdt_msr_get(struct vlapic *vlapic);
[-- Attachment #3: Type: text/plain, Size: 126 bytes --]
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
http://lists.xen.org/xen-devel
^ permalink raw reply [flat|nested] 11+ messages in thread
* [PATCH v5 2/4] x86/HVM: fix ID handling of x2APIC emulation
2014-09-24 15:28 [PATCH v5 0/4] x86/HVM: fix various aspects of APIC emulation Jan Beulich
2014-09-24 15:34 ` [PATCH v5 1/4] x86/HVM: fix miscellaneous aspects of x2APIC emulation Jan Beulich
@ 2014-09-24 15:35 ` Jan Beulich
2014-09-24 15:59 ` Andrew Cooper
2014-09-24 15:36 ` [PATCH v5 3/4] x86/vlapic: a few type adjustments Jan Beulich
` (2 subsequent siblings)
4 siblings, 1 reply; 11+ messages in thread
From: Jan Beulich @ 2014-09-24 15:35 UTC (permalink / raw)
To: xen-devel; +Cc: Andrew Cooper, Keir Fraser
[-- Attachment #1: Type: text/plain, Size: 8580 bytes --]
- 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)
- 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()
Signed-off-by: Jan Beulich <jbeulich@suse.com>
---
v5: Convert optional checks done in lapic_load_fixup() to trigger a
printk() instead of preventing the fixup. Move unrelated type
changes to 3rd patch.
v4: Replace unpause-based approach with one latching most recently
loaded values.
v2: Some changes broken out to separate patch. Correct ID and
LDR after domain restore (if necessary); as stated previously the
only compatibility problem this creates is when migrating a VM _to_
an unfixed (i.e. old) hypervisor, a scenario which supposedly isn't
supported.
--- 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) )
{
@@ -208,7 +207,7 @@ 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)
+ int short_hand, uint32_t dest, uint8_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);
@@ -353,7 +353,7 @@ 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)
+ int short_hand, uint32_t dest, uint8_t dest_mode)
{
int old = d->arch.hvm_domain.irq.round_robin_prev_vcpu;
uint32_t ppr, target_ppr = UINT_MAX;
@@ -438,9 +438,7 @@ void vlapic_ipi(
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 )
{
@@ -619,10 +617,6 @@ 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;
@@ -656,6 +650,8 @@ static int vlapic_reg_write(struct vcpu
struct vlapic *vlapic = vcpu_vlapic(v);
int rc = X86EMUL_OKAY;
+ memset(&vlapic->loaded, 0, sizeof(vlapic->loaded));
+
switch ( offset )
{
case APIC_ID:
@@ -960,6 +956,15 @@ const struct hvm_mmio_handler vlapic_mmi
.write_handler = vlapic_write
};
+static void set_x2apic_id(struct vlapic *vlapic)
+{
+ 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);
+}
+
bool_t vlapic_msr_set(struct vlapic *vlapic, uint64_t value)
{
if ( (vlapic->hw.apic_base_msr ^ value) & MSR_IA32_APICBASE_ENABLE )
@@ -985,13 +990,10 @@ bool_t vlapic_msr_set(struct vlapic *vla
return 0;
vlapic->hw.apic_base_msr = value;
+ memset(&vlapic->loaded, 0, sizeof(vlapic->loaded));
if ( vlapic_x2apic_mode(vlapic) )
- {
- u32 id = vlapic_get_reg(vlapic, APIC_ID);
- u32 ldr = ((id & ~0xf) << 16) | (1 << (id & 0xf));
- vlapic_set_reg(vlapic, APIC_LDR, ldr);
- }
+ set_x2apic_id(vlapic);
vmx_vlapic_msr_changed(vlapic_vcpu(vlapic));
@@ -1263,6 +1265,35 @@ static int lapic_save_regs(struct domain
return rc;
}
+/*
+ * Following lapic_load_hidden()/lapic_load_regs() we may need to
+ * correct ID and LDR when they come from an old, broken hypervisor.
+ */
+static void lapic_load_fixup(struct vlapic *vlapic)
+{
+ uint32_t id = vlapic->loaded.id;
+
+ if ( vlapic_x2apic_mode(vlapic) && id && vlapic->loaded.ldr == 1 )
+ {
+ /*
+ * 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.
+ */
+ 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);
+ }
+}
+
static int lapic_load_hidden(struct domain *d, hvm_domain_context_t *h)
{
uint16_t vcpuid;
@@ -1282,6 +1313,10 @@ static int lapic_load_hidden(struct doma
if ( hvm_load_entry_zeroextend(LAPIC, h, &s->hw) != 0 )
return -EINVAL;
+ s->loaded.hw = 1;
+ if ( s->loaded.regs )
+ lapic_load_fixup(s);
+
if ( !(s->hw.apic_base_msr & MSR_IA32_APICBASE_ENABLE) &&
unlikely(vlapic_x2apic_mode(s)) )
return -EINVAL;
@@ -1310,6 +1345,12 @@ static int lapic_load_regs(struct domain
if ( hvm_load_entry(LAPIC_REGS, h, s->regs) != 0 )
return -EINVAL;
+ s->loaded.id = vlapic_get_reg(s, APIC_ID);
+ s->loaded.ldr = vlapic_get_reg(s, APIC_LDR);
+ s->loaded.regs = 1;
+ if ( s->loaded.hw )
+ lapic_load_fixup(s);
+
if ( hvm_funcs.process_isr )
hvm_funcs.process_isr(vlapic_find_highest_isr(s), v);
--- 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:
@@ -70,6 +71,10 @@
struct vlapic {
struct hvm_hw_lapic hw;
struct hvm_hw_lapic_regs *regs;
+ struct {
+ bool_t hw, regs;
+ uint32_t id, ldr;
+ } loaded;
struct periodic_time pt;
s_time_t timer_last_update;
struct page_info *regs_page;
@@ -124,10 +129,10 @@ int vlapic_apicv_write(struct vcpu *v, u
struct vlapic *vlapic_lowest_prio(
struct domain *d, struct vlapic *source,
- int short_hand, uint8_t dest, uint8_t dest_mode);
+ int short_hand, uint32_t dest, uint8_t dest_mode);
bool_t vlapic_match_dest(
struct vlapic *target, struct vlapic *source,
- int short_hand, uint8_t dest, uint8_t dest_mode);
+ int short_hand, uint32_t dest, uint8_t dest_mode);
#endif /* __ASM_X86_HVM_VLAPIC_H__ */
[-- Attachment #2: x86-HVM-x2APIC-id.patch --]
[-- Type: text/plain, Size: 8624 bytes --]
x86/HVM: fix ID handling of x2APIC emulation
- 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)
- 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()
Signed-off-by: Jan Beulich <jbeulich@suse.com>
---
v5: Convert optional checks done in lapic_load_fixup() to trigger a
printk() instead of preventing the fixup. Move unrelated type
changes to 3rd patch.
v4: Replace unpause-based approach with one latching most recently
loaded values.
v2: Some changes broken out to separate patch. Correct ID and
LDR after domain restore (if necessary); as stated previously the
only compatibility problem this creates is when migrating a VM _to_
an unfixed (i.e. old) hypervisor, a scenario which supposedly isn't
supported.
--- 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) )
{
@@ -208,7 +207,7 @@ 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)
+ int short_hand, uint32_t dest, uint8_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);
@@ -353,7 +353,7 @@ 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)
+ int short_hand, uint32_t dest, uint8_t dest_mode)
{
int old = d->arch.hvm_domain.irq.round_robin_prev_vcpu;
uint32_t ppr, target_ppr = UINT_MAX;
@@ -438,9 +438,7 @@ void vlapic_ipi(
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 )
{
@@ -619,10 +617,6 @@ 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;
@@ -656,6 +650,8 @@ static int vlapic_reg_write(struct vcpu
struct vlapic *vlapic = vcpu_vlapic(v);
int rc = X86EMUL_OKAY;
+ memset(&vlapic->loaded, 0, sizeof(vlapic->loaded));
+
switch ( offset )
{
case APIC_ID:
@@ -960,6 +956,15 @@ const struct hvm_mmio_handler vlapic_mmi
.write_handler = vlapic_write
};
+static void set_x2apic_id(struct vlapic *vlapic)
+{
+ 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);
+}
+
bool_t vlapic_msr_set(struct vlapic *vlapic, uint64_t value)
{
if ( (vlapic->hw.apic_base_msr ^ value) & MSR_IA32_APICBASE_ENABLE )
@@ -985,13 +990,10 @@ bool_t vlapic_msr_set(struct vlapic *vla
return 0;
vlapic->hw.apic_base_msr = value;
+ memset(&vlapic->loaded, 0, sizeof(vlapic->loaded));
if ( vlapic_x2apic_mode(vlapic) )
- {
- u32 id = vlapic_get_reg(vlapic, APIC_ID);
- u32 ldr = ((id & ~0xf) << 16) | (1 << (id & 0xf));
- vlapic_set_reg(vlapic, APIC_LDR, ldr);
- }
+ set_x2apic_id(vlapic);
vmx_vlapic_msr_changed(vlapic_vcpu(vlapic));
@@ -1263,6 +1265,35 @@ static int lapic_save_regs(struct domain
return rc;
}
+/*
+ * Following lapic_load_hidden()/lapic_load_regs() we may need to
+ * correct ID and LDR when they come from an old, broken hypervisor.
+ */
+static void lapic_load_fixup(struct vlapic *vlapic)
+{
+ uint32_t id = vlapic->loaded.id;
+
+ if ( vlapic_x2apic_mode(vlapic) && id && vlapic->loaded.ldr == 1 )
+ {
+ /*
+ * 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.
+ */
+ 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);
+ }
+}
+
static int lapic_load_hidden(struct domain *d, hvm_domain_context_t *h)
{
uint16_t vcpuid;
@@ -1282,6 +1313,10 @@ static int lapic_load_hidden(struct doma
if ( hvm_load_entry_zeroextend(LAPIC, h, &s->hw) != 0 )
return -EINVAL;
+ s->loaded.hw = 1;
+ if ( s->loaded.regs )
+ lapic_load_fixup(s);
+
if ( !(s->hw.apic_base_msr & MSR_IA32_APICBASE_ENABLE) &&
unlikely(vlapic_x2apic_mode(s)) )
return -EINVAL;
@@ -1310,6 +1345,12 @@ static int lapic_load_regs(struct domain
if ( hvm_load_entry(LAPIC_REGS, h, s->regs) != 0 )
return -EINVAL;
+ s->loaded.id = vlapic_get_reg(s, APIC_ID);
+ s->loaded.ldr = vlapic_get_reg(s, APIC_LDR);
+ s->loaded.regs = 1;
+ if ( s->loaded.hw )
+ lapic_load_fixup(s);
+
if ( hvm_funcs.process_isr )
hvm_funcs.process_isr(vlapic_find_highest_isr(s), v);
--- 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:
@@ -70,6 +71,10 @@
struct vlapic {
struct hvm_hw_lapic hw;
struct hvm_hw_lapic_regs *regs;
+ struct {
+ bool_t hw, regs;
+ uint32_t id, ldr;
+ } loaded;
struct periodic_time pt;
s_time_t timer_last_update;
struct page_info *regs_page;
@@ -124,10 +129,10 @@ int vlapic_apicv_write(struct vcpu *v, u
struct vlapic *vlapic_lowest_prio(
struct domain *d, struct vlapic *source,
- int short_hand, uint8_t dest, uint8_t dest_mode);
+ int short_hand, uint32_t dest, uint8_t dest_mode);
bool_t vlapic_match_dest(
struct vlapic *target, struct vlapic *source,
- int short_hand, uint8_t dest, uint8_t dest_mode);
+ int short_hand, uint32_t dest, uint8_t dest_mode);
#endif /* __ASM_X86_HVM_VLAPIC_H__ */
[-- Attachment #3: Type: text/plain, Size: 126 bytes --]
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
http://lists.xen.org/xen-devel
^ permalink raw reply [flat|nested] 11+ messages in thread
* [PATCH v5 3/4] x86/vlapic: a few type adjustments
2014-09-24 15:28 [PATCH v5 0/4] x86/HVM: fix various aspects of APIC emulation Jan Beulich
2014-09-24 15:34 ` [PATCH v5 1/4] x86/HVM: fix miscellaneous aspects of x2APIC emulation Jan Beulich
2014-09-24 15:35 ` [PATCH v5 2/4] x86/HVM: fix ID handling " Jan Beulich
@ 2014-09-24 15:36 ` Jan Beulich
2014-09-24 16:02 ` Andrew Cooper
2014-09-24 15:37 ` [PATCH v5 4/4] x86/vlapic: don't silently accept bad vectors Jan Beulich
2014-09-25 11:17 ` [PATCH v5 0/4] x86/HVM: fix various aspects of APIC emulation Tim Deegan
4 siblings, 1 reply; 11+ messages in thread
From: Jan Beulich @ 2014-09-24 15:36 UTC (permalink / raw)
To: xen-devel; +Cc: Andrew Cooper, Keir Fraser
[-- Attachment #1: Type: text/plain, Size: 4456 bytes --]
Constify a couple of pointer parameters, convert a boolean function
return type to bool_t, and clean up a printk() being touched anyway.
Suggested-by: Andrew Cooper <andrew.cooper3@citrix.com>
Signed-off-by: Jan Beulich <jbeulich@suse.com>
---
v5: Move some more type changes from patch 2 to here.
--- a/xen/arch/x86/hvm/vlapic.c
+++ b/xen/arch/x86/hvm/vlapic.c
@@ -173,9 +173,10 @@ uint32_t vlapic_set_ppr(struct vlapic *v
return ppr;
}
-static int vlapic_match_logical_addr(struct vlapic *vlapic, uint32_t mda)
+static bool_t vlapic_match_logical_addr(const struct vlapic *vlapic,
+ uint32_t mda)
{
- int result = 0;
+ bool_t result = 0;
uint32_t logical_id = vlapic_get_reg(vlapic, APIC_LDR);
if ( vlapic_x2apic_mode(vlapic) )
@@ -196,9 +197,9 @@ static int vlapic_match_logical_addr(str
result = 1;
break;
default:
- gdprintk(XENLOG_WARNING, "Bad DFR value for lapic of vcpu %d: %08x\n",
- vlapic_vcpu(vlapic)->vcpu_id,
- vlapic_get_reg(vlapic, APIC_DFR));
+ printk(XENLOG_G_WARNING "%pv: bad LAPIC DFR value %08x\n",
+ const_vlapic_vcpu(vlapic),
+ vlapic_get_reg(vlapic, APIC_DFR));
break;
}
@@ -206,8 +207,8 @@ static int vlapic_match_logical_addr(str
}
bool_t vlapic_match_dest(
- struct vlapic *target, struct vlapic *source,
- int short_hand, uint32_t dest, uint8_t dest_mode)
+ const 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",
@@ -286,7 +287,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 +353,8 @@ static void vlapic_accept_irq(struct vcp
}
struct vlapic *vlapic_lowest_prio(
- struct domain *d, struct vlapic *source,
- int short_hand, uint32_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;
@@ -434,7 +435,7 @@ 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);
--- a/xen/include/asm-x86/hvm/vlapic.h
+++ b/xen/include/asm-x86/hvm/vlapic.h
@@ -28,6 +28,8 @@
#define vcpu_vlapic(x) (&(x)->arch.hvm_vcpu.vlapic)
#define vlapic_vcpu(x) (container_of((x), struct vcpu, arch.hvm_vcpu.vlapic))
+#define const_vlapic_vcpu(x) (container_of((x), const struct vcpu, \
+ arch.hvm_vcpu.vlapic))
#define vlapic_domain(x) (vlapic_vcpu(x)->domain)
#define _VLAPIC_ID(vlapic, id) (vlapic_x2apic_mode(vlapic) \
@@ -88,7 +90,8 @@ struct vlapic {
/* vlapic's frequence is 100 MHz */
#define APIC_BUS_CYCLE_NS 10
-static inline uint32_t vlapic_get_reg(struct vlapic *vlapic, uint32_t reg)
+static inline uint32_t vlapic_get_reg(const struct vlapic *vlapic,
+ uint32_t reg)
{
return *((uint32_t *)(&vlapic->regs->data[reg]));
}
@@ -128,11 +131,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, uint32_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, uint32_t dest, uint8_t dest_mode);
+ const struct vlapic *target, const struct vlapic *source,
+ int short_hand, uint32_t dest, bool_t dest_mode);
#endif /* __ASM_X86_HVM_VLAPIC_H__ */
[-- Attachment #2: x86-HVM-x2APIC-types.patch --]
[-- Type: text/plain, Size: 4490 bytes --]
x86/vlapic: a few type adjustments
Constify a couple of pointer parameters, convert a boolean function
return type to bool_t, and clean up a printk() being touched anyway.
Suggested-by: Andrew Cooper <andrew.cooper3@citrix.com>
Signed-off-by: Jan Beulich <jbeulich@suse.com>
---
v5: Move some more type changes from patch 2 to here.
--- a/xen/arch/x86/hvm/vlapic.c
+++ b/xen/arch/x86/hvm/vlapic.c
@@ -173,9 +173,10 @@ uint32_t vlapic_set_ppr(struct vlapic *v
return ppr;
}
-static int vlapic_match_logical_addr(struct vlapic *vlapic, uint32_t mda)
+static bool_t vlapic_match_logical_addr(const struct vlapic *vlapic,
+ uint32_t mda)
{
- int result = 0;
+ bool_t result = 0;
uint32_t logical_id = vlapic_get_reg(vlapic, APIC_LDR);
if ( vlapic_x2apic_mode(vlapic) )
@@ -196,9 +197,9 @@ static int vlapic_match_logical_addr(str
result = 1;
break;
default:
- gdprintk(XENLOG_WARNING, "Bad DFR value for lapic of vcpu %d: %08x\n",
- vlapic_vcpu(vlapic)->vcpu_id,
- vlapic_get_reg(vlapic, APIC_DFR));
+ printk(XENLOG_G_WARNING "%pv: bad LAPIC DFR value %08x\n",
+ const_vlapic_vcpu(vlapic),
+ vlapic_get_reg(vlapic, APIC_DFR));
break;
}
@@ -206,8 +207,8 @@ static int vlapic_match_logical_addr(str
}
bool_t vlapic_match_dest(
- struct vlapic *target, struct vlapic *source,
- int short_hand, uint32_t dest, uint8_t dest_mode)
+ const 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",
@@ -286,7 +287,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 +353,8 @@ static void vlapic_accept_irq(struct vcp
}
struct vlapic *vlapic_lowest_prio(
- struct domain *d, struct vlapic *source,
- int short_hand, uint32_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;
@@ -434,7 +435,7 @@ 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);
--- a/xen/include/asm-x86/hvm/vlapic.h
+++ b/xen/include/asm-x86/hvm/vlapic.h
@@ -28,6 +28,8 @@
#define vcpu_vlapic(x) (&(x)->arch.hvm_vcpu.vlapic)
#define vlapic_vcpu(x) (container_of((x), struct vcpu, arch.hvm_vcpu.vlapic))
+#define const_vlapic_vcpu(x) (container_of((x), const struct vcpu, \
+ arch.hvm_vcpu.vlapic))
#define vlapic_domain(x) (vlapic_vcpu(x)->domain)
#define _VLAPIC_ID(vlapic, id) (vlapic_x2apic_mode(vlapic) \
@@ -88,7 +90,8 @@ struct vlapic {
/* vlapic's frequence is 100 MHz */
#define APIC_BUS_CYCLE_NS 10
-static inline uint32_t vlapic_get_reg(struct vlapic *vlapic, uint32_t reg)
+static inline uint32_t vlapic_get_reg(const struct vlapic *vlapic,
+ uint32_t reg)
{
return *((uint32_t *)(&vlapic->regs->data[reg]));
}
@@ -128,11 +131,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, uint32_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, uint32_t dest, uint8_t dest_mode);
+ const struct vlapic *target, const struct vlapic *source,
+ int short_hand, uint32_t dest, bool_t dest_mode);
#endif /* __ASM_X86_HVM_VLAPIC_H__ */
[-- Attachment #3: Type: text/plain, Size: 126 bytes --]
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
http://lists.xen.org/xen-devel
^ permalink raw reply [flat|nested] 11+ messages in thread
* [PATCH v5 4/4] x86/vlapic: don't silently accept bad vectors
2014-09-24 15:28 [PATCH v5 0/4] x86/HVM: fix various aspects of APIC emulation Jan Beulich
` (2 preceding siblings ...)
2014-09-24 15:36 ` [PATCH v5 3/4] x86/vlapic: a few type adjustments Jan Beulich
@ 2014-09-24 15:37 ` Jan Beulich
2014-09-25 11:17 ` [PATCH v5 0/4] x86/HVM: fix various aspects of APIC emulation Tim Deegan
4 siblings, 0 replies; 11+ messages in thread
From: Jan Beulich @ 2014-09-24 15:37 UTC (permalink / raw)
To: xen-devel; +Cc: Andrew Cooper, Keir Fraser
[-- Attachment #1: Type: text/plain, Size: 3108 bytes --]
Vectors 0-15 are reserved, and a physical LAPIC - upon sending or
receiving one - would generate an APIC error instead of doing the
requested action. Make our emulation behave similarly.
Signed-off-by: Jan Beulich <jbeulich@suse.com>
Reviewed-by: Andrew Cooper <andrew.cooper3@citrix.com>
--- a/xen/arch/x86/hvm/vlapic.c
+++ b/xen/arch/x86/hvm/vlapic.c
@@ -123,10 +123,34 @@ static int vlapic_find_highest_irr(struc
return vlapic_find_highest_vector(&vlapic->regs->data[APIC_IRR]);
}
+static void vlapic_error(struct vlapic *vlapic, unsigned int errmask)
+{
+ unsigned long flags;
+ uint32_t esr;
+
+ spin_lock_irqsave(&vlapic->esr_lock, flags);
+ esr = vlapic_get_reg(vlapic, APIC_ESR);
+ if ( (esr & errmask) != errmask )
+ {
+ uint32_t lvterr = vlapic_get_reg(vlapic, APIC_LVTERR);
+
+ vlapic_set_reg(vlapic, APIC_ESR, esr | errmask);
+ if ( !(lvterr & APIC_LVT_MASKED) )
+ vlapic_set_irq(vlapic, lvterr & APIC_VECTOR_MASK, 0);
+ }
+ spin_unlock_irqrestore(&vlapic->esr_lock, flags);
+}
+
void vlapic_set_irq(struct vlapic *vlapic, uint8_t vec, uint8_t trig)
{
struct vcpu *target = vlapic_vcpu(vlapic);
+ if ( unlikely(vec < 16) )
+ {
+ vlapic_error(vlapic, APIC_ESR_RECVILL);
+ return;
+ }
+
if ( trig )
vlapic_set_vector(vec, &vlapic->regs->data[APIC_TMR]);
@@ -460,7 +484,12 @@ void vlapic_ipi(
struct vlapic *target = vlapic_lowest_prio(
vlapic_domain(vlapic), vlapic, short_hand, dest, dest_mode);
if ( target != NULL )
- vlapic_accept_irq(vlapic_vcpu(target), icr_low);
+ {
+ if ( likely((icr_low & APIC_VECTOR_MASK) >= 16) )
+ vlapic_accept_irq(vlapic_vcpu(target), icr_low);
+ else
+ vlapic_error(vlapic, APIC_ESR_SENDILL);
+ }
break;
}
@@ -468,6 +497,11 @@ void vlapic_ipi(
struct vcpu *v;
bool_t batch = is_multicast_dest(vlapic, short_hand, dest, dest_mode);
+ if ( unlikely((icr_low & APIC_VECTOR_MASK) < 16) )
+ {
+ vlapic_error(vlapic, APIC_ESR_SENDILL);
+ break;
+ }
if ( batch )
cpu_raise_softirq_batch_begin();
for_each_vcpu ( vlapic_domain(vlapic), v )
@@ -1403,6 +1437,8 @@ int vlapic_init(struct vcpu *v)
if ( v->vcpu_id == 0 )
vlapic->hw.apic_base_msr |= MSR_IA32_APICBASE_BSP;
+ spin_lock_init(&vlapic->esr_lock);
+
tasklet_init(&vlapic->init_sipi.tasklet,
vlapic_init_sipi_action,
(unsigned long)v);
--- a/xen/include/asm-x86/hvm/vlapic.h
+++ b/xen/include/asm-x86/hvm/vlapic.h
@@ -77,6 +77,7 @@ struct vlapic {
bool_t hw, regs;
uint32_t id, ldr;
} loaded;
+ spinlock_t esr_lock;
struct periodic_time pt;
s_time_t timer_last_update;
struct page_info *regs_page;
[-- Attachment #2: x86-HVM-LAPIC-bad-vector.patch --]
[-- Type: text/plain, Size: 3151 bytes --]
x86/vlapic: don't silently accept bad vectors
Vectors 0-15 are reserved, and a physical LAPIC - upon sending or
receiving one - would generate an APIC error instead of doing the
requested action. Make our emulation behave similarly.
Signed-off-by: Jan Beulich <jbeulich@suse.com>
Reviewed-by: Andrew Cooper <andrew.cooper3@citrix.com>
--- a/xen/arch/x86/hvm/vlapic.c
+++ b/xen/arch/x86/hvm/vlapic.c
@@ -123,10 +123,34 @@ static int vlapic_find_highest_irr(struc
return vlapic_find_highest_vector(&vlapic->regs->data[APIC_IRR]);
}
+static void vlapic_error(struct vlapic *vlapic, unsigned int errmask)
+{
+ unsigned long flags;
+ uint32_t esr;
+
+ spin_lock_irqsave(&vlapic->esr_lock, flags);
+ esr = vlapic_get_reg(vlapic, APIC_ESR);
+ if ( (esr & errmask) != errmask )
+ {
+ uint32_t lvterr = vlapic_get_reg(vlapic, APIC_LVTERR);
+
+ vlapic_set_reg(vlapic, APIC_ESR, esr | errmask);
+ if ( !(lvterr & APIC_LVT_MASKED) )
+ vlapic_set_irq(vlapic, lvterr & APIC_VECTOR_MASK, 0);
+ }
+ spin_unlock_irqrestore(&vlapic->esr_lock, flags);
+}
+
void vlapic_set_irq(struct vlapic *vlapic, uint8_t vec, uint8_t trig)
{
struct vcpu *target = vlapic_vcpu(vlapic);
+ if ( unlikely(vec < 16) )
+ {
+ vlapic_error(vlapic, APIC_ESR_RECVILL);
+ return;
+ }
+
if ( trig )
vlapic_set_vector(vec, &vlapic->regs->data[APIC_TMR]);
@@ -460,7 +484,12 @@ void vlapic_ipi(
struct vlapic *target = vlapic_lowest_prio(
vlapic_domain(vlapic), vlapic, short_hand, dest, dest_mode);
if ( target != NULL )
- vlapic_accept_irq(vlapic_vcpu(target), icr_low);
+ {
+ if ( likely((icr_low & APIC_VECTOR_MASK) >= 16) )
+ vlapic_accept_irq(vlapic_vcpu(target), icr_low);
+ else
+ vlapic_error(vlapic, APIC_ESR_SENDILL);
+ }
break;
}
@@ -468,6 +497,11 @@ void vlapic_ipi(
struct vcpu *v;
bool_t batch = is_multicast_dest(vlapic, short_hand, dest, dest_mode);
+ if ( unlikely((icr_low & APIC_VECTOR_MASK) < 16) )
+ {
+ vlapic_error(vlapic, APIC_ESR_SENDILL);
+ break;
+ }
if ( batch )
cpu_raise_softirq_batch_begin();
for_each_vcpu ( vlapic_domain(vlapic), v )
@@ -1403,6 +1437,8 @@ int vlapic_init(struct vcpu *v)
if ( v->vcpu_id == 0 )
vlapic->hw.apic_base_msr |= MSR_IA32_APICBASE_BSP;
+ spin_lock_init(&vlapic->esr_lock);
+
tasklet_init(&vlapic->init_sipi.tasklet,
vlapic_init_sipi_action,
(unsigned long)v);
--- a/xen/include/asm-x86/hvm/vlapic.h
+++ b/xen/include/asm-x86/hvm/vlapic.h
@@ -77,6 +77,7 @@ struct vlapic {
bool_t hw, regs;
uint32_t id, ldr;
} loaded;
+ spinlock_t esr_lock;
struct periodic_time pt;
s_time_t timer_last_update;
struct page_info *regs_page;
[-- Attachment #3: Type: text/plain, Size: 126 bytes --]
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
http://lists.xen.org/xen-devel
^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: [PATCH v5 1/4] x86/HVM: fix miscellaneous aspects of x2APIC emulation
2014-09-24 15:34 ` [PATCH v5 1/4] x86/HVM: fix miscellaneous aspects of x2APIC emulation Jan Beulich
@ 2014-09-24 15:57 ` Andrew Cooper
2014-09-25 8:42 ` Jan Beulich
0 siblings, 1 reply; 11+ messages in thread
From: Andrew Cooper @ 2014-09-24 15:57 UTC (permalink / raw)
To: Jan Beulich, xen-devel; +Cc: Keir Fraser
On 24/09/14 16:34, Jan Beulich wrote:
> - generate #GP on invalid APIC base MSR transitions
> - fail reads from the EOI and self-IPI registers (which are write-only)
> - handle self-IPI writes and the ICR2 half of ICR writes largely in
> hvm_x2apic_msr_write() and (for self-IPI only) vlapic_apicv_write()
> - don't permit MMIO-based access in x2APIC mode
> - filter writes to read-only registers in hvm_x2apic_msr_write(),
> allowing conditionals to be dropped from vlapic_reg_write()
> - don't ignore upper half of MSR-based write to ESR being non-zero
> - don't ignore other writes to reserved bits
> - VMX's EXIT_REASON_APIC_WRITE must not result in #GP (this exit being
> trap-like, this exception would get raised on the wrong RIP)
> - make hvm_x2apic_msr_read() produce X86EMUL_* return codes just like
> hvm_x2apic_msr_write() does (benign to the only caller)
>
> Signed-off-by: Jan Beulich <jbeulich@suse.com>
One nit and one bug.
> @@ -877,8 +854,16 @@ static int vlapic_write(struct vcpu *v,
>
> int vlapic_apicv_write(struct vcpu *v, unsigned int offset)
> {
> - uint32_t val = vlapic_get_reg(vcpu_vlapic(v), offset);
> - return vlapic_reg_write(v, offset, val);
> + struct vlapic *vlapic = vcpu_vlapic(v);
> + uint32_t val = vlapic_get_reg(vlapic, offset);
> +
> + if ( !vlapic_x2apic_mode(vlapic) )
> + return vlapic_reg_write(v, offset, val);
> +
> + if ( offset != APIC_SELF_IPI )
> + return X86EMUL_UNHANDLEABLE;
> +
> + return vlapic_reg_write(v, APIC_ICR, APIC_DEST_SELF | (uint8_t)val);
(val & APIC_VECTOR_MASK) instead of a uint8_t cast?
> }
>
> int hvm_x2apic_msr_write(struct vcpu *v, unsigned int msr, uint64_t msr_content)
> @@ -891,16 +876,69 @@ int hvm_x2apic_msr_write(struct vcpu *v,
>
> switch ( offset )
> {
> - int rc;
> + case APIC_TASKPRI:
> + if ( msr_content & ~APIC_TPRI_MASK )
> + return X86EMUL_UNHANDLEABLE;
> + break;
> +
> + case APIC_SPIV:
> + if ( msr_content & ~(APIC_VECTOR_MASK | APIC_SPIV_APIC_ENABLED |
> + (VLAPIC_VERSION & APIC_LVR_DIRECTED_EOI
Need brackets for the logical (VLAPIC_VERSION & APIC_LVR_DIRECTED_EOI)
test to avoid making a constant expression with the precedence of the
ternary operator.
> + ? APIC_SPIV_DIRECTED_EOI : 0)) )
> + return X86EMUL_UNHANDLEABLE;
> + break;
> +
Otherwise, Reviewed-by: Andrew Cooper <andrew.cooper3@citrix.com>
^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: [PATCH v5 2/4] x86/HVM: fix ID handling of x2APIC emulation
2014-09-24 15:35 ` [PATCH v5 2/4] x86/HVM: fix ID handling " Jan Beulich
@ 2014-09-24 15:59 ` Andrew Cooper
0 siblings, 0 replies; 11+ messages in thread
From: Andrew Cooper @ 2014-09-24 15:59 UTC (permalink / raw)
To: Jan Beulich, xen-devel; +Cc: Keir Fraser
On 24/09/14 16:35, Jan Beulich wrote:
> - 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)
> - 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()
>
> Signed-off-by: Jan Beulich <jbeulich@suse.com>
Much clearer with some of the changes moved to patch 3.
Reviewed-by: Andrew Cooper <andrew.cooper3@citrix.com>
> ---
> v5: Convert optional checks done in lapic_load_fixup() to trigger a
> printk() instead of preventing the fixup. Move unrelated type
> changes to 3rd patch.
> v4: Replace unpause-based approach with one latching most recently
> loaded values.
> v2: Some changes broken out to separate patch. Correct ID and
> LDR after domain restore (if necessary); as stated previously the
> only compatibility problem this creates is when migrating a VM _to_
> an unfixed (i.e. old) hypervisor, a scenario which supposedly isn't
> supported.
>
> --- 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) )
> {
> @@ -208,7 +207,7 @@ 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)
> + int short_hand, uint32_t dest, uint8_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);
> @@ -353,7 +353,7 @@ 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)
> + int short_hand, uint32_t dest, uint8_t dest_mode)
> {
> int old = d->arch.hvm_domain.irq.round_robin_prev_vcpu;
> uint32_t ppr, target_ppr = UINT_MAX;
> @@ -438,9 +438,7 @@ void vlapic_ipi(
>
> 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 )
> {
> @@ -619,10 +617,6 @@ 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;
> @@ -656,6 +650,8 @@ static int vlapic_reg_write(struct vcpu
> struct vlapic *vlapic = vcpu_vlapic(v);
> int rc = X86EMUL_OKAY;
>
> + memset(&vlapic->loaded, 0, sizeof(vlapic->loaded));
> +
> switch ( offset )
> {
> case APIC_ID:
> @@ -960,6 +956,15 @@ const struct hvm_mmio_handler vlapic_mmi
> .write_handler = vlapic_write
> };
>
> +static void set_x2apic_id(struct vlapic *vlapic)
> +{
> + 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);
> +}
> +
> bool_t vlapic_msr_set(struct vlapic *vlapic, uint64_t value)
> {
> if ( (vlapic->hw.apic_base_msr ^ value) & MSR_IA32_APICBASE_ENABLE )
> @@ -985,13 +990,10 @@ bool_t vlapic_msr_set(struct vlapic *vla
> return 0;
>
> vlapic->hw.apic_base_msr = value;
> + memset(&vlapic->loaded, 0, sizeof(vlapic->loaded));
>
> if ( vlapic_x2apic_mode(vlapic) )
> - {
> - u32 id = vlapic_get_reg(vlapic, APIC_ID);
> - u32 ldr = ((id & ~0xf) << 16) | (1 << (id & 0xf));
> - vlapic_set_reg(vlapic, APIC_LDR, ldr);
> - }
> + set_x2apic_id(vlapic);
>
> vmx_vlapic_msr_changed(vlapic_vcpu(vlapic));
>
> @@ -1263,6 +1265,35 @@ static int lapic_save_regs(struct domain
> return rc;
> }
>
> +/*
> + * Following lapic_load_hidden()/lapic_load_regs() we may need to
> + * correct ID and LDR when they come from an old, broken hypervisor.
> + */
> +static void lapic_load_fixup(struct vlapic *vlapic)
> +{
> + uint32_t id = vlapic->loaded.id;
> +
> + if ( vlapic_x2apic_mode(vlapic) && id && vlapic->loaded.ldr == 1 )
> + {
> + /*
> + * 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.
> + */
> + 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);
> + }
> +}
> +
> static int lapic_load_hidden(struct domain *d, hvm_domain_context_t *h)
> {
> uint16_t vcpuid;
> @@ -1282,6 +1313,10 @@ static int lapic_load_hidden(struct doma
> if ( hvm_load_entry_zeroextend(LAPIC, h, &s->hw) != 0 )
> return -EINVAL;
>
> + s->loaded.hw = 1;
> + if ( s->loaded.regs )
> + lapic_load_fixup(s);
> +
> if ( !(s->hw.apic_base_msr & MSR_IA32_APICBASE_ENABLE) &&
> unlikely(vlapic_x2apic_mode(s)) )
> return -EINVAL;
> @@ -1310,6 +1345,12 @@ static int lapic_load_regs(struct domain
> if ( hvm_load_entry(LAPIC_REGS, h, s->regs) != 0 )
> return -EINVAL;
>
> + s->loaded.id = vlapic_get_reg(s, APIC_ID);
> + s->loaded.ldr = vlapic_get_reg(s, APIC_LDR);
> + s->loaded.regs = 1;
> + if ( s->loaded.hw )
> + lapic_load_fixup(s);
> +
> if ( hvm_funcs.process_isr )
> hvm_funcs.process_isr(vlapic_find_highest_isr(s), v);
>
> --- 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:
> @@ -70,6 +71,10 @@
> struct vlapic {
> struct hvm_hw_lapic hw;
> struct hvm_hw_lapic_regs *regs;
> + struct {
> + bool_t hw, regs;
> + uint32_t id, ldr;
> + } loaded;
> struct periodic_time pt;
> s_time_t timer_last_update;
> struct page_info *regs_page;
> @@ -124,10 +129,10 @@ int vlapic_apicv_write(struct vcpu *v, u
>
> struct vlapic *vlapic_lowest_prio(
> struct domain *d, struct vlapic *source,
> - int short_hand, uint8_t dest, uint8_t dest_mode);
> + int short_hand, uint32_t dest, uint8_t dest_mode);
>
> bool_t vlapic_match_dest(
> struct vlapic *target, struct vlapic *source,
> - int short_hand, uint8_t dest, uint8_t dest_mode);
> + int short_hand, uint32_t dest, uint8_t dest_mode);
>
> #endif /* __ASM_X86_HVM_VLAPIC_H__ */
>
>
^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: [PATCH v5 3/4] x86/vlapic: a few type adjustments
2014-09-24 15:36 ` [PATCH v5 3/4] x86/vlapic: a few type adjustments Jan Beulich
@ 2014-09-24 16:02 ` Andrew Cooper
0 siblings, 0 replies; 11+ messages in thread
From: Andrew Cooper @ 2014-09-24 16:02 UTC (permalink / raw)
To: Jan Beulich, xen-devel; +Cc: Keir Fraser
On 24/09/14 16:36, Jan Beulich wrote:
> Constify a couple of pointer parameters, convert a boolean function
> return type to bool_t, and clean up a printk() being touched anyway.
>
> Suggested-by: Andrew Cooper <andrew.cooper3@citrix.com>
> Signed-off-by: Jan Beulich <jbeulich@suse.com>
Reviewed-by: Andrew Cooper <andrew.cooper3@citrix.com>
> ---
> v5: Move some more type changes from patch 2 to here.
>
> --- a/xen/arch/x86/hvm/vlapic.c
> +++ b/xen/arch/x86/hvm/vlapic.c
> @@ -173,9 +173,10 @@ uint32_t vlapic_set_ppr(struct vlapic *v
> return ppr;
> }
>
> -static int vlapic_match_logical_addr(struct vlapic *vlapic, uint32_t mda)
> +static bool_t vlapic_match_logical_addr(const struct vlapic *vlapic,
> + uint32_t mda)
> {
> - int result = 0;
> + bool_t result = 0;
> uint32_t logical_id = vlapic_get_reg(vlapic, APIC_LDR);
>
> if ( vlapic_x2apic_mode(vlapic) )
> @@ -196,9 +197,9 @@ static int vlapic_match_logical_addr(str
> result = 1;
> break;
> default:
> - gdprintk(XENLOG_WARNING, "Bad DFR value for lapic of vcpu %d: %08x\n",
> - vlapic_vcpu(vlapic)->vcpu_id,
> - vlapic_get_reg(vlapic, APIC_DFR));
> + printk(XENLOG_G_WARNING "%pv: bad LAPIC DFR value %08x\n",
> + const_vlapic_vcpu(vlapic),
> + vlapic_get_reg(vlapic, APIC_DFR));
> break;
> }
>
> @@ -206,8 +207,8 @@ static int vlapic_match_logical_addr(str
> }
>
> bool_t vlapic_match_dest(
> - struct vlapic *target, struct vlapic *source,
> - int short_hand, uint32_t dest, uint8_t dest_mode)
> + const 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",
> @@ -286,7 +287,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 +353,8 @@ static void vlapic_accept_irq(struct vcp
> }
>
> struct vlapic *vlapic_lowest_prio(
> - struct domain *d, struct vlapic *source,
> - int short_hand, uint32_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;
> @@ -434,7 +435,7 @@ 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);
>
> --- a/xen/include/asm-x86/hvm/vlapic.h
> +++ b/xen/include/asm-x86/hvm/vlapic.h
> @@ -28,6 +28,8 @@
>
> #define vcpu_vlapic(x) (&(x)->arch.hvm_vcpu.vlapic)
> #define vlapic_vcpu(x) (container_of((x), struct vcpu, arch.hvm_vcpu.vlapic))
> +#define const_vlapic_vcpu(x) (container_of((x), const struct vcpu, \
> + arch.hvm_vcpu.vlapic))
> #define vlapic_domain(x) (vlapic_vcpu(x)->domain)
>
> #define _VLAPIC_ID(vlapic, id) (vlapic_x2apic_mode(vlapic) \
> @@ -88,7 +90,8 @@ struct vlapic {
> /* vlapic's frequence is 100 MHz */
> #define APIC_BUS_CYCLE_NS 10
>
> -static inline uint32_t vlapic_get_reg(struct vlapic *vlapic, uint32_t reg)
> +static inline uint32_t vlapic_get_reg(const struct vlapic *vlapic,
> + uint32_t reg)
> {
> return *((uint32_t *)(&vlapic->regs->data[reg]));
> }
> @@ -128,11 +131,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, uint32_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, uint32_t dest, uint8_t dest_mode);
> + const struct vlapic *target, const struct vlapic *source,
> + int short_hand, uint32_t dest, bool_t dest_mode);
>
> #endif /* __ASM_X86_HVM_VLAPIC_H__ */
>
>
^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: [PATCH v5 1/4] x86/HVM: fix miscellaneous aspects of x2APIC emulation
2014-09-24 15:57 ` Andrew Cooper
@ 2014-09-25 8:42 ` Jan Beulich
2014-09-25 11:26 ` Andrew Cooper
0 siblings, 1 reply; 11+ messages in thread
From: Jan Beulich @ 2014-09-25 8:42 UTC (permalink / raw)
To: Andrew Cooper; +Cc: xen-devel, Keir Fraser
>>> On 24.09.14 at 17:57, <andrew.cooper3@citrix.com> wrote:
> On 24/09/14 16:34, Jan Beulich wrote:
>> @@ -877,8 +854,16 @@ static int vlapic_write(struct vcpu *v,
>>
>> int vlapic_apicv_write(struct vcpu *v, unsigned int offset)
>> {
>> - uint32_t val = vlapic_get_reg(vcpu_vlapic(v), offset);
>> - return vlapic_reg_write(v, offset, val);
>> + struct vlapic *vlapic = vcpu_vlapic(v);
>> + uint32_t val = vlapic_get_reg(vlapic, offset);
>> +
>> + if ( !vlapic_x2apic_mode(vlapic) )
>> + return vlapic_reg_write(v, offset, val);
>> +
>> + if ( offset != APIC_SELF_IPI )
>> + return X86EMUL_UNHANDLEABLE;
>> +
>> + return vlapic_reg_write(v, APIC_ICR, APIC_DEST_SELF | (uint8_t)val);
>
> (val & APIC_VECTOR_MASK) instead of a uint8_t cast?
Ah, one more case I failed to convert.
>> @@ -891,16 +876,69 @@ int hvm_x2apic_msr_write(struct vcpu *v,
>>
>> switch ( offset )
>> {
>> - int rc;
>> + case APIC_TASKPRI:
>> + if ( msr_content & ~APIC_TPRI_MASK )
>> + return X86EMUL_UNHANDLEABLE;
>> + break;
>> +
>> + case APIC_SPIV:
>> + if ( msr_content & ~(APIC_VECTOR_MASK | APIC_SPIV_APIC_ENABLED |
>> + (VLAPIC_VERSION & APIC_LVR_DIRECTED_EOI
>
> Need brackets for the logical (VLAPIC_VERSION & APIC_LVR_DIRECTED_EOI)
> test to avoid making a constant expression with the precedence of the
> ternary operator.
Not sure what you mean or what you're thinking here: & has higher
precedence than even &&, not to speak of ?:, so the expression is
quite fine without further parentheses. And yes, the whole ?: is
intended to be a compile time constant, just being prepared for
directed-EOI support to be added (and as this doesn't seem to need
any kind of conditional code, I'm expecting that to get added
directly to the VLAPIC_VERSION #define).
Jan
^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: [PATCH v5 0/4] x86/HVM: fix various aspects of APIC emulation
2014-09-24 15:28 [PATCH v5 0/4] x86/HVM: fix various aspects of APIC emulation Jan Beulich
` (3 preceding siblings ...)
2014-09-24 15:37 ` [PATCH v5 4/4] x86/vlapic: don't silently accept bad vectors Jan Beulich
@ 2014-09-25 11:17 ` Tim Deegan
4 siblings, 0 replies; 11+ messages in thread
From: Tim Deegan @ 2014-09-25 11:17 UTC (permalink / raw)
To: Jan Beulich; +Cc: xen-devel, Keir Fraser, Andrew Cooper
At 16:28 +0100 on 24 Sep (1411572494), Jan Beulich wrote:
> 1: fix miscellaneous aspects of x2APIC emulation
> 2: fix ID handling of x2APIC emulation
> 3: a few type adjustments
> 4: don't silently accept bad vectors
>
> Signed-off-by: Jan Beulich <jbeulich@suse.com>
The load stuff looks much nicer now, thanks.
Acked-by: Tim Deegan <tim@xen.org>
^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: [PATCH v5 1/4] x86/HVM: fix miscellaneous aspects of x2APIC emulation
2014-09-25 8:42 ` Jan Beulich
@ 2014-09-25 11:26 ` Andrew Cooper
0 siblings, 0 replies; 11+ messages in thread
From: Andrew Cooper @ 2014-09-25 11:26 UTC (permalink / raw)
To: Jan Beulich; +Cc: xen-devel, Keir Fraser
On 25/09/14 09:42, Jan Beulich wrote:
>>>> On 24.09.14 at 17:57, <andrew.cooper3@citrix.com> wrote:
>> On 24/09/14 16:34, Jan Beulich wrote:
>>> @@ -877,8 +854,16 @@ static int vlapic_write(struct vcpu *v,
>>>
>>> int vlapic_apicv_write(struct vcpu *v, unsigned int offset)
>>> {
>>> - uint32_t val = vlapic_get_reg(vcpu_vlapic(v), offset);
>>> - return vlapic_reg_write(v, offset, val);
>>> + struct vlapic *vlapic = vcpu_vlapic(v);
>>> + uint32_t val = vlapic_get_reg(vlapic, offset);
>>> +
>>> + if ( !vlapic_x2apic_mode(vlapic) )
>>> + return vlapic_reg_write(v, offset, val);
>>> +
>>> + if ( offset != APIC_SELF_IPI )
>>> + return X86EMUL_UNHANDLEABLE;
>>> +
>>> + return vlapic_reg_write(v, APIC_ICR, APIC_DEST_SELF | (uint8_t)val);
>> (val & APIC_VECTOR_MASK) instead of a uint8_t cast?
> Ah, one more case I failed to convert.
>
>>> @@ -891,16 +876,69 @@ int hvm_x2apic_msr_write(struct vcpu *v,
>>>
>>> switch ( offset )
>>> {
>>> - int rc;
>>> + case APIC_TASKPRI:
>>> + if ( msr_content & ~APIC_TPRI_MASK )
>>> + return X86EMUL_UNHANDLEABLE;
>>> + break;
>>> +
>>> + case APIC_SPIV:
>>> + if ( msr_content & ~(APIC_VECTOR_MASK | APIC_SPIV_APIC_ENABLED |
>>> + (VLAPIC_VERSION & APIC_LVR_DIRECTED_EOI
>> Need brackets for the logical (VLAPIC_VERSION & APIC_LVR_DIRECTED_EOI)
>> test to avoid making a constant expression with the precedence of the
>> ternary operator.
> Not sure what you mean or what you're thinking here: & has higher
> precedence than even &&, not to speak of ?:, so the expression is
> quite fine without further parentheses. And yes, the whole ?: is
> intended to be a compile time constant, just being prepared for
> directed-EOI support to be added (and as this doesn't seem to need
> any kind of conditional code, I'm expecting that to get added
> directly to the VLAPIC_VERSION #define).
>
> Jan
>
Ah - I had logically inverted the condition I thought I had spotted.
Reviewed-by: Andrew Cooper <andrew.cooper3@citrix.com>
^ permalink raw reply [flat|nested] 11+ messages in thread
end of thread, other threads:[~2014-09-25 11:26 UTC | newest]
Thread overview: 11+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2014-09-24 15:28 [PATCH v5 0/4] x86/HVM: fix various aspects of APIC emulation Jan Beulich
2014-09-24 15:34 ` [PATCH v5 1/4] x86/HVM: fix miscellaneous aspects of x2APIC emulation Jan Beulich
2014-09-24 15:57 ` Andrew Cooper
2014-09-25 8:42 ` Jan Beulich
2014-09-25 11:26 ` Andrew Cooper
2014-09-24 15:35 ` [PATCH v5 2/4] x86/HVM: fix ID handling " Jan Beulich
2014-09-24 15:59 ` Andrew Cooper
2014-09-24 15:36 ` [PATCH v5 3/4] x86/vlapic: a few type adjustments Jan Beulich
2014-09-24 16:02 ` Andrew Cooper
2014-09-24 15:37 ` [PATCH v5 4/4] x86/vlapic: don't silently accept bad vectors Jan Beulich
2014-09-25 11:17 ` [PATCH v5 0/4] x86/HVM: fix various aspects of APIC emulation Tim Deegan
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).