[PATCH v3 for-4.12 0/7] Docs improvements, and dom0 construction fixes

[PATCH v3 for-4.12 0/7] Docs improvements, and dom0 construction fixes

[Andrew Cooper]

This is logically two series, but they were co-developed and tightly coupled.

The first 4 patches are improvements to our documentation and command line
parsing.  It fixes two key issues - first that sub-booleans now all behave
consistently, and second is the removal of block indentation markup for main
documentation.  It is excessively verbose and renders poorly to both HTML and
PDF.

The second 3 patches make improvements to dom0 parsing and configuration, with
the intent of making it possible to use a PVH Xen and PVH dom0 XTF test for
development purposes.

Patch 5 at a bare minimum is required for 4.12 to fix an issue which hasn't
been released yet.  All others are nice-to-have with a low risk of problems.

Major changes from v2:

  * The cmdline_strcmp() functionality from patch 5 was split out into a
    separate bugfix for backporting purposes.

Andrew Cooper (7):
  docs: Improve documentation for dom0= and dom0-iommu=
  docs: Improve documentation and parsing for iommu=
  docs: Improve documentation and parsing for pci=
  docs: Improve documentation and parsing for efi=
  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.pandoc     | 319 +++++++++++++++++-----------------
 xen/arch/x86/dom0_build.c             |  14 +-
 xen/common/efi/boot.c                 |  11 +-
 xen/drivers/passthrough/arm/smmu.c    |   4 -
 xen/drivers/passthrough/iommu.c       |  71 +++-----
 xen/drivers/passthrough/pci.c         |  20 +--
 xen/drivers/passthrough/vtd/x86/vtd.c |   6 -
 xen/drivers/passthrough/x86/iommu.c   |  14 +-
 xen/include/xen/iommu.h               |   2 +-
 9 files changed, 197 insertions(+), 264 deletions(-)

-- 
2.1.4


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

[PATCH v3 1/7] docs: Improve documentation for dom0= and dom0-iommu=

[Andrew Cooper]

Update to the latest metadata style, and discuss the options more
completely where appropriate.

Drop the redundant comment beside parse_dom0_param() - it is already 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>
CC: Juergen Gross <jgross@suse.com>

v2:
 * Fix statement of defaults
 * Tweak wording.  In particular, expand the description of passthrough.
v3:
 * Rebase over dom0=verbose
---
 docs/misc/xen-command-line.pandoc | 130 ++++++++++++++++++++------------------
 xen/arch/x86/dom0_build.c         |   6 --
 2 files changed, 67 insertions(+), 69 deletions(-)

diff --git a/docs/misc/xen-command-line.pandoc b/docs/misc/xen-command-line.pandoc
index d39bcee..243193d 100644
--- a/docs/misc/xen-command-line.pandoc
+++ b/docs/misc/xen-command-line.pandoc
@@ -636,61 +636,83 @@ trace feature is only enabled in debugging builds of Xen.
 
 Specify the bit width of the DMA heap.
 
-### dom0 (x86)
-> `= List of [ pvh | shadow | verbose ]`
+### dom0
+    = List of [ pvh=<bool>, shadow=<bool>, verbose=<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`
+    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.
 
-Flag that makes a dom0 use shadow paging. Only works when "pvh" is
-enabled.
+*   The `verbose` boolean is intended for diagnostics, and prints out extra
+    information during the dom0 build.  It defaults to false.
 
-> `verbose`
+### dom0-iommu
+    = List of [ passthrough=<bool>, strict=<bool>, map-inclusive=<bool>,
+                map-reserved=<bool> ]
 
-> Default: `false`
+Controls for the dom0 IOMMU setup.
 
-Print debug information during dom0 build.
+*   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`).
 
-### 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.
+    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>`
@@ -1181,20 +1203,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`
@@ -1241,21 +1254,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 c0bc022..7f6ee7f 100644
--- a/xen/arch/x86/dom0_build.c
+++ b/xen/arch/x86/dom0_build.c
@@ -283,12 +283,6 @@ bool __initdata opt_dom0_shadow;
 bool __initdata dom0_pvh;
 bool __initdata dom0_verbose;
 
-/*
- * 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

Re: [PATCH v3 1/7] docs: Improve documentation for dom0= and dom0-iommu=

[Roger Pau Monné]

On Wed, Jan 16, 2019 at 09:00:44AM +0000, Andrew Cooper wrote:
> Update to the latest metadata style, and discuss the options more
> completely where appropriate.
> 
> Drop the redundant comment beside parse_dom0_param() - it is already 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>

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

Just one comment.

[...]
> +*   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.

Should you add "This option is enabled by default on x86 systems, and
invalid on ARM systems." here?

Thanks, Roger.

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

Re: [PATCH v3 1/7] docs: Improve documentation for dom0= and dom0-iommu=

[Andrew Cooper]

On 16/01/2019 10:53, Roger Pau Monné wrote:
> On Wed, Jan 16, 2019 at 09:00:44AM +0000, Andrew Cooper wrote:
>> Update to the latest metadata style, and discuss the options more
>> completely where appropriate.
>>
>> Drop the redundant comment beside parse_dom0_param() - it is already 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>
> Reviewed-by: Roger Pau Monné <roger.pau@citrix.com>
>
> Just one comment.
>
> [...]
>> +*   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.
> Should you add "This option is enabled by default on x86 systems, and
> invalid on ARM systems." here?

What I listed here where the differences from map-inclusive, so the ARM
bit is covered IMO.

However, this bit of text doesn't survive past patch 6 anyway, so it
really doesn't matter.

~Andrew

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

Re: [PATCH v3 1/7] docs: Improve documentation for dom0= and dom0-iommu=

[Jan Beulich]

>>> On 16.01.19 at 10:00, <andrew.cooper3@citrix.com> wrote:
> --- a/docs/misc/xen-command-line.pandoc
> +++ b/docs/misc/xen-command-line.pandoc
> @@ -636,61 +636,83 @@ trace feature is only enabled in debugging builds of Xen.
>  
>  Specify the bit width of the DMA heap.
>  
> -### dom0 (x86)
> -> `= List of [ pvh | shadow | verbose ]`
> +### dom0
> +    = List of [ pvh=<bool>, shadow=<bool>, verbose=<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`
> +    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.

As mentioned elsewhere, I object to adding CONFIG_* into this doc,
which is intended to be meaningful to non-developers. But not to the
degree of NAK-ing the whole thing, if everyone else disagrees with me.

Jan



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

Re: [PATCH v3 1/7] docs: Improve documentation for dom0= and dom0-iommu=

[Andrew Cooper]

On 16/01/2019 11:52, Jan Beulich wrote:
>>>> On 16.01.19 at 10:00, <andrew.cooper3@citrix.com> wrote:
>> --- a/docs/misc/xen-command-line.pandoc
>> +++ b/docs/misc/xen-command-line.pandoc
>> @@ -636,61 +636,83 @@ trace feature is only enabled in debugging builds of Xen.
>>  
>>  Specify the bit width of the DMA heap.
>>  
>> -### dom0 (x86)
>> -> `= List of [ pvh | shadow | verbose ]`
>> +### dom0
>> +    = List of [ pvh=<bool>, shadow=<bool>, verbose=<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`
>> +    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.
> As mentioned elsewhere, I object to adding CONFIG_* into this doc,
> which is intended to be meaningful to non-developers. But not to the
> degree of NAK-ing the whole thing, if everyone else disagrees with me.

I'm not sure what else to say.  I object to purposefully omitting
relevant information from our documentation.

Most people reading it, including non-developers, will know what Kconfig
is and how to check, owing to at least a basic knowledge of Linux. 
Those that don't will be capable of basic human interaction such as
asking a question of someone more knowledgeable.

~Andrew

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

Re: [PATCH v3 1/7] docs: Improve documentation for dom0= and dom0-iommu=

[Roger Pau Monné]

On Wed, Jan 16, 2019 at 07:51:33PM +0000, Andrew Cooper wrote:
> On 16/01/2019 11:52, Jan Beulich wrote:
> >>>> On 16.01.19 at 10:00, <andrew.cooper3@citrix.com> wrote:
> >> --- a/docs/misc/xen-command-line.pandoc
> >> +++ b/docs/misc/xen-command-line.pandoc
> >> @@ -636,61 +636,83 @@ trace feature is only enabled in debugging builds of Xen.
> >>  
> >>  Specify the bit width of the DMA heap.
> >>  
> >> -### dom0 (x86)
> >> -> `= List of [ pvh | shadow | verbose ]`
> >> +### dom0
> >> +    = List of [ pvh=<bool>, shadow=<bool>, verbose=<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`
> >> +    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.
> > As mentioned elsewhere, I object to adding CONFIG_* into this doc,
> > which is intended to be meaningful to non-developers. But not to the
> > degree of NAK-ing the whole thing, if everyone else disagrees with me.
> 
> I'm not sure what else to say.  I object to purposefully omitting
> relevant information from our documentation.

Maybe it would be helpful to add some kind of tag that could
standardize the relationship between Kconfig options and command line
options?

    Kconfig: SHADOW_PAGING

Or similar. This would get the specific Kconfig details out of the
general description of the functionality, thus not harming readability
by non-expert users?

Using such tag would require some explanation of it's meaning at the
top of the document.

> Most people reading it, including non-developers, will know what Kconfig
> is and how to check, owing to at least a basic knowledge of Linux. 
> Those that don't will be capable of basic human interaction such as
> asking a question of someone more knowledgeable.

If the above is not suitable, I might suggest to reword the sentence
as:

"This option is unavailable when the Kconfig `SHADOW_PAGING` option is
not selected at build time."

Explicitly mentioning Kconfig and selected simplifies the language for
non-expert users IMO, and makes it clear this is exclusively a build
time decision. Note I'm not a native speaker, so my sense of easier to
understand could be completely wrong.

Thanks, Roger.

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

Re: [PATCH v3 1/7] docs: Improve documentation for dom0= and dom0-iommu=

[Andrew Cooper]

On 17/01/2019 08:43, Roger Pau Monné wrote:
> On Wed, Jan 16, 2019 at 07:51:33PM +0000, Andrew Cooper wrote:
>> On 16/01/2019 11:52, Jan Beulich wrote:
>>>>>> On 16.01.19 at 10:00, <andrew.cooper3@citrix.com> wrote:
>>>> --- a/docs/misc/xen-command-line.pandoc
>>>> +++ b/docs/misc/xen-command-line.pandoc
>>>> @@ -636,61 +636,83 @@ trace feature is only enabled in debugging builds of Xen.
>>>>  
>>>>  Specify the bit width of the DMA heap.
>>>>  
>>>> -### dom0 (x86)
>>>> -> `= List of [ pvh | shadow | verbose ]`
>>>> +### dom0
>>>> +    = List of [ pvh=<bool>, shadow=<bool>, verbose=<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`
>>>> +    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.
>>> As mentioned elsewhere, I object to adding CONFIG_* into this doc,
>>> which is intended to be meaningful to non-developers. But not to the
>>> degree of NAK-ing the whole thing, if everyone else disagrees with me.
>> I'm not sure what else to say.  I object to purposefully omitting
>> relevant information from our documentation.
> Maybe it would be helpful to add some kind of tag that could
> standardize the relationship between Kconfig options and command line
> options?
>
>     Kconfig: SHADOW_PAGING
>
> Or similar. This would get the specific Kconfig details out of the
> general description of the functionality, thus not harming readability
> by non-expert users?
>
> Using such tag would require some explanation of it's meaning at the
> top of the document.
>
>> Most people reading it, including non-developers, will know what Kconfig
>> is and how to check, owing to at least a basic knowledge of Linux. 
>> Those that don't will be capable of basic human interaction such as
>> asking a question of someone more knowledgeable.
> If the above is not suitable, I might suggest to reword the sentence
> as:
>
> "This option is unavailable when the Kconfig `SHADOW_PAGING` option is
> not selected at build time."
>
> Explicitly mentioning Kconfig and selected simplifies the language for
> non-expert users IMO, and makes it clear this is exclusively a build
> time decision. Note I'm not a native speaker, so my sense of easier to
> understand could be completely wrong.

I have a rewrite of the head of the document pending anyway which I hope
to get sorted properly for 4.12

While having a Kconfig: section would probably be fine for ~80% of the
simple cases, it doesn't work for this patch.

I guess the root of the issue is that I do not believe that phrasing the
information like this makes it harder for non-expert users
read/comprehend, and there definitely are a group of readers for which
this information is relevant.

~Andrew

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

Re: [PATCH v3 1/7] docs: Improve documentation for dom0= and dom0-iommu=

[Juergen Gross]

On 17/01/2019 10:08, Andrew Cooper wrote:
> On 17/01/2019 08:43, Roger Pau Monné wrote:
>> On Wed, Jan 16, 2019 at 07:51:33PM +0000, Andrew Cooper wrote:
>>> On 16/01/2019 11:52, Jan Beulich wrote:
>>>>>>> On 16.01.19 at 10:00, <andrew.cooper3@citrix.com> wrote:
>>>>> --- a/docs/misc/xen-command-line.pandoc
>>>>> +++ b/docs/misc/xen-command-line.pandoc
>>>>> @@ -636,61 +636,83 @@ trace feature is only enabled in debugging builds of Xen.
>>>>>  
>>>>>  Specify the bit width of the DMA heap.
>>>>>  
>>>>> -### dom0 (x86)
>>>>> -> `= List of [ pvh | shadow | verbose ]`
>>>>> +### dom0
>>>>> +    = List of [ pvh=<bool>, shadow=<bool>, verbose=<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`
>>>>> +    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.
>>>> As mentioned elsewhere, I object to adding CONFIG_* into this doc,
>>>> which is intended to be meaningful to non-developers. But not to the
>>>> degree of NAK-ing the whole thing, if everyone else disagrees with me.
>>> I'm not sure what else to say.  I object to purposefully omitting
>>> relevant information from our documentation.
>> Maybe it would be helpful to add some kind of tag that could
>> standardize the relationship between Kconfig options and command line
>> options?
>>
>>     Kconfig: SHADOW_PAGING
>>
>> Or similar. This would get the specific Kconfig details out of the
>> general description of the functionality, thus not harming readability
>> by non-expert users?
>>
>> Using such tag would require some explanation of it's meaning at the
>> top of the document.
>>
>>> Most people reading it, including non-developers, will know what Kconfig
>>> is and how to check, owing to at least a basic knowledge of Linux. 
>>> Those that don't will be capable of basic human interaction such as
>>> asking a question of someone more knowledgeable.
>> If the above is not suitable, I might suggest to reword the sentence
>> as:
>>
>> "This option is unavailable when the Kconfig `SHADOW_PAGING` option is
>> not selected at build time."
>>
>> Explicitly mentioning Kconfig and selected simplifies the language for
>> non-expert users IMO, and makes it clear this is exclusively a build
>> time decision. Note I'm not a native speaker, so my sense of easier to
>> understand could be completely wrong.
> 
> I have a rewrite of the head of the document pending anyway which I hope
> to get sorted properly for 4.12
> 
> While having a Kconfig: section would probably be fine for ~80% of the
> simple cases, it doesn't work for this patch.
> 
> I guess the root of the issue is that I do not believe that phrasing the
> information like this makes it harder for non-expert users
> read/comprehend, and there definitely are a group of readers for which
> this information is relevant.

In any case I'd prefer to spell out the complete config option (e.g.
CONFIG_FOO) in case we ever get a way to read the config from the
running hypervisor (FWIW I'm just writing a series for doing that).


Juergen

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

Re: [PATCH v3 1/7] docs: Improve documentation for dom0= and dom0-iommu=

[Jan Beulich]

>>> On 17.01.19 at 10:14, <jgross@suse.com> wrote:
> On 17/01/2019 10:08, Andrew Cooper wrote:
>> On 17/01/2019 08:43, Roger Pau Monné wrote:
>>> On Wed, Jan 16, 2019 at 07:51:33PM +0000, Andrew Cooper wrote:
>>>> On 16/01/2019 11:52, Jan Beulich wrote:
>>>>>>>> On 16.01.19 at 10:00, <andrew.cooper3@citrix.com> wrote:
>>>>>> --- a/docs/misc/xen-command-line.pandoc
>>>>>> +++ b/docs/misc/xen-command-line.pandoc
>>>>>> @@ -636,61 +636,83 @@ trace feature is only enabled in debugging builds of 
> Xen.
>>>>>>  
>>>>>>  Specify the bit width of the DMA heap.
>>>>>>  
>>>>>> -### dom0 (x86)
>>>>>> -> `= List of [ pvh | shadow | verbose ]`
>>>>>> +### dom0
>>>>>> +    = List of [ pvh=<bool>, shadow=<bool>, verbose=<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`
>>>>>> +    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.
>>>>> As mentioned elsewhere, I object to adding CONFIG_* into this doc,
>>>>> which is intended to be meaningful to non-developers. But not to the
>>>>> degree of NAK-ing the whole thing, if everyone else disagrees with me.
>>>> I'm not sure what else to say.  I object to purposefully omitting
>>>> relevant information from our documentation.
>>> Maybe it would be helpful to add some kind of tag that could
>>> standardize the relationship between Kconfig options and command line
>>> options?
>>>
>>>     Kconfig: SHADOW_PAGING
>>>
>>> Or similar. This would get the specific Kconfig details out of the
>>> general description of the functionality, thus not harming readability
>>> by non-expert users?
>>>
>>> Using such tag would require some explanation of it's meaning at the
>>> top of the document.
>>>
>>>> Most people reading it, including non-developers, will know what Kconfig
>>>> is and how to check, owing to at least a basic knowledge of Linux. 
>>>> Those that don't will be capable of basic human interaction such as
>>>> asking a question of someone more knowledgeable.
>>> If the above is not suitable, I might suggest to reword the sentence
>>> as:
>>>
>>> "This option is unavailable when the Kconfig `SHADOW_PAGING` option is
>>> not selected at build time."
>>>
>>> Explicitly mentioning Kconfig and selected simplifies the language for
>>> non-expert users IMO, and makes it clear this is exclusively a build
>>> time decision. Note I'm not a native speaker, so my sense of easier to
>>> understand could be completely wrong.
>> 
>> I have a rewrite of the head of the document pending anyway which I hope
>> to get sorted properly for 4.12
>> 
>> While having a Kconfig: section would probably be fine for ~80% of the
>> simple cases, it doesn't work for this patch.
>> 
>> I guess the root of the issue is that I do not believe that phrasing the
>> information like this makes it harder for non-expert users
>> read/comprehend, and there definitely are a group of readers for which
>> this information is relevant.
> 
> In any case I'd prefer to spell out the complete config option (e.g.
> CONFIG_FOO) in case we ever get a way to read the config from the
> running hypervisor (FWIW I'm just writing a series for doing that).

Well, as expressed in earlier replies - I'm particularly against the
CONFIG_ prefix, which no-one will find when grep-ing Kconfig
files. This prefix is an implementation detail, to reduce the risk of
name collisions. FWIW I'm very much in favor of going with either
of the variants suggested by Roger; it is really secondary to me
which of the two was chosen.

Jan


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

Re: [PATCH v3 1/7] docs: Improve documentation for dom0= and dom0-iommu=

[Andrew Cooper]

On 17/01/2019 09:14, Juergen Gross wrote:
> On 17/01/2019 10:08, Andrew Cooper wrote:
>> On 17/01/2019 08:43, Roger Pau Monné wrote:
>>> On Wed, Jan 16, 2019 at 07:51:33PM +0000, Andrew Cooper wrote:
>>>> On 16/01/2019 11:52, Jan Beulich wrote:
>>>>>>>> On 16.01.19 at 10:00, <andrew.cooper3@citrix.com> wrote:
>>>>>> --- a/docs/misc/xen-command-line.pandoc
>>>>>> +++ b/docs/misc/xen-command-line.pandoc
>>>>>> @@ -636,61 +636,83 @@ trace feature is only enabled in debugging builds of Xen.
>>>>>>  
>>>>>>  Specify the bit width of the DMA heap.
>>>>>>  
>>>>>> -### dom0 (x86)
>>>>>> -> `= List of [ pvh | shadow | verbose ]`
>>>>>> +### dom0
>>>>>> +    = List of [ pvh=<bool>, shadow=<bool>, verbose=<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`
>>>>>> +    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.
>>>>> As mentioned elsewhere, I object to adding CONFIG_* into this doc,
>>>>> which is intended to be meaningful to non-developers. But not to the
>>>>> degree of NAK-ing the whole thing, if everyone else disagrees with me.
>>>> I'm not sure what else to say.  I object to purposefully omitting
>>>> relevant information from our documentation.
>>> Maybe it would be helpful to add some kind of tag that could
>>> standardize the relationship between Kconfig options and command line
>>> options?
>>>
>>>     Kconfig: SHADOW_PAGING
>>>
>>> Or similar. This would get the specific Kconfig details out of the
>>> general description of the functionality, thus not harming readability
>>> by non-expert users?
>>>
>>> Using such tag would require some explanation of it's meaning at the
>>> top of the document.
>>>
>>>> Most people reading it, including non-developers, will know what Kconfig
>>>> is and how to check, owing to at least a basic knowledge of Linux. 
>>>> Those that don't will be capable of basic human interaction such as
>>>> asking a question of someone more knowledgeable.
>>> If the above is not suitable, I might suggest to reword the sentence
>>> as:
>>>
>>> "This option is unavailable when the Kconfig `SHADOW_PAGING` option is
>>> not selected at build time."
>>>
>>> Explicitly mentioning Kconfig and selected simplifies the language for
>>> non-expert users IMO, and makes it clear this is exclusively a build
>>> time decision. Note I'm not a native speaker, so my sense of easier to
>>> understand could be completely wrong.
>> I have a rewrite of the head of the document pending anyway which I hope
>> to get sorted properly for 4.12
>>
>> While having a Kconfig: section would probably be fine for ~80% of the
>> simple cases, it doesn't work for this patch.
>>
>> I guess the root of the issue is that I do not believe that phrasing the
>> information like this makes it harder for non-expert users
>> read/comprehend, and there definitely are a group of readers for which
>> this information is relevant.
> In any case I'd prefer to spell out the complete config option (e.g.
> CONFIG_FOO) in case we ever get a way to read the config from the
> running hypervisor (FWIW I'm just writing a series for doing that).

I think having a Xen equivalent of /proc/config.gz is a good move,
irrespective of any documentation concerns.

~Andrew

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

Re: [PATCH v3 1/7] docs: Improve documentation for dom0= and dom0-iommu=

[Jan Beulich]

>>> On 16.01.19 at 20:51, <andrew.cooper3@citrix.com> wrote:
> On 16/01/2019 11:52, Jan Beulich wrote:
>>>>> On 16.01.19 at 10:00, <andrew.cooper3@citrix.com> wrote:
>>> --- a/docs/misc/xen-command-line.pandoc
>>> +++ b/docs/misc/xen-command-line.pandoc
>>> @@ -636,61 +636,83 @@ trace feature is only enabled in debugging builds of Xen.
>>>  
>>>  Specify the bit width of the DMA heap.
>>>  
>>> -### dom0 (x86)
>>> -> `= List of [ pvh | shadow | verbose ]`
>>> +### dom0
>>> +    = List of [ pvh=<bool>, shadow=<bool>, verbose=<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`
>>> +    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.
>> As mentioned elsewhere, I object to adding CONFIG_* into this doc,
>> which is intended to be meaningful to non-developers. But not to the
>> degree of NAK-ing the whole thing, if everyone else disagrees with me.
> 
> I'm not sure what else to say.  I object to purposefully omitting
> relevant information from our documentation.

But I'm not asking to omit the information. I'm asking to present it
in a way understandable to anyone, irrespective of their Kconfig
knowledge.

Jan



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

Re: [PATCH v3 1/7] docs: Improve documentation for dom0= and dom0-iommu=

[Andrew Cooper]

On 17/01/2019 11:20, Jan Beulich wrote:
>>>> On 16.01.19 at 20:51, <andrew.cooper3@citrix.com> wrote:
>> On 16/01/2019 11:52, Jan Beulich wrote:
>>>>>> On 16.01.19 at 10:00, <andrew.cooper3@citrix.com> wrote:
>>>> --- a/docs/misc/xen-command-line.pandoc
>>>> +++ b/docs/misc/xen-command-line.pandoc
>>>> @@ -636,61 +636,83 @@ trace feature is only enabled in debugging builds of Xen.
>>>>  
>>>>  Specify the bit width of the DMA heap.
>>>>  
>>>> -### dom0 (x86)
>>>> -> `= List of [ pvh | shadow | verbose ]`
>>>> +### dom0
>>>> +    = List of [ pvh=<bool>, shadow=<bool>, verbose=<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`
>>>> +    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.
>>> As mentioned elsewhere, I object to adding CONFIG_* into this doc,
>>> which is intended to be meaningful to non-developers. But not to the
>>> degree of NAK-ing the whole thing, if everyone else disagrees with me.
>> I'm not sure what else to say.  I object to purposefully omitting
>> relevant information from our documentation.
> But I'm not asking to omit the information. I'm asking to present it
> in a way understandable to anyone, irrespective of their Kconfig
> knowledge.

You have literally contradicted yourself in your two replies here.

Your latest reply suggests that you didn't mean what you actually wrote
earlier.  If this is the case, please take more care to get your point
across clearly.

~Andrew

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

Re: [PATCH v3 1/7] docs: Improve documentation for dom0= and dom0-iommu=

[Jan Beulich]

>>> On 17.01.19 at 13:15, <andrew.cooper3@citrix.com> wrote:
> On 17/01/2019 11:20, Jan Beulich wrote:
>>>>> On 16.01.19 at 20:51, <andrew.cooper3@citrix.com> wrote:
>>> On 16/01/2019 11:52, Jan Beulich wrote:
>>>>>>> On 16.01.19 at 10:00, <andrew.cooper3@citrix.com> wrote:
>>>>> --- a/docs/misc/xen-command-line.pandoc
>>>>> +++ b/docs/misc/xen-command-line.pandoc
>>>>> @@ -636,61 +636,83 @@ trace feature is only enabled in debugging builds of Xen.
>>>>>  
>>>>>  Specify the bit width of the DMA heap.
>>>>>  
>>>>> -### dom0 (x86)
>>>>> -> `= List of [ pvh | shadow | verbose ]`
>>>>> +### dom0
>>>>> +    = List of [ pvh=<bool>, shadow=<bool>, verbose=<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`
>>>>> +    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.
>>>> As mentioned elsewhere, I object to adding CONFIG_* into this doc,
>>>> which is intended to be meaningful to non-developers. But not to the
>>>> degree of NAK-ing the whole thing, if everyone else disagrees with me.
>>> I'm not sure what else to say.  I object to purposefully omitting
>>> relevant information from our documentation.
>> But I'm not asking to omit the information. I'm asking to present it
>> in a way understandable to anyone, irrespective of their Kconfig
>> knowledge.
> 
> You have literally contradicted yourself in your two replies here.
> 
> Your latest reply suggests that you didn't mean what you actually wrote
> earlier.  If this is the case, please take more care to get your point
> across clearly.

Hmm, apologies, my use CONFIG_* above was indeed ambiguous
without the context implied by "mentioned elsewhere". As "mentioned
elsewhere" I'm fine with any form of wording that names the option
without making it a primary part of the text, and without the CONFIG_
prefix. As also said elsewhere (but perhaps due to not being a native
speaker) I also dislike your use of "compiled out" (and its inverse).
Both parts get addressed by either of Roger's suggestions.

Jan



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

Re: [PATCH v3 1/7] docs: Improve documentation for dom0= and dom0-iommu=

[Andrew Cooper]

On 17/01/2019 12:26, Jan Beulich wrote:
>>>> On 17.01.19 at 13:15, <andrew.cooper3@citrix.com> wrote:
>> On 17/01/2019 11:20, Jan Beulich wrote:
>>>>>> On 16.01.19 at 20:51, <andrew.cooper3@citrix.com> wrote:
>>>> On 16/01/2019 11:52, Jan Beulich wrote:
>>>>>>>> On 16.01.19 at 10:00, <andrew.cooper3@citrix.com> wrote:
>>>>>> --- a/docs/misc/xen-command-line.pandoc
>>>>>> +++ b/docs/misc/xen-command-line.pandoc
>>>>>> @@ -636,61 +636,83 @@ trace feature is only enabled in debugging builds of Xen.
>>>>>>  
>>>>>>  Specify the bit width of the DMA heap.
>>>>>>  
>>>>>> -### dom0 (x86)
>>>>>> -> `= List of [ pvh | shadow | verbose ]`
>>>>>> +### dom0
>>>>>> +    = List of [ pvh=<bool>, shadow=<bool>, verbose=<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`
>>>>>> +    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.
>>>>> As mentioned elsewhere, I object to adding CONFIG_* into this doc,
>>>>> which is intended to be meaningful to non-developers. But not to the
>>>>> degree of NAK-ing the whole thing, if everyone else disagrees with me.
>>>> I'm not sure what else to say.  I object to purposefully omitting
>>>> relevant information from our documentation.
>>> But I'm not asking to omit the information. I'm asking to present it
>>> in a way understandable to anyone, irrespective of their Kconfig
>>> knowledge.
>> You have literally contradicted yourself in your two replies here.
>>
>> Your latest reply suggests that you didn't mean what you actually wrote
>> earlier.  If this is the case, please take more care to get your point
>> across clearly.
> Hmm, apologies, my use CONFIG_* above was indeed ambiguous
> without the context implied by "mentioned elsewhere". As "mentioned
> elsewhere" I'm fine with any form of wording that names the option
> without making it a primary part of the text, and without the CONFIG_
> prefix. As also said elsewhere (but perhaps due to not being a native
> speaker) I also dislike your use of "compiled out" (and its inverse).
> Both parts get addressed by either of Roger's suggestions.

I've switched to phrasing things as enabled/disabled rather than
compiled in/out.

However, the CONFIG_ prefixes are staying, because as said by myself and
others in this thread, they are critical for the understanding of
non-expert users, who won't be aware of implicitly dropping or adding
the prefix depending on circumstance.

~Andrew

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

[PATCH v3 2/7] docs: Improve documentation and parsing for iommu=

[Andrew Cooper]

Update parse_iommu_param() to uniformly use parse_boolean(), so the sub
booleans behave like other Xen boolean options.  Reposition the
custom_param() to avoid a forward declaration of parse_iommu_param().

Rewrite the command line documentation almost from scratch, including
far more detail.

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: Juergen Gross <jgross@suse.com>

v3:
 * New
---
 docs/misc/xen-command-line.pandoc | 153 ++++++++++++++++++++------------------
 xen/drivers/passthrough/iommu.c   |  63 +++++-----------
 2 files changed, 99 insertions(+), 117 deletions(-)

diff --git a/docs/misc/xen-command-line.pandoc b/docs/misc/xen-command-line.pandoc
index 243193d..ab486e0 100644
--- a/docs/misc/xen-command-line.pandoc
+++ b/docs/misc/xen-command-line.pandoc
@@ -1146,104 +1146,113 @@ detection of systems known to misbehave upon accesses to that port.
 > Default: `new` unless directed-EOI is supported
 
 ### iommu
-> `= List of [ <boolean> | force | required | intremap | intpost | qinval | snoop | sharept | dom0-passthrough | dom0-strict | amd-iommu-perdev-intremap | workaround_bios_bug | igfx | verbose | debug ]`
+    = List of [ <bool>, verbose, debug, force, required,
+                sharept, intremap, intpost,
+                snoop, qinval, igfx, workaround_bios_bug,
+                amd-iommu-perdev-intremap,
+                dom0-{passthrough,strict} ]
 
-> Sub-options:
+    All sub-options are boolean in nature.
 
-> `<boolean>`
+I/O Memory Memory Units perform a function similar to the CPU MMU (hence the
+name), but typically exist as a discrete device, integrated as part of a PCI
+Root Complex.  The most common configuration is to have one IOMMU per package
+(for on-die PCIe devices and directly attached PCIe lanes), and one IOMMU
+covering the remaining I/O in the system.
 
-> Default: `on`
-
->> Control the use of IOMMU(s) in the system.
-
-> All other sub-options are of boolean kind and can be prefixed with `no-` to
-> effect the inverse meaning.
-
-> `force` or `required`
+The functionality in an IOMMU commonly falls into two orthogonal categories:
 
-> Default: `false`
-
->> Don't continue booting unless IOMMU support is found and can be initialized
->> successfully.
+1.  DMA remapping which uses a pagetable-like hierarchical structure and maps
+    I/O Virtual Addresses (DFNs - Device Frame Numbers in Xen's terminology)
+    to System Physical Addresses (MFNs - Machine Frame Numbers in Xen's
+    terminology).
 
-> `intremap`
+2.  Interrupt Remapping, which controls incoming Message Signalled Interrupt
+    requests, including their routing to specific CPUs.
 
-> Default: `true`
+IOMMU functionality can be used either to provide a translation which the
+hardware device driver isn't aware of (e.g. PCI Passthrough and a native
+driver inside the guest) or to enforce fine-grained control over the memory
+and interrupts which a device is attempting to access.
 
->> Control the use of interrupt remapping (DMA remapping will always be enabled
->> if IOMMU functionality is enabled).
+By default, IOMMUs are configured for use if they are available.  An overall
+boolean (e.g. `iommu=no`) can override this and leave the IOMMUs disabled.
 
-> `intpost`
+*   The `verbose` and `debug` booleans can be used to print additional
+    diagnostic information.  Neither are active by default.
 
-> Default: `false`
+*   The `force` and `required` booleans are synonymous and, when requested, will
+    prevent Xen from booting if IOMMUs aren't discovered and enabled
+    successfully.
 
->> Control the use of interrupt posting, which depends on the availability of
->> interrupt remapping.
-
-> `qinval` (VT-d)
-
-> Default: `true`
-
->> Control the use of Queued Invalidation.
-
-> `snoop` (Intel)
-
-> Default: `true`
+*   The `sharept` boolean controls whether the IOMMU pagetables are shared with
+    the CPU-side HAP pagetables, or allocated separately.  Sharing reduces the
+    memory overhead, but doesn't work in combination with CPU-side
+    pagefault-based features, e.g. dirty VRAM tracking when a PCI device is
+    assigned.
 
->> Control the use of Snoop Control.
-
-> `sharept`
-
-> Default: `true`
-
->> Control whether CPU and IOMMU page tables should be shared.
-
-> `dom0-passthrough`
-
-> **WARNING: This command line option is deprecated, and superseded by
-> _dom0-iommu=passthrough_ - using both options in combination is undefined.**
-
-> `dom0-strict`
+    Due to implementation choices, sharing pagetables doesn't work on AMD
+    hardware, and this option is ignored.  It is enabled by default on Intel
+    systems.
 
-> **WARNING: This command line option is deprecated, and superseded by
-> _dom0-iommu=strict_ - using both options in combination is undefined.**
+    This option is ignored on ARM, and the pagetables are always shared.
 
-> `amd-iommu-perdev-intremap`
+*   The `intremap` boolean controls the Interrupt Remapping sub-feature, and is
+    active by default on compatible hardware.  On x86 systems, the first
+    generation of IOMMUs only supported DMA remapping, and Interrupt Remapping
+    appeared in the second generation.
 
-> Default: `true`
+*   The `intpost` boolean controls the Posted Interrupt sub-feature.  In
+    combination with APIC acceleration (VT-x APICV, SVM AVIC), the IOMMU can
+    be configured to deliver interrupts from assigned PCI devices directly
+    into the guest, without trapping out into hypervisor context.
 
->> Control whether to set up interrupt remapping data structures per device
->> rather that once for the entire system. Turning this off is making PCI
->> device pass-through insecure and hence unsupported.
+    This option depends on `intremap`, and is disabled by default due to some
+    corner cases in the implementation which have yet to be resolved.
 
-> `workaround_bios_bug` (VT-d)
+The following options are specific to Intel VT-d hardware:
 
-> Default: `false`
+*   The `snoop` boolean controls the Snoop Control sub-feature, and is
+    active by default on compatible hardware.
 
->> Causes DRHD entries without any PCI discoverable devices under them to be
->> ignored (normally IOMMU setup fails if any of the devices listed by a DRHD
->> entry aren't PCI discoverable).
+    An incomming DMA request may specify _Snooped_ (query the CPU caches
+    for the appropriate lines) or _Non-Snooped_ (don't query the CPU
+    caches).  _Non-Snooped_ accesses incur less latency, but
+    behind-the-scenes hypervisor activity can invalidate the
+    expectations of the device driver, and Snoop Control allows the
+    hypervisor to force DMA requests to be _Snooped_ when they would
+    otherwise not be.
 
-> `igfx` (VT-d)
+*   The `qinval` boolean controls the Queued Invalidation sub-feature, and is
+    active by default on compatible hardware.  Queued Invalidation is a
+    feature in second-generation IOMMUs and is a functional prerequisite for
+    Interrupt Remapping.
 
-> Default: `true`
+*   The `igfx` boolean is active by default, and controls whether the
+    IOMMU in front of an Intel Graphics Device is enabled or not.
 
->> Enable IOMMU for Intel graphics devices. The intended usage of this option
->> is `no-igfx`, which is similar to Linux `intel_iommu=igfx_off` option used
->> to workaround graphics issues. If adding `no-igfx` fixes anything, you
->> should file a bug reporting the problem.
+    It is intended as a debugging mechanism for graphics issues, and to
+    be similar to Linux's `intel_iommu=igfx_off` option.  If specifying
+    `no-igfx` fixes anything, please report the problem.
 
-> `verbose`
+*   The `workaround_bios_bug` boolean is disabled by default.  It can be
+    used to ignore errors when parsing the ACPI tables, and finding a
+    listed PCI device which doesn't appear to exist in the system.
 
-> Default: `false`
-
->> Increase IOMMU code's verbosity.
+The following options are specific to AMD-Vi hardware:
 
-> `debug`
+*   The `amd-iommu-perdev-intremap` boolean controls whether the interrupt
+    remapping table is per device (the default), or a single global
+    table for the entire system.
 
-> Default: `false`
+    Using a global table is not security supported as it allows all
+    devices to impersonate each other as far as interrupts as concerned
+    (see XSA-36), but it is a workaround for SP5100 Erratum 28.
 
->> Enable IOMMU debugging code (implies `verbose`).
+**WARNING: The `dom0-passthrough` and `dom0-strict` booleans are both
+deprecated, and superseded by _dom0-iommu={passthrough,strict}_
+respectively - using both the old and new command line options in
+combination is undefined.**
 
 ### iommu_dev_iotlb_timeout
 > `= <integer>`
diff --git a/xen/drivers/passthrough/iommu.c b/xen/drivers/passthrough/iommu.c
index bd1af35..9ac9e05 100644
--- a/xen/drivers/passthrough/iommu.c
+++ b/xen/drivers/passthrough/iommu.c
@@ -21,34 +21,11 @@
 #include <xen/keyhandler.h>
 #include <xsm/xsm.h>
 
-static int parse_iommu_param(const char *s);
 static void iommu_dump_p2m_table(unsigned char key);
 
 unsigned int __read_mostly iommu_dev_iotlb_timeout = 1000;
 integer_param("iommu_dev_iotlb_timeout", iommu_dev_iotlb_timeout);
 
-/*
- * The 'iommu' parameter enables the IOMMU.  Optional comma separated
- * value may contain:
- *
- *   off|no|false|disable       Disable IOMMU (default)
- *   force|required             Don't boot unless IOMMU is enabled
- *   no-intremap                Disable interrupt remapping
- *   intpost                    Enable VT-d Interrupt posting
- *   verbose                    Be more verbose
- *   debug                      Enable debugging messages and checks
- *   workaround_bios_bug        Workaround some bios issue to still enable
- *                              VT-d, don't guarantee security
- *   dom0-passthrough           No DMA translation at all for Dom0
- *   dom0-strict                No 1:1 memory mapping for Dom0
- *   no-sharept                 Don't share VT-d and EPT page tables
- *   no-snoop                   Disable VT-d Snoop Control
- *   no-qinval                  Disable VT-d Queued Invalidation
- *   no-igfx                    Disable VT-d for IGD devices (insecure)
- *   no-amd-iommu-perdev-intremap Don't use per-device interrupt remapping
- *                              tables (insecure)
- */
-custom_param("iommu", parse_iommu_param);
 bool_t __initdata iommu_enable = 1;
 bool_t __read_mostly iommu_enabled;
 bool_t __read_mostly force_iommu;
@@ -84,50 +61,45 @@ static struct tasklet iommu_pt_cleanup_tasklet;
 static int __init parse_iommu_param(const char *s)
 {
     const char *ss;
-    int val, b, rc = 0;
+    int val, rc = 0;
 
     do {
-        val = !!strncmp(s, "no-", 3);
-        if ( !val )
-            s += 3;
-
         ss = strchr(s, ',');
         if ( !ss )
             ss = strchr(s, '\0');
 
-        b = parse_bool(s, ss);
-        if ( b >= 0 )
-            iommu_enable = b;
-        else if ( !cmdline_strcmp(s, "force") ||
-                  !cmdline_strcmp(s, "required") )
+        if ( (val = parse_bool(s, ss)) >= 0 )
+            iommu_enable = val;
+        else if ( (val = parse_boolean("force", s, ss)) >= 0 ||
+                  (val = parse_boolean("required", s, ss)) >= 0 )
             force_iommu = val;
-        else if ( !cmdline_strcmp(s, "workaround_bios_bug") )
+        else if ( (val = parse_boolean("workaround_bios_bug", s, ss)) >= 0 )
             iommu_workaround_bios_bug = val;
-        else if ( !cmdline_strcmp(s, "igfx") )
+        else if ( (val = parse_boolean("igfx", s, ss)) >= 0 )
             iommu_igfx = val;
-        else if ( !cmdline_strcmp(s, "verbose") )
+        else if ( (val = parse_boolean("verbose", s, ss)) >= 0 )
             iommu_verbose = val;
-        else if ( !cmdline_strcmp(s, "snoop") )
+        else if ( (val = parse_boolean("snoop", s, ss)) >= 0 )
             iommu_snoop = val;
-        else if ( !cmdline_strcmp(s, "qinval") )
+        else if ( (val = parse_boolean("qinval", s, ss)) >= 0 )
             iommu_qinval = val;
-        else if ( !cmdline_strcmp(s, "intremap") )
+        else if ( (val = parse_boolean("intremap", s, ss)) >= 0 )
             iommu_intremap = val;
-        else if ( !cmdline_strcmp(s, "intpost") )
+        else if ( (val = parse_boolean("intpost", s, ss)) >= 0 )
             iommu_intpost = val;
-        else if ( !cmdline_strcmp(s, "debug") )
+        else if ( (val = parse_boolean("debug", s, ss)) >= 0 )
         {
             iommu_debug = val;
             if ( val )
                 iommu_verbose = 1;
         }
-        else if ( !cmdline_strcmp(s, "amd-iommu-perdev-intremap") )
+        else if ( (val = parse_boolean("amd-iommu-perdev-intremap", s, ss)) >= 0 )
             amd_iommu_perdev_intremap = val;
-        else if ( !cmdline_strcmp(s, "dom0-passthrough") )
+        else if ( (val = parse_boolean("dom0-passthrough", s, ss)) >= 0 )
             iommu_hwdom_passthrough = val;
-        else if ( !cmdline_strcmp(s, "dom0-strict") )
+        else if ( (val = parse_boolean("dom0-strict", s, ss)) >= 0 )
             iommu_hwdom_strict = val;
-        else if ( !cmdline_strcmp(s, "sharept") )
+        else if ( (val = parse_boolean("sharept", s, ss)) >= 0 )
             iommu_hap_pt_share = val;
         else
             rc = -EINVAL;
@@ -137,6 +109,7 @@ static int __init parse_iommu_param(const char *s)
 
     return rc;
 }
+custom_param("iommu", parse_iommu_param);
 
 static int __init parse_dom0_iommu_param(const char *s)
 {
-- 
2.1.4


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

Re: [PATCH v3 2/7] docs: Improve documentation and parsing for iommu=

[Roger Pau Monné]

On Wed, Jan 16, 2019 at 09:00:45AM +0000, Andrew Cooper wrote:
> Update parse_iommu_param() to uniformly use parse_boolean(), so the sub
> booleans behave like other Xen boolean options.  Reposition the
> custom_param() to avoid a forward declaration of parse_iommu_param().
> 
> Rewrite the command line documentation almost from scratch, including
> far more detail.
> 
> Signed-off-by: Andrew Cooper <andrew.cooper3@citrix.com>

Thanks for doing this:

Reviewed-by: Roger Pau Monné <roger.pau@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: Juergen Gross <jgross@suse.com>
> 
> v3:
>  * New
> ---
>  docs/misc/xen-command-line.pandoc | 153 ++++++++++++++++++++------------------
>  xen/drivers/passthrough/iommu.c   |  63 +++++-----------
>  2 files changed, 99 insertions(+), 117 deletions(-)
> 
> diff --git a/docs/misc/xen-command-line.pandoc b/docs/misc/xen-command-line.pandoc
> index 243193d..ab486e0 100644
> --- a/docs/misc/xen-command-line.pandoc
> +++ b/docs/misc/xen-command-line.pandoc
> @@ -1146,104 +1146,113 @@ detection of systems known to misbehave upon accesses to that port.
>  > Default: `new` unless directed-EOI is supported
>  
>  ### iommu
> -> `= List of [ <boolean> | force | required | intremap | intpost | qinval | snoop | sharept | dom0-passthrough | dom0-strict | amd-iommu-perdev-intremap | workaround_bios_bug | igfx | verbose | debug ]`
> +    = List of [ <bool>, verbose, debug, force, required,

Should this be in the form of "..., verbose=<bool>, debug=<bool>,
..."?

So it's in the same format that's used by the dom0 option
documentation in patch 1.

> +                sharept, intremap, intpost,
> +                snoop, qinval, igfx, workaround_bios_bug,
> +                amd-iommu-perdev-intremap,
> +                dom0-{passthrough,strict} ]
>  
> -> Sub-options:
> +    All sub-options are boolean in nature.

Oh, maybe that's enough and you don't need the =<bool> I suggested
above.

>  
> -> `<boolean>`
> +I/O Memory Memory Units perform a function similar to the CPU MMU (hence the
> +name), but typically exist as a discrete device, integrated as part of a PCI
> +Root Complex.  The most common configuration is to have one IOMMU per package
> +(for on-die PCIe devices and directly attached PCIe lanes), and one IOMMU
> +covering the remaining I/O in the system.
>  
> -> Default: `on`
> -
> ->> Control the use of IOMMU(s) in the system.
> -
> -> All other sub-options are of boolean kind and can be prefixed with `no-` to
> -> effect the inverse meaning.
> -
> -> `force` or `required`
> +The functionality in an IOMMU commonly falls into two orthogonal categories:
>  
> -> Default: `false`
> -
> ->> Don't continue booting unless IOMMU support is found and can be initialized
> ->> successfully.
> +1.  DMA remapping which uses a pagetable-like hierarchical structure and maps
> +    I/O Virtual Addresses (DFNs - Device Frame Numbers in Xen's terminology)
> +    to System Physical Addresses (MFNs - Machine Frame Numbers in Xen's
> +    terminology).
>  
> -> `intremap`
> +2.  Interrupt Remapping, which controls incoming Message Signalled Interrupt
> +    requests, including their routing to specific CPUs.
>  
> -> Default: `true`
> +IOMMU functionality can be used either to provide a translation which the
> +hardware device driver isn't aware of (e.g. PCI Passthrough and a native
> +driver inside the guest) or to enforce fine-grained control over the memory
> +and interrupts which a device is attempting to access.
>  
> ->> Control the use of interrupt remapping (DMA remapping will always be enabled
> ->> if IOMMU functionality is enabled).
> +By default, IOMMUs are configured for use if they are available.  An overall
> +boolean (e.g. `iommu=no`) can override this and leave the IOMMUs disabled.
>  
> -> `intpost`
> +*   The `verbose` and `debug` booleans can be used to print additional
> +    diagnostic information.  Neither are active by default.
>  
> -> Default: `false`
> +*   The `force` and `required` booleans are synonymous and, when requested, will
> +    prevent Xen from booting if IOMMUs aren't discovered and enabled
> +    successfully.
>  
> ->> Control the use of interrupt posting, which depends on the availability of
> ->> interrupt remapping.
> -
> -> `qinval` (VT-d)
> -
> -> Default: `true`
> -
> ->> Control the use of Queued Invalidation.
> -
> -> `snoop` (Intel)
> -
> -> Default: `true`
> +*   The `sharept` boolean controls whether the IOMMU pagetables are shared with
> +    the CPU-side HAP pagetables, or allocated separately.  Sharing reduces the
> +    memory overhead, but doesn't work in combination with CPU-side
> +    pagefault-based features, e.g. dirty VRAM tracking when a PCI device is
> +    assigned.
>  
> ->> Control the use of Snoop Control.
> -
> -> `sharept`
> -
> -> Default: `true`
> -
> ->> Control whether CPU and IOMMU page tables should be shared.
> -
> -> `dom0-passthrough`
> -
> -> **WARNING: This command line option is deprecated, and superseded by
> -> _dom0-iommu=passthrough_ - using both options in combination is undefined.**
> -
> -> `dom0-strict`
> +    Due to implementation choices, sharing pagetables doesn't work on AMD
> +    hardware, and this option is ignored.  It is enabled by default on Intel
                                                                         ^ compatible

Thanks, Roger.

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

Re: [PATCH v3 2/7] docs: Improve documentation and parsing for iommu=

[Andrew Cooper]

On 16/01/2019 11:06, Roger Pau Monné wrote:
> On Wed, Jan 16, 2019 at 09:00:45AM +0000, Andrew Cooper wrote:
>>  
>> ->> Control the use of Snoop Control.
>> -
>> -> `sharept`
>> -
>> -> Default: `true`
>> -
>> ->> Control whether CPU and IOMMU page tables should be shared.
>> -
>> -> `dom0-passthrough`
>> -
>> -> **WARNING: This command line option is deprecated, and superseded by
>> -> _dom0-iommu=passthrough_ - using both options in combination is undefined.**
>> -
>> -> `dom0-strict`
>> +    Due to implementation choices, sharing pagetables doesn't work on AMD
>> +    hardware, and this option is ignored.  It is enabled by default on Intel
>                                                                          ^ compatible

The VT-d driver binds specifically to Intel systems.  I'm not aware of
any non-Intel systems which use VT-d, but we'd need to make code changes
if we find any.

~Andrew

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

Re: [PATCH v3 2/7] docs: Improve documentation and parsing for iommu=

[Jan Beulich]

>>> On 16.01.19 at 10:00, <andrew.cooper3@citrix.com> wrote:
>[...]
> +The functionality in an IOMMU commonly falls into two orthogonal categories:
>  
> -> Default: `false`
> -
> ->> Don't continue booting unless IOMMU support is found and can be initialized
> ->> successfully.
> +1.  DMA remapping which uses a pagetable-like hierarchical structure and maps
> +    I/O Virtual Addresses (DFNs - Device Frame Numbers in Xen's terminology)
> +    to System Physical Addresses (MFNs - Machine Frame Numbers in Xen's
> +    terminology).
>  
> -> `intremap`
> +2.  Interrupt Remapping, which controls incoming Message Signalled Interrupt
> +    requests, including their routing to specific CPUs.
>  
> -> Default: `true`
> +IOMMU functionality can be used either to provide a translation which the

Instead of the "either" here, would it be better to use ...

> +hardware device driver isn't aware of (e.g. PCI Passthrough and a native
> +driver inside the guest) or to enforce fine-grained control over the memory

... "and/or" here?

> +and interrupts which a device is attempting to access.
>[...]
> +The following options are specific to Intel VT-d hardware:
>  
> -> Default: `false`
> +*   The `snoop` boolean controls the Snoop Control sub-feature, and is
> +    active by default on compatible hardware.
>  
> ->> Causes DRHD entries without any PCI discoverable devices under them to be
> ->> ignored (normally IOMMU setup fails if any of the devices listed by a DRHD
> ->> entry aren't PCI discoverable).
> +    An incomming DMA request may specify _Snooped_ (query the CPU caches

incoming ?

Apart from these
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

[PATCH v3 3/7] docs: Improve documentation and parsing for pci=

[Andrew Cooper]

Alter parse_pci_param() to use parse_boolean(), so the sub options
behave like other Xen booleans.

Update the command line documentation for consistency.

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: Juergen Gross <jgross@suse.com>

v3:
 * New
---
 docs/misc/xen-command-line.pandoc |  9 ++++-----
 xen/drivers/passthrough/pci.c     | 20 ++++----------------
 2 files changed, 8 insertions(+), 21 deletions(-)

diff --git a/docs/misc/xen-command-line.pandoc b/docs/misc/xen-command-line.pandoc
index ab486e0..41dec5b 100644
--- a/docs/misc/xen-command-line.pandoc
+++ b/docs/misc/xen-command-line.pandoc
@@ -1502,13 +1502,12 @@ This option is ignored in **pv-shim** mode.
 > Default: `on`
 
 ### pci
-> `= {no-}serr | {no-}perr`
+    = List of [ serr=<bool>, perr=<bool> ]
 
-> Default: Signaling left as set by firmware.
-
-Disable signaling of SERR (system errors) and/or PERR (parity errors)
-on all PCI devices.
+    Default: Signaling left as set by firmware.
 
+Override the firmware settings, and explicitly enable or disable the
+signalling of PCI System and Parity errors.
 
 ### pci-phantom
 > `=[<seg>:]<bus>:<device>,<stride>`
diff --git a/xen/drivers/passthrough/pci.c b/xen/drivers/passthrough/pci.c
index 93c20b9..8108ed5 100644
--- a/xen/drivers/passthrough/pci.c
+++ b/xen/drivers/passthrough/pci.c
@@ -188,37 +188,25 @@ custom_param("pci-phantom", parse_phantom_dev);
 static u16 __read_mostly command_mask;
 static u16 __read_mostly bridge_ctl_mask;
 
-/*
- * The 'pci' parameter controls certain PCI device aspects.
- * Optional comma separated value may contain:
- *
- *   serr                       don't suppress system errors (default)
- *   no-serr                    suppress system errors
- *   perr                       don't suppress parity errors (default)
- *   no-perr                    suppress parity errors
- */
 static int __init parse_pci_param(const char *s)
 {
     const char *ss;
     int rc = 0;
 
     do {
-        bool_t on = !!strncmp(s, "no-", 3);
+        int val;
         u16 cmd_mask = 0, brctl_mask = 0;
 
-        if ( !on )
-            s += 3;
-
         ss = strchr(s, ',');
         if ( !ss )
             ss = strchr(s, '\0');
 
-        if ( !cmdline_strcmp(s, "serr") )
+        if ( (val = parse_boolean("serr", s, ss)) >= 0 )
         {
             cmd_mask = PCI_COMMAND_SERR;
             brctl_mask = PCI_BRIDGE_CTL_SERR | PCI_BRIDGE_CTL_DTMR_SERR;
         }
-        else if ( !cmdline_strcmp(s, "perr") )
+        else if ( (val = parse_boolean("perr", s, ss)) >= 0 )
         {
             cmd_mask = PCI_COMMAND_PARITY;
             brctl_mask = PCI_BRIDGE_CTL_PARITY;
@@ -226,7 +214,7 @@ static int __init parse_pci_param(const char *s)
         else
             rc = -EINVAL;
 
-        if ( on )
+        if ( val )
         {
             command_mask &= ~cmd_mask;
             bridge_ctl_mask &= ~brctl_mask;
-- 
2.1.4


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

Re: [PATCH v3 3/7] docs: Improve documentation and parsing for pci=

[Roger Pau Monné]

On Wed, Jan 16, 2019 at 09:00:46AM +0000, Andrew Cooper wrote:
> Alter parse_pci_param() to use parse_boolean(), so the sub options
> behave like other Xen booleans.
> 
> Update the command line documentation for consistency.
> 
> Signed-off-by: Andrew Cooper <andrew.cooper3@citrix.com>

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 v3 3/7] docs: Improve documentation and parsing for pci=

[Jan Beulich]

>>> On 16.01.19 at 10:00, <andrew.cooper3@citrix.com> wrote:
> Alter parse_pci_param() to use parse_boolean(), so the sub options
> behave like other Xen booleans.
> 
> Update the command line documentation for consistency.
> 
> Signed-off-by: Andrew Cooper <andrew.cooper3@citrix.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

[PATCH v3 4/7] 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.  This is a functional
change, but "no-attr=uc" is ambiguous and shouldn't be accepted.

Update the command line documentation for consistency.

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

v3:
 * New
---
 docs/misc/xen-command-line.pandoc | 22 ++++++++--------------
 xen/common/efi/boot.c             | 11 +++--------
 2 files changed, 11 insertions(+), 22 deletions(-)

diff --git a/docs/misc/xen-command-line.pandoc b/docs/misc/xen-command-line.pandoc
index 41dec5b..4f27e54 100644
--- a/docs/misc/xen-command-line.pandoc
+++ b/docs/misc/xen-command-line.pandoc
@@ -851,23 +851,17 @@ 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=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=uc` string exists as a workaround, to allow mapping memory regions
+    with unknown/unrecognised cacheability as uncacheable, rather than not
+    present.
 
 ### ept
 > `= List of [ ad=<bool>, pml=<bool> ]`
diff --git a/xen/common/efi/boot.c b/xen/common/efi/boot.c
index 1e1a551..597b2ff 100644
--- a/xen/common/efi/boot.c
+++ b/xen/common/efi/boot.c
@@ -1389,19 +1389,14 @@ 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);
@@ -1409,7 +1404,7 @@ static int __init parse_efi_param(const char *s)
                 __clear_bit(EFI_RS, &efi_flags);
         }
         else if ( !cmdline_strcmp(s, "attr=uc") )
-            efi_map_uc = val;
+            efi_map_uc = true;
         else
             rc = -EINVAL;
 
-- 
2.1.4


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

Re: [PATCH v3 4/7] docs: Improve documentation and parsing for efi=

[Jan Beulich]

>>> On 16.01.19 at 10:00, <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.  This is a functional
> change, but "no-attr=uc" is ambiguous and shouldn't be accepted.

"no-attr=uc" is of course rubbish, and should never have been
allowed. However, I'd really like to retain a way to specify things
both ways, i.e. I think "attr=no" (or, less desirable, "attr=no-uc",
as that would get clumsy if we find a need for other cache
attributes to be added here) should be supported.

Jan



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

[PATCH v3 5/7] 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>
CC: Juergen Gross <jgross@suse.com>

This is fine for compatibility purposes, as this option wasn't present
in Xen 4.11

v2:
 * New
v3:
 * Use cmdline_strcmp() rather than introducing yet more buggy strncmp()
---
 docs/misc/xen-command-line.pandoc | 16 +++++++++-------
 xen/arch/x86/dom0_build.c         |  8 +++++---
 2 files changed, 14 insertions(+), 10 deletions(-)

diff --git a/docs/misc/xen-command-line.pandoc b/docs/misc/xen-command-line.pandoc
index 4f27e54..365d2ee 100644
--- a/docs/misc/xen-command-line.pandoc
+++ b/docs/misc/xen-command-line.pandoc
@@ -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>, verbose=<bool> ]
+    = List of [ pv | pvh, shadow=<bool>, verbose=<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 7f6ee7f..2b4d9e9 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);
 bool __initdata dom0_verbose;
 
 static int __init parse_dom0_param(const char *s)
@@ -295,8 +295,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) && !cmdline_strcmp(s, "pv") )
+            dom0_pvh = false;
+        else if ( IS_ENABLED(CONFIG_HVM) && !cmdline_strcmp(s, "pvh") )
+            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

Re: [PATCH v3 5/7] x86/dom0: Improve dom0= useability

[Roger Pau Monné]

On Wed, Jan 16, 2019 at 09:00:48AM +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>

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

> ---
> CC: Jan Beulich <JBeulich@suse.com>
> CC: Wei Liu <wei.liu2@citrix.com>
> CC: Roger Pau Monné <roger.pau@citrix.com>
> CC: Juergen Gross <jgross@suse.com>
> 
> This is fine for compatibility purposes, as this option wasn't present
> in Xen 4.11
> 
> v2:
>  * New
> v3:
>  * Use cmdline_strcmp() rather than introducing yet more buggy strncmp()
> ---
>  docs/misc/xen-command-line.pandoc | 16 +++++++++-------
>  xen/arch/x86/dom0_build.c         |  8 +++++---
>  2 files changed, 14 insertions(+), 10 deletions(-)
> 
> diff --git a/docs/misc/xen-command-line.pandoc b/docs/misc/xen-command-line.pandoc
> index 4f27e54..365d2ee 100644
> --- a/docs/misc/xen-command-line.pandoc
> +++ b/docs/misc/xen-command-line.pandoc
> @@ -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>, verbose=<bool> ]
> +    = List of [ pv | pvh, shadow=<bool>, verbose=<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.

I would maybe add something along the lines of: "Setting both the `pv`
and the `pvh` options in combination is undefined".

Thanks.

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

Re: [PATCH v3 5/7] x86/dom0: Improve dom0= useability

[Jan Beulich]

>>> On 16.01.19 at 10:00, <andrew.cooper3@citrix.com> wrote:
> --- a/docs/misc/xen-command-line.pandoc
> +++ b/docs/misc/xen-command-line.pandoc
> @@ -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>, verbose=<bool> ]
> +    = List of [ pv | pvh, shadow=<bool>, verbose=<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.

Seeing that the CONFIG_* here merely move up from ...

> +    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,

... here, the patch as a whole can have my
Reviewed-by: Jan Beulich <jbeulich@suse.com>

Of course if an overall decision is taken to avoid mention of
CONFIG_* here I'd appreciate if you did respective adjustments
here as well.

> -        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

As an aside - is this restriction still necessary, now that the code
was fixed to allow shadow enabling for L1TF even for Dom0?

Jan



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

Re: [PATCH v3 5/7] x86/dom0: Improve dom0= useability

[Andrew Cooper]

On 16/01/2019 14:08, Jan Beulich wrote:
>
>> -        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
> As an aside - is this restriction still necessary, now that the code
> was fixed to allow shadow enabling for L1TF even for Dom0?

I see your point, but the documentation here currently matches the
behaviour of Xen, therefore shouldn't change.

In some copious free time, it might be nice to upstream my Dom0 L1TF
debugging patch, but that is a separate task.

~Andrew

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

Re: [PATCH v3 5/7] x86/dom0: Improve dom0= useability

[Jan Beulich]

>>> On 16.01.19 at 20:58, <andrew.cooper3@citrix.com> wrote:
> On 16/01/2019 14:08, Jan Beulich wrote:
>>
>>> -        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
>> As an aside - is this restriction still necessary, now that the code
>> was fixed to allow shadow enabling for L1TF even for Dom0?
> 
> I see your point, but the documentation here currently matches the
> behaviour of Xen, therefore shouldn't change.

Oh, of course I didn't mean to change the doc without also
removing the restriction from actual code.

Jan



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

[PATCH v3 6/7] 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: Juergen Gross <jgross@suse.com>
CC: Jun Nakajima <jun.nakajima@intel.com>
CC: Kevin Tian <kevin.tian@intel.com>

v2:
 * New
---
 docs/misc/xen-command-line.pandoc     | 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.pandoc b/docs/misc/xen-command-line.pandoc
index 365d2ee..ccfad4c 100644
--- a/docs/misc/xen-command-line.pandoc
+++ b/docs/misc/xen-command-line.pandoc
@@ -667,8 +667,7 @@ Controls for how dom0 is constructed on x86 systems.
     information during the dom0 build.  It defaults to false.
 
 ### 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.
 
@@ -695,9 +694,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
@@ -711,11 +709,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>`
 
@@ -1259,12 +1252,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 73c8048..e35ae13 100644
--- a/xen/drivers/passthrough/arm/smmu.c
+++ b/xen/drivers/passthrough/arm/smmu.c
@@ -2714,10 +2714,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 9ac9e05..696d189 100644
--- a/xen/drivers/passthrough/iommu.c
+++ b/xen/drivers/passthrough/iommu.c
@@ -38,7 +38,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;
 
 /*
@@ -127,8 +126,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 e40d7a7..9fe2329 100644
--- a/xen/drivers/passthrough/x86/iommu.c
+++ b/xen/drivers/passthrough/x86/iommu.c
@@ -172,10 +172,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;
     }
 
@@ -214,20 +214,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 cdc8021..1bd7326 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

Re: [PATCH v3 6/7] xen/dom0: Drop iommu_hwdom_inclusive entirely

[Roger Pau Monné]

On Wed, Jan 16, 2019 at 09:00:49AM +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>

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

> diff --git a/xen/drivers/passthrough/x86/iommu.c b/xen/drivers/passthrough/x86/iommu.c
> index e40d7a7..9fe2329 100644
> --- a/xen/drivers/passthrough/x86/iommu.c
> +++ b/xen/drivers/passthrough/x86/iommu.c
> @@ -172,10 +172,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;
>      }

AFAICT the logic in the switch above can be simplified if
iommu_hwdom_inclusive is dropped, I will prepare a patch do this as
soon as this is committed, since I don't want to delay this series.

Thanks, Roger.

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

Re: [PATCH v3 6/7] xen/dom0: Drop iommu_hwdom_inclusive entirely

[Jan Beulich]

>>> On 16.01.19 at 10:00, <andrew.cooper3@citrix.com> 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 final part of this sentence is the main culprit, resulting in me
being uncertain about this move. I'm not outright opposed, since
I agree with the "is not sensible" part above. But there surely
was a reason why the option was introduced. May I suggest as
an alternative to take an intermediate step for at least one
release cycle, and attach a warning_add() to any use of this
option?

> --- a/xen/drivers/passthrough/x86/iommu.c
> +++ b/xen/drivers/passthrough/x86/iommu.c
> @@ -172,10 +172,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;
>      }

Removal of this option could be imagined taking an intermediate
step where the variable resolves to constant false. This suggests
to me that the first of these two changes is correct, but the
second is not. Instead the "else if()" ought to collapse to just
"else", which in turn would suggest to consider folding if()/else
into a single if().

Jan



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

Re: [PATCH v3 6/7] xen/dom0: Drop iommu_hwdom_inclusive entirely

[Roger Pau Monné]

On Thu, Jan 17, 2019 at 06:32:01AM -0700, Jan Beulich wrote:
> >>> On 16.01.19 at 10:00, <andrew.cooper3@citrix.com> 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 final part of this sentence is the main culprit, resulting in me
> being uncertain about this move. I'm not outright opposed, since
> I agree with the "is not sensible" part above. But there surely
> was a reason why the option was introduced. May I suggest as
> an alternative to take an intermediate step for at least one
> release cycle, and attach a warning_add() to any use of this
> option?
> 
> > --- a/xen/drivers/passthrough/x86/iommu.c
> > +++ b/xen/drivers/passthrough/x86/iommu.c
> > @@ -172,10 +172,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;
> >      }
> 
> Removal of this option could be imagined taking an intermediate
> step where the variable resolves to constant false. This suggests
> to me that the first of these two changes is correct, but the
> second is not. Instead the "else if()" ought to collapse to just
> "else", which in turn would suggest to consider folding if()/else
> into a single if().

I've also noted that the whole switch can be simplified in a reply to
a previous version:

https://lists.xenproject.org/archives/html/xen-devel/2019-01/msg00217.html

Thanks, Roger.

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

[PATCH v3 7/7] 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>
---
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: Juergen Gross <jgross@suse.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.
---
 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 ccfad4c..410a7f6 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.
     information during the dom0 build.  It defaults to false.
 
 ### 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.
 
@@ -709,6 +709,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/drivers/passthrough/iommu.c b/xen/drivers/passthrough/iommu.c
index 696d189..0e0fe67 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_reserved = -1;
@@ -128,6 +129,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;
 
@@ -156,7 +159,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 v3 7/7] xen/dom0: Add a dom0-iommu=none option

[Roger Pau Monné]

On Wed, Jan 16, 2019 at 09:00:50AM +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.
> 
> Signed-off-by: Andrew Cooper <andrew.cooper3@citrix.com>

Reviewed-by: Roger Pau Monné <roger.pau@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: Juergen Gross <jgross@suse.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.
> ---
>  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 ccfad4c..410a7f6 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.
>      information during the dom0 build.  It defaults to false.
>  
>  ### dom0-iommu
> -    = List of [ passthrough=<bool>, strict=<bool>, map-reserved=<bool> ]
> +    = List of [ passthrough=<bool>, strict=<bool>, map-reserved=<bool>, none ]

I would also be fine with using dom0-iommu = [ <bool>, ... ] but
that's also fine and less confusing.

Thanks, Roger.

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

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

[Wei Liu]

On Wed, Jan 16, 2019 at 09:00:50AM +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.

Nice.

/me takes note to add a corresponding smoke test to gitlab.

Wei.

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

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

[Jan Beulich]

>>> On 16.01.19 at 10:00, <andrew.cooper3@citrix.com> wrote:
> @@ -709,6 +709,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.

Would you mind inserting "PVH" ahead of "dom0"?

> @@ -156,7 +159,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;

Seeing the __hwdom_init, wouldn't it be better to restrict this
relaxation to Xen boot time created Dom0?

Jan



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

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

[Andrew Cooper]

On 17/01/2019 13:35, Jan Beulich wrote:
>>>> On 16.01.19 at 10:00, <andrew.cooper3@citrix.com> wrote:
>> @@ -709,6 +709,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.
> Would you mind inserting "PVH" ahead of "dom0"?

That would result in an inaccurate description of the functionality. 
check_hwdom_reqs() is not specific to PVH guests.

>
>> @@ -156,7 +159,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;
> Seeing the __hwdom_init, wouldn't it be better to restrict this
> relaxation to Xen boot time created Dom0?

No, I don't think so.

That would complicate the change (which is already only for use by
developers), and unnecessarily prohibit testing of the late hwdom paths.

~Andrew

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

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

[Jan Beulich]

>>> On 21.01.19 at 19:08, <andrew.cooper3@citrix.com> wrote:
> On 17/01/2019 13:35, Jan Beulich wrote:
>>>>> On 16.01.19 at 10:00, <andrew.cooper3@citrix.com> wrote:
>>> @@ -709,6 +709,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.
>> Would you mind inserting "PVH" ahead of "dom0"?
> 
> That would result in an inaccurate description of the functionality. 
> check_hwdom_reqs() is not specific to PVH guests.

How is the paging_mode_translate() check your patch actually
amends not making this function effectively PVH-specific? Or
are you meaning to imply that some hypothetical future addition
to the checks affecting PV Dom0 is to be covered here as well?

>>> @@ -156,7 +159,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;
>> Seeing the __hwdom_init, wouldn't it be better to restrict this
>> relaxation to Xen boot time created Dom0?
> 
> No, I don't think so.
> 
> That would complicate the change (which is already only for use by
> developers), and unnecessarily prohibit testing of the late hwdom paths.

Well, I don't fully agree, but okay then. Therefore if the answer
to the second question above is "yes", then
Acked-by: Jan Beulich <jbeulich@suse.com>

Jan



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

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

[Andrew Cooper]

On 22/01/2019 08:06, Jan Beulich wrote:
>>>> On 21.01.19 at 19:08, <andrew.cooper3@citrix.com> wrote:
>> On 17/01/2019 13:35, Jan Beulich wrote:
>>>>>> On 16.01.19 at 10:00, <andrew.cooper3@citrix.com> wrote:
>>>> @@ -709,6 +709,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.
>>> Would you mind inserting "PVH" ahead of "dom0"?
>> That would result in an inaccurate description of the functionality. 
>> check_hwdom_reqs() is not specific to PVH guests.
> How is the paging_mode_translate() check your patch actually
> amends not making this function effectively PVH-specific? Or
> are you meaning to imply that some hypothetical future addition
> to the checks affecting PV Dom0 is to be covered here as well?

I expect it is far more likely that the paging_mode_translate() check be
adjusted in the future, than the caller to be modified to exclude PV guests.

>
>>>> @@ -156,7 +159,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;
>>> Seeing the __hwdom_init, wouldn't it be better to restrict this
>>> relaxation to Xen boot time created Dom0?
>> No, I don't think so.
>>
>> That would complicate the change (which is already only for use by
>> developers), and unnecessarily prohibit testing of the late hwdom paths.
> Well, I don't fully agree, but okay then. Therefore if the answer
> to the second question above is "yes", then
> Acked-by: Jan Beulich <jbeulich@suse.com>

Thanks,

~Andrew

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

Re: [PATCH v3 for-4.12 0/7] Docs improvements, and dom0 construction fixes

[Juergen Gross]

On 16/01/2019 10:00, Andrew Cooper wrote:
> This is logically two series, but they were co-developed and tightly coupled.
> 
> The first 4 patches are improvements to our documentation and command line
> parsing.  It fixes two key issues - first that sub-booleans now all behave
> consistently, and second is the removal of block indentation markup for main
> documentation.  It is excessively verbose and renders poorly to both HTML and
> PDF.
> 
> The second 3 patches make improvements to dom0 parsing and configuration, with
> the intent of making it possible to use a PVH Xen and PVH dom0 XTF test for
> development purposes.
> 
> Patch 5 at a bare minimum is required for 4.12 to fix an issue which hasn't
> been released yet.  All others are nice-to-have with a low risk of problems.
> 
> Major changes from v2:
> 
>   * The cmdline_strcmp() functionality from patch 5 was split out into a
>     separate bugfix for backporting purposes.
> 
> Andrew Cooper (7):
>   docs: Improve documentation for dom0= and dom0-iommu=
>   docs: Improve documentation and parsing for iommu=
>   docs: Improve documentation and parsing for pci=
>   docs: Improve documentation and parsing for efi=
>   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.pandoc     | 319 +++++++++++++++++-----------------
>  xen/arch/x86/dom0_build.c             |  14 +-
>  xen/common/efi/boot.c                 |  11 +-
>  xen/drivers/passthrough/arm/smmu.c    |   4 -
>  xen/drivers/passthrough/iommu.c       |  71 +++-----
>  xen/drivers/passthrough/pci.c         |  20 +--
>  xen/drivers/passthrough/vtd/x86/vtd.c |   6 -
>  xen/drivers/passthrough/x86/iommu.c   |  14 +-
>  xen/include/xen/iommu.h               |   2 +-
>  9 files changed, 197 insertions(+), 264 deletions(-)
> 

For the series: Release-acked-by: Juergen Gross <jgross@suse.com>


Juergen

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