[PATCH v2 0/2] iommu: add rmrr Xen command line option

[PATCH v2 0/2] iommu: add rmrr Xen command line option

[elena.ufimtseva]

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

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 v2:                                                                  
 - move rmrr parser to dmar.c and make it custom_param;                         
 - change of rmrr command line oprion format; since adding multiple device
 per range support needs to utilize more special characters and offered from
 the previous review ';' is not supported, '[' ']' are reserved, ':' and used in pci
 format, range and devices are separated by '#'; Suggestions are welcome;  
 - added support for multiple devices per range;                                
 - moved adding misc RMRRs before ACPI RMRR parsing;                            
 - make parser fail if pci device is specified incorrectly; 

Elena Ufimtseva (2):
  iommu VT-d: separate rmrr addition function
  iommu: add rmrr Xen command line option for misc rmrrs

 docs/misc/xen-command-line.markdown |   8 ++
 xen/drivers/passthrough/vtd/dmar.c  | 239 +++++++++++++++++++++++++++---------
 xen/drivers/passthrough/vtd/dmar.h  |  11 ++
 3 files changed, 199 insertions(+), 59 deletions(-)

-- 
2.1.3

[PATCH v2 1/2] 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.
No functional changes.

Signed-off-by: Elena Ufimtseva <elena.ufimtseva@oracle.com>
---
 xen/drivers/passthrough/vtd/dmar.c | 125 ++++++++++++++++++++-----------------
 1 file changed, 66 insertions(+), 59 deletions(-)

diff --git a/xen/drivers/passthrough/vtd/dmar.c b/xen/drivers/passthrough/vtd/dmar.c
index 1152c3a..979fac1 100644
--- a/xen/drivers/passthrough/vtd/dmar.c
+++ b/xen/drivers/passthrough/vtd/dmar.c
@@ -567,6 +567,69 @@ 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, d, f;
+
+        b = d = f = 0;
+        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(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);
+        ret = -EFAULT;
+    }
+    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);
+        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)
 {
@@ -617,66 +680,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);
-    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) == 0 )
-            {
-                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);
-            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);
+    if ( !ret && (rmrru->scope.devices_cnt != 0) )
+        ret = register_one_rmrr(rmrru);
+        if ( ret )
             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;
 }
-- 
2.1.3

[PATCH v2 2/2] iommu: add rmrr Xen command line option for misc 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>=sbdf1[,sbdf2[,...]]#start<-end>=sbdf1[,sbdf2[,...]]

Signed-off-by: Elena Ufimtseva <elena.ufimtseva@oracle.com>
---
 docs/misc/xen-command-line.markdown |   8 +++
 xen/drivers/passthrough/vtd/dmar.c  | 114 ++++++++++++++++++++++++++++++++++++
 xen/drivers/passthrough/vtd/dmar.h  |  11 ++++
 3 files changed, 133 insertions(+)

diff --git a/docs/misc/xen-command-line.markdown b/docs/misc/xen-command-line.markdown
index 1dda1f0..92aefdb 100644
--- a/docs/misc/xen-command-line.markdown
+++ b/docs/misc/xen-command-line.markdown
@@ -1170,6 +1170,14 @@ 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>=sbdf1[,sbdf2[,...]]#start<-end>=sbdf1[,sbdf2[,...]]
+
+Define RMRRs 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 end and start
+are specified.
+
 ### ro-hpet
 > `= <boolean>`
 
diff --git a/xen/drivers/passthrough/vtd/dmar.c b/xen/drivers/passthrough/vtd/dmar.c
index 979fac1..e8e043b 100644
--- a/xen/drivers/passthrough/vtd/dmar.c
+++ b/xen/drivers/passthrough/vtd/dmar.c
@@ -49,6 +49,7 @@ static LIST_HEAD_READ_MOSTLY(acpi_rhsa_units);
 static struct acpi_table_header *__read_mostly dmar_table;
 static int __read_mostly dmar_flags;
 static u64 __read_mostly igd_drhd_address;
+static void __init add_misc_rmrr(void);
 
 static void __init dmar_scope_add_buses(struct dmar_scope *scope, u16 sec_bus,
                                         u16 sub_bus)
@@ -868,6 +869,7 @@ int __init acpi_dmar_init(void)
                          PAGE_HYPERVISOR);
         dmar_table = __va(dmar_addr);
     }
+    add_misc_rmrr();
 
     return parse_dmar_table(acpi_parse_dmar);
 }
@@ -900,3 +902,115 @@ 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 apci_rmrr_unit list to mapped
+ * as RMRRs parsed from ACPI.
+ * Format rmrr=start<-end>=sbdf1[,sbdf2[,...]]#start<-end>=sbdf1[,sbdf2[,...]]
+ * end address can be ommited and one page will be used
+ * for mapping with start pfn.
+ */
+
+#define MAX_MISC_RMRR 10
+__initdata LIST_HEAD(misc_rmrr_units);
+__initdata unsigned int nr_rmrr = 0;
+struct __initdata misc_rmrr_unit rmrru[MAX_MISC_RMRR];
+
+static void __init parse_rmrr_param(const char *str)
+{
+    unsigned int seg, bus, dev, func;
+    const char *s = str, *cur, *stmp;
+    unsigned int i = 0, rmrrs = 0;
+    u64 start, end;
+
+    do {
+        start = simple_strtoull(cur = s, &s, 0);
+        if ( cur == s )
+            break;
+
+        if ( *s == '-' )
+        {
+            end = simple_strtoull(cur = s + 1, &s, 0);
+            if ( cur == s )
+                break;
+        }
+        else
+            end = start;
+        if ( end >= start && rmrrs < MAX_MISC_RMRR )
+        {
+            rmrru[i].base_address = start << PAGE_SHIFT;
+            rmrru[i].end_address = (end + 1) << PAGE_SHIFT;
+            rmrru[i].dev_count = 0;
+        }
+        else
+        {
+            printk(XENLOG_WARNING "Bad rmrr: start > end, %"PRIx64" > %"PRIx64"\n",
+                   start, end);
+            break;
+        }
+
+        if ( *s != '=' )
+            continue;
+
+        do {
+            if ( rmrru[i].dev_count >= MAX_MISC_RMRR_DEV )
+                break;
+            if ( *s == '#' )
+                break;
+            seg = bus = dev = func = 0;
+            stmp = parse_pci(s + 1, &seg, &bus, &dev, &func);
+            if ( !stmp )
+                break;
+            rmrru[i].devices[rmrru[i].dev_count++] = PCI_BDF(bus, dev, func);
+            rmrru[i].segment = seg;
+            s = stmp;
+        } while ( *s == ',' );
+
+        if ( rmrru[i].dev_count ) {
+            list_add(&rmrru[i].list, &misc_rmrr_units);
+            i++;
+        }
+        rmrrs++;
+
+    } while ( *s++ == '#' );
+
+    nr_rmrr = i;
+}
+custom_param("rmrr", parse_rmrr_param);
+
+static void __init add_misc_rmrr(void)
+{
+    struct acpi_rmrr_unit *rmrrn;
+    struct misc_rmrr_unit *rmrru, *r;
+
+    list_for_each_entry_safe( rmrru, r, &misc_rmrr_units, list )
+    {
+        rmrrn = xzalloc(struct acpi_rmrr_unit);
+        if ( !rmrrn )
+            goto del;
+
+        rmrrn->scope.devices = xzalloc(typeof(*rmrrn->scope.devices));
+        if ( !rmrrn->scope.devices )
+        {
+            xfree(rmrrn);
+            goto del;
+        }
+        rmrrn->segment = rmrru->segment;
+        rmrrn->base_address = rmrru->base_address;
+        rmrrn->end_address = rmrru->end_address;
+
+        for (int dev = 0; dev < rmrru->dev_count; dev++)
+            rmrrn->scope.devices[dev] = rmrru->devices[dev];
+
+        rmrrn->scope.devices_cnt = rmrru->dev_count;
+
+        if ( register_one_rmrr(rmrrn) )
+        {
+            xfree(rmrrn->scope.devices);
+            xfree(rmrrn);
+        }
+ del:
+        list_del(&rmrru->list);
+    }
+}
diff --git a/xen/drivers/passthrough/vtd/dmar.h b/xen/drivers/passthrough/vtd/dmar.h
index af1feef..99a20ed 100644
--- a/xen/drivers/passthrough/vtd/dmar.h
+++ b/xen/drivers/passthrough/vtd/dmar.h
@@ -132,4 +132,15 @@ void disable_pmr(struct iommu *iommu);
 int is_usb_device(u16 seg, u8 bus, u8 devfn);
 int is_igd_drhd(struct acpi_drhd_unit *drhd);
 
+/*RMRR units derived from command line rmrr option */
+#define MAX_MISC_RMRR_DEV 20
+struct misc_rmrr_unit {
+    struct list_head list;
+    u64    base_address;
+    u64    end_address;
+    u16    segment;
+    u16    devices[MAX_MISC_RMRR_DEV];
+    u16    dev_count;
+};
+
 #endif /* _DMAR_H_ */
-- 
2.1.3

Re: [PATCH v2 0/2] iommu: add rmrr Xen command line option

[Jan Beulich]

>>> On 24.03.15 at 17:08, <elena.ufimtseva@oracle.com> wrote:
> Changes in v2:                                                               
>    
>  - move rmrr parser to dmar.c and make it custom_param;                       
>   
>  - change of rmrr command line oprion format; since adding multiple device
>  per range support needs to utilize more special characters and offered from
>  the previous review ';' is not supported, '[' ']' are reserved, ':' and 
> used in pci
>  format, range and devices are separated by '#'; Suggestions are welcome;  

What is it that makes ';' not supported?

Jan

Re: [PATCH v2 0/2] iommu: add rmrr Xen command line option

[Elena Ufimtseva]

On Tue, Mar 24, 2015 at 04:19:24PM +0000, Jan Beulich wrote:
> >>> On 24.03.15 at 17:08, <elena.ufimtseva@oracle.com> wrote:
> > Changes in v2:                                                               
> >    
> >  - move rmrr parser to dmar.c and make it custom_param;                       
> >   
> >  - change of rmrr command line oprion format; since adding multiple device
> >  per range support needs to utilize more special characters and offered from
> >  the previous review ';' is not supported, '[' ']' are reserved, ':' and 
> > used in pci
> >  format, range and devices are separated by '#'; Suggestions are welcome;  
> 
> What is it that makes ';' not supported?

Grub interprets the string after ; as an environment variable,
so I get this with version 2.02~beta2-15:

This is when there is ';' before second range 0xd5d45-0xd5d46.

  Booting a command list

  error: invalid variable name `0xd5d45-0xd5d46'.
  Loading Linux 3.1X.X-rcX
  Loading initial ramdisk ...

> 
> Jan
> 

Re: [PATCH v2 0/2] iommu: add rmrr Xen command line option

[Jan Beulich]

>>> On 24.03.15 at 19:54, <elena.ufimtseva@oracle.com> wrote:
> On Tue, Mar 24, 2015 at 04:19:24PM +0000, Jan Beulich wrote:
>> >>> On 24.03.15 at 17:08, <elena.ufimtseva@oracle.com> wrote:
>> > Changes in v2:                                                              
>  
>> >    
>> >  - move rmrr parser to dmar.c and make it custom_param;                      
>  
>> >   
>> >  - change of rmrr command line oprion format; since adding multiple device
>> >  per range support needs to utilize more special characters and offered 
> from
>> >  the previous review ';' is not supported, '[' ']' are reserved, ':' and 
>> > used in pci
>> >  format, range and devices are separated by '#'; Suggestions are welcome;  
>> 
>> What is it that makes ';' not supported?
> 
> Grub interprets the string after ; as an environment variable,
> so I get this with version 2.02~beta2-15:
> 
> This is when there is ';' before second range 0xd5d45-0xd5d46.
> 
>   Booting a command list
> 
>   error: invalid variable name `0xd5d45-0xd5d46'.

To be honest I'm not sure we want to work around grub bugs (or
should I call it features) like this. Grub should be fixed instead. At
the very least ';' should remain the primary separator here, and a
secondary one might be supported to work around this grub quirk.
Apart from that - is there anything else that's broken, or is the
presence of the error message just a cosmetic issue?

Jan

Re: [PATCH v2 0/2] iommu: add rmrr Xen command line option

[Elena Ufimtseva]

On Wed, Mar 25, 2015 at 10:08:20AM +0000, Jan Beulich wrote:
> >>> On 24.03.15 at 19:54, <elena.ufimtseva@oracle.com> wrote:
> > On Tue, Mar 24, 2015 at 04:19:24PM +0000, Jan Beulich wrote:
> >> >>> On 24.03.15 at 17:08, <elena.ufimtseva@oracle.com> wrote:
> >> > Changes in v2:                                                              
> >  
> >> >    
> >> >  - move rmrr parser to dmar.c and make it custom_param;                      
> >  
> >> >   
> >> >  - change of rmrr command line oprion format; since adding multiple device
> >> >  per range support needs to utilize more special characters and offered 
> > from
> >> >  the previous review ';' is not supported, '[' ']' are reserved, ':' and 
> >> > used in pci
> >> >  format, range and devices are separated by '#'; Suggestions are welcome;  
> >> 
> >> What is it that makes ';' not supported?
> > 
> > Grub interprets the string after ; as an environment variable,
> > so I get this with version 2.02~beta2-15:
> > 
> > This is when there is ';' before second range 0xd5d45-0xd5d46.
> > 
> >   Booting a command list
> > 
> >   error: invalid variable name `0xd5d45-0xd5d46'.
> 
> To be honest I'm not sure we want to work around grub bugs (or
> should I call it features) like this. Grub should be fixed instead. At
> the very least ';' should remain the primary separator here, and a
> secondary one might be supported to work around this grub quirk.
> Apart from that - is there anything else that's broken, or is the
> presence of the error message just a cosmetic issue?
>
Hi Jan                                                                          
Looks like ';' is a valid syntax reserved character for grub, see
section       
5.2:                                                                            
http://www.gnu.org/software/grub/manual/grub.html                               
Along the messages, the part of the string after ';' is ignored.                
Does not look its a bug, but rather a feature.

> Jan
> 

Re: [PATCH v2 0/2] iommu: add rmrr Xen command line option

[Jan Beulich]

>>> On 25.03.15 at 11:32, <elena.ufimtseva@oracle.com> wrote:
> Looks like ';' is a valid syntax reserved character for grub, see
> section       
> 5.2:                                                                         
>    
> http://www.gnu.org/software/grub/manual/grub.html                            

But the same section says how to escape/quote such special
characters. I.e. even less of a reason to using something odd
like the # you suggested.

Jan

Re: [PATCH v2 0/2] iommu: add rmrr Xen command line option

[Elena Ufimtseva]

On Wed, Mar 25, 2015 at 10:39:36AM +0000, Jan Beulich wrote:
> >>> On 25.03.15 at 11:32, <elena.ufimtseva@oracle.com> wrote:
> > Looks like ';' is a valid syntax reserved character for grub, see
> > section       
> > 5.2:                                                                         
> >    
> > http://www.gnu.org/software/grub/manual/grub.html                            
> 
> But the same section says how to escape/quote such special
> characters. I.e. even less of a reason to using something odd
> like the # you suggested.

Saw it and thought its not a good option.
Will be that ok if I specify in doc that the option should be quoted/escaped?

> 
> Jan
> 

Re: [PATCH v2 0/2] iommu: add rmrr Xen command line option

[Jan Beulich]

>>> On 25.03.15 at 11:44, <elena.ufimtseva@oracle.com> wrote:
> On Wed, Mar 25, 2015 at 10:39:36AM +0000, Jan Beulich wrote:
>> >>> On 25.03.15 at 11:32, <elena.ufimtseva@oracle.com> wrote:
>> > Looks like ';' is a valid syntax reserved character for grub, see
>> > section       
>> > 5.2:                                                                        
>  
>> >    
>> > http://www.gnu.org/software/grub/manual/grub.html                           
>  
>> 
>> But the same section says how to escape/quote such special
>> characters. I.e. even less of a reason to using something odd
>> like the # you suggested.
> 
> Saw it and thought its not a good option.
> Will be that ok if I specify in doc that the option should be 
> quoted/escaped?

Yes, that's what you should do.

Jan

Re: [PATCH v2 0/2] iommu: add rmrr Xen command line option

[Elena Ufimtseva]

On Wed, Mar 25, 2015 at 10:57:17AM +0000, Jan Beulich wrote:
> >>> On 25.03.15 at 11:44, <elena.ufimtseva@oracle.com> wrote:
> > On Wed, Mar 25, 2015 at 10:39:36AM +0000, Jan Beulich wrote:
> >> >>> On 25.03.15 at 11:32, <elena.ufimtseva@oracle.com> wrote:
> >> > Looks like ';' is a valid syntax reserved character for grub, see
> >> > section       
> >> > 5.2:                                                                        
> >  
> >> >    
> >> > http://www.gnu.org/software/grub/manual/grub.html                           
> >  
> >> 
> >> But the same section says how to escape/quote such special
> >> characters. I.e. even less of a reason to using something odd
> >> like the # you suggested.
> > 
> > Saw it and thought its not a good option.
> > Will be that ok if I specify in doc that the option should be 
> > quoted/escaped?
> 
> Yes, that's what you should do.

Great! Thanks Jan. Will repost.

> 
> Jan
> 

Re: [PATCH v2 1/2] iommu VT-d: separate rmrr addition function

[Jan Beulich]

>>> On 24.03.15 at 17:08, <elena.ufimtseva@oracle.com> wrote:
> +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, d, f;
> +
> +        b = d = f = 0;

???

> +        b = PCI_BUS(rmrru->scope.devices[i]);
> +        d = PCI_SLOT(rmrru->scope.devices[i]);
> +        f = PCI_FUNC(rmrru->scope.devices[i]);

Quoting my reply on v1: "Please make this the declarations (with
initializers) of these variables; they don't appear to be used outside
this scope." I.e.

u8 b = PCI_BUS(...);

> +    if ( !ret && (rmrru->scope.devices_cnt != 0) )
> +        ret = register_one_rmrr(rmrru);
> +        if ( ret )
>              xfree(rmrru);

Indentation is screwed up here.

Jan

Re: [PATCH v2 1/2] iommu VT-d: separate rmrr addition function

[Elena Ufimtseva]

On Wed, Mar 25, 2015 at 05:04:38PM +0000, Jan Beulich wrote:
> >>> On 24.03.15 at 17:08, <elena.ufimtseva@oracle.com> wrote:
> > +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, d, f;
> > +
> > +        b = d = f = 0;
> 
> ???
> 
> > +        b = PCI_BUS(rmrru->scope.devices[i]);
> > +        d = PCI_SLOT(rmrru->scope.devices[i]);
> > +        f = PCI_FUNC(rmrru->scope.devices[i]);
> 
> Quoting my reply on v1: "Please make this the declarations (with
> initializers) of these variables; they don't appear to be used outside
> this scope." I.e.
> 
> u8 b = PCI_BUS(...);

Got it, I did read it differently first time. Example helps for sure!

> 
> > +    if ( !ret && (rmrru->scope.devices_cnt != 0) )
> > +        ret = register_one_rmrr(rmrru);
> > +        if ( ret )
> >              xfree(rmrru);
> 
> Indentation is screwed up here.

Ok, will fix.

> 
> Jan
> 

Re: [PATCH v2 2/2] iommu: add rmrr Xen command line option for misc rmrrs

[Jan Beulich]

>>> On 24.03.15 at 17:08, <elena.ufimtseva@oracle.com> wrote:
> --- a/docs/misc/xen-command-line.markdown
> +++ b/docs/misc/xen-command-line.markdown
> @@ -1170,6 +1170,14 @@ 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>=sbdf1[,sbdf2[,...]]#start<-end>=sbdf1[,sbdf2[,...]]

> '= start<-end>=sbdf1[,bdf2[,...]];start<-end>=sbdf1[,bdf2[,...]]

Segments can't vary between multiple devices associated with a
single RMRR.

> --- a/xen/drivers/passthrough/vtd/dmar.c
> +++ b/xen/drivers/passthrough/vtd/dmar.c
> @@ -49,6 +49,7 @@ static LIST_HEAD_READ_MOSTLY(acpi_rhsa_units);
>  static struct acpi_table_header *__read_mostly dmar_table;
>  static int __read_mostly dmar_flags;
>  static u64 __read_mostly igd_drhd_address;
> +static void __init add_misc_rmrr(void);

Please simply put the function definition not at the end of the file,
thus avoiding the need for a separate declaration.

> @@ -900,3 +902,115 @@ 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 apci_rmrr_unit list to mapped
> + * as RMRRs parsed from ACPI.
> + * Format rmrr=start<-end>=sbdf1[,sbdf2[,...]]#start<-end>=sbdf1[,sbdf2[,...]]
> + * end address can be ommited and one page will be used
> + * for mapping with start pfn.
> + */
> +
> +#define MAX_MISC_RMRR 10
> +__initdata LIST_HEAD(misc_rmrr_units);
> +__initdata unsigned int nr_rmrr = 0;
> +struct __initdata misc_rmrr_unit rmrru[MAX_MISC_RMRR];

All three static. The one zero initializer then also becomes pointless.

> +static void __init parse_rmrr_param(const char *str)
> +{
> +    unsigned int seg, bus, dev, func;
> +    const char *s = str, *cur, *stmp;
> +    unsigned int i = 0, rmrrs = 0;

Afaict you could easily do with just one of these two variables.

> +    u64 start, end;
> +
> +    do {
> +        start = simple_strtoull(cur = s, &s, 0);
> +        if ( cur == s )
> +            break;
> +
> +        if ( *s == '-' )
> +        {
> +            end = simple_strtoull(cur = s + 1, &s, 0);
> +            if ( cur == s )
> +                break;
> +        }
> +        else
> +            end = start;
> +        if ( end >= start && rmrrs < MAX_MISC_RMRR )
> +        {
> +            rmrru[i].base_address = start << PAGE_SHIFT;
> +            rmrru[i].end_address = (end + 1) << PAGE_SHIFT;
> +            rmrru[i].dev_count = 0;
> +        }
> +        else
> +        {
> +            printk(XENLOG_WARNING "Bad rmrr: start > end, %"PRIx64" > %"PRIx64"\n",
> +                   start, end);

This is misleading (as we may end up here due to the other half of the
if(). But a message logged from a command line parsing function isn't
really that useful anyway, so perhaps just drop it? Maybe instead
make add_misc_rmrr() more verbose, at least in iommu_verbose mode?
Also if you already validate the range, then please also make sure both
ends are valid physical addresses (may again need to be done in the
other function).

> +            break;
> +        }
> +
> +        if ( *s != '=' )
> +            continue;
> +
> +        do {
> +            if ( rmrru[i].dev_count >= MAX_MISC_RMRR_DEV )
> +                break;
> +            if ( *s == '#' )
> +                break;
> +            seg = bus = dev = func = 0;

Pointless initializers.

> +            stmp = parse_pci(s + 1, &seg, &bus, &dev, &func);
> +            if ( !stmp )
> +                break;
> +            rmrru[i].devices[rmrru[i].dev_count++] = PCI_BDF(bus, dev, func);
> +            rmrru[i].segment = seg;

As said above, this must be parsed only on the first device, or (to
keep the code here simple) verified to match what was specified on
the first one (which may require a small adjustment to parse_pci()
so that you can tell a default segment [currently zero] from one
explicitly specified as such).

> +            s = stmp;
> +        } while ( *s == ',' );
> +
> +        if ( rmrru[i].dev_count ) {

Coding style.

> +static void __init add_misc_rmrr(void)
> +{
> +    struct acpi_rmrr_unit *rmrrn;
> +    struct misc_rmrr_unit *rmrru, *r;
> +
> +    list_for_each_entry_safe( rmrru, r, &misc_rmrr_units, list )
> +    {
> +        rmrrn = xzalloc(struct acpi_rmrr_unit);
> +        if ( !rmrrn )
> +            goto del;
> +
> +        rmrrn->scope.devices = xzalloc(typeof(*rmrrn->scope.devices));

Why xzalloc() rather than xmalloc()? And why just a single array
element?

> +        if ( !rmrrn->scope.devices )
> +        {
> +            xfree(rmrrn);
> +            goto del;
> +        }
> +        rmrrn->segment = rmrru->segment;
> +        rmrrn->base_address = rmrru->base_address;
> +        rmrrn->end_address = rmrru->end_address;
> +
> +        for (int dev = 0; dev < rmrru->dev_count; dev++)

Coding style again. Also no initializers inside statements please (and
the variable should be of unsigned type).

> +            rmrrn->scope.devices[dev] = rmrru->devices[dev];

Actually - memcpy() for the whole array?

> +
> +        rmrrn->scope.devices_cnt = rmrru->dev_count;
> +
> +        if ( register_one_rmrr(rmrrn) )
> +        {
> +            xfree(rmrrn->scope.devices);
> +            xfree(rmrrn);
> +        }
> + del:
> +        list_del(&rmrru->list);

I don't think you need this, and then you also don't need the _safe
iterator.

> --- a/xen/drivers/passthrough/vtd/dmar.h
> +++ b/xen/drivers/passthrough/vtd/dmar.h
> @@ -132,4 +132,15 @@ void disable_pmr(struct iommu *iommu);
>  int is_usb_device(u16 seg, u8 bus, u8 devfn);
>  int is_igd_drhd(struct acpi_drhd_unit *drhd);
>  
> +/*RMRR units derived from command line rmrr option */
> +#define MAX_MISC_RMRR_DEV 20
> +struct misc_rmrr_unit {
> +    struct list_head list;
> +    u64    base_address;
> +    u64    end_address;
> +    u16    segment;
> +    u16    devices[MAX_MISC_RMRR_DEV];
> +    u16    dev_count;
> +};

This type isn't used outside of dmar.c, so I don't see why it gets
defined in a header.

Jan