diff mbox

[1/3,v2] Add Directed EOI support to APIC emulation

Message ID 1246191331-31085-2-git-send-email-gleb@redhat.com (mailing list archive)
State New, archived
Headers show

Commit Message

Gleb Natapov June 28, 2009, 12:15 p.m. UTC 
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(-)

Comments

Avi Kivity June 29, 2009, 9:18 a.m. UTC | #1 
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?
Gleb Natapov June 29, 2009, 9:29 a.m. UTC | #2 
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
Avi Kivity June 29, 2009, 9:49 a.m. UTC | #3 
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.
Gleb Natapov June 29, 2009, 9:52 a.m. UTC | #4 
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
Avi Kivity June 29, 2009, 10:01 a.m. UTC | #5 
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 mbox

Patch

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;