x86/HVM: fix various aspects 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) - 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 --- 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. --- 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__ */