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

["Roger Pau Monné" <roger.pau@citrix.com>]

On Wed, Oct 09, 2024 at 02:09:33PM +0200, Jan Beulich wrote:
> 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.

The same could be stated about devices in a IVMD block that are not
assigned to an IOMMU: it could also be Xen that screwed up and wrongly
concluded they are not assigned to an IOMMU.

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

No, don't take it further: not ignore all errors, I think we should
ignore errors when the device in the IVMD is not behind an IOMMU.  And
I think that should apply globally, regardless of whether all devices
in the IVMD block fall in that category.

That will bring AMD-Vi inline with VT-d RMRR, as from what I can see
acpi_parse_one_rmrr() doesn't care whether the device scope in the
entry is behind an IOMMU or not, or whether the whole RMRR doesn't
effectively apply to any device because none of them is covered by an
IOMMU.

> What I don't really understand is why you want to relax our checking
> beyond what's necessary for the one issue at hand.

This issue has been reported to us and we have been able to debug.
However, I worry what other malformed IVMD blocks might be out there,
for example an IVMD block that applies to a single device (type 21h),
but such single device doesn't exist (or it's not behind an IOMMU).
Maybe next time we simply won't get a report, the user will try Xen,
see it's not working and move to something else.

I've taken a quick look at Linux, and when parsing the IVMD blocks
there's no checking that referred devices are behind an IOMMU, the
regions are just added to a list.

Thanks, Roger.