[PATCH 0/2] Ease development with a PVH Xen and XTF PVH dom0

[PATCH 0/2] Ease development with a PVH Xen and XTF PVH dom0

[Andrew Cooper]

Andrew Cooper (2):
  xen/dom0: Improve documentation for dom0= and dom0-iommu=
  xen/dom0: Add a dom0-iommu=none option

 docs/misc/xen-command-line.markdown | 109 ++++++++++++++++++++++--------------
 xen/arch/x86/dom0_build.c           |   6 --
 xen/common/kernel.c                 |  21 +++++++
 xen/drivers/passthrough/iommu.c     |   6 +-
 xen/include/xen/lib.h               |   7 +++
 5 files changed, 101 insertions(+), 48 deletions(-)

-- 
2.1.4


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

[PATCH 1/2] 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.

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.

It occurs to me that:

 * The choice of dom0 boot mode should in part be derived from the available
   CONFIG_* options, and ELF notes advertised in the dom0 kernel.

 * AMD probably needs to gain an `ivmd=` to mirror `rmrr=` on the Intel side,
   because we know there are other errors in the IVRS table.

 * Neither of map-{inclusive,reserved} should be active by default, even on
   Intel hardware, and we should (wherever possible) have quirks like we have
   for all other firmware screwups.  Requiring the user to diagnose/work
   around firmware problems like this is quite rude.

But this is all future work to do.
---
 docs/misc/xen-command-line.markdown | 103 ++++++++++++++++++++++--------------
 xen/arch/x86/dom0_build.c           |   6 ---
 2 files changed, 62 insertions(+), 47 deletions(-)

diff --git a/docs/misc/xen-command-line.markdown b/docs/misc/xen-command-line.markdown
index 44ee51a..94ee703 100644
--- a/docs/misc/xen-command-line.markdown
+++ b/docs/misc/xen-command-line.markdown
@@ -636,55 +636,76 @@ 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:
-
-> `pvh`
+> Applicability: x86
 
-> Default: `false`
+Controls for how dom0 is constructed on x86 systems.
 
-Flag that makes a dom0 boot in PVHv2 mode.
+*   The `pvh` boolean controls whether dom0 is constructed as a PV or a PVH
+    guest.  For backwards compatibility, the default is PV.  In addition, the
+    following requirements must be met.
 
-> `shadow`
+    *    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.
 
-> Default: `false`
+*   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.
 
-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 is applicable to x86 PV dom0's only and defaults
+    to false.  It controls whether the IOMMU is fully disabled for devices
+    belonging to dom0 (`passthrough=1`), or whether the IOMMU is set up with
+    an identity transform for dom0 (`passthrough=0`) to prevent dom0 from
+    DMA'ing outside of its permitted areas.
+
+    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 DMA
+    remapping for all non-RAM regions below 4GB except for unusable ranges.
+
+    Typically, some devices in a system use bits of RAM for communication, and
+    these areas should be listed 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 writing its APCI tables, 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 Intel systems, disabled by
+    default on other 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.
+
+    This option is enabled by default on x86 Intel systems, disabled by
+    default on other x86 systems, and invalid on ARM systems.
 
 ### dom0\_ioports\_disable (x86)
 > `= List of <hex>-<hex>`
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 2/2] 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 resonably 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) eagerly matches all options
which begin with "opt".

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)
---
 docs/misc/xen-command-line.markdown |  8 +++++++-
 xen/common/kernel.c                 | 21 +++++++++++++++++++++
 xen/drivers/passthrough/iommu.c     |  6 +++++-
 xen/include/xen/lib.h               |  7 +++++++
 4 files changed, 40 insertions(+), 2 deletions(-)

diff --git a/docs/misc/xen-command-line.markdown b/docs/misc/xen-command-line.markdown
index 94ee703..eb0a65e 100644
--- a/docs/misc/xen-command-line.markdown
+++ b/docs/misc/xen-command-line.markdown
@@ -663,7 +663,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.
 
@@ -707,6 +707,12 @@ Controls for the dom0 IOMMU setup.
     This option is enabled by default on x86 Intel systems, disabled by
     default on other 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..fa2d9f3 100644
--- a/xen/common/kernel.c
+++ b/xen/common/kernel.c
@@ -4,6 +4,7 @@
  * Copyright (c) 2002-2005 K A Fraser
  */
 
+#include <xen/ctype.h>
 #include <xen/init.h>
 #include <xen/lib.h>
 #include <xen/errno.h>
@@ -271,6 +272,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 punctuation in 'frag' implies success. */
+            if ( *name == '\0' && ispunct(*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 ac62d7f..67efb10 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;
 
+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 = -1;
@@ -158,6 +159,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;
 
@@ -189,7 +192,8 @@ static void __hwdom_init check_hwdom_reqs(struct domain *d)
     if ( !paging_mode_translate(d) )
         return;
 
-    arch_iommu_check_autotranslated_hwdom(d);
+    if ( !iommu_hwdom_none )
+        arch_iommu_check_autotranslated_hwdom(d);
 
     iommu_hwdom_passthrough = false;
     iommu_hwdom_strict = true;
diff --git a/xen/include/xen/lib.h b/xen/include/xen/lib.h
index 972fc84..58a7ea9 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 punctuationin '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 1/2] xen/dom0: Improve documentation for dom0= and dom0-iommu=

[Jan Beulich]

>>> On 21.12.18 at 00:40, <andrew.cooper3@citrix.com> wrote:
> --- a/docs/misc/xen-command-line.markdown
> +++ b/docs/misc/xen-command-line.markdown
> @@ -636,55 +636,76 @@ 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:
> -
> -> `pvh`
> +> Applicability: x86

Why the new tag, when everything else uses (x86) next to the
option name?

>  ### 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 is applicable to x86 PV dom0's only and defaults
> +    to false.  It controls whether the IOMMU is fully disabled for devices
> +    belonging to dom0 (`passthrough=1`), or whether the IOMMU is set up with
> +    an identity transform for dom0 (`passthrough=0`) to prevent dom0 from
> +    DMA'ing outside of its permitted areas.
> +
> +    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 DMA
> +    remapping for all non-RAM regions below 4GB except for unusable ranges.

I don't thinks this is PV-specific, just its default is.

Jan



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

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

[Roger Pau Monné]

On Thu, Dec 20, 2018 at 11:40:51PM +0000, Andrew Cooper wrote:
> 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.
> 
> Signed-off-by: Andrew Cooper <andrew.cooper3@citrix.com>

Thanks! A couple of fixes below, because the original text is actually
wrong...

> ---
> 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.
> 
> It occurs to me that:
> 
>  * The choice of dom0 boot mode should in part be derived from the available
>    CONFIG_* options, and ELF notes advertised in the dom0 kernel.

This is indeed doable, but would require parsing the dom0 kernel
before building the domain.

> 
>  * AMD probably needs to gain an `ivmd=` to mirror `rmrr=` on the Intel side,
>    because we know there are other errors in the IVRS table.

Yes, albeit using rmrr is quite cumbersome because it's mostly a
trial-and-error process until there are no more iommu faults (unless
you can get the correct rmrr command for your hardware somewhere).

> 
>  * Neither of map-{inclusive,reserved} should be active by default, even on
>    Intel hardware, and we should (wherever possible) have quirks like we have
>    for all other firmware screwups.  Requiring the user to diagnose/work
>    around firmware problems like this is quite rude.

That would indeed be nice, but I think there are too many vendor
firmware versions to be able to correctly identify such quirks, the
more that vendors don't even list missing RMRR as erratum.

> +Controls for the dom0 IOMMU setup.
> +
> +*   The `passthrough` boolean is applicable to x86 PV dom0's only and defaults
> +    to false.  It controls whether the IOMMU is fully disabled for devices
> +    belonging to dom0 (`passthrough=1`), or whether the IOMMU is set up with
> +    an identity transform for dom0 (`passthrough=0`) to prevent dom0 from
> +    DMA'ing outside of its permitted areas.
> +
> +    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 DMA
> +    remapping for all non-RAM regions below 4GB except for unusable ranges.
> +
> +    Typically, some devices in a system use bits of RAM for communication, and
> +    these areas should be listed 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 writing its APCI tables, 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 Intel systems, disabled by
> +    default on other x86 systems, and invalid on ARM systems.

I'm afraid the previous text was wrong. I later discovered that AMD
also had such workarounds applied by default, and unified the code,
but failed to update the documentation, sorry.

map-inclusive is enabled by default on x86 for a PV dom0. See
xen/drivers/passthrough/x86/iommu.c:215 (arch_iommu_hwdom_init).

> +
> +*   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.
> +
> +    This option is enabled by default on x86 Intel systems, disabled by
> +    default on other x86 systems, and invalid on ARM systems.

map-reserved is enabled by default on x86,
xen/drivers/passthrough/x86/iommu.c:218 (arch_iommu_hwdom_init).

The text itself looks OK to me.

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

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

[Jan Beulich]

>>> On 21.12.18 at 00:40, <andrew.cooper3@citrix.com> wrote:
> @@ -271,6 +272,26 @@ int parse_boolean(const char *name, const char *s, const char *e)
>      return -1;
>  }
>  
> +int cmdline_strcmp(const char *frag, const char *name)

__init ?

> +{
> +    while ( 1 )
> +    {
> +        int res = (*frag - *name);
> +
> +        if ( res || *name == '\0' )
> +        {
> +            /* NUL in 'name' matching punctuation in 'frag' implies success. */
> +            if ( *name == '\0' && ispunct(*frag) )
> +                res = 0;

Isn't ispunct() true for dashes and perhaps also underscores?
I don't think it can be this generic, the more that ...

> --- 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 punctuationin 'frag'.  Designed for picking exact string
> + * matches out of a comma-separated command line fragment.
> + */
> +int cmdline_strcmp(const char *frag, const char *name);

... you talk of commas only here.

Jan



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

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

[Andrew Cooper]

[-- Attachment #1.1: Type: text/plain, Size: 4570 bytes --]

On 21/12/2018 12:08, Jan Beulich wrote:
>>>> On 21.12.18 at 00:40, <andrew.cooper3@citrix.com> wrote:
>> --- a/docs/misc/xen-command-line.markdown
>> +++ b/docs/misc/xen-command-line.markdown
>> @@ -636,55 +636,76 @@ 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:
>> -
>> -> `pvh`
>> +> Applicability: x86
> Why the new tag, when everything else uses (x86) next to the
> option name?

See the commit message of c/s a3a99df44e5405d2092ec59087681765fa4cdee7

The problem is with the generated HTML anchors when trying to cross
reference the options.

>
>>  ### 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 is applicable to x86 PV dom0's only and defaults
>> +    to false.  It controls whether the IOMMU is fully disabled for devices
>> +    belonging to dom0 (`passthrough=1`), or whether the IOMMU is set up with
>> +    an identity transform for dom0 (`passthrough=0`) to prevent dom0 from
>> +    DMA'ing outside of its permitted areas.
>> +
>> +    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 DMA
>> +    remapping for all non-RAM regions below 4GB except for unusable ranges.
> I don't thinks this is PV-specific, just its default is.

>From arch_iommu_hwdom_init():

    /* 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;
    }


Attempting to use this option with a PVH dom0 will cause Xen to force it
off.

~Andrew

[-- Attachment #1.2: Type: text/html, Size: 5570 bytes --]

[-- Attachment #2: Type: text/plain, Size: 157 bytes --]

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

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

[Roger Pau Monné]

On Thu, Dec 20, 2018 at 11:40:52PM +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 resonably 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.

This looks very similar to the current 'passthrough' option, maybe it
would be enough to allow PVH dom0 to use the passthrough option
provided a warning is added to
arch_iommu_check_autotranslated_hwdom?

Thanks, Roger.

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

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

[Andrew Cooper]

On 21/12/2018 12:13, Jan Beulich wrote:
>>>> On 21.12.18 at 00:40, <andrew.cooper3@citrix.com> wrote:
>> @@ -271,6 +272,26 @@ int parse_boolean(const char *name, const char *s, const char *e)
>>      return -1;
>>  }
>>  
>> +int cmdline_strcmp(const char *frag, const char *name)
> __init ?

I think there are some runtime parameters in need of some fixing as well.

>
>> +{
>> +    while ( 1 )
>> +    {
>> +        int res = (*frag - *name);
>> +
>> +        if ( res || *name == '\0' )
>> +        {
>> +            /* NUL in 'name' matching punctuation in 'frag' implies success. */
>> +            if ( *name == '\0' && ispunct(*frag) )
>> +                res = 0;
> Isn't ispunct() true for dashes and perhaps also underscores?
> I don't think it can be this generic, the more that ...
>
>> --- 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 punctuationin 'frag'.  Designed for picking exact string
>> + * matches out of a comma-separated command line fragment.
>> + */
>> +int cmdline_strcmp(const char *frag, const char *name);
> ... you talk of commas only here.

I actually borrowed this function from my CPUID cmdline patch.  In 99%
of cases, we only need to match = and , but we have some other
parameters such as psr= which use : for delimiters, hence the use of
ispunct().

As an alternative, I could revert back to explicitly checking the
expected punctuation.  It is not as if this is a fastpath.

~Andrew

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

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

[Andrew Cooper]

On 21/12/2018 12:44, Roger Pau Monné wrote:
> On Thu, Dec 20, 2018 at 11:40:52PM +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 resonably 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.
> This looks very similar to the current 'passthrough' option, maybe it
> would be enough to allow PVH dom0 to use the passthrough option
> provided a warning is added to
> arch_iommu_check_autotranslated_hwdom?

I considered that, but "dom0-iommu=passthrough" isn't an accurate
description of what is going on.  Frankly, its not correct for PV either.

TBH, dom0-iommu=none is better for both.  How about I introduce that as
the new option, and leave passthrough as a legacy alias?

~Andrew

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

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

[Andrew Cooper]

On 21/12/2018 12:08, Roger Pau Monné wrote:
> On Thu, Dec 20, 2018 at 11:40:51PM +0000, Andrew Cooper wrote:
>> 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.
>>
>> Signed-off-by: Andrew Cooper <andrew.cooper3@citrix.com>
> Thanks! A couple of fixes below, because the original text is actually
> wrong...

TBH, that is my default assumption every time I do work like this :)

>
>> ---
>> 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.
>>
>> It occurs to me that:
>>
>>  * The choice of dom0 boot mode should in part be derived from the available
>>    CONFIG_* options, and ELF notes advertised in the dom0 kernel.
> This is indeed doable, but would require parsing the dom0 kernel
> before building the domain.

I don't see anything wrong with parsing the ELF headers ahead of
building the domain.  From the overall boot time, its just an
order-of-operations issue.

>
>>  * AMD probably needs to gain an `ivmd=` to mirror `rmrr=` on the Intel side,
>>    because we know there are other errors in the IVRS table.
> Yes, albeit using rmrr is quite cumbersome because it's mostly a
> trial-and-error process until there are no more iommu faults (unless
> you can get the correct rmrr command for your hardware somewhere).
>
>>  * Neither of map-{inclusive,reserved} should be active by default, even on
>>    Intel hardware, and we should (wherever possible) have quirks like we have
>>    for all other firmware screwups.  Requiring the user to diagnose/work
>>    around firmware problems like this is quite rude.
> That would indeed be nice, but I think there are too many vendor
> firmware versions to be able to correctly identify such quirks, the
> more that vendors don't even list missing RMRR as erratum.

I don't agree.  We already have quirks based on DMI (at the moment,
mainly for reboot overrides), and the vast majority of the offending
cases are the BMC shared mailbox, which will be in a fixed per-platform
location.

I don't expect we'll ever find and fix all quirks, but where we do find
suitable ones, we should put them into the boot code.

>
>> +Controls for the dom0 IOMMU setup.
>> +
>> +*   The `passthrough` boolean is applicable to x86 PV dom0's only and defaults
>> +    to false.  It controls whether the IOMMU is fully disabled for devices
>> +    belonging to dom0 (`passthrough=1`), or whether the IOMMU is set up with
>> +    an identity transform for dom0 (`passthrough=0`) to prevent dom0 from
>> +    DMA'ing outside of its permitted areas.
>> +
>> +    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 DMA
>> +    remapping for all non-RAM regions below 4GB except for unusable ranges.
>> +
>> +    Typically, some devices in a system use bits of RAM for communication, and
>> +    these areas should be listed 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 writing its APCI tables, 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 Intel systems, disabled by
>> +    default on other x86 systems, and invalid on ARM systems.
> I'm afraid the previous text was wrong. I later discovered that AMD
> also had such workarounds applied by default, and unified the code,
> but failed to update the documentation, sorry.
>
> map-inclusive is enabled by default on x86 for a PV dom0. See
> xen/drivers/passthrough/x86/iommu.c:215 (arch_iommu_hwdom_init).
>
>> +
>> +*   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.
>> +
>> +    This option is enabled by default on x86 Intel systems, disabled by
>> +    default on other x86 systems, and invalid on ARM systems.
> map-reserved is enabled by default on x86,
> xen/drivers/passthrough/x86/iommu.c:218 (arch_iommu_hwdom_init).

Ok for both.  Will fix up.

~Andrew

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

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

[Roger Pau Monné]

On Fri, Dec 21, 2018 at 01:01:22PM +0000, Andrew Cooper wrote:
> On 21/12/2018 12:44, Roger Pau Monné wrote:
> > On Thu, Dec 20, 2018 at 11:40:52PM +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 resonably 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.
> > This looks very similar to the current 'passthrough' option, maybe it
> > would be enough to allow PVH dom0 to use the passthrough option
> > provided a warning is added to
> > arch_iommu_check_autotranslated_hwdom?
> 
> I considered that, but "dom0-iommu=passthrough" isn't an accurate
> description of what is going on.  Frankly, its not correct for PV either.

And what about turning dom0-iommu into a boolean option itself, so
that you can do "dom0-iommu=false"?

I think that's more similar to other Xen command line options that
have a boolean value and/or a list of sub-options.

> TBH, dom0-iommu=none is better for both.  How about I introduce that as
> the new option, and leave passthrough as a legacy alias?

That sounds good, I would like to avoid (if possible) the
proliferation of new iommu_hwdom_* variables, because we already have
a bunch and the functionality introduced by the 'none' option looks
very similar to what 'passthrough' aims to achieve.

Thanks, Roger.

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

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

[Roger Pau Monné]

On Fri, Dec 21, 2018 at 01:13:25PM +0000, Andrew Cooper wrote:
> On 21/12/2018 12:08, Roger Pau Monné wrote:
> > On Thu, Dec 20, 2018 at 11:40:51PM +0000, Andrew Cooper wrote:
> >> 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.
> >>
> >> Signed-off-by: Andrew Cooper <andrew.cooper3@citrix.com>
> > Thanks! A couple of fixes below, because the original text is actually
> > wrong...
> 
> TBH, that is my default assumption every time I do work like this :)

In this case it's my fault :(, because I changed the code and forgot
about the docs.

> >
> >> ---
> >> 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.
> >>
> >> It occurs to me that:
> >>
> >>  * The choice of dom0 boot mode should in part be derived from the available
> >>    CONFIG_* options, and ELF notes advertised in the dom0 kernel.
> > This is indeed doable, but would require parsing the dom0 kernel
> > before building the domain.
> 
> I don't see anything wrong with parsing the ELF headers ahead of
> building the domain.  From the overall boot time, its just an
> order-of-operations issue.

Oh yes, I didn't mean my comment to sound like criticism. I agree
there should be no issues in parsing the ELF earlier, or if there are
issues they should be fixed.

> >
> >>  * AMD probably needs to gain an `ivmd=` to mirror `rmrr=` on the Intel side,
> >>    because we know there are other errors in the IVRS table.
> > Yes, albeit using rmrr is quite cumbersome because it's mostly a
> > trial-and-error process until there are no more iommu faults (unless
> > you can get the correct rmrr command for your hardware somewhere).
> >
> >>  * Neither of map-{inclusive,reserved} should be active by default, even on
> >>    Intel hardware, and we should (wherever possible) have quirks like we have
> >>    for all other firmware screwups.  Requiring the user to diagnose/work
> >>    around firmware problems like this is quite rude.
> > That would indeed be nice, but I think there are too many vendor
> > firmware versions to be able to correctly identify such quirks, the
> > more that vendors don't even list missing RMRR as erratum.
> 
> I don't agree.  We already have quirks based on DMI (at the moment,
> mainly for reboot overrides), and the vast majority of the offending
> cases are the BMC shared mailbox, which will be in a fixed per-platform
> location.

IIRC I've only found a single box that worked without map-reserved,
and that's my NUC which has firmware from Intel. And even in that case
the USB ports weren't fully working.

I guess such quirks could be applied based on the chipset version then
if Xen realizes the firmware is either wrong or missing obvious RMRR
regions?

> I don't expect we'll ever find and fix all quirks, but where we do find
> suitable ones, we should put them into the boot code.

Sadly I agree. What I'm worried about is turning the default
map-{inclusive/reserved} to off, that's likely to make dom0 unable to
boot on a huge amount of hardware.

Thanks, Roger.

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

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

[Andrew Cooper]

On 21/12/2018 16:06, Roger Pau Monné wrote:
> On Fri, Dec 21, 2018 at 01:13:25PM +0000, Andrew Cooper wrote:
>> On 21/12/2018 12:08, Roger Pau Monné wrote:
>>> On Thu, Dec 20, 2018 at 11:40:51PM +0000, Andrew Cooper wrote:
>>>>  * AMD probably needs to gain an `ivmd=` to mirror `rmrr=` on the Intel side,
>>>>    because we know there are other errors in the IVRS table.
>>> Yes, albeit using rmrr is quite cumbersome because it's mostly a
>>> trial-and-error process until there are no more iommu faults (unless
>>> you can get the correct rmrr command for your hardware somewhere).
>>>
>>>>  * Neither of map-{inclusive,reserved} should be active by default, even on
>>>>    Intel hardware, and we should (wherever possible) have quirks like we have
>>>>    for all other firmware screwups.  Requiring the user to diagnose/work
>>>>    around firmware problems like this is quite rude.
>>> That would indeed be nice, but I think there are too many vendor
>>> firmware versions to be able to correctly identify such quirks, the
>>> more that vendors don't even list missing RMRR as erratum.
>> I don't agree.  We already have quirks based on DMI (at the moment,
>> mainly for reboot overrides), and the vast majority of the offending
>> cases are the BMC shared mailbox, which will be in a fixed per-platform
>> location.
> IIRC I've only found a single box that worked without map-reserved,
> and that's my NUC which has firmware from Intel. And even in that case
> the USB ports weren't fully working.
>
> I guess such quirks could be applied based on the chipset version then
> if Xen realizes the firmware is either wrong or missing obvious RMRR
> regions?

Ah - I'd forgotten the USB legacy keyboard support case.  We ought to be
able to reverse engineer a suitable RMRR from the USB port configuration.

>
>> I don't expect we'll ever find and fix all quirks, but where we do find
>> suitable ones, we should put them into the boot code.
> Sadly I agree. What I'm worried about is turning the default
> map-{inclusive/reserved} to off, that's likely to make dom0 unable to
> boot on a huge amount of hardware.

We've already got a difference between map-{inclusive/reserved} where
the former is enabled for PV and strictly unavailable for PVH, whereas
the latter is enabled for both.

When it comes to the IOMMU setup, there should not be a difference
between PV and PVH, because the difference in virtulisation mode is not
relevant to how the devices on the system behave.

IMO, map-inclusive should be disabled by default, and (due to
map-reserved being on by default), I expect we'll see change in behaviour.

~Andrew

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

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

[Jan Beulich]

>>> On 21.12.18 at 13:44, <andrew.cooper3@citrix.com> wrote:
> On 21/12/2018 12:08, Jan Beulich wrote:
>>>>> On 21.12.18 at 00:40, <andrew.cooper3@citrix.com> wrote:
>>> --- a/docs/misc/xen-command-line.markdown
>>> +++ b/docs/misc/xen-command-line.markdown
>>> @@ -636,55 +636,76 @@ 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:
>>> -
>>> -> `pvh`
>>> +> Applicability: x86
>> Why the new tag, when everything else uses (x86) next to the
>> option name?
> 
> See the commit message of c/s a3a99df44e5405d2092ec59087681765fa4cdee7
> 
> The problem is with the generated HTML anchors when trying to cross
> reference the options.

That commit message doesn't talk about anchors, so I can only
guess about the connection. But I think I can see your point.
From a purely textual perspective I have to admit I'd prefer
the original model (without the new tag), but if cross refs don't
work I agree it's better to switch.

>>>  ### 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 is applicable to x86 PV dom0's only and defaults
>>> +    to false.  It controls whether the IOMMU is fully disabled for devices
>>> +    belonging to dom0 (`passthrough=1`), or whether the IOMMU is set up with
>>> +    an identity transform for dom0 (`passthrough=0`) to prevent dom0 from
>>> +    DMA'ing outside of its permitted areas.
>>> +
>>> +    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 DMA
>>> +    remapping for all non-RAM regions below 4GB except for unusable ranges.
>> I don't thinks this is PV-specific, just its default is.
> 
> From arch_iommu_hwdom_init():
> 
>     /* 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;
>     }
> 
> 
> Attempting to use this option with a PVH dom0 will cause Xen to force it
> off.

Oh, I see - one of the bad effects of disconnects in source code
(the 3rd if() would better be else-if() for the 1st).

Jan


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

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

[Jan Beulich]

>>> On 21.12.18 at 13:55, <andrew.cooper3@citrix.com> wrote:
> On 21/12/2018 12:13, Jan Beulich wrote:
>>>>> On 21.12.18 at 00:40, <andrew.cooper3@citrix.com> wrote:
>>> @@ -271,6 +272,26 @@ int parse_boolean(const char *name, const char *s, const char *e)
>>>      return -1;
>>>  }
>>>  
>>> +int cmdline_strcmp(const char *frag, const char *name)
>> __init ?
> 
> I think there are some runtime parameters in need of some fixing as well.

Ideally we'd drop __init at the point such a change indeed goes in. As
a compromise you may want to mention the reason for the omission
in the description (preferably with a concrete example).

>>> +{
>>> +    while ( 1 )
>>> +    {
>>> +        int res = (*frag - *name);
>>> +
>>> +        if ( res || *name == '\0' )
>>> +        {
>>> +            /* NUL in 'name' matching punctuation in 'frag' implies success. */
>>> +            if ( *name == '\0' && ispunct(*frag) )
>>> +                res = 0;
>> Isn't ispunct() true for dashes and perhaps also underscores?
>> I don't think it can be this generic, the more that ...
>>
>>> --- 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 punctuationin 'frag'.  Designed for picking exact string
>>> + * matches out of a comma-separated command line fragment.
>>> + */
>>> +int cmdline_strcmp(const char *frag, const char *name);
>> ... you talk of commas only here.
> 
> I actually borrowed this function from my CPUID cmdline patch.  In 99%
> of cases, we only need to match = and , but we have some other
> parameters such as psr= which use : for delimiters, hence the use of
> ispunct().
> 
> As an alternative, I could revert back to explicitly checking the
> expected punctuation.  It is not as if this is a fastpath.

Imo this would be better than treating punctuation we don't want
to consider separators the same as intended-to-be-separators.

Jan



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

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

[Roger Pau Monné]

On Mon, Dec 24, 2018 at 12:46:06PM +0000, Andrew Cooper wrote:
> When it comes to the IOMMU setup, there should not be a difference
> between PV and PVH, because the difference in virtulisation mode is not
> relevant to how the devices on the system behave.
> 
> IMO, map-inclusive should be disabled by default, and (due to
> map-reserved being on by default), I expect we'll see change in behaviour.

I'm OK with this, would you like to send a patch to that effect?

Thanks, Roger.

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

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

[Andrew Cooper]

On 04/01/2019 12:04, Roger Pau Monné wrote:
> On Mon, Dec 24, 2018 at 12:46:06PM +0000, Andrew Cooper wrote:
>> When it comes to the IOMMU setup, there should not be a difference
>> between PV and PVH, because the difference in virtulisation mode is not
>> relevant to how the devices on the system behave.
>>
>> IMO, map-inclusive should be disabled by default, and (due to
>> map-reserved being on by default), I expect we'll see change in behaviour.
> I'm OK with this, would you like to send a patch to that effect?

There is a patch to this effect in the v2 series which I posted.

I'll have to post a v3 to rebase over the strncmp(... ss - s) issue, but
the content in the series is broadly fine.

~Andrew

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