[PATCH 0/4] x86/iommu: PVH Dom0 workarounds for missing RMRR/IRSV entries

[PATCH 0/4] x86/iommu: PVH Dom0 workarounds for missing RMRR/IRSV entries

[Roger Pau Monne]

Hello,

The following series implement a workaround for missing RMRR/IRSV
entries for a PVH Dom0. It's based on the iommu_inclusive_mapping VTd
option, which is generalized to be used by both VTd and AMD-Vi
hardware.

The PVH workaround identity maps all regions marked as reserved in the
memory map.

The series can be found at:

git://xenbits.xen.org/people/royger/xen.git iommu_inclusive_v1

Thanks, Roger.

Roger Pau Monne (4):
  iommu: remove unneeded return from iommu_hwdom_init
  iommu: generalize iommu_inclusive_mapping
  x86/iommu: reorder conditions used in the inclusive iommu mappings
  x86/iommu: add PVH support to the inclusive options

 docs/misc/xen-command-line.markdown   |  20 +++++
 xen/drivers/passthrough/arm/iommu.c   |   4 +
 xen/drivers/passthrough/iommu.c       |   8 +-
 xen/drivers/passthrough/vtd/extern.h  |   2 -
 xen/drivers/passthrough/vtd/iommu.c   |   6 --
 xen/drivers/passthrough/vtd/x86/vtd.c |  66 +--------------
 xen/drivers/passthrough/x86/iommu.c   | 117 ++++++++++++++++++++++++++
 xen/include/xen/iommu.h               |   2 +
 8 files changed, 151 insertions(+), 74 deletions(-)

-- 
2.18.0


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

[PATCH 1/4] iommu: remove unneeded return from iommu_hwdom_init

[Roger Pau Monne]

Signed-off-by: Roger Pau Monné <roger.pau@citrix.com>
---
Cc: Jan Beulich <jbeulich@suse.com>
---
 xen/drivers/passthrough/iommu.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/xen/drivers/passthrough/iommu.c b/xen/drivers/passthrough/iommu.c
index 2c44fabf99..70d218f910 100644
--- a/xen/drivers/passthrough/iommu.c
+++ b/xen/drivers/passthrough/iommu.c
@@ -207,7 +207,7 @@ void __hwdom_init iommu_hwdom_init(struct domain *d)
                    d->domain_id, rc);
     }
 
-    return hd->platform_ops->hwdom_init(d);
+    hd->platform_ops->hwdom_init(d);
 }
 
 void iommu_teardown(struct domain *d)
-- 
2.18.0


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

Re: [PATCH 1/4] iommu: remove unneeded return from iommu_hwdom_init

[Paul Durrant]

> -----Original Message-----
> From: Xen-devel [mailto:xen-devel-bounces@lists.xenproject.org] On Behalf
> Of Roger Pau Monne
> Sent: 27 July 2018 16:32
> To: xen-devel@lists.xenproject.org
> Cc: Jan Beulich <jbeulich@suse.com>; Roger Pau Monne
> <roger.pau@citrix.com>
> Subject: [Xen-devel] [PATCH 1/4] iommu: remove unneeded return from
> iommu_hwdom_init
> 
> Signed-off-by: Roger Pau Monné <roger.pau@citrix.com>

Yeah, I noticed this too...

Reviewed-by: Paul Durrant <paul.durrant@citrix.com>

> ---
> Cc: Jan Beulich <jbeulich@suse.com>
> ---
>  xen/drivers/passthrough/iommu.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/xen/drivers/passthrough/iommu.c
> b/xen/drivers/passthrough/iommu.c
> index 2c44fabf99..70d218f910 100644
> --- a/xen/drivers/passthrough/iommu.c
> +++ b/xen/drivers/passthrough/iommu.c
> @@ -207,7 +207,7 @@ void __hwdom_init iommu_hwdom_init(struct
> domain *d)
>                     d->domain_id, rc);
>      }
> 
> -    return hd->platform_ops->hwdom_init(d);
> +    hd->platform_ops->hwdom_init(d);
>  }
> 
>  void iommu_teardown(struct domain *d)
> --
> 2.18.0
> 
> 
> _______________________________________________
> Xen-devel mailing list
> Xen-devel@lists.xenproject.org
> https://lists.xenproject.org/mailman/listinfo/xen-devel
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel

[PATCH 2/4] iommu: generalize iommu_inclusive_mapping

[Roger Pau Monne]

Introduce a new iommu=inclusive generic option that supersedes
iommu_inclusive_mapping. This should be a non-functional change on
Intel hardware, while AMD hardware will gain the same functionality of
mapping almost everything below the 4GB boundary.

Note that is a noop for ARM hardware.

Signed-off-by: Roger Pau Monné <roger.pau@citrix.com>
---
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: Kevin Tian <kevin.tian@intel.com>
---
 docs/misc/xen-command-line.markdown   | 14 ++++++
 xen/drivers/passthrough/arm/iommu.c   |  4 ++
 xen/drivers/passthrough/iommu.c       |  6 +++
 xen/drivers/passthrough/vtd/extern.h  |  2 -
 xen/drivers/passthrough/vtd/iommu.c   |  6 ---
 xen/drivers/passthrough/vtd/x86/vtd.c | 66 +------------------------
 xen/drivers/passthrough/x86/iommu.c   | 70 +++++++++++++++++++++++++++
 xen/include/xen/iommu.h               |  2 +
 8 files changed, 97 insertions(+), 73 deletions(-)

diff --git a/docs/misc/xen-command-line.markdown b/docs/misc/xen-command-line.markdown
index 65b4754418..91a8bfc9a6 100644
--- a/docs/misc/xen-command-line.markdown
+++ b/docs/misc/xen-command-line.markdown
@@ -1198,6 +1198,17 @@ detection of systems known to misbehave upon accesses to that port.
 
 >> Enable IOMMU debugging code (implies `verbose`).
 
+> `inclusive`
+
+> Default: `true`
+
+>> Use this to work around firmware issues providing incorrect RMRR or 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. Also note that if `dom0-strict` mode is enabled
+>> then conventional RAM pages not assigned to dom0 will not be mapped.
+
 ### iommu\_dev\_iotlb\_timeout
 > `= <integer>`
 
@@ -1212,6 +1223,9 @@ wait descriptor timed out', try increasing this value.
 
 > Default: `true`
 
+**WARNING: This command line option is deprecated, and superseded by
+_iommu=inclusive_ - using both options in combination is undefined.**
+
 Use this to work around firmware issues providing incorrect RMRR 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
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/iommu.c b/xen/drivers/passthrough/iommu.c
index 70d218f910..3f3aa71b2c 100644
--- a/xen/drivers/passthrough/iommu.c
+++ b/xen/drivers/passthrough/iommu.c
@@ -47,6 +47,9 @@ integer_param("iommu_dev_iotlb_timeout", iommu_dev_iotlb_timeout);
  *   no-igfx                    Disable VT-d for IGD devices (insecure)
  *   no-amd-iommu-perdev-intremap Don't use per-device interrupt remapping
  *                              tables (insecure)
+ *   inclusive                  Map additional regions into the IOMMU page
+ *                              tables in order to workaround bugs in ACPI
+ *                              tables.
  */
 custom_param("iommu", parse_iommu_param);
 bool_t __initdata iommu_enable = 1;
@@ -60,6 +63,7 @@ bool_t __read_mostly iommu_passthrough;
 bool_t __read_mostly iommu_snoop = 1;
 bool_t __read_mostly iommu_qinval = 1;
 bool_t __read_mostly iommu_intremap = 1;
+bool __hwdom_initdata iommu_inclusive = true;
 
 /*
  * In the current implementation of VT-d posted interrupts, in some extreme
@@ -208,6 +212,8 @@ void __hwdom_init iommu_hwdom_init(struct domain *d)
     }
 
     hd->platform_ops->hwdom_init(d);
+
+    arch_iommu_hwdom_init(d);
 }
 
 void iommu_teardown(struct domain *d)
diff --git a/xen/drivers/passthrough/vtd/extern.h b/xen/drivers/passthrough/vtd/extern.h
index fb7edfaef9..91cadc602e 100644
--- a/xen/drivers/passthrough/vtd/extern.h
+++ b/xen/drivers/passthrough/vtd/extern.h
@@ -99,6 +99,4 @@ void pci_vtd_quirk(const struct pci_dev *);
 bool_t platform_supports_intremap(void);
 bool_t platform_supports_x2apic(void);
 
-void vtd_set_hwdom_mapping(struct domain *d);
-
 #endif // _VTD_EXTERN_H_
diff --git a/xen/drivers/passthrough/vtd/iommu.c b/xen/drivers/passthrough/vtd/iommu.c
index 1710256823..569ec4aec2 100644
--- a/xen/drivers/passthrough/vtd/iommu.c
+++ b/xen/drivers/passthrough/vtd/iommu.c
@@ -1304,12 +1304,6 @@ static void __hwdom_init intel_iommu_hwdom_init(struct domain *d)
 {
     struct acpi_drhd_unit *drhd;
 
-    if ( !iommu_passthrough && is_pv_domain(d) )
-    {
-        /* Set up 1:1 page table for hardware domain. */
-        vtd_set_hwdom_mapping(d);
-    }
-
     setup_hwdom_pci_devices(d, setup_hwdom_device);
     setup_hwdom_rmrr(d);
 
diff --git a/xen/drivers/passthrough/vtd/x86/vtd.c b/xen/drivers/passthrough/vtd/x86/vtd.c
index cc2bfea162..55d74a97e2 100644
--- a/xen/drivers/passthrough/vtd/x86/vtd.c
+++ b/xen/drivers/passthrough/vtd/x86/vtd.c
@@ -35,8 +35,7 @@
  * iommu_inclusive_mapping: when set, all memory below 4GB is included in dom0
  * 1:1 iommu mappings except xen and unusable regions.
  */
-static bool_t __hwdom_initdata iommu_inclusive_mapping = 1;
-boolean_param("iommu_inclusive_mapping", iommu_inclusive_mapping);
+boolean_param("iommu_inclusive_mapping", iommu_inclusive);
 
 void *map_vtd_domain_page(u64 maddr)
 {
@@ -108,66 +107,3 @@ void hvm_dpci_isairq_eoi(struct domain *d, unsigned int isairq)
     spin_unlock(&d->event_lock);
 }
 
-void __hwdom_init vtd_set_hwdom_mapping(struct domain *d)
-{
-    unsigned long i, j, tmp, top, max_pfn;
-
-    BUG_ON(!is_hardware_domain(d));
-
-    max_pfn = (GB(4) >> PAGE_SHIFT) - 1;
-    top = max(max_pdx, pfn_to_pdx(max_pfn) + 1);
-
-    for ( i = 0; i < top; i++ )
-    {
-        unsigned long pfn = pdx_to_pfn(i);
-        bool map;
-        int rc = 0;
-
-        /*
-         * Set up 1:1 mapping for dom0. Default to include only
-         * conventional RAM areas and let RMRRs include needed reserved
-         * regions. When set, the inclusive mapping additionally maps in
-         * every pfn up to 4GB except those that fall in unusable ranges.
-         */
-        if ( pfn > max_pfn && !mfn_valid(_mfn(pfn)) )
-            continue;
-
-        if ( iommu_inclusive_mapping && pfn <= max_pfn )
-            map = !page_is_ram_type(pfn, RAM_TYPE_UNUSABLE);
-        else
-            map = page_is_ram_type(pfn, RAM_TYPE_CONVENTIONAL);
-
-        if ( !map )
-            continue;
-
-        /* Exclude Xen bits */
-        if ( xen_in_range(pfn) )
-            continue;
-
-        /*
-         * If dom0-strict mode is enabled then exclude conventional RAM
-         * and let the common code map dom0's pages.
-         */
-        if ( iommu_dom0_strict &&
-             page_is_ram_type(pfn, RAM_TYPE_CONVENTIONAL) )
-            continue;
-
-        tmp = 1 << (PAGE_SHIFT - PAGE_SHIFT_4K);
-        for ( j = 0; j < tmp; j++ )
-        {
-            int ret = iommu_map_page(d, pfn * tmp + j, pfn * tmp + j,
-                                     IOMMUF_readable|IOMMUF_writable);
-
-            if ( !rc )
-               rc = ret;
-        }
-
-        if ( rc )
-            printk(XENLOG_WARNING VTDPREFIX " d%d: IOMMU mapping failed: %d\n",
-                   d->domain_id, rc);
-
-        if (!(i & (0xfffff >> (PAGE_SHIFT - PAGE_SHIFT_4K))))
-            process_pending_softirqs();
-    }
-}
-
diff --git a/xen/drivers/passthrough/x86/iommu.c b/xen/drivers/passthrough/x86/iommu.c
index 68182afd91..ba0bbd9a15 100644
--- a/xen/drivers/passthrough/x86/iommu.c
+++ b/xen/drivers/passthrough/x86/iommu.c
@@ -20,6 +20,8 @@
 #include <xen/softirq.h>
 #include <xsm/xsm.h>
 
+#include <asm/setup.h>
+
 void iommu_update_ire_from_apic(
     unsigned int apic, unsigned int reg, unsigned int value)
 {
@@ -132,6 +134,74 @@ void arch_iommu_domain_destroy(struct domain *d)
 {
 }
 
+void __hwdom_init arch_iommu_hwdom_init(struct domain *d)
+{
+    unsigned long i, j, tmp, top, max_pfn;
+
+    if ( iommu_passthrough || !is_pv_domain(d) )
+        return;
+
+    BUG_ON(!is_hardware_domain(d));
+
+    max_pfn = (GB(4) >> PAGE_SHIFT) - 1;
+    top = max(max_pdx, pfn_to_pdx(max_pfn) + 1);
+
+    for ( i = 0; i < top; i++ )
+    {
+        unsigned long pfn = pdx_to_pfn(i);
+        bool map;
+        int rc = 0;
+
+        /*
+         * Set up 1:1 mapping for dom0. Default to include only
+         * conventional RAM areas and let RMRRs include needed reserved
+         * regions. When set, the inclusive mapping additionally maps in
+         * every pfn up to 4GB except those that fall in unusable ranges.
+         */
+        if ( pfn > max_pfn && !mfn_valid(_mfn(pfn)) )
+            continue;
+
+        if ( iommu_inclusive && pfn <= max_pfn )
+            map = !page_is_ram_type(pfn, RAM_TYPE_UNUSABLE);
+        else
+            map = page_is_ram_type(pfn, RAM_TYPE_CONVENTIONAL);
+
+        if ( !map )
+            continue;
+
+        /* Exclude Xen bits */
+        if ( xen_in_range(pfn) )
+            continue;
+
+        /*
+         * If dom0-strict mode is enabled then exclude conventional RAM
+         * and let the common code map dom0's pages.
+         */
+        if ( iommu_dom0_strict &&
+             page_is_ram_type(pfn, RAM_TYPE_CONVENTIONAL) )
+            continue;
+
+        tmp = 1 << (PAGE_SHIFT - PAGE_SHIFT_4K);
+        for ( j = 0; j < tmp; j++ )
+        {
+            int ret = iommu_map_page(d, pfn * tmp + j, pfn * tmp + j,
+                                     IOMMUF_readable|IOMMUF_writable);
+
+            if ( !rc )
+               rc = ret;
+        }
+
+        if ( rc )
+            printk(XENLOG_WARNING "d%d: IOMMU mapping failed: %d\n",
+                   d->domain_id, rc);
+
+        if (!(i & (0xfffff >> (PAGE_SHIFT - PAGE_SHIFT_4K))))
+            process_pending_softirqs();
+    }
+
+
+}
+
 /*
  * Local variables:
  * mode: C
diff --git a/xen/include/xen/iommu.h b/xen/include/xen/iommu.h
index 6b42e3b876..787566a4e7 100644
--- a/xen/include/xen/iommu.h
+++ b/xen/include/xen/iommu.h
@@ -35,6 +35,7 @@ extern bool_t iommu_snoop, iommu_qinval, iommu_intremap, iommu_intpost;
 extern bool_t iommu_hap_pt_share;
 extern bool_t iommu_debug;
 extern bool_t amd_iommu_perdev_intremap;
+extern bool iommu_inclusive;
 
 extern unsigned int iommu_dev_iotlb_timeout;
 
@@ -49,6 +50,7 @@ void arch_iommu_domain_destroy(struct domain *d);
 int arch_iommu_domain_init(struct domain *d);
 int arch_iommu_populate_page_table(struct domain *d);
 void arch_iommu_check_autotranslated_hwdom(struct domain *d);
+void arch_iommu_hwdom_init(struct domain *d);
 
 int iommu_construct(struct domain *d);
 
-- 
2.18.0


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

Re: [PATCH 2/4] iommu: generalize iommu_inclusive_mapping

[Paul Durrant]

> -----Original Message-----
> From: Xen-devel [mailto:xen-devel-bounces@lists.xenproject.org] On Behalf
> Of Roger Pau Monne
> Sent: 27 July 2018 16:32
> To: xen-devel@lists.xenproject.org
> Cc: Kevin Tian <kevin.tian@intel.com>; Stefano Stabellini
> <sstabellini@kernel.org>; Wei Liu <wei.liu2@citrix.com>; George Dunlap
> <George.Dunlap@citrix.com>; Andrew Cooper
> <Andrew.Cooper3@citrix.com>; Ian Jackson <Ian.Jackson@citrix.com>; Tim
> (Xen.org) <tim@xen.org>; Julien Grall <julien.grall@arm.com>; Jan Beulich
> <jbeulich@suse.com>; Roger Pau Monne <roger.pau@citrix.com>
> Subject: [Xen-devel] [PATCH 2/4] iommu: generalize
> iommu_inclusive_mapping
> 
> Introduce a new iommu=inclusive generic option that supersedes
> iommu_inclusive_mapping. This should be a non-functional change on
> Intel hardware, while AMD hardware will gain the same functionality of
> mapping almost everything below the 4GB boundary.
> 
> Note that is a noop for ARM hardware.
> 
> Signed-off-by: Roger Pau Monné <roger.pau@citrix.com>
> ---
> 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: Kevin Tian <kevin.tian@intel.com>
> ---
>  docs/misc/xen-command-line.markdown   | 14 ++++++
>  xen/drivers/passthrough/arm/iommu.c   |  4 ++
>  xen/drivers/passthrough/iommu.c       |  6 +++
>  xen/drivers/passthrough/vtd/extern.h  |  2 -
>  xen/drivers/passthrough/vtd/iommu.c   |  6 ---
>  xen/drivers/passthrough/vtd/x86/vtd.c | 66 +------------------------
>  xen/drivers/passthrough/x86/iommu.c   | 70
> +++++++++++++++++++++++++++
>  xen/include/xen/iommu.h               |  2 +
>  8 files changed, 97 insertions(+), 73 deletions(-)
> 
> diff --git a/docs/misc/xen-command-line.markdown b/docs/misc/xen-
> command-line.markdown
> index 65b4754418..91a8bfc9a6 100644
> --- a/docs/misc/xen-command-line.markdown
> +++ b/docs/misc/xen-command-line.markdown
> @@ -1198,6 +1198,17 @@ detection of systems known to misbehave upon
> accesses to that port.
> 
>  >> Enable IOMMU debugging code (implies `verbose`).
> 
> +> `inclusive`

This is a dom0 (or hwdom) specific setting so perhaps dom0-inclusive?

Actually the dom0 iommu options are starting to get unwieldy as they are conflated with the general host iommu options so I think it may be worthwhile splitting things out into a separate 'dom0-iommu=' top level parameter at this stage. (My reasons are slightly selfish as I intend to add another dom0 iommu option to give it just reserved regions, to avoid unnecessary set-up if we know it will be using PV-IOMMU).

Cheers,

  Paul

> +
> +> Default: `true`
> +
> +>> Use this to work around firmware issues providing incorrect RMRR or
> 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. Also note that if `dom0-strict` mode is enabled
> +>> then conventional RAM pages not assigned to dom0 will not be mapped.
> +
>  ### iommu\_dev\_iotlb\_timeout
>  > `= <integer>`
> 
> @@ -1212,6 +1223,9 @@ wait descriptor timed out', try increasing this value.
> 
>  > Default: `true`
> 
> +**WARNING: This command line option is deprecated, and superseded by
> +_iommu=inclusive_ - using both options in combination is undefined.**
> +
>  Use this to work around firmware issues providing incorrect RMRR 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
> 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/iommu.c
> b/xen/drivers/passthrough/iommu.c
> index 70d218f910..3f3aa71b2c 100644
> --- a/xen/drivers/passthrough/iommu.c
> +++ b/xen/drivers/passthrough/iommu.c
> @@ -47,6 +47,9 @@ integer_param("iommu_dev_iotlb_timeout",
> iommu_dev_iotlb_timeout);
>   *   no-igfx                    Disable VT-d for IGD devices (insecure)
>   *   no-amd-iommu-perdev-intremap Don't use per-device interrupt
> remapping
>   *                              tables (insecure)
> + *   inclusive                  Map additional regions into the IOMMU page
> + *                              tables in order to workaround bugs in ACPI
> + *                              tables.
>   */
>  custom_param("iommu", parse_iommu_param);
>  bool_t __initdata iommu_enable = 1;
> @@ -60,6 +63,7 @@ bool_t __read_mostly iommu_passthrough;
>  bool_t __read_mostly iommu_snoop = 1;
>  bool_t __read_mostly iommu_qinval = 1;
>  bool_t __read_mostly iommu_intremap = 1;
> +bool __hwdom_initdata iommu_inclusive = true;
> 
>  /*
>   * In the current implementation of VT-d posted interrupts, in some
> extreme
> @@ -208,6 +212,8 @@ void __hwdom_init iommu_hwdom_init(struct
> domain *d)
>      }
> 
>      hd->platform_ops->hwdom_init(d);
> +
> +    arch_iommu_hwdom_init(d);
>  }
> 
>  void iommu_teardown(struct domain *d)
> diff --git a/xen/drivers/passthrough/vtd/extern.h
> b/xen/drivers/passthrough/vtd/extern.h
> index fb7edfaef9..91cadc602e 100644
> --- a/xen/drivers/passthrough/vtd/extern.h
> +++ b/xen/drivers/passthrough/vtd/extern.h
> @@ -99,6 +99,4 @@ void pci_vtd_quirk(const struct pci_dev *);
>  bool_t platform_supports_intremap(void);
>  bool_t platform_supports_x2apic(void);
> 
> -void vtd_set_hwdom_mapping(struct domain *d);
> -
>  #endif // _VTD_EXTERN_H_
> diff --git a/xen/drivers/passthrough/vtd/iommu.c
> b/xen/drivers/passthrough/vtd/iommu.c
> index 1710256823..569ec4aec2 100644
> --- a/xen/drivers/passthrough/vtd/iommu.c
> +++ b/xen/drivers/passthrough/vtd/iommu.c
> @@ -1304,12 +1304,6 @@ static void __hwdom_init
> intel_iommu_hwdom_init(struct domain *d)
>  {
>      struct acpi_drhd_unit *drhd;
> 
> -    if ( !iommu_passthrough && is_pv_domain(d) )
> -    {
> -        /* Set up 1:1 page table for hardware domain. */
> -        vtd_set_hwdom_mapping(d);
> -    }
> -
>      setup_hwdom_pci_devices(d, setup_hwdom_device);
>      setup_hwdom_rmrr(d);
> 
> diff --git a/xen/drivers/passthrough/vtd/x86/vtd.c
> b/xen/drivers/passthrough/vtd/x86/vtd.c
> index cc2bfea162..55d74a97e2 100644
> --- a/xen/drivers/passthrough/vtd/x86/vtd.c
> +++ b/xen/drivers/passthrough/vtd/x86/vtd.c
> @@ -35,8 +35,7 @@
>   * iommu_inclusive_mapping: when set, all memory below 4GB is included in
> dom0
>   * 1:1 iommu mappings except xen and unusable regions.
>   */
> -static bool_t __hwdom_initdata iommu_inclusive_mapping = 1;
> -boolean_param("iommu_inclusive_mapping", iommu_inclusive_mapping);
> +boolean_param("iommu_inclusive_mapping", iommu_inclusive);
> 
>  void *map_vtd_domain_page(u64 maddr)
>  {
> @@ -108,66 +107,3 @@ void hvm_dpci_isairq_eoi(struct domain *d,
> unsigned int isairq)
>      spin_unlock(&d->event_lock);
>  }
> 
> -void __hwdom_init vtd_set_hwdom_mapping(struct domain *d)
> -{
> -    unsigned long i, j, tmp, top, max_pfn;
> -
> -    BUG_ON(!is_hardware_domain(d));
> -
> -    max_pfn = (GB(4) >> PAGE_SHIFT) - 1;
> -    top = max(max_pdx, pfn_to_pdx(max_pfn) + 1);
> -
> -    for ( i = 0; i < top; i++ )
> -    {
> -        unsigned long pfn = pdx_to_pfn(i);
> -        bool map;
> -        int rc = 0;
> -
> -        /*
> -         * Set up 1:1 mapping for dom0. Default to include only
> -         * conventional RAM areas and let RMRRs include needed reserved
> -         * regions. When set, the inclusive mapping additionally maps in
> -         * every pfn up to 4GB except those that fall in unusable ranges.
> -         */
> -        if ( pfn > max_pfn && !mfn_valid(_mfn(pfn)) )
> -            continue;
> -
> -        if ( iommu_inclusive_mapping && pfn <= max_pfn )
> -            map = !page_is_ram_type(pfn, RAM_TYPE_UNUSABLE);
> -        else
> -            map = page_is_ram_type(pfn, RAM_TYPE_CONVENTIONAL);
> -
> -        if ( !map )
> -            continue;
> -
> -        /* Exclude Xen bits */
> -        if ( xen_in_range(pfn) )
> -            continue;
> -
> -        /*
> -         * If dom0-strict mode is enabled then exclude conventional RAM
> -         * and let the common code map dom0's pages.
> -         */
> -        if ( iommu_dom0_strict &&
> -             page_is_ram_type(pfn, RAM_TYPE_CONVENTIONAL) )
> -            continue;
> -
> -        tmp = 1 << (PAGE_SHIFT - PAGE_SHIFT_4K);
> -        for ( j = 0; j < tmp; j++ )
> -        {
> -            int ret = iommu_map_page(d, pfn * tmp + j, pfn * tmp + j,
> -                                     IOMMUF_readable|IOMMUF_writable);
> -
> -            if ( !rc )
> -               rc = ret;
> -        }
> -
> -        if ( rc )
> -            printk(XENLOG_WARNING VTDPREFIX " d%d: IOMMU mapping failed:
> %d\n",
> -                   d->domain_id, rc);
> -
> -        if (!(i & (0xfffff >> (PAGE_SHIFT - PAGE_SHIFT_4K))))
> -            process_pending_softirqs();
> -    }
> -}
> -
> diff --git a/xen/drivers/passthrough/x86/iommu.c
> b/xen/drivers/passthrough/x86/iommu.c
> index 68182afd91..ba0bbd9a15 100644
> --- a/xen/drivers/passthrough/x86/iommu.c
> +++ b/xen/drivers/passthrough/x86/iommu.c
> @@ -20,6 +20,8 @@
>  #include <xen/softirq.h>
>  #include <xsm/xsm.h>
> 
> +#include <asm/setup.h>
> +
>  void iommu_update_ire_from_apic(
>      unsigned int apic, unsigned int reg, unsigned int value)
>  {
> @@ -132,6 +134,74 @@ void arch_iommu_domain_destroy(struct domain
> *d)
>  {
>  }
> 
> +void __hwdom_init arch_iommu_hwdom_init(struct domain *d)
> +{
> +    unsigned long i, j, tmp, top, max_pfn;
> +
> +    if ( iommu_passthrough || !is_pv_domain(d) )
> +        return;
> +
> +    BUG_ON(!is_hardware_domain(d));
> +
> +    max_pfn = (GB(4) >> PAGE_SHIFT) - 1;
> +    top = max(max_pdx, pfn_to_pdx(max_pfn) + 1);
> +
> +    for ( i = 0; i < top; i++ )
> +    {
> +        unsigned long pfn = pdx_to_pfn(i);
> +        bool map;
> +        int rc = 0;
> +
> +        /*
> +         * Set up 1:1 mapping for dom0. Default to include only
> +         * conventional RAM areas and let RMRRs include needed reserved
> +         * regions. When set, the inclusive mapping additionally maps in
> +         * every pfn up to 4GB except those that fall in unusable ranges.
> +         */
> +        if ( pfn > max_pfn && !mfn_valid(_mfn(pfn)) )
> +            continue;
> +
> +        if ( iommu_inclusive && pfn <= max_pfn )
> +            map = !page_is_ram_type(pfn, RAM_TYPE_UNUSABLE);
> +        else
> +            map = page_is_ram_type(pfn, RAM_TYPE_CONVENTIONAL);
> +
> +        if ( !map )
> +            continue;
> +
> +        /* Exclude Xen bits */
> +        if ( xen_in_range(pfn) )
> +            continue;
> +
> +        /*
> +         * If dom0-strict mode is enabled then exclude conventional RAM
> +         * and let the common code map dom0's pages.
> +         */
> +        if ( iommu_dom0_strict &&
> +             page_is_ram_type(pfn, RAM_TYPE_CONVENTIONAL) )
> +            continue;
> +
> +        tmp = 1 << (PAGE_SHIFT - PAGE_SHIFT_4K);
> +        for ( j = 0; j < tmp; j++ )
> +        {
> +            int ret = iommu_map_page(d, pfn * tmp + j, pfn * tmp + j,
> +                                     IOMMUF_readable|IOMMUF_writable);
> +
> +            if ( !rc )
> +               rc = ret;
> +        }
> +
> +        if ( rc )
> +            printk(XENLOG_WARNING "d%d: IOMMU mapping failed: %d\n",
> +                   d->domain_id, rc);
> +
> +        if (!(i & (0xfffff >> (PAGE_SHIFT - PAGE_SHIFT_4K))))
> +            process_pending_softirqs();
> +    }
> +
> +
> +}
> +
>  /*
>   * Local variables:
>   * mode: C
> diff --git a/xen/include/xen/iommu.h b/xen/include/xen/iommu.h
> index 6b42e3b876..787566a4e7 100644
> --- a/xen/include/xen/iommu.h
> +++ b/xen/include/xen/iommu.h
> @@ -35,6 +35,7 @@ extern bool_t iommu_snoop, iommu_qinval,
> iommu_intremap, iommu_intpost;
>  extern bool_t iommu_hap_pt_share;
>  extern bool_t iommu_debug;
>  extern bool_t amd_iommu_perdev_intremap;
> +extern bool iommu_inclusive;
> 
>  extern unsigned int iommu_dev_iotlb_timeout;
> 
> @@ -49,6 +50,7 @@ void arch_iommu_domain_destroy(struct domain *d);
>  int arch_iommu_domain_init(struct domain *d);
>  int arch_iommu_populate_page_table(struct domain *d);
>  void arch_iommu_check_autotranslated_hwdom(struct domain *d);
> +void arch_iommu_hwdom_init(struct domain *d);
> 
>  int iommu_construct(struct domain *d);
> 
> --
> 2.18.0
> 
> 
> _______________________________________________
> Xen-devel mailing list
> Xen-devel@lists.xenproject.org
> https://lists.xenproject.org/mailman/listinfo/xen-devel
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel

Re: [PATCH 2/4] iommu: generalize iommu_inclusive_mapping

[Roger Pau Monné]

On Tue, Jul 31, 2018 at 08:18:36AM +0100, Paul Durrant wrote:
> > -----Original Message-----
> > From: Xen-devel [mailto:xen-devel-bounces@lists.xenproject.org] On Behalf
> > Of Roger Pau Monne
> > Sent: 27 July 2018 16:32
> > To: xen-devel@lists.xenproject.org
> > Cc: Kevin Tian <kevin.tian@intel.com>; Stefano Stabellini
> > <sstabellini@kernel.org>; Wei Liu <wei.liu2@citrix.com>; George Dunlap
> > <George.Dunlap@citrix.com>; Andrew Cooper
> > <Andrew.Cooper3@citrix.com>; Ian Jackson <Ian.Jackson@citrix.com>; Tim
> > (Xen.org) <tim@xen.org>; Julien Grall <julien.grall@arm.com>; Jan Beulich
> > <jbeulich@suse.com>; Roger Pau Monne <roger.pau@citrix.com>
> > Subject: [Xen-devel] [PATCH 2/4] iommu: generalize
> > iommu_inclusive_mapping
> > 
> > Introduce a new iommu=inclusive generic option that supersedes
> > iommu_inclusive_mapping. This should be a non-functional change on
> > Intel hardware, while AMD hardware will gain the same functionality of
> > mapping almost everything below the 4GB boundary.
> > 
> > Note that is a noop for ARM hardware.
> > 
> > Signed-off-by: Roger Pau Monné <roger.pau@citrix.com>
> > ---
> > 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: Kevin Tian <kevin.tian@intel.com>
> > ---
> >  docs/misc/xen-command-line.markdown   | 14 ++++++
> >  xen/drivers/passthrough/arm/iommu.c   |  4 ++
> >  xen/drivers/passthrough/iommu.c       |  6 +++
> >  xen/drivers/passthrough/vtd/extern.h  |  2 -
> >  xen/drivers/passthrough/vtd/iommu.c   |  6 ---
> >  xen/drivers/passthrough/vtd/x86/vtd.c | 66 +------------------------
> >  xen/drivers/passthrough/x86/iommu.c   | 70
> > +++++++++++++++++++++++++++
> >  xen/include/xen/iommu.h               |  2 +
> >  8 files changed, 97 insertions(+), 73 deletions(-)
> > 
> > diff --git a/docs/misc/xen-command-line.markdown b/docs/misc/xen-
> > command-line.markdown
> > index 65b4754418..91a8bfc9a6 100644
> > --- a/docs/misc/xen-command-line.markdown
> > +++ b/docs/misc/xen-command-line.markdown
> > @@ -1198,6 +1198,17 @@ detection of systems known to misbehave upon
> > accesses to that port.
> > 
> >  >> Enable IOMMU debugging code (implies `verbose`).
> > 
> > +> `inclusive`
> 
> This is a dom0 (or hwdom) specific setting so perhaps dom0-inclusive?
> 
> Actually the dom0 iommu options are starting to get unwieldy as they are conflated with the general host iommu options so I think it may be worthwhile splitting things out into a separate 'dom0-iommu=' top level parameter at this stage. (My reasons are slightly selfish as I intend to add another dom0 iommu option to give it just reserved regions, to avoid unnecessary set-up if we know it will be using PV-IOMMU).

Mapping just the reserved regions is what I actually do for PVH with
iommu=inclusive (patch 4/4), so maybe it would make sense to speak about the
naming here in order to use the same naming for PV and PVH.

TBH I don't really like the dom0- prefix, the command line iommu
options either apply to all domains or Dom0 only, having
domu-inclusive for example makes no sense IMO.

Maybe the current iommu_inclusive_mapping could be mapped to
iommu=inclusive-mapping instead of iommu=inclusive and the new option
could be added as iommu=reserved-mapping?

That would work for PVH AFAICT.

Thanks, Roger.

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

Re: [PATCH 2/4] iommu: generalize iommu_inclusive_mapping

[Paul Durrant]

> -----Original Message-----
> From: Roger Pau Monne
> Sent: 31 July 2018 09:16
> To: Paul Durrant <Paul.Durrant@citrix.com>
> Cc: xen-devel@lists.xenproject.org; Kevin Tian <kevin.tian@intel.com>;
> Stefano Stabellini <sstabellini@kernel.org>; Wei Liu <wei.liu2@citrix.com>;
> George Dunlap <George.Dunlap@citrix.com>; Andrew Cooper
> <Andrew.Cooper3@citrix.com>; Ian Jackson <Ian.Jackson@citrix.com>; Tim
> (Xen.org) <tim@xen.org>; Julien Grall <julien.grall@arm.com>; Jan Beulich
> <jbeulich@suse.com>
> Subject: Re: [Xen-devel] [PATCH 2/4] iommu: generalize
> iommu_inclusive_mapping
> 
> On Tue, Jul 31, 2018 at 08:18:36AM +0100, Paul Durrant wrote:
> > > -----Original Message-----
> > > From: Xen-devel [mailto:xen-devel-bounces@lists.xenproject.org] On
> Behalf
> > > Of Roger Pau Monne
> > > Sent: 27 July 2018 16:32
> > > To: xen-devel@lists.xenproject.org
> > > Cc: Kevin Tian <kevin.tian@intel.com>; Stefano Stabellini
> > > <sstabellini@kernel.org>; Wei Liu <wei.liu2@citrix.com>; George Dunlap
> > > <George.Dunlap@citrix.com>; Andrew Cooper
> > > <Andrew.Cooper3@citrix.com>; Ian Jackson <Ian.Jackson@citrix.com>;
> Tim
> > > (Xen.org) <tim@xen.org>; Julien Grall <julien.grall@arm.com>; Jan
> Beulich
> > > <jbeulich@suse.com>; Roger Pau Monne <roger.pau@citrix.com>
> > > Subject: [Xen-devel] [PATCH 2/4] iommu: generalize
> > > iommu_inclusive_mapping
> > >
> > > Introduce a new iommu=inclusive generic option that supersedes
> > > iommu_inclusive_mapping. This should be a non-functional change on
> > > Intel hardware, while AMD hardware will gain the same functionality of
> > > mapping almost everything below the 4GB boundary.
> > >
> > > Note that is a noop for ARM hardware.
> > >
> > > Signed-off-by: Roger Pau Monné <roger.pau@citrix.com>
> > > ---
> > > 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: Kevin Tian <kevin.tian@intel.com>
> > > ---
> > >  docs/misc/xen-command-line.markdown   | 14 ++++++
> > >  xen/drivers/passthrough/arm/iommu.c   |  4 ++
> > >  xen/drivers/passthrough/iommu.c       |  6 +++
> > >  xen/drivers/passthrough/vtd/extern.h  |  2 -
> > >  xen/drivers/passthrough/vtd/iommu.c   |  6 ---
> > >  xen/drivers/passthrough/vtd/x86/vtd.c | 66 +------------------------
> > >  xen/drivers/passthrough/x86/iommu.c   | 70
> > > +++++++++++++++++++++++++++
> > >  xen/include/xen/iommu.h               |  2 +
> > >  8 files changed, 97 insertions(+), 73 deletions(-)
> > >
> > > diff --git a/docs/misc/xen-command-line.markdown b/docs/misc/xen-
> > > command-line.markdown
> > > index 65b4754418..91a8bfc9a6 100644
> > > --- a/docs/misc/xen-command-line.markdown
> > > +++ b/docs/misc/xen-command-line.markdown
> > > @@ -1198,6 +1198,17 @@ detection of systems known to misbehave
> upon
> > > accesses to that port.
> > >
> > >  >> Enable IOMMU debugging code (implies `verbose`).
> > >
> > > +> `inclusive`
> >
> > This is a dom0 (or hwdom) specific setting so perhaps dom0-inclusive?
> >
> > Actually the dom0 iommu options are starting to get unwieldy as they are
> conflated with the general host iommu options so I think it may be
> worthwhile splitting things out into a separate 'dom0-iommu=' top level
> parameter at this stage. (My reasons are slightly selfish as I intend to add
> another dom0 iommu option to give it just reserved regions, to avoid
> unnecessary set-up if we know it will be using PV-IOMMU).
> 
> Mapping just the reserved regions is what I actually do for PVH with
> iommu=inclusive (patch 4/4), so maybe it would make sense to speak about
> the
> naming here in order to use the same naming for PV and PVH.
> 
> TBH I don't really like the dom0- prefix, the command line iommu
> options either apply to all domains or Dom0 only, having
> domu-inclusive for example makes no sense IMO.

No, I think there are some options that you may want to apply to dom0 only, but these are more like the dom0_mem or dom0_max_vpus options. Particularly, the inclusive option is probably something that is only desirable for dom0. Clearly dom0-passthrough and dom0-strict are already defined to relate to dom0 only, and options such as 'reserved' should only be specific on the command line in relation to dom0 IMO. For other domains such an option should be specified via xl.cfg.

  Paul

> 
> Maybe the current iommu_inclusive_mapping could be mapped to
> iommu=inclusive-mapping instead of iommu=inclusive and the new option
> could be added as iommu=reserved-mapping?
> 
> That would work for PVH AFAICT.
> 
> Thanks, Roger.

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

Re: [PATCH 2/4] iommu: generalize iommu_inclusive_mapping

[Roger Pau Monné]

On Tue, Jul 31, 2018 at 09:27:03AM +0100, Paul Durrant wrote:
> > -----Original Message-----
> > From: Roger Pau Monne
> > Sent: 31 July 2018 09:16
> > To: Paul Durrant <Paul.Durrant@citrix.com>
> > Cc: xen-devel@lists.xenproject.org; Kevin Tian <kevin.tian@intel.com>;
> > Stefano Stabellini <sstabellini@kernel.org>; Wei Liu <wei.liu2@citrix.com>;
> > George Dunlap <George.Dunlap@citrix.com>; Andrew Cooper
> > <Andrew.Cooper3@citrix.com>; Ian Jackson <Ian.Jackson@citrix.com>; Tim
> > (Xen.org) <tim@xen.org>; Julien Grall <julien.grall@arm.com>; Jan Beulich
> > <jbeulich@suse.com>
> > Subject: Re: [Xen-devel] [PATCH 2/4] iommu: generalize
> > iommu_inclusive_mapping
> > 
> > On Tue, Jul 31, 2018 at 08:18:36AM +0100, Paul Durrant wrote:
> > > > -----Original Message-----
> > > > From: Xen-devel [mailto:xen-devel-bounces@lists.xenproject.org] On
> > Behalf
> > > > Of Roger Pau Monne
> > > > Sent: 27 July 2018 16:32
> > > > To: xen-devel@lists.xenproject.org
> > > > Cc: Kevin Tian <kevin.tian@intel.com>; Stefano Stabellini
> > > > <sstabellini@kernel.org>; Wei Liu <wei.liu2@citrix.com>; George Dunlap
> > > > <George.Dunlap@citrix.com>; Andrew Cooper
> > > > <Andrew.Cooper3@citrix.com>; Ian Jackson <Ian.Jackson@citrix.com>;
> > Tim
> > > > (Xen.org) <tim@xen.org>; Julien Grall <julien.grall@arm.com>; Jan
> > Beulich
> > > > <jbeulich@suse.com>; Roger Pau Monne <roger.pau@citrix.com>
> > > > Subject: [Xen-devel] [PATCH 2/4] iommu: generalize
> > > > iommu_inclusive_mapping
> > > >
> > > > Introduce a new iommu=inclusive generic option that supersedes
> > > > iommu_inclusive_mapping. This should be a non-functional change on
> > > > Intel hardware, while AMD hardware will gain the same functionality of
> > > > mapping almost everything below the 4GB boundary.
> > > >
> > > > Note that is a noop for ARM hardware.
> > > >
> > > > Signed-off-by: Roger Pau Monné <roger.pau@citrix.com>
> > > > ---
> > > > 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: Kevin Tian <kevin.tian@intel.com>
> > > > ---
> > > >  docs/misc/xen-command-line.markdown   | 14 ++++++
> > > >  xen/drivers/passthrough/arm/iommu.c   |  4 ++
> > > >  xen/drivers/passthrough/iommu.c       |  6 +++
> > > >  xen/drivers/passthrough/vtd/extern.h  |  2 -
> > > >  xen/drivers/passthrough/vtd/iommu.c   |  6 ---
> > > >  xen/drivers/passthrough/vtd/x86/vtd.c | 66 +------------------------
> > > >  xen/drivers/passthrough/x86/iommu.c   | 70
> > > > +++++++++++++++++++++++++++
> > > >  xen/include/xen/iommu.h               |  2 +
> > > >  8 files changed, 97 insertions(+), 73 deletions(-)
> > > >
> > > > diff --git a/docs/misc/xen-command-line.markdown b/docs/misc/xen-
> > > > command-line.markdown
> > > > index 65b4754418..91a8bfc9a6 100644
> > > > --- a/docs/misc/xen-command-line.markdown
> > > > +++ b/docs/misc/xen-command-line.markdown
> > > > @@ -1198,6 +1198,17 @@ detection of systems known to misbehave
> > upon
> > > > accesses to that port.
> > > >
> > > >  >> Enable IOMMU debugging code (implies `verbose`).
> > > >
> > > > +> `inclusive`
> > >
> > > This is a dom0 (or hwdom) specific setting so perhaps dom0-inclusive?
> > >
> > > Actually the dom0 iommu options are starting to get unwieldy as they are
> > conflated with the general host iommu options so I think it may be
> > worthwhile splitting things out into a separate 'dom0-iommu=' top level
> > parameter at this stage. (My reasons are slightly selfish as I intend to add
> > another dom0 iommu option to give it just reserved regions, to avoid
> > unnecessary set-up if we know it will be using PV-IOMMU).
> > 
> > Mapping just the reserved regions is what I actually do for PVH with
> > iommu=inclusive (patch 4/4), so maybe it would make sense to speak about
> > the
> > naming here in order to use the same naming for PV and PVH.
> > 
> > TBH I don't really like the dom0- prefix, the command line iommu
> > options either apply to all domains or Dom0 only, having
> > domu-inclusive for example makes no sense IMO.
> 
> No, I think there are some options that you may want to apply to dom0 only, but these are more like the dom0_mem or dom0_max_vpus options. Particularly, the inclusive option is probably something that is only desirable for dom0. Clearly dom0-passthrough and dom0-strict are already defined to relate to dom0 only, and options such as 'reserved' should only be specific on the command line in relation to dom0 IMO. For other domains such an option should be specified via xl.cfg.

Yes, we already have a bunch of those, so then I think dom0-inclusive
and dom0-reserved would be appropriate?

dom0-inclusive-mapping or dom0-reserved-mapping seems too long.

Thanks, Roger.

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

Re: [PATCH 2/4] iommu: generalize iommu_inclusive_mapping

[Paul Durrant]

> -----Original Message-----
> From: Roger Pau Monne
> Sent: 31 July 2018 09:34
> To: Paul Durrant <Paul.Durrant@citrix.com>
> Cc: xen-devel@lists.xenproject.org; Kevin Tian <kevin.tian@intel.com>;
> Stefano Stabellini <sstabellini@kernel.org>; Wei Liu <wei.liu2@citrix.com>;
> George Dunlap <George.Dunlap@citrix.com>; Andrew Cooper
> <Andrew.Cooper3@citrix.com>; Ian Jackson <Ian.Jackson@citrix.com>; Tim
> (Xen.org) <tim@xen.org>; Julien Grall <julien.grall@arm.com>; Jan Beulich
> <jbeulich@suse.com>
> Subject: Re: [Xen-devel] [PATCH 2/4] iommu: generalize
> iommu_inclusive_mapping
> 
> On Tue, Jul 31, 2018 at 09:27:03AM +0100, Paul Durrant wrote:
> > > -----Original Message-----
> > > From: Roger Pau Monne
> > > Sent: 31 July 2018 09:16
> > > To: Paul Durrant <Paul.Durrant@citrix.com>
> > > Cc: xen-devel@lists.xenproject.org; Kevin Tian <kevin.tian@intel.com>;
> > > Stefano Stabellini <sstabellini@kernel.org>; Wei Liu
> <wei.liu2@citrix.com>;
> > > George Dunlap <George.Dunlap@citrix.com>; Andrew Cooper
> > > <Andrew.Cooper3@citrix.com>; Ian Jackson <Ian.Jackson@citrix.com>;
> Tim
> > > (Xen.org) <tim@xen.org>; Julien Grall <julien.grall@arm.com>; Jan
> Beulich
> > > <jbeulich@suse.com>
> > > Subject: Re: [Xen-devel] [PATCH 2/4] iommu: generalize
> > > iommu_inclusive_mapping
> > >
> > > On Tue, Jul 31, 2018 at 08:18:36AM +0100, Paul Durrant wrote:
> > > > > -----Original Message-----
> > > > > From: Xen-devel [mailto:xen-devel-bounces@lists.xenproject.org]
> On
> > > Behalf
> > > > > Of Roger Pau Monne
> > > > > Sent: 27 July 2018 16:32
> > > > > To: xen-devel@lists.xenproject.org
> > > > > Cc: Kevin Tian <kevin.tian@intel.com>; Stefano Stabellini
> > > > > <sstabellini@kernel.org>; Wei Liu <wei.liu2@citrix.com>; George
> Dunlap
> > > > > <George.Dunlap@citrix.com>; Andrew Cooper
> > > > > <Andrew.Cooper3@citrix.com>; Ian Jackson
> <Ian.Jackson@citrix.com>;
> > > Tim
> > > > > (Xen.org) <tim@xen.org>; Julien Grall <julien.grall@arm.com>; Jan
> > > Beulich
> > > > > <jbeulich@suse.com>; Roger Pau Monne <roger.pau@citrix.com>
> > > > > Subject: [Xen-devel] [PATCH 2/4] iommu: generalize
> > > > > iommu_inclusive_mapping
> > > > >
> > > > > Introduce a new iommu=inclusive generic option that supersedes
> > > > > iommu_inclusive_mapping. This should be a non-functional change
> on
> > > > > Intel hardware, while AMD hardware will gain the same functionality
> of
> > > > > mapping almost everything below the 4GB boundary.
> > > > >
> > > > > Note that is a noop for ARM hardware.
> > > > >
> > > > > Signed-off-by: Roger Pau Monné <roger.pau@citrix.com>
> > > > > ---
> > > > > 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: Kevin Tian <kevin.tian@intel.com>
> > > > > ---
> > > > >  docs/misc/xen-command-line.markdown   | 14 ++++++
> > > > >  xen/drivers/passthrough/arm/iommu.c   |  4 ++
> > > > >  xen/drivers/passthrough/iommu.c       |  6 +++
> > > > >  xen/drivers/passthrough/vtd/extern.h  |  2 -
> > > > >  xen/drivers/passthrough/vtd/iommu.c   |  6 ---
> > > > >  xen/drivers/passthrough/vtd/x86/vtd.c | 66 +------------------------
> > > > >  xen/drivers/passthrough/x86/iommu.c   | 70
> > > > > +++++++++++++++++++++++++++
> > > > >  xen/include/xen/iommu.h               |  2 +
> > > > >  8 files changed, 97 insertions(+), 73 deletions(-)
> > > > >
> > > > > diff --git a/docs/misc/xen-command-line.markdown
> b/docs/misc/xen-
> > > > > command-line.markdown
> > > > > index 65b4754418..91a8bfc9a6 100644
> > > > > --- a/docs/misc/xen-command-line.markdown
> > > > > +++ b/docs/misc/xen-command-line.markdown
> > > > > @@ -1198,6 +1198,17 @@ detection of systems known to misbehave
> > > upon
> > > > > accesses to that port.
> > > > >
> > > > >  >> Enable IOMMU debugging code (implies `verbose`).
> > > > >
> > > > > +> `inclusive`
> > > >
> > > > This is a dom0 (or hwdom) specific setting so perhaps dom0-inclusive?
> > > >
> > > > Actually the dom0 iommu options are starting to get unwieldy as they
> are
> > > conflated with the general host iommu options so I think it may be
> > > worthwhile splitting things out into a separate 'dom0-iommu=' top level
> > > parameter at this stage. (My reasons are slightly selfish as I intend to add
> > > another dom0 iommu option to give it just reserved regions, to avoid
> > > unnecessary set-up if we know it will be using PV-IOMMU).
> > >
> > > Mapping just the reserved regions is what I actually do for PVH with
> > > iommu=inclusive (patch 4/4), so maybe it would make sense to speak
> about
> > > the
> > > naming here in order to use the same naming for PV and PVH.
> > >
> > > TBH I don't really like the dom0- prefix, the command line iommu
> > > options either apply to all domains or Dom0 only, having
> > > domu-inclusive for example makes no sense IMO.
> >
> > No, I think there are some options that you may want to apply to dom0
> only, but these are more like the dom0_mem or dom0_max_vpus options.
> Particularly, the inclusive option is probably something that is only desirable
> for dom0. Clearly dom0-passthrough and dom0-strict are already defined to
> relate to dom0 only, and options such as 'reserved' should only be specific on
> the command line in relation to dom0 IMO. For other domains such an option
> should be specified via xl.cfg.
> 
> Yes, we already have a bunch of those, so then I think dom0-inclusive
> and dom0-reserved would be appropriate?
> 
> dom0-inclusive-mapping or dom0-reserved-mapping seems too long.

Yes, those names are ok, but I still think it better in the long run if we have something like:

dom0_iommu=[inclusive,][reserved,][strict,][none,][relaxed]

where relaxed is the default and 'none' (I think) is equivalent to the current iommu=dom0-passthrough.

  Paul

> 
> Thanks, Roger.

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

Re: [PATCH 2/4] iommu: generalize iommu_inclusive_mapping

[Jan Beulich]

>>> On 31.07.18 at 10:37, <Paul.Durrant@citrix.com> wrote:
>>  -----Original Message-----
>> From: Roger Pau Monne
>> Sent: 31 July 2018 09:34
>> To: Paul Durrant <Paul.Durrant@citrix.com>
>> Cc: xen-devel@lists.xenproject.org; Kevin Tian <kevin.tian@intel.com>;
>> Stefano Stabellini <sstabellini@kernel.org>; Wei Liu <wei.liu2@citrix.com>;
>> George Dunlap <George.Dunlap@citrix.com>; Andrew Cooper
>> <Andrew.Cooper3@citrix.com>; Ian Jackson <Ian.Jackson@citrix.com>; Tim
>> (Xen.org) <tim@xen.org>; Julien Grall <julien.grall@arm.com>; Jan Beulich
>> <jbeulich@suse.com>
>> Subject: Re: [Xen-devel] [PATCH 2/4] iommu: generalize
>> iommu_inclusive_mapping
>> 
>> On Tue, Jul 31, 2018 at 09:27:03AM +0100, Paul Durrant wrote:
>> > > -----Original Message-----
>> > > From: Roger Pau Monne
>> > > Sent: 31 July 2018 09:16
>> > > To: Paul Durrant <Paul.Durrant@citrix.com>
>> > > Cc: xen-devel@lists.xenproject.org; Kevin Tian <kevin.tian@intel.com>;
>> > > Stefano Stabellini <sstabellini@kernel.org>; Wei Liu
>> <wei.liu2@citrix.com>;
>> > > George Dunlap <George.Dunlap@citrix.com>; Andrew Cooper
>> > > <Andrew.Cooper3@citrix.com>; Ian Jackson <Ian.Jackson@citrix.com>;
>> Tim
>> > > (Xen.org) <tim@xen.org>; Julien Grall <julien.grall@arm.com>; Jan
>> Beulich
>> > > <jbeulich@suse.com>
>> > > Subject: Re: [Xen-devel] [PATCH 2/4] iommu: generalize
>> > > iommu_inclusive_mapping
>> > >
>> > > On Tue, Jul 31, 2018 at 08:18:36AM +0100, Paul Durrant wrote:
>> > > > > -----Original Message-----
>> > > > > From: Xen-devel [mailto:xen-devel-bounces@lists.xenproject.org]
>> On
>> > > Behalf
>> > > > > Of Roger Pau Monne
>> > > > > Sent: 27 July 2018 16:32
>> > > > > To: xen-devel@lists.xenproject.org 
>> > > > > Cc: Kevin Tian <kevin.tian@intel.com>; Stefano Stabellini
>> > > > > <sstabellini@kernel.org>; Wei Liu <wei.liu2@citrix.com>; George
>> Dunlap
>> > > > > <George.Dunlap@citrix.com>; Andrew Cooper
>> > > > > <Andrew.Cooper3@citrix.com>; Ian Jackson
>> <Ian.Jackson@citrix.com>;
>> > > Tim
>> > > > > (Xen.org) <tim@xen.org>; Julien Grall <julien.grall@arm.com>; Jan
>> > > Beulich
>> > > > > <jbeulich@suse.com>; Roger Pau Monne <roger.pau@citrix.com>
>> > > > > Subject: [Xen-devel] [PATCH 2/4] iommu: generalize
>> > > > > iommu_inclusive_mapping
>> > > > >
>> > > > > Introduce a new iommu=inclusive generic option that supersedes
>> > > > > iommu_inclusive_mapping. This should be a non-functional change
>> on
>> > > > > Intel hardware, while AMD hardware will gain the same functionality
>> of
>> > > > > mapping almost everything below the 4GB boundary.
>> > > > >
>> > > > > Note that is a noop for ARM hardware.
>> > > > >
>> > > > > Signed-off-by: Roger Pau Monné <roger.pau@citrix.com>
>> > > > > ---
>> > > > > 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: Kevin Tian <kevin.tian@intel.com>
>> > > > > ---
>> > > > >  docs/misc/xen-command-line.markdown   | 14 ++++++
>> > > > >  xen/drivers/passthrough/arm/iommu.c   |  4 ++
>> > > > >  xen/drivers/passthrough/iommu.c       |  6 +++
>> > > > >  xen/drivers/passthrough/vtd/extern.h  |  2 -
>> > > > >  xen/drivers/passthrough/vtd/iommu.c   |  6 ---
>> > > > >  xen/drivers/passthrough/vtd/x86/vtd.c | 66 +------------------------
>> > > > >  xen/drivers/passthrough/x86/iommu.c   | 70
>> > > > > +++++++++++++++++++++++++++
>> > > > >  xen/include/xen/iommu.h               |  2 +
>> > > > >  8 files changed, 97 insertions(+), 73 deletions(-)
>> > > > >
>> > > > > diff --git a/docs/misc/xen-command-line.markdown
>> b/docs/misc/xen-
>> > > > > command-line.markdown
>> > > > > index 65b4754418..91a8bfc9a6 100644
>> > > > > --- a/docs/misc/xen-command-line.markdown
>> > > > > +++ b/docs/misc/xen-command-line.markdown
>> > > > > @@ -1198,6 +1198,17 @@ detection of systems known to misbehave
>> > > upon
>> > > > > accesses to that port.
>> > > > >
>> > > > >  >> Enable IOMMU debugging code (implies `verbose`).
>> > > > >
>> > > > > +> `inclusive`
>> > > >
>> > > > This is a dom0 (or hwdom) specific setting so perhaps dom0-inclusive?
>> > > >
>> > > > Actually the dom0 iommu options are starting to get unwieldy as they
>> are
>> > > conflated with the general host iommu options so I think it may be
>> > > worthwhile splitting things out into a separate 'dom0-iommu=' top level
>> > > parameter at this stage. (My reasons are slightly selfish as I intend to 
> add
>> > > another dom0 iommu option to give it just reserved regions, to avoid
>> > > unnecessary set-up if we know it will be using PV-IOMMU).
>> > >
>> > > Mapping just the reserved regions is what I actually do for PVH with
>> > > iommu=inclusive (patch 4/4), so maybe it would make sense to speak
>> about
>> > > the
>> > > naming here in order to use the same naming for PV and PVH.
>> > >
>> > > TBH I don't really like the dom0- prefix, the command line iommu
>> > > options either apply to all domains or Dom0 only, having
>> > > domu-inclusive for example makes no sense IMO.
>> >
>> > No, I think there are some options that you may want to apply to dom0
>> only, but these are more like the dom0_mem or dom0_max_vpus options.
>> Particularly, the inclusive option is probably something that is only desirable
>> for dom0. Clearly dom0-passthrough and dom0-strict are already defined to
>> relate to dom0 only, and options such as 'reserved' should only be specific on
>> the command line in relation to dom0 IMO. For other domains such an option
>> should be specified via xl.cfg.
>> 
>> Yes, we already have a bunch of those, so then I think dom0-inclusive
>> and dom0-reserved would be appropriate?
>> 
>> dom0-inclusive-mapping or dom0-reserved-mapping seems too long.
> 
> Yes, those names are ok, but I still think it better in the long run if we 
> have something like:
> 
> dom0_iommu=[inclusive,][reserved,][strict,][none,][relaxed]
> 
> where relaxed is the default and 'none' (I think) is equivalent to the 
> current iommu=dom0-passthrough.

Or, along the lines of the other reply just sent, e.g.

dom0=pvh,iommu:inclusive;reserved,shadow

But perhaps the difference between , and ; gets too confusing then.

Jan


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

Re: [PATCH 2/4] iommu: generalize iommu_inclusive_mapping

[Roger Pau Monné]

On Tue, Jul 31, 2018 at 02:49:19AM -0600, Jan Beulich wrote:
> >>> On 31.07.18 at 10:37, <Paul.Durrant@citrix.com> wrote:
> >>  -----Original Message-----
> >> From: Roger Pau Monne
> >> Sent: 31 July 2018 09:34
> >> To: Paul Durrant <Paul.Durrant@citrix.com>
> >> Cc: xen-devel@lists.xenproject.org; Kevin Tian <kevin.tian@intel.com>;
> >> Stefano Stabellini <sstabellini@kernel.org>; Wei Liu <wei.liu2@citrix.com>;
> >> George Dunlap <George.Dunlap@citrix.com>; Andrew Cooper
> >> <Andrew.Cooper3@citrix.com>; Ian Jackson <Ian.Jackson@citrix.com>; Tim
> >> (Xen.org) <tim@xen.org>; Julien Grall <julien.grall@arm.com>; Jan Beulich
> >> <jbeulich@suse.com>
> >> Subject: Re: [Xen-devel] [PATCH 2/4] iommu: generalize
> >> iommu_inclusive_mapping
> >> 
> >> On Tue, Jul 31, 2018 at 09:27:03AM +0100, Paul Durrant wrote:
> >> > > -----Original Message-----
> >> > > From: Roger Pau Monne
> >> > > Sent: 31 July 2018 09:16
> >> > > To: Paul Durrant <Paul.Durrant@citrix.com>
> >> > > Cc: xen-devel@lists.xenproject.org; Kevin Tian <kevin.tian@intel.com>;
> >> > > Stefano Stabellini <sstabellini@kernel.org>; Wei Liu
> >> <wei.liu2@citrix.com>;
> >> > > George Dunlap <George.Dunlap@citrix.com>; Andrew Cooper
> >> > > <Andrew.Cooper3@citrix.com>; Ian Jackson <Ian.Jackson@citrix.com>;
> >> Tim
> >> > > (Xen.org) <tim@xen.org>; Julien Grall <julien.grall@arm.com>; Jan
> >> Beulich
> >> > > <jbeulich@suse.com>
> >> > > Subject: Re: [Xen-devel] [PATCH 2/4] iommu: generalize
> >> > > iommu_inclusive_mapping
> >> > >
> >> > > On Tue, Jul 31, 2018 at 08:18:36AM +0100, Paul Durrant wrote:
> >> > > > > -----Original Message-----
> >> > > > > From: Xen-devel [mailto:xen-devel-bounces@lists.xenproject.org]
> >> On
> >> > > Behalf
> >> > > > > Of Roger Pau Monne
> >> > > > > Sent: 27 July 2018 16:32
> >> > > > > To: xen-devel@lists.xenproject.org 
> >> > > > > Cc: Kevin Tian <kevin.tian@intel.com>; Stefano Stabellini
> >> > > > > <sstabellini@kernel.org>; Wei Liu <wei.liu2@citrix.com>; George
> >> Dunlap
> >> > > > > <George.Dunlap@citrix.com>; Andrew Cooper
> >> > > > > <Andrew.Cooper3@citrix.com>; Ian Jackson
> >> <Ian.Jackson@citrix.com>;
> >> > > Tim
> >> > > > > (Xen.org) <tim@xen.org>; Julien Grall <julien.grall@arm.com>; Jan
> >> > > Beulich
> >> > > > > <jbeulich@suse.com>; Roger Pau Monne <roger.pau@citrix.com>
> >> > > > > Subject: [Xen-devel] [PATCH 2/4] iommu: generalize
> >> > > > > iommu_inclusive_mapping
> >> > > > >
> >> > > > > Introduce a new iommu=inclusive generic option that supersedes
> >> > > > > iommu_inclusive_mapping. This should be a non-functional change
> >> on
> >> > > > > Intel hardware, while AMD hardware will gain the same functionality
> >> of
> >> > > > > mapping almost everything below the 4GB boundary.
> >> > > > >
> >> > > > > Note that is a noop for ARM hardware.
> >> > > > >
> >> > > > > Signed-off-by: Roger Pau Monné <roger.pau@citrix.com>
> >> > > > > ---
> >> > > > > 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: Kevin Tian <kevin.tian@intel.com>
> >> > > > > ---
> >> > > > >  docs/misc/xen-command-line.markdown   | 14 ++++++
> >> > > > >  xen/drivers/passthrough/arm/iommu.c   |  4 ++
> >> > > > >  xen/drivers/passthrough/iommu.c       |  6 +++
> >> > > > >  xen/drivers/passthrough/vtd/extern.h  |  2 -
> >> > > > >  xen/drivers/passthrough/vtd/iommu.c   |  6 ---
> >> > > > >  xen/drivers/passthrough/vtd/x86/vtd.c | 66 +------------------------
> >> > > > >  xen/drivers/passthrough/x86/iommu.c   | 70
> >> > > > > +++++++++++++++++++++++++++
> >> > > > >  xen/include/xen/iommu.h               |  2 +
> >> > > > >  8 files changed, 97 insertions(+), 73 deletions(-)
> >> > > > >
> >> > > > > diff --git a/docs/misc/xen-command-line.markdown
> >> b/docs/misc/xen-
> >> > > > > command-line.markdown
> >> > > > > index 65b4754418..91a8bfc9a6 100644
> >> > > > > --- a/docs/misc/xen-command-line.markdown
> >> > > > > +++ b/docs/misc/xen-command-line.markdown
> >> > > > > @@ -1198,6 +1198,17 @@ detection of systems known to misbehave
> >> > > upon
> >> > > > > accesses to that port.
> >> > > > >
> >> > > > >  >> Enable IOMMU debugging code (implies `verbose`).
> >> > > > >
> >> > > > > +> `inclusive`
> >> > > >
> >> > > > This is a dom0 (or hwdom) specific setting so perhaps dom0-inclusive?
> >> > > >
> >> > > > Actually the dom0 iommu options are starting to get unwieldy as they
> >> are
> >> > > conflated with the general host iommu options so I think it may be
> >> > > worthwhile splitting things out into a separate 'dom0-iommu=' top level
> >> > > parameter at this stage. (My reasons are slightly selfish as I intend to 
> > add
> >> > > another dom0 iommu option to give it just reserved regions, to avoid
> >> > > unnecessary set-up if we know it will be using PV-IOMMU).
> >> > >
> >> > > Mapping just the reserved regions is what I actually do for PVH with
> >> > > iommu=inclusive (patch 4/4), so maybe it would make sense to speak
> >> about
> >> > > the
> >> > > naming here in order to use the same naming for PV and PVH.
> >> > >
> >> > > TBH I don't really like the dom0- prefix, the command line iommu
> >> > > options either apply to all domains or Dom0 only, having
> >> > > domu-inclusive for example makes no sense IMO.
> >> >
> >> > No, I think there are some options that you may want to apply to dom0
> >> only, but these are more like the dom0_mem or dom0_max_vpus options.
> >> Particularly, the inclusive option is probably something that is only desirable
> >> for dom0. Clearly dom0-passthrough and dom0-strict are already defined to
> >> relate to dom0 only, and options such as 'reserved' should only be specific on
> >> the command line in relation to dom0 IMO. For other domains such an option
> >> should be specified via xl.cfg.
> >> 
> >> Yes, we already have a bunch of those, so then I think dom0-inclusive
> >> and dom0-reserved would be appropriate?
> >> 
> >> dom0-inclusive-mapping or dom0-reserved-mapping seems too long.
> > 
> > Yes, those names are ok, but I still think it better in the long run if we 
> > have something like:
> > 
> > dom0_iommu=[inclusive,][reserved,][strict,][none,][relaxed]
> > 
> > where relaxed is the default and 'none' (I think) is equivalent to the 
> > current iommu=dom0-passthrough.
> 
> Or, along the lines of the other reply just sent, e.g.
> 
> dom0=pvh,iommu:inclusive;reserved,shadow
> 
> But perhaps the difference between , and ; gets too confusing then.

So I think we have the following options:

1. dom0_iommu=[inclusive,][reserved,][strict,][none,][relaxed]
2. dom0=[pvh,][shadow,][iommu=[inclusive;][reserved;][strict;][none;][relaxed]]
3. dom0=[pvh,][shadow,][iommu-inclusive,][iommu-reserved,][iommu-strict,][iommu-none,][iommu-relaxed]

I don't have a strong preference between 1 and 3, but I would prefer
to avoid 2 because I think suboptions inside of options it's too
complex IMO.

Thanks, Roger.

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

Re: [PATCH 2/4] iommu: generalize iommu_inclusive_mapping

[Jan Beulich]

>>> On 31.07.18 at 11:05, <roger.pau@citrix.com> wrote:
> On Tue, Jul 31, 2018 at 02:49:19AM -0600, Jan Beulich wrote:
>> >>> On 31.07.18 at 10:37, <Paul.Durrant@citrix.com> wrote:
>> >>  -----Original Message-----
>> >> From: Roger Pau Monne
>> >> Sent: 31 July 2018 09:34
>> >> To: Paul Durrant <Paul.Durrant@citrix.com>
>> >> Cc: xen-devel@lists.xenproject.org; Kevin Tian <kevin.tian@intel.com>;
>> >> Stefano Stabellini <sstabellini@kernel.org>; Wei Liu <wei.liu2@citrix.com>;
>> >> George Dunlap <George.Dunlap@citrix.com>; Andrew Cooper
>> >> <Andrew.Cooper3@citrix.com>; Ian Jackson <Ian.Jackson@citrix.com>; Tim
>> >> (Xen.org) <tim@xen.org>; Julien Grall <julien.grall@arm.com>; Jan Beulich
>> >> <jbeulich@suse.com>
>> >> Subject: Re: [Xen-devel] [PATCH 2/4] iommu: generalize
>> >> iommu_inclusive_mapping
>> >> 
>> >> On Tue, Jul 31, 2018 at 09:27:03AM +0100, Paul Durrant wrote:
>> >> > > -----Original Message-----
>> >> > > From: Roger Pau Monne
>> >> > > Sent: 31 July 2018 09:16
>> >> > > To: Paul Durrant <Paul.Durrant@citrix.com>
>> >> > > Cc: xen-devel@lists.xenproject.org; Kevin Tian <kevin.tian@intel.com>;
>> >> > > Stefano Stabellini <sstabellini@kernel.org>; Wei Liu
>> >> <wei.liu2@citrix.com>;
>> >> > > George Dunlap <George.Dunlap@citrix.com>; Andrew Cooper
>> >> > > <Andrew.Cooper3@citrix.com>; Ian Jackson <Ian.Jackson@citrix.com>;
>> >> Tim
>> >> > > (Xen.org) <tim@xen.org>; Julien Grall <julien.grall@arm.com>; Jan
>> >> Beulich
>> >> > > <jbeulich@suse.com>
>> >> > > Subject: Re: [Xen-devel] [PATCH 2/4] iommu: generalize
>> >> > > iommu_inclusive_mapping
>> >> > >
>> >> > > On Tue, Jul 31, 2018 at 08:18:36AM +0100, Paul Durrant wrote:
>> >> > > > > -----Original Message-----
>> >> > > > > From: Xen-devel [mailto:xen-devel-bounces@lists.xenproject.org]
>> >> On
>> >> > > Behalf
>> >> > > > > Of Roger Pau Monne
>> >> > > > > Sent: 27 July 2018 16:32
>> >> > > > > To: xen-devel@lists.xenproject.org 
>> >> > > > > Cc: Kevin Tian <kevin.tian@intel.com>; Stefano Stabellini
>> >> > > > > <sstabellini@kernel.org>; Wei Liu <wei.liu2@citrix.com>; George
>> >> Dunlap
>> >> > > > > <George.Dunlap@citrix.com>; Andrew Cooper
>> >> > > > > <Andrew.Cooper3@citrix.com>; Ian Jackson
>> >> <Ian.Jackson@citrix.com>;
>> >> > > Tim
>> >> > > > > (Xen.org) <tim@xen.org>; Julien Grall <julien.grall@arm.com>; Jan
>> >> > > Beulich
>> >> > > > > <jbeulich@suse.com>; Roger Pau Monne <roger.pau@citrix.com>
>> >> > > > > Subject: [Xen-devel] [PATCH 2/4] iommu: generalize
>> >> > > > > iommu_inclusive_mapping
>> >> > > > >
>> >> > > > > Introduce a new iommu=inclusive generic option that supersedes
>> >> > > > > iommu_inclusive_mapping. This should be a non-functional change
>> >> on
>> >> > > > > Intel hardware, while AMD hardware will gain the same functionality
>> >> of
>> >> > > > > mapping almost everything below the 4GB boundary.
>> >> > > > >
>> >> > > > > Note that is a noop for ARM hardware.
>> >> > > > >
>> >> > > > > Signed-off-by: Roger Pau Monné <roger.pau@citrix.com>
>> >> > > > > ---
>> >> > > > > 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: Kevin Tian <kevin.tian@intel.com>
>> >> > > > > ---
>> >> > > > >  docs/misc/xen-command-line.markdown   | 14 ++++++
>> >> > > > >  xen/drivers/passthrough/arm/iommu.c   |  4 ++
>> >> > > > >  xen/drivers/passthrough/iommu.c       |  6 +++
>> >> > > > >  xen/drivers/passthrough/vtd/extern.h  |  2 -
>> >> > > > >  xen/drivers/passthrough/vtd/iommu.c   |  6 ---
>> >> > > > >  xen/drivers/passthrough/vtd/x86/vtd.c | 66 +------------------------
>> >> > > > >  xen/drivers/passthrough/x86/iommu.c   | 70
>> >> > > > > +++++++++++++++++++++++++++
>> >> > > > >  xen/include/xen/iommu.h               |  2 +
>> >> > > > >  8 files changed, 97 insertions(+), 73 deletions(-)
>> >> > > > >
>> >> > > > > diff --git a/docs/misc/xen-command-line.markdown
>> >> b/docs/misc/xen-
>> >> > > > > command-line.markdown
>> >> > > > > index 65b4754418..91a8bfc9a6 100644
>> >> > > > > --- a/docs/misc/xen-command-line.markdown
>> >> > > > > +++ b/docs/misc/xen-command-line.markdown
>> >> > > > > @@ -1198,6 +1198,17 @@ detection of systems known to misbehave
>> >> > > upon
>> >> > > > > accesses to that port.
>> >> > > > >
>> >> > > > >  >> Enable IOMMU debugging code (implies `verbose`).
>> >> > > > >
>> >> > > > > +> `inclusive`
>> >> > > >
>> >> > > > This is a dom0 (or hwdom) specific setting so perhaps dom0-inclusive?
>> >> > > >
>> >> > > > Actually the dom0 iommu options are starting to get unwieldy as they
>> >> are
>> >> > > conflated with the general host iommu options so I think it may be
>> >> > > worthwhile splitting things out into a separate 'dom0-iommu=' top level
>> >> > > parameter at this stage. (My reasons are slightly selfish as I intend to 
>> > add
>> >> > > another dom0 iommu option to give it just reserved regions, to avoid
>> >> > > unnecessary set-up if we know it will be using PV-IOMMU).
>> >> > >
>> >> > > Mapping just the reserved regions is what I actually do for PVH with
>> >> > > iommu=inclusive (patch 4/4), so maybe it would make sense to speak
>> >> about
>> >> > > the
>> >> > > naming here in order to use the same naming for PV and PVH.
>> >> > >
>> >> > > TBH I don't really like the dom0- prefix, the command line iommu
>> >> > > options either apply to all domains or Dom0 only, having
>> >> > > domu-inclusive for example makes no sense IMO.
>> >> >
>> >> > No, I think there are some options that you may want to apply to dom0
>> >> only, but these are more like the dom0_mem or dom0_max_vpus options.
>> >> Particularly, the inclusive option is probably something that is only 
> desirable
>> >> for dom0. Clearly dom0-passthrough and dom0-strict are already defined to
>> >> relate to dom0 only, and options such as 'reserved' should only be specific 
> on
>> >> the command line in relation to dom0 IMO. For other domains such an option
>> >> should be specified via xl.cfg.
>> >> 
>> >> Yes, we already have a bunch of those, so then I think dom0-inclusive
>> >> and dom0-reserved would be appropriate?
>> >> 
>> >> dom0-inclusive-mapping or dom0-reserved-mapping seems too long.
>> > 
>> > Yes, those names are ok, but I still think it better in the long run if we 
>> > have something like:
>> > 
>> > dom0_iommu=[inclusive,][reserved,][strict,][none,][relaxed]
>> > 
>> > where relaxed is the default and 'none' (I think) is equivalent to the 
>> > current iommu=dom0-passthrough.
>> 
>> Or, along the lines of the other reply just sent, e.g.
>> 
>> dom0=pvh,iommu:inclusive;reserved,shadow
>> 
>> But perhaps the difference between , and ; gets too confusing then.
> 
> So I think we have the following options:
> 
> 1. dom0_iommu=[inclusive,][reserved,][strict,][none,][relaxed]

Nit: dom0-iommu= (no underscores in new options)

> 2. dom0=[pvh,][shadow,][iommu=[inclusive;][reserved;][strict;][none;][relaxed]]
> 3. dom0=[pvh,][shadow,][iommu-inclusive,][iommu-reserved,][iommu-strict,][iommu-none,][iommu-relaxed]
> 
> I don't have a strong preference between 1 and 3, but I would prefer
> to avoid 2 because I think suboptions inside of options it's too
> complex IMO.

While generally I prefer to limit the number of top level options, in
this case I think I'd prefer 1 after all. Or wait - does any pair of the
(sub)options actually make sense to be specified? Isn't it rather a
choice of five than an enumeration of up to 5? In which case I'd
still prefer 2 (as then there's no need for a second separator
beside comma), the more that we have at least one example with
such sub-options (cpufreq).

Jan


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

Re: [PATCH 2/4] iommu: generalize iommu_inclusive_mapping

[Roger Pau Monné]

On Tue, Jul 31, 2018 at 03:14:47AM -0600, Jan Beulich wrote:
> >>> On 31.07.18 at 11:05, <roger.pau@citrix.com> wrote:
> > On Tue, Jul 31, 2018 at 02:49:19AM -0600, Jan Beulich wrote:
> >> >>> On 31.07.18 at 10:37, <Paul.Durrant@citrix.com> wrote:
> >> >>  -----Original Message-----
> >> >> From: Roger Pau Monne
> >> >> Sent: 31 July 2018 09:34
> >> >> To: Paul Durrant <Paul.Durrant@citrix.com>
> >> >> Cc: xen-devel@lists.xenproject.org; Kevin Tian <kevin.tian@intel.com>;
> >> >> Stefano Stabellini <sstabellini@kernel.org>; Wei Liu <wei.liu2@citrix.com>;
> >> >> George Dunlap <George.Dunlap@citrix.com>; Andrew Cooper
> >> >> <Andrew.Cooper3@citrix.com>; Ian Jackson <Ian.Jackson@citrix.com>; Tim
> >> >> (Xen.org) <tim@xen.org>; Julien Grall <julien.grall@arm.com>; Jan Beulich
> >> >> <jbeulich@suse.com>
> >> >> Subject: Re: [Xen-devel] [PATCH 2/4] iommu: generalize
> >> >> iommu_inclusive_mapping
> >> >> 
> >> >> On Tue, Jul 31, 2018 at 09:27:03AM +0100, Paul Durrant wrote:
> >> >> > > -----Original Message-----
> >> >> > > From: Roger Pau Monne
> >> >> > > Sent: 31 July 2018 09:16
> >> >> > > To: Paul Durrant <Paul.Durrant@citrix.com>
> >> >> > > Cc: xen-devel@lists.xenproject.org; Kevin Tian <kevin.tian@intel.com>;
> >> >> > > Stefano Stabellini <sstabellini@kernel.org>; Wei Liu
> >> >> <wei.liu2@citrix.com>;
> >> >> > > George Dunlap <George.Dunlap@citrix.com>; Andrew Cooper
> >> >> > > <Andrew.Cooper3@citrix.com>; Ian Jackson <Ian.Jackson@citrix.com>;
> >> >> Tim
> >> >> > > (Xen.org) <tim@xen.org>; Julien Grall <julien.grall@arm.com>; Jan
> >> >> Beulich
> >> >> > > <jbeulich@suse.com>
> >> >> > > Subject: Re: [Xen-devel] [PATCH 2/4] iommu: generalize
> >> >> > > iommu_inclusive_mapping
> >> >> > >
> >> >> > > On Tue, Jul 31, 2018 at 08:18:36AM +0100, Paul Durrant wrote:
> >> >> > > > > -----Original Message-----
> >> >> > > > > From: Xen-devel [mailto:xen-devel-bounces@lists.xenproject.org]
> >> >> On
> >> >> > > Behalf
> >> >> > > > > Of Roger Pau Monne
> >> >> > > > > Sent: 27 July 2018 16:32
> >> >> > > > > To: xen-devel@lists.xenproject.org 
> >> >> > > > > Cc: Kevin Tian <kevin.tian@intel.com>; Stefano Stabellini
> >> >> > > > > <sstabellini@kernel.org>; Wei Liu <wei.liu2@citrix.com>; George
> >> >> Dunlap
> >> >> > > > > <George.Dunlap@citrix.com>; Andrew Cooper
> >> >> > > > > <Andrew.Cooper3@citrix.com>; Ian Jackson
> >> >> <Ian.Jackson@citrix.com>;
> >> >> > > Tim
> >> >> > > > > (Xen.org) <tim@xen.org>; Julien Grall <julien.grall@arm.com>; Jan
> >> >> > > Beulich
> >> >> > > > > <jbeulich@suse.com>; Roger Pau Monne <roger.pau@citrix.com>
> >> >> > > > > Subject: [Xen-devel] [PATCH 2/4] iommu: generalize
> >> >> > > > > iommu_inclusive_mapping
> >> >> > > > >
> >> >> > > > > Introduce a new iommu=inclusive generic option that supersedes
> >> >> > > > > iommu_inclusive_mapping. This should be a non-functional change
> >> >> on
> >> >> > > > > Intel hardware, while AMD hardware will gain the same functionality
> >> >> of
> >> >> > > > > mapping almost everything below the 4GB boundary.
> >> >> > > > >
> >> >> > > > > Note that is a noop for ARM hardware.
> >> >> > > > >
> >> >> > > > > Signed-off-by: Roger Pau Monné <roger.pau@citrix.com>
> >> >> > > > > ---
> >> >> > > > > 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: Kevin Tian <kevin.tian@intel.com>
> >> >> > > > > ---
> >> >> > > > >  docs/misc/xen-command-line.markdown   | 14 ++++++
> >> >> > > > >  xen/drivers/passthrough/arm/iommu.c   |  4 ++
> >> >> > > > >  xen/drivers/passthrough/iommu.c       |  6 +++
> >> >> > > > >  xen/drivers/passthrough/vtd/extern.h  |  2 -
> >> >> > > > >  xen/drivers/passthrough/vtd/iommu.c   |  6 ---
> >> >> > > > >  xen/drivers/passthrough/vtd/x86/vtd.c | 66 +------------------------
> >> >> > > > >  xen/drivers/passthrough/x86/iommu.c   | 70
> >> >> > > > > +++++++++++++++++++++++++++
> >> >> > > > >  xen/include/xen/iommu.h               |  2 +
> >> >> > > > >  8 files changed, 97 insertions(+), 73 deletions(-)
> >> >> > > > >
> >> >> > > > > diff --git a/docs/misc/xen-command-line.markdown
> >> >> b/docs/misc/xen-
> >> >> > > > > command-line.markdown
> >> >> > > > > index 65b4754418..91a8bfc9a6 100644
> >> >> > > > > --- a/docs/misc/xen-command-line.markdown
> >> >> > > > > +++ b/docs/misc/xen-command-line.markdown
> >> >> > > > > @@ -1198,6 +1198,17 @@ detection of systems known to misbehave
> >> >> > > upon
> >> >> > > > > accesses to that port.
> >> >> > > > >
> >> >> > > > >  >> Enable IOMMU debugging code (implies `verbose`).
> >> >> > > > >
> >> >> > > > > +> `inclusive`
> >> >> > > >
> >> >> > > > This is a dom0 (or hwdom) specific setting so perhaps dom0-inclusive?
> >> >> > > >
> >> >> > > > Actually the dom0 iommu options are starting to get unwieldy as they
> >> >> are
> >> >> > > conflated with the general host iommu options so I think it may be
> >> >> > > worthwhile splitting things out into a separate 'dom0-iommu=' top level
> >> >> > > parameter at this stage. (My reasons are slightly selfish as I intend to 
> >> > add
> >> >> > > another dom0 iommu option to give it just reserved regions, to avoid
> >> >> > > unnecessary set-up if we know it will be using PV-IOMMU).
> >> >> > >
> >> >> > > Mapping just the reserved regions is what I actually do for PVH with
> >> >> > > iommu=inclusive (patch 4/4), so maybe it would make sense to speak
> >> >> about
> >> >> > > the
> >> >> > > naming here in order to use the same naming for PV and PVH.
> >> >> > >
> >> >> > > TBH I don't really like the dom0- prefix, the command line iommu
> >> >> > > options either apply to all domains or Dom0 only, having
> >> >> > > domu-inclusive for example makes no sense IMO.
> >> >> >
> >> >> > No, I think there are some options that you may want to apply to dom0
> >> >> only, but these are more like the dom0_mem or dom0_max_vpus options.
> >> >> Particularly, the inclusive option is probably something that is only 
> > desirable
> >> >> for dom0. Clearly dom0-passthrough and dom0-strict are already defined to
> >> >> relate to dom0 only, and options such as 'reserved' should only be specific 
> > on
> >> >> the command line in relation to dom0 IMO. For other domains such an option
> >> >> should be specified via xl.cfg.
> >> >> 
> >> >> Yes, we already have a bunch of those, so then I think dom0-inclusive
> >> >> and dom0-reserved would be appropriate?
> >> >> 
> >> >> dom0-inclusive-mapping or dom0-reserved-mapping seems too long.
> >> > 
> >> > Yes, those names are ok, but I still think it better in the long run if we 
> >> > have something like:
> >> > 
> >> > dom0_iommu=[inclusive,][reserved,][strict,][none,][relaxed]
> >> > 
> >> > where relaxed is the default and 'none' (I think) is equivalent to the 
> >> > current iommu=dom0-passthrough.
> >> 
> >> Or, along the lines of the other reply just sent, e.g.
> >> 
> >> dom0=pvh,iommu:inclusive;reserved,shadow
> >> 
> >> But perhaps the difference between , and ; gets too confusing then.
> > 
> > So I think we have the following options:
> > 
> > 1. dom0_iommu=[inclusive,][reserved,][strict,][none,][relaxed]
> 
> Nit: dom0-iommu= (no underscores in new options)
> 
> > 2. dom0=[pvh,][shadow,][iommu=[inclusive;][reserved;][strict;][none;][relaxed]]
> > 3. dom0=[pvh,][shadow,][iommu-inclusive,][iommu-reserved,][iommu-strict,][iommu-none,][iommu-relaxed]
> > 
> > I don't have a strong preference between 1 and 3, but I would prefer
> > to avoid 2 because I think suboptions inside of options it's too
> > complex IMO.
> 
> While generally I prefer to limit the number of top level options, in
> this case I think I'd prefer 1 after all. Or wait - does any pair of the
> (sub)options actually make sense to be specified?

Yes, for example you can use strict and inclusive at the same time, I
think it's something like:

dom0=[pvh,][shadow,][iommu=[inclusive|reserved;][strict|none|relaxed]]

> Isn't it rather a
> choice of five than an enumeration of up to 5? In which case I'd
> still prefer 2 (as then there's no need for a second separator
> beside comma), the more that we have at least one example with
> such sub-options (cpufreq).

OK, I can do the nested iommu option inside of dom0 if that's the
preference.

Thanks, Roger.

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

Re: [PATCH 2/4] iommu: generalize iommu_inclusive_mapping

[Paul Durrant]

> -----Original Message-----
> From: Roger Pau Monne
> Sent: 31 July 2018 10:35
> To: Jan Beulich <JBeulich@suse.com>
> Cc: Julien Grall <julien.grall@arm.com>; Andrew Cooper
> <Andrew.Cooper3@citrix.com>; George Dunlap
> <George.Dunlap@citrix.com>; Ian Jackson <Ian.Jackson@citrix.com>; Paul
> Durrant <Paul.Durrant@citrix.com>; Wei Liu <wei.liu2@citrix.com>; Kevin
> Tian <kevin.tian@intel.com>; Stefano Stabellini <sstabellini@kernel.org>;
> xen-devel <xen-devel@lists.xenproject.org>; Tim (Xen.org) <tim@xen.org>
> Subject: Re: [Xen-devel] [PATCH 2/4] iommu: generalize
> iommu_inclusive_mapping
> 
> On Tue, Jul 31, 2018 at 03:14:47AM -0600, Jan Beulich wrote:
> > >>> On 31.07.18 at 11:05, <roger.pau@citrix.com> wrote:
> > > On Tue, Jul 31, 2018 at 02:49:19AM -0600, Jan Beulich wrote:
> > >> >>> On 31.07.18 at 10:37, <Paul.Durrant@citrix.com> wrote:
> > >> >>  -----Original Message-----
> > >> >> From: Roger Pau Monne
> > >> >> Sent: 31 July 2018 09:34
> > >> >> To: Paul Durrant <Paul.Durrant@citrix.com>
> > >> >> Cc: xen-devel@lists.xenproject.org; Kevin Tian
> <kevin.tian@intel.com>;
> > >> >> Stefano Stabellini <sstabellini@kernel.org>; Wei Liu
> <wei.liu2@citrix.com>;
> > >> >> George Dunlap <George.Dunlap@citrix.com>; Andrew Cooper
> > >> >> <Andrew.Cooper3@citrix.com>; Ian Jackson
> <Ian.Jackson@citrix.com>; Tim
> > >> >> (Xen.org) <tim@xen.org>; Julien Grall <julien.grall@arm.com>; Jan
> Beulich
> > >> >> <jbeulich@suse.com>
> > >> >> Subject: Re: [Xen-devel] [PATCH 2/4] iommu: generalize
> > >> >> iommu_inclusive_mapping
> > >> >>
> > >> >> On Tue, Jul 31, 2018 at 09:27:03AM +0100, Paul Durrant wrote:
> > >> >> > > -----Original Message-----
> > >> >> > > From: Roger Pau Monne
> > >> >> > > Sent: 31 July 2018 09:16
> > >> >> > > To: Paul Durrant <Paul.Durrant@citrix.com>
> > >> >> > > Cc: xen-devel@lists.xenproject.org; Kevin Tian
> <kevin.tian@intel.com>;
> > >> >> > > Stefano Stabellini <sstabellini@kernel.org>; Wei Liu
> > >> >> <wei.liu2@citrix.com>;
> > >> >> > > George Dunlap <George.Dunlap@citrix.com>; Andrew Cooper
> > >> >> > > <Andrew.Cooper3@citrix.com>; Ian Jackson
> <Ian.Jackson@citrix.com>;
> > >> >> Tim
> > >> >> > > (Xen.org) <tim@xen.org>; Julien Grall <julien.grall@arm.com>;
> Jan
> > >> >> Beulich
> > >> >> > > <jbeulich@suse.com>
> > >> >> > > Subject: Re: [Xen-devel] [PATCH 2/4] iommu: generalize
> > >> >> > > iommu_inclusive_mapping
> > >> >> > >
> > >> >> > > On Tue, Jul 31, 2018 at 08:18:36AM +0100, Paul Durrant wrote:
> > >> >> > > > > -----Original Message-----
> > >> >> > > > > From: Xen-devel [mailto:xen-devel-
> bounces@lists.xenproject.org]
> > >> >> On
> > >> >> > > Behalf
> > >> >> > > > > Of Roger Pau Monne
> > >> >> > > > > Sent: 27 July 2018 16:32
> > >> >> > > > > To: xen-devel@lists.xenproject.org
> > >> >> > > > > Cc: Kevin Tian <kevin.tian@intel.com>; Stefano Stabellini
> > >> >> > > > > <sstabellini@kernel.org>; Wei Liu <wei.liu2@citrix.com>;
> George
> > >> >> Dunlap
> > >> >> > > > > <George.Dunlap@citrix.com>; Andrew Cooper
> > >> >> > > > > <Andrew.Cooper3@citrix.com>; Ian Jackson
> > >> >> <Ian.Jackson@citrix.com>;
> > >> >> > > Tim
> > >> >> > > > > (Xen.org) <tim@xen.org>; Julien Grall
> <julien.grall@arm.com>; Jan
> > >> >> > > Beulich
> > >> >> > > > > <jbeulich@suse.com>; Roger Pau Monne
> <roger.pau@citrix.com>
> > >> >> > > > > Subject: [Xen-devel] [PATCH 2/4] iommu: generalize
> > >> >> > > > > iommu_inclusive_mapping
> > >> >> > > > >
> > >> >> > > > > Introduce a new iommu=inclusive generic option that
> supersedes
> > >> >> > > > > iommu_inclusive_mapping. This should be a non-functional
> change
> > >> >> on
> > >> >> > > > > Intel hardware, while AMD hardware will gain the same
> functionality
> > >> >> of
> > >> >> > > > > mapping almost everything below the 4GB boundary.
> > >> >> > > > >
> > >> >> > > > > Note that is a noop for ARM hardware.
> > >> >> > > > >
> > >> >> > > > > Signed-off-by: Roger Pau Monné <roger.pau@citrix.com>
> > >> >> > > > > ---
> > >> >> > > > > 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: Kevin Tian <kevin.tian@intel.com>
> > >> >> > > > > ---
> > >> >> > > > >  docs/misc/xen-command-line.markdown   | 14 ++++++
> > >> >> > > > >  xen/drivers/passthrough/arm/iommu.c   |  4 ++
> > >> >> > > > >  xen/drivers/passthrough/iommu.c       |  6 +++
> > >> >> > > > >  xen/drivers/passthrough/vtd/extern.h  |  2 -
> > >> >> > > > >  xen/drivers/passthrough/vtd/iommu.c   |  6 ---
> > >> >> > > > >  xen/drivers/passthrough/vtd/x86/vtd.c | 66 +------------------
> ------
> > >> >> > > > >  xen/drivers/passthrough/x86/iommu.c   | 70
> > >> >> > > > > +++++++++++++++++++++++++++
> > >> >> > > > >  xen/include/xen/iommu.h               |  2 +
> > >> >> > > > >  8 files changed, 97 insertions(+), 73 deletions(-)
> > >> >> > > > >
> > >> >> > > > > diff --git a/docs/misc/xen-command-line.markdown
> > >> >> b/docs/misc/xen-
> > >> >> > > > > command-line.markdown
> > >> >> > > > > index 65b4754418..91a8bfc9a6 100644
> > >> >> > > > > --- a/docs/misc/xen-command-line.markdown
> > >> >> > > > > +++ b/docs/misc/xen-command-line.markdown
> > >> >> > > > > @@ -1198,6 +1198,17 @@ detection of systems known to
> misbehave
> > >> >> > > upon
> > >> >> > > > > accesses to that port.
> > >> >> > > > >
> > >> >> > > > >  >> Enable IOMMU debugging code (implies `verbose`).
> > >> >> > > > >
> > >> >> > > > > +> `inclusive`
> > >> >> > > >
> > >> >> > > > This is a dom0 (or hwdom) specific setting so perhaps dom0-
> inclusive?
> > >> >> > > >
> > >> >> > > > Actually the dom0 iommu options are starting to get unwieldy
> as they
> > >> >> are
> > >> >> > > conflated with the general host iommu options so I think it may
> be
> > >> >> > > worthwhile splitting things out into a separate 'dom0-iommu='
> top level
> > >> >> > > parameter at this stage. (My reasons are slightly selfish as I
> intend to
> > >> > add
> > >> >> > > another dom0 iommu option to give it just reserved regions, to
> avoid
> > >> >> > > unnecessary set-up if we know it will be using PV-IOMMU).
> > >> >> > >
> > >> >> > > Mapping just the reserved regions is what I actually do for PVH
> with
> > >> >> > > iommu=inclusive (patch 4/4), so maybe it would make sense to
> speak
> > >> >> about
> > >> >> > > the
> > >> >> > > naming here in order to use the same naming for PV and PVH.
> > >> >> > >
> > >> >> > > TBH I don't really like the dom0- prefix, the command line iommu
> > >> >> > > options either apply to all domains or Dom0 only, having
> > >> >> > > domu-inclusive for example makes no sense IMO.
> > >> >> >
> > >> >> > No, I think there are some options that you may want to apply to
> dom0
> > >> >> only, but these are more like the dom0_mem or dom0_max_vpus
> options.
> > >> >> Particularly, the inclusive option is probably something that is only
> > > desirable
> > >> >> for dom0. Clearly dom0-passthrough and dom0-strict are already
> defined to
> > >> >> relate to dom0 only, and options such as 'reserved' should only be
> specific
> > > on
> > >> >> the command line in relation to dom0 IMO. For other domains such
> an option
> > >> >> should be specified via xl.cfg.
> > >> >>
> > >> >> Yes, we already have a bunch of those, so then I think dom0-
> inclusive
> > >> >> and dom0-reserved would be appropriate?
> > >> >>
> > >> >> dom0-inclusive-mapping or dom0-reserved-mapping seems too
> long.
> > >> >
> > >> > Yes, those names are ok, but I still think it better in the long run if we
> > >> > have something like:
> > >> >
> > >> > dom0_iommu=[inclusive,][reserved,][strict,][none,][relaxed]
> > >> >
> > >> > where relaxed is the default and 'none' (I think) is equivalent to the
> > >> > current iommu=dom0-passthrough.
> > >>
> > >> Or, along the lines of the other reply just sent, e.g.
> > >>
> > >> dom0=pvh,iommu:inclusive;reserved,shadow
> > >>
> > >> But perhaps the difference between , and ; gets too confusing then.
> > >
> > > So I think we have the following options:
> > >
> > > 1. dom0_iommu=[inclusive,][reserved,][strict,][none,][relaxed]
> >
> > Nit: dom0-iommu= (no underscores in new options)
> >
> > > 2.
> dom0=[pvh,][shadow,][iommu=[inclusive;][reserved;][strict;][none;][relaxe
> d]]
> > > 3. dom0=[pvh,][shadow,][iommu-inclusive,][iommu-reserved,][iommu-
> strict,][iommu-none,][iommu-relaxed]
> > >
> > > I don't have a strong preference between 1 and 3, but I would prefer
> > > to avoid 2 because I think suboptions inside of options it's too
> > > complex IMO.
> >
> > While generally I prefer to limit the number of top level options, in
> > this case I think I'd prefer 1 after all. Or wait - does any pair of the
> > (sub)options actually make sense to be specified?
> 
> Yes, for example you can use strict and inclusive at the same time, I
> think it's something like:
> 
> dom0=[pvh,][shadow,][iommu=[inclusive|reserved;][strict|none|relaxed]]

I think it may be:

dom0=[pvh,][shadow,][iommu=[inclusive|reserved|none;][strict|relaxed]]

where 'none' in the first part means the second part is moot. (Not sure how to express that cleanly).

  Paul

> 
> > Isn't it rather a
> > choice of five than an enumeration of up to 5? In which case I'd
> > still prefer 2 (as then there's no need for a second separator
> > beside comma), the more that we have at least one example with
> > such sub-options (cpufreq).
> 
> OK, I can do the nested iommu option inside of dom0 if that's the
> preference.
> 
> Thanks, Roger.

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

Re: [PATCH 2/4] iommu: generalize iommu_inclusive_mapping

[Jan Beulich]

>>> On 31.07.18 at 11:34, <roger.pau@citrix.com> wrote:
> On Tue, Jul 31, 2018 at 03:14:47AM -0600, Jan Beulich wrote:
>> >>> On 31.07.18 at 11:05, <roger.pau@citrix.com> wrote:
>> > On Tue, Jul 31, 2018 at 02:49:19AM -0600, Jan Beulich wrote:
>> >> >>> On 31.07.18 at 10:37, <Paul.Durrant@citrix.com> wrote:
>> >> >>  -----Original Message-----
>> >> >> From: Roger Pau Monne
>> >> >> Sent: 31 July 2018 09:34
>> >> >> To: Paul Durrant <Paul.Durrant@citrix.com>
>> >> >> Cc: xen-devel@lists.xenproject.org; Kevin Tian <kevin.tian@intel.com>;
>> >> >> Stefano Stabellini <sstabellini@kernel.org>; Wei Liu <wei.liu2@citrix.com>;
>> >> >> George Dunlap <George.Dunlap@citrix.com>; Andrew Cooper
>> >> >> <Andrew.Cooper3@citrix.com>; Ian Jackson <Ian.Jackson@citrix.com>; Tim
>> >> >> (Xen.org) <tim@xen.org>; Julien Grall <julien.grall@arm.com>; Jan Beulich
>> >> >> <jbeulich@suse.com>
>> >> >> Subject: Re: [Xen-devel] [PATCH 2/4] iommu: generalize
>> >> >> iommu_inclusive_mapping
>> >> >> 
>> >> >> On Tue, Jul 31, 2018 at 09:27:03AM +0100, Paul Durrant wrote:
>> >> >> > > -----Original Message-----
>> >> >> > > From: Roger Pau Monne
>> >> >> > > Sent: 31 July 2018 09:16
>> >> >> > > To: Paul Durrant <Paul.Durrant@citrix.com>
>> >> >> > > Cc: xen-devel@lists.xenproject.org; Kevin Tian <kevin.tian@intel.com>;
>> >> >> > > Stefano Stabellini <sstabellini@kernel.org>; Wei Liu
>> >> >> <wei.liu2@citrix.com>;
>> >> >> > > George Dunlap <George.Dunlap@citrix.com>; Andrew Cooper
>> >> >> > > <Andrew.Cooper3@citrix.com>; Ian Jackson <Ian.Jackson@citrix.com>;
>> >> >> Tim
>> >> >> > > (Xen.org) <tim@xen.org>; Julien Grall <julien.grall@arm.com>; Jan
>> >> >> Beulich
>> >> >> > > <jbeulich@suse.com>
>> >> >> > > Subject: Re: [Xen-devel] [PATCH 2/4] iommu: generalize
>> >> >> > > iommu_inclusive_mapping
>> >> >> > >
>> >> >> > > On Tue, Jul 31, 2018 at 08:18:36AM +0100, Paul Durrant wrote:
>> >> >> > > > > -----Original Message-----
>> >> >> > > > > From: Xen-devel [mailto:xen-devel-bounces@lists.xenproject.org]
>> >> >> On
>> >> >> > > Behalf
>> >> >> > > > > Of Roger Pau Monne
>> >> >> > > > > Sent: 27 July 2018 16:32
>> >> >> > > > > To: xen-devel@lists.xenproject.org 
>> >> >> > > > > Cc: Kevin Tian <kevin.tian@intel.com>; Stefano Stabellini
>> >> >> > > > > <sstabellini@kernel.org>; Wei Liu <wei.liu2@citrix.com>; George
>> >> >> Dunlap
>> >> >> > > > > <George.Dunlap@citrix.com>; Andrew Cooper
>> >> >> > > > > <Andrew.Cooper3@citrix.com>; Ian Jackson
>> >> >> <Ian.Jackson@citrix.com>;
>> >> >> > > Tim
>> >> >> > > > > (Xen.org) <tim@xen.org>; Julien Grall <julien.grall@arm.com>; Jan
>> >> >> > > Beulich
>> >> >> > > > > <jbeulich@suse.com>; Roger Pau Monne <roger.pau@citrix.com>
>> >> >> > > > > Subject: [Xen-devel] [PATCH 2/4] iommu: generalize
>> >> >> > > > > iommu_inclusive_mapping
>> >> >> > > > >
>> >> >> > > > > Introduce a new iommu=inclusive generic option that supersedes
>> >> >> > > > > iommu_inclusive_mapping. This should be a non-functional change
>> >> >> on
>> >> >> > > > > Intel hardware, while AMD hardware will gain the same functionality
>> >> >> of
>> >> >> > > > > mapping almost everything below the 4GB boundary.
>> >> >> > > > >
>> >> >> > > > > Note that is a noop for ARM hardware.
>> >> >> > > > >
>> >> >> > > > > Signed-off-by: Roger Pau Monné <roger.pau@citrix.com>
>> >> >> > > > > ---
>> >> >> > > > > 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: Kevin Tian <kevin.tian@intel.com>
>> >> >> > > > > ---
>> >> >> > > > >  docs/misc/xen-command-line.markdown   | 14 ++++++
>> >> >> > > > >  xen/drivers/passthrough/arm/iommu.c   |  4 ++
>> >> >> > > > >  xen/drivers/passthrough/iommu.c       |  6 +++
>> >> >> > > > >  xen/drivers/passthrough/vtd/extern.h  |  2 -
>> >> >> > > > >  xen/drivers/passthrough/vtd/iommu.c   |  6 ---
>> >> >> > > > >  xen/drivers/passthrough/vtd/x86/vtd.c | 66 +------------------------
>> >> >> > > > >  xen/drivers/passthrough/x86/iommu.c   | 70
>> >> >> > > > > +++++++++++++++++++++++++++
>> >> >> > > > >  xen/include/xen/iommu.h               |  2 +
>> >> >> > > > >  8 files changed, 97 insertions(+), 73 deletions(-)
>> >> >> > > > >
>> >> >> > > > > diff --git a/docs/misc/xen-command-line.markdown
>> >> >> b/docs/misc/xen-
>> >> >> > > > > command-line.markdown
>> >> >> > > > > index 65b4754418..91a8bfc9a6 100644
>> >> >> > > > > --- a/docs/misc/xen-command-line.markdown
>> >> >> > > > > +++ b/docs/misc/xen-command-line.markdown
>> >> >> > > > > @@ -1198,6 +1198,17 @@ detection of systems known to misbehave
>> >> >> > > upon
>> >> >> > > > > accesses to that port.
>> >> >> > > > >
>> >> >> > > > >  >> Enable IOMMU debugging code (implies `verbose`).
>> >> >> > > > >
>> >> >> > > > > +> `inclusive`
>> >> >> > > >
>> >> >> > > > This is a dom0 (or hwdom) specific setting so perhaps dom0-inclusive?
>> >> >> > > >
>> >> >> > > > Actually the dom0 iommu options are starting to get unwieldy as they
>> >> >> are
>> >> >> > > conflated with the general host iommu options so I think it may be
>> >> >> > > worthwhile splitting things out into a separate 'dom0-iommu=' top level
>> >> >> > > parameter at this stage. (My reasons are slightly selfish as I intend to 
> 
>> >> > add
>> >> >> > > another dom0 iommu option to give it just reserved regions, to avoid
>> >> >> > > unnecessary set-up if we know it will be using PV-IOMMU).
>> >> >> > >
>> >> >> > > Mapping just the reserved regions is what I actually do for PVH with
>> >> >> > > iommu=inclusive (patch 4/4), so maybe it would make sense to speak
>> >> >> about
>> >> >> > > the
>> >> >> > > naming here in order to use the same naming for PV and PVH.
>> >> >> > >
>> >> >> > > TBH I don't really like the dom0- prefix, the command line iommu
>> >> >> > > options either apply to all domains or Dom0 only, having
>> >> >> > > domu-inclusive for example makes no sense IMO.
>> >> >> >
>> >> >> > No, I think there are some options that you may want to apply to dom0
>> >> >> only, but these are more like the dom0_mem or dom0_max_vpus options.
>> >> >> Particularly, the inclusive option is probably something that is only 
>> > desirable
>> >> >> for dom0. Clearly dom0-passthrough and dom0-strict are already defined to
>> >> >> relate to dom0 only, and options such as 'reserved' should only be 
> specific 
>> > on
>> >> >> the command line in relation to dom0 IMO. For other domains such an option
>> >> >> should be specified via xl.cfg.
>> >> >> 
>> >> >> Yes, we already have a bunch of those, so then I think dom0-inclusive
>> >> >> and dom0-reserved would be appropriate?
>> >> >> 
>> >> >> dom0-inclusive-mapping or dom0-reserved-mapping seems too long.
>> >> > 
>> >> > Yes, those names are ok, but I still think it better in the long run if we 
> 
>> >> > have something like:
>> >> > 
>> >> > dom0_iommu=[inclusive,][reserved,][strict,][none,][relaxed]
>> >> > 
>> >> > where relaxed is the default and 'none' (I think) is equivalent to the 
>> >> > current iommu=dom0-passthrough.
>> >> 
>> >> Or, along the lines of the other reply just sent, e.g.
>> >> 
>> >> dom0=pvh,iommu:inclusive;reserved,shadow
>> >> 
>> >> But perhaps the difference between , and ; gets too confusing then.
>> > 
>> > So I think we have the following options:
>> > 
>> > 1. dom0_iommu=[inclusive,][reserved,][strict,][none,][relaxed]
>> 
>> Nit: dom0-iommu= (no underscores in new options)
>> 
>> > 2. dom0=[pvh,][shadow,][iommu=[inclusive;][reserved;][strict;][none;][relaxed]]
>> > 3. dom0=[pvh,][shadow,][iommu-inclusive,][iommu-reserved,][iommu-strict,][iommu-none,][iommu-relaxed]
>> > 
>> > I don't have a strong preference between 1 and 3, but I would prefer
>> > to avoid 2 because I think suboptions inside of options it's too
>> > complex IMO.
>> 
>> While generally I prefer to limit the number of top level options, in
>> this case I think I'd prefer 1 after all. Or wait - does any pair of the
>> (sub)options actually make sense to be specified?
> 
> Yes, for example you can use strict and inclusive at the same time, I
> think it's something like:
> 
> dom0=[pvh,][shadow,][iommu=[inclusive|reserved;][strict|none|relaxed]]
> 
>> Isn't it rather a
>> choice of five than an enumeration of up to 5? In which case I'd
>> still prefer 2 (as then there's no need for a second separator
>> beside comma), the more that we have at least one example with
>> such sub-options (cpufreq).
> 
> OK, I can do the nested iommu option inside of dom0 if that's the
> preference.

Well, no, then let's go with 1 (with the dash) I would say.

Jan


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

Re: [PATCH 2/4] iommu: generalize iommu_inclusive_mapping

[Paul Durrant]

> -----Original Message-----
> From: Jan Beulich [mailto:JBeulich@suse.com]
> Sent: 31 July 2018 10:42
> To: Roger Pau Monne <roger.pau@citrix.com>
> Cc: Julien Grall <julien.grall@arm.com>; Andrew Cooper
> <Andrew.Cooper3@citrix.com>; George Dunlap
> <George.Dunlap@citrix.com>; Ian Jackson <Ian.Jackson@citrix.com>; Paul
> Durrant <Paul.Durrant@citrix.com>; Wei Liu <wei.liu2@citrix.com>; Kevin
> Tian <kevin.tian@intel.com>; Stefano Stabellini <sstabellini@kernel.org>;
> xen-devel <xen-devel@lists.xenproject.org>; Tim (Xen.org) <tim@xen.org>
> Subject: Re: [Xen-devel] [PATCH 2/4] iommu: generalize
> iommu_inclusive_mapping
> 
> >>> On 31.07.18 at 11:34, <roger.pau@citrix.com> wrote:
> > On Tue, Jul 31, 2018 at 03:14:47AM -0600, Jan Beulich wrote:
> >> >>> On 31.07.18 at 11:05, <roger.pau@citrix.com> wrote:
> >> > On Tue, Jul 31, 2018 at 02:49:19AM -0600, Jan Beulich wrote:
> >> >> >>> On 31.07.18 at 10:37, <Paul.Durrant@citrix.com> wrote:
> >> >> >>  -----Original Message-----
> >> >> >> From: Roger Pau Monne
> >> >> >> Sent: 31 July 2018 09:34
> >> >> >> To: Paul Durrant <Paul.Durrant@citrix.com>
> >> >> >> Cc: xen-devel@lists.xenproject.org; Kevin Tian
> <kevin.tian@intel.com>;
> >> >> >> Stefano Stabellini <sstabellini@kernel.org>; Wei Liu
> <wei.liu2@citrix.com>;
> >> >> >> George Dunlap <George.Dunlap@citrix.com>; Andrew Cooper
> >> >> >> <Andrew.Cooper3@citrix.com>; Ian Jackson
> <Ian.Jackson@citrix.com>; Tim
> >> >> >> (Xen.org) <tim@xen.org>; Julien Grall <julien.grall@arm.com>; Jan
> Beulich
> >> >> >> <jbeulich@suse.com>
> >> >> >> Subject: Re: [Xen-devel] [PATCH 2/4] iommu: generalize
> >> >> >> iommu_inclusive_mapping
> >> >> >>
> >> >> >> On Tue, Jul 31, 2018 at 09:27:03AM +0100, Paul Durrant wrote:
> >> >> >> > > -----Original Message-----
> >> >> >> > > From: Roger Pau Monne
> >> >> >> > > Sent: 31 July 2018 09:16
> >> >> >> > > To: Paul Durrant <Paul.Durrant@citrix.com>
> >> >> >> > > Cc: xen-devel@lists.xenproject.org; Kevin Tian
> <kevin.tian@intel.com>;
> >> >> >> > > Stefano Stabellini <sstabellini@kernel.org>; Wei Liu
> >> >> >> <wei.liu2@citrix.com>;
> >> >> >> > > George Dunlap <George.Dunlap@citrix.com>; Andrew Cooper
> >> >> >> > > <Andrew.Cooper3@citrix.com>; Ian Jackson
> <Ian.Jackson@citrix.com>;
> >> >> >> Tim
> >> >> >> > > (Xen.org) <tim@xen.org>; Julien Grall <julien.grall@arm.com>;
> Jan
> >> >> >> Beulich
> >> >> >> > > <jbeulich@suse.com>
> >> >> >> > > Subject: Re: [Xen-devel] [PATCH 2/4] iommu: generalize
> >> >> >> > > iommu_inclusive_mapping
> >> >> >> > >
> >> >> >> > > On Tue, Jul 31, 2018 at 08:18:36AM +0100, Paul Durrant wrote:
> >> >> >> > > > > -----Original Message-----
> >> >> >> > > > > From: Xen-devel [mailto:xen-devel-
> bounces@lists.xenproject.org]
> >> >> >> On
> >> >> >> > > Behalf
> >> >> >> > > > > Of Roger Pau Monne
> >> >> >> > > > > Sent: 27 July 2018 16:32
> >> >> >> > > > > To: xen-devel@lists.xenproject.org
> >> >> >> > > > > Cc: Kevin Tian <kevin.tian@intel.com>; Stefano Stabellini
> >> >> >> > > > > <sstabellini@kernel.org>; Wei Liu <wei.liu2@citrix.com>;
> George
> >> >> >> Dunlap
> >> >> >> > > > > <George.Dunlap@citrix.com>; Andrew Cooper
> >> >> >> > > > > <Andrew.Cooper3@citrix.com>; Ian Jackson
> >> >> >> <Ian.Jackson@citrix.com>;
> >> >> >> > > Tim
> >> >> >> > > > > (Xen.org) <tim@xen.org>; Julien Grall
> <julien.grall@arm.com>; Jan
> >> >> >> > > Beulich
> >> >> >> > > > > <jbeulich@suse.com>; Roger Pau Monne
> <roger.pau@citrix.com>
> >> >> >> > > > > Subject: [Xen-devel] [PATCH 2/4] iommu: generalize
> >> >> >> > > > > iommu_inclusive_mapping
> >> >> >> > > > >
> >> >> >> > > > > Introduce a new iommu=inclusive generic option that
> supersedes
> >> >> >> > > > > iommu_inclusive_mapping. This should be a non-functional
> change
> >> >> >> on
> >> >> >> > > > > Intel hardware, while AMD hardware will gain the same
> functionality
> >> >> >> of
> >> >> >> > > > > mapping almost everything below the 4GB boundary.
> >> >> >> > > > >
> >> >> >> > > > > Note that is a noop for ARM hardware.
> >> >> >> > > > >
> >> >> >> > > > > Signed-off-by: Roger Pau Monné <roger.pau@citrix.com>
> >> >> >> > > > > ---
> >> >> >> > > > > 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: Kevin Tian <kevin.tian@intel.com>
> >> >> >> > > > > ---
> >> >> >> > > > >  docs/misc/xen-command-line.markdown   | 14 ++++++
> >> >> >> > > > >  xen/drivers/passthrough/arm/iommu.c   |  4 ++
> >> >> >> > > > >  xen/drivers/passthrough/iommu.c       |  6 +++
> >> >> >> > > > >  xen/drivers/passthrough/vtd/extern.h  |  2 -
> >> >> >> > > > >  xen/drivers/passthrough/vtd/iommu.c   |  6 ---
> >> >> >> > > > >  xen/drivers/passthrough/vtd/x86/vtd.c | 66 +-----------------
> -------
> >> >> >> > > > >  xen/drivers/passthrough/x86/iommu.c   | 70
> >> >> >> > > > > +++++++++++++++++++++++++++
> >> >> >> > > > >  xen/include/xen/iommu.h               |  2 +
> >> >> >> > > > >  8 files changed, 97 insertions(+), 73 deletions(-)
> >> >> >> > > > >
> >> >> >> > > > > diff --git a/docs/misc/xen-command-line.markdown
> >> >> >> b/docs/misc/xen-
> >> >> >> > > > > command-line.markdown
> >> >> >> > > > > index 65b4754418..91a8bfc9a6 100644
> >> >> >> > > > > --- a/docs/misc/xen-command-line.markdown
> >> >> >> > > > > +++ b/docs/misc/xen-command-line.markdown
> >> >> >> > > > > @@ -1198,6 +1198,17 @@ detection of systems known to
> misbehave
> >> >> >> > > upon
> >> >> >> > > > > accesses to that port.
> >> >> >> > > > >
> >> >> >> > > > >  >> Enable IOMMU debugging code (implies `verbose`).
> >> >> >> > > > >
> >> >> >> > > > > +> `inclusive`
> >> >> >> > > >
> >> >> >> > > > This is a dom0 (or hwdom) specific setting so perhaps dom0-
> inclusive?
> >> >> >> > > >
> >> >> >> > > > Actually the dom0 iommu options are starting to get unwieldy
> as they
> >> >> >> are
> >> >> >> > > conflated with the general host iommu options so I think it may
> be
> >> >> >> > > worthwhile splitting things out into a separate 'dom0-iommu='
> top level
> >> >> >> > > parameter at this stage. (My reasons are slightly selfish as I
> intend to
> >
> >> >> > add
> >> >> >> > > another dom0 iommu option to give it just reserved regions, to
> avoid
> >> >> >> > > unnecessary set-up if we know it will be using PV-IOMMU).
> >> >> >> > >
> >> >> >> > > Mapping just the reserved regions is what I actually do for PVH
> with
> >> >> >> > > iommu=inclusive (patch 4/4), so maybe it would make sense to
> speak
> >> >> >> about
> >> >> >> > > the
> >> >> >> > > naming here in order to use the same naming for PV and PVH.
> >> >> >> > >
> >> >> >> > > TBH I don't really like the dom0- prefix, the command line
> iommu
> >> >> >> > > options either apply to all domains or Dom0 only, having
> >> >> >> > > domu-inclusive for example makes no sense IMO.
> >> >> >> >
> >> >> >> > No, I think there are some options that you may want to apply to
> dom0
> >> >> >> only, but these are more like the dom0_mem or dom0_max_vpus
> options.
> >> >> >> Particularly, the inclusive option is probably something that is only
> >> > desirable
> >> >> >> for dom0. Clearly dom0-passthrough and dom0-strict are already
> defined to
> >> >> >> relate to dom0 only, and options such as 'reserved' should only be
> > specific
> >> > on
> >> >> >> the command line in relation to dom0 IMO. For other domains such
> an option
> >> >> >> should be specified via xl.cfg.
> >> >> >>
> >> >> >> Yes, we already have a bunch of those, so then I think dom0-
> inclusive
> >> >> >> and dom0-reserved would be appropriate?
> >> >> >>
> >> >> >> dom0-inclusive-mapping or dom0-reserved-mapping seems too
> long.
> >> >> >
> >> >> > Yes, those names are ok, but I still think it better in the long run if we
> >
> >> >> > have something like:
> >> >> >
> >> >> > dom0_iommu=[inclusive,][reserved,][strict,][none,][relaxed]
> >> >> >
> >> >> > where relaxed is the default and 'none' (I think) is equivalent to the
> >> >> > current iommu=dom0-passthrough.
> >> >>
> >> >> Or, along the lines of the other reply just sent, e.g.
> >> >>
> >> >> dom0=pvh,iommu:inclusive;reserved,shadow
> >> >>
> >> >> But perhaps the difference between , and ; gets too confusing then.
> >> >
> >> > So I think we have the following options:
> >> >
> >> > 1. dom0_iommu=[inclusive,][reserved,][strict,][none,][relaxed]
> >>
> >> Nit: dom0-iommu= (no underscores in new options)
> >>
> >> > 2.
> dom0=[pvh,][shadow,][iommu=[inclusive;][reserved;][strict;][none;][relaxe
> d]]
> >> > 3. dom0=[pvh,][shadow,][iommu-inclusive,][iommu-reserved,][iommu-
> strict,][iommu-none,][iommu-relaxed]
> >> >
> >> > I don't have a strong preference between 1 and 3, but I would prefer
> >> > to avoid 2 because I think suboptions inside of options it's too
> >> > complex IMO.
> >>
> >> While generally I prefer to limit the number of top level options, in
> >> this case I think I'd prefer 1 after all. Or wait - does any pair of the
> >> (sub)options actually make sense to be specified?
> >
> > Yes, for example you can use strict and inclusive at the same time, I
> > think it's something like:
> >
> >
> dom0=[pvh,][shadow,][iommu=[inclusive|reserved;][strict|none|relaxed]]
> >
> >> Isn't it rather a
> >> choice of five than an enumeration of up to 5? In which case I'd
> >> still prefer 2 (as then there's no need for a second separator
> >> beside comma), the more that we have at least one example with
> >> such sub-options (cpufreq).
> >
> > OK, I can do the nested iommu option inside of dom0 if that's the
> > preference.
> 
> Well, no, then let's go with 1 (with the dash) I would say.
> 

+1

  Paul

> Jan

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

Re: [PATCH 2/4] iommu: generalize iommu_inclusive_mapping

[Jan Beulich]

>>> On 31.07.18 at 10:27, <Paul.Durrant@citrix.com> wrote:
>>  -----Original Message-----
>> From: Roger Pau Monne
>> Sent: 31 July 2018 09:16
>> To: Paul Durrant <Paul.Durrant@citrix.com>
>> Cc: xen-devel@lists.xenproject.org; Kevin Tian <kevin.tian@intel.com>;
>> Stefano Stabellini <sstabellini@kernel.org>; Wei Liu <wei.liu2@citrix.com>;
>> George Dunlap <George.Dunlap@citrix.com>; Andrew Cooper
>> <Andrew.Cooper3@citrix.com>; Ian Jackson <Ian.Jackson@citrix.com>; Tim
>> (Xen.org) <tim@xen.org>; Julien Grall <julien.grall@arm.com>; Jan Beulich
>> <jbeulich@suse.com>
>> Subject: Re: [Xen-devel] [PATCH 2/4] iommu: generalize
>> iommu_inclusive_mapping
>> 
>> On Tue, Jul 31, 2018 at 08:18:36AM +0100, Paul Durrant wrote:
>> > > -----Original Message-----
>> > > From: Xen-devel [mailto:xen-devel-bounces@lists.xenproject.org] On
>> Behalf
>> > > Of Roger Pau Monne
>> > > Sent: 27 July 2018 16:32
>> > > To: xen-devel@lists.xenproject.org 
>> > > Cc: Kevin Tian <kevin.tian@intel.com>; Stefano Stabellini
>> > > <sstabellini@kernel.org>; Wei Liu <wei.liu2@citrix.com>; George Dunlap
>> > > <George.Dunlap@citrix.com>; Andrew Cooper
>> > > <Andrew.Cooper3@citrix.com>; Ian Jackson <Ian.Jackson@citrix.com>;
>> Tim
>> > > (Xen.org) <tim@xen.org>; Julien Grall <julien.grall@arm.com>; Jan
>> Beulich
>> > > <jbeulich@suse.com>; Roger Pau Monne <roger.pau@citrix.com>
>> > > Subject: [Xen-devel] [PATCH 2/4] iommu: generalize
>> > > iommu_inclusive_mapping
>> > >
>> > > Introduce a new iommu=inclusive generic option that supersedes
>> > > iommu_inclusive_mapping. This should be a non-functional change on
>> > > Intel hardware, while AMD hardware will gain the same functionality of
>> > > mapping almost everything below the 4GB boundary.
>> > >
>> > > Note that is a noop for ARM hardware.
>> > >
>> > > Signed-off-by: Roger Pau Monné <roger.pau@citrix.com>
>> > > ---
>> > > 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: Kevin Tian <kevin.tian@intel.com>
>> > > ---
>> > >  docs/misc/xen-command-line.markdown   | 14 ++++++
>> > >  xen/drivers/passthrough/arm/iommu.c   |  4 ++
>> > >  xen/drivers/passthrough/iommu.c       |  6 +++
>> > >  xen/drivers/passthrough/vtd/extern.h  |  2 -
>> > >  xen/drivers/passthrough/vtd/iommu.c   |  6 ---
>> > >  xen/drivers/passthrough/vtd/x86/vtd.c | 66 +------------------------
>> > >  xen/drivers/passthrough/x86/iommu.c   | 70
>> > > +++++++++++++++++++++++++++
>> > >  xen/include/xen/iommu.h               |  2 +
>> > >  8 files changed, 97 insertions(+), 73 deletions(-)
>> > >
>> > > diff --git a/docs/misc/xen-command-line.markdown b/docs/misc/xen-
>> > > command-line.markdown
>> > > index 65b4754418..91a8bfc9a6 100644
>> > > --- a/docs/misc/xen-command-line.markdown
>> > > +++ b/docs/misc/xen-command-line.markdown
>> > > @@ -1198,6 +1198,17 @@ detection of systems known to misbehave
>> upon
>> > > accesses to that port.
>> > >
>> > >  >> Enable IOMMU debugging code (implies `verbose`).
>> > >
>> > > +> `inclusive`
>> >
>> > This is a dom0 (or hwdom) specific setting so perhaps dom0-inclusive?
>> >
>> > Actually the dom0 iommu options are starting to get unwieldy as they are
>> conflated with the general host iommu options so I think it may be
>> worthwhile splitting things out into a separate 'dom0-iommu=' top level
>> parameter at this stage. (My reasons are slightly selfish as I intend to add
>> another dom0 iommu option to give it just reserved regions, to avoid
>> unnecessary set-up if we know it will be using PV-IOMMU).
>> 
>> Mapping just the reserved regions is what I actually do for PVH with
>> iommu=inclusive (patch 4/4), so maybe it would make sense to speak about
>> the
>> naming here in order to use the same naming for PV and PVH.
>> 
>> TBH I don't really like the dom0- prefix, the command line iommu
>> options either apply to all domains or Dom0 only, having
>> domu-inclusive for example makes no sense IMO.
> 
> No, I think there are some options that you may want to apply to dom0 only, 
> but these are more like the dom0_mem or dom0_max_vpus options. Particularly, 
> the inclusive option is probably something that is only desirable for dom0. 
> Clearly dom0-passthrough and dom0-strict are already defined to relate to dom0 
> only, and options such as 'reserved' should only be specific on the command 
> line in relation to dom0 IMO. For other domains such an option should be 
> specified via xl.cfg.

So perhaps "dom0=iommu-inclusive" etc?

Jan


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

Re: [PATCH 2/4] iommu: generalize iommu_inclusive_mapping

[Jan Beulich]

>>> On 27.07.18 at 17:31, <roger.pau@citrix.com> wrote:
> Introduce a new iommu=inclusive generic option that supersedes
> iommu_inclusive_mapping. This should be a non-functional change on
> Intel hardware, while AMD hardware will gain the same functionality of
> mapping almost everything below the 4GB boundary.

So first of all - what's the motivation behind this change? So far we
had no need for hacks line the VT-d side one on AMD. I don't think
this should be widened without there being indication of a problem
with non-niche AMD systems.

> --- a/xen/drivers/passthrough/x86/iommu.c
> +++ b/xen/drivers/passthrough/x86/iommu.c
> @@ -20,6 +20,8 @@
>  #include <xen/softirq.h>
>  #include <xsm/xsm.h>
>  
> +#include <asm/setup.h>

Why? And if it's needed here now, can it be dropped from the VT-d
file where you removed the code?

> @@ -132,6 +134,74 @@ void arch_iommu_domain_destroy(struct domain *d)
>  {
>  }
>  
> +void __hwdom_init arch_iommu_hwdom_init(struct domain *d)
> +{
> +    unsigned long i, j, tmp, top, max_pfn;
> +
> +    if ( iommu_passthrough || !is_pv_domain(d) )
> +        return;
> +
> +    BUG_ON(!is_hardware_domain(d));
> +
> +    max_pfn = (GB(4) >> PAGE_SHIFT) - 1;
> +    top = max(max_pdx, pfn_to_pdx(max_pfn) + 1);
> +
> +    for ( i = 0; i < top; i++ )
> +    {
> +        unsigned long pfn = pdx_to_pfn(i);
> +        bool map;
> +        int rc = 0;
> +
> +        /*
> +         * Set up 1:1 mapping for dom0. Default to include only
> +         * conventional RAM areas and let RMRRs include needed reserved
> +         * regions. When set, the inclusive mapping additionally maps in
> +         * every pfn up to 4GB except those that fall in unusable ranges.
> +         */
> +        if ( pfn > max_pfn && !mfn_valid(_mfn(pfn)) )
> +            continue;
> +
> +        if ( iommu_inclusive && pfn <= max_pfn )
> +            map = !page_is_ram_type(pfn, RAM_TYPE_UNUSABLE);
> +        else
> +            map = page_is_ram_type(pfn, RAM_TYPE_CONVENTIONAL);
> +
> +        if ( !map )
> +            continue;
> +
> +        /* Exclude Xen bits */
> +        if ( xen_in_range(pfn) )
> +            continue;
> +
> +        /*
> +         * If dom0-strict mode is enabled then exclude conventional RAM
> +         * and let the common code map dom0's pages.
> +         */
> +        if ( iommu_dom0_strict &&
> +             page_is_ram_type(pfn, RAM_TYPE_CONVENTIONAL) )
> +            continue;
> +
> +        tmp = 1 << (PAGE_SHIFT - PAGE_SHIFT_4K);
> +        for ( j = 0; j < tmp; j++ )
> +        {
> +            int ret = iommu_map_page(d, pfn * tmp + j, pfn * tmp + j,
> +                                     IOMMUF_readable|IOMMUF_writable);
> +
> +            if ( !rc )
> +               rc = ret;
> +        }

To VT-d specific code was this way to also cope with ia64. I don't
see the need for this to be a loop when the code is now x86-
specific.

> +        if ( rc )
> +            printk(XENLOG_WARNING "d%d: IOMMU mapping failed: %d\n",
> +                   d->domain_id, rc);
> +
> +        if (!(i & (0xfffff >> (PAGE_SHIFT - PAGE_SHIFT_4K))))

Same here for the unnecessary shifting.

> +            process_pending_softirqs();
> +    }
> +
> +
> +}

Double blank line above here when there should be none at all.

Jan



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

Re: [PATCH 2/4] iommu: generalize iommu_inclusive_mapping

[Roger Pau Monné]

On Tue, Jul 31, 2018 at 08:39:22AM -0600, Jan Beulich wrote:
> >>> On 27.07.18 at 17:31, <roger.pau@citrix.com> wrote:
> > Introduce a new iommu=inclusive generic option that supersedes
> > iommu_inclusive_mapping. This should be a non-functional change on
> > Intel hardware, while AMD hardware will gain the same functionality of
> > mapping almost everything below the 4GB boundary.
> 
> So first of all - what's the motivation behind this change? So far we
> had no need for hacks line the VT-d side one on AMD. I don't think
> this should be widened without there being indication of a problem
> with non-niche AMD systems.

OK, I can leave the default on for Intel and off for everything else,
but I will introduce the generic dom0-iommu= option anyway.

> > --- a/xen/drivers/passthrough/x86/iommu.c
> > +++ b/xen/drivers/passthrough/x86/iommu.c
> > @@ -20,6 +20,8 @@
> >  #include <xen/softirq.h>
> >  #include <xsm/xsm.h>
> >  
> > +#include <asm/setup.h>
> 
> Why? And if it's needed here now, can it be dropped from the VT-d
> file where you removed the code?

setup.h is needed for xen_in_range. And yes, I can drop it from the
VTd code then.

> > @@ -132,6 +134,74 @@ void arch_iommu_domain_destroy(struct domain *d)
> >  {
> >  }
> >  
> > +void __hwdom_init arch_iommu_hwdom_init(struct domain *d)
> > +{
> > +    unsigned long i, j, tmp, top, max_pfn;
> > +
> > +    if ( iommu_passthrough || !is_pv_domain(d) )
> > +        return;
> > +
> > +    BUG_ON(!is_hardware_domain(d));
> > +
> > +    max_pfn = (GB(4) >> PAGE_SHIFT) - 1;
> > +    top = max(max_pdx, pfn_to_pdx(max_pfn) + 1);
> > +
> > +    for ( i = 0; i < top; i++ )
> > +    {
> > +        unsigned long pfn = pdx_to_pfn(i);
> > +        bool map;
> > +        int rc = 0;
> > +
> > +        /*
> > +         * Set up 1:1 mapping for dom0. Default to include only
> > +         * conventional RAM areas and let RMRRs include needed reserved
> > +         * regions. When set, the inclusive mapping additionally maps in
> > +         * every pfn up to 4GB except those that fall in unusable ranges.
> > +         */
> > +        if ( pfn > max_pfn && !mfn_valid(_mfn(pfn)) )
> > +            continue;
> > +
> > +        if ( iommu_inclusive && pfn <= max_pfn )
> > +            map = !page_is_ram_type(pfn, RAM_TYPE_UNUSABLE);
> > +        else
> > +            map = page_is_ram_type(pfn, RAM_TYPE_CONVENTIONAL);
> > +
> > +        if ( !map )
> > +            continue;
> > +
> > +        /* Exclude Xen bits */
> > +        if ( xen_in_range(pfn) )
> > +            continue;
> > +
> > +        /*
> > +         * If dom0-strict mode is enabled then exclude conventional RAM
> > +         * and let the common code map dom0's pages.
> > +         */
> > +        if ( iommu_dom0_strict &&
> > +             page_is_ram_type(pfn, RAM_TYPE_CONVENTIONAL) )
> > +            continue;
> > +
> > +        tmp = 1 << (PAGE_SHIFT - PAGE_SHIFT_4K);
> > +        for ( j = 0; j < tmp; j++ )
> > +        {
> > +            int ret = iommu_map_page(d, pfn * tmp + j, pfn * tmp + j,
> > +                                     IOMMUF_readable|IOMMUF_writable);
> > +
> > +            if ( !rc )
> > +               rc = ret;
> > +        }
> 
> To VT-d specific code was this way to also cope with ia64. I don't
> see the need for this to be a loop when the code is now x86-
> specific.

Oh, I wondered about this and TBH I couldn't figure out why it's this
way, now I understand.

Thanks, Roger.

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

Re: [PATCH 2/4] iommu: generalize iommu_inclusive_mapping

[Jan Beulich]

>>> On 31.07.18 at 17:33, <roger.pau@citrix.com> wrote:
> On Tue, Jul 31, 2018 at 08:39:22AM -0600, Jan Beulich wrote:
>> >>> On 27.07.18 at 17:31, <roger.pau@citrix.com> wrote:
>> > Introduce a new iommu=inclusive generic option that supersedes
>> > iommu_inclusive_mapping. This should be a non-functional change on
>> > Intel hardware, while AMD hardware will gain the same functionality of
>> > mapping almost everything below the 4GB boundary.
>> 
>> So first of all - what's the motivation behind this change? So far we
>> had no need for hacks line the VT-d side one on AMD. I don't think
>> this should be widened without there being indication of a problem
>> with non-niche AMD systems.
> 
> OK, I can leave the default on for Intel and off for everything else,
> but I will introduce the generic dom0-iommu= option anyway.

Hmm, I've always been wishing we'd change to a default of off for
VT-d as well - imo we shouldn't by default assume broken firmware.
Kevin?

As to AMD - you still don't really say why this would be needed
there. I'm not in favor of workarounds when there's nothing to
work around. IOW - if the logic isn't needed on AMD, do we need
this code movement in the first place?

Jan



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

Re: [PATCH 2/4] iommu: generalize iommu_inclusive_mapping

[Andrew Cooper]

On 01/08/2018 09:20, Jan Beulich wrote:
>>>> On 31.07.18 at 17:33, <roger.pau@citrix.com> wrote:
>> On Tue, Jul 31, 2018 at 08:39:22AM -0600, Jan Beulich wrote:
>>>>>> On 27.07.18 at 17:31, <roger.pau@citrix.com> wrote:
>>>> Introduce a new iommu=inclusive generic option that supersedes
>>>> iommu_inclusive_mapping. This should be a non-functional change on
>>>> Intel hardware, while AMD hardware will gain the same functionality of
>>>> mapping almost everything below the 4GB boundary.
>>> So first of all - what's the motivation behind this change? So far we
>>> had no need for hacks line the VT-d side one on AMD. I don't think
>>> this should be widened without there being indication of a problem
>>> with non-niche AMD systems.
>> OK, I can leave the default on for Intel and off for everything else,
>> but I will introduce the generic dom0-iommu= option anyway.
> Hmm, I've always been wishing we'd change to a default of off for
> VT-d as well - imo we shouldn't by default assume broken firmware.
> Kevin?
>
> As to AMD - you still don't really say why this would be needed
> there. I'm not in favor of workarounds when there's nothing to
> work around. IOW - if the logic isn't needed on AMD, do we need
> this code movement in the first place?

This is sufficiently broken on AMD as well.  Take a look at all the
IVRS/IVHT reports we've had on xen-devel.

Furthermore, on by default is the only reasonable option.  Dom0 is
specifically given cpu mappings of the reserved regions, therefore it
should have matching IOMMU mappings.

~Andrew

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

Re: [PATCH 2/4] iommu: generalize iommu_inclusive_mapping

[Jan Beulich]

>>> On 01.08.18 at 10:32, <andrew.cooper3@citrix.com> wrote:
> On 01/08/2018 09:20, Jan Beulich wrote:
>>>>> On 31.07.18 at 17:33, <roger.pau@citrix.com> wrote:
>>> On Tue, Jul 31, 2018 at 08:39:22AM -0600, Jan Beulich wrote:
>>>>>>> On 27.07.18 at 17:31, <roger.pau@citrix.com> wrote:
>>>>> Introduce a new iommu=inclusive generic option that supersedes
>>>>> iommu_inclusive_mapping. This should be a non-functional change on
>>>>> Intel hardware, while AMD hardware will gain the same functionality of
>>>>> mapping almost everything below the 4GB boundary.
>>>> So first of all - what's the motivation behind this change? So far we
>>>> had no need for hacks line the VT-d side one on AMD. I don't think
>>>> this should be widened without there being indication of a problem
>>>> with non-niche AMD systems.
>>> OK, I can leave the default on for Intel and off for everything else,
>>> but I will introduce the generic dom0-iommu= option anyway.
>> Hmm, I've always been wishing we'd change to a default of off for
>> VT-d as well - imo we shouldn't by default assume broken firmware.
>> Kevin?
>>
>> As to AMD - you still don't really say why this would be needed
>> there. I'm not in favor of workarounds when there's nothing to
>> work around. IOW - if the logic isn't needed on AMD, do we need
>> this code movement in the first place?
> 
> This is sufficiently broken on AMD as well.  Take a look at all the
> IVRS/IVHT reports we've had on xen-devel.

We've had IVRS issues, yes, and we now have command line options
to deal with them. That's entirely unrelated to memory ranges though.

> Furthermore, on by default is the only reasonable option.  Dom0 is
> specifically given cpu mappings of the reserved regions, therefore it
> should have matching IOMMU mappings.

I don't think I can uniformly agree with this: There are reserved
regions which specifically would better not have IOMMU mappings:
LAPIC, IO-APIC, MMCFG just to name things I can immediately
think of.

Jan



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

Re: [PATCH 2/4] iommu: generalize iommu_inclusive_mapping

[Andrew Cooper]

On 01/08/2018 10:10, Jan Beulich wrote:
>> Furthermore, on by default is the only reasonable option.  Dom0 is
>> specifically given cpu mappings of the reserved regions, therefore it
>> should have matching IOMMU mappings.
> I don't think I can uniformly agree with this: There are reserved
> regions which specifically would better not have IOMMU mappings:
> LAPIC, IO-APIC, MMCFG just to name things I can immediately
> think of.

Right, but that's not what I said.

For any CPU mapping, we should have a matching IOMMU mapping.  This is
how hardware behaves in a native system, and is how all operating
systems are written to behave.  Breaking this expectation will break
software in unexpected ways.

For the items you list there, we don't give dom0 CPU mappings (because
restricting them is indeed a sensible thing), and the "matching" mapping
in the IOMMU is also not present.

~Andrew

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

Re: [PATCH 2/4] iommu: generalize iommu_inclusive_mapping

[Jan Beulich]

>>> On 01.08.18 at 11:20, <andrew.cooper3@citrix.com> wrote:
> On 01/08/2018 10:10, Jan Beulich wrote:
>>> Furthermore, on by default is the only reasonable option.  Dom0 is
>>> specifically given cpu mappings of the reserved regions, therefore it
>>> should have matching IOMMU mappings.
>> I don't think I can uniformly agree with this: There are reserved
>> regions which specifically would better not have IOMMU mappings:
>> LAPIC, IO-APIC, MMCFG just to name things I can immediately
>> think of.
> 
> Right, but that's not what I said.
> 
> For any CPU mapping, we should have a matching IOMMU mapping.  This is
> how hardware behaves in a native system, and is how all operating
> systems are written to behave.  Breaking this expectation will break
> software in unexpected ways.

Well, LAPIC is a clear exception to this rule, as from devices' point
of view the 0xFEExxxxx range has a completely different meaning.

> For the items you list there, we don't give dom0 CPU mappings (because
> restricting them is indeed a sensible thing), and the "matching" mapping
> in the IOMMU is also not present.

Not exactly: We still emulate those ranges for CPU side accesses,
while we don't emulate IOMMU / device side ones. So there's a
difference to native hardware anyway.

Jan



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

Re: [PATCH 2/4] iommu: generalize iommu_inclusive_mapping

[Andrew Cooper]

On 01/08/18 10:59, Jan Beulich wrote:
>>>> On 01.08.18 at 11:20, <andrew.cooper3@citrix.com> wrote:
>> On 01/08/2018 10:10, Jan Beulich wrote:
>>>> Furthermore, on by default is the only reasonable option.  Dom0 is
>>>> specifically given cpu mappings of the reserved regions, therefore it
>>>> should have matching IOMMU mappings.
>>> I don't think I can uniformly agree with this: There are reserved
>>> regions which specifically would better not have IOMMU mappings:
>>> LAPIC, IO-APIC, MMCFG just to name things I can immediately
>>> think of.
>> Right, but that's not what I said.
>>
>> For any CPU mapping, we should have a matching IOMMU mapping.  This is
>> how hardware behaves in a native system, and is how all operating
>> systems are written to behave.  Breaking this expectation will break
>> software in unexpected ways.
> Well, LAPIC is a clear exception to this rule, as from devices' point
> of view the 0xFEExxxxx range has a completely different meaning.

And furthermore is magic anyway when it comes to DMA's hitting that region.

>
>> For the items you list there, we don't give dom0 CPU mappings (because
>> restricting them is indeed a sensible thing), and the "matching" mapping
>> in the IOMMU is also not present.
> Not exactly: We still emulate those ranges for CPU side accesses,
> while we don't emulate IOMMU / device side ones. So there's a
> difference to native hardware anyway.

We can't emulate the IOMMU side at all, nor would we want to in the
context of disallowing dom0 access to those bits of real hardware.

~Andrew

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

Re: [PATCH 2/4] iommu: generalize iommu_inclusive_mapping

[Paul Durrant]

> -----Original Message-----
> From: Xen-devel [mailto:xen-devel-bounces@lists.xenproject.org] On Behalf
> Of Jan Beulich
> Sent: 01 August 2018 09:21
> To: Roger Pau Monne <roger.pau@citrix.com>
> Cc: Kevin Tian <kevin.tian@intel.com>; Stefano Stabellini
> <sstabellini@kernel.org>; Wei Liu <wei.liu2@citrix.com>; George Dunlap
> <George.Dunlap@citrix.com>; Andrew Cooper
> <Andrew.Cooper3@citrix.com>; Ian Jackson <Ian.Jackson@citrix.com>; Tim
> (Xen.org) <tim@xen.org>; Julien Grall <julien.grall@arm.com>; xen-devel
> <xen-devel@lists.xenproject.org>
> Subject: Re: [Xen-devel] [PATCH 2/4] iommu: generalize
> iommu_inclusive_mapping
> 
> >>> On 31.07.18 at 17:33, <roger.pau@citrix.com> wrote:
> > On Tue, Jul 31, 2018 at 08:39:22AM -0600, Jan Beulich wrote:
> >> >>> On 27.07.18 at 17:31, <roger.pau@citrix.com> wrote:
> >> > Introduce a new iommu=inclusive generic option that supersedes
> >> > iommu_inclusive_mapping. This should be a non-functional change on
> >> > Intel hardware, while AMD hardware will gain the same functionality of
> >> > mapping almost everything below the 4GB boundary.
> >>
> >> So first of all - what's the motivation behind this change? So far we
> >> had no need for hacks line the VT-d side one on AMD. I don't think
> >> this should be widened without there being indication of a problem
> >> with non-niche AMD systems.
> >
> > OK, I can leave the default on for Intel and off for everything else,
> > but I will introduce the generic dom0-iommu= option anyway.
> 
> Hmm, I've always been wishing we'd change to a default of off for
> VT-d as well - imo we shouldn't by default assume broken firmware.
> Kevin?

I suspect that a seriously large number of Xen users will find their systems fail to boot if the default is changed. I'm testing on a Dell R730 with (to my knowledge) up-to-date firmware. Not exactly a rare system and turning off inclusive mapping will cause it to wedge during boot.

  Paul

> 
> As to AMD - you still don't really say why this would be needed
> there. I'm not in favor of workarounds when there's nothing to
> work around. IOW - if the logic isn't needed on AMD, do we need
> this code movement in the first place?
> 
> Jan
> 
> 
> 
> _______________________________________________
> Xen-devel mailing list
> Xen-devel@lists.xenproject.org
> https://lists.xenproject.org/mailman/listinfo/xen-devel
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel

Re: [PATCH 2/4] iommu: generalize iommu_inclusive_mapping

[Jan Beulich]

>>> On 01.08.18 at 10:33, <Paul.Durrant@citrix.com> wrote:
>> From: Xen-devel [mailto:xen-devel-bounces@lists.xenproject.org] On Behalf
>> Of Jan Beulich
>> Sent: 01 August 2018 09:21
>> >>> On 31.07.18 at 17:33, <roger.pau@citrix.com> wrote:
>> > On Tue, Jul 31, 2018 at 08:39:22AM -0600, Jan Beulich wrote:
>> >> >>> On 27.07.18 at 17:31, <roger.pau@citrix.com> wrote:
>> >> > Introduce a new iommu=inclusive generic option that supersedes
>> >> > iommu_inclusive_mapping. This should be a non-functional change on
>> >> > Intel hardware, while AMD hardware will gain the same functionality of
>> >> > mapping almost everything below the 4GB boundary.
>> >>
>> >> So first of all - what's the motivation behind this change? So far we
>> >> had no need for hacks line the VT-d side one on AMD. I don't think
>> >> this should be widened without there being indication of a problem
>> >> with non-niche AMD systems.
>> >
>> > OK, I can leave the default on for Intel and off for everything else,
>> > but I will introduce the generic dom0-iommu= option anyway.
>> 
>> Hmm, I've always been wishing we'd change to a default of off for
>> VT-d as well - imo we shouldn't by default assume broken firmware.
>> Kevin?
> 
> I suspect that a seriously large number of Xen users will find their systems 
> fail to boot if the default is changed. I'm testing on a Dell R730 with (to 
> my knowledge) up-to-date firmware. Not exactly a rare system and turning off 
> inclusive mapping will cause it to wedge during boot.

Hmm, that's rather sad indeed.

Jan



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

Re: [PATCH 2/4] iommu: generalize iommu_inclusive_mapping

[Tian, Kevin]

> From: Paul Durrant [mailto:Paul.Durrant@citrix.com]
> Sent: Wednesday, August 1, 2018 4:34 PM
> 
> > -----Original Message-----
> > From: Xen-devel [mailto:xen-devel-bounces@lists.xenproject.org] On
> Behalf
> > Of Jan Beulich
> > Sent: 01 August 2018 09:21
> > To: Roger Pau Monne <roger.pau@citrix.com>
> > Cc: Kevin Tian <kevin.tian@intel.com>; Stefano Stabellini
> > <sstabellini@kernel.org>; Wei Liu <wei.liu2@citrix.com>; George Dunlap
> > <George.Dunlap@citrix.com>; Andrew Cooper
> > <Andrew.Cooper3@citrix.com>; Ian Jackson <Ian.Jackson@citrix.com>;
> Tim
> > (Xen.org) <tim@xen.org>; Julien Grall <julien.grall@arm.com>; xen-devel
> > <xen-devel@lists.xenproject.org>
> > Subject: Re: [Xen-devel] [PATCH 2/4] iommu: generalize
> > iommu_inclusive_mapping
> >
> > >>> On 31.07.18 at 17:33, <roger.pau@citrix.com> wrote:
> > > On Tue, Jul 31, 2018 at 08:39:22AM -0600, Jan Beulich wrote:
> > >> >>> On 27.07.18 at 17:31, <roger.pau@citrix.com> wrote:
> > >> > Introduce a new iommu=inclusive generic option that supersedes
> > >> > iommu_inclusive_mapping. This should be a non-functional change
> on
> > >> > Intel hardware, while AMD hardware will gain the same functionality
> of
> > >> > mapping almost everything below the 4GB boundary.
> > >>
> > >> So first of all - what's the motivation behind this change? So far we
> > >> had no need for hacks line the VT-d side one on AMD. I don't think
> > >> this should be widened without there being indication of a problem
> > >> with non-niche AMD systems.
> > >
> > > OK, I can leave the default on for Intel and off for everything else,
> > > but I will introduce the generic dom0-iommu= option anyway.
> >
> > Hmm, I've always been wishing we'd change to a default of off for
> > VT-d as well - imo we shouldn't by default assume broken firmware.
> > Kevin?
> 
> I suspect that a seriously large number of Xen users will find their systems
> fail to boot if the default is changed. I'm testing on a Dell R730 with (to my
> knowledge) up-to-date firmware. Not exactly a rare system and turning off
> inclusive mapping will cause it to wedge during boot.
> 
>   Paul

Though I agree with Jan in concept, the reality means we have to
keep default on as before. :-)

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

Re: [PATCH 2/4] iommu: generalize iommu_inclusive_mapping

[Roger Pau Monné]

On Wed, Aug 01, 2018 at 02:20:36AM -0600, Jan Beulich wrote:
> >>> On 31.07.18 at 17:33, <roger.pau@citrix.com> wrote:
> > On Tue, Jul 31, 2018 at 08:39:22AM -0600, Jan Beulich wrote:
> >> >>> On 27.07.18 at 17:31, <roger.pau@citrix.com> wrote:
> >> > Introduce a new iommu=inclusive generic option that supersedes
> >> > iommu_inclusive_mapping. This should be a non-functional change on
> >> > Intel hardware, while AMD hardware will gain the same functionality of
> >> > mapping almost everything below the 4GB boundary.
> >> 
> >> So first of all - what's the motivation behind this change? So far we
> >> had no need for hacks line the VT-d side one on AMD. I don't think
> >> this should be widened without there being indication of a problem
> >> with non-niche AMD systems.
> > 
> > OK, I can leave the default on for Intel and off for everything else,
> > but I will introduce the generic dom0-iommu= option anyway.
> 
> Hmm, I've always been wishing we'd change to a default of off for
> VT-d as well - imo we shouldn't by default assume broken firmware.
> Kevin?

I'm all for it, but even in newish systems (as recently reported by
people trying PVH Dom0) RMRR entries are missing and will make some
devices unusable.

> As to AMD - you still don't really say why this would be needed
> there. I'm not in favor of workarounds when there's nothing to
> work around. IOW - if the logic isn't needed on AMD, do we need
> this code movement in the first place?

There's nothing VTd specific in this code at all, I could see it
living in a VTd specific file if it had quirks specific to VTd, but I
think the code is generic enough that it warrants being shared, so
that people running on AMD hw can also enable it if required.

As requested I've left this off for AMD and on by default on Intel, so
the default functionality is not changed at all.

Thanks, Roger.

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

[PATCH 3/4] x86/iommu: reorder conditions used in the inclusive iommu mappings

[Roger Pau Monne]

In order to place all the map conditions in a single if ... else
conditional.

No functional change.

Signed-off-by: Roger Pau Monné <roger.pau@citrix.com>
---
Cc: Jan Beulich <jbeulich@suse.com>
---
 xen/drivers/passthrough/x86/iommu.c | 23 ++++++++++-------------
 1 file changed, 10 insertions(+), 13 deletions(-)

diff --git a/xen/drivers/passthrough/x86/iommu.c b/xen/drivers/passthrough/x86/iommu.c
index ba0bbd9a15..24cc591aa5 100644
--- a/xen/drivers/passthrough/x86/iommu.c
+++ b/xen/drivers/passthrough/x86/iommu.c
@@ -158,19 +158,9 @@ void __hwdom_init arch_iommu_hwdom_init(struct domain *d)
          * regions. When set, the inclusive mapping additionally maps in
          * every pfn up to 4GB except those that fall in unusable ranges.
          */
-        if ( pfn > max_pfn && !mfn_valid(_mfn(pfn)) )
-            continue;
-
-        if ( iommu_inclusive && pfn <= max_pfn )
-            map = !page_is_ram_type(pfn, RAM_TYPE_UNUSABLE);
-        else
-            map = page_is_ram_type(pfn, RAM_TYPE_CONVENTIONAL);
-
-        if ( !map )
-            continue;
-
-        /* Exclude Xen bits */
-        if ( xen_in_range(pfn) )
+        if ( (pfn > max_pfn && !mfn_valid(_mfn(pfn))) ||
+             /* Exclude Xen bits */
+             xen_in_range(pfn) )
             continue;
 
         /*
@@ -179,6 +169,13 @@ void __hwdom_init arch_iommu_hwdom_init(struct domain *d)
          */
         if ( iommu_dom0_strict &&
              page_is_ram_type(pfn, RAM_TYPE_CONVENTIONAL) )
+            map = false;
+        else if ( iommu_inclusive && pfn <= max_pfn )
+            map = !page_is_ram_type(pfn, RAM_TYPE_UNUSABLE);
+        else
+            map = page_is_ram_type(pfn, RAM_TYPE_CONVENTIONAL);
+
+        if ( !map )
             continue;
 
         tmp = 1 << (PAGE_SHIFT - PAGE_SHIFT_4K);
-- 
2.18.0


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

Re: [PATCH 3/4] x86/iommu: reorder conditions used in the inclusive iommu mappings

[Paul Durrant]

> -----Original Message-----
> From: Xen-devel [mailto:xen-devel-bounces@lists.xenproject.org] On Behalf
> Of Roger Pau Monne
> Sent: 27 July 2018 16:32
> To: xen-devel@lists.xenproject.org
> Cc: Jan Beulich <jbeulich@suse.com>; Roger Pau Monne
> <roger.pau@citrix.com>
> Subject: [Xen-devel] [PATCH 3/4] x86/iommu: reorder conditions used in the
> inclusive iommu mappings
> 
> In order to place all the map conditions in a single if ... else
> conditional.
> 
> No functional change.
> 
> Signed-off-by: Roger Pau Monné <roger.pau@citrix.com>
> ---
> Cc: Jan Beulich <jbeulich@suse.com>
> ---
>  xen/drivers/passthrough/x86/iommu.c | 23 ++++++++++-------------
>  1 file changed, 10 insertions(+), 13 deletions(-)
> 
> diff --git a/xen/drivers/passthrough/x86/iommu.c
> b/xen/drivers/passthrough/x86/iommu.c
> index ba0bbd9a15..24cc591aa5 100644
> --- a/xen/drivers/passthrough/x86/iommu.c
> +++ b/xen/drivers/passthrough/x86/iommu.c
> @@ -158,19 +158,9 @@ void __hwdom_init arch_iommu_hwdom_init(struct
> domain *d)
>           * regions. When set, the inclusive mapping additionally maps in
>           * every pfn up to 4GB except those that fall in unusable ranges.
>           */
> -        if ( pfn > max_pfn && !mfn_valid(_mfn(pfn)) )
> -            continue;
> -
> -        if ( iommu_inclusive && pfn <= max_pfn )
> -            map = !page_is_ram_type(pfn, RAM_TYPE_UNUSABLE);
> -        else
> -            map = page_is_ram_type(pfn, RAM_TYPE_CONVENTIONAL);
> -
> -        if ( !map )
> -            continue;
> -
> -        /* Exclude Xen bits */
> -        if ( xen_in_range(pfn) )
> +        if ( (pfn > max_pfn && !mfn_valid(_mfn(pfn))) ||
> +             /* Exclude Xen bits */
> +             xen_in_range(pfn) )
>              continue;
> 
>          /*
> @@ -179,6 +169,13 @@ void __hwdom_init arch_iommu_hwdom_init(struct
> domain *d)
>           */
>          if ( iommu_dom0_strict &&
>               page_is_ram_type(pfn, RAM_TYPE_CONVENTIONAL) )
> +            map = false;
> +        else if ( iommu_inclusive && pfn <= max_pfn )
> +            map = !page_is_ram_type(pfn, RAM_TYPE_UNUSABLE);
> +        else
> +            map = page_is_ram_type(pfn, RAM_TYPE_CONVENTIONAL);
> +

Maybe better as...

If ( page_is_ram_type(pfn, RAM_TYPE_CONVENTIONAL) )
  map = !iommu_dom0_strict;
else if ( !page_is_ram_type(pfn, RAM_TYPE_UNUSABLE) )
  map = (iommu_inclusive && pfn <= max_pfn);
else
  map = false; 

(I think that logic is correct).

  Paul

> +        if ( !map )
>              continue;
> 
>          tmp = 1 << (PAGE_SHIFT - PAGE_SHIFT_4K);
> --
> 2.18.0
> 
> 
> _______________________________________________
> Xen-devel mailing list
> Xen-devel@lists.xenproject.org
> https://lists.xenproject.org/mailman/listinfo/xen-devel
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel

Re: [PATCH 3/4] x86/iommu: reorder conditions used in the inclusive iommu mappings

[Roger Pau Monné]

On Tue, Jul 31, 2018 at 08:29:20AM +0100, Paul Durrant wrote:
> > -----Original Message-----
> > From: Xen-devel [mailto:xen-devel-bounces@lists.xenproject.org] On Behalf
> > @@ -179,6 +169,13 @@ void __hwdom_init arch_iommu_hwdom_init(struct
> > domain *d)
> >           */
> >          if ( iommu_dom0_strict &&
> >               page_is_ram_type(pfn, RAM_TYPE_CONVENTIONAL) )
> > +            map = false;
> > +        else if ( iommu_inclusive && pfn <= max_pfn )
> > +            map = !page_is_ram_type(pfn, RAM_TYPE_UNUSABLE);
> > +        else
> > +            map = page_is_ram_type(pfn, RAM_TYPE_CONVENTIONAL);
> > +
> 
> Maybe better as...
> 
> If ( page_is_ram_type(pfn, RAM_TYPE_CONVENTIONAL) )
>   map = !iommu_dom0_strict;
> else if ( !page_is_ram_type(pfn, RAM_TYPE_UNUSABLE) )
>   map = (iommu_inclusive && pfn <= max_pfn);
> else
>   map = false; 
> 
> (I think that logic is correct).

Yes I think it's correct. Should have looked more closely when moving
this code around.

Thanks, Roger.

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

[PATCH 4/4] x86/iommu: add PVH support to the inclusive options

[Roger Pau Monne]

Several people have reported hardware issues (malfunctioning USB
controllers) due to iommu page faults. Those faults are caused by
missing RMRR (VTd) or IRVS (AMD-Vi) entries in the ACPI tables. Those
can be worked around on VTd hardware by manually adding RMRR entries
on the command line, this is however limited to Intel hardware and
quite cumbersome to do.

In order to solve those issues add PVH support to the inclusive option
that identity maps all regions marked as reserved in the memory map.
Note that regions used by devices emulated by Xen (LAPIC, IO-APIC or
PCIe MCFG regions) are specifically avoided. Note that this option
currently relies on no MSIX MMIO areas residing in a reserved region,
or else Xen won't be able to trap those accesses.

Signed-off-by: Roger Pau Monné <roger.pau@citrix.com>
---
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>
---
 docs/misc/xen-command-line.markdown | 16 ++++--
 xen/drivers/passthrough/x86/iommu.c | 82 +++++++++++++++++++++++------
 2 files changed, 77 insertions(+), 21 deletions(-)

diff --git a/docs/misc/xen-command-line.markdown b/docs/misc/xen-command-line.markdown
index 91a8bfc9a6..c7c9a38c19 100644
--- a/docs/misc/xen-command-line.markdown
+++ b/docs/misc/xen-command-line.markdown
@@ -1203,11 +1203,17 @@ detection of systems known to misbehave upon accesses to that port.
 > Default: `true`
 
 >> Use this to work around firmware issues providing incorrect RMRR or 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. Also note that if `dom0-strict` mode is enabled
->> then conventional RAM pages not assigned to dom0 will not be mapped.
+>> entries. The behaviour of this option is slightly different between a PV and
+>> a PVH Dom0:
+>>
+>> * For a PV Dom0 all pages up to 4GB not marked as unusable in the memory
+>>   map will get a mapping established. Note that if `dom0-strict` mode is
+>>   enabled then conventional RAM pages not assigned to dom0 will not be
+>>   mapped.
+>>
+>> * For a PVH Dom0 all memory regions marked as reserved in the memory map
+>>   that don't overlap with any MMIO region from emulated devices will be
+>>   identity mapped.
 
 ### iommu\_dev\_iotlb\_timeout
 > `= <integer>`
diff --git a/xen/drivers/passthrough/x86/iommu.c b/xen/drivers/passthrough/x86/iommu.c
index 24cc591aa5..cfafe1b572 100644
--- a/xen/drivers/passthrough/x86/iommu.c
+++ b/xen/drivers/passthrough/x86/iommu.c
@@ -20,6 +20,8 @@
 #include <xen/softirq.h>
 #include <xsm/xsm.h>
 
+#include <asm/apicdef.h>
+#include <asm/io_apic.h>
 #include <asm/setup.h>
 
 void iommu_update_ire_from_apic(
@@ -134,11 +136,62 @@ void arch_iommu_domain_destroy(struct domain *d)
 {
 }
 
+static bool __hwdom_init pv_inclusive_map(unsigned long pfn,
+                                          unsigned long max_pfn)
+{
+    /*
+     * If dom0-strict mode is enabled then exclude conventional RAM
+     * and let the common code map dom0's pages.
+     */
+    if ( iommu_dom0_strict && page_is_ram_type(pfn, RAM_TYPE_CONVENTIONAL) )
+        return false;
+    if ( iommu_inclusive && pfn <= max_pfn )
+        return !page_is_ram_type(pfn, RAM_TYPE_UNUSABLE);
+
+    return page_is_ram_type(pfn, RAM_TYPE_CONVENTIONAL);
+}
+
+static bool __hwdom_init pvh_inclusive_map(const struct domain *d,
+                                           unsigned long pfn)
+{
+    unsigned int i;
+
+    /*
+     * Ignore any address below 1MB, that's already identity mapped by the
+     * domain builder.
+     */
+    if ( pfn < PFN_DOWN(MB(1)) )
+        return false;
+
+    /* Only add reserved regions. */
+    if ( !page_is_ram_type(pfn, RAM_TYPE_RESERVED) )
+        return false;
+
+    /* Check that it doesn't overlap with the LAPIC */
+    if ( pfn == PFN_DOWN(APIC_DEFAULT_PHYS_BASE) )
+        return false;
+    /* ... or the IO-APIC */
+    for ( i = 0; i < nr_ioapics; i++ )
+        if ( pfn == PFN_DOWN(domain_vioapic(d, i)->base_address) )
+            return false;
+    /* ... or the PCIe MCFG regions. */
+    for ( i = 0; i < pci_mmcfg_config_num; i++ )
+    {
+        unsigned long addr = PFN_DOWN(pci_mmcfg_config[i].address);
+
+        if ( pfn >= addr + (pci_mmcfg_config[i].start_bus_number << 8) &&
+             pfn < addr + (pci_mmcfg_config[i].end_bus_number << 8) )
+            return false;
+    }
+
+    return true;
+}
+
 void __hwdom_init arch_iommu_hwdom_init(struct domain *d)
 {
     unsigned long i, j, tmp, top, max_pfn;
 
-    if ( iommu_passthrough || !is_pv_domain(d) )
+    if ( iommu_passthrough )
         return;
 
     BUG_ON(!is_hardware_domain(d));
@@ -149,7 +202,6 @@ void __hwdom_init arch_iommu_hwdom_init(struct domain *d)
     for ( i = 0; i < top; i++ )
     {
         unsigned long pfn = pdx_to_pfn(i);
-        bool map;
         int rc = 0;
 
         /*
@@ -163,25 +215,23 @@ void __hwdom_init arch_iommu_hwdom_init(struct domain *d)
              xen_in_range(pfn) )
             continue;
 
-        /*
-         * If dom0-strict mode is enabled then exclude conventional RAM
-         * and let the common code map dom0's pages.
-         */
-        if ( iommu_dom0_strict &&
-             page_is_ram_type(pfn, RAM_TYPE_CONVENTIONAL) )
-            map = false;
-        else if ( iommu_inclusive && pfn <= max_pfn )
-            map = !page_is_ram_type(pfn, RAM_TYPE_UNUSABLE);
-        else
-            map = page_is_ram_type(pfn, RAM_TYPE_CONVENTIONAL);
-
-        if ( !map )
+        if ( is_pv_domain(d) ? !pv_inclusive_map(pfn, max_pfn)
+                             : !pvh_inclusive_map(d, pfn) )
             continue;
 
         tmp = 1 << (PAGE_SHIFT - PAGE_SHIFT_4K);
         for ( j = 0; j < tmp; j++ )
         {
-            int ret = iommu_map_page(d, pfn * tmp + j, pfn * tmp + j,
+            int ret;
+
+            if ( iommu_use_hap_pt(d) )
+            {
+                ASSERT(is_hvm_domain(d));
+                ret = set_identity_p2m_entry(d, pfn * tmp + j, p2m_access_rw,
+                                             0);
+            }
+            else
+                ret = iommu_map_page(d, pfn * tmp + j, pfn * tmp + j,
                                      IOMMUF_readable|IOMMUF_writable);
 
             if ( !rc )
-- 
2.18.0


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

Re: [PATCH 4/4] x86/iommu: add PVH support to the inclusive options

[Paul Durrant]

> -----Original Message-----
> From: Xen-devel [mailto:xen-devel-bounces@lists.xenproject.org] On Behalf
> Of Roger Pau Monne
> Sent: 27 July 2018 16:32
> To: xen-devel@lists.xenproject.org
> Cc: Stefano Stabellini <sstabellini@kernel.org>; Wei Liu
> <wei.liu2@citrix.com>; George Dunlap <George.Dunlap@citrix.com>;
> Andrew Cooper <Andrew.Cooper3@citrix.com>; Ian Jackson
> <Ian.Jackson@citrix.com>; Tim (Xen.org) <tim@xen.org>; Julien Grall
> <julien.grall@arm.com>; Jan Beulich <jbeulich@suse.com>; Roger Pau
> Monne <roger.pau@citrix.com>
> Subject: [Xen-devel] [PATCH 4/4] x86/iommu: add PVH support to the
> inclusive options
> 
> Several people have reported hardware issues (malfunctioning USB
> controllers) due to iommu page faults. Those faults are caused by
> missing RMRR (VTd) or IRVS (AMD-Vi) entries in the ACPI tables. Those
> can be worked around on VTd hardware by manually adding RMRR entries
> on the command line, this is however limited to Intel hardware and
> quite cumbersome to do.
> 
> In order to solve those issues add PVH support to the inclusive option
> that identity maps all regions marked as reserved in the memory map.
> Note that regions used by devices emulated by Xen (LAPIC, IO-APIC or
> PCIe MCFG regions) are specifically avoided. Note that this option
> currently relies on no MSIX MMIO areas residing in a reserved region,
> or else Xen won't be able to trap those accesses.
> 
> Signed-off-by: Roger Pau Monné <roger.pau@citrix.com>
> ---
> 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>
> ---
>  docs/misc/xen-command-line.markdown | 16 ++++--
>  xen/drivers/passthrough/x86/iommu.c | 82 +++++++++++++++++++++++--
> ----
>  2 files changed, 77 insertions(+), 21 deletions(-)
> 
> diff --git a/docs/misc/xen-command-line.markdown b/docs/misc/xen-
> command-line.markdown
> index 91a8bfc9a6..c7c9a38c19 100644
> --- a/docs/misc/xen-command-line.markdown
> +++ b/docs/misc/xen-command-line.markdown
> @@ -1203,11 +1203,17 @@ detection of systems known to misbehave upon
> accesses to that port.
>  > Default: `true`
> 
>  >> Use this to work around firmware issues providing incorrect RMRR or
> 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. Also note that if `dom0-strict` mode is enabled
> ->> then conventional RAM pages not assigned to dom0 will not be mapped.
> +>> entries. The behaviour of this option is slightly different between a PV
> and
> +>> a PVH Dom0:
> +>>
> +>> * For a PV Dom0 all pages up to 4GB not marked as unusable in the
> memory
> +>>   map will get a mapping established. Note that if `dom0-strict` mode is
> +>>   enabled then conventional RAM pages not assigned to dom0 will not be
> +>>   mapped.
> +>>
> +>> * For a PVH Dom0 all memory regions marked as reserved in the
> memory map
> +>>   that don't overlap with any MMIO region from emulated devices will be
> +>>   identity mapped.
> 
>  ### iommu\_dev\_iotlb\_timeout
>  > `= <integer>`
> diff --git a/xen/drivers/passthrough/x86/iommu.c
> b/xen/drivers/passthrough/x86/iommu.c
> index 24cc591aa5..cfafe1b572 100644
> --- a/xen/drivers/passthrough/x86/iommu.c
> +++ b/xen/drivers/passthrough/x86/iommu.c
> @@ -20,6 +20,8 @@
>  #include <xen/softirq.h>
>  #include <xsm/xsm.h>
> 
> +#include <asm/apicdef.h>
> +#include <asm/io_apic.h>
>  #include <asm/setup.h>
> 
>  void iommu_update_ire_from_apic(
> @@ -134,11 +136,62 @@ void arch_iommu_domain_destroy(struct domain
> *d)
>  {
>  }
> 
> +static bool __hwdom_init pv_inclusive_map(unsigned long pfn,
> +                                          unsigned long max_pfn)

Perhaps pv_hwdom_inclusive_map() (and similarly pvh_hwdom_inclusive_map()) to make it obvious that they are intended only for the hardware domain. (I know the annotation makes this reasonably obvious but other hwdom-specific functions seem to carry this in their name).

  Paul

> +{
> +    /*
> +     * If dom0-strict mode is enabled then exclude conventional RAM
> +     * and let the common code map dom0's pages.
> +     */
> +    if ( iommu_dom0_strict && page_is_ram_type(pfn,
> RAM_TYPE_CONVENTIONAL) )
> +        return false;
> +    if ( iommu_inclusive && pfn <= max_pfn )
> +        return !page_is_ram_type(pfn, RAM_TYPE_UNUSABLE);
> +
> +    return page_is_ram_type(pfn, RAM_TYPE_CONVENTIONAL);
> +}
> +
> +static bool __hwdom_init pvh_inclusive_map(const struct domain *d,
> +                                           unsigned long pfn)
> +{
> +    unsigned int i;
> +
> +    /*
> +     * Ignore any address below 1MB, that's already identity mapped by the
> +     * domain builder.
> +     */
> +    if ( pfn < PFN_DOWN(MB(1)) )
> +        return false;
> +
> +    /* Only add reserved regions. */
> +    if ( !page_is_ram_type(pfn, RAM_TYPE_RESERVED) )
> +        return false;
> +
> +    /* Check that it doesn't overlap with the LAPIC */
> +    if ( pfn == PFN_DOWN(APIC_DEFAULT_PHYS_BASE) )
> +        return false;
> +    /* ... or the IO-APIC */
> +    for ( i = 0; i < nr_ioapics; i++ )
> +        if ( pfn == PFN_DOWN(domain_vioapic(d, i)->base_address) )
> +            return false;
> +    /* ... or the PCIe MCFG regions. */
> +    for ( i = 0; i < pci_mmcfg_config_num; i++ )
> +    {
> +        unsigned long addr = PFN_DOWN(pci_mmcfg_config[i].address);
> +
> +        if ( pfn >= addr + (pci_mmcfg_config[i].start_bus_number << 8) &&
> +             pfn < addr + (pci_mmcfg_config[i].end_bus_number << 8) )
> +            return false;
> +    }
> +
> +    return true;
> +}
> +
>  void __hwdom_init arch_iommu_hwdom_init(struct domain *d)
>  {
>      unsigned long i, j, tmp, top, max_pfn;
> 
> -    if ( iommu_passthrough || !is_pv_domain(d) )
> +    if ( iommu_passthrough )
>          return;
> 
>      BUG_ON(!is_hardware_domain(d));
> @@ -149,7 +202,6 @@ void __hwdom_init arch_iommu_hwdom_init(struct
> domain *d)
>      for ( i = 0; i < top; i++ )
>      {
>          unsigned long pfn = pdx_to_pfn(i);
> -        bool map;
>          int rc = 0;
> 
>          /*
> @@ -163,25 +215,23 @@ void __hwdom_init
> arch_iommu_hwdom_init(struct domain *d)
>               xen_in_range(pfn) )
>              continue;
> 
> -        /*
> -         * If dom0-strict mode is enabled then exclude conventional RAM
> -         * and let the common code map dom0's pages.
> -         */
> -        if ( iommu_dom0_strict &&
> -             page_is_ram_type(pfn, RAM_TYPE_CONVENTIONAL) )
> -            map = false;
> -        else if ( iommu_inclusive && pfn <= max_pfn )
> -            map = !page_is_ram_type(pfn, RAM_TYPE_UNUSABLE);
> -        else
> -            map = page_is_ram_type(pfn, RAM_TYPE_CONVENTIONAL);
> -
> -        if ( !map )
> +        if ( is_pv_domain(d) ? !pv_inclusive_map(pfn, max_pfn)
> +                             : !pvh_inclusive_map(d, pfn) )
>              continue;
> 
>          tmp = 1 << (PAGE_SHIFT - PAGE_SHIFT_4K);
>          for ( j = 0; j < tmp; j++ )
>          {
> -            int ret = iommu_map_page(d, pfn * tmp + j, pfn * tmp + j,
> +            int ret;
> +
> +            if ( iommu_use_hap_pt(d) )
> +            {
> +                ASSERT(is_hvm_domain(d));
> +                ret = set_identity_p2m_entry(d, pfn * tmp + j, p2m_access_rw,
> +                                             0);
> +            }
> +            else
> +                ret = iommu_map_page(d, pfn * tmp + j, pfn * tmp + j,
>                                       IOMMUF_readable|IOMMUF_writable);
> 
>              if ( !rc )
> --
> 2.18.0
> 
> 
> _______________________________________________
> Xen-devel mailing list
> Xen-devel@lists.xenproject.org
> https://lists.xenproject.org/mailman/listinfo/xen-devel
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel

Re: [PATCH 4/4] x86/iommu: add PVH support to the inclusive options

[Roger Pau Monné]

On Tue, Jul 31, 2018 at 08:36:02AM +0100, Paul Durrant wrote:
> > -----Original Message-----
> > From: Xen-devel [mailto:xen-devel-bounces@lists.xenproject.org] On Behalf
> > Of Roger Pau Monne
> > Sent: 27 July 2018 16:32
> > To: xen-devel@lists.xenproject.org
> > Cc: Stefano Stabellini <sstabellini@kernel.org>; Wei Liu
> > <wei.liu2@citrix.com>; George Dunlap <George.Dunlap@citrix.com>;
> > Andrew Cooper <Andrew.Cooper3@citrix.com>; Ian Jackson
> > <Ian.Jackson@citrix.com>; Tim (Xen.org) <tim@xen.org>; Julien Grall
> > <julien.grall@arm.com>; Jan Beulich <jbeulich@suse.com>; Roger Pau
> > Monne <roger.pau@citrix.com>
> > Subject: [Xen-devel] [PATCH 4/4] x86/iommu: add PVH support to the
> > inclusive options
> > 
> > Several people have reported hardware issues (malfunctioning USB
> > controllers) due to iommu page faults. Those faults are caused by
> > missing RMRR (VTd) or IRVS (AMD-Vi) entries in the ACPI tables. Those
> > can be worked around on VTd hardware by manually adding RMRR entries
> > on the command line, this is however limited to Intel hardware and
> > quite cumbersome to do.
> > 
> > In order to solve those issues add PVH support to the inclusive option
> > that identity maps all regions marked as reserved in the memory map.
> > Note that regions used by devices emulated by Xen (LAPIC, IO-APIC or
> > PCIe MCFG regions) are specifically avoided. Note that this option
> > currently relies on no MSIX MMIO areas residing in a reserved region,
> > or else Xen won't be able to trap those accesses.
> > 
> > Signed-off-by: Roger Pau Monné <roger.pau@citrix.com>
> > ---
> > 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>
> > ---
> >  docs/misc/xen-command-line.markdown | 16 ++++--
> >  xen/drivers/passthrough/x86/iommu.c | 82 +++++++++++++++++++++++--
> > ----
> >  2 files changed, 77 insertions(+), 21 deletions(-)
> > 
> > diff --git a/docs/misc/xen-command-line.markdown b/docs/misc/xen-
> > command-line.markdown
> > index 91a8bfc9a6..c7c9a38c19 100644
> > --- a/docs/misc/xen-command-line.markdown
> > +++ b/docs/misc/xen-command-line.markdown
> > @@ -1203,11 +1203,17 @@ detection of systems known to misbehave upon
> > accesses to that port.
> >  > Default: `true`
> > 
> >  >> Use this to work around firmware issues providing incorrect RMRR or
> > 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. Also note that if `dom0-strict` mode is enabled
> > ->> then conventional RAM pages not assigned to dom0 will not be mapped.
> > +>> entries. The behaviour of this option is slightly different between a PV
> > and
> > +>> a PVH Dom0:
> > +>>
> > +>> * For a PV Dom0 all pages up to 4GB not marked as unusable in the
> > memory
> > +>>   map will get a mapping established. Note that if `dom0-strict` mode is
> > +>>   enabled then conventional RAM pages not assigned to dom0 will not be
> > +>>   mapped.
> > +>>
> > +>> * For a PVH Dom0 all memory regions marked as reserved in the
> > memory map
> > +>>   that don't overlap with any MMIO region from emulated devices will be
> > +>>   identity mapped.
> > 
> >  ### iommu\_dev\_iotlb\_timeout
> >  > `= <integer>`
> > diff --git a/xen/drivers/passthrough/x86/iommu.c
> > b/xen/drivers/passthrough/x86/iommu.c
> > index 24cc591aa5..cfafe1b572 100644
> > --- a/xen/drivers/passthrough/x86/iommu.c
> > +++ b/xen/drivers/passthrough/x86/iommu.c
> > @@ -20,6 +20,8 @@
> >  #include <xen/softirq.h>
> >  #include <xsm/xsm.h>
> > 
> > +#include <asm/apicdef.h>
> > +#include <asm/io_apic.h>
> >  #include <asm/setup.h>
> > 
> >  void iommu_update_ire_from_apic(
> > @@ -134,11 +136,62 @@ void arch_iommu_domain_destroy(struct domain
> > *d)
> >  {
> >  }
> > 
> > +static bool __hwdom_init pv_inclusive_map(unsigned long pfn,
> > +                                          unsigned long max_pfn)
> 
> Perhaps pv_hwdom_inclusive_map() (and similarly pvh_hwdom_inclusive_map()) to make it obvious that they are intended only for the hardware domain. (I know the annotation makes this reasonably obvious but other hwdom-specific functions seem to carry this in their name)

Given the naming conversation in patch 2 I think this should be named
hwdom_inclusive_map and the new function should be hwdom_reserved_map.
I think the pv/pvh prefix can be avoided if you think the reserved
mapping is helpful for classic PV also.

Thanks, Roger.

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

Re: [PATCH 4/4] x86/iommu: add PVH support to the inclusive options

[Jan Beulich]

>>> On 27.07.18 at 17:31, <roger.pau@citrix.com> wrote:
> Several people have reported hardware issues (malfunctioning USB
> controllers) due to iommu page faults. Those faults are caused by
> missing RMRR (VTd) or IRVS (AMD-Vi) entries in the ACPI tables. Those
> can be worked around on VTd hardware by manually adding RMRR entries
> on the command line, this is however limited to Intel hardware and
> quite cumbersome to do.
> 
> In order to solve those issues add PVH support to the inclusive option
> that identity maps all regions marked as reserved in the memory map.
> Note that regions used by devices emulated by Xen (LAPIC, IO-APIC or
> PCIe MCFG regions) are specifically avoided. Note that this option
> currently relies on no MSIX MMIO areas residing in a reserved region,
> or else Xen won't be able to trap those accesses.

But that would be a firmware bug anyway: These are sub-ranges
of PCI device BARs, and those must not overlap reserved ranges
in E820.

> --- a/xen/drivers/passthrough/x86/iommu.c
> +++ b/xen/drivers/passthrough/x86/iommu.c
> @@ -20,6 +20,8 @@
>  #include <xen/softirq.h>
>  #include <xsm/xsm.h>
>  
> +#include <asm/apicdef.h>
> +#include <asm/io_apic.h>

Why? You're looking for the guest view of things, which I don't
think you can derive from definitions in these two headers.

> +static bool __hwdom_init pvh_inclusive_map(const struct domain *d,
> +                                           unsigned long pfn)
> +{
> +    unsigned int i;
> +
> +    /*
> +     * Ignore any address below 1MB, that's already identity mapped by the
> +     * domain builder.
> +     */
> +    if ( pfn < PFN_DOWN(MB(1)) )
> +        return false;
> +
> +    /* Only add reserved regions. */
> +    if ( !page_is_ram_type(pfn, RAM_TYPE_RESERVED) )
> +        return false;
> +
> +    /* Check that it doesn't overlap with the LAPIC */
> +    if ( pfn == PFN_DOWN(APIC_DEFAULT_PHYS_BASE) )

I.e. this should interact with vlapic.c.

> +        return false;
> +    /* ... or the IO-APIC */
> +    for ( i = 0; i < nr_ioapics; i++ )
> +        if ( pfn == PFN_DOWN(domain_vioapic(d, i)->base_address) )
> +            return false;

Ah, nr_ioapics legitimately comes from that header. But do you
really need that? Can't you use d->arch.hvm_domain.nr_vioapics?

> +    /* ... or the PCIe MCFG regions. */
> +    for ( i = 0; i < pci_mmcfg_config_num; i++ )
> +    {
> +        unsigned long addr = PFN_DOWN(pci_mmcfg_config[i].address);
> +
> +        if ( pfn >= addr + (pci_mmcfg_config[i].start_bus_number << 8) &&
> +             pfn < addr + (pci_mmcfg_config[i].end_bus_number << 8) )
> +            return false;
> +    }

Same here - this would better use domain state.

Also end_bus_number is inclusive iirc, so the above doesn't cover
the entire range as it seems.

Jan



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

Re: [PATCH 4/4] x86/iommu: add PVH support to the inclusive options

[Roger Pau Monné]

On Tue, Jul 31, 2018 at 08:52:14AM -0600, Jan Beulich wrote:
> >>> On 27.07.18 at 17:31, <roger.pau@citrix.com> wrote:
> > Several people have reported hardware issues (malfunctioning USB
> > controllers) due to iommu page faults. Those faults are caused by
> > missing RMRR (VTd) or IRVS (AMD-Vi) entries in the ACPI tables. Those
> > can be worked around on VTd hardware by manually adding RMRR entries
> > on the command line, this is however limited to Intel hardware and
> > quite cumbersome to do.
> > 
> > In order to solve those issues add PVH support to the inclusive option
> > that identity maps all regions marked as reserved in the memory map.
> > Note that regions used by devices emulated by Xen (LAPIC, IO-APIC or
> > PCIe MCFG regions) are specifically avoided. Note that this option
> > currently relies on no MSIX MMIO areas residing in a reserved region,
> > or else Xen won't be able to trap those accesses.
> 
> But that would be a firmware bug anyway: These are sub-ranges
> of PCI device BARs, and those must not overlap reserved ranges
> in E820.

Considering this is already a workaround for broken firmware I won't
be surprised that on certain hardware BARs overlap with reserved
ranges.

> > --- a/xen/drivers/passthrough/x86/iommu.c
> > +++ b/xen/drivers/passthrough/x86/iommu.c
> > @@ -20,6 +20,8 @@
> >  #include <xen/softirq.h>
> >  #include <xsm/xsm.h>
> >  
> > +#include <asm/apicdef.h>
> > +#include <asm/io_apic.h>
> 
> Why? You're looking for the guest view of things, which I don't
> think you can derive from definitions in these two headers.

I can use the domain state for the lapic and the ioapic...

> > +    /* ... or the PCIe MCFG regions. */
> > +    for ( i = 0; i < pci_mmcfg_config_num; i++ )
> > +    {
> > +        unsigned long addr = PFN_DOWN(pci_mmcfg_config[i].address);
> > +
> > +        if ( pfn >= addr + (pci_mmcfg_config[i].start_bus_number << 8) &&
> > +             pfn < addr + (pci_mmcfg_config[i].end_bus_number << 8) )
> > +            return false;
> > +    }
> 
> Same here - this would better use domain state.

... but pci mcfg regions are currently added after the iommu
initialization in the HVM dom0 builder. I can shuffle that call so
that iommu initialization is performed after the mcfg regions are
added.

Thanks, Roger.

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

Re: [PATCH 4/4] x86/iommu: add PVH support to the inclusive options

[Roger Pau Monné]

On Tue, Jul 31, 2018 at 05:15:22PM +0200, Roger Pau Monné wrote:
> On Tue, Jul 31, 2018 at 08:52:14AM -0600, Jan Beulich wrote:
> > >>> On 27.07.18 at 17:31, <roger.pau@citrix.com> wrote:
> > > --- a/xen/drivers/passthrough/x86/iommu.c
> > > +++ b/xen/drivers/passthrough/x86/iommu.c
> > > @@ -20,6 +20,8 @@
> > >  #include <xen/softirq.h>
> > >  #include <xsm/xsm.h>
> > >  
> > > +#include <asm/apicdef.h>
> > > +#include <asm/io_apic.h>
> > 
> > Why? You're looking for the guest view of things, which I don't
> > think you can derive from definitions in these two headers.
> 
> I can use the domain state for the lapic and the ioapic...
> 
> > > +    /* ... or the PCIe MCFG regions. */
> > > +    for ( i = 0; i < pci_mmcfg_config_num; i++ )
> > > +    {
> > > +        unsigned long addr = PFN_DOWN(pci_mmcfg_config[i].address);
> > > +
> > > +        if ( pfn >= addr + (pci_mmcfg_config[i].start_bus_number << 8) &&
> > > +             pfn < addr + (pci_mmcfg_config[i].end_bus_number << 8) )
> > > +            return false;
> > > +    }
> > 
> > Same here - this would better use domain state.
> 
> ... but pci mcfg regions are currently added after the iommu
> initialization in the HVM dom0 builder. I can shuffle that call so
> that iommu initialization is performed after the mcfg regions are
> added.

So that's not so easy. The current hvm_mmcfg used to store the MMCFG
data in the domain struct is private to io.c, I could move it's
declaration to a header file, but maybe it's better to just use
pci_mmcfg_config directly?

Thanks, Roger.

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

Re: [PATCH 4/4] x86/iommu: add PVH support to the inclusive options

[Jan Beulich]

>>> On 31.07.18 at 17:27, <roger.pau@citrix.com> wrote:
> On Tue, Jul 31, 2018 at 05:15:22PM +0200, Roger Pau Monné wrote:
>> On Tue, Jul 31, 2018 at 08:52:14AM -0600, Jan Beulich wrote:
>> > >>> On 27.07.18 at 17:31, <roger.pau@citrix.com> wrote:
>> > > --- a/xen/drivers/passthrough/x86/iommu.c
>> > > +++ b/xen/drivers/passthrough/x86/iommu.c
>> > > @@ -20,6 +20,8 @@
>> > >  #include <xen/softirq.h>
>> > >  #include <xsm/xsm.h>
>> > >  
>> > > +#include <asm/apicdef.h>
>> > > +#include <asm/io_apic.h>
>> > 
>> > Why? You're looking for the guest view of things, which I don't
>> > think you can derive from definitions in these two headers.
>> 
>> I can use the domain state for the lapic and the ioapic...
>> 
>> > > +    /* ... or the PCIe MCFG regions. */
>> > > +    for ( i = 0; i < pci_mmcfg_config_num; i++ )
>> > > +    {
>> > > +        unsigned long addr = PFN_DOWN(pci_mmcfg_config[i].address);
>> > > +
>> > > +        if ( pfn >= addr + (pci_mmcfg_config[i].start_bus_number << 8) &&
>> > > +             pfn < addr + (pci_mmcfg_config[i].end_bus_number << 8) )
>> > > +            return false;
>> > > +    }
>> > 
>> > Same here - this would better use domain state.
>> 
>> ... but pci mcfg regions are currently added after the iommu
>> initialization in the HVM dom0 builder. I can shuffle that call so
>> that iommu initialization is performed after the mcfg regions are
>> added.
> 
> So that's not so easy. The current hvm_mmcfg used to store the MMCFG
> data in the domain struct is private to io.c, I could move it's
> declaration to a header file, but maybe it's better to just use
> pci_mmcfg_config directly?

Or add a helper function in that file?

Jan


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

Re: [PATCH 4/4] x86/iommu: add PVH support to the inclusive options

[Jan Beulich]

>>> On 31.07.18 at 17:15, <roger.pau@citrix.com> wrote:
> On Tue, Jul 31, 2018 at 08:52:14AM -0600, Jan Beulich wrote:
>> >>> On 27.07.18 at 17:31, <roger.pau@citrix.com> wrote:
>> > +    /* ... or the PCIe MCFG regions. */
>> > +    for ( i = 0; i < pci_mmcfg_config_num; i++ )
>> > +    {
>> > +        unsigned long addr = PFN_DOWN(pci_mmcfg_config[i].address);
>> > +
>> > +        if ( pfn >= addr + (pci_mmcfg_config[i].start_bus_number << 8) &&
>> > +             pfn < addr + (pci_mmcfg_config[i].end_bus_number << 8) )
>> > +            return false;
>> > +    }
>> 
>> Same here - this would better use domain state.
> 
> ... but pci mcfg regions are currently added after the iommu
> initialization in the HVM dom0 builder. I can shuffle that call so
> that iommu initialization is performed after the mcfg regions are
> added.

Or at least leave a todo comment. After all MCFG regions might
further be added while Dom0 boots and parses AML.

Jan



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