|
On 10/09/14 14: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)
I think the addition of arch_domain_unpause() at least needs
mentioning in the commit message, although...
Signed-off-by: Jan Beulich <jbeulich@xxxxxxxx>
---
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)
target should be const as well, and looks as if it can be by pushing
const-ness down into vlapic_match_logical_addr() and
vlapic_get_reg().
{
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 )
{
@@ -593,10 +591,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;
@@ -891,6 +885,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);
I know this mimics the existing behaviour, but I should point out
that it is an Intel-ism (which assumes no hyperthreads) which is
wrong on AMD systems, and confuses algorithms which following the
BIOS/Systems guides. I do plan to fix it as part of my
cpuid/feature levelling fixes.
+ 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 )
@@ -918,11 +921,7 @@ bool_t vlapic_msr_set(struct vlapic *vla
vlapic->hw.apic_base_msr = value;
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));
@@ -1209,6 +1208,7 @@ static int lapic_load_hidden(struct doma
return -EINVAL;
}
s = vcpu_vlapic(v);
+ s->loaded = 1;
if ( hvm_load_entry_zeroextend(LAPIC, h, &s->hw) != 0 )
return -EINVAL;
@@ -1237,6 +1237,7 @@ static int lapic_load_regs(struct domain
return -EINVAL;
}
s = vcpu_vlapic(v);
+ s->loaded = 1;
if ( hvm_load_entry(LAPIC_REGS, h, s->regs) != 0 )
return -EINVAL;
@@ -1254,6 +1255,29 @@ HVM_REGISTER_SAVE_RESTORE(LAPIC, lapic_s
HVM_REGISTER_SAVE_RESTORE(LAPIC_REGS, lapic_save_regs, lapic_load_regs,
1, HVMSR_PER_VCPU);
+void vlapic_domain_unpause(const struct domain *d)
+{
+ /*
+ * Following lapic_load_hidden()/lapic_load_regs() we may need to
+ * correct ID and LDR when they come from an old, broken hypervisor.
+ */
This seems like aweful overhead for the domain_{,un}pause() path.
Why can't it be fixed up once in lapic_load_{regs,hidden}(), or
possibly deferred to the end of hvm_load()?
+ struct vcpu *v;
+
+ for_each_vcpu ( d, v )
+ {
+ struct vlapic *vlapic = vcpu_vlapic(v);
+ u32 id = vlapic_get_reg(vlapic, APIC_ID);
+
+ if ( vlapic->loaded && vlapic_x2apic_mode(vlapic) &&
+ id && GET_xAPIC_ID(id) == v->vcpu_id * 2 &&
+ id == SET_xAPIC_ID(GET_xAPIC_ID(id)) &&
+ vlapic_get_reg(vlapic, APIC_LDR) == 1 )
+ set_x2apic_id(vlapic);
+
+ vlapic->loaded = 0;
+ }
+}
+
int vlapic_init(struct vcpu *v)
{
struct vlapic *vlapic = vcpu_vlapic(v);
--- a/xen/common/domain.c
+++ b/xen/common/domain.c
@@ -952,8 +952,11 @@ void domain_unpause(struct domain *d)
struct vcpu *v;
if ( atomic_dec_and_test(&d->pause_count) )
+ {
+ arch_domain_unpause(d);
for_each_vcpu( d, v )
vcpu_wake(v);
+ }
}
int __domain_pause_by_systemcontroller(struct domain *d,
--- a/xen/include/asm-arm/domain.h
+++ b/xen/include/asm-arm/domain.h
@@ -241,6 +241,8 @@ struct arch_vcpu
void vcpu_show_execution_state(struct vcpu *);
void vcpu_show_registers(const struct vcpu *);
+#define arch_domain_unpause(d) ((void)(d))
+
#endif /* __ASM_DOMAIN_H__ */
/*
--- a/xen/include/asm-x86/domain.h
+++ b/xen/include/asm-x86/domain.h
@@ -500,6 +500,12 @@ void domain_cpuid(struct domain *d,
unsigned int *ecx,
unsigned int *edx);
+#define arch_domain_unpause(d) ({ \
+ const struct domain *d_ = (d); \
+ if ( is_hvm_domain(d_) ) \
+ vlapic_domain_unpause(d_); \
+})
+
#endif /* __ASM_DOMAIN_H__ */
/*
--- 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))
Some comment regarding the difference between these two?
~Andrew
/*
* APIC can be disabled in two ways:
@@ -70,6 +71,7 @@
struct vlapic {
struct hvm_hw_lapic hw;
struct hvm_hw_lapic_regs *regs;
+ bool_t loaded;
struct periodic_time pt;
s_time_t timer_last_update;
struct page_info *regs_page;
@@ -123,11 +125,13 @@ 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);
+
+void vlapic_domain_unpause(const struct domain *);
#endif /* __ASM_X86_HVM_VLAPIC_H__ */
_______________________________________________
Xen-devel mailing list
Xen-devel@xxxxxxxxxxxxx
http://lists.xen.org/xen-devel
|
