| Message ID | 20240201170159.66330-4-roger.pau@citrix.com (mailing list archive) |
|---|---|
| State | New |
| Headers | show |
| Series | iommu/x86: fixes/improvements for unity range checks | expand |
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
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.
On 07.02.2024 10:01, Roger Pau Monné wrote: > 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. Impossible only in so far as we don't know whether a such named region would actually still be accessed post-boot. But yes, if it wouldn't be accessed, there would also be no need for passing the extra option. > Also note that > iommu_inclusive_mapping is only applicable to PV, so the message was > already wrong in the PVH case. This is a fair point, which probably wants mentioning as (partial) justification. Plus iirc the intention was to get rid of that option anyway, at some point. Jan
diff --git a/xen/drivers/passthrough/vtd/dmar.c b/xen/drivers/passthrough/vtd/dmar.c index 07772f178fe6..005b42706a34 100644 --- 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; rmrru = xzalloc(struct acpi_rmrr_unit); if ( !rmrru )
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. Signed-off-by: Roger Pau Monné <roger.pau@citrix.com> --- xen/drivers/passthrough/vtd/dmar.c | 14 +++----------- 1 file changed, 3 insertions(+), 11 deletions(-)