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 --- 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__ */