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

[Andrew Cooper <andrew.cooper3@citrix.com>]

On 27/06/13 09:47, Jan Beulich wrote:
>>>> On 27.06.13 at 01:28, Suravee Suthikulanit <suravee.suthikulpanit@amd.com>
> wrote:
>> 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?
> I'd just drop the "BUG:". And I can certainly do so while applying.
> So in cases where you want something trivial changed, you could
> simply give an ack saying under what conditions that ack applies.
>
> Jan

I am happy with either of the two suggested tweaks to the wording.

~Andrew

>
>>> 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 
>