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

["Jan Beulich" <JBeulich@xxxxxxxx>]

>>> On 27.06.13 at 01:28, Suravee Suthikulanit <suravee.suthikulpanit@xxxxxxx>
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@xxxxxxxxxx>
>> 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

>> 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@xxxxxxxxxxxxx 
>>> http://lists.xen.org/xen-devel 
>>



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