* [PATCH v4 1/4] x86/HVM: fix miscellaneous aspects of x2APIC emulation
2014-09-18 14:35 [PATCH v4 0/4] x86/HVM: fix various aspects of APIC emulation Jan Beulich
@ 2014-09-18 14:43 ` Jan Beulich
2014-09-22 13:14 ` Andrew Cooper
2014-09-18 14:44 ` [PATCH v4 2/4] x86/HVM: fix ID handling " Jan Beulich
` (2 subsequent siblings)
3 siblings, 1 reply; 15+ messages in thread
From: Jan Beulich @ 2014-09-18 14:43 UTC (permalink / raw)
To: xen-devel; +Cc: Tim Deegan, Keir Fraser
[-- Attachment #1: Type: text/plain, Size: 9068 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
- 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>
---
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
@@ -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,33 @@ int hvm_x2apic_msr_write(struct vcpu *v,
switch ( offset )
{
- int rc;
+ case APIC_TASKPRI:
+ case APIC_EOI:
+ case APIC_SPIV:
+ case APIC_CMCI:
+ case APIC_LVTT ... APIC_LVTERR:
+ case APIC_TMICT:
+ case APIC_TMCCT:
+ case APIC_TDCR:
+ break;
case APIC_ICR:
- rc = vlapic_reg_write(v, APIC_ICR2, (uint32_t)(msr_content >> 32));
- if ( rc )
- return rc;
+ vlapic_set_reg(vlapic, APIC_ICR2, msr_content >> 32);
break;
- case APIC_ICR2:
- return X86EMUL_UNHANDLEABLE;
+ case APIC_SELF_IPI:
+ offset = APIC_ICR;
+ msr_content = APIC_DEST_SELF | (uint8_t)msr_content;
+ break;
+
+ case APIC_ESR:
+ if ( msr_content )
+ {
+ printk(XENLOG_G_WARNING "%pv: non-zero (%lx) LAPIC ESR write\n",
+ v, msr_content);
+ default:
+ return X86EMUL_UNHANDLEABLE;
+ }
}
return vlapic_reg_write(v, offset, (uint32_t)msr_content);
@@ -910,7 +912,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 +924,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 +938,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 +961,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 +1246,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
@@ -3095,8 +3095,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: 9124 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
- 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>
---
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
@@ -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,33 @@ int hvm_x2apic_msr_write(struct vcpu *v,
switch ( offset )
{
- int rc;
+ case APIC_TASKPRI:
+ case APIC_EOI:
+ case APIC_SPIV:
+ case APIC_CMCI:
+ case APIC_LVTT ... APIC_LVTERR:
+ case APIC_TMICT:
+ case APIC_TMCCT:
+ case APIC_TDCR:
+ break;
case APIC_ICR:
- rc = vlapic_reg_write(v, APIC_ICR2, (uint32_t)(msr_content >> 32));
- if ( rc )
- return rc;
+ vlapic_set_reg(vlapic, APIC_ICR2, msr_content >> 32);
break;
- case APIC_ICR2:
- return X86EMUL_UNHANDLEABLE;
+ case APIC_SELF_IPI:
+ offset = APIC_ICR;
+ msr_content = APIC_DEST_SELF | (uint8_t)msr_content;
+ break;
+
+ case APIC_ESR:
+ if ( msr_content )
+ {
+ printk(XENLOG_G_WARNING "%pv: non-zero (%lx) LAPIC ESR write\n",
+ v, msr_content);
+ default:
+ return X86EMUL_UNHANDLEABLE;
+ }
}
return vlapic_reg_write(v, offset, (uint32_t)msr_content);
@@ -910,7 +912,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 +924,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 +938,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 +961,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 +1246,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
@@ -3095,8 +3095,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] 15+ messages in thread* Re: [PATCH v4 1/4] x86/HVM: fix miscellaneous aspects of x2APIC emulation
2014-09-18 14:43 ` [PATCH v4 1/4] x86/HVM: fix miscellaneous aspects of x2APIC emulation Jan Beulich
@ 2014-09-22 13:14 ` Andrew Cooper
2014-09-22 13:40 ` Jan Beulich
0 siblings, 1 reply; 15+ messages in thread
From: Andrew Cooper @ 2014-09-22 13:14 UTC (permalink / raw)
To: Jan Beulich, xen-devel; +Cc: Keir Fraser, Tim Deegan
On 18/09/14 15:43, Jan Beulich wrote:
> int hvm_x2apic_msr_write(struct vcpu *v, unsigned int msr, uint64_t msr_content)
> @@ -891,16 +876,33 @@ int hvm_x2apic_msr_write(struct vcpu *v,
>
> switch ( offset )
> {
> - int rc;
> + case APIC_TASKPRI:
> + case APIC_EOI:
> + case APIC_SPIV:
> + case APIC_CMCI:
> + case APIC_LVTT ... APIC_LVTERR:
> + case APIC_TMICT:
> + case APIC_TMCCT:
> + case APIC_TDCR:
Most (all?) of these MSRs have reserved bits, which should fail with a
#GP(0) for attempts to set. vlapic_reg_read() masks most of the relevant
bits, but doesn't appear to hit a misbehaving VM.
> + break;
>
> case APIC_ICR:
> - rc = vlapic_reg_write(v, APIC_ICR2, (uint32_t)(msr_content >> 32));
> - if ( rc )
> - return rc;
> + vlapic_set_reg(vlapic, APIC_ICR2, msr_content >> 32);
> break;
>
> - case APIC_ICR2:
> - return X86EMUL_UNHANDLEABLE;
> + case APIC_SELF_IPI:
> + offset = APIC_ICR;
> + msr_content = APIC_DEST_SELF | (uint8_t)msr_content;
> + break;
> +
> + case APIC_ESR:
> + if ( msr_content )
> + {
> + printk(XENLOG_G_WARNING "%pv: non-zero (%lx) LAPIC ESR write\n",
> + v, msr_content);
I know this is just moving an existing error message, but is it actually
useful? ESR is no more special than the other registers with some/all
bits reserved.
> + default:
> + return X86EMUL_UNHANDLEABLE;
> + }
> }
>
> return vlapic_reg_write(v, offset, (uint32_t)msr_content);
> @@ -910,7 +912,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);
This check is too restrictive, at least on Intel. From SDM Vol 3 29.4.3.3
"As noted in Section 29.5, execution of WRMSR with ECX = 83FH (self-IPI
MSR) can lead to an APIC-write VM exit
if the “virtual-interrupt delivery” VM-execution control is 1. The exit
qualification for such an APIC-write VM exit is
3F0H."
So we can still end up wandering the vlapic MMIO codepaths even in
x2apic mode.
~Andrew
^ permalink raw reply [flat|nested] 15+ messages in thread* Re: [PATCH v4 1/4] x86/HVM: fix miscellaneous aspects of x2APIC emulation
2014-09-22 13:14 ` Andrew Cooper
@ 2014-09-22 13:40 ` Jan Beulich
2014-09-23 16:56 ` Andrew Cooper
0 siblings, 1 reply; 15+ messages in thread
From: Jan Beulich @ 2014-09-22 13:40 UTC (permalink / raw)
To: Andrew Cooper; +Cc: xen-devel, Keir Fraser, Tim Deegan
>>> On 22.09.14 at 15:14, <andrew.cooper3@citrix.com> wrote:
> On 18/09/14 15:43, Jan Beulich wrote:
>> @@ -891,16 +876,33 @@ int hvm_x2apic_msr_write(struct vcpu *v,
>>
>> switch ( offset )
>> {
>> - int rc;
>> + case APIC_TASKPRI:
>> + case APIC_EOI:
>> + case APIC_SPIV:
>> + case APIC_CMCI:
>> + case APIC_LVTT ... APIC_LVTERR:
>> + case APIC_TMICT:
>> + case APIC_TMCCT:
>> + case APIC_TDCR:
>
> Most (all?) of these MSRs have reserved bits, which should fail with a
> #GP(0) for attempts to set. vlapic_reg_read() masks most of the relevant
> bits, but doesn't appear to hit a misbehaving VM.
Perhaps, but not in this patch.
>> + break;
>>
>> case APIC_ICR:
>> - rc = vlapic_reg_write(v, APIC_ICR2, (uint32_t)(msr_content >> 32));
>> - if ( rc )
>> - return rc;
>> + vlapic_set_reg(vlapic, APIC_ICR2, msr_content >> 32);
>> break;
>>
>> - case APIC_ICR2:
>> - return X86EMUL_UNHANDLEABLE;
>> + case APIC_SELF_IPI:
>> + offset = APIC_ICR;
>> + msr_content = APIC_DEST_SELF | (uint8_t)msr_content;
>> + break;
>> +
>> + case APIC_ESR:
>> + if ( msr_content )
>> + {
>> + printk(XENLOG_G_WARNING "%pv: non-zero (%lx) LAPIC ESR write\n",
>> + v, msr_content);
>
> I know this is just moving an existing error message, but is it actually
> useful? ESR is no more special than the other registers with some/all
> bits reserved.
It is different in that legacy APIC mode allows these to be written.
But yes, I wondered about the usefulness of this message too.
>> @@ -910,7 +912,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);
>
> This check is too restrictive, at least on Intel. From SDM Vol 3 29.4.3.3
>
> "As noted in Section 29.5, execution of WRMSR with ECX = 83FH (self-IPI
> MSR) can lead to an APIC-write VM exit
> if the “virtual-interrupt delivery” VM-execution control is 1. The exit
> qualification for such an APIC-write VM exit is
> 3F0H."
>
> So we can still end up wandering the vlapic MMIO codepaths even in
> x2apic mode.
Not exactly: These accesses would arrive at vlapic_apicv_write()
(where APIC_SELF_IPI is being handled even for the x2APIC case)
and hence won't go the normal MMIO path (including vlapic_range()).
Jan
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
http://lists.xen.org/xen-devel
^ permalink raw reply [flat|nested] 15+ messages in thread* Re: [PATCH v4 1/4] x86/HVM: fix miscellaneous aspects of x2APIC emulation
2014-09-22 13:40 ` Jan Beulich
@ 2014-09-23 16:56 ` Andrew Cooper
2014-09-24 8:02 ` Jan Beulich
0 siblings, 1 reply; 15+ messages in thread
From: Andrew Cooper @ 2014-09-23 16:56 UTC (permalink / raw)
To: Jan Beulich; +Cc: xen-devel, Keir Fraser, Tim Deegan
On 22/09/14 14:40, Jan Beulich wrote:
>>>> On 22.09.14 at 15:14, <andrew.cooper3@citrix.com> wrote:
>> On 18/09/14 15:43, Jan Beulich wrote:
>>> @@ -891,16 +876,33 @@ int hvm_x2apic_msr_write(struct vcpu *v,
>>>
>>> switch ( offset )
>>> {
>>> - int rc;
>>> + case APIC_TASKPRI:
>>> + case APIC_EOI:
>>> + case APIC_SPIV:
>>> + case APIC_CMCI:
>>> + case APIC_LVTT ... APIC_LVTERR:
>>> + case APIC_TMICT:
>>> + case APIC_TMCCT:
>>> + case APIC_TDCR:
>> Most (all?) of these MSRs have reserved bits, which should fail with a
>> #GP(0) for attempts to set. vlapic_reg_read() masks most of the relevant
>> bits, but doesn't appear to hit a misbehaving VM.
> Perhaps, but not in this patch.
Does it not count towards fixing misc aspects of x2APIC emulation?
>
>>> + break;
>>>
>>> case APIC_ICR:
>>> - rc = vlapic_reg_write(v, APIC_ICR2, (uint32_t)(msr_content >> 32));
>>> - if ( rc )
>>> - return rc;
>>> + vlapic_set_reg(vlapic, APIC_ICR2, msr_content >> 32);
>>> break;
>>>
>>> - case APIC_ICR2:
>>> - return X86EMUL_UNHANDLEABLE;
>>> + case APIC_SELF_IPI:
>>> + offset = APIC_ICR;
>>> + msr_content = APIC_DEST_SELF | (uint8_t)msr_content;
>>> + break;
>>> +
>>> + case APIC_ESR:
>>> + if ( msr_content )
>>> + {
>>> + printk(XENLOG_G_WARNING "%pv: non-zero (%lx) LAPIC ESR write\n",
>>> + v, msr_content);
>> I know this is just moving an existing error message, but is it actually
>> useful? ESR is no more special than the other registers with some/all
>> bits reserved.
> It is different in that legacy APIC mode allows these to be written.
> But yes, I wondered about the usefulness of this message too.
Even for legacy APIC, the value written is discarded by hardware. I
would just drop the message.
>
>>> @@ -910,7 +912,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);
>> This check is too restrictive, at least on Intel. From SDM Vol 3 29.4.3.3
>>
>> "As noted in Section 29.5, execution of WRMSR with ECX = 83FH (self-IPI
>> MSR) can lead to an APIC-write VM exit
>> if the “virtual-interrupt delivery” VM-execution control is 1. The exit
>> qualification for such an APIC-write VM exit is
>> 3F0H."
>>
>> So we can still end up wandering the vlapic MMIO codepaths even in
>> x2apic mode.
> Not exactly: These accesses would arrive at vlapic_apicv_write()
> (where APIC_SELF_IPI is being handled even for the x2APIC case)
> and hence won't go the normal MMIO path (including vlapic_range()).
Ah yes, which bypasses the vlapic_range() test.
In which case, this change appears ok.
~Andrew
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
http://lists.xen.org/xen-devel
^ permalink raw reply [flat|nested] 15+ messages in thread* Re: [PATCH v4 1/4] x86/HVM: fix miscellaneous aspects of x2APIC emulation
2014-09-23 16:56 ` Andrew Cooper
@ 2014-09-24 8:02 ` Jan Beulich
0 siblings, 0 replies; 15+ messages in thread
From: Jan Beulich @ 2014-09-24 8:02 UTC (permalink / raw)
To: Andrew Cooper; +Cc: xen-devel, Keir Fraser, Tim Deegan
>>> On 23.09.14 at 18:56, <andrew.cooper3@citrix.com> wrote:
> On 22/09/14 14:40, Jan Beulich wrote:
>>>>> On 22.09.14 at 15:14, <andrew.cooper3@citrix.com> wrote:
>>> On 18/09/14 15:43, Jan Beulich wrote:
>>>> @@ -891,16 +876,33 @@ int hvm_x2apic_msr_write(struct vcpu *v,
>>>>
>>>> switch ( offset )
>>>> {
>>>> - int rc;
>>>> + case APIC_TASKPRI:
>>>> + case APIC_EOI:
>>>> + case APIC_SPIV:
>>>> + case APIC_CMCI:
>>>> + case APIC_LVTT ... APIC_LVTERR:
>>>> + case APIC_TMICT:
>>>> + case APIC_TMCCT:
>>>> + case APIC_TDCR:
>>> Most (all?) of these MSRs have reserved bits, which should fail with a
>>> #GP(0) for attempts to set. vlapic_reg_read() masks most of the relevant
>>> bits, but doesn't appear to hit a misbehaving VM.
>> Perhaps, but not in this patch.
>
> Does it not count towards fixing misc aspects of x2APIC emulation?
Sure it does - I guess I should go and see by how much it grows
the patch (I really don't want it to double its size because of this).
Jan
^ permalink raw reply [flat|nested] 15+ messages in thread
* [PATCH v4 2/4] x86/HVM: fix ID handling of x2APIC emulation
2014-09-18 14:35 [PATCH v4 0/4] x86/HVM: fix various aspects of APIC emulation Jan Beulich
2014-09-18 14:43 ` [PATCH v4 1/4] x86/HVM: fix miscellaneous aspects of x2APIC emulation Jan Beulich
@ 2014-09-18 14:44 ` Jan Beulich
2014-09-19 6:09 ` Jan Beulich
2014-09-22 14:30 ` Andrew Cooper
2014-09-18 14:44 ` [PATCH v4 3/4] x86/HVM: a few type adjustments Jan Beulich
2014-09-18 14:45 ` [PATCH v4 4/4] x86/vlapic: don't silently accept bad vectors Jan Beulich
3 siblings, 2 replies; 15+ messages in thread
From: Jan Beulich @ 2014-09-18 14:44 UTC (permalink / raw)
To: xen-devel; +Cc: Tim Deegan, Keir Fraser
[-- Attachment #1: Type: text/plain, Size: 8896 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()
- slightly adjust other parameter types of vlapic_match_dest() and
vlapic_lowest_prio() (and related local variable ones)
Signed-off-by: Jan Beulich <jbeulich@suse.com>
---
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) )
{
@@ -207,8 +206,8 @@ 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)
+ 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",
@@ -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);
@@ -286,7 +286,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 +352,8 @@ 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)
+ 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,13 +434,11 @@ 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);
- 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:
@@ -924,6 +920,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 )
@@ -949,13 +954,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));
@@ -1227,6 +1229,22 @@ 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 &&
+ /* Further checks are optional: ID != 0 contradicts LDR == 1. */
+ GET_xAPIC_ID(id) == vlapic_vcpu(vlapic)->vcpu_id * 2 &&
+ id == SET_xAPIC_ID(GET_xAPIC_ID(id)) )
+ set_x2apic_id(vlapic);
+}
+
static int lapic_load_hidden(struct domain *d, hvm_domain_context_t *h)
{
uint16_t vcpuid;
@@ -1246,6 +1264,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;
@@ -1274,6 +1296,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;
@@ -123,11 +128,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, uint8_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, uint8_t dest, uint8_t dest_mode);
+ 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-id.patch --]
[-- Type: text/plain, Size: 8940 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()
- slightly adjust other parameter types of vlapic_match_dest() and
vlapic_lowest_prio() (and related local variable ones)
Signed-off-by: Jan Beulich <jbeulich@suse.com>
---
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) )
{
@@ -207,8 +206,8 @@ 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)
+ 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",
@@ -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);
@@ -286,7 +286,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 +352,8 @@ 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)
+ 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,13 +434,11 @@ 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);
- 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:
@@ -924,6 +920,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 )
@@ -949,13 +954,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));
@@ -1227,6 +1229,22 @@ 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 &&
+ /* Further checks are optional: ID != 0 contradicts LDR == 1. */
+ GET_xAPIC_ID(id) == vlapic_vcpu(vlapic)->vcpu_id * 2 &&
+ id == SET_xAPIC_ID(GET_xAPIC_ID(id)) )
+ set_x2apic_id(vlapic);
+}
+
static int lapic_load_hidden(struct domain *d, hvm_domain_context_t *h)
{
uint16_t vcpuid;
@@ -1246,6 +1264,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;
@@ -1274,6 +1296,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;
@@ -123,11 +128,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, uint8_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, uint8_t dest, uint8_t dest_mode);
+ 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] 15+ messages in thread* Re: [PATCH v4 2/4] x86/HVM: fix ID handling of x2APIC emulation
2014-09-18 14:44 ` [PATCH v4 2/4] x86/HVM: fix ID handling " Jan Beulich
@ 2014-09-19 6:09 ` Jan Beulich
2014-09-22 14:30 ` Andrew Cooper
1 sibling, 0 replies; 15+ messages in thread
From: Jan Beulich @ 2014-09-19 6:09 UTC (permalink / raw)
To: xen-devel; +Cc: Tim Deegan, Keir Fraser
>>> On 18.09.14 at 16:44, <JBeulich@suse.com> wrote:
> @@ -1227,6 +1229,22 @@ 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 &&
> + /* Further checks are optional: ID != 0 contradicts LDR == 1. */
> + GET_xAPIC_ID(id) == vlapic_vcpu(vlapic)->vcpu_id * 2 &&
> + id == SET_xAPIC_ID(GET_xAPIC_ID(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);
+ }
> +}
Jan
^ permalink raw reply [flat|nested] 15+ messages in thread* Re: [PATCH v4 2/4] x86/HVM: fix ID handling of x2APIC emulation
2014-09-18 14:44 ` [PATCH v4 2/4] x86/HVM: fix ID handling " Jan Beulich
2014-09-19 6:09 ` Jan Beulich
@ 2014-09-22 14:30 ` Andrew Cooper
2014-09-22 15:19 ` Jan Beulich
1 sibling, 1 reply; 15+ messages in thread
From: Andrew Cooper @ 2014-09-22 14:30 UTC (permalink / raw)
To: Jan Beulich, xen-devel; +Cc: Keir Fraser, Tim Deegan
[-- Attachment #1.1: Type: text/plain, Size: 10017 bytes --]
On 18/09/14 15:44, 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()
> - slightly adjust other parameter types of vlapic_match_dest() and
> vlapic_lowest_prio() (and related local variable ones)
>
> Signed-off-by: Jan Beulich <jbeulich@suse.com>
> ---
> 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) )
> {
> @@ -207,8 +206,8 @@ 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)
> + struct vlapic *target, const struct vlapic *source,
This constification of source could move to patch 3 (if there are
sufficient other changes to warrent a respin of the series). Here and
elsewhere.
> + 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",
> @@ -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);
> @@ -286,7 +286,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);
This could also move to patch 3.
> struct vcpu *v;
>
> if ( icr == 0 )
> @@ -352,8 +352,8 @@ 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)
> + 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,13 +434,11 @@ 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);
>
> - 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));
This is slightly expensive to do for each register write. Why does this
clobbering need to be here? Could it not be....
> +
> switch ( offset )
> {
> case APIC_ID:
> @@ -924,6 +920,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 )
> @@ -949,13 +954,10 @@ bool_t vlapic_msr_set(struct vlapic *vla
> return 0;
>
> vlapic->hw.apic_base_msr = value;
> + memset(&vlapic->loaded, 0, sizeof(vlapic->loaded));
(Similarly here...)
>
> 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));
>
> @@ -1227,6 +1229,22 @@ 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 &&
> + /* Further checks are optional: ID != 0 contradicts LDR == 1. */
> + GET_xAPIC_ID(id) == vlapic_vcpu(vlapic)->vcpu_id * 2 &&
> + id == SET_xAPIC_ID(GET_xAPIC_ID(id)) )
> + set_x2apic_id(vlapic);
... in here, where we have positively identified whether fixup is needed
or not.
~Andrew
> +}
> +
> static int lapic_load_hidden(struct domain *d, hvm_domain_context_t *h)
> {
> uint16_t vcpuid;
> @@ -1246,6 +1264,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;
> @@ -1274,6 +1296,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;
> @@ -123,11 +128,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, uint8_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, uint8_t dest, uint8_t dest_mode);
> + struct vlapic *target, const struct vlapic *source,
> + int short_hand, uint32_t dest, bool_t dest_mode);
>
> #endif /* __ASM_X86_HVM_VLAPIC_H__ */
>
>
>
>
> _______________________________________________
> Xen-devel mailing list
> Xen-devel@lists.xen.org
> http://lists.xen.org/xen-devel
[-- Attachment #1.2: Type: text/html, Size: 11010 bytes --]
[-- Attachment #2: 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] 15+ messages in thread* Re: [PATCH v4 2/4] x86/HVM: fix ID handling of x2APIC emulation
2014-09-22 14:30 ` Andrew Cooper
@ 2014-09-22 15:19 ` Jan Beulich
2014-09-24 10:42 ` Andrew Cooper
0 siblings, 1 reply; 15+ messages in thread
From: Jan Beulich @ 2014-09-22 15:19 UTC (permalink / raw)
To: Andrew Cooper; +Cc: xen-devel, Keir Fraser, Tim Deegan
>>> On 22.09.14 at 16:30, <andrew.cooper3@citrix.com> wrote:
> On 18/09/14 15:44, Jan Beulich wrote:
>> @@ -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));
>
> This is slightly expensive to do for each register write. Why does this
> clobbering need to be here? Could it not be....
No, clearing this state can't be done in the load functions themselves
(or else we wouldn't need the state in the first place. This, however,
is more obvious with the follow-up addition I mailed in reply to the
patch here, adding an "else" path to lapic_load_fixup(). That code
wouldn't work as intended if we cleared the state early.
If writing 12 bytes to memory would really turn out expensive, we
may end up having to limit the clearing to certain register writes,
but I was really instead considering to also do the clearing on
register reads.
>> +static void lapic_load_fixup(struct vlapic *vlapic)
>> +{
>> + uint32_t id = vlapic->loaded.id;
>> +
>> + if ( vlapic_x2apic_mode(vlapic) &&
>> + id && vlapic->loaded.ldr == 1 &&
>> + /* Further checks are optional: ID != 0 contradicts LDR == 1. */
>> + GET_xAPIC_ID(id) == vlapic_vcpu(vlapic)->vcpu_id * 2 &&
>> + id == SET_xAPIC_ID(GET_xAPIC_ID(id)) )
>> + set_x2apic_id(vlapic);
>
> ... in here, where we have positively identified whether fixup is needed
> or not.
For full context, here's the full intended function again:
+static void lapic_load_fixup(struct vlapic *vlapic)
+{
+ uint32_t id = vlapic->loaded.id;
+
+ if ( vlapic_x2apic_mode(vlapic) && id && vlapic->loaded.ldr == 1 &&
+ /* Further checks are optional: ID != 0 contradicts LDR == 1. */
+ GET_xAPIC_ID(id) == vlapic_vcpu(vlapic)->vcpu_id * 2 &&
+ id == SET_xAPIC_ID(GET_xAPIC_ID(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);
+ }
+}
Jan
^ permalink raw reply [flat|nested] 15+ messages in thread* Re: [PATCH v4 2/4] x86/HVM: fix ID handling of x2APIC emulation
2014-09-22 15:19 ` Jan Beulich
@ 2014-09-24 10:42 ` Andrew Cooper
2014-09-24 11:41 ` Jan Beulich
0 siblings, 1 reply; 15+ messages in thread
From: Andrew Cooper @ 2014-09-24 10:42 UTC (permalink / raw)
To: Jan Beulich; +Cc: xen-devel, Keir Fraser, Tim Deegan
On 22/09/14 16:19, Jan Beulich wrote:
>>>> On 22.09.14 at 16:30, <andrew.cooper3@citrix.com> wrote:
>> On 18/09/14 15:44, Jan Beulich wrote:
>>> @@ -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));
>> This is slightly expensive to do for each register write. Why does this
>> clobbering need to be here? Could it not be....
> No, clearing this state can't be done in the load functions themselves
> (or else we wouldn't need the state in the first place. This, however,
> is more obvious with the follow-up addition I mailed in reply to the
> patch here, adding an "else" path to lapic_load_fixup(). That code
> wouldn't work as intended if we cleared the state early.
>
> If writing 12 bytes to memory would really turn out expensive, we
> may end up having to limit the clearing to certain register writes,
> but I was really instead considering to also do the clearing on
> register reads.
>
>>> +static void lapic_load_fixup(struct vlapic *vlapic)
>>> +{
>>> + uint32_t id = vlapic->loaded.id;
>>> +
>>> + if ( vlapic_x2apic_mode(vlapic) &&
>>> + id && vlapic->loaded.ldr == 1 &&
>>> + /* Further checks are optional: ID != 0 contradicts LDR == 1. */
>>> + GET_xAPIC_ID(id) == vlapic_vcpu(vlapic)->vcpu_id * 2 &&
>>> + id == SET_xAPIC_ID(GET_xAPIC_ID(id)) )
>>> + set_x2apic_id(vlapic);
>> ... in here, where we have positively identified whether fixup is needed
>> or not.
> For full context, here's the full intended function again:
>
> +static void lapic_load_fixup(struct vlapic *vlapic)
> +{
> + uint32_t id = vlapic->loaded.id;
> +
> + if ( vlapic_x2apic_mode(vlapic) && id && vlapic->loaded.ldr == 1 &&
> + /* Further checks are optional: ID != 0 contradicts LDR == 1. */
> + GET_xAPIC_ID(id) == vlapic_vcpu(vlapic)->vcpu_id * 2 &&
> + id == SET_xAPIC_ID(GET_xAPIC_ID(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);
> + }
> +}
How about dropping the optional checks, as "id && vlapic->loaded.ldr ==
1" covers the broken hypervisor case?
The "id = vcpu_id * 2" is a broken assumption which I do need to fix as
part of the cpuid infrastructure improvements, which would then break
this check for a broken Xen.
Furthermore, vlapic_x2apic_mode(vlapic) contradicts the use of
{GET,SET}_xAPIC_ID().
~Andrew
^ permalink raw reply [flat|nested] 15+ messages in thread* Re: [PATCH v4 2/4] x86/HVM: fix ID handling of x2APIC emulation
2014-09-24 10:42 ` Andrew Cooper
@ 2014-09-24 11:41 ` Jan Beulich
2014-09-24 12:10 ` Andrew Cooper
0 siblings, 1 reply; 15+ messages in thread
From: Jan Beulich @ 2014-09-24 11:41 UTC (permalink / raw)
To: Andrew Cooper; +Cc: xen-devel, Keir Fraser, Tim Deegan
>>> On 24.09.14 at 12:42, <andrew.cooper3@citrix.com> wrote:
> On 22/09/14 16:19, Jan Beulich wrote:
>> For full context, here's the full intended function again:
>>
>> +static void lapic_load_fixup(struct vlapic *vlapic)
>> +{
>> + uint32_t id = vlapic->loaded.id;
>> +
>> + if ( vlapic_x2apic_mode(vlapic) && id && vlapic->loaded.ldr == 1 &&
>> + /* Further checks are optional: ID != 0 contradicts LDR == 1. */
>> + GET_xAPIC_ID(id) == vlapic_vcpu(vlapic)->vcpu_id * 2 &&
>> + id == SET_xAPIC_ID(GET_xAPIC_ID(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);
>> + }
>> +}
>
> How about dropping the optional checks, as "id && vlapic->loaded.ldr ==
> 1" covers the broken hypervisor case?
I'd like to keep them for a while - after all that's why I added the
comment saying they're optional. The moment they start conflicting
with something else, they could be dropped.
The alternative would be to make them WARN_ON()s inside the if().
> The "id = vcpu_id * 2" is a broken assumption which I do need to fix as
> part of the cpuid infrastructure improvements, which would then break
> this check for a broken Xen.
For one it's not a broken assumption imo: The APIC ID gets set up
this way. And then I don't see why altering the APIC ID setting
would break this check here: If altering how the ID gets established
would get backported, I'd surely expect the change to the ID
handling here to also be.
> Furthermore, vlapic_x2apic_mode(vlapic) contradicts the use of
> {GET,SET}_xAPIC_ID().
So it does, but intentionally. Remember - we're checking whether some
fixup to what came in is necessary, and part of the brokenness was
that the ID was left set in a legacy APIC manner.
Jan
^ permalink raw reply [flat|nested] 15+ messages in thread* Re: [PATCH v4 2/4] x86/HVM: fix ID handling of x2APIC emulation
2014-09-24 11:41 ` Jan Beulich
@ 2014-09-24 12:10 ` Andrew Cooper
0 siblings, 0 replies; 15+ messages in thread
From: Andrew Cooper @ 2014-09-24 12:10 UTC (permalink / raw)
To: Jan Beulich; +Cc: xen-devel, Keir Fraser, Tim Deegan
On 24/09/14 12:41, Jan Beulich wrote:
>>>> On 24.09.14 at 12:42, <andrew.cooper3@citrix.com> wrote:
>> On 22/09/14 16:19, Jan Beulich wrote:
>>> For full context, here's the full intended function again:
>>>
>>> +static void lapic_load_fixup(struct vlapic *vlapic)
>>> +{
>>> + uint32_t id = vlapic->loaded.id;
>>> +
>>> + if ( vlapic_x2apic_mode(vlapic) && id && vlapic->loaded.ldr == 1 &&
>>> + /* Further checks are optional: ID != 0 contradicts LDR == 1. */
>>> + GET_xAPIC_ID(id) == vlapic_vcpu(vlapic)->vcpu_id * 2 &&
>>> + id == SET_xAPIC_ID(GET_xAPIC_ID(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);
>>> + }
>>> +}
>> How about dropping the optional checks, as "id && vlapic->loaded.ldr ==
>> 1" covers the broken hypervisor case?
> I'd like to keep them for a while - after all that's why I added the
> comment saying they're optional. The moment they start conflicting
> with something else, they could be dropped.
>
> The alternative would be to make them WARN_ON()s inside the if().
Making them WARN_ON()s would make it more obvious if underlying
assumption/implementations change which subsequently invalidate the checks.
>
>> The "id = vcpu_id * 2" is a broken assumption which I do need to fix as
>> part of the cpuid infrastructure improvements, which would then break
>> this check for a broken Xen.
> For one it's not a broken assumption imo: The APIC ID gets set up
> this way.
Sorry - it is one which is expected to change in future development work.
> And then I don't see why altering the APIC ID setting
> would break this check here: If altering how the ID gets established
> would get backported, I'd surely expect the change to the ID
> handling here to also be.
This check gets applied equally to migrations from the same version of
Xen as to those from older versions. In this case I suppose the "id &&
vlapic->loaded.ldr" will short circuit the vcpu_id*2 check, in makes it
ok (assuming no backports).
>
>> Furthermore, vlapic_x2apic_mode(vlapic) contradicts the use of
>> {GET,SET}_xAPIC_ID().
> So it does, but intentionally. Remember - we're checking whether some
> fixup to what came in is necessary, and part of the brokenness was
> that the ID was left set in a legacy APIC manner.
Right, which again due to short circuiting ok given that we have never
supported x2apic with ids greater than 255.
~Andrew
^ permalink raw reply [flat|nested] 15+ messages in thread
* [PATCH v4 3/4] x86/HVM: a few type adjustments
2014-09-18 14:35 [PATCH v4 0/4] x86/HVM: fix various aspects of APIC emulation Jan Beulich
2014-09-18 14:43 ` [PATCH v4 1/4] x86/HVM: fix miscellaneous aspects of x2APIC emulation Jan Beulich
2014-09-18 14:44 ` [PATCH v4 2/4] x86/HVM: fix ID handling " Jan Beulich
@ 2014-09-18 14:44 ` Jan Beulich
2014-09-18 14:45 ` [PATCH v4 4/4] x86/vlapic: don't silently accept bad vectors Jan Beulich
3 siblings, 0 replies; 15+ messages in thread
From: Jan Beulich @ 2014-09-18 14:44 UTC (permalink / raw)
To: xen-devel; +Cc: Tim Deegan, Keir Fraser
[-- Attachment #1: Type: text/plain, Size: 2858 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>
Reviewed-by: Andrew Cooper <andrew.cooper3@citrix.com>
--- a/xen/arch/x86/hvm/vlapic.c
+++ b/xen/arch/x86/hvm/vlapic.c
@@ -173,9 +173,9 @@ 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 +196,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,7 +206,7 @@ static int vlapic_match_logical_addr(str
}
bool_t vlapic_match_dest(
- struct vlapic *target, const struct vlapic *source,
+ 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, "
--- 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]));
}
@@ -132,7 +135,7 @@ struct vlapic *vlapic_lowest_prio(
int short_hand, uint32_t dest, bool_t dest_mode);
bool_t vlapic_match_dest(
- struct vlapic *target, const struct vlapic *source,
+ 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: 2890 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>
Reviewed-by: Andrew Cooper <andrew.cooper3@citrix.com>
--- a/xen/arch/x86/hvm/vlapic.c
+++ b/xen/arch/x86/hvm/vlapic.c
@@ -173,9 +173,9 @@ 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 +196,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,7 +206,7 @@ static int vlapic_match_logical_addr(str
}
bool_t vlapic_match_dest(
- struct vlapic *target, const struct vlapic *source,
+ 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, "
--- 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]));
}
@@ -132,7 +135,7 @@ struct vlapic *vlapic_lowest_prio(
int short_hand, uint32_t dest, bool_t dest_mode);
bool_t vlapic_match_dest(
- struct vlapic *target, const struct vlapic *source,
+ 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] 15+ messages in thread* [PATCH v4 4/4] x86/vlapic: don't silently accept bad vectors
2014-09-18 14:35 [PATCH v4 0/4] x86/HVM: fix various aspects of APIC emulation Jan Beulich
` (2 preceding siblings ...)
2014-09-18 14:44 ` [PATCH v4 3/4] x86/HVM: a few type adjustments Jan Beulich
@ 2014-09-18 14:45 ` Jan Beulich
3 siblings, 0 replies; 15+ messages in thread
From: Jan Beulich @ 2014-09-18 14:45 UTC (permalink / raw)
To: xen-devel; +Cc: Tim Deegan, 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]);
@@ -459,7 +483,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;
}
@@ -467,6 +496,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 )
@@ -1353,6 +1387,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]);
@@ -459,7 +483,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;
}
@@ -467,6 +496,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 )
@@ -1353,6 +1387,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] 15+ messages in thread