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

From: "Roger Pau Monné" <roger.pau@citrix.com>
To: Jan Beulich <jbeulich@suse.com>
Cc: Kevin Tian <kevin.tian@intel.com>, xen-devel@lists.xenproject.org
Subject: Re: [PATCH 3/4] iommu/vt-d: switch to common RMRR checker
Date: Wed, 7 Feb 2024 10:01:24 +0100	[thread overview]
Message-ID: <ZcNG5C9gEzlcWJPw@macbook> (raw)
In-Reply-To: <90f8e183-5e78-49e8-ae2f-9898c1d7686b@suse.com>

On Tue, Feb 06, 2024 at 12:28:07PM +0100, Jan Beulich wrote:
> 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?

This is now better handled, since the VT-d code will use the same
logic as the AMD-Vi logic and attempt to 'convert' such bogus RMRR
regions so they can be safely used.  iommu_unity_region_ok() signals
the RMRR region is impossible to be used, and hence not even
iommu_inclusive_mapping would help in that case.  Also note that
iommu_inclusive_mapping is only applicable to PV, so the message was
already wrong in the PVH case.

> 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()?

Oh, indeed, I've converted them all to WARN, when the first one is
indeed WARN, but the following two are 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?

I debated myself whether to keep the RMRR/IVMD prefix, but I didn't
think there was a lot of value in it, since whether it's an RMRR or an
IVMD region can be deduced from previous messages.  Anyway will pass
the prefix as a function parameter.

Thanks, Roger.