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 15:37, Roger Pau Monné wrote:
> 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.

Hmm, okay, after some more chewing on it on this basis
Acked-by: Jan Beulich <jbeulich@xxxxxxxx>

Jan