|
On 09/09/14 16:51, 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)
- fail reads from the self-IPI register (which is write-only)
- handle self-IPI writes and the ICR2 half of ICR writes largely in
hvm_x2apic_msr_write()
- 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@xxxxxxxx>
---
RFC reason: The previously broken APIC ID handling would seem to cause
problems on migration; so far I didn't have a good idea how to
transparently and correctly convert the bogus old ID (and derived LDR)
to the correct one. Since for a single vCPU guest this doesn't matter
(the only vCPU has ID zero), perhaps checking for LDR being wrongly 1
on all vCPU-s might be a usable approach. I further hope that dealing
with migration _to_ an old, broken hypervisor isn't going to be
necessary.
Hmm - This could explain our apicv bugs with migration. The
symptoms were that migrating a VM between two identical pieces of
hardware, one with apicv enabled and one without resulted in a loss
of interrupts being delivered to the VM. The VMs in question were
using x2apic, and this mixup with APIC IDs sounds very plausible.
Once I have got libxl migrationv2 posted, I will reinvestigate the
apicv issue with this patch applied.
Having said all of that, I too am very concerned about the migration
implication. This is certainly something which we absolutely don't
want to break.
In the past, it has been suggested that doing compatibility fixing
like this in the toolstack is far better than in Xen. A toolstack
is in a better position to know that a migration is occurring from
an older version of xen to a newer version, and across this
difference, LDR needs fixing up in the lapic state. Furthermore, an
ability like this can be used in reverse to safely migrate
backwards, a requested feature of XenServer incase a partial upgrade
needs to be stopped or undone.
Is it possible to split this patch between the safe cleanup and the
unsafe cleanup?
~Andrew
--- 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;
@@ -414,13 +414,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 )
{
@@ -611,15 +609,12 @@ 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;
case APIC_ICR2:
+ case APIC_SELF_IPI:
return 1;
}
@@ -710,9 +705,7 @@ static int vlapic_reg_write(struct vcpu
break;
case APIC_SELF_IPI:
- rc = vlapic_x2apic_mode(vlapic)
- ? vlapic_reg_write(v, APIC_ICR, 0x40000 | (val & 0xff))
- : X86EMUL_UNHANDLEABLE;
+ rc = X86EMUL_UNHANDLEABLE;
break;
case APIC_ICR:
@@ -722,9 +715,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 */
@@ -883,16 +874,17 @@ int hvm_x2apic_msr_write(struct vcpu *v,
switch ( offset )
{
- int rc;
-
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;
}
return vlapic_reg_write(v, offset, (uint32_t)msr_content);
@@ -932,8 +924,10 @@ void vlapic_msr_set(struct vlapic *vlapi
if ( vlapic_x2apic_mode(vlapic) )
{
- u32 id = vlapic_get_reg(vlapic, APIC_ID);
- u32 ldr = ((id & ~0xf) << 16) | (1 << (id & 0xf));
+ 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);
}
--- 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:
@@ -123,11 +124,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@xxxxxxxxxxxxx
http://lists.xen.org/xen-devel
|
