Re: [PATCH] iommu/amd-vi: do not error if device referenced in IVMD is not behind any IOMMU

[Jan Beulich <jbeulich@xxxxxxxx>]

On 09.10.2024 14:28, Teddy Astie wrote:
> Hello,
> 
> Le 09/10/2024 à 14:09, Jan Beulich a écrit :
>> On 09.10.2024 13:47, Roger Pau Monné wrote:
>>> On Wed, Oct 09, 2024 at 01:28:19PM +0200, Jan Beulich wrote:
>>>> On 09.10.2024 13:13, Roger Pau Monné wrote:
>>>>> I also think returning an error when no device in the IVMD range is
>>>>> covered by an IOMMU is dubious.  Xen will already print warning
>>>>> messages about such firmware inconsistencies, but refusing to boot is
>>>>> too strict.
>>>>
>>>> I disagree. We shouldn't enable DMA remapping in such an event. Whereas
>>>
>>> I'm not sure I understand why you would go as far as refusing to
>>> enable DMA remapping.  How is a IVMD block having references to some
>>> devices not assigned to any IOMMU different to all devices referenced
>>> not assigned to any IOMMU?  We should deal with both in the same
>>> way.
>>
>> Precisely because of ...
>>
>>> If all devices in the IVMD block are not covered by an IOMMU, the
>>> IVMD block is useless.
>>
>> ... this. We simply can't judge whether such a useless block really was
>> meant to cover something. If it was, we're hosed. Or maybe we screwed up
>> and wrongly conclude it's useless.
>>
>>>   But there's nothing for Xen to action, due to
>>> the devices not having an IOMMU assigned.  IOW: it would be the same
>>> as booting natively without parsing the IVRS in the first place.
>>
>> Not really, no. Not parsing IVRS means not turning on any IOMMU. We
>> then know we can't pass through any devices. We can't assess the
>> security of passing through devices (as far as it's under our control)
>> if we enable the IOMMU in perhaps a flawed way.
>>
>> A formally valid IVMD we can't make sense of is imo no different from
>> a formally invalid IVMD, for which we would return ENODEV as well (and
>> hence fail to enable the IOMMU). Whereas what you're suggesting is, if
>> I take it further, to basically ignore (almost) all errors in table
>> parsing, and enable the IOMMU(s) in a best effort manner, no matter
>> whether that leads to a functional (let alone secure [to the degree
>> possible]) system.
>>
>> What I don't really understand is why you want to relax our checking
>> beyond what's necessary for the one issue at hand.
>>
>>>> the "refusing to boot" is interrupt remapping related iirc, if x2APIC
>>>> is already enabled. We need to properly separate the two (and the
>>>> discussion there was started quite a long time ago, but it got stuck at
>>>> some point); until such time it is simply an undesirable side effect of
>>>> the inappropriate implementation that in certain case we fail boot when
>>>> we shouldn't.
>>>
>>> Yes, but that's a different topic, and not something I plan to fix as
>>> the scope of this patch :).
>>
>> Sure, I'm merely asking to accept that, until that's resolved, issues
>> with boot failure can result here, and need to be lived with.
> 
> Would it be possible to find a middle-ground by adding a "non-security 
> supported" xen command-line option to allow a workaround on this issue ?
> 
> Something like iommu=amd-skip-unknown-ivmd ?

Do we need to go as far? Isn't "iommu=off" enough of a (boot) workaround?

Jan