[PATCH] Fixing ioapic write order in io_apic_write_remap_rte

[PATCH] Fixing ioapic write order in io_apic_write_remap_rte

[Han, Weidong]

[-- Attachment #1: Type: text/plain, Size: 1091 bytes --]

At the end of io_apic_write_remap_rte, it writes new entry (remapped interrupt) to ioapic. But it writes low 32 bits before high 32 bits, it unmasks interrupt before writing high 32 bits if 'mask' bit in low 32 bits is cleared. Thus it may result in issues. This patch fixes this issue by writing high 32 bits before low 32 bits. 

Signed-off-by: Jiang, Yunhong <yunhong.jiang@intel.com>
Signed-off-by: Weidong Han <weidong.han@intel.com>

diff -r 6c18aa5a848e xen/drivers/passthrough/vtd/intremap.c
--- a/xen/drivers/passthrough/vtd/intremap.c	Mon Aug 02 12:49:12 2010 -0400
+++ b/xen/drivers/passthrough/vtd/intremap.c	Fri Aug 06 11:47:03 2010 -0400
@@ -444,10 +444,10 @@ void io_apic_write_remap_rte(
     }
 
     /* write new entry to ioapic */
+    *IO_APIC_BASE(apic) = reg + 1;
+    *(IO_APIC_BASE(apic)+4) = *(((u32 *)&old_rte)+1);
     *IO_APIC_BASE(apic) = reg;
     *(IO_APIC_BASE(apic)+4) = *(((u32 *)&old_rte)+0);
-    *IO_APIC_BASE(apic) = reg + 1;
-    *(IO_APIC_BASE(apic)+4) = *(((u32 *)&old_rte)+1);
 }
 
 #if defined(__i386__) || defined(__x86_64__)

[-- Attachment #2: fix-IR-write.patch --]
[-- Type: application/octet-stream, Size: 630 bytes --]

diff -r 6c18aa5a848e xen/drivers/passthrough/vtd/intremap.c
--- a/xen/drivers/passthrough/vtd/intremap.c	Mon Aug 02 12:49:12 2010 -0400
+++ b/xen/drivers/passthrough/vtd/intremap.c	Fri Aug 06 11:47:03 2010 -0400
@@ -444,10 +444,10 @@ void io_apic_write_remap_rte(
     }
 
     /* write new entry to ioapic */
+    *IO_APIC_BASE(apic) = reg + 1;
+    *(IO_APIC_BASE(apic)+4) = *(((u32 *)&old_rte)+1);
     *IO_APIC_BASE(apic) = reg;
     *(IO_APIC_BASE(apic)+4) = *(((u32 *)&old_rte)+0);
-    *IO_APIC_BASE(apic) = reg + 1;
-    *(IO_APIC_BASE(apic)+4) = *(((u32 *)&old_rte)+1);
 }
 
 #if defined(__i386__) || defined(__x86_64__)

[-- Attachment #3: Type: text/plain, Size: 138 bytes --]

_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xensource.com
http://lists.xensource.com/xen-devel

Re: [PATCH] Fixing ioapic write order in io_apic_write_remap_rte

[Jan Beulich]

>>> On 09.08.10 at 05:18, "Han, Weidong" <weidong.han@intel.com> wrote:
> At the end of io_apic_write_remap_rte, it writes new entry (remapped 
> interrupt) to ioapic. But it writes low 32 bits before high 32 bits, it 
> unmasks interrupt before writing high 32 bits if 'mask' bit in low 32 bits is 
> cleared. Thus it may result in issues. This patch fixes this issue by writing 
> high 32 bits before low 32 bits. 

While I fully agree with this change, isn't there another problem in the
error handling path in that the mask bit would not get cleared again
if the write is to the upper half of the RTE?

Jan

Re: [PATCH] Fixing ioapic write order in io_apic_write_remap_rte

[Weidong Han]

[-- Attachment #1: Type: text/plain, Size: 1637 bytes --]

Jan Beulich wrote:
>>>> On 09.08.10 at 05:18, "Han, Weidong" <weidong.han@intel.com> wrote:
>>>>         
>> At the end of io_apic_write_remap_rte, it writes new entry (remapped 
>> interrupt) to ioapic. But it writes low 32 bits before high 32 bits, it 
>> unmasks interrupt before writing high 32 bits if 'mask' bit in low 32 bits is 
>> cleared. Thus it may result in issues. This patch fixes this issue by writing 
>> high 32 bits before low 32 bits. 
>>     
>
> While I fully agree with this change, isn't there another problem in the
> error handling path in that the mask bit would not get cleared again
> if the write is to the upper half of the RTE?
>
> Jan
>
>   
Yes, it's a problem. Good catch. Below patch should fix it.

When ioapic_rte_to_remap_entry fails, currently it just writes value to 
ioapic. But the 'mask' bit may be changed if it writes to the upper half 
of RTE. This patch ensures to recover the original value of 'mask' bit 
in this case.

Signed-off-by: Weidong Han <weidong.han@intel.com>

diff -r aceb28f902ec xen/drivers/passthrough/vtd/intremap.c
--- a/xen/drivers/passthrough/vtd/intremap.c    Fri Aug 06 11:47:46 2010 
-0400
+++ b/xen/drivers/passthrough/vtd/intremap.c    Mon Aug 09 12:34:43 2010 
-0400
@@ -440,6 +440,13 @@ void io_apic_write_remap_rte(
     {
         *IO_APIC_BASE(apic) = rte_upper ? (reg + 1) : reg;
         *(IO_APIC_BASE(apic)+4) = value;
+
+        /* Recover the original value of 'mask' bit */
+        if ( rte_upper )
+        {
+            *IO_APIC_BASE(apic) = reg;
+            *(IO_APIC_BASE(apic)+4) = *(((u32 *)&old_rte)+0);
+        }
         return;
     }
 

[-- Attachment #2: recover-mask-bit.patch --]
[-- Type: text/plain, Size: 607 bytes --]

diff -r aceb28f902ec xen/drivers/passthrough/vtd/intremap.c
--- a/xen/drivers/passthrough/vtd/intremap.c	Fri Aug 06 11:47:46 2010 -0400
+++ b/xen/drivers/passthrough/vtd/intremap.c	Mon Aug 09 12:34:43 2010 -0400
@@ -440,6 +440,13 @@ void io_apic_write_remap_rte(
     {
         *IO_APIC_BASE(apic) = rte_upper ? (reg + 1) : reg;
         *(IO_APIC_BASE(apic)+4) = value;
+
+        /* Recover the original value of 'mask' bit */
+        if ( rte_upper )
+        {
+            *IO_APIC_BASE(apic) = reg;
+            *(IO_APIC_BASE(apic)+4) = *(((u32 *)&old_rte)+0);
+        }
         return;
     }
 

[-- Attachment #3: Type: text/plain, Size: 138 bytes --]

_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xensource.com
http://lists.xensource.com/xen-devel