[PATCH v3 0/2] VT-d: make dom0-strict work with buggy firmware

[PATCH v3 0/2] VT-d: make dom0-strict work with buggy firmware

[Paul Durrant]

When dom0-strict mode is enabled the iommu_inclusive_mapping workaround
for firmware with undeclared RMRRs is rendered useless. This series fixes
the problem.

Paul Durrant (2):
  VT-d: re-phrase logic in vtd_set_hwdom_mapping() for clarity
  VT-d: reconcile iommu_inclusive_mapping and iommu=dom0-strict

 docs/misc/xen-command-line.markdown   |  6 +++--
 xen/drivers/passthrough/vtd/iommu.c   |  2 +-
 xen/drivers/passthrough/vtd/x86/vtd.c | 42 ++++++++++++++++++++++-------------
 xen/include/xen/iommu.h               |  2 +-
 4 files changed, 33 insertions(+), 19 deletions(-)

-- 
2.11.0


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

[PATCH v3 1/2] VT-d: re-phrase logic in vtd_set_hwdom_mapping() for clarity

[Paul Durrant]

It is hard to reconcile the comment at the top of the loop in
vtd_set_hwdom_mapping() with the if statement following it. This patch
re-phrases the logic, preserving the semantics, but making it easier
to read.

The patch also modifies the Xen command line documentation to make it
clear that iommu_inclusive_mapping only applies to pages up to the 4GB
boundary.

NOTE: This patch also corrects the indentation of the printk() towards
      the end of vtd_set_hwdom_mapping().

Signed-off-by: Paul Durrant <paul.durrant@citrix.com>
Reviewed-by: Roger Pau Monne <roger.pau@citrix.com>
---
Cc: Andrew Cooper <andrew.cooper3@citrix.com>
Cc: George Dunlap <George.Dunlap@eu.citrix.com>
Cc: Ian Jackson <ian.jackson@eu.citrix.com>
Cc: Jan Beulich <jbeulich@suse.com>
Cc: Julien Grall <julien.grall@arm.com>
Cc: Konrad Rzeszutek Wilk <konrad.wilk@oracle.com>
Cc: Stefano Stabellini <sstabellini@kernel.org>
Cc: Tim Deegan <tim@xen.org>
Cc: Wei Liu <wei.liu2@citrix.com>
Cc: Kevin Tian <kevin.tian@intel.com>

v3:
 - Fix top calculation by introducing max_pfn.
 - Move comment.

v2:
 - Compare against GB(4) rather than 0xffffffff.
---
 docs/misc/xen-command-line.markdown   |  4 ++--
 xen/drivers/passthrough/vtd/x86/vtd.c | 34 +++++++++++++++++++---------------
 2 files changed, 21 insertions(+), 17 deletions(-)

diff --git a/docs/misc/xen-command-line.markdown b/docs/misc/xen-command-line.markdown
index 8712a833a2..b75471b51a 100644
--- a/docs/misc/xen-command-line.markdown
+++ b/docs/misc/xen-command-line.markdown
@@ -1212,8 +1212,8 @@ wait descriptor timed out', try increasing this value.
 
 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 not marked as unusable in the E820 table will get a mapping
-established.
+option all pages up to 4GB, not marked as unusable in the E820 table, will
+get a mapping established.
 
 ### irq\_ratelimit (x86)
 > `= <integer>`
diff --git a/xen/drivers/passthrough/vtd/x86/vtd.c b/xen/drivers/passthrough/vtd/x86/vtd.c
index 88a60b3307..6551f01e31 100644
--- a/xen/drivers/passthrough/vtd/x86/vtd.c
+++ b/xen/drivers/passthrough/vtd/x86/vtd.c
@@ -110,30 +110,34 @@ void hvm_dpci_isairq_eoi(struct domain *d, unsigned int isairq)
 
 void __hwdom_init vtd_set_hwdom_mapping(struct domain *d)
 {
-    unsigned long i, j, tmp, top;
+    unsigned long i, j, tmp, top, max_pfn;
 
     BUG_ON(!is_hardware_domain(d));
 
-    top = max(max_pdx, pfn_to_pdx(0xffffffffUL >> PAGE_SHIFT) + 1);
+    max_pfn = (GB(4) >> PAGE_SHIFT) - 1;
+    top = max(max_pdx, pfn_to_pdx(max_pfn) + 1);
 
     for ( i = 0; i < top; i++ )
     {
+        unsigned long pfn = pdx_to_pfn(i);
+        bool map;
         int rc = 0;
 
         /*
-         * Set up 1:1 mapping for dom0. Default to use only conventional RAM
-         * areas and let RMRRs include needed reserved regions. When set, the
-         * inclusive mapping maps in everything below 4GB except unusable
-         * ranges.
+         * Set up 1:1 mapping for dom0. Default to include only
+         * conventional RAM areas and let RMRRs include needed reserved
+         * regions. When set, the inclusive mapping maps in every pfn up
+         * to 4GB except those that fall in unusable ranges.
          */
-        unsigned long pfn = pdx_to_pfn(i);
+        if ( pfn > max_pfn && !mfn_valid(_mfn(pfn)) )
+            continue;
+
+        if ( iommu_inclusive_mapping && pfn <= max_pfn )
+            map = !page_is_ram_type(pfn, RAM_TYPE_UNUSABLE);
+        else
+            map = page_is_ram_type(pfn, RAM_TYPE_CONVENTIONAL);
 
-        if ( pfn > (0xffffffffUL >> PAGE_SHIFT) ?
-             (!mfn_valid(_mfn(pfn)) ||
-              !page_is_ram_type(pfn, RAM_TYPE_CONVENTIONAL)) :
-             iommu_inclusive_mapping ?
-             page_is_ram_type(pfn, RAM_TYPE_UNUSABLE) :
-             !page_is_ram_type(pfn, RAM_TYPE_CONVENTIONAL) )
+        if ( !map )
             continue;
 
         /* Exclude Xen bits */
@@ -151,8 +155,8 @@ void __hwdom_init vtd_set_hwdom_mapping(struct domain *d)
         }
 
         if ( rc )
-           printk(XENLOG_WARNING VTDPREFIX " d%d: IOMMU mapping failed: %d\n",
-                  d->domain_id, rc);
+            printk(XENLOG_WARNING VTDPREFIX " d%d: IOMMU mapping failed: %d\n",
+                   d->domain_id, rc);
 
         if (!(i & (0xfffff >> (PAGE_SHIFT - PAGE_SHIFT_4K))))
             process_pending_softirqs();
-- 
2.11.0


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

[PATCH v3 2/2] VT-d: reconcile iommu_inclusive_mapping and iommu=dom0-strict

[Paul Durrant]

The documentation for the iommu_inclusive_mapping Xen command line option
states:

"Use this to work around firmware issues providing incorrect RMRR entries"

Unfortunately this workaround does not function correctly if the dom0-strict
iommu option is also specified.

The documentation goes on to say:

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

This patch modifies the VT-d hardware domain initialization code such that
the workaround will continue to function in dom0-strict mode, by mapping
all pages not marked as unusable *unless* they are RAM pages not assigned
to dom0.

NOTE: This patch modifies the test in drivers/passthrough/vtd/iommu.c from
      need_iommu() to is_pv_domain() since dom0-strict implies need_iommu()
      so we no longer want to gate invocation od vtd_set_hwdom_mapping()
      on that.
      It also exports the iommu_dom0_strict flag so that the implementation
      of vtd_set_hwdom_mapping() can test it explicitly. It would be
      possible to test need_iommu() instead, but it is more illustrative
      to test the original flag rather than one of its side-effects.

Signed-off-by: Paul Durrant <paul.durrant@citrix.com>
Reviewed-by: Roger Pau Monne <roger.pau@citrix.com>
---
Cc: Andrew Cooper <andrew.cooper3@citrix.com>
Cc: George Dunlap <George.Dunlap@eu.citrix.com>
Cc: Ian Jackson <ian.jackson@eu.citrix.com>
Cc: Jan Beulich <jbeulich@suse.com>
Cc: Julien Grall <julien.grall@arm.com>
Cc: Konrad Rzeszutek Wilk <konrad.wilk@oracle.com>
Cc: Stefano Stabellini <sstabellini@kernel.org>
Cc: Tim Deegan <tim@xen.org>
Cc: Wei Liu <wei.liu2@citrix.com>
Cc: Kevin Tian <kevin.tian@intel.com>

v3:
 - Re-word the xen-command-line.markdown modification slightly.
 - Add a note to the commit comment to better explain some of the hunks.

v2:
 - Make sure that the initial mapping only applies to PV dom0.
---
 docs/misc/xen-command-line.markdown   | 4 +++-
 xen/drivers/passthrough/vtd/iommu.c   | 2 +-
 xen/drivers/passthrough/vtd/x86/vtd.c | 8 ++++++++
 xen/include/xen/iommu.h               | 2 +-
 4 files changed, 13 insertions(+), 3 deletions(-)

diff --git a/docs/misc/xen-command-line.markdown b/docs/misc/xen-command-line.markdown
index b75471b51a..d58bdcb162 100644
--- a/docs/misc/xen-command-line.markdown
+++ b/docs/misc/xen-command-line.markdown
@@ -1213,7 +1213,9 @@ wait descriptor timed out', try increasing this value.
 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.
+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/drivers/passthrough/vtd/iommu.c b/xen/drivers/passthrough/vtd/iommu.c
index 08bce92d40..1710256823 100644
--- a/xen/drivers/passthrough/vtd/iommu.c
+++ b/xen/drivers/passthrough/vtd/iommu.c
@@ -1304,7 +1304,7 @@ static void __hwdom_init intel_iommu_hwdom_init(struct domain *d)
 {
     struct acpi_drhd_unit *drhd;
 
-    if ( !iommu_passthrough && !need_iommu(d) )
+    if ( !iommu_passthrough && is_pv_domain(d) )
     {
         /* Set up 1:1 page table for hardware domain. */
         vtd_set_hwdom_mapping(d);
diff --git a/xen/drivers/passthrough/vtd/x86/vtd.c b/xen/drivers/passthrough/vtd/x86/vtd.c
index 6551f01e31..37316b0900 100644
--- a/xen/drivers/passthrough/vtd/x86/vtd.c
+++ b/xen/drivers/passthrough/vtd/x86/vtd.c
@@ -144,6 +144,14 @@ void __hwdom_init vtd_set_hwdom_mapping(struct domain *d)
         if ( xen_in_range(pfn) )
             continue;
 
+        /*
+         * If dom0-strict mode is enabled then exclude conventional RAM
+         * and let the common code map dom0's pages.
+         */
+        if ( iommu_dom0_strict &&
+             page_is_ram_type(pfn, RAM_TYPE_CONVENTIONAL) )
+            continue;
+
         tmp = 1 << (PAGE_SHIFT - PAGE_SHIFT_4K);
         for ( j = 0; j < tmp; j++ )
         {
diff --git a/xen/include/xen/iommu.h b/xen/include/xen/iommu.h
index 33c8b221dc..6b42e3b876 100644
--- a/xen/include/xen/iommu.h
+++ b/xen/include/xen/iommu.h
@@ -29,7 +29,7 @@
 #include <asm/iommu.h>
 
 extern bool_t iommu_enable, iommu_enabled;
-extern bool_t force_iommu, iommu_verbose;
+extern bool_t force_iommu, iommu_dom0_strict, iommu_verbose;
 extern bool_t iommu_workaround_bios_bug, iommu_igfx, iommu_passthrough;
 extern bool_t iommu_snoop, iommu_qinval, iommu_intremap, iommu_intpost;
 extern bool_t iommu_hap_pt_share;
-- 
2.11.0


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

Re: [PATCH v3 1/2] VT-d: re-phrase logic in vtd_set_hwdom_mapping() for clarity

[Tian, Kevin]

> From: Paul Durrant [mailto:paul.durrant@citrix.com]
> Sent: Saturday, June 16, 2018 12:31 AM
> 
> It is hard to reconcile the comment at the top of the loop in
> vtd_set_hwdom_mapping() with the if statement following it. This patch
> re-phrases the logic, preserving the semantics, but making it easier
> to read.
> 
> The patch also modifies the Xen command line documentation to make it
> clear that iommu_inclusive_mapping only applies to pages up to the 4GB
> boundary.
> 
> NOTE: This patch also corrects the indentation of the printk() towards
>       the end of vtd_set_hwdom_mapping().
> 
> Signed-off-by: Paul Durrant <paul.durrant@citrix.com>
> Reviewed-by: Roger Pau Monne <roger.pau@citrix.com>

Acked-by: Kevin Tian <kevin.tian@intel.com>, with one small comment
below

> ---
> Cc: Andrew Cooper <andrew.cooper3@citrix.com>
> Cc: George Dunlap <George.Dunlap@eu.citrix.com>
> Cc: Ian Jackson <ian.jackson@eu.citrix.com>
> Cc: Jan Beulich <jbeulich@suse.com>
> Cc: Julien Grall <julien.grall@arm.com>
> Cc: Konrad Rzeszutek Wilk <konrad.wilk@oracle.com>
> Cc: Stefano Stabellini <sstabellini@kernel.org>
> Cc: Tim Deegan <tim@xen.org>
> Cc: Wei Liu <wei.liu2@citrix.com>
> Cc: Kevin Tian <kevin.tian@intel.com>
> 
> v3:
>  - Fix top calculation by introducing max_pfn.
>  - Move comment.
> 
> v2:
>  - Compare against GB(4) rather than 0xffffffff.
> ---
>  docs/misc/xen-command-line.markdown   |  4 ++--
>  xen/drivers/passthrough/vtd/x86/vtd.c | 34 +++++++++++++++++++---------
> ------
>  2 files changed, 21 insertions(+), 17 deletions(-)
> 
> diff --git a/docs/misc/xen-command-line.markdown b/docs/misc/xen-
> command-line.markdown
> index 8712a833a2..b75471b51a 100644
> --- a/docs/misc/xen-command-line.markdown
> +++ b/docs/misc/xen-command-line.markdown
> @@ -1212,8 +1212,8 @@ wait descriptor timed out', try increasing this
> value.
> 
>  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 not marked as unusable in the E820 table will get a
> mapping
> -established.
> +option all pages up to 4GB, not marked as unusable in the E820 table, will
> +get a mapping established.
> 
>  ### irq\_ratelimit (x86)
>  > `= <integer>`
> diff --git a/xen/drivers/passthrough/vtd/x86/vtd.c
> b/xen/drivers/passthrough/vtd/x86/vtd.c
> index 88a60b3307..6551f01e31 100644
> --- a/xen/drivers/passthrough/vtd/x86/vtd.c
> +++ b/xen/drivers/passthrough/vtd/x86/vtd.c
> @@ -110,30 +110,34 @@ void hvm_dpci_isairq_eoi(struct domain *d,
> unsigned int isairq)
> 
>  void __hwdom_init vtd_set_hwdom_mapping(struct domain *d)
>  {
> -    unsigned long i, j, tmp, top;
> +    unsigned long i, j, tmp, top, max_pfn;
> 
>      BUG_ON(!is_hardware_domain(d));
> 
> -    top = max(max_pdx, pfn_to_pdx(0xffffffffUL >> PAGE_SHIFT) + 1);
> +    max_pfn = (GB(4) >> PAGE_SHIFT) - 1;
> +    top = max(max_pdx, pfn_to_pdx(max_pfn) + 1);
> 
>      for ( i = 0; i < top; i++ )
>      {
> +        unsigned long pfn = pdx_to_pfn(i);
> +        bool map;
>          int rc = 0;
> 
>          /*
> -         * Set up 1:1 mapping for dom0. Default to use only conventional RAM
> -         * areas and let RMRRs include needed reserved regions. When set,
> the
> -         * inclusive mapping maps in everything below 4GB except unusable
> -         * ranges.
> +         * Set up 1:1 mapping for dom0. Default to include only
> +         * conventional RAM areas and let RMRRs include needed reserved
> +         * regions. When set, the inclusive mapping maps in every pfn up
> +         * to 4GB except those that fall in unusable ranges.
>           */

I'm not sure whether others feel same as me. I read the last sentence
as if 'inclusive mapping' ONLY maps <4GB frames while leaving >=4GB
frames unmapped. If it is the case, adding a 'further' i.e. "the inclusive
mapping further maps" sounds more clear?

> -        unsigned long pfn = pdx_to_pfn(i);
> +        if ( pfn > max_pfn && !mfn_valid(_mfn(pfn)) )
> +            continue;
> +
> +        if ( iommu_inclusive_mapping && pfn <= max_pfn )
> +            map = !page_is_ram_type(pfn, RAM_TYPE_UNUSABLE);
> +        else
> +            map = page_is_ram_type(pfn, RAM_TYPE_CONVENTIONAL);
> 
> -        if ( pfn > (0xffffffffUL >> PAGE_SHIFT) ?
> -             (!mfn_valid(_mfn(pfn)) ||
> -              !page_is_ram_type(pfn, RAM_TYPE_CONVENTIONAL)) :
> -             iommu_inclusive_mapping ?
> -             page_is_ram_type(pfn, RAM_TYPE_UNUSABLE) :
> -             !page_is_ram_type(pfn, RAM_TYPE_CONVENTIONAL) )
> +        if ( !map )
>              continue;
> 
>          /* Exclude Xen bits */
> @@ -151,8 +155,8 @@ void __hwdom_init
> vtd_set_hwdom_mapping(struct domain *d)
>          }
> 
>          if ( rc )
> -           printk(XENLOG_WARNING VTDPREFIX " d%d: IOMMU mapping
> failed: %d\n",
> -                  d->domain_id, rc);
> +            printk(XENLOG_WARNING VTDPREFIX " d%d: IOMMU mapping
> failed: %d\n",
> +                   d->domain_id, rc);
> 
>          if (!(i & (0xfffff >> (PAGE_SHIFT - PAGE_SHIFT_4K))))
>              process_pending_softirqs();
> --
> 2.11.0


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

Re: [PATCH v3 2/2] VT-d: reconcile iommu_inclusive_mapping and iommu=dom0-strict

[Tian, Kevin]

> From: Paul Durrant [mailto:paul.durrant@citrix.com]
> Sent: Saturday, June 16, 2018 12:31 AM
> 
> The documentation for the iommu_inclusive_mapping Xen command line
> option
> states:
> 
> "Use this to work around firmware issues providing incorrect RMRR
> entries"
> 
> Unfortunately this workaround does not function correctly if the dom0-
> strict
> iommu option is also specified.
> 
> The documentation goes on to say:
> 
> "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."
> 
> This patch modifies the VT-d hardware domain initialization code such that
> the workaround will continue to function in dom0-strict mode, by mapping
> all pages not marked as unusable *unless* they are RAM pages not
> assigned
> to dom0.
> 
> NOTE: This patch modifies the test in drivers/passthrough/vtd/iommu.c
> from
>       need_iommu() to is_pv_domain() since dom0-strict implies
> need_iommu()
>       so we no longer want to gate invocation od vtd_set_hwdom_mapping()

od->of

>       on that.
>       It also exports the iommu_dom0_strict flag so that the implementation
>       of vtd_set_hwdom_mapping() can test it explicitly. It would be
>       possible to test need_iommu() instead, but it is more illustrative
>       to test the original flag rather than one of its side-effects.
> 
> Signed-off-by: Paul Durrant <paul.durrant@citrix.com>
> Reviewed-by: Roger Pau Monne <roger.pau@citrix.com>

Acked-by: Kevin Tian <kevin.tian@intel.com>

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

Re: [PATCH v3 1/2] VT-d: re-phrase logic in vtd_set_hwdom_mapping() for clarity

[Paul Durrant]

> -----Original Message-----
> From: Tian, Kevin [mailto:kevin.tian@intel.com]
> Sent: 21 June 2018 08:17
> To: Paul Durrant <Paul.Durrant@citrix.com>; xen-devel@lists.xenproject.org
> Cc: Andrew Cooper <Andrew.Cooper3@citrix.com>; George Dunlap
> <George.Dunlap@citrix.com>; Ian Jackson <Ian.Jackson@citrix.com>; Jan
> Beulich <jbeulich@suse.com>; Julien Grall <julien.grall@arm.com>; Konrad
> Rzeszutek Wilk <konrad.wilk@oracle.com>; Stefano Stabellini
> <sstabellini@kernel.org>; Tim (Xen.org) <tim@xen.org>; Wei Liu
> <wei.liu2@citrix.com>
> Subject: RE: [PATCH v3 1/2] VT-d: re-phrase logic in
> vtd_set_hwdom_mapping() for clarity
> 
> > From: Paul Durrant [mailto:paul.durrant@citrix.com]
> > Sent: Saturday, June 16, 2018 12:31 AM
> >
> > It is hard to reconcile the comment at the top of the loop in
> > vtd_set_hwdom_mapping() with the if statement following it. This patch
> > re-phrases the logic, preserving the semantics, but making it easier
> > to read.
> >
> > The patch also modifies the Xen command line documentation to make it
> > clear that iommu_inclusive_mapping only applies to pages up to the 4GB
> > boundary.
> >
> > NOTE: This patch also corrects the indentation of the printk() towards
> >       the end of vtd_set_hwdom_mapping().
> >
> > Signed-off-by: Paul Durrant <paul.durrant@citrix.com>
> > Reviewed-by: Roger Pau Monne <roger.pau@citrix.com>
> 
> Acked-by: Kevin Tian <kevin.tian@intel.com>, with one small comment
> below
> 
> > ---
> > Cc: Andrew Cooper <andrew.cooper3@citrix.com>
> > Cc: George Dunlap <George.Dunlap@eu.citrix.com>
> > Cc: Ian Jackson <ian.jackson@eu.citrix.com>
> > Cc: Jan Beulich <jbeulich@suse.com>
> > Cc: Julien Grall <julien.grall@arm.com>
> > Cc: Konrad Rzeszutek Wilk <konrad.wilk@oracle.com>
> > Cc: Stefano Stabellini <sstabellini@kernel.org>
> > Cc: Tim Deegan <tim@xen.org>
> > Cc: Wei Liu <wei.liu2@citrix.com>
> > Cc: Kevin Tian <kevin.tian@intel.com>
> >
> > v3:
> >  - Fix top calculation by introducing max_pfn.
> >  - Move comment.
> >
> > v2:
> >  - Compare against GB(4) rather than 0xffffffff.
> > ---
> >  docs/misc/xen-command-line.markdown   |  4 ++--
> >  xen/drivers/passthrough/vtd/x86/vtd.c | 34 +++++++++++++++++++------
> ---
> > ------
> >  2 files changed, 21 insertions(+), 17 deletions(-)
> >
> > diff --git a/docs/misc/xen-command-line.markdown b/docs/misc/xen-
> > command-line.markdown
> > index 8712a833a2..b75471b51a 100644
> > --- a/docs/misc/xen-command-line.markdown
> > +++ b/docs/misc/xen-command-line.markdown
> > @@ -1212,8 +1212,8 @@ wait descriptor timed out', try increasing this
> > value.
> >
> >  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 not marked as unusable in the E820 table will get a
> > mapping
> > -established.
> > +option all pages up to 4GB, not marked as unusable in the E820 table, will
> > +get a mapping established.
> >
> >  ### irq\_ratelimit (x86)
> >  > `= <integer>`
> > diff --git a/xen/drivers/passthrough/vtd/x86/vtd.c
> > b/xen/drivers/passthrough/vtd/x86/vtd.c
> > index 88a60b3307..6551f01e31 100644
> > --- a/xen/drivers/passthrough/vtd/x86/vtd.c
> > +++ b/xen/drivers/passthrough/vtd/x86/vtd.c
> > @@ -110,30 +110,34 @@ void hvm_dpci_isairq_eoi(struct domain *d,
> > unsigned int isairq)
> >
> >  void __hwdom_init vtd_set_hwdom_mapping(struct domain *d)
> >  {
> > -    unsigned long i, j, tmp, top;
> > +    unsigned long i, j, tmp, top, max_pfn;
> >
> >      BUG_ON(!is_hardware_domain(d));
> >
> > -    top = max(max_pdx, pfn_to_pdx(0xffffffffUL >> PAGE_SHIFT) + 1);
> > +    max_pfn = (GB(4) >> PAGE_SHIFT) - 1;
> > +    top = max(max_pdx, pfn_to_pdx(max_pfn) + 1);
> >
> >      for ( i = 0; i < top; i++ )
> >      {
> > +        unsigned long pfn = pdx_to_pfn(i);
> > +        bool map;
> >          int rc = 0;
> >
> >          /*
> > -         * Set up 1:1 mapping for dom0. Default to use only conventional RAM
> > -         * areas and let RMRRs include needed reserved regions. When set,
> > the
> > -         * inclusive mapping maps in everything below 4GB except unusable
> > -         * ranges.
> > +         * Set up 1:1 mapping for dom0. Default to include only
> > +         * conventional RAM areas and let RMRRs include needed reserved
> > +         * regions. When set, the inclusive mapping maps in every pfn up
> > +         * to 4GB except those that fall in unusable ranges.
> >           */
> 
> I'm not sure whether others feel same as me. I read the last sentence
> as if 'inclusive mapping' ONLY maps <4GB frames while leaving >=4GB
> frames unmapped. If it is the case, adding a 'further' i.e. "the inclusive
> mapping further maps" sounds more clear?

Agreed. Although I'd probably go for 'additionally' rather than 'further'. Hopefully this is something that can be fixed up at commit time.

  Paul

> 
> > -        unsigned long pfn = pdx_to_pfn(i);
> > +        if ( pfn > max_pfn && !mfn_valid(_mfn(pfn)) )
> > +            continue;
> > +
> > +        if ( iommu_inclusive_mapping && pfn <= max_pfn )
> > +            map = !page_is_ram_type(pfn, RAM_TYPE_UNUSABLE);
> > +        else
> > +            map = page_is_ram_type(pfn, RAM_TYPE_CONVENTIONAL);
> >
> > -        if ( pfn > (0xffffffffUL >> PAGE_SHIFT) ?
> > -             (!mfn_valid(_mfn(pfn)) ||
> > -              !page_is_ram_type(pfn, RAM_TYPE_CONVENTIONAL)) :
> > -             iommu_inclusive_mapping ?
> > -             page_is_ram_type(pfn, RAM_TYPE_UNUSABLE) :
> > -             !page_is_ram_type(pfn, RAM_TYPE_CONVENTIONAL) )
> > +        if ( !map )
> >              continue;
> >
> >          /* Exclude Xen bits */
> > @@ -151,8 +155,8 @@ void __hwdom_init
> > vtd_set_hwdom_mapping(struct domain *d)
> >          }
> >
> >          if ( rc )
> > -           printk(XENLOG_WARNING VTDPREFIX " d%d: IOMMU mapping
> > failed: %d\n",
> > -                  d->domain_id, rc);
> > +            printk(XENLOG_WARNING VTDPREFIX " d%d: IOMMU mapping
> > failed: %d\n",
> > +                   d->domain_id, rc);
> >
> >          if (!(i & (0xfffff >> (PAGE_SHIFT - PAGE_SHIFT_4K))))
> >              process_pending_softirqs();
> > --
> > 2.11.0


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

Re: [PATCH v3 2/2] VT-d: reconcile iommu_inclusive_mapping and iommu=dom0-strict

[Paul Durrant]

> -----Original Message-----
> From: Tian, Kevin [mailto:kevin.tian@intel.com]
> Sent: 21 June 2018 08:21
> To: Paul Durrant <Paul.Durrant@citrix.com>; xen-devel@lists.xenproject.org
> Cc: Andrew Cooper <Andrew.Cooper3@citrix.com>; George Dunlap
> <George.Dunlap@citrix.com>; Ian Jackson <Ian.Jackson@citrix.com>; Jan
> Beulich <jbeulich@suse.com>; Julien Grall <julien.grall@arm.com>; Konrad
> Rzeszutek Wilk <konrad.wilk@oracle.com>; Stefano Stabellini
> <sstabellini@kernel.org>; Tim (Xen.org) <tim@xen.org>; Wei Liu
> <wei.liu2@citrix.com>
> Subject: RE: [PATCH v3 2/2] VT-d: reconcile iommu_inclusive_mapping and
> iommu=dom0-strict
> 
> > From: Paul Durrant [mailto:paul.durrant@citrix.com]
> > Sent: Saturday, June 16, 2018 12:31 AM
> >
> > The documentation for the iommu_inclusive_mapping Xen command line
> > option
> > states:
> >
> > "Use this to work around firmware issues providing incorrect RMRR
> > entries"
> >
> > Unfortunately this workaround does not function correctly if the dom0-
> > strict
> > iommu option is also specified.
> >
> > The documentation goes on to say:
> >
> > "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."
> >
> > This patch modifies the VT-d hardware domain initialization code such that
> > the workaround will continue to function in dom0-strict mode, by mapping
> > all pages not marked as unusable *unless* they are RAM pages not
> > assigned
> > to dom0.
> >
> > NOTE: This patch modifies the test in drivers/passthrough/vtd/iommu.c
> > from
> >       need_iommu() to is_pv_domain() since dom0-strict implies
> > need_iommu()
> >       so we no longer want to gate invocation od vtd_set_hwdom_mapping()
> 
> od->of

Good spot. Can this be fixed up at commit please?

  Paul

> 
> >       on that.
> >       It also exports the iommu_dom0_strict flag so that the implementation
> >       of vtd_set_hwdom_mapping() can test it explicitly. It would be
> >       possible to test need_iommu() instead, but it is more illustrative
> >       to test the original flag rather than one of its side-effects.
> >
> > Signed-off-by: Paul Durrant <paul.durrant@citrix.com>
> > Reviewed-by: Roger Pau Monne <roger.pau@citrix.com>
> 
> Acked-by: Kevin Tian <kevin.tian@intel.com>

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