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