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

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

On 17/06/13 10:00, Jan Beulich wrote:
>>>> On 17.06.13 at 10:55, George Dunlap <george.dunlap@eu.citrix.com> wrote:
>> On 15/06/13 02:13, Suravee Suthikulanit wrote:
>>> 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
>> At the very least it can't say BUG -- that needs to be reserved for 
>> things that actually cause the host to crash (a la BUG_ON()).
> That was worded this way in Andrew's original version of the patch
> too, and I had also already noted that this wording is too strong.
>
> Jan
>

I am not overly attached to the current wording, so feel free to tweak
if you wish.

As for my opinions of the revised patch:

The explicit print of the IOMMU mode is nice, although those in the know
could already work it out given the reference or lackthereof to XSA-36.

The explicit print of the vector map is wrong, and is liable to be
disappearing in 4.4 anyway.

On the face of it, the revised patch seems much more like a general
cleanup of the printing, rather than the temporary bugfix it is indented
to be.  I dont have a problem with the cleanup persay, but it should be
part of a separate patch.

~Andrew