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

[Jan Beulich <jbeulich@xxxxxxxx>]

On 08.10.2024 12:47, Roger Pau Monne wrote:
> IVMD table contains restrictions about memory which must be mandatory assigned
> to devices (and which permissions it should use), or memory that should be
> never accessible to devices.
> 
> Some hardware however contains ranges in IVMD that reference devices outside 
> of
> the IVHD tables (in other words, devices not behind any IOMMU).  Such mismatch
> will cause Xen to fail in register_range_for_device(), ultimately leading to
> the IOMMU being disabled, and Xen crashing as x2APIC support might be already
> enabled and relying on the IOMMU functionality.

I find it hard to believe that on x86 systems with IOMMUs some devices would
be left uncovered by any IOMMU. Is it possible that IVHD is flawed there? In
which case we might rightfully refuse to boot? (Can you share e.g. that
"iommu=debug" output that results from parsing the tables on that system?)

> Relax IVMD parsing: allow IVMD blocks to reference devices not assigned to any
> IOMMU.  It's impossible for Xen to fulfill the requirement in the IVMD block 
> if
> the device is not behind any IOMMU, but it's no worse than booting without
> IOMMU support, and thus not parsing ACPI IVRS in the first place.
> 
> Reported-by: Willi Junga <xenproject@xxxxxx>
> Signed-off-by: Roger Pau Monné <roger.pau@xxxxxxxxxx>
> ---
>  xen/drivers/passthrough/amd/iommu_acpi.c | 5 +++--
>  1 file changed, 3 insertions(+), 2 deletions(-)
> 
> diff --git a/xen/drivers/passthrough/amd/iommu_acpi.c 
> b/xen/drivers/passthrough/amd/iommu_acpi.c
> index 3f5508eba049..c416120326c9 100644
> --- a/xen/drivers/passthrough/amd/iommu_acpi.c
> +++ b/xen/drivers/passthrough/amd/iommu_acpi.c
> @@ -248,8 +248,9 @@ static int __init register_range_for_device(
>      iommu = find_iommu_for_device(seg, bdf);
>      if ( !iommu )
>      {
> -        AMD_IOMMU_ERROR("IVMD: no IOMMU for Dev_Id %#x\n", bdf);
> -        return -ENODEV;
> +        AMD_IOMMU_WARN("IVMD: no IOMMU for device %pp - ignoring 
> constrain\n",

I'm not a native speaker, but "constrain" to me can only be a verb (with
"constraint" being the noun). IOW as worded I'm afraid I can't make sense
of the message.

> +                       &PCI_SBDF(seg, bdf));
> +        return 0;
>      }
>      req = ivrs_mappings[bdf].dte_requestor_id;
>  

Down from here in parse_ivmd_device_iommu() is somewhat similar code.
Wouldn't that need adjusting similarly then? Or else shouldn't the
adjustment above be accompanied by a comment clarifying that the
behavior is just because of observations on certain hardware?

Jan