Re: [PATCH v3] AMD/intremap: Prevent use of per-device vector maps until irq logic is fixed

[Suravee Suthikulanit <suravee.suthikulpanit@amd.com>]

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

On 6/10/2013 7:25 AM, Jan Beulich wrote:
>>>> On 04.06.13 at 18:38, Andrew Cooper <andrew.cooper3@citrix.com> wrote:
>> XSA-36 changed the default vector map mode from global to per-device.  This is
>> because a global vector map does not prevent one PCI device from
>> impersonating
>> another and launching a DoS on the system.
>>
>> However, the per-device vector map logic is broken for devices with multiple
>> MSI-X vectors, which can either result in a failed ASSERT() or misprogramming
>> of a guests interrupt remapping tables.  The core problem is not trivial to
>> fix.
>>
>> In an effort to get AMD systems back to a non-regressed state, introduce a
>> new
>> type of vector map called per-device-global.  This uses per-device vector maps
>> in the IOMMU, but uses a single used_vector map for the core IRQ logic.
>>
>> This patch is intended to be removed as soon as the per-device logic is fixed
>> correctly.
>>
>> Signed-off-by: Andrew Cooper <andrew.cooper3@citrix.com>
> Suravee, Jacob,
>
> no opinion on this at all? I've been talked into considering this
> acceptable

Sorry for late reply, and for having missed this conversation previously.

If we have to go with this solution temporary until we have the permanent fix.
I think that is okay with me.  Although, would you mind pointing out the affect
of having "per-device" vs. "global" irq vector map?  I am not quite familiar
with the differences.

> (with a small coding style fixup, and with the question on
> the usefulness of the final warning message - imo redundant with the
> immediately preceding message that is being left untouched)

I also think the messages are quite confusing.  Actually, now that we can have
irq vector map and intremap map with different mode, we should be more explicit
in the message.

Also, the message "Not overriding irq_vector_map setting" is confusing to me.

Would you mind considering the attached patch?  Here is the sample output

(XEN) AMD-Vi: IOMMU 0 Enabled.
(XEN) AMD-Vi BUG: per-device vector map logic is broken.  Using per-device-global maps instead until a fix is found
(XEN) AMD-Vi: Enabling global irq vector map
(XEN) AMD-Vi: Enabling per-device interrupt remap table.

Thank you,

Suravee

[-- Attachment #2: AMD-intremap-Prevent-use-of-per-device-vector-maps-V4.patch --]
[-- Type: text/plain, Size: 3208 bytes --]


XSA-36 changed the default vector map mode from global to per-device.  This is
because a global vector map does not prevent one PCI device from impersonating
another and launching a DoS on the system.

However, the per-device vector map logic is broken for devices with multiple
MSI-X vectors, which can either result in a failed ASSERT() or misprogramming
of a guests interrupt remapping tables.  The core problem is not trivial to
fix.

In an effort to get AMD systems back to a non-regressed state, introduce a new
type of vector map called per-device-global.  This uses per-device vector maps
in the IOMMU, but uses a single used_vector map for the core IRQ logic.

This patch is intended to be removed as soon as the per-device logic is fixed
correctly.

Signed-off-by: Andrew Cooper <andrew.cooper3@citrix.com>

Clean up the message and  explicitely list the mode of the irq map and
interrupt remap table.

Signed-off-by: Suravee Suthikulpanit <suravee.suthikulpanit@amd.com>
---
 xen/drivers/passthrough/amd/pci_amd_iommu.c |   33 +++++++++++++++++----------
 1 file changed, 21 insertions(+), 12 deletions(-)

diff --git a/xen/drivers/passthrough/amd/pci_amd_iommu.c b/xen/drivers/passthrough/amd/pci_amd_iommu.c
index 60696d7..a11e239 100644
--- a/xen/drivers/passthrough/amd/pci_amd_iommu.c
+++ b/xen/drivers/passthrough/amd/pci_amd_iommu.c
@@ -223,21 +223,30 @@ int __init amd_iov_detect(void)
     {
         if ( amd_iommu_perdev_intremap )
         {
-            printk("AMD-Vi: Enabling per-device vector maps\n");
-            opt_irq_vector_map = OPT_IRQ_VECTOR_MAP_PERDEV;
-        }
-        else
-        {
-            printk("AMD-Vi: Enabling global vector map\n");
-            opt_irq_vector_map = OPT_IRQ_VECTOR_MAP_GLOBAL;
+            /* Per-device vector map logic is broken for devices with multiple
+             * MSI-X interrupts (and would also be for multiple MSI, if Xen
+             * supported it).
+             *
+             * Until this is fixed, use global vector tables as far as the irq
+             * logic is concerned to avoid the buggy behaviour of per-device
+             * maps in map_domain_pirq(), and use per-device tables as far as
+             * intremap code is concerned to avoid the security issue.
+             */
+            printk(XENLOG_WARNING "AMD-Vi BUG: per-device vector map logic is broken.  "
+                "Using per-device-global maps instead until a fix is found\n");
         }
+        opt_irq_vector_map = OPT_IRQ_VECTOR_MAP_GLOBAL;
     }
-    else
-    {
-        printk("AMD-Vi: Not overriding irq_vector_map setting\n");
-    }
+
+    printk("AMD-Vi: Enabling %s irq vector map\n",
+        (opt_irq_vector_map == OPT_IRQ_VECTOR_MAP_PERDEV)? "per-device": "global");
+    
+    printk("AMD-Vi: Enabling %s interrupt remap table.\n",
+        (amd_iommu_perdev_intremap)? "per-device": "global");
+
     if ( !amd_iommu_perdev_intremap )
-        printk(XENLOG_WARNING "AMD-Vi: Using global interrupt remap table is not recommended (see XSA-36)!\n");
+        printk(XENLOG_WARNING "AMD-Vi: Using global interrupt remap table is not recommended. (See XSA-36!)\n");
+
     return scan_pci_devices();
 }
 
-- 
1.7.10.4


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

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