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

[Teddy Astie <teddy.astie@xxxxxxxxxx>]

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

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 ?

And preventing boot otherwise.

Teddy


Teddy Astie | Vates XCP-ng Intern

XCP-ng & Xen Orchestra - Vates solutions

web: https://vates.tech