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

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

On 6/26/2013 4:54 AM, Andrew Cooper wrote:
> On 04/06/13 17:38, Andrew Cooper 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>
> Can we get a decision on this?  The 4.3 is looming and multi MSI-X PCI
> functions are *still* broken on AMD systems, in all stable versions of
> Xen, regressed by XSA-36.
>
>  From my understanding of the points so far, we have agreed that this
> patch is suitable for 4.3 and previous, with Jan's multi-MSI series
> being the correct solution going forwards into 4.4.
Since the feedback suggesting that cleaning up is probably not 
necessary, the only thing is probably the use of the word "BUG". Could 
it be replaced with "Workaround" instead?

Suravee
>
> The only query at the moment is for the exact wording, which has had no
> attention for a week.
>
> ~Andrew
>
>> ---
>> Changes since v2:
>>   * Do not override command line.
>>   * reuse OPT_IRQ_VECTOR_MAP_GLOBAL.
>>
>> Changes since v1:
>>   * Correct stupid mistake in commit message, making it confusing to read
>>
>> diff -r 2d37d2d652a8 -r a017d74f346d xen/drivers/passthrough/amd/pci_amd_iommu.c
>> --- a/xen/drivers/passthrough/amd/pci_amd_iommu.c
>> +++ b/xen/drivers/passthrough/amd/pci_amd_iommu.c
>> @@ -223,8 +223,19 @@ 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;
>> +            /* 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
>>           {
>> @@ -235,6 +246,12 @@ int __init amd_iov_detect(void)
>>       else
>>       {
>>           printk("AMD-Vi: Not overriding irq_vector_map setting\n");
>> +
>> +        if ( opt_irq_vector_map != OPT_IRQ_VECTOR_MAP_GLOBAL )
>> +        {
>> +            printk(XENLOG_WARNING "AMD-Vi BUG: per-device vector map logic is broken.  "
>> +                   "Use irq_vector_map=global to work around.");
>> +        }
>>       }
>>       if ( !amd_iommu_perdev_intremap )
>>           printk(XENLOG_WARNING "AMD-Vi: Using global interrupt remap table is not recommended (see XSA-36)!\n");
>>
>> _______________________________________________
>> Xen-devel mailing list
>> Xen-devel@lists.xen.org
>> http://lists.xen.org/xen-devel
>