[PATCH v7 0/6] x86/iommu: PVH Dom0 workarounds for missing RMRR entries

[PATCH v7 0/6] x86/iommu: PVH Dom0 workarounds for missing RMRR entries

[Roger Pau Monne]

Hello,

The following series implement a workaround for missing RMRR
entries for a PVH Dom0. It's based on the iommu_inclusive_mapping VTd
option.

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

Note that this workaround is enabled by default on Intel hardware. It's
also available to AMD hardware, although it's disabled by default in
that case.

The series can be found at:

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

Thanks.

Roger Pau Monne (6):
  iommu: rename iommu_dom0_strict and iommu_passthrough
  iommu: introduce dom0-iommu option
  iommu: make iommu_inclusive_mapping a suboption of dom0-iommu
  mm: introduce a helper to get the memory type of a page
  x86/iommu: switch the hwdom mapping function to use page_get_type
  x86/iommu: add map-reserved dom0-iommu option to map reserved memory
    ranges

 docs/misc/xen-command-line.markdown         |  41 ++++++++
 xen/arch/x86/hvm/io.c                       |   5 +
 xen/arch/x86/mm.c                           |  42 ++++++++
 xen/arch/x86/x86_64/mm.c                    |   3 +-
 xen/drivers/passthrough/amd/iommu_init.c    |   2 +-
 xen/drivers/passthrough/amd/pci_amd_iommu.c |  11 ++-
 xen/drivers/passthrough/arm/iommu.c         |   4 +
 xen/drivers/passthrough/arm/smmu.c          |   3 +
 xen/drivers/passthrough/iommu.c             |  71 ++++++++++---
 xen/drivers/passthrough/vtd/extern.h        |   2 -
 xen/drivers/passthrough/vtd/iommu.c         |  25 ++---
 xen/drivers/passthrough/vtd/x86/vtd.c       |  58 +----------
 xen/drivers/passthrough/x86/iommu.c         | 104 ++++++++++++++++++++
 xen/include/asm-x86/hvm/io.h                |   3 +
 xen/include/xen/iommu.h                     |   8 +-
 xen/include/xen/mm.h                        |   3 +
 16 files changed, 292 insertions(+), 93 deletions(-)

-- 
2.18.0


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

[PATCH v7 1/6] iommu: rename iommu_dom0_strict and iommu_passthrough

[Roger Pau Monne]

To iommu_hwdom_strict and iommu_hwdom_passthrough which is more
descriptive of their usage. Also change their type from bool_t to
bool.

No functional change.

Signed-off-by: Roger Pau Monné <roger.pau@citrix.com>
Reviewed-by: Jan Beulich <jbeulich@suse.com>
Reviewed-by: Kevin Tian <kevin.tian@intel.com>
---
Cc: Jan Beulich <jbeulich@suse.com>
Cc: Andrew Cooper <andrew.cooper3@citrix.com>
Cc: Suravee Suthikulpanit <suravee.suthikulpanit@amd.com>
Cc: Brian Woods <brian.woods@amd.com>
Cc: Kevin Tian <kevin.tian@intel.com>
---
Changes since v5:
 - Remove unneeded !!.

Changes since v4:
 - New in this version.
---
 xen/arch/x86/x86_64/mm.c                    |  3 ++-
 xen/drivers/passthrough/amd/iommu_init.c    |  2 +-
 xen/drivers/passthrough/amd/pci_amd_iommu.c |  4 +--
 xen/drivers/passthrough/iommu.c             | 27 +++++++++++----------
 xen/drivers/passthrough/vtd/iommu.c         | 16 ++++++------
 xen/drivers/passthrough/vtd/x86/vtd.c       |  2 +-
 xen/include/xen/iommu.h                     |  6 +++--
 7 files changed, 32 insertions(+), 28 deletions(-)

diff --git a/xen/arch/x86/x86_64/mm.c b/xen/arch/x86/x86_64/mm.c
index cca4ae926e..093ad5dab1 100644
--- a/xen/arch/x86/x86_64/mm.c
+++ b/xen/arch/x86/x86_64/mm.c
@@ -1426,7 +1426,8 @@ int memory_add(unsigned long spfn, unsigned long epfn, unsigned int pxm)
     if ( ret )
         goto destroy_m2p;
 
-    if ( iommu_enabled && !iommu_passthrough && !need_iommu(hardware_domain) )
+    if ( iommu_enabled && !iommu_hwdom_passthrough &&
+         !need_iommu(hardware_domain) )
     {
         for ( i = spfn; i < epfn; i++ )
             if ( iommu_map_page(hardware_domain, i, i, IOMMUF_readable|IOMMUF_writable) )
diff --git a/xen/drivers/passthrough/amd/iommu_init.c b/xen/drivers/passthrough/amd/iommu_init.c
index 474992a75a..15c10b0929 100644
--- a/xen/drivers/passthrough/amd/iommu_init.c
+++ b/xen/drivers/passthrough/amd/iommu_init.c
@@ -1062,7 +1062,7 @@ static void __init amd_iommu_init_cleanup(void)
     radix_tree_destroy(&ivrs_maps, xfree);
 
     iommu_enabled = 0;
-    iommu_passthrough = 0;
+    iommu_hwdom_passthrough = false;
     iommu_intremap = 0;
     iommuv2_enabled = 0;
 }
diff --git a/xen/drivers/passthrough/amd/pci_amd_iommu.c b/xen/drivers/passthrough/amd/pci_amd_iommu.c
index 12d2695b89..ab39e7500d 100644
--- a/xen/drivers/passthrough/amd/pci_amd_iommu.c
+++ b/xen/drivers/passthrough/amd/pci_amd_iommu.c
@@ -121,7 +121,7 @@ static void amd_iommu_setup_domain_device(
     BUG_ON( !hd->arch.root_table || !hd->arch.paging_mode ||
             !iommu->dev_table.buffer );
 
-    if ( iommu_passthrough && is_hardware_domain(domain) )
+    if ( iommu_hwdom_passthrough && is_hardware_domain(domain) )
         valid = 0;
 
     if ( ats_enabled )
@@ -256,7 +256,7 @@ static void __hwdom_init amd_iommu_hwdom_init(struct domain *d)
     if ( allocate_domain_resources(dom_iommu(d)) )
         BUG();
 
-    if ( !iommu_passthrough && !need_iommu(d) )
+    if ( !iommu_hwdom_passthrough && !need_iommu(d) )
     {
         int rc = 0;
 
diff --git a/xen/drivers/passthrough/iommu.c b/xen/drivers/passthrough/iommu.c
index 70d218f910..729ee5c373 100644
--- a/xen/drivers/passthrough/iommu.c
+++ b/xen/drivers/passthrough/iommu.c
@@ -52,15 +52,16 @@ custom_param("iommu", parse_iommu_param);
 bool_t __initdata iommu_enable = 1;
 bool_t __read_mostly iommu_enabled;
 bool_t __read_mostly force_iommu;
-bool_t __hwdom_initdata iommu_dom0_strict;
 bool_t __read_mostly iommu_verbose;
 bool_t __read_mostly iommu_workaround_bios_bug;
 bool_t __read_mostly iommu_igfx = 1;
-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_hwdom_strict;
+bool __read_mostly iommu_hwdom_passthrough;
+
 /*
  * In the current implementation of VT-d posted interrupts, in some extreme
  * cases, the per cpu list which saves the blocked vCPU will be very long,
@@ -121,9 +122,9 @@ static int __init parse_iommu_param(const char *s)
         else if ( !strncmp(s, "amd-iommu-perdev-intremap", ss - s) )
             amd_iommu_perdev_intremap = val;
         else if ( !strncmp(s, "dom0-passthrough", ss - s) )
-            iommu_passthrough = val;
+            iommu_hwdom_passthrough = val;
         else if ( !strncmp(s, "dom0-strict", ss - s) )
-            iommu_dom0_strict = val;
+            iommu_hwdom_strict = val;
         else if ( !strncmp(s, "sharept", ss - s) )
             iommu_hap_pt_share = val;
         else
@@ -158,11 +159,11 @@ static void __hwdom_init check_hwdom_reqs(struct domain *d)
 
     arch_iommu_check_autotranslated_hwdom(d);
 
-    if ( iommu_passthrough )
+    if ( iommu_hwdom_passthrough )
         panic("Dom0 uses paging translated mode, dom0-passthrough must not be "
               "enabled\n");
 
-    iommu_dom0_strict = 1;
+    iommu_hwdom_strict = true;
 }
 
 void __hwdom_init iommu_hwdom_init(struct domain *d)
@@ -175,7 +176,7 @@ void __hwdom_init iommu_hwdom_init(struct domain *d)
         return;
 
     register_keyhandler('o', &iommu_dump_p2m_table, "dump iommu p2m table", 0);
-    d->need_iommu = !!iommu_dom0_strict;
+    d->need_iommu = iommu_hwdom_strict;
     if ( need_iommu(d) && !iommu_use_hap_pt(d) )
     {
         struct page_info *page;
@@ -371,8 +372,8 @@ int __init iommu_setup(void)
     int rc = -ENODEV;
     bool_t force_intremap = force_iommu && iommu_intremap;
 
-    if ( iommu_dom0_strict )
-        iommu_passthrough = 0;
+    if ( iommu_hwdom_strict )
+        iommu_hwdom_passthrough = false;
 
     if ( iommu_enable )
     {
@@ -393,15 +394,15 @@ int __init iommu_setup(void)
     if ( !iommu_enabled )
     {
         iommu_snoop = 0;
-        iommu_passthrough = 0;
-        iommu_dom0_strict = 0;
+        iommu_hwdom_passthrough = false;
+        iommu_hwdom_strict = false;
     }
     printk("I/O virtualisation %sabled\n", iommu_enabled ? "en" : "dis");
     if ( iommu_enabled )
     {
         printk(" - Dom0 mode: %s\n",
-               iommu_passthrough ? "Passthrough" :
-               iommu_dom0_strict ? "Strict" : "Relaxed");
+               iommu_hwdom_passthrough ? "Passthrough" :
+               iommu_hwdom_strict ? "Strict" : "Relaxed");
         printk("Interrupt remapping %sabled\n", iommu_intremap ? "en" : "dis");
         tasklet_init(&iommu_pt_cleanup_tasklet, iommu_free_pagetables, 0);
     }
diff --git a/xen/drivers/passthrough/vtd/iommu.c b/xen/drivers/passthrough/vtd/iommu.c
index 1710256823..32bf0fc299 100644
--- a/xen/drivers/passthrough/vtd/iommu.c
+++ b/xen/drivers/passthrough/vtd/iommu.c
@@ -1304,7 +1304,7 @@ static void __hwdom_init intel_iommu_hwdom_init(struct domain *d)
 {
     struct acpi_drhd_unit *drhd;
 
-    if ( !iommu_passthrough && is_pv_domain(d) )
+    if ( !iommu_hwdom_passthrough && is_pv_domain(d) )
     {
         /* Set up 1:1 page table for hardware domain. */
         vtd_set_hwdom_mapping(d);
@@ -1391,7 +1391,7 @@ int domain_context_mapping_one(
         return res;
     }
 
-    if ( iommu_passthrough && is_hardware_domain(domain) )
+    if ( iommu_hwdom_passthrough && is_hardware_domain(domain) )
     {
         context_set_translation_type(*context, CONTEXT_TT_PASS_THRU);
         agaw = level_to_agaw(iommu->nr_pt_levels);
@@ -1781,7 +1781,7 @@ static int __must_check intel_iommu_map_page(struct domain *d,
         return 0;
 
     /* Do nothing if hardware domain and iommu supports pass thru. */
-    if ( iommu_passthrough && is_hardware_domain(d) )
+    if ( iommu_hwdom_passthrough && is_hardware_domain(d) )
         return 0;
 
     spin_lock(&hd->arch.mapping_lock);
@@ -1826,7 +1826,7 @@ static int __must_check intel_iommu_unmap_page(struct domain *d,
                                                unsigned long gfn)
 {
     /* Do nothing if hardware domain and iommu supports pass thru. */
-    if ( iommu_passthrough && is_hardware_domain(d) )
+    if ( iommu_hwdom_passthrough && is_hardware_domain(d) )
         return 0;
 
     return dma_pte_clear_one(d, (paddr_t)gfn << PAGE_SHIFT_4K);
@@ -2269,8 +2269,8 @@ int __init intel_vtd_setup(void)
         if ( iommu_snoop && !ecap_snp_ctl(iommu->ecap) )
             iommu_snoop = 0;
 
-        if ( iommu_passthrough && !ecap_pass_thru(iommu->ecap) )
-            iommu_passthrough = 0;
+        if ( iommu_hwdom_passthrough && !ecap_pass_thru(iommu->ecap) )
+            iommu_hwdom_passthrough = false;
 
         if ( iommu_qinval && !ecap_queued_inval(iommu->ecap) )
             iommu_qinval = 0;
@@ -2308,7 +2308,7 @@ int __init intel_vtd_setup(void)
 
 #define P(p,s) printk("Intel VT-d %s %senabled.\n", s, (p)? "" : "not ")
     P(iommu_snoop, "Snoop Control");
-    P(iommu_passthrough, "Dom0 DMA Passthrough");
+    P(iommu_hwdom_passthrough, "Dom0 DMA Passthrough");
     P(iommu_qinval, "Queued Invalidation");
     P(iommu_intremap, "Interrupt Remapping");
     P(iommu_intpost, "Posted Interrupt");
@@ -2330,7 +2330,7 @@ int __init intel_vtd_setup(void)
  error:
     iommu_enabled = 0;
     iommu_snoop = 0;
-    iommu_passthrough = 0;
+    iommu_hwdom_passthrough = false;
     iommu_qinval = 0;
     iommu_intremap = 0;
     iommu_intpost = 0;
diff --git a/xen/drivers/passthrough/vtd/x86/vtd.c b/xen/drivers/passthrough/vtd/x86/vtd.c
index 00a9891005..8f8682161b 100644
--- a/xen/drivers/passthrough/vtd/x86/vtd.c
+++ b/xen/drivers/passthrough/vtd/x86/vtd.c
@@ -148,7 +148,7 @@ void __hwdom_init vtd_set_hwdom_mapping(struct domain *d)
          * If dom0-strict mode is enabled then exclude conventional RAM
          * and let the common code map dom0's pages.
          */
-        if ( iommu_dom0_strict &&
+        if ( iommu_hwdom_strict &&
              page_is_ram_type(pfn, RAM_TYPE_CONVENTIONAL) )
             continue;
 
diff --git a/xen/include/xen/iommu.h b/xen/include/xen/iommu.h
index e35d941f3c..392cf1e6b9 100644
--- a/xen/include/xen/iommu.h
+++ b/xen/include/xen/iommu.h
@@ -29,13 +29,15 @@
 #include <asm/iommu.h>
 
 extern bool_t iommu_enable, iommu_enabled;
-extern bool_t force_iommu, iommu_dom0_strict, iommu_verbose;
-extern bool_t iommu_workaround_bios_bug, iommu_igfx, iommu_passthrough;
+extern bool_t force_iommu, iommu_verbose;
+extern bool_t iommu_workaround_bios_bug, iommu_igfx;
 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_hwdom_strict, iommu_hwdom_passthrough;
+
 extern unsigned int iommu_dev_iotlb_timeout;
 
 int iommu_setup(void);
-- 
2.18.0


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

[PATCH v7 2/6] iommu: introduce dom0-iommu option

[Roger Pau Monne]

To select the iommu configuration used by Dom0. This option supersedes
iommu=dom0-strict|dom0-passthrough.

Signed-off-by: Roger Pau Monné <roger.pau@citrix.com>
Reviewed-by: Jan Beulich <jbeulich@suse.com>
---
Changes since v5:
 - Fix typo in docs.
 - Force iommu_hwdom_passthrough to false for PVH Dom0. Note this in
   the documentation.

Changes since v4:
 - Move the iommu_dom0_* variable renaming to a previous patch.
 - Use dom0-iommu=passthrough|strict booleans, like the iommu option
   used.

Changes since v2:
 - Change the style and text used in the xen command line document.
 - Don't allow none, strict or relaxed to use the no- prefix.

Changes since v1:
 - New in this version.
---
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 | 21 +++++++++++++++++++
 xen/drivers/passthrough/iommu.c     | 31 +++++++++++++++++++++++++----
 2 files changed, 48 insertions(+), 4 deletions(-)

diff --git a/docs/misc/xen-command-line.markdown b/docs/misc/xen-command-line.markdown
index 559c0662fa..cd57960ede 100644
--- a/docs/misc/xen-command-line.markdown
+++ b/docs/misc/xen-command-line.markdown
@@ -681,6 +681,21 @@ Flag that makes a dom0 boot in PVHv2 mode.
 Flag that makes a dom0 use shadow paging. Only works when "pvh" is
 enabled.
 
+### dom0-iommu
+> `= List of [ passthrough | strict ]`
+
+This list of booleans controls the iommu usage by Dom0:
+
+* `passthrough`: disables DMA remapping for Dom0. Default is `false`. Note that
+  this option is hard coded to `false` for a PVH Dom0 and any attempt to
+  overwrite it from the command line is ignored.
+
+* `strict`: sets up DMA remapping only for the RAM Dom0 actually got assigned.
+  Default is `false` which means Dom0 will get mappings for all the host
+  RAM except regions in use by Xen. Note that this option is hard coded to
+  `true` for a PVH Dom0 and any attempt to overwrite it from the command line
+  is ignored.
+
 ### dom0\_ioports\_disable (x86)
 > `= List of <hex>-<hex>`
 
@@ -1152,12 +1167,18 @@ detection of systems known to misbehave upon accesses to that port.
 
 > `dom0-passthrough`
 
+> **WARNING: This command line option is deprecated, and superseded by
+> _dom0-iommu=passthrough_ - using both options in combination is undefined.**
+
 > Default: `false`
 
 >> Control whether to disable DMA remapping for Dom0.
 
 > `dom0-strict`
 
+> **WARNING: This command line option is deprecated, and superseded by
+> _dom0-iommu=strict_ - using both options in combination is undefined.**
+
 > Default: `false`
 
 >> Control whether to set up DMA remapping only for the memory Dom0 actually
diff --git a/xen/drivers/passthrough/iommu.c b/xen/drivers/passthrough/iommu.c
index 729ee5c373..8f9975cf4e 100644
--- a/xen/drivers/passthrough/iommu.c
+++ b/xen/drivers/passthrough/iommu.c
@@ -136,6 +136,32 @@ static int __init parse_iommu_param(const char *s)
     return rc;
 }
 
+static int __init parse_dom0_iommu_param(const char *s)
+{
+    const char *ss;
+    int rc = 0;
+
+    do {
+        int val;
+
+        ss = strchr(s, ',');
+        if ( !ss )
+            ss = strchr(s, '\0');
+
+        if ( (val = parse_boolean("passthrough", s, ss)) >= 0 )
+            iommu_hwdom_passthrough = val;
+        else if ( (val = parse_boolean("strict", s, ss)) >= 0 )
+            iommu_hwdom_strict = val;
+        else
+            rc = -EINVAL;
+
+        s = ss + 1;
+    } while ( *ss );
+
+    return rc;
+}
+custom_param("dom0-iommu", parse_dom0_iommu_param);
+
 int iommu_domain_init(struct domain *d)
 {
     struct domain_iommu *hd = dom_iommu(d);
@@ -159,10 +185,7 @@ static void __hwdom_init check_hwdom_reqs(struct domain *d)
 
     arch_iommu_check_autotranslated_hwdom(d);
 
-    if ( iommu_hwdom_passthrough )
-        panic("Dom0 uses paging translated mode, dom0-passthrough must not be "
-              "enabled\n");
-
+    iommu_hwdom_passthrough = false;
     iommu_hwdom_strict = true;
 }
 
-- 
2.18.0


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

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

[Roger Pau Monne]

Introduce a new dom0-iommu=map-inclusive generic option that
supersedes iommu_inclusive_mapping. The previous behavior is preserved
and the option should only be enabled by default on Intel hardware.

Signed-off-by: Roger Pau Monné <roger.pau@citrix.com>
Reviewed-by: Paul Durrant <paul.durrant@citrix.com>
Reviewed-by: Jan Beulich <jbeulich@suse.com>
Reviewed-by: Kevin Tian <kevin.tian@intel.com>
---
Changes since v4:
 - Use an if to set the default option value.
 - Set the option to false unconditionally on ARM.

Changes since v2:
 - Fix typo in commit message.
 - Change style and text of the documentation in xen command line.
 - Set the defaults in {intel/amd}_iommu_hwdom_init for inclusive.
 - Re-add the iommu_dom0_passthrough || !is_pv_domain(d) check.

Changes since v1:
 - Use dom0-iommu instead of the iommu option.
 - Only enable by default on Intel hardware.
---
Cc: Andrew Cooper <andrew.cooper3@citrix.com>
Cc: George Dunlap <George.Dunlap@eu.citrix.com>
Cc: Ian Jackson <ian.jackson@eu.citrix.com>
Cc: Jan Beulich <jbeulich@suse.com>
Cc: Julien Grall <julien.grall@arm.com>
Cc: Konrad Rzeszutek Wilk <konrad.wilk@oracle.com>
Cc: Stefano Stabellini <sstabellini@kernel.org>
Cc: Tim Deegan <tim@xen.org>
Cc: Wei Liu <wei.liu2@citrix.com>
Cc: Suravee Suthikulpanit <suravee.suthikulpanit@amd.com>
Cc: Brian Woods <brian.woods@amd.com>
Cc: Kevin Tian <kevin.tian@intel.com>
---
 docs/misc/xen-command-line.markdown         | 13 ++++-
 xen/drivers/passthrough/amd/pci_amd_iommu.c |  4 ++
 xen/drivers/passthrough/arm/iommu.c         |  4 ++
 xen/drivers/passthrough/arm/smmu.c          |  2 +
 xen/drivers/passthrough/iommu.c             | 12 +++++
 xen/drivers/passthrough/vtd/extern.h        |  2 -
 xen/drivers/passthrough/vtd/iommu.c         |  8 ++-
 xen/drivers/passthrough/vtd/x86/vtd.c       | 58 +-------------------
 xen/drivers/passthrough/x86/iommu.c         | 59 +++++++++++++++++++++
 xen/include/xen/iommu.h                     |  2 +
 10 files changed, 99 insertions(+), 65 deletions(-)

diff --git a/docs/misc/xen-command-line.markdown b/docs/misc/xen-command-line.markdown
index cd57960ede..98f0f3b68b 100644
--- a/docs/misc/xen-command-line.markdown
+++ b/docs/misc/xen-command-line.markdown
@@ -682,7 +682,7 @@ Flag that makes a dom0 use shadow paging. Only works when "pvh" is
 enabled.
 
 ### dom0-iommu
-> `= List of [ passthrough | strict ]`
+> `= List of [ passthrough | strict | map-inclusive ]`
 
 This list of booleans controls the iommu usage by Dom0:
 
@@ -696,6 +696,14 @@ This list of booleans controls the iommu usage by Dom0:
   `true` for a PVH Dom0 and any attempt to overwrite it from the command line
   is ignored.
 
+* `map-inclusive`: sets up DMA remapping for all the non-RAM regions below 4GB
+  except for unusable ranges. Use this to work around firmware issues providing
+  incorrect RMRR/IVMD entries. Rather than only mapping RAM pages for IOMMU
+  accesses for Dom0, with this option all pages up to 4GB, not marked as
+  unusable in the E820 table, will get a mapping established. Note that this
+  option is only applicable to a PV Dom0 and is enabled by default on Intel
+  hardware.
+
 ### dom0\_ioports\_disable (x86)
 > `= List of <hex>-<hex>`
 
@@ -1233,6 +1241,9 @@ wait descriptor timed out', try increasing this value.
 ### iommu\_inclusive\_mapping (VT-d)
 > `= <boolean>`
 
+**WARNING: This command line option is deprecated, and superseded by
+_dom0-iommu=map-inclusive_ - using both options in combination is undefined.**
+
 > Default: `true`
 
 Use this to work around firmware issues providing incorrect RMRR entries.
diff --git a/xen/drivers/passthrough/amd/pci_amd_iommu.c b/xen/drivers/passthrough/amd/pci_amd_iommu.c
index ab39e7500d..27eb49619d 100644
--- a/xen/drivers/passthrough/amd/pci_amd_iommu.c
+++ b/xen/drivers/passthrough/amd/pci_amd_iommu.c
@@ -253,6 +253,10 @@ static void __hwdom_init amd_iommu_hwdom_init(struct domain *d)
     unsigned long i; 
     const struct amd_iommu *iommu;
 
+    /* Inclusive IOMMU mappings are disabled by default on AMD hardware. */
+    if ( iommu_hwdom_inclusive == -1 )
+        iommu_hwdom_inclusive = false;
+
     if ( allocate_domain_resources(dom_iommu(d)) )
         BUG();
 
diff --git a/xen/drivers/passthrough/arm/iommu.c b/xen/drivers/passthrough/arm/iommu.c
index 95b1abb972..325997b19f 100644
--- a/xen/drivers/passthrough/arm/iommu.c
+++ b/xen/drivers/passthrough/arm/iommu.c
@@ -73,3 +73,7 @@ int arch_iommu_populate_page_table(struct domain *d)
     /* The IOMMU shares the p2m with the CPU */
     return -ENOSYS;
 }
+
+void __hwdom_init arch_iommu_hwdom_init(struct domain *d)
+{
+}
diff --git a/xen/drivers/passthrough/arm/smmu.c b/xen/drivers/passthrough/arm/smmu.c
index 74c09b0991..b142677b8c 100644
--- a/xen/drivers/passthrough/arm/smmu.c
+++ b/xen/drivers/passthrough/arm/smmu.c
@@ -2727,6 +2727,8 @@ static int arm_smmu_iommu_domain_init(struct domain *d)
 
 static void __hwdom_init arm_smmu_iommu_hwdom_init(struct domain *d)
 {
+	/* Set to false options not supported on ARM. */
+	iommu_hwdom_inclusive = false;
 }
 
 static void arm_smmu_iommu_domain_teardown(struct domain *d)
diff --git a/xen/drivers/passthrough/iommu.c b/xen/drivers/passthrough/iommu.c
index 8f9975cf4e..5798142730 100644
--- a/xen/drivers/passthrough/iommu.c
+++ b/xen/drivers/passthrough/iommu.c
@@ -61,6 +61,7 @@ bool_t __read_mostly iommu_intremap = 1;
 
 bool __hwdom_initdata iommu_hwdom_strict;
 bool __read_mostly iommu_hwdom_passthrough;
+int8_t __hwdom_initdata iommu_hwdom_inclusive = -1;
 
 /*
  * In the current implementation of VT-d posted interrupts, in some extreme
@@ -152,6 +153,8 @@ static int __init parse_dom0_iommu_param(const char *s)
             iommu_hwdom_passthrough = val;
         else if ( (val = parse_boolean("strict", s, ss)) >= 0 )
             iommu_hwdom_strict = val;
+        else if ( (val = parse_boolean("map-inclusive", s, ss)) >= 0 )
+            iommu_hwdom_inclusive = val;
         else
             rc = -EINVAL;
 
@@ -232,6 +235,15 @@ void __hwdom_init iommu_hwdom_init(struct domain *d)
     }
 
     hd->platform_ops->hwdom_init(d);
+
+    if ( iommu_hwdom_inclusive && !is_pv_domain(d) )
+    {
+        printk(XENLOG_WARNING
+               "IOMMU inclusive mappings are only supported on PV Dom0\n");
+        iommu_hwdom_inclusive = false;
+    }
+
+    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 32bf0fc299..a09e02c8db 100644
--- a/xen/drivers/passthrough/vtd/iommu.c
+++ b/xen/drivers/passthrough/vtd/iommu.c
@@ -1304,11 +1304,9 @@ static void __hwdom_init intel_iommu_hwdom_init(struct domain *d)
 {
     struct acpi_drhd_unit *drhd;
 
-    if ( !iommu_hwdom_passthrough && is_pv_domain(d) )
-    {
-        /* Set up 1:1 page table for hardware domain. */
-        vtd_set_hwdom_mapping(d);
-    }
+    /* Inclusive mappings are enabled by default on Intel hardware for PV. */
+    if ( iommu_hwdom_inclusive == -1 )
+        iommu_hwdom_inclusive = is_pv_domain(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 8f8682161b..4eaec2eed6 100644
--- a/xen/drivers/passthrough/vtd/x86/vtd.c
+++ b/xen/drivers/passthrough/vtd/x86/vtd.c
@@ -25,7 +25,6 @@
 #include <xen/irq.h>
 #include <xen/numa.h>
 #include <asm/fixmap.h>
-#include <asm/setup.h>
 #include "../iommu.h"
 #include "../dmar.h"
 #include "../vtd.h"
@@ -35,8 +34,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_hwdom_inclusive);
 
 void *map_vtd_domain_page(u64 maddr)
 {
@@ -108,57 +106,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, 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;
-
-        /*
-         * 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_hwdom_strict &&
-             page_is_ram_type(pfn, RAM_TYPE_CONVENTIONAL) )
-            continue;
-
-        rc = iommu_map_page(d, pfn, pfn, IOMMUF_readable|IOMMUF_writable);
-        if ( rc )
-            printk(XENLOG_WARNING VTDPREFIX " d%d: IOMMU mapping failed: %d\n",
-                   d->domain_id, rc);
-
-        if (!(i & 0xfffff))
-            process_pending_softirqs();
-    }
-}
-
diff --git a/xen/drivers/passthrough/x86/iommu.c b/xen/drivers/passthrough/x86/iommu.c
index 68182afd91..25e1ebf8b3 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,63 @@ void arch_iommu_domain_destroy(struct domain *d)
 {
 }
 
+void __hwdom_init arch_iommu_hwdom_init(struct domain *d)
+{
+    unsigned long i, top, max_pfn;
+
+    BUG_ON(!is_hardware_domain(d));
+
+    if ( iommu_hwdom_passthrough || !is_pv_domain(d) )
+        return;
+
+    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;
+
+        /*
+         * 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_hwdom_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_hwdom_strict &&
+             page_is_ram_type(pfn, RAM_TYPE_CONVENTIONAL) )
+            continue;
+
+        rc = iommu_map_page(d, pfn, pfn, IOMMUF_readable|IOMMUF_writable);
+        if ( rc )
+            printk(XENLOG_WARNING " d%d: IOMMU mapping failed: %d\n",
+                   d->domain_id, rc);
+
+        if (!(i & 0xfffff))
+            process_pending_softirqs();
+    }
+}
+
 /*
  * Local variables:
  * mode: C
diff --git a/xen/include/xen/iommu.h b/xen/include/xen/iommu.h
index 392cf1e6b9..89c6830689 100644
--- a/xen/include/xen/iommu.h
+++ b/xen/include/xen/iommu.h
@@ -37,6 +37,7 @@ extern bool_t iommu_debug;
 extern bool_t amd_iommu_perdev_intremap;
 
 extern bool iommu_hwdom_strict, iommu_hwdom_passthrough;
+extern int8_t iommu_hwdom_inclusive;
 
 extern unsigned int iommu_dev_iotlb_timeout;
 
@@ -51,6 +52,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

[PATCH v7 4/6] mm: introduce a helper to get the memory type of a page

[Roger Pau Monne]

Returns all the memory types applicable to a page.

This function is unimplemented for ARM.

Signed-off-by: Roger Pau Monné <roger.pau@citrix.com>
Reviewed-by: Jan Beulich <jbeulich@suse.com>
---
Changes since v5:
 - Return all types that apply to a page, since the types themselves
   are flags that can be or'ed together.
---
Cc: Stefano Stabellini <sstabellini@kernel.org>
Cc: Julien Grall <julien.grall@arm.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: Konrad Rzeszutek Wilk <konrad.wilk@oracle.com>
Cc: Tim Deegan <tim@xen.org>
Cc: Wei Liu <wei.liu2@citrix.com>
---
 xen/arch/x86/mm.c    | 42 ++++++++++++++++++++++++++++++++++++++++++
 xen/include/xen/mm.h |  3 +++
 2 files changed, 45 insertions(+)

diff --git a/xen/arch/x86/mm.c b/xen/arch/x86/mm.c
index 8ac4412554..fcf4bb0afc 100644
--- a/xen/arch/x86/mm.c
+++ b/xen/arch/x86/mm.c
@@ -430,6 +430,48 @@ int page_is_ram_type(unsigned long mfn, unsigned long mem_type)
     return 0;
 }
 
+unsigned int page_get_ram_type(unsigned long mfn)
+{
+    uint64_t last = 0, maddr = pfn_to_paddr(mfn);
+    unsigned int i, type = 0;
+
+    for ( i = 0; i < e820.nr_map;
+          last = e820.map[i].addr + e820.map[i].size, i++ )
+    {
+        if ( (maddr + PAGE_SIZE) > last && maddr < e820.map[i].addr )
+            type |= RAM_TYPE_UNKNOWN;
+
+        if ( (maddr + PAGE_SIZE) <= e820.map[i].addr ||
+             maddr >= (e820.map[i].addr + e820.map[i].size) )
+            continue;
+
+        switch ( e820.map[i].type )
+        {
+        case E820_RAM:
+            type |= RAM_TYPE_CONVENTIONAL;
+            break;
+
+        case E820_RESERVED:
+            type |= RAM_TYPE_RESERVED;
+            break;
+
+        case E820_UNUSABLE:
+            type |= RAM_TYPE_UNUSABLE;
+            break;
+
+        case E820_ACPI:
+        case E820_NVS:
+            type |= RAM_TYPE_ACPI;
+            break;
+
+        default:
+            ASSERT_UNREACHABLE();
+        }
+    }
+
+    return type ?: RAM_TYPE_UNKNOWN;
+}
+
 unsigned long domain_get_maximum_gpfn(struct domain *d)
 {
     if ( is_hvm_domain(d) )
diff --git a/xen/include/xen/mm.h b/xen/include/xen/mm.h
index 24654e8e22..dbaf10495a 100644
--- a/xen/include/xen/mm.h
+++ b/xen/include/xen/mm.h
@@ -595,8 +595,11 @@ int __must_check donate_page(struct domain *d, struct page_info *page,
 #define RAM_TYPE_RESERVED     0x00000002
 #define RAM_TYPE_UNUSABLE     0x00000004
 #define RAM_TYPE_ACPI         0x00000008
+#define RAM_TYPE_UNKNOWN      0x00000010
 /* TRUE if the whole page at @mfn is of the requested RAM type(s) above. */
 int page_is_ram_type(unsigned long mfn, unsigned long mem_type);
+/* Returns the page type(s). */
+unsigned int page_get_ram_type(unsigned long mfn);
 
 /* Prepare/destroy a ring for a dom0 helper. Helper with talk
  * with Xen on behalf of this domain. */
-- 
2.18.0


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

[PATCH v7 5/6] x86/iommu: switch the hwdom mapping function to use page_get_type

[Roger Pau Monne]

This avoids repeated calls to page_is_ram_type which improves
performance and makes the code easier to read.

Signed-off-by: Roger Pau Monné <roger.pau@citrix.com>
Reviewed-by: Jan Beulich <jbeulich@suse.com>
---
Changes since v4:
 - New in this version.
---
Cc: Jan Beulich <jbeulich@suse.com>
---
 xen/drivers/passthrough/x86/iommu.c | 60 +++++++++++++++--------------
 1 file changed, 32 insertions(+), 28 deletions(-)

diff --git a/xen/drivers/passthrough/x86/iommu.c b/xen/drivers/passthrough/x86/iommu.c
index 25e1ebf8b3..b947ed6043 100644
--- a/xen/drivers/passthrough/x86/iommu.c
+++ b/xen/drivers/passthrough/x86/iommu.c
@@ -134,6 +134,37 @@ void arch_iommu_domain_destroy(struct domain *d)
 {
 }
 
+static bool __hwdom_init hwdom_iommu_map(const struct domain *d,
+                                         unsigned long pfn,
+                                         unsigned long max_pfn)
+{
+    /*
+     * 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))) || xen_in_range(pfn) )
+        return false;
+
+    switch ( page_get_ram_type(pfn) )
+    {
+    case RAM_TYPE_UNUSABLE:
+        return false;
+
+    case RAM_TYPE_CONVENTIONAL:
+        if ( iommu_hwdom_strict )
+            return false;
+        break;
+
+    default:
+        if ( !iommu_hwdom_inclusive || pfn > max_pfn )
+            return false;
+    }
+
+    return true;
+}
+
 void __hwdom_init arch_iommu_hwdom_init(struct domain *d)
 {
     unsigned long i, top, max_pfn;
@@ -149,36 +180,9 @@ 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;
 
-        /*
-         * 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_hwdom_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_hwdom_strict &&
-             page_is_ram_type(pfn, RAM_TYPE_CONVENTIONAL) )
+        if ( !hwdom_iommu_map(d, pfn, max_pfn) )
             continue;
 
         rc = iommu_map_page(d, pfn, pfn, IOMMUF_readable|IOMMUF_writable);
-- 
2.18.0


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

[PATCH v7 6/6] x86/iommu: add map-reserved dom0-iommu option to map reserved memory ranges

[Roger Pau Monne]

Several people have reported hardware issues (malfunctioning USB
controllers) due to iommu page faults on Intel hardware. Those faults
are caused by missing RMRR (VTd) 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 a new dom0-iommu=map-reserved
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
is available to all Dom0 modes (as opposed to the inclusive option
which only works for PV Dom0).

Signed-off-by: Roger Pau Monné <roger.pau@citrix.com>
Reviewed-by: Kevin Tian <kevin.tian@intel.com>
---
Changes since v6:
 - Reword the map-reserved help to make it clear it's available to
   both PV and PVH Dom0.
 - Assign type inside of the switch expression.
 - Remove the comment about IO-APIC MMIO relocation, this is not
   supported ATM.

Changes since v5:
 - Merge with the vpci MMCFG helper patch.
 - Add a TODO item about the issues with relocating the LAPIC or
   IOAPIC MMIO regions.
 - Use the newly introduced page_get_ram_type that returns all the
   types that fall between a page.
 - Use paging_mode_translate instead of iommu_use_hap_pt when deciding
   whether to use set_identity_p2m_entry or iommu_map_page.

Changes since v4:
 - Use pfn_to_paddr.
 - Rebase on top of previous changes.
 - Change the default option setting to use if instead of a ternary
   operator.
 - Rename to map-reserved.

Changes since v3:
 - Add mappings if the iommu page tables are shared.

Changes since v2:
 - Fix comment regarding dom0-strict.
 - Change documentation style of xen command line.
 - Rename iommu_map to hwdom_iommu_map.
 - Move all the checks to hwdom_iommu_map.

Changes since v1:
 - Introduce a new reserved option instead of abusing the inclusive
   option.
 - Use the same helper function for PV and PVH in order to decide if a
   page should be added to the domain page tables.
 - Use the data inside of the domain struct to detect overlaps with
   emulated MMIO regions.
---
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: Paul Durrant <paul.durrant@citrix.com>
Cc: Suravee Suthikulpanit <suravee.suthikulpanit@amd.com>
Cc: Brian Woods <brian.woods@amd.com>
Cc: Kevin Tian <kevin.tian@intel.com>
---
 docs/misc/xen-command-line.markdown         |  9 ++++
 xen/arch/x86/hvm/io.c                       |  5 ++
 xen/drivers/passthrough/amd/pci_amd_iommu.c |  3 ++
 xen/drivers/passthrough/arm/smmu.c          |  1 +
 xen/drivers/passthrough/iommu.c             |  3 ++
 xen/drivers/passthrough/vtd/iommu.c         |  3 ++
 xen/drivers/passthrough/x86/iommu.c         | 53 ++++++++++++++++++---
 xen/include/asm-x86/hvm/io.h                |  3 ++
 xen/include/xen/iommu.h                     |  2 +-
 9 files changed, 75 insertions(+), 7 deletions(-)

diff --git a/docs/misc/xen-command-line.markdown b/docs/misc/xen-command-line.markdown
index 98f0f3b68b..1ffd586224 100644
--- a/docs/misc/xen-command-line.markdown
+++ b/docs/misc/xen-command-line.markdown
@@ -704,6 +704,15 @@ This list of booleans controls the iommu usage by Dom0:
   option is only applicable to a PV Dom0 and is enabled by default on Intel
   hardware.
 
+* `map-reserved`: sets up DMA remapping for all the reserved regions in the
+  memory map for Dom0. Use this to work around firmware issues providing
+  incorrect RMRR/IVMD entries. Rather than only mapping RAM pages for IOMMU
+  accesses for 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. This option maps a subset of the memory that would be
+  mapped when using the `map-inclusive` option. This option is available to all
+  Dom0 modes and is enabled by default on Intel hardware.
+
 ### dom0\_ioports\_disable (x86)
 > `= List of <hex>-<hex>`
 
diff --git a/xen/arch/x86/hvm/io.c b/xen/arch/x86/hvm/io.c
index bf4d8748d3..1f8fe36168 100644
--- a/xen/arch/x86/hvm/io.c
+++ b/xen/arch/x86/hvm/io.c
@@ -404,6 +404,11 @@ static const struct hvm_mmcfg *vpci_mmcfg_find(const struct domain *d,
     return NULL;
 }
 
+bool vpci_is_mmcfg_address(const struct domain *d, paddr_t addr)
+{
+    return vpci_mmcfg_find(d, addr);
+}
+
 static unsigned int vpci_mmcfg_decode_addr(const struct hvm_mmcfg *mmcfg,
                                            paddr_t addr, pci_sbdf_t *sbdf)
 {
diff --git a/xen/drivers/passthrough/amd/pci_amd_iommu.c b/xen/drivers/passthrough/amd/pci_amd_iommu.c
index 27eb49619d..49d934e1ac 100644
--- a/xen/drivers/passthrough/amd/pci_amd_iommu.c
+++ b/xen/drivers/passthrough/amd/pci_amd_iommu.c
@@ -256,6 +256,9 @@ static void __hwdom_init amd_iommu_hwdom_init(struct domain *d)
     /* Inclusive IOMMU mappings are disabled by default on AMD hardware. */
     if ( iommu_hwdom_inclusive == -1 )
         iommu_hwdom_inclusive = false;
+    /* Reserved IOMMU mappings are disabled by default on AMD hardware. */
+    if ( iommu_hwdom_reserved == -1 )
+        iommu_hwdom_reserved = false;
 
     if ( allocate_domain_resources(dom_iommu(d)) )
         BUG();
diff --git a/xen/drivers/passthrough/arm/smmu.c b/xen/drivers/passthrough/arm/smmu.c
index b142677b8c..8ea39659d1 100644
--- a/xen/drivers/passthrough/arm/smmu.c
+++ b/xen/drivers/passthrough/arm/smmu.c
@@ -2729,6 +2729,7 @@ static void __hwdom_init arm_smmu_iommu_hwdom_init(struct domain *d)
 {
 	/* Set to false options not supported on ARM. */
 	iommu_hwdom_inclusive = false;
+	iommu_hwdom_reserved = false;
 }
 
 static void arm_smmu_iommu_domain_teardown(struct domain *d)
diff --git a/xen/drivers/passthrough/iommu.c b/xen/drivers/passthrough/iommu.c
index 5798142730..95a471aa89 100644
--- a/xen/drivers/passthrough/iommu.c
+++ b/xen/drivers/passthrough/iommu.c
@@ -62,6 +62,7 @@ bool_t __read_mostly iommu_intremap = 1;
 bool __hwdom_initdata iommu_hwdom_strict;
 bool __read_mostly iommu_hwdom_passthrough;
 int8_t __hwdom_initdata iommu_hwdom_inclusive = -1;
+int8_t __hwdom_initdata iommu_hwdom_reserved = -1;
 
 /*
  * In the current implementation of VT-d posted interrupts, in some extreme
@@ -155,6 +156,8 @@ static int __init parse_dom0_iommu_param(const char *s)
             iommu_hwdom_strict = val;
         else if ( (val = parse_boolean("map-inclusive", s, ss)) >= 0 )
             iommu_hwdom_inclusive = val;
+        else if ( (val = parse_boolean("map-reserved", s, ss)) >= 0 )
+            iommu_hwdom_inclusive = val;
         else
             rc = -EINVAL;
 
diff --git a/xen/drivers/passthrough/vtd/iommu.c b/xen/drivers/passthrough/vtd/iommu.c
index a09e02c8db..4152c59713 100644
--- a/xen/drivers/passthrough/vtd/iommu.c
+++ b/xen/drivers/passthrough/vtd/iommu.c
@@ -1307,6 +1307,9 @@ static void __hwdom_init intel_iommu_hwdom_init(struct domain *d)
     /* Inclusive mappings are enabled by default on Intel hardware for PV. */
     if ( iommu_hwdom_inclusive == -1 )
         iommu_hwdom_inclusive = is_pv_domain(d);
+    /* Reserved IOMMU mappings are enabled by default on Intel hardware. */
+    if ( iommu_hwdom_reserved == -1 )
+        iommu_hwdom_reserved = true;
 
     setup_hwdom_pci_devices(d, setup_hwdom_device);
     setup_hwdom_rmrr(d);
diff --git a/xen/drivers/passthrough/x86/iommu.c b/xen/drivers/passthrough/x86/iommu.c
index b947ed6043..c85bf0bcd7 100644
--- a/xen/drivers/passthrough/x86/iommu.c
+++ b/xen/drivers/passthrough/x86/iommu.c
@@ -20,6 +20,7 @@
 #include <xen/softirq.h>
 #include <xsm/xsm.h>
 
+#include <asm/hvm/io.h>
 #include <asm/setup.h>
 
 void iommu_update_ire_from_apic(
@@ -138,16 +139,23 @@ static bool __hwdom_init hwdom_iommu_map(const struct domain *d,
                                          unsigned long pfn,
                                          unsigned long max_pfn)
 {
+    unsigned int i, type;
+
     /*
      * 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.
+     * that fall in unusable ranges for PV Dom0.
      */
-    if ( (pfn > max_pfn && !mfn_valid(_mfn(pfn))) || xen_in_range(pfn) )
+    if ( (pfn > max_pfn && !mfn_valid(_mfn(pfn))) || xen_in_range(pfn) ||
+         /*
+          * Ignore any address below 1MB, that's already identity mapped by the
+          * Dom0 builder for HVM.
+          */
+         (!d->domain_id && is_hvm_domain(d) && pfn < PFN_DOWN(MB(1))) )
         return false;
 
-    switch ( page_get_ram_type(pfn) )
+    switch ( type = page_get_ram_type(pfn) )
     {
     case RAM_TYPE_UNUSABLE:
         return false;
@@ -158,10 +166,40 @@ static bool __hwdom_init hwdom_iommu_map(const struct domain *d,
         break;
 
     default:
-        if ( !iommu_hwdom_inclusive || pfn > max_pfn )
+        if ( type & RAM_TYPE_RESERVED )
+        {
+            if ( !iommu_hwdom_inclusive && !iommu_hwdom_reserved )
+                return false;
+        }
+        else if ( is_hvm_domain(d) || !iommu_hwdom_inclusive || pfn > max_pfn )
             return false;
     }
 
+    /*
+     * Check that it doesn't overlap with the LAPIC
+     * TODO: if the guest relocates the MMIO area of the LAPIC Xen should make
+     * sure there's nothing in the new address that would prevent trapping.
+     */
+    if ( has_vlapic(d) )
+    {
+        const struct vcpu *v;
+
+        for_each_vcpu(d, v)
+            if ( pfn == PFN_DOWN(vlapic_base_address(vcpu_vlapic(v))) )
+                return false;
+    }
+    /* ... or the IO-APIC */
+    for ( i = 0; has_vioapic(d) && i < d->arch.hvm_domain.nr_vioapics; i++ )
+        if ( pfn == PFN_DOWN(domain_vioapic(d, i)->base_address) )
+            return false;
+    /*
+     * ... or the PCIe MCFG regions.
+     * TODO: runtime added MMCFG regions are not checked to make sure they
+     * don't overlap with already mapped regions, thus preventing trapping.
+     */
+    if ( has_vpci(d) && vpci_is_mmcfg_address(d, pfn_to_paddr(pfn)) )
+        return false;
+
     return true;
 }
 
@@ -171,7 +209,7 @@ void __hwdom_init arch_iommu_hwdom_init(struct domain *d)
 
     BUG_ON(!is_hardware_domain(d));
 
-    if ( iommu_hwdom_passthrough || !is_pv_domain(d) )
+    if ( iommu_hwdom_passthrough )
         return;
 
     max_pfn = (GB(4) >> PAGE_SHIFT) - 1;
@@ -185,7 +223,10 @@ void __hwdom_init arch_iommu_hwdom_init(struct domain *d)
         if ( !hwdom_iommu_map(d, pfn, max_pfn) )
             continue;
 
-        rc = iommu_map_page(d, pfn, pfn, IOMMUF_readable|IOMMUF_writable);
+        if ( paging_mode_translate(d) )
+            rc = set_identity_p2m_entry(d, pfn, p2m_access_rw, 0);
+        else
+            rc = iommu_map_page(d, pfn, pfn, IOMMUF_readable|IOMMUF_writable);
         if ( rc )
             printk(XENLOG_WARNING " d%d: IOMMU mapping failed: %d\n",
                    d->domain_id, rc);
diff --git a/xen/include/asm-x86/hvm/io.h b/xen/include/asm-x86/hvm/io.h
index e6b6ed0b92..83431b44f2 100644
--- a/xen/include/asm-x86/hvm/io.h
+++ b/xen/include/asm-x86/hvm/io.h
@@ -180,6 +180,9 @@ int register_vpci_mmcfg_handler(struct domain *d, paddr_t addr,
 /* Destroy tracked MMCFG areas. */
 void destroy_vpci_mmcfg(struct domain *d);
 
+/* Check if an address is between a MMCFG region for a domain. */
+bool vpci_is_mmcfg_address(const struct domain *d, paddr_t addr);
+
 #endif /* __ASM_X86_HVM_IO_H__ */
 
 
diff --git a/xen/include/xen/iommu.h b/xen/include/xen/iommu.h
index 89c6830689..57c4e81ec6 100644
--- a/xen/include/xen/iommu.h
+++ b/xen/include/xen/iommu.h
@@ -37,7 +37,7 @@ extern bool_t iommu_debug;
 extern bool_t amd_iommu_perdev_intremap;
 
 extern bool iommu_hwdom_strict, iommu_hwdom_passthrough;
-extern int8_t iommu_hwdom_inclusive;
+extern int8_t iommu_hwdom_inclusive, iommu_hwdom_reserved;
 
 extern unsigned int iommu_dev_iotlb_timeout;
 
-- 
2.18.0


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

Re: [PATCH v7 2/6] iommu: introduce dom0-iommu option

[Wei Liu]

On Wed, Aug 22, 2018 at 09:51:56AM +0200, Roger Pau Monne wrote:
> To select the iommu configuration used by Dom0. This option supersedes
> iommu=dom0-strict|dom0-passthrough.
> 
> Signed-off-by: Roger Pau Monné <roger.pau@citrix.com>
> Reviewed-by: Jan Beulich <jbeulich@suse.com>

Reviewed-by: Wei Liu <wei.liu2@citrix.com>

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

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

[Wei Liu]

On Wed, Aug 22, 2018 at 09:51:57AM +0200, Roger Pau Monne wrote:
> Introduce a new dom0-iommu=map-inclusive generic option that
> supersedes iommu_inclusive_mapping. The previous behavior is preserved
> and the option should only be enabled by default on Intel hardware.
> 
> Signed-off-by: Roger Pau Monné <roger.pau@citrix.com>
> Reviewed-by: Paul Durrant <paul.durrant@citrix.com>
> Reviewed-by: Jan Beulich <jbeulich@suse.com>
> Reviewed-by: Kevin Tian <kevin.tian@intel.com>

Reviewed-by: Wei Liu <wei.liu2@citrix.com>

With one nit below:

> diff --git a/xen/drivers/passthrough/arm/smmu.c b/xen/drivers/passthrough/arm/smmu.c
> index 74c09b0991..b142677b8c 100644
> --- a/xen/drivers/passthrough/arm/smmu.c
> +++ b/xen/drivers/passthrough/arm/smmu.c
> @@ -2727,6 +2727,8 @@ static int arm_smmu_iommu_domain_init(struct domain *d)
>  
>  static void __hwdom_init arm_smmu_iommu_hwdom_init(struct domain *d)
>  {
> +	/* Set to false options not supported on ARM. */

Missing a comma after "false".

And in case it is not clear to Julien and Stefano, an ack from ARM side
is needed.

Wei.

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

Re: [PATCH v7 4/6] mm: introduce a helper to get the memory type of a page

[Wei Liu]

On Wed, Aug 22, 2018 at 09:51:58AM +0200, Roger Pau Monne wrote:
> Returns all the memory types applicable to a page.
> 
> This function is unimplemented for ARM.
> 
> Signed-off-by: Roger Pau Monné <roger.pau@citrix.com>
> Reviewed-by: Jan Beulich <jbeulich@suse.com>
> ---
> Changes since v5:
>  - Return all types that apply to a page, since the types themselves
>    are flags that can be or'ed together.
> ---
> Cc: Stefano Stabellini <sstabellini@kernel.org>
> Cc: Julien Grall <julien.grall@arm.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: Konrad Rzeszutek Wilk <konrad.wilk@oracle.com>
> Cc: Tim Deegan <tim@xen.org>
> Cc: Wei Liu <wei.liu2@citrix.com>
> ---
>  xen/arch/x86/mm.c    | 42 ++++++++++++++++++++++++++++++++++++++++++
>  xen/include/xen/mm.h |  3 +++
>  2 files changed, 45 insertions(+)
> 
> diff --git a/xen/arch/x86/mm.c b/xen/arch/x86/mm.c
> index 8ac4412554..fcf4bb0afc 100644
> --- a/xen/arch/x86/mm.c
> +++ b/xen/arch/x86/mm.c
> @@ -430,6 +430,48 @@ int page_is_ram_type(unsigned long mfn, unsigned long mem_type)
>      return 0;
>  }
>  
> +unsigned int page_get_ram_type(unsigned long mfn)

It would be better if you can use type-safe mfn_t here.


> +{
> +    uint64_t last = 0, maddr = pfn_to_paddr(mfn);

And then

    maddr = mfn_to_maddr(mfn);

The rest looks good to me.

Wei.

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

Re: [PATCH v7 1/6] iommu: rename iommu_dom0_strict and iommu_passthrough

[Wei Liu]

On Wed, Aug 22, 2018 at 09:51:55AM +0200, Roger Pau Monne wrote:
> To iommu_hwdom_strict and iommu_hwdom_passthrough which is more
> descriptive of their usage. Also change their type from bool_t to
> bool.
> 
> No functional change.
> 
> Signed-off-by: Roger Pau Monné <roger.pau@citrix.com>
> Reviewed-by: Jan Beulich <jbeulich@suse.com>
> Reviewed-by: Kevin Tian <kevin.tian@intel.com>

Reviewed-by: Wei Liu <wei.liu2@citrix.com>

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

Re: [PATCH v7 5/6] x86/iommu: switch the hwdom mapping function to use page_get_type

[Wei Liu]

On Wed, Aug 22, 2018 at 09:51:59AM +0200, Roger Pau Monne wrote:
> This avoids repeated calls to page_is_ram_type which improves
> performance and makes the code easier to read.
> 
> Signed-off-by: Roger Pau Monné <roger.pau@citrix.com>
> Reviewed-by: Jan Beulich <jbeulich@suse.com>

Reviewed-by: Wei Liu <wei.liu2@citrix.com>

> ---
> Changes since v4:
>  - New in this version.
> ---
> Cc: Jan Beulich <jbeulich@suse.com>
> ---
>  xen/drivers/passthrough/x86/iommu.c | 60 +++++++++++++++--------------
>  1 file changed, 32 insertions(+), 28 deletions(-)
> 
> diff --git a/xen/drivers/passthrough/x86/iommu.c b/xen/drivers/passthrough/x86/iommu.c
> index 25e1ebf8b3..b947ed6043 100644
> --- a/xen/drivers/passthrough/x86/iommu.c
> +++ b/xen/drivers/passthrough/x86/iommu.c
> @@ -134,6 +134,37 @@ void arch_iommu_domain_destroy(struct domain *d)
>  {
>  }
>  
> +static bool __hwdom_init hwdom_iommu_map(const struct domain *d,
> +                                         unsigned long pfn,
> +                                         unsigned long max_pfn)

Consider using type-safe types if possible?

Wei.

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

Re: [PATCH v7 6/6] x86/iommu: add map-reserved dom0-iommu option to map reserved memory ranges

[Wei Liu]

On Wed, Aug 22, 2018 at 09:52:00AM +0200, Roger Pau Monne wrote:
> Several people have reported hardware issues (malfunctioning USB
> controllers) due to iommu page faults on Intel hardware. Those faults
> are caused by missing RMRR (VTd) 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 a new dom0-iommu=map-reserved
> 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
> is available to all Dom0 modes (as opposed to the inclusive option
> which only works for PV Dom0).
> 
> Signed-off-by: Roger Pau Monné <roger.pau@citrix.com>
> Reviewed-by: Kevin Tian <kevin.tian@intel.com>

Reviewed-by: Wei Liu <wei.liu2@citrix.com>

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

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

[Julien Grall]

Hi Roger,

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

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

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

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

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

Same remark as above about mixing boolean and integer.

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

Cheers,

-- 
Julien Grall

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

Re: [PATCH v7 6/6] x86/iommu: add map-reserved dom0-iommu option to map reserved memory ranges

[Julien Grall]

Hi Roger,

On 22/08/18 08:52, Roger Pau Monne wrote:
> Several people have reported hardware issues (malfunctioning USB
> controllers) due to iommu page faults on Intel hardware. Those faults
> are caused by missing RMRR (VTd) 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 a new dom0-iommu=map-reserved
> 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
> is available to all Dom0 modes (as opposed to the inclusive option
> which only works for PV Dom0).
> 
> Signed-off-by: Roger Pau Monné <roger.pau@citrix.com>
> Reviewed-by: Kevin Tian <kevin.tian@intel.com>
> ---
> Changes since v6:
>   - Reword the map-reserved help to make it clear it's available to
>     both PV and PVH Dom0.
>   - Assign type inside of the switch expression.
>   - Remove the comment about IO-APIC MMIO relocation, this is not
>     supported ATM.
> 
> Changes since v5:
>   - Merge with the vpci MMCFG helper patch.
>   - Add a TODO item about the issues with relocating the LAPIC or
>     IOAPIC MMIO regions.
>   - Use the newly introduced page_get_ram_type that returns all the
>     types that fall between a page.
>   - Use paging_mode_translate instead of iommu_use_hap_pt when deciding
>     whether to use set_identity_p2m_entry or iommu_map_page.
> 
> Changes since v4:
>   - Use pfn_to_paddr.
>   - Rebase on top of previous changes.
>   - Change the default option setting to use if instead of a ternary
>     operator.
>   - Rename to map-reserved.
> 
> Changes since v3:
>   - Add mappings if the iommu page tables are shared.
> 
> Changes since v2:
>   - Fix comment regarding dom0-strict.
>   - Change documentation style of xen command line.
>   - Rename iommu_map to hwdom_iommu_map.
>   - Move all the checks to hwdom_iommu_map.
> 
> Changes since v1:
>   - Introduce a new reserved option instead of abusing the inclusive
>     option.
>   - Use the same helper function for PV and PVH in order to decide if a
>     page should be added to the domain page tables.
>   - Use the data inside of the domain struct to detect overlaps with
>     emulated MMIO regions.
> ---
> 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: Paul Durrant <paul.durrant@citrix.com>
> Cc: Suravee Suthikulpanit <suravee.suthikulpanit@amd.com>
> Cc: Brian Woods <brian.woods@amd.com>
> Cc: Kevin Tian <kevin.tian@intel.com>
> ---
>   docs/misc/xen-command-line.markdown         |  9 ++++
>   xen/arch/x86/hvm/io.c                       |  5 ++
>   xen/drivers/passthrough/amd/pci_amd_iommu.c |  3 ++
>   xen/drivers/passthrough/arm/smmu.c          |  1 +
>   xen/drivers/passthrough/iommu.c             |  3 ++
>   xen/drivers/passthrough/vtd/iommu.c         |  3 ++
>   xen/drivers/passthrough/x86/iommu.c         | 53 ++++++++++++++++++---
>   xen/include/asm-x86/hvm/io.h                |  3 ++
>   xen/include/xen/iommu.h                     |  2 +-
>   9 files changed, 75 insertions(+), 7 deletions(-)
> 
> diff --git a/docs/misc/xen-command-line.markdown b/docs/misc/xen-command-line.markdown
> index 98f0f3b68b..1ffd586224 100644
> --- a/docs/misc/xen-command-line.markdown
> +++ b/docs/misc/xen-command-line.markdown
> @@ -704,6 +704,15 @@ This list of booleans controls the iommu usage by Dom0:
>     option is only applicable to a PV Dom0 and is enabled by default on Intel
>     hardware.
>   
> +* `map-reserved`: sets up DMA remapping for all the reserved regions in the
> +  memory map for Dom0. Use this to work around firmware issues providing
> +  incorrect RMRR/IVMD entries. Rather than only mapping RAM pages for IOMMU
> +  accesses for 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. This option maps a subset of the memory that would be
> +  mapped when using the `map-inclusive` option. This option is available to all
> +  Dom0 modes and is enabled by default on Intel hardware.
> +
>   ### dom0\_ioports\_disable (x86)
>   > `= List of <hex>-<hex>`
>   
> diff --git a/xen/arch/x86/hvm/io.c b/xen/arch/x86/hvm/io.c
> index bf4d8748d3..1f8fe36168 100644
> --- a/xen/arch/x86/hvm/io.c
> +++ b/xen/arch/x86/hvm/io.c
> @@ -404,6 +404,11 @@ static const struct hvm_mmcfg *vpci_mmcfg_find(const struct domain *d,
>       return NULL;
>   }
>   
> +bool vpci_is_mmcfg_address(const struct domain *d, paddr_t addr)
> +{
> +    return vpci_mmcfg_find(d, addr);
> +}
> +
>   static unsigned int vpci_mmcfg_decode_addr(const struct hvm_mmcfg *mmcfg,
>                                              paddr_t addr, pci_sbdf_t *sbdf)
>   {
> diff --git a/xen/drivers/passthrough/amd/pci_amd_iommu.c b/xen/drivers/passthrough/amd/pci_amd_iommu.c
> index 27eb49619d..49d934e1ac 100644
> --- a/xen/drivers/passthrough/amd/pci_amd_iommu.c
> +++ b/xen/drivers/passthrough/amd/pci_amd_iommu.c
> @@ -256,6 +256,9 @@ static void __hwdom_init amd_iommu_hwdom_init(struct domain *d)
>       /* Inclusive IOMMU mappings are disabled by default on AMD hardware. */
>       if ( iommu_hwdom_inclusive == -1 )
>           iommu_hwdom_inclusive = false;
> +    /* Reserved IOMMU mappings are disabled by default on AMD hardware. */
> +    if ( iommu_hwdom_reserved == -1 )
> +        iommu_hwdom_reserved = false;

Same as patch #1, you are mixing boolean and integer.

Cheers,

-- 
Julien Grall

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

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

[Roger Pau Monné]

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

I don't think I can fix that. An integer is used so that the default
value is set to -1. Then at runtime a default value based on the
hardware is set unless the user has already specified a value on the
command line.

It needs to be a tristate in order to detect whether the user has
specified a value on the command line.

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

Sure.

Thanks, Roger.

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

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

[Julien Grall]

Hi,

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

In that case the solution is to use -1, 0, 1. So replacing false with 0 
and true with 1.

Cheers,

-- 
Julien Grall

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

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

[Roger Pau Monné]

On Wed, Aug 22, 2018 at 10:45:28AM +0100, Wei Liu wrote:
> On Wed, Aug 22, 2018 at 09:51:57AM +0200, Roger Pau Monne wrote:
> > Introduce a new dom0-iommu=map-inclusive generic option that
> > supersedes iommu_inclusive_mapping. The previous behavior is preserved
> > and the option should only be enabled by default on Intel hardware.
> > 
> > Signed-off-by: Roger Pau Monné <roger.pau@citrix.com>
> > Reviewed-by: Paul Durrant <paul.durrant@citrix.com>
> > Reviewed-by: Jan Beulich <jbeulich@suse.com>
> > Reviewed-by: Kevin Tian <kevin.tian@intel.com>
> 
> Reviewed-by: Wei Liu <wei.liu2@citrix.com>
> 
> With one nit below:
> 
> > diff --git a/xen/drivers/passthrough/arm/smmu.c b/xen/drivers/passthrough/arm/smmu.c
> > index 74c09b0991..b142677b8c 100644
> > --- a/xen/drivers/passthrough/arm/smmu.c
> > +++ b/xen/drivers/passthrough/arm/smmu.c
> > @@ -2727,6 +2727,8 @@ static int arm_smmu_iommu_domain_init(struct domain *d)
> >  
> >  static void __hwdom_init arm_smmu_iommu_hwdom_init(struct domain *d)
> >  {
> > +	/* Set to false options not supported on ARM. */
> 
> Missing a comma after "false".

It seems weird to me to add a comma there, at least if I was to read
the sentence I won't make a pause there.

Thanks, Roger.

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

Re: [PATCH v7 4/6] mm: introduce a helper to get the memory type of a page

[Roger Pau Monné]

On Wed, Aug 22, 2018 at 10:45:34AM +0100, Wei Liu wrote:
> On Wed, Aug 22, 2018 at 09:51:58AM +0200, Roger Pau Monne wrote:
> > Returns all the memory types applicable to a page.
> > 
> > This function is unimplemented for ARM.
> > 
> > Signed-off-by: Roger Pau Monné <roger.pau@citrix.com>
> > Reviewed-by: Jan Beulich <jbeulich@suse.com>
> > ---
> > Changes since v5:
> >  - Return all types that apply to a page, since the types themselves
> >    are flags that can be or'ed together.
> > ---
> > Cc: Stefano Stabellini <sstabellini@kernel.org>
> > Cc: Julien Grall <julien.grall@arm.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: Konrad Rzeszutek Wilk <konrad.wilk@oracle.com>
> > Cc: Tim Deegan <tim@xen.org>
> > Cc: Wei Liu <wei.liu2@citrix.com>
> > ---
> >  xen/arch/x86/mm.c    | 42 ++++++++++++++++++++++++++++++++++++++++++
> >  xen/include/xen/mm.h |  3 +++
> >  2 files changed, 45 insertions(+)
> > 
> > diff --git a/xen/arch/x86/mm.c b/xen/arch/x86/mm.c
> > index 8ac4412554..fcf4bb0afc 100644
> > --- a/xen/arch/x86/mm.c
> > +++ b/xen/arch/x86/mm.c
> > @@ -430,6 +430,48 @@ int page_is_ram_type(unsigned long mfn, unsigned long mem_type)
> >      return 0;
> >  }
> >  
> > +unsigned int page_get_ram_type(unsigned long mfn)
> 
> It would be better if you can use type-safe mfn_t here.
> 
> 
> > +{
> > +    uint64_t last = 0, maddr = pfn_to_paddr(mfn);
> 
> And then
> 
>     maddr = mfn_to_maddr(mfn);
> 
> The rest looks good to me.

OK, I haven't done it before because the only caller already uses
unsigned long to deal with mfns, and the function itself will also
end up translating the mfn into unsigned long, so it seemed to me
quite pointless to add this extra translation that doesn't seem to
benefit either the caller or the function itself, but will change it
now.

Thanks, Roger.

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

Re: [PATCH v7 0/6] x86/iommu: PVH Dom0 workarounds for missing RMRR entries

[Jan Beulich]

>>> On 22.08.18 at 09:51, <roger.pau@citrix.com> wrote:
> Hello,
> 
> The following series implement a workaround for missing RMRR
> entries for a PVH Dom0. It's based on the iommu_inclusive_mapping VTd
> option.
> 
> The PVH workaround identity maps all regions marked as reserved in the
> memory map.
> 
> Note that this workaround is enabled by default on Intel hardware. It's
> also available to AMD hardware, although it's disabled by default in
> that case.
> 
> The series can be found at:
> 
> git://xenbits.xen.org/people/royger/xen.git iommu_inclusive_v7
> 
> Thanks.
> 
> Roger Pau Monne (6):
>   iommu: rename iommu_dom0_strict and iommu_passthrough
>   iommu: introduce dom0-iommu option
>   iommu: make iommu_inclusive_mapping a suboption of dom0-iommu

Brian, Suravee,

the first three patches of this series could go in if only we had
an ack from one of you on the first and third...

Jan



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

Re: [PATCH v7 6/6] x86/iommu: add map-reserved dom0-iommu option to map reserved memory ranges

[Jan Beulich]

>>> On 22.08.18 at 09:52, <roger.pau@citrix.com> wrote:
> Several people have reported hardware issues (malfunctioning USB
> controllers) due to iommu page faults on Intel hardware. Those faults
> are caused by missing RMRR (VTd) 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 a new dom0-iommu=map-reserved
> 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
> is available to all Dom0 modes (as opposed to the inclusive option
> which only works for PV Dom0).
> 
> Signed-off-by: Roger Pau Monné <roger.pau@citrix.com>
> Reviewed-by: Kevin Tian <kevin.tian@intel.com>

Acked-by: Jan Beulich <jbeulich@suse.com>


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