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

[Suravee Suthikulpanit <suravee.suthikulpanit@xxxxxxx>]

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?

> 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.

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?

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?

> +    /* Inclusive mappings are enabled by default for PV. */
> +    if ( iommu_hwdom_inclusive == -1 )
> +        iommu_hwdom_inclusive = is_pv_domain(d);
> +    /* Reserved IOMMU mappings are enabled by default. */
> +    if ( iommu_hwdom_reserved == -1 )
> +        iommu_hwdom_reserved = 1;
> +
>       if ( iommu_hwdom_inclusive && !is_pv_domain(d) )
>       {
>           printk(XENLOG_WARNING
Thanks,
Suravee

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