[PATCH v8 0/4] iommu: add rmrr Xen command line option

[PATCH v8 0/4] iommu: add rmrr Xen command line option

[elena.ufimtseva]

From: Elena Ufimtseva <elena.ufimtseva@oracle.com>

v8 of rmrr comman line patches.                                                 
                                                                                
Add Xen command line option rmrr to specify RMRR                                
regions for devices that are not defined in ACPI thus                           
causing IO Page Fault while booting dom0 in PVH mode.                           
These additional regions will be added to the list of                           
RMRR regions parsed from ACPI.                                                  
                                                                                
Changes in v8:                                                                  
 - removed bogus debug in patch 1 with non-functional changes;                  
 - changed PRI_RMRRL macro for formatting to reflect the fact that two arguments
   are used, so make it PRI_RMRR(s,e) for formatting inclusive RMRR range;      
   'L' is also removed from macro name, which meant to server as a type of arguments (%lx);
 - added overlapping check with RMRRs from ACPI;                                
 - added check based on paddr_bits for pfn's in extra RMRR range (not sure if   
   its redundant with mfn_valid);                                               
 - addressed while loop exit condition in extra RMRRs parser; 
      
Changes in v7:                                                                  
 - make sure RMRRs ranges are being checked correctly;                          
 - dont interrupt RMRRs checking if some of checks fails, instead               
 continue to next RMRR;                                                         
 - make rmrr variable names more obvious;                                       
 - fix debug output formatting to match type of rmrr range;                     
 - fix typos in rmrr command line document and in comments;                     
                                                                                
Changes in v6:                                                                  
 - make __parse_pci return correct result and error codes;                      
 - move add_extra_rmrr                                                          
 - previous patch was missing RMRR addresses in range check, add it here;       
 - add overlap check and range boundaries check;                                
 - moved extra rmrr structure definition to dmar.c;                             
 - change def_seg in __parse_pci type from int to bool_t;                       
 - change name for extra rmrr range to reflect they hold now pfns;   

Changes in v5:                                                                  
 - make parse_pci a wrapper and add __parse_pci with additional def_seg param   
   to identify if segment was specified;                                        
 - make possible not to define segment for each device within same rmrr;        
 - limit number of pages for one RMRR by 16;                                    
 - run mfn_valid check for every address in RMRR range;                         
 - add PCI_SBDF macro;                                                          
 - remove list for extra rmrrs as they are kept in static array;                
                                              
Elena Ufimtseva (4):
  pci: add PCI_SBDF and PCI_SEG macros
  iommu VT-d: separate rmrr addition function
  pci: add wrapper for parse_pci
  iommu: add rmrr Xen command line option for extra rmrrs

 docs/misc/xen-command-line.markdown |  13 ++
 xen/drivers/passthrough/vtd/dmar.c  | 360 ++++++++++++++++++++++++++++--------
 xen/drivers/pci/pci.c               |  11 ++
 xen/include/xen/pci.h               |   5 +
 4 files changed, 311 insertions(+), 78 deletions(-)

-- 
2.1.3

[PATCH v8 1/4] pci: add PCI_SBDF and PCI_SEG macros

[elena.ufimtseva]

From: Elena Ufimtseva <elena.ufimtseva@oracle.com>

Signed-off-by: Elena Ufimtseva <elena.ufimtseva@oracle.com>
---
 xen/include/xen/pci.h | 2 ++
 1 file changed, 2 insertions(+)

diff --git a/xen/include/xen/pci.h b/xen/include/xen/pci.h
index 3908146..414106a 100644
--- a/xen/include/xen/pci.h
+++ b/xen/include/xen/pci.h
@@ -33,6 +33,8 @@
 #define PCI_DEVFN2(bdf) ((bdf) & 0xff)
 #define PCI_BDF(b,d,f)  ((((b) & 0xff) << 8) | PCI_DEVFN(d,f))
 #define PCI_BDF2(b,df)  ((((b) & 0xff) << 8) | ((df) & 0xff))
+#define PCI_SBDF(s,b,d,f) ((((s) & 0xffff) << 16) | PCI_BDF(b,d,f))
+#define PCI_SEG(sbdf) (((sbdf) >> 16) & 0xffff)
 
 struct pci_dev_info {
     bool_t is_extfn;
-- 
2.1.3

Re: [PATCH v8 1/4] pci: add PCI_SBDF and PCI_SEG macros

[Konrad Rzeszutek Wilk]

On Tue, Jun 30, 2015 at 07:33:59PM -0400, elena.ufimtseva@oracle.com wrote:
> From: Elena Ufimtseva <elena.ufimtseva@oracle.com>
> 

You usually say why you need this patch. Something as simple as:

"In preperation for patch XXXX which will use it" is OK.

> Signed-off-by: Elena Ufimtseva <elena.ufimtseva@oracle.com>
> ---
>  xen/include/xen/pci.h | 2 ++
>  1 file changed, 2 insertions(+)
> 
> diff --git a/xen/include/xen/pci.h b/xen/include/xen/pci.h
> index 3908146..414106a 100644
> --- a/xen/include/xen/pci.h
> +++ b/xen/include/xen/pci.h
> @@ -33,6 +33,8 @@
>  #define PCI_DEVFN2(bdf) ((bdf) & 0xff)
>  #define PCI_BDF(b,d,f)  ((((b) & 0xff) << 8) | PCI_DEVFN(d,f))
>  #define PCI_BDF2(b,df)  ((((b) & 0xff) << 8) | ((df) & 0xff))
> +#define PCI_SBDF(s,b,d,f) ((((s) & 0xffff) << 16) | PCI_BDF(b,d,f))
> +#define PCI_SEG(sbdf) (((sbdf) >> 16) & 0xffff)
>  
>  struct pci_dev_info {
>      bool_t is_extfn;
> -- 
> 2.1.3
> 

Re: [PATCH v8 1/4] pci: add PCI_SBDF and PCI_SEG macros

[Jan Beulich]

>>> On 08.07.15 at 19:27, <konrad.wilk@oracle.com> wrote:
> On Tue, Jun 30, 2015 at 07:33:59PM -0400, elena.ufimtseva@oracle.com wrote:
>> From: Elena Ufimtseva <elena.ufimtseva@oracle.com>
>> 
> 
> You usually say why you need this patch. Something as simple as:
> 
> "In preperation for patch XXXX which will use it" is OK.

Or, even better, add such macros when the first user appears. Iirc
I said so before...

Jan

Re: [PATCH v8 1/4] pci: add PCI_SBDF and PCI_SEG macros

[Elena Ufimtseva]

On Thu, Jul 09, 2015 at 09:10:06AM +0100, Jan Beulich wrote:
> >>> On 08.07.15 at 19:27, <konrad.wilk@oracle.com> wrote:
> > On Tue, Jun 30, 2015 at 07:33:59PM -0400, elena.ufimtseva@oracle.com wrote:
> >> From: Elena Ufimtseva <elena.ufimtseva@oracle.com>
> >> 
> > 
> > You usually say why you need this patch. Something as simple as:
> > 
> > "In preperation for patch XXXX which will use it" is OK.
> 
> Or, even better, add such macros when the first user appears. Iirc
> I said so before...
>
Yes, I realized this late. Will move over in the next version if needed. 
> Jan
> 

Re: [PATCH v8 1/4] pci: add PCI_SBDF and PCI_SEG macros

[Jan Beulich]

>>> On 09.07.15 at 13:13, <elena.ufimtseva@oracle.com> wrote:
> On Thu, Jul 09, 2015 at 09:10:06AM +0100, Jan Beulich wrote:
>> >>> On 08.07.15 at 19:27, <konrad.wilk@oracle.com> wrote:
>> > On Tue, Jun 30, 2015 at 07:33:59PM -0400, elena.ufimtseva@oracle.com wrote:
>> >> From: Elena Ufimtseva <elena.ufimtseva@oracle.com>
>> >> 
>> > 
>> > You usually say why you need this patch. Something as simple as:
>> > 
>> > "In preperation for patch XXXX which will use it" is OK.
>> 
>> Or, even better, add such macros when the first user appears. Iirc
>> I said so before...
>>
> Yes, I realized this late. Will move over in the next version if needed. 

Don't you need to rebase on top of v6 of "dmar: device scope mem
leak fix" anyway? Or does the series not conflict with those changes?

Jan

[PATCH v8 2/4] iommu VT-d: separate rmrr addition function

[elena.ufimtseva]

From: Elena Ufimtseva <elena.ufimtseva@oracle.com>

In preparation for auxiliary RMRR data provided on Xen
command line, make RMRR adding a separate function.
Also free memery for rmrr device scope in error path.
I will also post additional patch with cleanups to address                      
other non-functional changes as per Jan's review of v7 of this patch.

Signed-off-by: Elena Ufimtseva <elena.ufimtseva@oracle.com>
---
Changes since v7:
 - removed bogus debug introduced in previous version;

 xen/drivers/passthrough/vtd/dmar.c | 126 +++++++++++++++++++------------------
 1 file changed, 65 insertions(+), 61 deletions(-)

diff --git a/xen/drivers/passthrough/vtd/dmar.c b/xen/drivers/passthrough/vtd/dmar.c
index 77ef708..a8e1e5d 100644
--- a/xen/drivers/passthrough/vtd/dmar.c
+++ b/xen/drivers/passthrough/vtd/dmar.c
@@ -585,6 +585,68 @@ out:
     return ret;
 }
 
+static int register_one_rmrr(struct acpi_rmrr_unit *rmrru)
+{
+    bool_t ignore = 0;
+    unsigned int i = 0;
+    int ret = 0;
+
+    /* Skip checking if segment is not accessible yet. */
+    if ( !pci_known_segment(rmrru->segment) )
+        i = UINT_MAX;
+
+    for ( ; i < rmrru->scope.devices_cnt; i++ )
+    {
+        u8 b = PCI_BUS(rmrru->scope.devices[i]);
+        u8 d = PCI_SLOT(rmrru->scope.devices[i]);
+        u8 f = PCI_FUNC(rmrru->scope.devices[i]);
+
+        if ( pci_device_detect(rmrru->segment, b, d, f) == 0 )
+        {
+            dprintk(XENLOG_WARNING VTDPREFIX,
+                    " Non-existent device (%04x:%02x:%02x.%u) is reported"
+                    " in RMRR (%"PRIx64", %"PRIx64")'s scope!\n",
+                    rmrru->segment, b, d, f,
+                    rmrru->base_address, rmrru->end_address);
+            ignore = 1;
+        }
+        else
+        {
+            ignore = 0;
+            break;
+        }
+    }
+
+    if ( ignore )
+    {
+        dprintk(XENLOG_WARNING VTDPREFIX,
+                "  Ignore the RMRR (%"PRIx64", %"PRIx64") due to "
+                "devices under its scope are not PCI discoverable!\n",
+            rmrru->base_address, rmrru->end_address);
+        scope_devices_free(&rmrru->scope);
+        xfree(rmrru);
+    }
+    else if ( rmrru->base_address > rmrru->end_address )
+    {
+        dprintk(XENLOG_WARNING VTDPREFIX,
+                "  The RMRR (%"PRIx64", %"PRIx64") is incorrect!\n",
+                rmrru->base_address, rmrru->end_address);
+        scope_devices_free(&rmrru->scope);
+        xfree(rmrru);
+        ret = -EFAULT;
+    }
+    else
+    {
+        if ( iommu_verbose )
+            dprintk(VTDPREFIX,
+                    "  RMRR region: base_addr %"PRIx64" end_address %"PRIx64"\n",
+                    rmrru->base_address, rmrru->end_address);
+        acpi_register_rmrr_unit(rmrru);
+    }
+
+    return ret;
+}
+
 static int __init
 acpi_parse_one_rmrr(struct acpi_dmar_header *header)
 {
@@ -635,68 +697,10 @@ acpi_parse_one_rmrr(struct acpi_dmar_header *header)
     ret = acpi_parse_dev_scope(dev_scope_start, dev_scope_end,
                                &rmrru->scope, RMRR_TYPE, rmrr->segment);
 
-    if ( ret || (rmrru->scope.devices_cnt == 0) )
-        xfree(rmrru);
+    if ( !ret && (rmrru->scope.devices_cnt != 0) )
+        register_one_rmrr(rmrru);
     else
-    {
-        u8 b, d, f;
-        bool_t ignore = 0;
-        unsigned int i = 0;
-
-        /* Skip checking if segment is not accessible yet. */
-        if ( !pci_known_segment(rmrr->segment) )
-            i = UINT_MAX;
-
-        for ( ; i < rmrru->scope.devices_cnt; i++ )
-        {
-            b = PCI_BUS(rmrru->scope.devices[i]);
-            d = PCI_SLOT(rmrru->scope.devices[i]);
-            f = PCI_FUNC(rmrru->scope.devices[i]);
-
-            if ( !pci_device_detect(rmrr->segment, b, d, f) )
-            {
-                dprintk(XENLOG_WARNING VTDPREFIX,
-                        " Non-existent device (%04x:%02x:%02x.%u) is reported"
-                        " in RMRR (%"PRIx64", %"PRIx64")'s scope!\n",
-                        rmrr->segment, b, d, f,
-                        rmrru->base_address, rmrru->end_address);
-                ignore = 1;
-            }
-            else
-            {
-                ignore = 0;
-                break;
-            }
-        }
-
-        if ( ignore )
-        {
-            dprintk(XENLOG_WARNING VTDPREFIX,
-                "  Ignore the RMRR (%"PRIx64", %"PRIx64") due to "
-                "devices under its scope are not PCI discoverable!\n",
-                rmrru->base_address, rmrru->end_address);
-            scope_devices_free(&rmrru->scope);
-            xfree(rmrru);
-        }
-        else if ( base_addr > end_addr )
-        {
-            dprintk(XENLOG_WARNING VTDPREFIX,
-                "  The RMRR (%"PRIx64", %"PRIx64") is incorrect!\n",
-                rmrru->base_address, rmrru->end_address);
-            scope_devices_free(&rmrru->scope);
-            xfree(rmrru);
-            ret = -EFAULT;
-        }
-        else
-        {
-            if ( iommu_verbose )
-                dprintk(VTDPREFIX,
-                        "  RMRR region: base_addr %"PRIx64
-                        " end_address %"PRIx64"\n",
-                        rmrru->base_address, rmrru->end_address);
-            acpi_register_rmrr_unit(rmrru);
-        }
-    }
+        xfree(rmrru);
 
     return ret;
 }
-- 
2.1.3

Re: [PATCH v8 2/4] iommu VT-d: separate rmrr addition function

[Konrad Rzeszutek Wilk]

On Tue, Jun 30, 2015 at 07:34:00PM -0400, elena.ufimtseva@oracle.com wrote:
> From: Elena Ufimtseva <elena.ufimtseva@oracle.com>
> 
> In preparation for auxiliary RMRR data provided on Xen
> command line, make RMRR adding a separate function.
> Also free memery for rmrr device scope in error path.
> I will also post additional patch with cleanups to address                      
> other non-functional changes as per Jan's review of v7 of this patch.

Those two linesL "I will also .." should be below the --- as there
is no need for that in the final git tree.

Otherwise looks OK.
> 
> Signed-off-by: Elena Ufimtseva <elena.ufimtseva@oracle.com>
> ---
> Changes since v7:
>  - removed bogus debug introduced in previous version;
> 
>  xen/drivers/passthrough/vtd/dmar.c | 126 +++++++++++++++++++------------------
>  1 file changed, 65 insertions(+), 61 deletions(-)
> 
> diff --git a/xen/drivers/passthrough/vtd/dmar.c b/xen/drivers/passthrough/vtd/dmar.c
> index 77ef708..a8e1e5d 100644
> --- a/xen/drivers/passthrough/vtd/dmar.c
> +++ b/xen/drivers/passthrough/vtd/dmar.c
> @@ -585,6 +585,68 @@ out:
>      return ret;
>  }
>  
> +static int register_one_rmrr(struct acpi_rmrr_unit *rmrru)
> +{
> +    bool_t ignore = 0;
> +    unsigned int i = 0;
> +    int ret = 0;
> +
> +    /* Skip checking if segment is not accessible yet. */
> +    if ( !pci_known_segment(rmrru->segment) )
> +        i = UINT_MAX;
> +
> +    for ( ; i < rmrru->scope.devices_cnt; i++ )
> +    {
> +        u8 b = PCI_BUS(rmrru->scope.devices[i]);
> +        u8 d = PCI_SLOT(rmrru->scope.devices[i]);
> +        u8 f = PCI_FUNC(rmrru->scope.devices[i]);
> +
> +        if ( pci_device_detect(rmrru->segment, b, d, f) == 0 )
> +        {
> +            dprintk(XENLOG_WARNING VTDPREFIX,
> +                    " Non-existent device (%04x:%02x:%02x.%u) is reported"
> +                    " in RMRR (%"PRIx64", %"PRIx64")'s scope!\n",
> +                    rmrru->segment, b, d, f,
> +                    rmrru->base_address, rmrru->end_address);
> +            ignore = 1;
> +        }
> +        else
> +        {
> +            ignore = 0;
> +            break;
> +        }
> +    }
> +
> +    if ( ignore )
> +    {
> +        dprintk(XENLOG_WARNING VTDPREFIX,
> +                "  Ignore the RMRR (%"PRIx64", %"PRIx64") due to "
> +                "devices under its scope are not PCI discoverable!\n",
> +            rmrru->base_address, rmrru->end_address);
> +        scope_devices_free(&rmrru->scope);
> +        xfree(rmrru);
> +    }
> +    else if ( rmrru->base_address > rmrru->end_address )
> +    {
> +        dprintk(XENLOG_WARNING VTDPREFIX,
> +                "  The RMRR (%"PRIx64", %"PRIx64") is incorrect!\n",
> +                rmrru->base_address, rmrru->end_address);
> +        scope_devices_free(&rmrru->scope);
> +        xfree(rmrru);
> +        ret = -EFAULT;
> +    }
> +    else
> +    {
> +        if ( iommu_verbose )
> +            dprintk(VTDPREFIX,
> +                    "  RMRR region: base_addr %"PRIx64" end_address %"PRIx64"\n",
> +                    rmrru->base_address, rmrru->end_address);
> +        acpi_register_rmrr_unit(rmrru);
> +    }
> +
> +    return ret;
> +}
> +
>  static int __init
>  acpi_parse_one_rmrr(struct acpi_dmar_header *header)
>  {
> @@ -635,68 +697,10 @@ acpi_parse_one_rmrr(struct acpi_dmar_header *header)
>      ret = acpi_parse_dev_scope(dev_scope_start, dev_scope_end,
>                                 &rmrru->scope, RMRR_TYPE, rmrr->segment);
>  
> -    if ( ret || (rmrru->scope.devices_cnt == 0) )
> -        xfree(rmrru);
> +    if ( !ret && (rmrru->scope.devices_cnt != 0) )
> +        register_one_rmrr(rmrru);
>      else
> -    {
> -        u8 b, d, f;
> -        bool_t ignore = 0;
> -        unsigned int i = 0;
> -
> -        /* Skip checking if segment is not accessible yet. */
> -        if ( !pci_known_segment(rmrr->segment) )
> -            i = UINT_MAX;
> -
> -        for ( ; i < rmrru->scope.devices_cnt; i++ )
> -        {
> -            b = PCI_BUS(rmrru->scope.devices[i]);
> -            d = PCI_SLOT(rmrru->scope.devices[i]);
> -            f = PCI_FUNC(rmrru->scope.devices[i]);
> -
> -            if ( !pci_device_detect(rmrr->segment, b, d, f) )
> -            {
> -                dprintk(XENLOG_WARNING VTDPREFIX,
> -                        " Non-existent device (%04x:%02x:%02x.%u) is reported"
> -                        " in RMRR (%"PRIx64", %"PRIx64")'s scope!\n",
> -                        rmrr->segment, b, d, f,
> -                        rmrru->base_address, rmrru->end_address);
> -                ignore = 1;
> -            }
> -            else
> -            {
> -                ignore = 0;
> -                break;
> -            }
> -        }
> -
> -        if ( ignore )
> -        {
> -            dprintk(XENLOG_WARNING VTDPREFIX,
> -                "  Ignore the RMRR (%"PRIx64", %"PRIx64") due to "
> -                "devices under its scope are not PCI discoverable!\n",
> -                rmrru->base_address, rmrru->end_address);
> -            scope_devices_free(&rmrru->scope);
> -            xfree(rmrru);
> -        }
> -        else if ( base_addr > end_addr )
> -        {
> -            dprintk(XENLOG_WARNING VTDPREFIX,
> -                "  The RMRR (%"PRIx64", %"PRIx64") is incorrect!\n",
> -                rmrru->base_address, rmrru->end_address);
> -            scope_devices_free(&rmrru->scope);
> -            xfree(rmrru);
> -            ret = -EFAULT;
> -        }
> -        else
> -        {
> -            if ( iommu_verbose )
> -                dprintk(VTDPREFIX,
> -                        "  RMRR region: base_addr %"PRIx64
> -                        " end_address %"PRIx64"\n",
> -                        rmrru->base_address, rmrru->end_address);
> -            acpi_register_rmrr_unit(rmrru);
> -        }
> -    }
> +        xfree(rmrru);
>  
>      return ret;
>  }
> -- 
> 2.1.3
> 

[PATCH v8 3/4] pci: add wrapper for parse_pci

[elena.ufimtseva]

From: Elena Ufimtseva <elena.ufimtseva@oracle.com>

For sbdf'si parsing in rmrr command line add __parse_pci with addtional
parameter def_seg. __parse_pci will help to identify if segment was
found
in string being parsed or default segment was used.
Make a wrapper parse_pci so the rest of the callers are not affected.

Signed-off-by: Elena Ufimtseva <elena.ufimtseva@oracle.com>
Acked-by: Jan Beulich <jbeulich@suse.com>
---
 xen/drivers/pci/pci.c | 11 +++++++++++
 xen/include/xen/pci.h |  3 +++
 2 files changed, 14 insertions(+)

diff --git a/xen/drivers/pci/pci.c b/xen/drivers/pci/pci.c
index ca07ed0..788a356 100644
--- a/xen/drivers/pci/pci.c
+++ b/xen/drivers/pci/pci.c
@@ -119,11 +119,21 @@ const char *__init parse_pci(const char *s, unsigned int *seg_p,
                              unsigned int *bus_p, unsigned int *dev_p,
                              unsigned int *func_p)
 {
+    bool_t def_seg;
+
+    return __parse_pci(s, seg_p, bus_p, dev_p, func_p, &def_seg);
+}
+
+const char *__init __parse_pci(const char *s, unsigned int *seg_p,
+                             unsigned int *bus_p, unsigned int *dev_p,
+                             unsigned int *func_p, bool_t *def_seg)
+{
     unsigned long seg = simple_strtoul(s, &s, 16), bus, dev, func;
 
     if ( *s != ':' )
         return NULL;
     bus = simple_strtoul(s + 1, &s, 16);
+    *def_seg = 0;
     if ( *s == ':' )
         dev = simple_strtoul(s + 1, &s, 16);
     else
@@ -131,6 +141,7 @@ const char *__init parse_pci(const char *s, unsigned int *seg_p,
         dev = bus;
         bus = seg;
         seg = 0;
+        *def_seg = 1;
     }
     if ( func_p )
     {
diff --git a/xen/include/xen/pci.h b/xen/include/xen/pci.h
index 414106a..d66ecab 100644
--- a/xen/include/xen/pci.h
+++ b/xen/include/xen/pci.h
@@ -150,6 +150,9 @@ int pci_find_ext_capability(int seg, int bus, int devfn, int cap);
 int pci_find_next_ext_capability(int seg, int bus, int devfn, int pos, int cap);
 const char *parse_pci(const char *, unsigned int *seg, unsigned int *bus,
                       unsigned int *dev, unsigned int *func);
+const char *__parse_pci(const char *, unsigned int *seg, unsigned int *bus,
+                      unsigned int *dev, unsigned int *func, bool_t *def_seg);
+
 
 bool_t pcie_aer_get_firmware_first(const struct pci_dev *);
 
-- 
2.1.3

Re: [PATCH v8 3/4] pci: add wrapper for parse_pci

[Konrad Rzeszutek Wilk]

On Tue, Jun 30, 2015 at 07:34:01PM -0400, elena.ufimtseva@oracle.com wrote:
> From: Elena Ufimtseva <elena.ufimtseva@oracle.com>
> 
> For sbdf'si parsing in rmrr command line add __parse_pci with addtional

sbdf'si ?

s/rmrr/RMRR/

s/addtional/additional/


> parameter def_seg. __parse_pci will help to identify if segment was
> found
> in string being parsed or default segment was used.
> Make a wrapper parse_pci so the rest of the callers are not affected.
> 
> Signed-off-by: Elena Ufimtseva <elena.ufimtseva@oracle.com>
> Acked-by: Jan Beulich <jbeulich@suse.com>
> ---
>  xen/drivers/pci/pci.c | 11 +++++++++++
>  xen/include/xen/pci.h |  3 +++
>  2 files changed, 14 insertions(+)
> 
> diff --git a/xen/drivers/pci/pci.c b/xen/drivers/pci/pci.c
> index ca07ed0..788a356 100644
> --- a/xen/drivers/pci/pci.c
> +++ b/xen/drivers/pci/pci.c
> @@ -119,11 +119,21 @@ const char *__init parse_pci(const char *s, unsigned int *seg_p,
>                               unsigned int *bus_p, unsigned int *dev_p,
>                               unsigned int *func_p)
>  {
> +    bool_t def_seg;
> +
> +    return __parse_pci(s, seg_p, bus_p, dev_p, func_p, &def_seg);
> +}
> +
> +const char *__init __parse_pci(const char *s, unsigned int *seg_p,
> +                             unsigned int *bus_p, unsigned int *dev_p,
> +                             unsigned int *func_p, bool_t *def_seg)
> +{
>      unsigned long seg = simple_strtoul(s, &s, 16), bus, dev, func;
>  
>      if ( *s != ':' )
>          return NULL;
>      bus = simple_strtoul(s + 1, &s, 16);
> +    *def_seg = 0;
>      if ( *s == ':' )
>          dev = simple_strtoul(s + 1, &s, 16);
>      else
> @@ -131,6 +141,7 @@ const char *__init parse_pci(const char *s, unsigned int *seg_p,
>          dev = bus;
>          bus = seg;
>          seg = 0;
> +        *def_seg = 1;
>      }
>      if ( func_p )
>      {
> diff --git a/xen/include/xen/pci.h b/xen/include/xen/pci.h
> index 414106a..d66ecab 100644
> --- a/xen/include/xen/pci.h
> +++ b/xen/include/xen/pci.h
> @@ -150,6 +150,9 @@ int pci_find_ext_capability(int seg, int bus, int devfn, int cap);
>  int pci_find_next_ext_capability(int seg, int bus, int devfn, int pos, int cap);
>  const char *parse_pci(const char *, unsigned int *seg, unsigned int *bus,
>                        unsigned int *dev, unsigned int *func);
> +const char *__parse_pci(const char *, unsigned int *seg, unsigned int *bus,
> +                      unsigned int *dev, unsigned int *func, bool_t *def_seg);
> +
>  
>  bool_t pcie_aer_get_firmware_first(const struct pci_dev *);
>  
> -- 
> 2.1.3
> 

[PATCH v8 4/4] iommu: add rmrr Xen command line option for extra rmrrs

[elena.ufimtseva]

From: Elena Ufimtseva <elena.ufimtseva@oracle.com>

On some platforms RMRR regions may be not specified
in ACPI and thus will not be mapped 1:1 in dom0. This
causes IO Page Faults and prevents dom0 from booting
in PVH mode.
New Xen command line option rmrr allows to specify
such devices and memory regions. These regions are added
to the list of RMRR defined in ACPI if the device
is present in system. As a result, additional RMRRs will
be mapped 1:1 in dom0 with correct permissions.

Mentioned above problems were discovered during PVH work with
ThinkCentre M and Dell 5600T. No official documentation
was found so far in regards to what devices and why cause this.
Experiments show that ThinkCentre M USB devices with enabled
debug port generate DMA read transactions to the regions of
memory marked reserved in host e820 map.
For Dell 5600T the device and faulting addresses are not found yet.

For detailed history of the discussion please check following threads:
http://lists.Xen.org/archives/html/xen-devel/2015-02/msg01724.html
http://lists.Xen.org/archives/html/xen-devel/2015-01/msg02513.html

Format for rmrr Xen command line option:
rmrr=start<-end>=[s1]bdf1[,[s1]bdf2[,...]];start<-end>=[s2]bdf1[,[s2]bdf2[,...]]
If grub2 used and multiple ranges are specified, ';' should be
quoted/escaped, refer to grub2 manual for more information.

Signed-off-by: Elena Ufimtseva <elena.ufimtseva@oracle.com>
---
 docs/misc/xen-command-line.markdown |  13 ++
 xen/drivers/passthrough/vtd/dmar.c  | 246 ++++++++++++++++++++++++++++++++----
 2 files changed, 236 insertions(+), 23 deletions(-)

diff --git a/docs/misc/xen-command-line.markdown b/docs/misc/xen-command-line.markdown
index aa684c0..f307f3d 100644
--- a/docs/misc/xen-command-line.markdown
+++ b/docs/misc/xen-command-line.markdown
@@ -1197,6 +1197,19 @@ Specify the host reboot method.
 'efi' instructs Xen to reboot using the EFI reboot call (in EFI mode by
  default it will use that method first).
 
+### rmrr
+> '= start<-end>=[s1]bdf1[,[s1]bdf2[,...]];start<-end>=[s2]bdf1[,[s2]bdf2[,...]]
+
+Define RMRR units that are missing from ACPI table along with device they
+belong to and use them for 1:1 mapping. End addresses can be omitted and one
+page will be mapped. The ranges are inclusive when start and end are specified.
+If segment of the first device is not specified, segment zero will be used.
+If other segments are not specified, first device segment will be used.
+If a segment is specified for other than the first device and it does not match
+the one specified for the first one, an error will be reported.
+Note: grub2 requires to escape or use quotations if special characters are used,
+namely ';', refer to the grub2 documentation if multiple ranges are specified.
+
 ### ro-hpet
 > `= <boolean>`
 
diff --git a/xen/drivers/passthrough/vtd/dmar.c b/xen/drivers/passthrough/vtd/dmar.c
index a8e1e5d..fa659a9 100644
--- a/xen/drivers/passthrough/vtd/dmar.c
+++ b/xen/drivers/passthrough/vtd/dmar.c
@@ -42,6 +42,8 @@
 #define MIN_SCOPE_LEN (sizeof(struct acpi_dmar_device_scope) + \
                        sizeof(struct acpi_dmar_pci_path))
 
+#define PRI_RMRR(s,e) "[%lx-%lx]"
+
 LIST_HEAD_READ_MOSTLY(acpi_drhd_units);
 LIST_HEAD_READ_MOSTLY(acpi_rmrr_units);
 static LIST_HEAD_READ_MOSTLY(acpi_atsr_units);
@@ -425,7 +427,7 @@ static int __init acpi_parse_dev_scope(
         default:
             if ( iommu_verbose )
                 printk(XENLOG_WARNING VTDPREFIX "Unknown scope type %#x\n",
-                       acpi_scope->entry_type);
+                acpi_scope->entry_type);
             start += acpi_scope->length;
             continue;
         }
@@ -479,8 +481,7 @@ acpi_parse_one_drhd(struct acpi_dmar_header *header)
     INIT_LIST_HEAD(&dmaru->ioapic_list);
     INIT_LIST_HEAD(&dmaru->hpet_list);
     if ( iommu_verbose )
-        dprintk(VTDPREFIX, "  dmaru->address = %"PRIx64"\n",
-                dmaru->address);
+        dprintk(VTDPREFIX, "  dmaru->address = %"PRIx64"\n", dmaru->address);
 
     ret = iommu_alloc(dmaru);
     if ( ret )
@@ -541,8 +542,8 @@ acpi_parse_one_drhd(struct acpi_dmar_header *header)
             if ( !pci_device_detect(drhd->segment, b, d, f) )
             {
                 dprintk(XENLOG_WARNING VTDPREFIX,
-                        " Non-existent device (%04x:%02x:%02x.%u) is reported"
-                        " in this DRHD's scope!\n", drhd->segment, b, d, f);
+                        " Non-existent device (%04x:%02x:%02x.%u) is reported in this DRHD's scope!\n",
+                        drhd->segment, b, d, f);
                 invalid_cnt++;
             }
         }
@@ -553,8 +554,8 @@ acpi_parse_one_drhd(struct acpi_dmar_header *header)
                  invalid_cnt == dmaru->scope.devices_cnt )
             {
                 dprintk(XENLOG_WARNING VTDPREFIX,
-                    "  Workaround BIOS bug: ignore the DRHD due to all "
-                    "devices under its scope are not PCI discoverable!\n");
+                        "  Workaround BIOS bug: ignore the DRHD due to all "
+                        "devices under its scope are not PCI discoverable!\n");
 
                 scope_devices_free(&dmaru->scope);
                 iommu_free(dmaru);
@@ -563,10 +564,10 @@ acpi_parse_one_drhd(struct acpi_dmar_header *header)
             else
             {
                 dprintk(XENLOG_WARNING VTDPREFIX,
-                    "  The DRHD is invalid due to there are devices under "
-                    "its scope are not PCI discoverable! Pls try option "
-                    "iommu=force or iommu=workaround_bios_bug if you "
-                    "really want VT-d\n");
+                        "  The DRHD is invalid due to there are devices under "
+                        "its scope are not PCI discoverable! Pls try option "
+                        "iommu=force or iommu=workaround_bios_bug if you "
+                        "really want VT-d\n");
                 ret = -EINVAL;
             }
         }
@@ -604,8 +605,7 @@ static int register_one_rmrr(struct acpi_rmrr_unit *rmrru)
         if ( pci_device_detect(rmrru->segment, b, d, f) == 0 )
         {
             dprintk(XENLOG_WARNING VTDPREFIX,
-                    " Non-existent device (%04x:%02x:%02x.%u) is reported"
-                    " in RMRR (%"PRIx64", %"PRIx64")'s scope!\n",
+                    " Non-existent device (%04x:%02x:%02x.%u) is reported in RMRR "PRI_RMRR(s,e)"'s scope!\n",
                     rmrru->segment, b, d, f,
                     rmrru->base_address, rmrru->end_address);
             ignore = 1;
@@ -620,16 +620,16 @@ static int register_one_rmrr(struct acpi_rmrr_unit *rmrru)
     if ( ignore )
     {
         dprintk(XENLOG_WARNING VTDPREFIX,
-                "  Ignore the RMRR (%"PRIx64", %"PRIx64") due to "
+                "  Ignore the RMRR "PRI_RMRR(s,e)" due to "
                 "devices under its scope are not PCI discoverable!\n",
-            rmrru->base_address, rmrru->end_address);
+                rmrru->base_address, rmrru->end_address);
         scope_devices_free(&rmrru->scope);
         xfree(rmrru);
     }
     else if ( rmrru->base_address > rmrru->end_address )
     {
         dprintk(XENLOG_WARNING VTDPREFIX,
-                "  The RMRR (%"PRIx64", %"PRIx64") is incorrect!\n",
+                "  The RMRR "PRI_RMRR(s,e)" is incorrect!\n",
                 rmrru->base_address, rmrru->end_address);
         scope_devices_free(&rmrru->scope);
         xfree(rmrru);
@@ -639,7 +639,7 @@ static int register_one_rmrr(struct acpi_rmrr_unit *rmrru)
     {
         if ( iommu_verbose )
             dprintk(VTDPREFIX,
-                    "  RMRR region: base_addr %"PRIx64" end_address %"PRIx64"\n",
+                    "  RMRR region: "PRI_RMRR(s,e)"\n",
                     rmrru->base_address, rmrru->end_address);
         acpi_register_rmrr_unit(rmrru);
     }
@@ -664,7 +664,7 @@ acpi_parse_one_rmrr(struct acpi_dmar_header *header)
        if ( base_addr <= rmrru->end_address && rmrru->base_address <= end_addr )
        {
            printk(XENLOG_ERR VTDPREFIX
-                  "Overlapping RMRRs [%"PRIx64",%"PRIx64"] and [%"PRIx64",%"PRIx64"]\n",
+                  "Overlapping RMRRs "PRI_RMRR(s,e)" and "PRI_RMRR(s,e)"\n",
                   rmrru->base_address, rmrru->end_address,
                   base_addr, end_addr);
            return -EEXIST;
@@ -678,8 +678,7 @@ acpi_parse_one_rmrr(struct acpi_dmar_header *header)
          (!page_is_ram_type(paddr_to_pfn(end_addr), RAM_TYPE_RESERVED)) )
     {
         dprintk(XENLOG_WARNING VTDPREFIX,
-                "  RMRR address range not in reserved memory "
-                "base = %"PRIx64" end = %"PRIx64"; "
+                "  RMRR address range "PRI_RMRR(s,e)" not in reserved memory; "
                 "iommu_inclusive_mapping=1 parameter may be needed.\n",
                 base_addr, end_addr);
     }
@@ -779,8 +778,7 @@ acpi_parse_one_rhsa(struct acpi_dmar_header *header)
     list_add_tail(&rhsau->list, &acpi_rhsa_units);
     if ( iommu_verbose )
         dprintk(VTDPREFIX,
-                "  rhsau->address: %"PRIx64
-                " rhsau->proximity_domain: %"PRIx32"\n",
+                "  rhsau->address: %"PRIx64" rhsau->proximity_domain: %"PRIx32"\n",
                 rhsau->address, rhsau->proximity_domain);
 
     return ret;
@@ -869,6 +867,140 @@ out:
     return ret;
 }
 
+#define MAX_EXTRA_RMRR_PAGES 16
+#define MAX_EXTRA_RMRR 10
+
+/* RMRR units derived from command line rmrr option */
+#define MAX_EXTRA_RMRR_DEV 20
+struct extra_rmrr_unit {
+    struct list_head list;
+    unsigned long base_pfn, end_pfn;
+    unsigned int dev_count;
+    u32    sbdf[MAX_EXTRA_RMRR_DEV];
+};
+static __initdata unsigned int nr_rmrr;
+static struct __initdata extra_rmrr_unit extra_rmrr_units[MAX_EXTRA_RMRR];
+
+/* Macro for RMRR inclusive range formatting */
+
+static void __init add_extra_rmrr(void)
+{
+    struct acpi_rmrr_unit *acpi_rmrr;
+    struct acpi_rmrr_unit *rmrru;
+    unsigned int dev, seg, i, j;
+    unsigned long pfn;
+
+    for ( i = 0; i < nr_rmrr; i++ )
+    {
+        if ( extra_rmrr_units[i].base_pfn > extra_rmrr_units[i].end_pfn )
+        {
+            printk(XENLOG_ERR VTDPREFIX
+                   "Invalid RMRR Range "PRI_RMRR(s,e)"\n",
+                   extra_rmrr_units[i].base_pfn, extra_rmrr_units[i].end_pfn);
+            continue;
+        }
+
+        if ( extra_rmrr_units[i].end_pfn - extra_rmrr_units[i].base_pfn >=
+             MAX_EXTRA_RMRR_PAGES )
+        {
+            printk(XENLOG_ERR VTDPREFIX
+                   "RMRR range "PRI_RMRR(s,e)" exceeds "__stringify(MAX_EXTRA_RMRR_PAGES)" pages\n",
+                   extra_rmrr_units[i].base_pfn, extra_rmrr_units[i].end_pfn);
+            continue;
+        }
+
+        for ( j = 0; j < nr_rmrr; j++ )
+        {
+            if ( i != j &&
+                 extra_rmrr_units[i].base_pfn <= extra_rmrr_units[j].end_pfn &&
+                 extra_rmrr_units[j].base_pfn <= extra_rmrr_units[i].end_pfn )
+            {
+                printk(XENLOG_ERR VTDPREFIX
+                      "Overlapping RMRRs "PRI_RMRR(s,e)" and "PRI_RMRR(s,e)"\n",
+                      extra_rmrr_units[i].base_pfn, extra_rmrr_units[i].end_pfn,
+                      extra_rmrr_units[j].base_pfn, extra_rmrr_units[j].end_pfn);
+                break;
+            }
+        }
+        /* Broke out of the overlap loop check, continue with next rmrr. */
+        if ( j < nr_rmrr )
+            continue;
+        list_for_each_entry(rmrru, &acpi_rmrr_units, list)
+        {
+            if ( pfn_to_paddr(extra_rmrr_units[i].base_pfn <= rmrru->end_address) &&
+                 rmrru->base_address <= pfn_to_paddr(extra_rmrr_units[i].end_pfn) )
+            {
+                printk(XENLOG_ERR VTDPREFIX
+                       "Overlapping extra RMRRs "PRI_RMRR(s,e)" and ACPI RMRRs "PRI_RMRR(s,e)"\n",
+                       extra_rmrr_units[i].base_pfn,
+                       extra_rmrr_units[i].end_pfn,
+                       paddr_to_pfn(rmrru->base_address),
+                       paddr_to_pfn(rmrru->end_address));
+                break;
+            }
+        }
+
+
+        pfn = extra_rmrr_units[i].base_pfn;
+        do
+        {
+            if ( !mfn_valid(pfn) || (pfn >> (paddr_bits - PAGE_SHIFT)) )
+            {
+                if ( iommu_verbose )
+                    printk(XENLOG_ERR VTDPREFIX
+                           "Invalid mfn in RMRR range "PRI_RMRR(s,e)"\n",
+                           extra_rmrr_units[i].base_pfn, extra_rmrr_units[i].end_pfn);
+                break;
+            }
+        } while ( pfn++ <= extra_rmrr_units[i].end_pfn );
+        /* The range had invalid mfn as the loop was broken out before reaching end_pfn. */
+        if ( pfn <= extra_rmrr_units[i].end_pfn )
+            continue;
+
+        acpi_rmrr = xzalloc(struct acpi_rmrr_unit);
+        if ( !acpi_rmrr )
+            return;
+
+        acpi_rmrr->scope.devices = xmalloc_array(u16,
+                                                 extra_rmrr_units[i].dev_count);
+        if ( !acpi_rmrr->scope.devices )
+        {
+            xfree(acpi_rmrr);
+            return;
+        }
+
+        seg = 0;
+        for ( dev = 0; dev < extra_rmrr_units[i].dev_count; dev++ )
+        {
+            acpi_rmrr->scope.devices[dev] = extra_rmrr_units[i].sbdf[dev];
+            seg = seg | PCI_SEG(extra_rmrr_units[i].sbdf[dev]);
+        }
+        if ( seg != PCI_SEG(extra_rmrr_units[i].sbdf[0]) )
+        {
+            printk(XENLOG_ERR VTDPREFIX
+                   "Segments are not equal for RMRR range "PRI_RMRR(s,e)"\n",
+                   extra_rmrr_units[i].base_pfn, extra_rmrr_units[i].end_pfn);
+            scope_devices_free(&acpi_rmrr->scope);
+            xfree(acpi_rmrr);
+            continue;
+        }
+
+        acpi_rmrr->segment = seg;
+        acpi_rmrr->base_address = pfn_to_paddr(extra_rmrr_units[i].base_pfn);
+        acpi_rmrr->end_address = pfn_to_paddr(extra_rmrr_units[i].end_pfn + 1);
+        acpi_rmrr->scope.devices_cnt = extra_rmrr_units[i].dev_count;
+
+        if ( register_one_rmrr(acpi_rmrr) )
+        {
+            printk(XENLOG_ERR VTDPREFIX
+                   "Could not register RMMR range "PRI_RMRR(s,e)"\n",
+                   extra_rmrr_units[i].base_pfn, extra_rmrr_units[i].end_pfn);
+            scope_devices_free(&acpi_rmrr->scope);
+            xfree(acpi_rmrr);
+        }
+    }
+}
+
 #include <asm/tboot.h>
 /* ACPI tables may not be DMA protected by tboot, so use DMAR copy */
 /* SINIT saved in SinitMleData in TXT heap (which is DMA protected) */
@@ -878,6 +1010,7 @@ int __init acpi_dmar_init(void)
 {
     acpi_physical_address dmar_addr;
     acpi_native_uint dmar_len;
+    int ret;
 
     if ( ACPI_SUCCESS(acpi_get_table_phys(ACPI_SIG_DMAR, 0,
                                           &dmar_addr, &dmar_len)) )
@@ -888,7 +1021,10 @@ int __init acpi_dmar_init(void)
         dmar_table = __va(dmar_addr);
     }
 
-    return parse_dmar_table(acpi_parse_dmar);
+    ret = parse_dmar_table(acpi_parse_dmar);
+    add_extra_rmrr();
+
+    return ret;
 }
 
 void acpi_dmar_reinstate(void)
@@ -919,3 +1055,67 @@ int platform_supports_x2apic(void)
     unsigned int mask = ACPI_DMAR_INTR_REMAP | ACPI_DMAR_X2APIC_OPT_OUT;
     return cpu_has_x2apic && ((dmar_flags & mask) == ACPI_DMAR_INTR_REMAP);
 }
+
+/*
+ * Parse rmrr Xen command line options and add parsed device and region into
+ * acpi_rmrr_unit list to mapped as RMRRs parsed from ACPI.
+ * Format:
+ * rmrr=start<-end>=[s1]bdf1[,[s1]bdf2[,...]];start<-end>=[s2]bdf1[,[s2]bdf2[,...]]
+ * If the segment of the first device is not specified, segment zero will be used.
+ * If other segments are not specified, first device segment will be used.
+ * If a segment is specified for other than the first device and it does not match
+ * the one specified for the first one, an error will be reported.
+ */
+static void __init parse_rmrr_param(const char *str)
+{
+    const char *s = str, *cur, *stmp;
+    unsigned int seg, bus, dev, func;
+    unsigned long start, end;
+
+    do {
+        start = simple_strtoul(cur = s, &s, 0);
+        if ( cur == s )
+            break;
+
+        if ( *s == '-' )
+        {
+            end = simple_strtoul(cur = s + 1, &s, 0);
+            if ( cur == s )
+                break;
+        }
+        else
+            end = start;
+
+        extra_rmrr_units[nr_rmrr].base_pfn = start;
+        extra_rmrr_units[nr_rmrr].end_pfn = end;
+        extra_rmrr_units[nr_rmrr].dev_count = 0;
+
+        if ( *s != '=' )
+            continue;
+
+        do {
+            bool_t default_segment = 0;
+
+            if ( *s == ';' )
+                break;
+            stmp = __parse_pci(s + 1, &seg, &bus, &dev, &func, &default_segment);
+            if ( !stmp )
+                break;
+
+            /* Not specified segment will be replaced with one from first device. */
+            if ( extra_rmrr_units[nr_rmrr].dev_count && default_segment )
+                seg = PCI_SEG(extra_rmrr_units[nr_rmrr].sbdf[0]);
+
+            /* Keep sbdf's even if they differ and later report an error. */
+            extra_rmrr_units[nr_rmrr].sbdf[extra_rmrr_units[nr_rmrr].dev_count] = PCI_SBDF(seg, bus, dev, func);
+            extra_rmrr_units[nr_rmrr].dev_count++;
+            s = stmp;
+        } while ( *s == ',' &&
+                  extra_rmrr_units[nr_rmrr].dev_count < MAX_EXTRA_RMRR_DEV );
+
+        if ( extra_rmrr_units[nr_rmrr].dev_count )
+            nr_rmrr++;
+
+    } while ( *s++ == ';' && nr_rmrr < MAX_EXTRA_RMRR );
+}
+custom_param("rmrr", parse_rmrr_param);
-- 
2.1.3

Re: [PATCH v8 4/4] iommu: add rmrr Xen command line option for extra rmrrs

[Konrad Rzeszutek Wilk]

. snip..
> diff --git a/xen/drivers/passthrough/vtd/dmar.c b/xen/drivers/passthrough/vtd/dmar.c
> index a8e1e5d..fa659a9 100644
> --- a/xen/drivers/passthrough/vtd/dmar.c
> +++ b/xen/drivers/passthrough/vtd/dmar.c
> @@ -42,6 +42,8 @@
>  #define MIN_SCOPE_LEN (sizeof(struct acpi_dmar_device_scope) + \
>                         sizeof(struct acpi_dmar_pci_path))
>  
> +#define PRI_RMRR(s,e) "[%lx-%lx]"
> +
>  LIST_HEAD_READ_MOSTLY(acpi_drhd_units);
>  LIST_HEAD_READ_MOSTLY(acpi_rmrr_units);
>  static LIST_HEAD_READ_MOSTLY(acpi_atsr_units);
> @@ -425,7 +427,7 @@ static int __init acpi_parse_dev_scope(
>          default:
>              if ( iommu_verbose )
>                  printk(XENLOG_WARNING VTDPREFIX "Unknown scope type %#x\n",
> -                       acpi_scope->entry_type);
> +                acpi_scope->entry_type);

Stray change? It should be in a seperate patch I think?

>              start += acpi_scope->length;
>              continue;
>          }
> @@ -479,8 +481,7 @@ acpi_parse_one_drhd(struct acpi_dmar_header *header)
>      INIT_LIST_HEAD(&dmaru->ioapic_list);
>      INIT_LIST_HEAD(&dmaru->hpet_list);
>      if ( iommu_verbose )
> -        dprintk(VTDPREFIX, "  dmaru->address = %"PRIx64"\n",
> -                dmaru->address);
> +        dprintk(VTDPREFIX, "  dmaru->address = %"PRIx64"\n", dmaru->address);

Ditto.
>  
>      ret = iommu_alloc(dmaru);
>      if ( ret )
> @@ -541,8 +542,8 @@ acpi_parse_one_drhd(struct acpi_dmar_header *header)
>              if ( !pci_device_detect(drhd->segment, b, d, f) )
>              {
>                  dprintk(XENLOG_WARNING VTDPREFIX,
> -                        " Non-existent device (%04x:%02x:%02x.%u) is reported"
> -                        " in this DRHD's scope!\n", drhd->segment, b, d, f);
> +                        " Non-existent device (%04x:%02x:%02x.%u) is reported in this DRHD's scope!\n",
> +                        drhd->segment, b, d, f);

As well - an seperate cleanup patch.
>                  invalid_cnt++;
>              }
>          }
> @@ -553,8 +554,8 @@ acpi_parse_one_drhd(struct acpi_dmar_header *header)
>                   invalid_cnt == dmaru->scope.devices_cnt )
>              {
>                  dprintk(XENLOG_WARNING VTDPREFIX,
> -                    "  Workaround BIOS bug: ignore the DRHD due to all "
> -                    "devices under its scope are not PCI discoverable!\n");
> +                        "  Workaround BIOS bug: ignore the DRHD due to all "
> +                        "devices under its scope are not PCI discoverable!\n");

This one too.
>  
>                  scope_devices_free(&dmaru->scope);
>                  iommu_free(dmaru);
> @@ -563,10 +564,10 @@ acpi_parse_one_drhd(struct acpi_dmar_header *header)
>              else
>              {
>                  dprintk(XENLOG_WARNING VTDPREFIX,
> -                    "  The DRHD is invalid due to there are devices under "
> -                    "its scope are not PCI discoverable! Pls try option "
> -                    "iommu=force or iommu=workaround_bios_bug if you "
> -                    "really want VT-d\n");
> +                        "  The DRHD is invalid due to there are devices under "
> +                        "its scope are not PCI discoverable! Pls try option "
> +                        "iommu=force or iommu=workaround_bios_bug if you "
> +                        "really want VT-d\n");

And that as well.
>                  ret = -EINVAL;
>              }
>          }
> @@ -604,8 +605,7 @@ static int register_one_rmrr(struct acpi_rmrr_unit *rmrru)
>          if ( pci_device_detect(rmrru->segment, b, d, f) == 0 )
>          {
>              dprintk(XENLOG_WARNING VTDPREFIX,
> -                    " Non-existent device (%04x:%02x:%02x.%u) is reported"
> -                    " in RMRR (%"PRIx64", %"PRIx64")'s scope!\n",
> +                    " Non-existent device (%04x:%02x:%02x.%u) is reported in RMRR "PRI_RMRR(s,e)"'s scope!\n",
>                      rmrru->segment, b, d, f,
>                      rmrru->base_address, rmrru->end_address);
>              ignore = 1;
> @@ -620,16 +620,16 @@ static int register_one_rmrr(struct acpi_rmrr_unit *rmrru)
>      if ( ignore )
>      {
>          dprintk(XENLOG_WARNING VTDPREFIX,
> -                "  Ignore the RMRR (%"PRIx64", %"PRIx64") due to "
> +                "  Ignore the RMRR "PRI_RMRR(s,e)" due to "
>                  "devices under its scope are not PCI discoverable!\n",
> -            rmrru->base_address, rmrru->end_address);
> +                rmrru->base_address, rmrru->end_address);
>          scope_devices_free(&rmrru->scope);
>          xfree(rmrru);
>      }
>      else if ( rmrru->base_address > rmrru->end_address )
>      {
>          dprintk(XENLOG_WARNING VTDPREFIX,
> -                "  The RMRR (%"PRIx64", %"PRIx64") is incorrect!\n",
> +                "  The RMRR "PRI_RMRR(s,e)" is incorrect!\n",
>                  rmrru->base_address, rmrru->end_address);
>          scope_devices_free(&rmrru->scope);
>          xfree(rmrru);
> @@ -639,7 +639,7 @@ static int register_one_rmrr(struct acpi_rmrr_unit *rmrru)
>      {
>          if ( iommu_verbose )
>              dprintk(VTDPREFIX,
> -                    "  RMRR region: base_addr %"PRIx64" end_address %"PRIx64"\n",
> +                    "  RMRR region: "PRI_RMRR(s,e)"\n",
>                      rmrru->base_address, rmrru->end_address);
>          acpi_register_rmrr_unit(rmrru);
>      }
> @@ -664,7 +664,7 @@ acpi_parse_one_rmrr(struct acpi_dmar_header *header)
>         if ( base_addr <= rmrru->end_address && rmrru->base_address <= end_addr )
>         {
>             printk(XENLOG_ERR VTDPREFIX
> -                  "Overlapping RMRRs [%"PRIx64",%"PRIx64"] and [%"PRIx64",%"PRIx64"]\n",
> +                  "Overlapping RMRRs "PRI_RMRR(s,e)" and "PRI_RMRR(s,e)"\n",
>                    rmrru->base_address, rmrru->end_address,
>                    base_addr, end_addr);
>             return -EEXIST;
> @@ -678,8 +678,7 @@ acpi_parse_one_rmrr(struct acpi_dmar_header *header)
>           (!page_is_ram_type(paddr_to_pfn(end_addr), RAM_TYPE_RESERVED)) )
>      {
>          dprintk(XENLOG_WARNING VTDPREFIX,
> -                "  RMRR address range not in reserved memory "
> -                "base = %"PRIx64" end = %"PRIx64"; "
> +                "  RMRR address range "PRI_RMRR(s,e)" not in reserved memory; "
>                  "iommu_inclusive_mapping=1 parameter may be needed.\n",
>                  base_addr, end_addr);
>      }
> @@ -779,8 +778,7 @@ acpi_parse_one_rhsa(struct acpi_dmar_header *header)
>      list_add_tail(&rhsau->list, &acpi_rhsa_units);
>      if ( iommu_verbose )
>          dprintk(VTDPREFIX,
> -                "  rhsau->address: %"PRIx64
> -                " rhsau->proximity_domain: %"PRIx32"\n",
> +                "  rhsau->address: %"PRIx64" rhsau->proximity_domain: %"PRIx32"\n",

All of those should go in a seperate cleanup patch please.

>                  rhsau->address, rhsau->proximity_domain);
>  
>      return ret;
> @@ -869,6 +867,140 @@ out:
>      return ret;
>  }
>  
> +#define MAX_EXTRA_RMRR_PAGES 16
> +#define MAX_EXTRA_RMRR 10
> +
> +/* RMRR units derived from command line rmrr option */

Missing full stop.

> +#define MAX_EXTRA_RMRR_DEV 20
> +struct extra_rmrr_unit {
> +    struct list_head list;
> +    unsigned long base_pfn, end_pfn;
> +    unsigned int dev_count;
> +    u32    sbdf[MAX_EXTRA_RMRR_DEV];
> +};
> +static __initdata unsigned int nr_rmrr;
> +static struct __initdata extra_rmrr_unit extra_rmrr_units[MAX_EXTRA_RMRR];
> +
> +/* Macro for RMRR inclusive range formatting */

Macro? Ah I think you meant the:
#define PRI_RMRR(s,e) "[%lx-%lx]"

which is defined somewhere at the top of the file with your patch. Please
move it here.

Also, missing full stop at the end of the line.
> +
> +static void __init add_extra_rmrr(void)
> +{
> +    struct acpi_rmrr_unit *acpi_rmrr;
> +    struct acpi_rmrr_unit *rmrru;
> +    unsigned int dev, seg, i, j;
> +    unsigned long pfn;
> +
> +    for ( i = 0; i < nr_rmrr; i++ )
> +    {
> +        if ( extra_rmrr_units[i].base_pfn > extra_rmrr_units[i].end_pfn )
> +        {
> +            printk(XENLOG_ERR VTDPREFIX
> +                   "Invalid RMRR Range "PRI_RMRR(s,e)"\n",
> +                   extra_rmrr_units[i].base_pfn, extra_rmrr_units[i].end_pfn);
> +            continue;
> +        }
> +
> +        if ( extra_rmrr_units[i].end_pfn - extra_rmrr_units[i].base_pfn >=
> +             MAX_EXTRA_RMRR_PAGES )
> +        {
> +            printk(XENLOG_ERR VTDPREFIX
> +                   "RMRR range "PRI_RMRR(s,e)" exceeds "__stringify(MAX_EXTRA_RMRR_PAGES)" pages\n",
> +                   extra_rmrr_units[i].base_pfn, extra_rmrr_units[i].end_pfn);
> +            continue;
> +        }
> +
> +        for ( j = 0; j < nr_rmrr; j++ )
> +        {
> +            if ( i != j &&
> +                 extra_rmrr_units[i].base_pfn <= extra_rmrr_units[j].end_pfn &&
> +                 extra_rmrr_units[j].base_pfn <= extra_rmrr_units[i].end_pfn )
> +            {
> +                printk(XENLOG_ERR VTDPREFIX
> +                      "Overlapping RMRRs "PRI_RMRR(s,e)" and "PRI_RMRR(s,e)"\n",
> +                      extra_rmrr_units[i].base_pfn, extra_rmrr_units[i].end_pfn,
> +                      extra_rmrr_units[j].base_pfn, extra_rmrr_units[j].end_pfn);
> +                break;
> +            }
> +        }
> +        /* Broke out of the overlap loop check, continue with next rmrr. */
> +        if ( j < nr_rmrr )
> +            continue;
> +        list_for_each_entry(rmrru, &acpi_rmrr_units, list)
> +        {
> +            if ( pfn_to_paddr(extra_rmrr_units[i].base_pfn <= rmrru->end_address) &&
> +                 rmrru->base_address <= pfn_to_paddr(extra_rmrr_units[i].end_pfn) )
> +            {
> +                printk(XENLOG_ERR VTDPREFIX
> +                       "Overlapping extra RMRRs "PRI_RMRR(s,e)" and ACPI RMRRs "PRI_RMRR(s,e)"\n",
> +                       extra_rmrr_units[i].base_pfn,
> +                       extra_rmrr_units[i].end_pfn,
> +                       paddr_to_pfn(rmrru->base_address),
> +                       paddr_to_pfn(rmrru->end_address));
> +                break;

You break out of the for loop (list_for_each_entry) - did ..
> +            }
> +        }

... you also want to continue with the next rmrr? Right now the code
will continue on with this 'i'.

> +
> +
> +        pfn = extra_rmrr_units[i].base_pfn;
> +        do
> +        {
> +            if ( !mfn_valid(pfn) || (pfn >> (paddr_bits - PAGE_SHIFT)) )
> +            {
> +                if ( iommu_verbose )

Why the 'iommu_verbose' ? Why not just print it out as all the other errors
are printed?

> +                    printk(XENLOG_ERR VTDPREFIX
> +                           "Invalid mfn in RMRR range "PRI_RMRR(s,e)"\n",
> +                           extra_rmrr_units[i].base_pfn, extra_rmrr_units[i].end_pfn);
> +                break;
> +            }
> +        } while ( pfn++ <= extra_rmrr_units[i].end_pfn );
> +        /* The range had invalid mfn as the loop was broken out before reaching end_pfn. */
> +        if ( pfn <= extra_rmrr_units[i].end_pfn )
> +            continue;
> +
> +        acpi_rmrr = xzalloc(struct acpi_rmrr_unit);
> +        if ( !acpi_rmrr )
> +            return;
> +
> +        acpi_rmrr->scope.devices = xmalloc_array(u16,
> +                                                 extra_rmrr_units[i].dev_count);
> +        if ( !acpi_rmrr->scope.devices )
> +        {
> +            xfree(acpi_rmrr);
> +            return;
> +        }
> +
> +        seg = 0;
> +        for ( dev = 0; dev < extra_rmrr_units[i].dev_count; dev++ )
> +        {
> +            acpi_rmrr->scope.devices[dev] = extra_rmrr_units[i].sbdf[dev];
> +            seg = seg | PCI_SEG(extra_rmrr_units[i].sbdf[dev]);
> +        }
> +        if ( seg != PCI_SEG(extra_rmrr_units[i].sbdf[0]) )
> +        {
> +            printk(XENLOG_ERR VTDPREFIX
> +                   "Segments are not equal for RMRR range "PRI_RMRR(s,e)"\n",
> +                   extra_rmrr_units[i].base_pfn, extra_rmrr_units[i].end_pfn);
> +            scope_devices_free(&acpi_rmrr->scope);
> +            xfree(acpi_rmrr);
> +            continue;
> +        }
> +
> +        acpi_rmrr->segment = seg;
> +        acpi_rmrr->base_address = pfn_to_paddr(extra_rmrr_units[i].base_pfn);
> +        acpi_rmrr->end_address = pfn_to_paddr(extra_rmrr_units[i].end_pfn + 1);
> +        acpi_rmrr->scope.devices_cnt = extra_rmrr_units[i].dev_count;
> +
> +        if ( register_one_rmrr(acpi_rmrr) )
> +        {
> +            printk(XENLOG_ERR VTDPREFIX
> +                   "Could not register RMMR range "PRI_RMRR(s,e)"\n",
> +                   extra_rmrr_units[i].base_pfn, extra_rmrr_units[i].end_pfn);
> +            scope_devices_free(&acpi_rmrr->scope);
> +            xfree(acpi_rmrr);
> +        }
> +    }
> +}
> +
>  #include <asm/tboot.h>
>  /* ACPI tables may not be DMA protected by tboot, so use DMAR copy */
>  /* SINIT saved in SinitMleData in TXT heap (which is DMA protected) */
> @@ -878,6 +1010,7 @@ int __init acpi_dmar_init(void)
>  {
>      acpi_physical_address dmar_addr;
>      acpi_native_uint dmar_len;
> +    int ret;
>  
>      if ( ACPI_SUCCESS(acpi_get_table_phys(ACPI_SIG_DMAR, 0,
>                                            &dmar_addr, &dmar_len)) )
> @@ -888,7 +1021,10 @@ int __init acpi_dmar_init(void)
>          dmar_table = __va(dmar_addr);
>      }
>  
> -    return parse_dmar_table(acpi_parse_dmar);
> +    ret = parse_dmar_table(acpi_parse_dmar);
> +    add_extra_rmrr();
> +
> +    return ret;
>  }
>  
>  void acpi_dmar_reinstate(void)
> @@ -919,3 +1055,67 @@ int platform_supports_x2apic(void)
>      unsigned int mask = ACPI_DMAR_INTR_REMAP | ACPI_DMAR_X2APIC_OPT_OUT;
>      return cpu_has_x2apic && ((dmar_flags & mask) == ACPI_DMAR_INTR_REMAP);
>  }
> +
> +/*
> + * Parse rmrr Xen command line options and add parsed device and region into
> + * acpi_rmrr_unit list to mapped as RMRRs parsed from ACPI.
> + * Format:
> + * rmrr=start<-end>=[s1]bdf1[,[s1]bdf2[,...]];start<-end>=[s2]bdf1[,[s2]bdf2[,...]]
> + * If the segment of the first device is not specified, segment zero will be used.
> + * If other segments are not specified, first device segment will be used.
> + * If a segment is specified for other than the first device and it does not match
> + * the one specified for the first one, an error will be reported.
> + */
> +static void __init parse_rmrr_param(const char *str)
> +{
> +    const char *s = str, *cur, *stmp;
> +    unsigned int seg, bus, dev, func;
> +    unsigned long start, end;
> +
> +    do {
> +        start = simple_strtoul(cur = s, &s, 0);
> +        if ( cur == s )
> +            break;
> +
> +        if ( *s == '-' )
> +        {
> +            end = simple_strtoul(cur = s + 1, &s, 0);
> +            if ( cur == s )
> +                break;
> +        }
> +        else
> +            end = start;
> +
> +        extra_rmrr_units[nr_rmrr].base_pfn = start;
> +        extra_rmrr_units[nr_rmrr].end_pfn = end;
> +        extra_rmrr_units[nr_rmrr].dev_count = 0;
> +
> +        if ( *s != '=' )
> +            continue;
> +
> +        do {
> +            bool_t default_segment = 0;
> +
> +            if ( *s == ';' )
> +                break;
> +            stmp = __parse_pci(s + 1, &seg, &bus, &dev, &func, &default_segment);
> +            if ( !stmp )
> +                break;
> +
> +            /* Not specified segment will be replaced with one from first device. */
> +            if ( extra_rmrr_units[nr_rmrr].dev_count && default_segment )
> +                seg = PCI_SEG(extra_rmrr_units[nr_rmrr].sbdf[0]);
> +
> +            /* Keep sbdf's even if they differ and later report an error. */
> +            extra_rmrr_units[nr_rmrr].sbdf[extra_rmrr_units[nr_rmrr].dev_count] = PCI_SBDF(seg, bus, dev, func);
> +            extra_rmrr_units[nr_rmrr].dev_count++;
> +            s = stmp;
> +        } while ( *s == ',' &&
> +                  extra_rmrr_units[nr_rmrr].dev_count < MAX_EXTRA_RMRR_DEV );
> +
> +        if ( extra_rmrr_units[nr_rmrr].dev_count )
> +            nr_rmrr++;
> +
> +    } while ( *s++ == ';' && nr_rmrr < MAX_EXTRA_RMRR );
> +}
> +custom_param("rmrr", parse_rmrr_param);


Otherwise looks fine!
> -- 
> 2.1.3
>