[PATCH v2 0/4] Functional and documentation improvements to dom0 setup

[PATCH v2 0/4] Functional and documentation improvements to dom0 setup

[Andrew Cooper]

Expanded somewhat from v1.  See patches for details.

Andrew Cooper (4):
  xen/dom0: Improve documentation for dom0= and dom0-iommu=
  x86/dom0: Improve dom0= useability
  xen/dom0: Drop iommu_hwdom_inclusive entirely
  xen/dom0: Add a dom0-iommu=none option

 docs/misc/xen-command-line.markdown   | 130 +++++++++++++++++-----------------
 xen/arch/x86/dom0_build.c             |  14 ++--
 xen/common/kernel.c                   |  20 ++++++
 xen/drivers/passthrough/arm/smmu.c    |   4 --
 xen/drivers/passthrough/iommu.c       |   8 +--
 xen/drivers/passthrough/vtd/x86/vtd.c |   6 --
 xen/drivers/passthrough/x86/iommu.c   |  14 +---
 xen/include/xen/iommu.h               |   2 +-
 xen/include/xen/lib.h                 |   7 ++
 9 files changed, 105 insertions(+), 100 deletions(-)

-- 
2.1.4


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

[PATCH v2 1/4] xen/dom0: Improve documentation for dom0= and dom0-iommu=

[Andrew Cooper]

Update to the latest metadata style, and expand each of the clauses with more
information, including applicable CONFIG_* options.

Drop the redundant comment beside parse_dom0_param(), to avoid it getting out
of sync with the main documentation.  Also drop the individual documentation
for deprecated options which refer to their newer versions, for the same
reason.

Signed-off-by: Andrew Cooper <andrew.cooper3@citrix.com>
---
CC: Jan Beulich <JBeulich@suse.com>
CC: Wei Liu <wei.liu2@citrix.com>
CC: Roger Pau Monné <roger.pau@citrix.com>
CC: Stefano Stabellini <sstabellini@kernel.org>
CC: Julien Grall <julien.grall@arm.com>

Please double check for correctness.  The text matches my
understanding/reading of the code, but some of it is rather subtle going.

v2:
 * Fix statement of defaults
 * Tweak wording.  In particular, expand the description of passthrough.
---
 docs/misc/xen-command-line.markdown | 127 +++++++++++++++++++-----------------
 xen/arch/x86/dom0_build.c           |   6 --
 2 files changed, 67 insertions(+), 66 deletions(-)

diff --git a/docs/misc/xen-command-line.markdown b/docs/misc/xen-command-line.markdown
index 78b207c..a173f10 100644
--- a/docs/misc/xen-command-line.markdown
+++ b/docs/misc/xen-command-line.markdown
@@ -636,55 +636,80 @@ trace feature is only enabled in debugging builds of Xen.
 
 Specify the bit width of the DMA heap.
 
-### dom0 (x86)
-> `= List of [ pvh | shadow ]`
+### dom0
+> `= List of [ pvh=<bool>, shadow=<bool> ]`
 
-> Sub-options:
+> Applicability: x86
 
-> `pvh`
+Controls for how dom0 is constructed on x86 systems.
 
-> Default: `false`
+*   The `pvh` boolean controls whether dom0 is constructed as a PV or a PVH
+    guest.  The default is PV.  In addition, the following requirements must
+    be met:
 
-Flag that makes a dom0 boot in PVHv2 mode.
+    *    The dom0 kernel selected by the boot loader must be capable of the
+         selected mode.
+    *    For a PV dom0, Xen must have been compiled with `CONFIG_PV` enabled.
+    *    For a PVH dom0, Xen must have been compiled with `CONFIG_HVM` enabled,
+         and the hardware must have VT-x/SVM extensions available.
 
-> `shadow`
+*   The `shadow` boolean is only applicable when dom0 is constructed as a PVH
+    guest, and controls whether dom0 uses hardware assisted paging, or shadow
+    paging.  The default is HAP when available, and shadow otherwise.
 
-> Default: `false`
-
-Flag that makes a dom0 use shadow paging. Only works when "pvh" is
-enabled.
+    This option is unavailable when `CONFIG_SHADOW_PAGING` is compiled out.  A
+    PVH dom0 cannot be used if `CONFIG_SHADOW_PAGING` is compiled out, and the
+    hardware is not HAP-capable.
 
 ### dom0-iommu
-> `= List of [ passthrough | strict | map-inclusive ]`
-
-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.
-
-* `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.
-
-* `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.
+> `= List of [ passthrough=<bool>, strict=<bool>, map-inclusive=<bool>,
+>              map-reserved=<bool> ]`
+
+Controls for the dom0 IOMMU setup.
+
+*   The `passthrough` boolean controls whether IOMMU translation functionality
+    is disabled for devices in dom0 (`passthrough=1`) or whether the IOMMU is
+    used to ensure that dom0 can only DMA to its permitted areas of RAM
+    (`passthrough=0`).
+
+    This option is only applicable to x86 PV dom0's, and defaults to false.
+
+    Some older Intel VT-d hardware isn't capable of disabling translation
+    functionality on a per-device basis, and will cause this option to be
+    ignored and assumed to be 0.  Similar behaviour on such systems is only
+    available by fully disabling all IOMMUs.
+
+    This option is hardwired to false for x86 PVH dom0's (where a non-identity
+    transform is required for dom0 to function), and is ignored for ARM.
+
+*   The `strict` boolean is applicable to x86 PV dom0's only and defaults to
+    false.  It controls whether dom0 can have IOMMU mappings for all domain
+    RAM in the system, or only for its allocated RAM (and grant mappings etc.)
+
+    This option is hardwired to true for x86 PVH dom0's (as RAM belonging to
+    other domains in the system don't live in a compatible address space), and
+    is ignored for ARM.
+
+*   The `map-inclusive` boolean is applicable to x86 PV dom0's, and sets up
+    identity IOMMU mappings for all non-RAM regions below 4GB except for
+    unusable ranges, and ranges belonging to Xen.
+
+    Typically, some devices in a system use bits of RAM for communication, and
+    these areas should be listed as reserved in the E820 table and identified
+    via RMRR or IVMD entries in the APCI tables, so Xen can ensure that they
+    are identity-mapped in the IOMMU.  However, some firmware makes mistakes,
+    and this option is a coarse-grain workaround for those errors.
+
+    Where possible, finer grain corrections should be made with the `rmrr=`,
+    `ivrs_hpet=` or `ivrs_ioapic=` command line options.
+
+    This option is enabled by default on x86 systems, and invalid on ARM
+    systems.
+
+*   The `map-reserved` functionality is very similar to `map-inclusive`, but is
+    applicable to both x86 PV and PVH dom0's, and represents a subset of the
+    correction by only mapping reserved memory regions rather than all non-RAM
+    regions.
 
 ### dom0\_ioports\_disable (x86)
 > `= List of <hex>-<hex>`
@@ -1175,20 +1200,11 @@ detection of systems known to misbehave upon accesses to that port.
 > **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
->> got assigned. Implies `no-dom0-passthrough`.
-
 > `amd-iommu-perdev-intremap`
 
 > Default: `true`
@@ -1235,21 +1251,12 @@ Specify the timeout of the device IOTLB invalidation in milliseconds.
 By default, the timeout is 1000 ms. When you see error 'Queue invalidate
 wait descriptor timed out', try increasing this value.
 
-### iommu\_inclusive\_mapping (VT-d)
+### iommu_inclusive_mapping
 > `= <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.
-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.
-
 ### irq\_ratelimit (x86)
 > `= <integer>`
 
diff --git a/xen/arch/x86/dom0_build.c b/xen/arch/x86/dom0_build.c
index 54737da..85d4ff2 100644
--- a/xen/arch/x86/dom0_build.c
+++ b/xen/arch/x86/dom0_build.c
@@ -282,12 +282,6 @@ bool __initdata opt_dom0_shadow;
 #endif
 bool __initdata dom0_pvh;
 
-/*
- * List of parameters that affect Dom0 creation:
- *
- *  - pvh               Create a PVHv2 Dom0.
- *  - shadow            Use shadow paging for Dom0.
- */
 static int __init parse_dom0_param(const char *s)
 {
     const char *ss;
-- 
2.1.4


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

[PATCH v2 2/4] x86/dom0: Improve dom0= useability

[Andrew Cooper]

Having a pvh boolean isn't ideal.  If we gain a 3rd virtulsation mode,
what does `dom0=no-pvh` mean?

Change the syntax to be "dom0 = pv | pvh" which offers an option to more
obviously select PV mode.  Hide both options behind the relevent
CONFIG_* settings, and default to PVH mode when CONFIG_PV is compiled
out.

Signed-off-by: Andrew Cooper <andrew.cooper3@citrix.com>
---
CC: Jan Beulich <JBeulich@suse.com>
CC: Wei Liu <wei.liu2@citrix.com>
CC: Roger Pau Monné <roger.pau@citrix.com>

v2:
 * New
---
 docs/misc/xen-command-line.markdown | 16 +++++++++-------
 xen/arch/x86/dom0_build.c           |  8 +++++---
 2 files changed, 14 insertions(+), 10 deletions(-)

diff --git a/docs/misc/xen-command-line.markdown b/docs/misc/xen-command-line.markdown
index a173f10..0aeb786 100644
--- a/docs/misc/xen-command-line.markdown
+++ b/docs/misc/xen-command-line.markdown
@@ -637,21 +637,23 @@ trace feature is only enabled in debugging builds of Xen.
 Specify the bit width of the DMA heap.
 
 ### dom0
-> `= List of [ pvh=<bool>, shadow=<bool> ]`
+> `= List of [ pv | pvh, shadow=<bool> ]`
 
 > Applicability: x86
 
 Controls for how dom0 is constructed on x86 systems.
 
-*   The `pvh` boolean controls whether dom0 is constructed as a PV or a PVH
-    guest.  The default is PV.  In addition, the following requirements must
-    be met:
+*   The `pv` and `pvh` options select the virtualisation mode of dom0.
+
+    The `pv` option is only available when `CONFIG_PV` is compiled in.  The
+    `pvh` option is only available when `CONFIG_HVM` is compiled in.  When
+    both options are compiled in, the default is `pv`.
+
+    In addition, the following requirements must be met:
 
     *    The dom0 kernel selected by the boot loader must be capable of the
          selected mode.
-    *    For a PV dom0, Xen must have been compiled with `CONFIG_PV` enabled.
-    *    For a PVH dom0, Xen must have been compiled with `CONFIG_HVM` enabled,
-         and the hardware must have VT-x/SVM extensions available.
+    *    For a PVH dom0, the hardware must have VT-x/SVM extensions available.
 
 *   The `shadow` boolean is only applicable when dom0 is constructed as a PVH
     guest, and controls whether dom0 uses hardware assisted paging, or shadow
diff --git a/xen/arch/x86/dom0_build.c b/xen/arch/x86/dom0_build.c
index 85d4ff2..85c1493 100644
--- a/xen/arch/x86/dom0_build.c
+++ b/xen/arch/x86/dom0_build.c
@@ -280,7 +280,7 @@ struct vcpu *__init alloc_dom0_vcpu0(struct domain *dom0)
 #ifdef CONFIG_SHADOW_PAGING
 bool __initdata opt_dom0_shadow;
 #endif
-bool __initdata dom0_pvh;
+bool __initdata dom0_pvh = !IS_ENABLED(CONFIG_PV);
 
 static int __init parse_dom0_param(const char *s)
 {
@@ -294,8 +294,10 @@ static int __init parse_dom0_param(const char *s)
         if ( !ss )
             ss = strchr(s, '\0');
 
-        if ( (val = parse_boolean("pvh", s, ss)) >= 0 )
-            dom0_pvh = val;
+        if ( IS_ENABLED(CONFIG_PV) && !strncmp("pv", s, ss - s) )
+            dom0_pvh = false;
+        else if ( IS_ENABLED(CONFIG_HVM) && !strncmp("pvh", s, ss - s) )
+            dom0_pvh = true;
 #ifdef CONFIG_SHADOW_PAGING
         else if ( (val = parse_boolean("shadow", s, ss)) >= 0 )
             opt_dom0_shadow = val;
-- 
2.1.4


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

[PATCH v2 3/4] xen/dom0: Drop iommu_hwdom_inclusive entirely

[Andrew Cooper]

This option is unique to x86 PV dom0's, but it is not sensible to have a
catch-all which blindly maps all non-RAM regions into the IOMMU.

The map-reserved option remains, and covers all the buggy firmware issues that
I am aware of.  The two common cases are legacy USB keyboard emulation, and
the BMC mailbox used by vendor firmware in NICs/HBAs to report information
back to the iLO/iDRAC/etc for remote remote management purposes.

A specific advantage of removing this option is that x86 dom0's IOMMU setup is
now consistent between PV and PVH.

This removal is not expected to have any impact, due to map-reserved
remaining.  In the unlikely case that it does cause an issue, we should
introduce other map-$SPECIFIC options rather than re-introducing this
catch-all.

Signed-off-by: Andrew Cooper <andrew.cooper3@citrix.com>
---
CC: Jan Beulich <JBeulich@suse.com>
CC: Wei Liu <wei.liu2@citrix.com>
CC: Roger Pau Monné <roger.pau@citrix.com>
CC: Stefano Stabellini <sstabellini@kernel.org>
CC: Julien Grall <julien.grall@arm.com>
CC: Jun Nakajima <jun.nakajima@intel.com>
CC: Kevin Tian <kevin.tian@intel.com>

v2:
 * New
---
 docs/misc/xen-command-line.markdown   | 19 +++----------------
 xen/drivers/passthrough/arm/smmu.c    |  4 ----
 xen/drivers/passthrough/iommu.c       |  3 ---
 xen/drivers/passthrough/vtd/x86/vtd.c |  6 ------
 xen/drivers/passthrough/x86/iommu.c   | 14 ++------------
 xen/include/xen/iommu.h               |  2 +-
 6 files changed, 6 insertions(+), 42 deletions(-)

diff --git a/docs/misc/xen-command-line.markdown b/docs/misc/xen-command-line.markdown
index 0aeb786..3a9af17 100644
--- a/docs/misc/xen-command-line.markdown
+++ b/docs/misc/xen-command-line.markdown
@@ -664,8 +664,7 @@ Controls for how dom0 is constructed on x86 systems.
     hardware is not HAP-capable.
 
 ### dom0-iommu
-> `= List of [ passthrough=<bool>, strict=<bool>, map-inclusive=<bool>,
->              map-reserved=<bool> ]`
+> `= List of [ passthrough=<bool>, strict=<bool>, map-reserved=<bool> ]`
 
 Controls for the dom0 IOMMU setup.
 
@@ -692,9 +691,8 @@ Controls for the dom0 IOMMU setup.
     other domains in the system don't live in a compatible address space), and
     is ignored for ARM.
 
-*   The `map-inclusive` boolean is applicable to x86 PV dom0's, and sets up
-    identity IOMMU mappings for all non-RAM regions below 4GB except for
-    unusable ranges, and ranges belonging to Xen.
+*   The `map-reserved` boolean is applicable to x86, and sets up identity IOMMU
+    mappings for all E820 reserved regions below 4GB.
 
     Typically, some devices in a system use bits of RAM for communication, and
     these areas should be listed as reserved in the E820 table and identified
@@ -708,11 +706,6 @@ Controls for the dom0 IOMMU setup.
     This option is enabled by default on x86 systems, and invalid on ARM
     systems.
 
-*   The `map-reserved` functionality is very similar to `map-inclusive`, but is
-    applicable to both x86 PV and PVH dom0's, and represents a subset of the
-    correction by only mapping reserved memory regions rather than all non-RAM
-    regions.
-
 ### dom0\_ioports\_disable (x86)
 > `= List of <hex>-<hex>`
 
@@ -1253,12 +1246,6 @@ Specify the timeout of the device IOTLB invalidation in milliseconds.
 By default, the timeout is 1000 ms. When you see error 'Queue invalidate
 wait descriptor timed out', try increasing this value.
 
-### iommu_inclusive_mapping
-> `= <boolean>`
-
-**WARNING: This command line option is deprecated, and superseded by
-_dom0-iommu=map-inclusive_ - using both options in combination is undefined.**
-
 ### irq\_ratelimit (x86)
 > `= <integer>`
 
diff --git a/xen/drivers/passthrough/arm/smmu.c b/xen/drivers/passthrough/arm/smmu.c
index 9612c0f..16de7e1 100644
--- a/xen/drivers/passthrough/arm/smmu.c
+++ b/xen/drivers/passthrough/arm/smmu.c
@@ -2711,10 +2711,6 @@ 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. */
-	if ( iommu_hwdom_inclusive == 1 )
-		printk(XENLOG_WARNING
-		"map-inclusive dom0-iommu option is not supported on ARM\n");
-	iommu_hwdom_inclusive = 0;
 	if ( iommu_hwdom_reserved == 1 )
 		printk(XENLOG_WARNING
 		"map-reserved dom0-iommu option is not supported on ARM\n");
diff --git a/xen/drivers/passthrough/iommu.c b/xen/drivers/passthrough/iommu.c
index ac62d7f..d2ee2ee 100644
--- a/xen/drivers/passthrough/iommu.c
+++ b/xen/drivers/passthrough/iommu.c
@@ -61,7 +61,6 @@ 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;
 
 /*
@@ -154,8 +153,6 @@ 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 if ( (val = parse_boolean("map-reserved", s, ss)) >= 0 )
             iommu_hwdom_reserved = val;
         else
diff --git a/xen/drivers/passthrough/vtd/x86/vtd.c b/xen/drivers/passthrough/vtd/x86/vtd.c
index ff456e1..cf9d7e1 100644
--- a/xen/drivers/passthrough/vtd/x86/vtd.c
+++ b/xen/drivers/passthrough/vtd/x86/vtd.c
@@ -30,12 +30,6 @@
 #include "../vtd.h"
 #include "../extern.h"
 
-/*
- * iommu_inclusive_mapping: when set, all memory below 4GB is included in dom0
- * 1:1 iommu mappings except xen and unusable regions.
- */
-boolean_param("iommu_inclusive_mapping", iommu_hwdom_inclusive);
-
 void *map_vtd_domain_page(u64 maddr)
 {
     return map_domain_page(_mfn(paddr_to_pfn(maddr)));
diff --git a/xen/drivers/passthrough/x86/iommu.c b/xen/drivers/passthrough/x86/iommu.c
index c68a722..0ccb754 100644
--- a/xen/drivers/passthrough/x86/iommu.c
+++ b/xen/drivers/passthrough/x86/iommu.c
@@ -169,10 +169,10 @@ static bool __hwdom_init hwdom_iommu_map(const struct domain *d,
     default:
         if ( type & RAM_TYPE_RESERVED )
         {
-            if ( !iommu_hwdom_inclusive && !iommu_hwdom_reserved )
+            if ( !iommu_hwdom_reserved )
                 return false;
         }
-        else if ( is_hvm_domain(d) || !iommu_hwdom_inclusive || pfn > max_pfn )
+        else if ( is_hvm_domain(d) || pfn > max_pfn )
             return false;
     }
 
@@ -210,20 +210,10 @@ void __hwdom_init arch_iommu_hwdom_init(struct domain *d)
 
     BUG_ON(!is_hardware_domain(d));
 
-    /* Inclusive mappings are enabled by default for PV. */
-    if ( iommu_hwdom_inclusive == -1 )
-        iommu_hwdom_inclusive = is_pv_domain(d);
     /* Reserved IOMMU mappings are enabled by default. */
     if ( iommu_hwdom_reserved == -1 )
         iommu_hwdom_reserved = 1;
 
-    if ( iommu_hwdom_inclusive && !is_pv_domain(d) )
-    {
-        printk(XENLOG_WARNING
-               "IOMMU inclusive mappings are only supported on PV Dom0\n");
-        iommu_hwdom_inclusive = 0;
-    }
-
     if ( iommu_hwdom_passthrough )
         return;
 
diff --git a/xen/include/xen/iommu.h b/xen/include/xen/iommu.h
index 3d78126..49d0f0e 100644
--- a/xen/include/xen/iommu.h
+++ b/xen/include/xen/iommu.h
@@ -61,7 +61,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, iommu_hwdom_reserved;
+extern int8_t iommu_hwdom_reserved;
 
 extern unsigned int iommu_dev_iotlb_timeout;
 
-- 
2.1.4


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

[PATCH v2 4/4] xen/dom0: Add a dom0-iommu=none option

[Andrew Cooper]

For development purposes, it is very convenient to boot Xen as a PVH guest,
with an XTF PV or PVH "dom0".  The edit-compile-go cycle is a matter of
seconds, and you can reasonably insert printk() debugging in places which
which would be completely infeasible when booting fully-fledged guests.

However, the PVH dom0 path insists on having a working IOMMU, which doesn't
exist when virtualised as a PVH guest, and isn't necessary for XTF anyway.

Introduce a developer mode to skip the IOMMU requirement.

To fix a corner case with command line parsing, cmdline_strcmp() is
introduced.  Because we no longer tokenise comma separated list with NUL's,
strcmp(line, "opt") doesn't work for a string in the middle of the comma
separated list, and strncmp("opt", s, ss - s) matches "o", "op" and "opt" on
the command line.

Signed-off-by: Andrew Cooper <andrew.cooper3@citrix.com>
---
CC: Jan Beulich <JBeulich@suse.com>
CC: Wei Liu <wei.liu2@citrix.com>
CC: Roger Pau Monné <roger.pau@citrix.com>
CC: Stefano Stabellini <sstabellini@kernel.org>
CC: Julien Grall <julien.grall@arm.com>

Slightly RFC.  I've been carrying this patch locally for ages, but decided
that the approach is more likely to be accepted:

    diff --git a/xen/drivers/passthrough/x86/iommu.c b/xen/drivers/passthrough/x86/iommu.c
    index c68a722..87f0fd9 100644
    --- a/xen/drivers/passthrough/x86/iommu.c
    +++ b/xen/drivers/passthrough/x86/iommu.c
    @@ -117,8 +117,6 @@ int arch_iommu_populate_page_table(struct domain *d)

     void __hwdom_init arch_iommu_check_autotranslated_hwdom(struct domain *d)
     {
    -    if ( !iommu_enabled )
    -        panic("Presently, iommu must be enabled for PVH hardware domain\n");
     }

     int arch_iommu_domain_init(struct domain *d)

v2:
 * Retain `none` as opposed to repurposing `passthrough`.
 * Update cmdline_strcmp() to look only for commas.
---
 docs/misc/xen-command-line.markdown |  8 +++++++-
 xen/common/kernel.c                 | 20 ++++++++++++++++++++
 xen/drivers/passthrough/iommu.c     |  5 ++++-
 xen/include/xen/lib.h               |  7 +++++++
 4 files changed, 38 insertions(+), 2 deletions(-)

diff --git a/docs/misc/xen-command-line.markdown b/docs/misc/xen-command-line.markdown
index 3a9af17..915d25c 100644
--- a/docs/misc/xen-command-line.markdown
+++ b/docs/misc/xen-command-line.markdown
@@ -664,7 +664,7 @@ Controls for how dom0 is constructed on x86 systems.
     hardware is not HAP-capable.
 
 ### dom0-iommu
-> `= List of [ passthrough=<bool>, strict=<bool>, map-reserved=<bool> ]`
+> `= List of [ passthrough=<bool>, strict=<bool>, map-reserved=<bool>, none ]`
 
 Controls for the dom0 IOMMU setup.
 
@@ -706,6 +706,12 @@ Controls for the dom0 IOMMU setup.
     This option is enabled by default on x86 systems, and invalid on ARM
     systems.
 
+*   The `none` option is intended for development purposes only, and skips
+    certain safety checks pertaining to the correct IOMMU configuration for
+    dom0 to boot.
+
+    Incorrect use of this option may result in a malfunctioning system.
+
 ### dom0\_ioports\_disable (x86)
 > `= List of <hex>-<hex>`
 
diff --git a/xen/common/kernel.c b/xen/common/kernel.c
index 5766a0f..af96850 100644
--- a/xen/common/kernel.c
+++ b/xen/common/kernel.c
@@ -271,6 +271,26 @@ int parse_boolean(const char *name, const char *s, const char *e)
     return -1;
 }
 
+int cmdline_strcmp(const char *frag, const char *name)
+{
+    while ( 1 )
+    {
+        int res = (*frag - *name);
+
+        if ( res || *name == '\0' )
+        {
+            /* NUL in 'name' matching a comma in 'frag' implies success. */
+            if ( *name == '\0' && *frag == ',' )
+                res = 0;
+
+            return res;
+        }
+
+        frag++;
+        name++;
+    }
+}
+
 unsigned int tainted;
 
 /**
diff --git a/xen/drivers/passthrough/iommu.c b/xen/drivers/passthrough/iommu.c
index d2ee2ee..94b5f4c 100644
--- a/xen/drivers/passthrough/iommu.c
+++ b/xen/drivers/passthrough/iommu.c
@@ -59,6 +59,7 @@ bool_t __read_mostly iommu_snoop = 1;
 bool_t __read_mostly iommu_qinval = 1;
 bool_t __read_mostly iommu_intremap = 1;
 
+static bool __hwdom_initdata iommu_hwdom_none;
 bool __hwdom_initdata iommu_hwdom_strict;
 bool __read_mostly iommu_hwdom_passthrough;
 int8_t __hwdom_initdata iommu_hwdom_reserved = -1;
@@ -155,6 +156,8 @@ static int __init parse_dom0_iommu_param(const char *s)
             iommu_hwdom_strict = val;
         else if ( (val = parse_boolean("map-reserved", s, ss)) >= 0 )
             iommu_hwdom_reserved = val;
+        else if ( !cmdline_strcmp(s, "none") )
+            iommu_hwdom_none = true;
         else
             rc = -EINVAL;
 
@@ -183,7 +186,7 @@ int iommu_domain_init(struct domain *d)
 
 static void __hwdom_init check_hwdom_reqs(struct domain *d)
 {
-    if ( !paging_mode_translate(d) )
+    if ( iommu_hwdom_none || !paging_mode_translate(d) )
         return;
 
     arch_iommu_check_autotranslated_hwdom(d);
diff --git a/xen/include/xen/lib.h b/xen/include/xen/lib.h
index 972fc84..1540fe0 100644
--- a/xen/include/xen/lib.h
+++ b/xen/include/xen/lib.h
@@ -79,6 +79,13 @@ int parse_bool(const char *s, const char *e);
  */
 int parse_boolean(const char *name, const char *s, const char *e);
 
+/**
+ * Very similar to strcmp(), but will declare a match if the NUL in 'name'
+ * lines up with comma in 'frag'.  Designed for picking exact string matches
+ * out of a comma-separated command line fragment.
+ */
+int cmdline_strcmp(const char *frag, const char *name);
+
 /*#define DEBUG_TRACE_DUMP*/
 #ifdef DEBUG_TRACE_DUMP
 extern void debugtrace_dump(void);
-- 
2.1.4


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

Re: [PATCH v2 0/4] Functional and documentation improvements to dom0 setup

[Andrew Cooper]

On 31/12/2018 15:16, Andrew Cooper wrote:
> Expanded somewhat from v1.  See patches for details.
>
> Andrew Cooper (4):
>   xen/dom0: Improve documentation for dom0= and dom0-iommu=
>   x86/dom0: Improve dom0= useability
>   xen/dom0: Drop iommu_hwdom_inclusive entirely
>   xen/dom0: Add a dom0-iommu=none option

Please ignore v2.  The corner case mentioned in patch 4 is far worse
than I originally intended, and I've split that fix out for backport.

~Andrew

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

Re: [PATCH v2 1/4] xen/dom0: Improve documentation for dom0= and dom0-iommu=

[Roger Pau Monné]

On Mon, Dec 31, 2018 at 03:16:20PM +0000, Andrew Cooper wrote:
> +Controls for the dom0 IOMMU setup.
> +
> +*   The `passthrough` boolean controls whether IOMMU translation functionality
> +    is disabled for devices in dom0 (`passthrough=1`) or whether the IOMMU is
> +    used to ensure that dom0 can only DMA to its permitted areas of RAM
> +    (`passthrough=0`).
> +
> +    This option is only applicable to x86 PV dom0's, and defaults to false.
> +
> +    Some older Intel VT-d hardware isn't capable of disabling translation
> +    functionality on a per-device basis, and will cause this option to be
> +    ignored and assumed to be 0.  Similar behaviour on such systems is only
> +    available by fully disabling all IOMMUs.
> +
> +    This option is hardwired to false for x86 PVH dom0's (where a non-identity
> +    transform is required for dom0 to function), and is ignored for ARM.
> +
> +*   The `strict` boolean is applicable to x86 PV dom0's only and defaults to
> +    false.  It controls whether dom0 can have IOMMU mappings for all domain

I would remove 'domain' from the end of this line.

With or without that:

Reviewed-by: Roger Pau Monné <roger.pau@citrix.com>

Thanks, Roger.

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

Re: [PATCH v2 2/4] x86/dom0: Improve dom0= useability

[Roger Pau Monné]

On Mon, Dec 31, 2018 at 03:16:21PM +0000, Andrew Cooper wrote:
> Having a pvh boolean isn't ideal.  If we gain a 3rd virtulsation mode,
> what does `dom0=no-pvh` mean?
> 
> Change the syntax to be "dom0 = pv | pvh" which offers an option to more
> obviously select PV mode.  Hide both options behind the relevent
> CONFIG_* settings, and default to PVH mode when CONFIG_PV is compiled
> out.
> 
> Signed-off-by: Andrew Cooper <andrew.cooper3@citrix.com>

With the strncmp issue fixed:

Reviewed-by: Roger Pau Monné <roger.pau@citrix.com>

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

Re: [PATCH v2 3/4] xen/dom0: Drop iommu_hwdom_inclusive entirely

[Roger Pau Monné]

On Mon, Dec 31, 2018 at 03:16:22PM +0000, Andrew Cooper wrote:
> This option is unique to x86 PV dom0's, but it is not sensible to have a
> catch-all which blindly maps all non-RAM regions into the IOMMU.
> 
> The map-reserved option remains, and covers all the buggy firmware issues that
> I am aware of.  The two common cases are legacy USB keyboard emulation, and
> the BMC mailbox used by vendor firmware in NICs/HBAs to report information
> back to the iLO/iDRAC/etc for remote remote management purposes.
> 
> A specific advantage of removing this option is that x86 dom0's IOMMU setup is
> now consistent between PV and PVH.
> 
> This removal is not expected to have any impact, due to map-reserved
> remaining.  In the unlikely case that it does cause an issue, we should
> introduce other map-$SPECIFIC options rather than re-introducing this
> catch-all.
> 
> Signed-off-by: Andrew Cooper <andrew.cooper3@citrix.com>
> ---
> CC: Jan Beulich <JBeulich@suse.com>
> CC: Wei Liu <wei.liu2@citrix.com>
> CC: Roger Pau Monné <roger.pau@citrix.com>
> CC: Stefano Stabellini <sstabellini@kernel.org>
> CC: Julien Grall <julien.grall@arm.com>
> CC: Jun Nakajima <jun.nakajima@intel.com>
> CC: Kevin Tian <kevin.tian@intel.com>
> 
> v2:
>  * New
> ---
>  docs/misc/xen-command-line.markdown   | 19 +++----------------
>  xen/drivers/passthrough/arm/smmu.c    |  4 ----
>  xen/drivers/passthrough/iommu.c       |  3 ---
>  xen/drivers/passthrough/vtd/x86/vtd.c |  6 ------
>  xen/drivers/passthrough/x86/iommu.c   | 14 ++------------
>  xen/include/xen/iommu.h               |  2 +-
>  6 files changed, 6 insertions(+), 42 deletions(-)
> 
> diff --git a/docs/misc/xen-command-line.markdown b/docs/misc/xen-command-line.markdown
> index 0aeb786..3a9af17 100644
> --- a/docs/misc/xen-command-line.markdown
> +++ b/docs/misc/xen-command-line.markdown
> @@ -664,8 +664,7 @@ Controls for how dom0 is constructed on x86 systems.
>      hardware is not HAP-capable.
>  
>  ### dom0-iommu
> -> `= List of [ passthrough=<bool>, strict=<bool>, map-inclusive=<bool>,
> ->              map-reserved=<bool> ]`
> +> `= List of [ passthrough=<bool>, strict=<bool>, map-reserved=<bool> ]`
>  
>  Controls for the dom0 IOMMU setup.
>  
> @@ -692,9 +691,8 @@ Controls for the dom0 IOMMU setup.
>      other domains in the system don't live in a compatible address space), and
>      is ignored for ARM.
>  
> -*   The `map-inclusive` boolean is applicable to x86 PV dom0's, and sets up
> -    identity IOMMU mappings for all non-RAM regions below 4GB except for
> -    unusable ranges, and ranges belonging to Xen.
> +*   The `map-reserved` boolean is applicable to x86, and sets up identity IOMMU
> +    mappings for all E820 reserved regions below 4GB.

Current code will map all reserved regions below max_pdx, but maybe
the code should be changed to only map reserved regions < 4GB?

Thanks, Roger.

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

Re: [PATCH v2 4/4] xen/dom0: Add a dom0-iommu=none option

[Roger Pau Monné]

On Mon, Dec 31, 2018 at 03:16:23PM +0000, Andrew Cooper wrote:
> For development purposes, it is very convenient to boot Xen as a PVH guest,
> with an XTF PV or PVH "dom0".  The edit-compile-go cycle is a matter of
> seconds, and you can reasonably insert printk() debugging in places which
> which would be completely infeasible when booting fully-fledged guests.
> 
> However, the PVH dom0 path insists on having a working IOMMU, which doesn't
> exist when virtualised as a PVH guest, and isn't necessary for XTF anyway.
> 
> Introduce a developer mode to skip the IOMMU requirement.
> 
> To fix a corner case with command line parsing, cmdline_strcmp() is
> introduced.  Because we no longer tokenise comma separated list with NUL's,
> strcmp(line, "opt") doesn't work for a string in the middle of the comma
> separated list, and strncmp("opt", s, ss - s) matches "o", "op" and "opt" on
> the command line.
> 
> Signed-off-by: Andrew Cooper <andrew.cooper3@citrix.com>

Reviewed-by: Roger Pau Monné <roger.pau@citrix.com>

Even with the cmdline_strcmp moved to a separate patch.

Thanks, Roger.

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

Re: [PATCH v2 3/4] xen/dom0: Drop iommu_hwdom_inclusive entirely

[Andrew Cooper]

On 04/01/2019 12:33, Roger Pau Monné wrote:
> On Mon, Dec 31, 2018 at 03:16:22PM +0000, Andrew Cooper wrote:
>> This option is unique to x86 PV dom0's, but it is not sensible to have a
>> catch-all which blindly maps all non-RAM regions into the IOMMU.
>>
>> The map-reserved option remains, and covers all the buggy firmware issues that
>> I am aware of.  The two common cases are legacy USB keyboard emulation, and
>> the BMC mailbox used by vendor firmware in NICs/HBAs to report information
>> back to the iLO/iDRAC/etc for remote remote management purposes.
>>
>> A specific advantage of removing this option is that x86 dom0's IOMMU setup is
>> now consistent between PV and PVH.
>>
>> This removal is not expected to have any impact, due to map-reserved
>> remaining.  In the unlikely case that it does cause an issue, we should
>> introduce other map-$SPECIFIC options rather than re-introducing this
>> catch-all.
>>
>> Signed-off-by: Andrew Cooper <andrew.cooper3@citrix.com>
>> ---
>> CC: Jan Beulich <JBeulich@suse.com>
>> CC: Wei Liu <wei.liu2@citrix.com>
>> CC: Roger Pau Monné <roger.pau@citrix.com>
>> CC: Stefano Stabellini <sstabellini@kernel.org>
>> CC: Julien Grall <julien.grall@arm.com>
>> CC: Jun Nakajima <jun.nakajima@intel.com>
>> CC: Kevin Tian <kevin.tian@intel.com>
>>
>> v2:
>>  * New
>> ---
>>  docs/misc/xen-command-line.markdown   | 19 +++----------------
>>  xen/drivers/passthrough/arm/smmu.c    |  4 ----
>>  xen/drivers/passthrough/iommu.c       |  3 ---
>>  xen/drivers/passthrough/vtd/x86/vtd.c |  6 ------
>>  xen/drivers/passthrough/x86/iommu.c   | 14 ++------------
>>  xen/include/xen/iommu.h               |  2 +-
>>  6 files changed, 6 insertions(+), 42 deletions(-)
>>
>> diff --git a/docs/misc/xen-command-line.markdown b/docs/misc/xen-command-line.markdown
>> index 0aeb786..3a9af17 100644
>> --- a/docs/misc/xen-command-line.markdown
>> +++ b/docs/misc/xen-command-line.markdown
>> @@ -664,8 +664,7 @@ Controls for how dom0 is constructed on x86 systems.
>>      hardware is not HAP-capable.
>>  
>>  ### dom0-iommu
>> -> `= List of [ passthrough=<bool>, strict=<bool>, map-inclusive=<bool>,
>> ->              map-reserved=<bool> ]`
>> +> `= List of [ passthrough=<bool>, strict=<bool>, map-reserved=<bool> ]`
>>  
>>  Controls for the dom0 IOMMU setup.
>>  
>> @@ -692,9 +691,8 @@ Controls for the dom0 IOMMU setup.
>>      other domains in the system don't live in a compatible address space), and
>>      is ignored for ARM.
>>  
>> -*   The `map-inclusive` boolean is applicable to x86 PV dom0's, and sets up
>> -    identity IOMMU mappings for all non-RAM regions below 4GB except for
>> -    unusable ranges, and ranges belonging to Xen.
>> +*   The `map-reserved` boolean is applicable to x86, and sets up identity IOMMU
>> +    mappings for all E820 reserved regions below 4GB.
> Current code will map all reserved regions below max_pdx, but maybe
> the code should be changed to only map reserved regions < 4GB?

Oh - no the code is correct.

If we want to map all E820 reserved regions, then we really want all of
them, not just those below 4G.  I'll fix the wording next time around.

~Andrew

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

Re: [PATCH v2 3/4] xen/dom0: Drop iommu_hwdom_inclusive entirely

[Roger Pau Monné]

On Fri, Jan 04, 2019 at 12:46:27PM +0000, Andrew Cooper wrote:
> On 04/01/2019 12:33, Roger Pau Monné wrote:
> > On Mon, Dec 31, 2018 at 03:16:22PM +0000, Andrew Cooper wrote:
> >> This option is unique to x86 PV dom0's, but it is not sensible to have a
> >> catch-all which blindly maps all non-RAM regions into the IOMMU.
> >>
> >> The map-reserved option remains, and covers all the buggy firmware issues that
> >> I am aware of.  The two common cases are legacy USB keyboard emulation, and
> >> the BMC mailbox used by vendor firmware in NICs/HBAs to report information
> >> back to the iLO/iDRAC/etc for remote remote management purposes.
> >>
> >> A specific advantage of removing this option is that x86 dom0's IOMMU setup is
> >> now consistent between PV and PVH.
> >>
> >> This removal is not expected to have any impact, due to map-reserved
> >> remaining.  In the unlikely case that it does cause an issue, we should
> >> introduce other map-$SPECIFIC options rather than re-introducing this
> >> catch-all.
> >>
> >> Signed-off-by: Andrew Cooper <andrew.cooper3@citrix.com>
> >> ---
> >> CC: Jan Beulich <JBeulich@suse.com>
> >> CC: Wei Liu <wei.liu2@citrix.com>
> >> CC: Roger Pau Monné <roger.pau@citrix.com>
> >> CC: Stefano Stabellini <sstabellini@kernel.org>
> >> CC: Julien Grall <julien.grall@arm.com>
> >> CC: Jun Nakajima <jun.nakajima@intel.com>
> >> CC: Kevin Tian <kevin.tian@intel.com>
> >>
> >> v2:
> >>  * New
> >> ---
> >>  docs/misc/xen-command-line.markdown   | 19 +++----------------
> >>  xen/drivers/passthrough/arm/smmu.c    |  4 ----
> >>  xen/drivers/passthrough/iommu.c       |  3 ---
> >>  xen/drivers/passthrough/vtd/x86/vtd.c |  6 ------
> >>  xen/drivers/passthrough/x86/iommu.c   | 14 ++------------
> >>  xen/include/xen/iommu.h               |  2 +-
> >>  6 files changed, 6 insertions(+), 42 deletions(-)
> >>
> >> diff --git a/docs/misc/xen-command-line.markdown b/docs/misc/xen-command-line.markdown
> >> index 0aeb786..3a9af17 100644
> >> --- a/docs/misc/xen-command-line.markdown
> >> +++ b/docs/misc/xen-command-line.markdown
> >> @@ -664,8 +664,7 @@ Controls for how dom0 is constructed on x86 systems.
> >>      hardware is not HAP-capable.
> >>  
> >>  ### dom0-iommu
> >> -> `= List of [ passthrough=<bool>, strict=<bool>, map-inclusive=<bool>,
> >> ->              map-reserved=<bool> ]`
> >> +> `= List of [ passthrough=<bool>, strict=<bool>, map-reserved=<bool> ]`
> >>  
> >>  Controls for the dom0 IOMMU setup.
> >>  
> >> @@ -692,9 +691,8 @@ Controls for the dom0 IOMMU setup.
> >>      other domains in the system don't live in a compatible address space), and
> >>      is ignored for ARM.
> >>  
> >> -*   The `map-inclusive` boolean is applicable to x86 PV dom0's, and sets up
> >> -    identity IOMMU mappings for all non-RAM regions below 4GB except for
> >> -    unusable ranges, and ranges belonging to Xen.
> >> +*   The `map-reserved` boolean is applicable to x86, and sets up identity IOMMU
> >> +    mappings for all E820 reserved regions below 4GB.
> > Current code will map all reserved regions below max_pdx, but maybe
> > the code should be changed to only map reserved regions < 4GB?
> 
> Oh - no the code is correct.
> 
> If we want to map all E820 reserved regions, then we really want all of
> them, not just those below 4G.  I'll fix the wording next time around.

Sadly the code is not fully correct, since any reserved region past
the last RAM region will be ignored, since max_pdx only accounts for
RAM regions. Will send a patch to fix it.

Roger.

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

Re: [PATCH v2 3/4] xen/dom0: Drop iommu_hwdom_inclusive entirely

[Roger Pau Monné]

On Mon, Dec 31, 2018 at 03:16:22PM +0000, Andrew Cooper wrote:
> diff --git a/xen/drivers/passthrough/x86/iommu.c b/xen/drivers/passthrough/x86/iommu.c
> index c68a722..0ccb754 100644
> --- a/xen/drivers/passthrough/x86/iommu.c
> +++ b/xen/drivers/passthrough/x86/iommu.c
> @@ -169,10 +169,10 @@ static bool __hwdom_init hwdom_iommu_map(const struct domain *d,
>      default:
>          if ( type & RAM_TYPE_RESERVED )
>          {
> -            if ( !iommu_hwdom_inclusive && !iommu_hwdom_reserved )
> +            if ( !iommu_hwdom_reserved )
>                  return false;
>          }
> -        else if ( is_hvm_domain(d) || !iommu_hwdom_inclusive || pfn > max_pfn )
> +        else if ( is_hvm_domain(d) || pfn > max_pfn )
>              return false;

I think the switch can be simplified as:

switch ( type = page_get_ram_type(mfn) )
{
case RAM_TYPE_CONVENTIONAL:
    if ( iommu_hwdom_strict )
        return false;
    break;

default:
    if ( !(type & RAM_TYPE_RESERVED) )
        return false;
    /* fallthrough. */
case RAM_TYPE_RESERVED:
    if ( !iommu_hwdom_reserved )
       return false;
    break;
}

And the max_pfn parameter is no longer needed.

Roger.

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