* [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 an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.