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

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

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

Thanks, Roger.

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

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

2018-08-22 Thread Julien Grall 

Hi,

On 22/08/18 16:22, Roger Pau Monné wrote:

On Wed, Aug 22, 2018 at 04:07:26PM +0100, Julien Grall wrote:

Hi Roger,

On 22/08/18 08:51, Roger Pau Monne wrote:

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

Signed-off-by: Roger Pau Monné 
Reviewed-by: Paul Durrant 
Reviewed-by: Jan Beulich 
Reviewed-by: Kevin Tian 
---
Changes since v4:
   - Use an if to set the default option value.
   - Set the option to false unconditionally on ARM.

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

Changes since v1:
   - Use dom0-iommu instead of the iommu option.
   - Only enable by default on Intel hardware.
---
Cc: Andrew Cooper 
Cc: George Dunlap 
Cc: Ian Jackson 
Cc: Jan Beulich 
Cc: Julien Grall 
Cc: Konrad Rzeszutek Wilk 
Cc: Stefano Stabellini 
Cc: Tim Deegan 
Cc: Wei Liu 
Cc: Suravee Suthikulpanit 
Cc: Brian Woods 
Cc: Kevin Tian 
---
   docs/misc/xen-command-line.markdown | 13 -
   xen/drivers/passthrough/amd/pci_amd_iommu.c |  4 ++
   xen/drivers/passthrough/arm/iommu.c |  4 ++
   xen/drivers/passthrough/arm/smmu.c  |  2 +
   xen/drivers/passthrough/iommu.c | 12 +
   xen/drivers/passthrough/vtd/extern.h|  2 -
   xen/drivers/passthrough/vtd/iommu.c |  8 ++-
   xen/drivers/passthrough/vtd/x86/vtd.c   | 58 +---
   xen/drivers/passthrough/x86/iommu.c | 59 +
   xen/include/xen/iommu.h |  2 +
   10 files changed, 99 insertions(+), 65 deletions(-)

diff --git a/docs/misc/xen-command-line.markdown 
b/docs/misc/xen-command-line.markdown
index cd57960ede..98f0f3b68b 100644
--- a/docs/misc/xen-command-line.markdown
+++ b/docs/misc/xen-command-line.markdown
@@ -682,7 +682,7 @@ Flag that makes a dom0 use shadow paging. Only works when 
"pvh" is
   enabled.
   ### dom0-iommu
-> `= List of [ passthrough | strict ]`
+> `= List of [ passthrough | strict | map-inclusive ]`
   This list of booleans controls the iommu usage by Dom0:
@@ -696,6 +696,14 @@ This list of booleans controls the iommu usage by Dom0:
 `true` for a PVH Dom0 and any attempt to overwrite it from the command line
 is ignored.
+* `map-inclusive`: sets up DMA remapping for all the non-RAM regions below 4GB
+  except for unusable ranges. Use this to work around firmware issues providing
+  incorrect RMRR/IVMD entries. Rather than only mapping RAM pages for IOMMU
+  accesses for Dom0, with this option all pages up to 4GB, not marked as
+  unusable in the E820 table, will get a mapping established. Note that this
+  option is only applicable to a PV Dom0 and is enabled by default on Intel
+  hardware.
+
   ### dom0\_ioports\_disable (x86)
   > `= List of -`
@@ -1233,6 +1241,9 @@ wait descriptor timed out', try increasing this value.
   ### iommu\_inclusive\_mapping (VT-d)
   > `= `
+**WARNING: This command line option is deprecated, and superseded by
+_dom0-iommu=map-inclusive_ - using both options in combination is undefined.**
+
   > Default: `true`
   Use this to work around firmware issues providing incorrect RMRR entries.
diff --git a/xen/drivers/passthrough/amd/pci_amd_iommu.c 
b/xen/drivers/passthrough/amd/pci_amd_iommu.c
index ab39e7500d..27eb49619d 100644
--- a/xen/drivers/passthrough/amd/pci_amd_iommu.c
+++ b/xen/drivers/passthrough/amd/pci_amd_iommu.c
@@ -253,6 +253,10 @@ static void __hwdom_init amd_iommu_hwdom_init(struct 
domain *d)
   unsigned long i;
   const struct amd_iommu *iommu;
+/* Inclusive IOMMU mappings are disabled by default on AMD hardware. */
+if ( iommu_hwdom_inclusive == -1 )
+iommu_hwdom_inclusive = false;


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


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

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


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


Cheers,

--
Julien Grall

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

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

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

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

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

> > +
> >   if ( allocate_domain_resources(dom_iommu(d)) )
> >   BUG();
> > diff --git a/xen/drivers/passthrough/arm/iommu.c 
> > b/xen/drivers/passthrough/arm/iommu.c
> > index 95b1abb972..325997b19f 100644
> > --- a/xen/drivers/passthrough/arm/iommu.c
> >

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

2018-08-22 Thread Julien Grall 

Hi Roger,

On 22/08/18 08:51, Roger Pau Monne wrote:

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

Signed-off-by: Roger Pau Monné 
Reviewed-by: Paul Durrant 
Reviewed-by: Jan Beulich 
Reviewed-by: Kevin Tian 
---
Changes since v4:
  - Use an if to set the default option value.
  - Set the option to false unconditionally on ARM.

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

Changes since v1:
  - Use dom0-iommu instead of the iommu option.
  - Only enable by default on Intel hardware.
---
Cc: Andrew Cooper 
Cc: George Dunlap 
Cc: Ian Jackson 
Cc: Jan Beulich 
Cc: Julien Grall 
Cc: Konrad Rzeszutek Wilk 
Cc: Stefano Stabellini 
Cc: Tim Deegan 
Cc: Wei Liu 
Cc: Suravee Suthikulpanit 
Cc: Brian Woods 
Cc: Kevin Tian 
---
  docs/misc/xen-command-line.markdown | 13 -
  xen/drivers/passthrough/amd/pci_amd_iommu.c |  4 ++
  xen/drivers/passthrough/arm/iommu.c |  4 ++
  xen/drivers/passthrough/arm/smmu.c  |  2 +
  xen/drivers/passthrough/iommu.c | 12 +
  xen/drivers/passthrough/vtd/extern.h|  2 -
  xen/drivers/passthrough/vtd/iommu.c |  8 ++-
  xen/drivers/passthrough/vtd/x86/vtd.c   | 58 +---
  xen/drivers/passthrough/x86/iommu.c | 59 +
  xen/include/xen/iommu.h |  2 +
  10 files changed, 99 insertions(+), 65 deletions(-)

diff --git a/docs/misc/xen-command-line.markdown 
b/docs/misc/xen-command-line.markdown
index cd57960ede..98f0f3b68b 100644
--- a/docs/misc/xen-command-line.markdown
+++ b/docs/misc/xen-command-line.markdown
@@ -682,7 +682,7 @@ Flag that makes a dom0 use shadow paging. Only works when 
"pvh" is
  enabled.
  
  ### dom0-iommu

-> `= List of [ passthrough | strict ]`
+> `= List of [ passthrough | strict | map-inclusive ]`
  
  This list of booleans controls the iommu usage by Dom0:
  
@@ -696,6 +696,14 @@ This list of booleans controls the iommu usage by Dom0:

`true` for a PVH Dom0 and any attempt to overwrite it from the command line
is ignored.
  
+* `map-inclusive`: sets up DMA remapping for all the non-RAM regions below 4GB

+  except for unusable ranges. Use this to work around firmware issues providing
+  incorrect RMRR/IVMD entries. Rather than only mapping RAM pages for IOMMU
+  accesses for Dom0, with this option all pages up to 4GB, not marked as
+  unusable in the E820 table, will get a mapping established. Note that this
+  option is only applicable to a PV Dom0 and is enabled by default on Intel
+  hardware.
+
  ### dom0\_ioports\_disable (x86)
  > `= List of -`
  
@@ -1233,6 +1241,9 @@ wait descriptor timed out', try increasing this value.

  ### iommu\_inclusive\_mapping (VT-d)
  > `= `
  
+**WARNING: This command line option is deprecated, and superseded by

+_dom0-iommu=map-inclusive_ - using both options in combination is undefined.**
+
  > Default: `true`
  
  Use this to work around firmware issues providing incorrect RMRR entries.

diff --git a/xen/drivers/passthrough/amd/pci_amd_iommu.c 
b/xen/drivers/passthrough/amd/pci_amd_iommu.c
index ab39e7500d..27eb49619d 100644
--- a/xen/drivers/passthrough/amd/pci_amd_iommu.c
+++ b/xen/drivers/passthrough/amd/pci_amd_iommu.c
@@ -253,6 +253,10 @@ static void __hwdom_init amd_iommu_hwdom_init(struct 
domain *d)
  unsigned long i;
  const struct amd_iommu *iommu;
  
+/* Inclusive IOMMU mappings are disabled by default on AMD hardware. */

+if ( iommu_hwdom_inclusive == -1 )
+iommu_hwdom_inclusive = false;


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



+
  if ( allocate_domain_resources(dom_iommu(d)) )
  BUG();
  
diff --git a/xen/drivers/passthrough/arm/iommu.c b/xen/drivers/passthrough/arm/iommu.c

index 95b1abb972..325997b19f 100644
--- a/xen/drivers/passthrough/arm/iommu.c
+++ b/xen/drivers/passthrough/arm/iommu.c
@@ -73,3 +73,7 @@ int arch_iommu_populate_page_table(struct domain *d)
  /* The IOMMU shares the p2m with the CPU */
  return -ENOSYS;
  }
+
+void __hwdom_init arch_iommu_hwdom_init(struct domain *d)
+{
+}
diff --git a/xen/drivers/passthrough/arm/smmu.c 
b/xen/drivers/passthrough/arm/smmu.c
index 74c09b0991..b142677b8c 100644
--- a/xen/drivers/passthrough/arm/smmu.c
+++ b/xen/drivers/passthrough/arm/smmu.c
@@ -2727,6 +2727,8 @@ static int arm_smmu_iommu_domain_init(struct domain *d)
  
  static void __hwdom_init arm_smmu_iommu_hwdom_init(struct domain *d)

  {
+   /* Set to false options not supported on ARM. */
+   iommu_hwdom_inclusive = false;


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

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

2018-08-22 Thread Wei Liu 
On Wed, Aug 22, 2018 at 09:51:57AM +0200, Roger Pau Monne wrote:
> Introduce a new dom0-iommu=map-inclusive generic option that
> supersedes iommu_inclusive_mapping. The previous behavior is preserved
> and the option should only be enabled by default on Intel hardware.
> 
> Signed-off-by: Roger Pau Monné 
> Reviewed-by: Paul Durrant 
> Reviewed-by: Jan Beulich 
> Reviewed-by: Kevin Tian 

Reviewed-by: Wei Liu 

With one nit below:

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

Missing a comma after "false".

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

Wei.

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