|
[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index] Re: [Xen-devel] [PATCH v5 2/4] x86/HVM: fix ID handling of x2APIC emulation
On 24/09/14 16:35, Jan Beulich wrote:
> - properly change ID when switching into x2APIC mode (instead of
> mimicking necessary behavior in hvm_x2apic_msr_read())
> - correctly (meaningfully) set LDR (so far it ended up being 1 on all
> vCPU-s)
> - even if we don't support more than 128 vCPU-s in a HVM guest for now,
> we should properly handle IDs as 32-bit values (i.e. not ignore the
> top 24 bits)
> - with that, properly do cluster ID and bit mask check in
> vlapic_match_logical_addr()
>
> Signed-off-by: Jan Beulich <jbeulich@xxxxxxxx>
Much clearer with some of the changes moved to patch 3.
Reviewed-by: Andrew Cooper <andrew.cooper3@xxxxxxxxxx>
> ---
> 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__ */
>
>
_______________________________________________
Xen-devel mailing list
Xen-devel@xxxxxxxxxxxxx
http://lists.xen.org/xen-devel
|
 |
Lists.xenproject.org is hosted with RackSpace, monitoring our |