[PATCH v4 for-4.12 0/3] Docs improvements, and dom0 construction fixes

[PATCH v4 for-4.12 0/3] Docs improvements, and dom0 construction fixes

[Andrew Cooper]

See individual patches for the delta from v3.

Andrew Cooper (3):
  docs: Improve documentation and parsing for efi=
  xen/dom0: Deprecate iommu_hwdom_inclusive and leave it disabled by default
  xen/dom0: Add a dom0-iommu=none option

 docs/misc/xen-command-line.pandoc   | 41 ++++++++++++++++++++-----------------
 xen/common/efi/boot.c               | 20 ++++++++++--------
 xen/drivers/passthrough/iommu.c     |  7 +++++--
 xen/drivers/passthrough/x86/iommu.c |  3 ---
 4 files changed, 38 insertions(+), 33 deletions(-)

-- 
2.1.4


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

[PATCH v4 1/3] docs: Improve documentation and parsing for efi=

[Andrew Cooper]

Update parse_efi_param() to use parse_boolean() for "rs", so it behaves
like other Xen booleans.

However, change "attr=uc" to not be a boolean.  "no-attr=uc" is ambiguous and
shouldn't be accepted, but accept "attr=no" as an acceptable alternative.

Update the command line documentation for consistency.

Signed-off-by: Andrew Cooper <andrew.cooper3@citrix.com>
Release-acked-by: Juergen Gross <jgross@suse.com>
---
CC: Jan Beulich <JBeulich@suse.com>

v3:
 * New
v4:
 * Support "attr=no"
---
 docs/misc/xen-command-line.pandoc | 23 +++++++++--------------
 xen/common/efi/boot.c             | 20 +++++++++++---------
 2 files changed, 20 insertions(+), 23 deletions(-)

diff --git a/docs/misc/xen-command-line.pandoc b/docs/misc/xen-command-line.pandoc
index 21d7b4a..8b1703d 100644
--- a/docs/misc/xen-command-line.pandoc
+++ b/docs/misc/xen-command-line.pandoc
@@ -853,23 +853,18 @@ disable it (edid=no). This option should not normally be required
 except for debugging purposes.
 
 ### efi
-> `= List of [ rs | attr ]`
+    = List of [ rs=<bool>, attr=no|uc ]
 
-All options are of boolean kind and can be prefixed with `no-` to
-effect the inverse meaning.
+Controls for interacting with the system Extended Firmware Interface.
 
-> `rs`
+*   The `rs` boolean controls whether Runtime Services are used.  By default,
+    Xen uses Runtime Services itself, and proxies certain calls on behalf of
+    dom0.  Selecting `rs=0` prohibits all use of Runtime Services.
 
-> Default: `true`
-
->> Force or disable use of EFI runtime services.
-
-> `attr=uc`
-
-> Default: `off`
-
->> Allows mapping of RuntimeServices which have no cachability attribute
->> set as UC.
+*   The `attr=` string exists to specify what to do with memory regions of
+    unknown/unrecognised cacheability.  `attr=no` is the default and will
+    leave the memory regions unmapped, while `attr=uc` will map them as fully
+    uncacheable.
 
 ### ept
 > `= List of [ ad=<bool>, pml=<bool> ]`
diff --git a/xen/common/efi/boot.c b/xen/common/efi/boot.c
index 1e1a551..7919378 100644
--- a/xen/common/efi/boot.c
+++ b/xen/common/efi/boot.c
@@ -1389,27 +1389,29 @@ static bool __initdata efi_map_uc;
 static int __init parse_efi_param(const char *s)
 {
     const char *ss;
-    int rc = 0;
+    int rc = 0, val;
 
     do {
-        bool val = strncmp(s, "no-", 3);
-
-        if ( !val )
-            s += 3;
-
         ss = strchr(s, ',');
         if ( !ss )
             ss = strchr(s, '\0');
 
-        if ( !cmdline_strcmp(s, "rs") )
+        if ( (val = parse_boolean("rs", s, ss)) >= 0 )
         {
             if ( val )
                 __set_bit(EFI_RS, &efi_flags);
             else
                 __clear_bit(EFI_RS, &efi_flags);
         }
-        else if ( !cmdline_strcmp(s, "attr=uc") )
-            efi_map_uc = val;
+        else if ( (ss - s) > 5 && !memcmp(s, "attr=", 5) )
+        {
+            if ( cmdline_strcmp(s + 5, "uc") )
+                efi_map_uc = true;
+            else if ( cmdline_strcmp(s + 5, "no") )
+                efi_map_uc = false;
+            else
+                rc = -EINVAL;
+        }
         else
             rc = -EINVAL;
 
-- 
2.1.4


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

[PATCH v4 2/3] xen/dom0: Deprecate iommu_hwdom_inclusive and leave it disabled by default

[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 this change is that x86 dom0's IOMMU setup is now
consistent between PV and PVH.

This change 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>
Release-acked-by: Juergen Gross <jgross@suse.com>
---
CC: Jan Beulich <JBeulich@suse.com>
CC: Wei Liu <wei.liu2@citrix.com>
CC: Roger Pau Monné <roger.pau@citrix.com>

v2:
 * New
v4:
 * Switch to only disabled-by-default and deprecated, rather than revmoving
   the option fully.
---
 docs/misc/xen-command-line.pandoc   | 10 ++++++----
 xen/drivers/passthrough/iommu.c     |  2 +-
 xen/drivers/passthrough/x86/iommu.c |  3 ---
 3 files changed, 7 insertions(+), 8 deletions(-)

diff --git a/docs/misc/xen-command-line.pandoc b/docs/misc/xen-command-line.pandoc
index 8b1703d..139c4e1 100644
--- a/docs/misc/xen-command-line.pandoc
+++ b/docs/misc/xen-command-line.pandoc
@@ -707,14 +707,16 @@ Controls for the dom0 IOMMU setup.
     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.
+    This option is disabled by default, and deprecated and intended for
+    removal in future versions of Xen.  If specifying `map-inclusive` is the
+    only way to make your system boot, please report a bug.
 
 *   The `map-reserved` functionality is very similar to `map-inclusive`.
 
     The differences from `map-inclusive` are that `map-reserved` 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.
+    to both x86 PV and PVH dom0's, is enabled by default, 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>`
diff --git a/xen/drivers/passthrough/iommu.c b/xen/drivers/passthrough/iommu.c
index 9ac9e05..b9ffe18 100644
--- a/xen/drivers/passthrough/iommu.c
+++ b/xen/drivers/passthrough/iommu.c
@@ -38,7 +38,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_inclusive;
 int8_t __hwdom_initdata iommu_hwdom_reserved = -1;
 
 /*
diff --git a/xen/drivers/passthrough/x86/iommu.c b/xen/drivers/passthrough/x86/iommu.c
index e40d7a7..2c051a0 100644
--- a/xen/drivers/passthrough/x86/iommu.c
+++ b/xen/drivers/passthrough/x86/iommu.c
@@ -214,9 +214,6 @@ 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;
-- 
2.1.4


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

[PATCH v4 3/3] 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.

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

v2:
 * Retain `none` as opposed to repurposing `passthrough`.  It turns out
   that they are different.
 * Update cmdline_strcmp() to look only for commas.
v3:
 * Rebase over splitting cmdline_strcmp() out into a standalone fix.
v4:
 * Rebase over retaining iommu_hwdom_inclusive.
---
 docs/misc/xen-command-line.pandoc | 8 +++++++-
 xen/drivers/passthrough/iommu.c   | 5 ++++-
 2 files changed, 11 insertions(+), 2 deletions(-)

diff --git a/docs/misc/xen-command-line.pandoc b/docs/misc/xen-command-line.pandoc
index 139c4e1..6a33775 100644
--- a/docs/misc/xen-command-line.pandoc
+++ b/docs/misc/xen-command-line.pandoc
@@ -667,7 +667,7 @@ Controls for how dom0 is constructed on x86 systems.
 
 ### dom0-iommu
     = List of [ passthrough=<bool>, strict=<bool>, map-inclusive=<bool>,
-                map-reserved=<bool> ]
+                map-reserved=<bool>, none ]
 
 Controls for the dom0 IOMMU setup.
 
@@ -718,6 +718,12 @@ Controls for the dom0 IOMMU setup.
     subset of the correction by only mapping reserved memory regions rather
     than all non-RAM regions.
 
+*   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/drivers/passthrough/iommu.c b/xen/drivers/passthrough/iommu.c
index b9ffe18..56150bc 100644
--- a/xen/drivers/passthrough/iommu.c
+++ b/xen/drivers/passthrough/iommu.c
@@ -36,6 +36,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_inclusive;
@@ -131,6 +132,8 @@ static int __init parse_dom0_iommu_param(const char *s)
             iommu_hwdom_inclusive = 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;
 
@@ -159,7 +162,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);
-- 
2.1.4


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

Re: [PATCH v4 2/3] xen/dom0: Deprecate iommu_hwdom_inclusive and leave it disabled by default

[Roger Pau Monné]

On Mon, Jan 21, 2019 at 07:02:25PM +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 this change is that x86 dom0's IOMMU setup is now
> consistent between PV and PVH.
> 
> This change 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>
> Release-acked-by: Juergen Gross <jgross@suse.com>

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

Will it be worth printing a warning message in arch_iommu_hwdom_init
about map-inclusive deprecation if it's enabled?

Thanks, Roger.

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

Re: [PATCH v4 1/3] docs: Improve documentation and parsing for efi=

[Jan Beulich]

>>> On 21.01.19 at 20:02, <andrew.cooper3@citrix.com> wrote:
> Update parse_efi_param() to use parse_boolean() for "rs", so it behaves
> like other Xen booleans.
> 
> However, change "attr=uc" to not be a boolean.  "no-attr=uc" is ambiguous and
> shouldn't be accepted, but accept "attr=no" as an acceptable alternative.
> 
> Update the command line documentation for consistency.
> 
> Signed-off-by: Andrew Cooper <andrew.cooper3@citrix.com>
> Release-acked-by: Juergen Gross <jgross@suse.com>

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



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

Re: [PATCH v4 2/3] xen/dom0: Deprecate iommu_hwdom_inclusive and leave it disabled by default

[Jan Beulich]

>>> On 21.01.19 at 20:02, <andrew.cooper3@citrix.com> wrote:
> --- a/xen/drivers/passthrough/iommu.c
> +++ b/xen/drivers/passthrough/iommu.c
> @@ -38,7 +38,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_inclusive;

I guess this could become bool now, but either way
Reviewed-by: Jan Beulich <jbeulich@suse.com>

Jan



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