Xen devel - Jul 2011 - [PATCH 1/3] x86-64/MMCFG: correct base address computation for regions not starting at bus 0

As per the specification, the base address reported by ACPI is the one
that would be used if the region started at bus 0. Hence the
start_bus_number offset needs to be added not only to the virtual
address, but also the physical one when establishing the mapping, and
it then needs to be subtracted when obtaining the virtual address for
doing accesses.

Signed-off-by: Jan Beulich <jbeulich@novell.com>

--- a/xen/arch/x86/x86_64/mmconfig_64.c
+++ b/xen/arch/x86/x86_64/mmconfig_64.c
@@ -25,7 +25,7 @@ struct mmcfg_virt {
 static struct mmcfg_virt *pci_mmcfg_virt;
 static int __initdata mmcfg_pci_segment_shift;
 
-static char __iomem *get_virt(unsigned int seg, unsigned bus)
+static char __iomem *get_virt(unsigned int seg, unsigned int *bus)
 {
     struct acpi_mcfg_allocation *cfg;
     int cfg_num;
@@ -33,9 +33,11 @@ static char __iomem *get_virt(unsigned i
     for (cfg_num = 0; cfg_num < pci_mmcfg_config_num; cfg_num++) {
         cfg = pci_mmcfg_virt[cfg_num].cfg;
         if (cfg->pci_segment == seg &&
-            (cfg->start_bus_number <= bus) &&
-            (cfg->end_bus_number >= bus))
+            (cfg->start_bus_number <= *bus) &&
+            (cfg->end_bus_number >= *bus)) {
+            *bus -= cfg->start_bus_number;
             return pci_mmcfg_virt[cfg_num].virt;
+        }
     }
 
     /* Fall back to type 0 */
@@ -46,7 +48,7 @@ static char __iomem *pci_dev_base(unsign
 {
     char __iomem *addr;
 
-    addr = get_virt(seg, bus);
+    addr = get_virt(seg, &bus);
     if (!addr)
         return NULL;
      return addr + ((bus << 20) | (devfn << 12));
@@ -121,8 +123,11 @@ static void __iomem * __init mcfg_iorema
     if (virt + size < virt || virt + size > PCI_MCFG_VIRT_END)
         return NULL;
 
-    map_pages_to_xen(virt, cfg->address >> PAGE_SHIFT,
-                     size >> PAGE_SHIFT, PAGE_HYPERVISOR_NOCACHE);
+    if (map_pages_to_xen(virt,
+                         (cfg->address >> PAGE_SHIFT) +
+                         (cfg->start_bus_number << (20 - PAGE_SHIFT)),
+                         size >> PAGE_SHIFT, PAGE_HYPERVISOR_NOCACHE))
+        return NULL;
 
     return (void __iomem *) virt;
 }





_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xensource.com
http://lists.xensource.com/xen-devel

Hi Jan,

I''m not sure if I understand the patch correctly...

My understanding of the flow for PCI MMCFG read case is
pci_mmcfg_read(...,bus,devfn,...)->pci_dev_base()->get_virt().  If you
modify "bus" in get_virt(), isn''t pci_mmcfg_read() going to
return the config space value of a different bus/devfn than originally intended?

Maybe we should just return -EINVAL if it is out of range.

Allen

-----Original Message-----
From: Jan Beulich [mailto:JBeulich@novell.com] 
Sent: Tuesday, July 19, 2011 1:42 AM
To: xen-devel@lists.xensource.com
Cc: Kay, Allen M
Subject: [PATCH 1/3] x86-64/MMCFG: correct base address computation for regions
not starting at bus 0

As per the specification, the base address reported by ACPI is the one that
would be used if the region started at bus 0. Hence the start_bus_number offset
needs to be added not only to the virtual address, but also the physical one
when establishing the mapping, and it then needs to be subtracted when obtaining
the virtual address for doing accesses.

Signed-off-by: Jan Beulich <jbeulich@novell.com>

--- a/xen/arch/x86/x86_64/mmconfig_64.c
+++ b/xen/arch/x86/x86_64/mmconfig_64.c
@@ -25,7 +25,7 @@ struct mmcfg_virt {
 static struct mmcfg_virt *pci_mmcfg_virt;  static int __initdata
mmcfg_pci_segment_shift;
 
-static char __iomem *get_virt(unsigned int seg, unsigned bus)
+static char __iomem *get_virt(unsigned int seg, unsigned int *bus)
 {
     struct acpi_mcfg_allocation *cfg;
     int cfg_num;
@@ -33,9 +33,11 @@ static char __iomem *get_virt(unsigned i
     for (cfg_num = 0; cfg_num < pci_mmcfg_config_num; cfg_num++) {
         cfg = pci_mmcfg_virt[cfg_num].cfg;
         if (cfg->pci_segment == seg &&
-            (cfg->start_bus_number <= bus) &&
-            (cfg->end_bus_number >= bus))
+            (cfg->start_bus_number <= *bus) &&
+            (cfg->end_bus_number >= *bus)) {
+            *bus -= cfg->start_bus_number;
             return pci_mmcfg_virt[cfg_num].virt;
+        }
     }
 
     /* Fall back to type 0 */
@@ -46,7 +48,7 @@ static char __iomem *pci_dev_base(unsign  {
     char __iomem *addr;
 
-    addr = get_virt(seg, bus);
+    addr = get_virt(seg, &bus);
     if (!addr)
         return NULL;
      return addr + ((bus << 20) | (devfn << 12)); @@ -121,8 +123,11
@@ static void __iomem * __init mcfg_iorema
     if (virt + size < virt || virt + size > PCI_MCFG_VIRT_END)
         return NULL;
 
-    map_pages_to_xen(virt, cfg->address >> PAGE_SHIFT,
-                     size >> PAGE_SHIFT, PAGE_HYPERVISOR_NOCACHE);
+    if (map_pages_to_xen(virt,
+                         (cfg->address >> PAGE_SHIFT) +
+                         (cfg->start_bus_number << (20 - PAGE_SHIFT)),
+                         size >> PAGE_SHIFT, PAGE_HYPERVISOR_NOCACHE))
+        return NULL;
 
     return (void __iomem *) virt;
 }




_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xensource.com
http://lists.xensource.com/xen-devel

>>> On 20.07.11 at 22:33, "Kay, Allen M"
<allen.m.kay@intel.com> wrote:
> Hi Jan,
> 
> I''m not sure if I understand the patch correctly...
> 
> My understanding of the flow for PCI MMCFG read case is 
> pci_mmcfg_read(...,bus,devfn,...)->pci_dev_base()->get_virt().  If
you modify
> "bus" in get_virt(), isn''t pci_mmcfg_read() going to
return the config space
> value of a different bus/devfn than originally intended?
No, as said in the patch description, the modification of "bus" in
get_virt()
is to compensate for the adjustment when establishing the mapping (in
mmcfg_ioremap()), which now accounts for start_bus_number.

The problem addressed by this patch is that the base address read from
the ACPI structure is referring to what would be bus 0 (even if bus 0
isn''t included in the descriptor''s bus range). Just look at
the respective
Linux code for another reference.
> Maybe we should just return -EINVAL if it is out of range.
I don''t think so - the range checking is already being done as needed,
and we must not preclude there possibly being to descriptors for the
same segment (but different bus ranges).

Jan
> Allen
> 
> -----Original Message-----
> From: Jan Beulich [mailto:JBeulich@novell.com] 
> Sent: Tuesday, July 19, 2011 1:42 AM
> To: xen-devel@lists.xensource.com 
> Cc: Kay, Allen M
> Subject: [PATCH 1/3] x86-64/MMCFG: correct base address computation for 
> regions not starting at bus 0
> 
> As per the specification, the base address reported by ACPI is the one that
> would be used if the region started at bus 0. Hence the start_bus_number 
> offset needs to be added not only to the virtual address, but also the 
> physical one when establishing the mapping, and it then needs to be 
> subtracted when obtaining the virtual address for doing accesses.
> 
> Signed-off-by: Jan Beulich <jbeulich@novell.com>
> 
> --- a/xen/arch/x86/x86_64/mmconfig_64.c
> +++ b/xen/arch/x86/x86_64/mmconfig_64.c
> @@ -25,7 +25,7 @@ struct mmcfg_virt {
>  static struct mmcfg_virt *pci_mmcfg_virt;  static int __initdata 
> mmcfg_pci_segment_shift;
>  
> -static char __iomem *get_virt(unsigned int seg, unsigned bus)
> +static char __iomem *get_virt(unsigned int seg, unsigned int *bus)
>  {
>      struct acpi_mcfg_allocation *cfg;
>      int cfg_num;
> @@ -33,9 +33,11 @@ static char __iomem *get_virt(unsigned i
>      for (cfg_num = 0; cfg_num < pci_mmcfg_config_num; cfg_num++) {
>          cfg = pci_mmcfg_virt[cfg_num].cfg;
>          if (cfg->pci_segment == seg &&
> -            (cfg->start_bus_number <= bus) &&
> -            (cfg->end_bus_number >= bus))
> +            (cfg->start_bus_number <= *bus) &&
> +            (cfg->end_bus_number >= *bus)) {
> +            *bus -= cfg->start_bus_number;
>              return pci_mmcfg_virt[cfg_num].virt;
> +        }
>      }
>  
>      /* Fall back to type 0 */
> @@ -46,7 +48,7 @@ static char __iomem *pci_dev_base(unsign  {
>      char __iomem *addr;
>  
> -    addr = get_virt(seg, bus);
> +    addr = get_virt(seg, &bus);
>      if (!addr)
>          return NULL;
>       return addr + ((bus << 20) | (devfn << 12)); @@ -121,8
+123,11 @@ static
> void __iomem * __init mcfg_iorema
>      if (virt + size < virt || virt + size > PCI_MCFG_VIRT_END)
>          return NULL;
>  
> -    map_pages_to_xen(virt, cfg->address >> PAGE_SHIFT,
> -                     size >> PAGE_SHIFT, PAGE_HYPERVISOR_NOCACHE);
> +    if (map_pages_to_xen(virt,
> +                         (cfg->address >> PAGE_SHIFT) +
> +                         (cfg->start_bus_number << (20 -
PAGE_SHIFT)),
> +                         size >> PAGE_SHIFT,
PAGE_HYPERVISOR_NOCACHE))
> +        return NULL;
>  
>      return (void __iomem *) virt;
>  }



_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xensource.com
http://lists.xensource.com/xen-devel