Re: [PATCH v7 3/6] iommu: make iommu_inclusive_mapping a suboption of dom0-iommu

[Julien Grall <julien.grall@arm.com>]

Hi Roger,

On 22/08/18 08:51, Roger Pau Monne wrote:
> Introduce a new dom0-iommu=map-inclusive generic option that
> supersedes iommu_inclusive_mapping. The previous behavior is preserved
> and the option should only be enabled by default on Intel hardware.
> 
> Signed-off-by: Roger Pau Monné <roger.pau@citrix.com>
> Reviewed-by: Paul Durrant <paul.durrant@citrix.com>
> Reviewed-by: Jan Beulich <jbeulich@suse.com>
> Reviewed-by: Kevin Tian <kevin.tian@intel.com>
> ---
> Changes since v4:
>   - Use an if to set the default option value.
>   - Set the option to false unconditionally on ARM.
> 
> Changes since v2:
>   - Fix typo in commit message.
>   - Change style and text of the documentation in xen command line.
>   - Set the defaults in {intel/amd}_iommu_hwdom_init for inclusive.
>   - Re-add the iommu_dom0_passthrough || !is_pv_domain(d) check.
> 
> Changes since v1:
>   - Use dom0-iommu instead of the iommu option.
>   - Only enable by default on Intel hardware.
> ---
> Cc: Andrew Cooper <andrew.cooper3@citrix.com>
> Cc: George Dunlap <George.Dunlap@eu.citrix.com>
> Cc: Ian Jackson <ian.jackson@eu.citrix.com>
> Cc: Jan Beulich <jbeulich@suse.com>
> Cc: Julien Grall <julien.grall@arm.com>
> Cc: Konrad Rzeszutek Wilk <konrad.wilk@oracle.com>
> Cc: Stefano Stabellini <sstabellini@kernel.org>
> Cc: Tim Deegan <tim@xen.org>
> Cc: Wei Liu <wei.liu2@citrix.com>
> Cc: Suravee Suthikulpanit <suravee.suthikulpanit@amd.com>
> Cc: Brian Woods <brian.woods@amd.com>
> Cc: Kevin Tian <kevin.tian@intel.com>
> ---
>   docs/misc/xen-command-line.markdown         | 13 ++++-
>   xen/drivers/passthrough/amd/pci_amd_iommu.c |  4 ++
>   xen/drivers/passthrough/arm/iommu.c         |  4 ++
>   xen/drivers/passthrough/arm/smmu.c          |  2 +
>   xen/drivers/passthrough/iommu.c             | 12 +++++
>   xen/drivers/passthrough/vtd/extern.h        |  2 -
>   xen/drivers/passthrough/vtd/iommu.c         |  8 ++-
>   xen/drivers/passthrough/vtd/x86/vtd.c       | 58 +-------------------
>   xen/drivers/passthrough/x86/iommu.c         | 59 +++++++++++++++++++++
>   xen/include/xen/iommu.h                     |  2 +
>   10 files changed, 99 insertions(+), 65 deletions(-)
> 
> diff --git a/docs/misc/xen-command-line.markdown b/docs/misc/xen-command-line.markdown
> index cd57960ede..98f0f3b68b 100644
> --- a/docs/misc/xen-command-line.markdown
> +++ b/docs/misc/xen-command-line.markdown
> @@ -682,7 +682,7 @@ Flag that makes a dom0 use shadow paging. Only works when "pvh" is
>   enabled.
>   
>   ### dom0-iommu
> -> `= List of [ passthrough | strict ]`
> +> `= List of [ passthrough | strict | map-inclusive ]`
>   
>   This list of booleans controls the iommu usage by Dom0:
>   
> @@ -696,6 +696,14 @@ This list of booleans controls the iommu usage by Dom0:
>     `true` for a PVH Dom0 and any attempt to overwrite it from the command line
>     is ignored.
>   
> +* `map-inclusive`: sets up DMA remapping for all the non-RAM regions below 4GB
> +  except for unusable ranges. Use this to work around firmware issues providing
> +  incorrect RMRR/IVMD entries. Rather than only mapping RAM pages for IOMMU
> +  accesses for Dom0, with this option all pages up to 4GB, not marked as
> +  unusable in the E820 table, will get a mapping established. Note that this
> +  option is only applicable to a PV Dom0 and is enabled by default on Intel
> +  hardware.
> +
>   ### dom0\_ioports\_disable (x86)
>   > `= List of <hex>-<hex>`
>   
> @@ -1233,6 +1241,9 @@ wait descriptor timed out', try increasing this value.
>   ### iommu\_inclusive\_mapping (VT-d)
>   > `= <boolean>`
>   
> +**WARNING: This command line option is deprecated, and superseded by
> +_dom0-iommu=map-inclusive_ - using both options in combination is undefined.**
> +
>   > Default: `true`
>   
>   Use this to work around firmware issues providing incorrect RMRR entries.
> diff --git a/xen/drivers/passthrough/amd/pci_amd_iommu.c b/xen/drivers/passthrough/amd/pci_amd_iommu.c
> index ab39e7500d..27eb49619d 100644
> --- a/xen/drivers/passthrough/amd/pci_amd_iommu.c
> +++ b/xen/drivers/passthrough/amd/pci_amd_iommu.c
> @@ -253,6 +253,10 @@ static void __hwdom_init amd_iommu_hwdom_init(struct domain *d)
>       unsigned long i;
>       const struct amd_iommu *iommu;
>   
> +    /* Inclusive IOMMU mappings are disabled by default on AMD hardware. */
> +    if ( iommu_hwdom_inclusive == -1 )
> +        iommu_hwdom_inclusive = false;

This is a quite bad practice to mix boolean and integer. Can you please 
rework the code to avoid that?

> +
>       if ( allocate_domain_resources(dom_iommu(d)) )
>           BUG();
>   
> diff --git a/xen/drivers/passthrough/arm/iommu.c b/xen/drivers/passthrough/arm/iommu.c
> index 95b1abb972..325997b19f 100644
> --- a/xen/drivers/passthrough/arm/iommu.c
> +++ b/xen/drivers/passthrough/arm/iommu.c
> @@ -73,3 +73,7 @@ int arch_iommu_populate_page_table(struct domain *d)
>       /* The IOMMU shares the p2m with the CPU */
>       return -ENOSYS;
>   }
> +
> +void __hwdom_init arch_iommu_hwdom_init(struct domain *d)
> +{
> +}
> diff --git a/xen/drivers/passthrough/arm/smmu.c b/xen/drivers/passthrough/arm/smmu.c
> index 74c09b0991..b142677b8c 100644
> --- a/xen/drivers/passthrough/arm/smmu.c
> +++ b/xen/drivers/passthrough/arm/smmu.c
> @@ -2727,6 +2727,8 @@ static int arm_smmu_iommu_domain_init(struct domain *d)
>   
>   static void __hwdom_init arm_smmu_iommu_hwdom_init(struct domain *d)
>   {
> +	/* Set to false options not supported on ARM. */
> +	iommu_hwdom_inclusive = false;

I would prefer if print a warning when the value is not false and then 
set to false.

>   }
>   
>   static void arm_smmu_iommu_domain_teardown(struct domain *d)
> diff --git a/xen/drivers/passthrough/iommu.c b/xen/drivers/passthrough/iommu.c
> index 8f9975cf4e..5798142730 100644
> --- a/xen/drivers/passthrough/iommu.c
> +++ b/xen/drivers/passthrough/iommu.c
> @@ -61,6 +61,7 @@ bool_t __read_mostly iommu_intremap = 1;
>   
>   bool __hwdom_initdata iommu_hwdom_strict;
>   bool __read_mostly iommu_hwdom_passthrough;
> +int8_t __hwdom_initdata iommu_hwdom_inclusive = -1;
>   
>   /*
>    * In the current implementation of VT-d posted interrupts, in some extreme
> @@ -152,6 +153,8 @@ static int __init parse_dom0_iommu_param(const char *s)
>               iommu_hwdom_passthrough = val;
>           else if ( (val = parse_boolean("strict", s, ss)) >= 0 )
>               iommu_hwdom_strict = val;
> +        else if ( (val = parse_boolean("map-inclusive", s, ss)) >= 0 )
> +            iommu_hwdom_inclusive = val;
>           else
>               rc = -EINVAL;
>   
> @@ -232,6 +235,15 @@ void __hwdom_init iommu_hwdom_init(struct domain *d)
>       }
>   
>       hd->platform_ops->hwdom_init(d);
> +
> +    if ( iommu_hwdom_inclusive && !is_pv_domain(d) )
> +    {
> +        printk(XENLOG_WARNING
> +               "IOMMU inclusive mappings are only supported on PV Dom0\n");
> +        iommu_hwdom_inclusive = false;

Same remark as above about mixing boolean and integer.

> +    }
> +
> +    arch_iommu_hwdom_init(d);
>   }
>   

Cheers,

-- 
Julien Grall

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