Re: [Xen-devel] [PATCH] amd/iommu: remove hidden AMD inclusive mappings

[Roger Pau Monné <roger.pau@xxxxxxxxxx>]

On Tue, Oct 02, 2018 at 02:37:31PM +0700, Suravee Suthikulpanit wrote:
> Roger,
> 
> I'm still catching up on the map-inclusive and map-reserved stuff.
> I have a couple questions below.
> 
> On 9/21/18 10:20 PM, Roger Pau Monne wrote:
> > And just rely on arch_iommu_hwdom_init to setup the correct inclusive
> > mappings as it's done for Intel.
> > 
> > AMD has code in amd_iommu_hwdom_init to setup inclusive mappings up to
> > max_pdx, remove this since it's now a duplication of
> > arch_iommu_hwdom_init. Note that AMD mapped every page with a valid
> > mfn up to max_pdx, arch_iommu_hwdom_init will only do so for memory
> > below 4GB, so this is a functional change for AMD.
> 
> Is there any reasons why limit to only below 4GB?

This is what Intel IOMMU has always done, so this patch unifies the
inclusive mappings code between Intel and AMD. Note that reserved
regions above 4GB will still be mapped.

> > Move the default setting of iommu_hwdom_{inclusive/reserved} to
> > arch_iommu_hwdom_init since the defaults are now the same for both
> > Intel and AMD.
> > 
> > Reported-by: Paul Durrant <paul.durrant@xxxxxxxxxx>
> > Signed-off-by: Roger Pau Monné <roger.pau@xxxxxxxxxx>
> > ---
> > Cc: Suravee Suthikulpanit <suravee.suthikulpanit@xxxxxxx>
> > Cc: Brian Woods <brian.woods@xxxxxxx>
> > Cc: Kevin Tian <kevin.tian@xxxxxxxxx>
> > Cc: Jan Beulich <jbeulich@xxxxxxxx>
> > Cc: Paul Durrant <paul.durrant@xxxxxxxxxx>
> > ---
> >   xen/drivers/passthrough/amd/pci_amd_iommu.c | 39 ---------------------
> >   xen/drivers/passthrough/vtd/iommu.c         |  7 ----
> >   xen/drivers/passthrough/x86/iommu.c         |  8 ++++-
> >   3 files changed, 7 insertions(+), 47 deletions(-)
> > 
> > ...
> >
> > diff --git a/xen/drivers/passthrough/x86/iommu.c 
> > b/xen/drivers/passthrough/x86/iommu.c
> > index b7c8b5be41..2de8822c59 100644
> > --- a/xen/drivers/passthrough/x86/iommu.c
> > +++ b/xen/drivers/passthrough/x86/iommu.c
> > @@ -210,7 +210,13 @@ void __hwdom_init arch_iommu_hwdom_init(struct domain 
> > *d)
> >       BUG_ON(!is_hardware_domain(d));
> > -    ASSERT(iommu_hwdom_inclusive != -1 && iommu_hwdom_inclusive != -1);
> 
> Not sure if this was a typo. The logic looks strange. Anyhow, it is removed.

Oh right, this should check inclusive and reserved.

> However, I notice in the xen/drivers/passthrough/iommu.c that
> the parsing logic for the map-reserved option is setting the 
> iommu_hwdom_inclusive
> instead of the iommu_hwdom_reserved. Is that intentional?

It's the same typo as above, will send a patch to fix it.

> Also, what's the difference b/w the option map-inclusive parameter in 
> drivers/passthrough/iommu.c:
> parse_dom0_iommu_param() and the option iommu_inclusive_mapping in 
> drivers/passthrough/vtd/x86/vtd.c?

The old iommu_inclusive_mapping option is keep for compatibility
reasons, but the documentation has been updated to mark that option as
deprecated.

Thanks, Roger.

_______________________________________________
Xen-devel mailing list
Xen-devel@xxxxxxxxxxxxxxxxxxxx
https://lists.xenproject.org/mailman/listinfo/xen-devel