| Message ID | 1246191331-31085-2-git-send-email-gleb@redhat.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
On 06/28/2009 03:15 PM, Gleb Natapov wrote: > Directed EOI is specified by x2APIC, but is available even when lapic is > in xAPIC mode. > > #define APIC_LVT_NUM 6 > /* 14 is the version for Xeon and Pentium 8.4.8*/ > -#define APIC_VERSION (0x14UL | ((APIC_LVT_NUM - 1)<< 16)) > +#define APIC_VERSION (0x14UL | ((APIC_LVT_NUM - 1)<< 16) | \ > + APIC_LVR_DIRECTED_EOI) > Better make that depend on the x2apic cpuid bit. > static void apic_send_ipi(struct kvm_lapic *apic) > @@ -683,7 +686,7 @@ static void apic_mmio_write(struct kvm_io_device *this, > break; > > case APIC_SPIV: > - apic_set_reg(apic, APIC_SPIV, val& 0x3ff); > + apic_set_reg(apic, APIC_SPIV, val& 0xfff); > if (!(val& APIC_SPIV_APIC_ENABLED)) { > int i; > u32 lvt_val; > Confused, you're adding bits 10 and 11 while APIC_SPIV_DIRECTED_EOI is bit 12?
On Mon, Jun 29, 2009 at 12:18:28PM +0300, Avi Kivity wrote: > On 06/28/2009 03:15 PM, Gleb Natapov wrote: >> Directed EOI is specified by x2APIC, but is available even when lapic is >> in xAPIC mode. >> >> #define APIC_LVT_NUM 6 >> /* 14 is the version for Xeon and Pentium 8.4.8*/ >> -#define APIC_VERSION (0x14UL | ((APIC_LVT_NUM - 1)<< 16)) >> +#define APIC_VERSION (0x14UL | ((APIC_LVT_NUM - 1)<< 16) | \ >> + APIC_LVR_DIRECTED_EOI) >> > > Better make that depend on the x2apic cpuid bit. > Are you sure. It looks like this feature is independent from x2APIC. It just specified by the same spec. >> static void apic_send_ipi(struct kvm_lapic *apic) >> @@ -683,7 +686,7 @@ static void apic_mmio_write(struct kvm_io_device *this, >> break; >> >> case APIC_SPIV: >> - apic_set_reg(apic, APIC_SPIV, val& 0x3ff); >> + apic_set_reg(apic, APIC_SPIV, val& 0xfff); >> if (!(val& APIC_SPIV_APIC_ENABLED)) { >> int i; >> u32 lvt_val; >> > > Confused, you're adding bits 10 and 11 while APIC_SPIV_DIRECTED_EOI is > bit 12? For well behaved guests it doesn't matter :) And Intel keep changing what reserved bits are in this register. Older doc says bit 9 is a Focus Processor bit, x2APIC doc says bit 9 is registered. So what should we do for bit 9? -- Gleb. -- To unsubscribe from this list: send the line "unsubscribe kvm" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On 06/29/2009 12:29 PM, Gleb Natapov wrote: > On Mon, Jun 29, 2009 at 12:18:28PM +0300, Avi Kivity wrote: > >> On 06/28/2009 03:15 PM, Gleb Natapov wrote: >> >>> Directed EOI is specified by x2APIC, but is available even when lapic is >>> in xAPIC mode. >>> >>> #define APIC_LVT_NUM 6 >>> /* 14 is the version for Xeon and Pentium 8.4.8*/ >>> -#define APIC_VERSION (0x14UL | ((APIC_LVT_NUM - 1)<< 16)) >>> +#define APIC_VERSION (0x14UL | ((APIC_LVT_NUM - 1)<< 16) | \ >>> + APIC_LVR_DIRECTED_EOI) >>> >>> >> Better make that depend on the x2apic cpuid bit. >> >> > Are you sure. It looks like this feature is independent from x2APIC. It just specified > by the same spec. > We're changing something that the guests sees. Suppose the guest has a bug in directed EOI, just upgrading kvm will cause it to trigger. If we make it dependent on x2apic (or something else that needs to be selected by the user), we maintain compatibility. >>> case APIC_SPIV: >>> - apic_set_reg(apic, APIC_SPIV, val& 0x3ff); >>> + apic_set_reg(apic, APIC_SPIV, val& 0xfff); >>> if (!(val& APIC_SPIV_APIC_ENABLED)) { >>> int i; >>> u32 lvt_val; >>> >>> >> Confused, you're adding bits 10 and 11 while APIC_SPIV_DIRECTED_EOI is >> bit 12? >> > For well behaved guests it doesn't matter :) And Intel keep changing > what reserved bits are in this register. Older doc says bit 9 is a Focus > Processor bit, x2APIC doc says bit 9 is registered. So what should we do > for bit 9? > Let's make it a separate patch in case something blows. I think you need to allow bit 9 even if x2apic retroactively reserves it.
On Mon, Jun 29, 2009 at 12:49:00PM +0300, Avi Kivity wrote: > On 06/29/2009 12:29 PM, Gleb Natapov wrote: >> On Mon, Jun 29, 2009 at 12:18:28PM +0300, Avi Kivity wrote: >> >>> On 06/28/2009 03:15 PM, Gleb Natapov wrote: >>> >>>> Directed EOI is specified by x2APIC, but is available even when lapic is >>>> in xAPIC mode. >>>> >>>> #define APIC_LVT_NUM 6 >>>> /* 14 is the version for Xeon and Pentium 8.4.8*/ >>>> -#define APIC_VERSION (0x14UL | ((APIC_LVT_NUM - 1)<< 16)) >>>> +#define APIC_VERSION (0x14UL | ((APIC_LVT_NUM - 1)<< 16) | \ >>>> + APIC_LVR_DIRECTED_EOI) >>>> >>>> >>> Better make that depend on the x2apic cpuid bit. >>> >>> >> Are you sure. It looks like this feature is independent from x2APIC. It just specified >> by the same spec. >> > > We're changing something that the guests sees. Suppose the guest has a > bug in directed EOI, just upgrading kvm will cause it to trigger. If we > make it dependent on x2apic (or something else that needs to be selected > by the user), we maintain compatibility. > Yes, I thought about something else (not x2apic). But yet another command line switch look like overkill. >>>> case APIC_SPIV: >>>> - apic_set_reg(apic, APIC_SPIV, val& 0x3ff); >>>> + apic_set_reg(apic, APIC_SPIV, val& 0xfff); >>>> if (!(val& APIC_SPIV_APIC_ENABLED)) { >>>> int i; >>>> u32 lvt_val; >>>> >>>> >>> Confused, you're adding bits 10 and 11 while APIC_SPIV_DIRECTED_EOI is >>> bit 12? >>> >> For well behaved guests it doesn't matter :) And Intel keep changing >> what reserved bits are in this register. Older doc says bit 9 is a Focus >> Processor bit, x2APIC doc says bit 9 is registered. So what should we do >> for bit 9? >> > > Let's make it a separate patch in case something blows. I think you > need to allow bit 9 even if x2apic retroactively reserves it. > OK. -- Gleb. -- To unsubscribe from this list: send the line "unsubscribe kvm" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On 06/29/2009 12:52 PM, Gleb Natapov wrote: >> We're changing something that the guests sees. Suppose the guest has a >> bug in directed EOI, just upgrading kvm will cause it to trigger. If we >> make it dependent on x2apic (or something else that needs to be selected >> by the user), we maintain compatibility. >> >> > Yes, I thought about something else (not x2apic). But yet another command line > switch look like overkill. > It is. So enabling on x2apic avoids the compatibility issue and avoids adding another useless control.
diff --git a/arch/x86/include/asm/apicdef.h b/arch/x86/include/asm/apicdef.h index 7ddb36a..74ca38f 100644 --- a/arch/x86/include/asm/apicdef.h +++ b/arch/x86/include/asm/apicdef.h @@ -14,6 +14,7 @@ #define APIC_LVR 0x30 #define APIC_LVR_MASK 0xFF00FF +#define APIC_LVR_DIRECTED_EOI (1 << 24) #define GET_APIC_VERSION(x) ((x) & 0xFFu) #define GET_APIC_MAXLVT(x) (((x) >> 16) & 0xFFu) #ifdef CONFIG_X86_32 @@ -40,6 +41,7 @@ #define APIC_DFR_CLUSTER 0x0FFFFFFFul #define APIC_DFR_FLAT 0xFFFFFFFFul #define APIC_SPIV 0xF0 +#define APIC_SPIV_DIRECTED_EOI (1 << 12) #define APIC_SPIV_FOCUS_DISABLED (1 << 9) #define APIC_SPIV_APIC_ENABLED (1 << 8) #define APIC_ISR 0x100 diff --git a/arch/x86/kvm/lapic.c b/arch/x86/kvm/lapic.c index 2e02865..db0c6ae 100644 --- a/arch/x86/kvm/lapic.c +++ b/arch/x86/kvm/lapic.c @@ -54,7 +54,8 @@ #define APIC_LVT_NUM 6 /* 14 is the version for Xeon and Pentium 8.4.8*/ -#define APIC_VERSION (0x14UL | ((APIC_LVT_NUM - 1) << 16)) +#define APIC_VERSION (0x14UL | ((APIC_LVT_NUM - 1) << 16) | \ + APIC_LVR_DIRECTED_EOI) #define LAPIC_MMIO_LENGTH (1 << 12) /* followed define is not in apicdef.h */ #define APIC_SHORT_MASK 0xc0000 @@ -442,9 +443,11 @@ static void apic_set_eoi(struct kvm_lapic *apic) trigger_mode = IOAPIC_LEVEL_TRIG; else trigger_mode = IOAPIC_EDGE_TRIG; - mutex_lock(&apic->vcpu->kvm->irq_lock); - kvm_ioapic_update_eoi(apic->vcpu->kvm, vector, trigger_mode); - mutex_unlock(&apic->vcpu->kvm->irq_lock); + if (!(apic_get_reg(apic, APIC_SPIV) & APIC_SPIV_DIRECTED_EOI)) { + mutex_lock(&apic->vcpu->kvm->irq_lock); + kvm_ioapic_update_eoi(apic->vcpu->kvm, vector, trigger_mode); + mutex_unlock(&apic->vcpu->kvm->irq_lock); + } } static void apic_send_ipi(struct kvm_lapic *apic) @@ -683,7 +686,7 @@ static void apic_mmio_write(struct kvm_io_device *this, break; case APIC_SPIV: - apic_set_reg(apic, APIC_SPIV, val & 0x3ff); + apic_set_reg(apic, APIC_SPIV, val & 0xfff); if (!(val & APIC_SPIV_APIC_ENABLED)) { int i; u32 lvt_val;
Directed EOI is specified by x2APIC, but is available even when lapic is in xAPIC mode. Signed-off-by: Gleb Natapov <gleb@redhat.com> --- arch/x86/include/asm/apicdef.h | 2 ++ arch/x86/kvm/lapic.c | 13 ++++++++----- 2 files changed, 10 insertions(+), 5 deletions(-)