Re: [PATCH 3/4] iommu/vt-d: switch to common RMRR checker

[Jan Beulich <jbeulich@xxxxxxxx>]

On 01.02.2024 18:01, Roger Pau Monne wrote:
> Use the newly introduced generic unity map checker.
> 
> Also drop the message recommending the usage of iommu_inclusive_mapping: the
> ranges would end up being mapped anyway even if some of the checks above
> failed, regardless of whether iommu_inclusive_mapping is set.

I'm afraid I don't understand this: When not in an appropriate E820
region, you now even fail IOMMU initialization. Shouldn't such
failure only occur when inclusive mappings weren't requested? At
which point referring to that option is still relevant?

Further to this failing - in patch 2 shouldn't the respective log
messages then be XENLOG_ERR, matching the earlier use of
AMD_IOMMU_ERROR()?

> --- a/xen/drivers/passthrough/vtd/dmar.c
> +++ b/xen/drivers/passthrough/vtd/dmar.c
> @@ -642,17 +642,9 @@ acpi_parse_one_rmrr(struct acpi_dmar_header *header)
>             return -EEXIST;
>         }
>  
> -    /* This check is here simply to detect when RMRR values are
> -     * not properly represented in the system memory map and
> -     * inform the user
> -     */
> -    if ( !e820_all_mapped(base_addr, end_addr + 1, E820_RESERVED) &&
> -         !e820_all_mapped(base_addr, end_addr + 1, E820_NVS) &&
> -         !e820_all_mapped(base_addr, end_addr + 1, E820_ACPI) )
> -        printk(XENLOG_WARNING VTDPREFIX
> -               " RMRR [%"PRIx64",%"PRIx64"] not in reserved memory;"
> -               " need \"iommu_inclusive_mapping=1\"?\n",
> -                base_addr, end_addr);
> +    if ( !iommu_unity_region_ok(maddr_to_mfn(base_addr),
> +                                maddr_to_mfn(end_addr + PAGE_SIZE)) )
> +        return -EIO;

Hmm, noticing only here, but applicable also to the earlier patch: The
"RMRR" (and there "IVMD") is lost, which removes some relevant context
information from the log messages. Can you add a const char* parameter
to the new helper, please?

Jan