[PATCH] tools/hvmloader: Use base instead of pci_mem_start for find_next_rmrr()

[PATCH] tools/hvmloader: Use base instead of pci_mem_start for find_next_rmrr()

[Xiong Zhang]

find_next_rmrr(base) is used to find the lowest RMRR ending above base
but below 4G. Current method couldn't cover the following situation:
a. two rmrr exist, small gap between them
b. pci_mem_start and mem_resource.base is below the first rmrr.base
c. find_next_rmrr(pci_mem_start) will find the first rmrr
d. After aligning mem_resource.base to bar size,
   first_rmrr.end < new_base < second_rmrr.base and
   new_base + bar_sz > second_rmrr.base.
   So the new bar will overlap with the second rmrr and don't overlap with
the first rmrr.
But the next_rmrr point to the first rmrr, then check_overlap() couldn't
find the overlap. Finally assign a wrong bar address to bar.

This patch using aligned new base to find the next rmrr, could fix the
above case and find all the overlapped rmrr with new base.

Signed-off-by: Xiong Zhang <xiong.y.zhang@intel.com>
---
 tools/firmware/hvmloader/pci.c | 20 +++++++++++---------
 1 file changed, 11 insertions(+), 9 deletions(-)

diff --git a/tools/firmware/hvmloader/pci.c b/tools/firmware/hvmloader/pci.c
index f4288a3..16fccbf 100644
--- a/tools/firmware/hvmloader/pci.c
+++ b/tools/firmware/hvmloader/pci.c
@@ -405,8 +405,6 @@ void pci_setup(void)
     io_resource.base = 0xc000;
     io_resource.max = 0x10000;
 
-    next_rmrr = find_next_rmrr(pci_mem_start);
-
     /* Assign iomem and ioport resources in descending order of size. */
     for ( i = 0; i < nr_bars; i++ )
     {
@@ -464,15 +462,19 @@ void pci_setup(void)
         base = (resource->base  + bar_sz - 1) & ~(uint64_t)(bar_sz - 1);
 
         /* If we're using mem_resource, check for RMRR conflicts. */
-        while ( resource == &mem_resource &&
-                next_rmrr >= 0 &&
-                check_overlap(base, bar_sz,
-                              memory_map.map[next_rmrr].addr,
-                              memory_map.map[next_rmrr].size) )
+        if ( resource == &mem_resource)
         {
-            base = memory_map.map[next_rmrr].addr + memory_map.map[next_rmrr].size;
-            base = (base + bar_sz - 1) & ~(bar_sz - 1);
             next_rmrr = find_next_rmrr(base);
+            while ( next_rmrr >= 0 &&
+                    check_overlap(base, bar_sz,
+                              memory_map.map[next_rmrr].addr,
+                              memory_map.map[next_rmrr].size) )
+            {
+                base = memory_map.map[next_rmrr].addr +
+                       memory_map.map[next_rmrr].size;
+                base = (base + bar_sz - 1) & ~(bar_sz - 1);
+                next_rmrr = find_next_rmrr(base);
+            }
         }
 
         bar_data |= (uint32_t)base;
-- 
2.7.4


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

Re: [PATCH] tools/hvmloader: Use base instead of pci_mem_start for find_next_rmrr()

[Jan Beulich]

>>> On 21.08.17 at 23:53, <xiong.y.zhang@intel.com> wrote:
> @@ -464,15 +462,19 @@ void pci_setup(void)
>          base = (resource->base  + bar_sz - 1) & ~(uint64_t)(bar_sz - 1);
>  
>          /* If we're using mem_resource, check for RMRR conflicts. */
> -        while ( resource == &mem_resource &&
> -                next_rmrr >= 0 &&
> -                check_overlap(base, bar_sz,
> -                              memory_map.map[next_rmrr].addr,
> -                              memory_map.map[next_rmrr].size) )
> +        if ( resource == &mem_resource)
>          {
> -            base = memory_map.map[next_rmrr].addr + memory_map.map[next_rmrr].size;
> -            base = (base + bar_sz - 1) & ~(bar_sz - 1);
>              next_rmrr = find_next_rmrr(base);
> +            while ( next_rmrr >= 0 &&
> +                    check_overlap(base, bar_sz,
> +                              memory_map.map[next_rmrr].addr,
> +                              memory_map.map[next_rmrr].size) )
> +            {
> +                base = memory_map.map[next_rmrr].addr +
> +                       memory_map.map[next_rmrr].size;
> +                base = (base + bar_sz - 1) & ~(bar_sz - 1);
> +                next_rmrr = find_next_rmrr(base);
> +            }
>          }

Looks good, but please reduce the scope of next_rmrr to just this
if() (afaict it's no longer used anywhere else).

Jan


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

[PATCH v2] tools/hvmloader: Use base instead of pci_mem_start for find_next_rmrr()

[Xiong Zhang]

find_next_rmrr(base) is used to find the lowest RMRR ending above base
but below 4G. Current method couldn't cover the following situation:
a. two rmrr exist, small gap between them
b. pci_mem_start and mem_resource.base is below the first rmrr.base
c. find_next_rmrr(pci_mem_start) will find the first rmrr
d. After aligning mem_resource.base to bar size,
   first_rmrr.end < new_base < second_rmrr.base and
   new_base + bar_sz > second_rmrr.base.
   So the new bar will overlap with the second rmrr and doesn't overlap
with the first rmrr.
But the next_rmrr point to the first rmrr, then check_overlap() couldn't
find the overlap. Finally assign a wrong address to bar.

This patch using aligned new base to find the next rmrr, could fix the
above case and find all the overlapped rmrr with new base.

Signed-off-by: Xiong Zhang <xiong.y.zhang@intel.com>

---
Changes since v1:
 - Reduce the scope of next_rmrr to just this if()
---
 tools/firmware/hvmloader/pci.c | 23 +++++++++++++----------
 1 file changed, 13 insertions(+), 10 deletions(-)

diff --git a/tools/firmware/hvmloader/pci.c b/tools/firmware/hvmloader/pci.c
index f4288a3..1858d7f 100644
--- a/tools/firmware/hvmloader/pci.c
+++ b/tools/firmware/hvmloader/pci.c
@@ -84,7 +84,6 @@ void pci_setup(void)
     uint32_t vga_devfn = 256;
     uint16_t class, vendor_id, device_id;
     unsigned int bar, pin, link, isa_irq;
-    int next_rmrr;
 
     /* Resources assignable to PCI devices via BARs. */
     struct resource {
@@ -405,8 +404,6 @@ void pci_setup(void)
     io_resource.base = 0xc000;
     io_resource.max = 0x10000;
 
-    next_rmrr = find_next_rmrr(pci_mem_start);
-
     /* Assign iomem and ioport resources in descending order of size. */
     for ( i = 0; i < nr_bars; i++ )
     {
@@ -464,15 +461,21 @@ void pci_setup(void)
         base = (resource->base  + bar_sz - 1) & ~(uint64_t)(bar_sz - 1);
 
         /* If we're using mem_resource, check for RMRR conflicts. */
-        while ( resource == &mem_resource &&
-                next_rmrr >= 0 &&
-                check_overlap(base, bar_sz,
-                              memory_map.map[next_rmrr].addr,
-                              memory_map.map[next_rmrr].size) )
+        if ( resource == &mem_resource)
         {
-            base = memory_map.map[next_rmrr].addr + memory_map.map[next_rmrr].size;
-            base = (base + bar_sz - 1) & ~(bar_sz - 1);
+            int next_rmrr;
+
             next_rmrr = find_next_rmrr(base);
+            while ( next_rmrr >= 0 &&
+                    check_overlap(base, bar_sz,
+                              memory_map.map[next_rmrr].addr,
+                              memory_map.map[next_rmrr].size) )
+            {
+                base = memory_map.map[next_rmrr].addr +
+                       memory_map.map[next_rmrr].size;
+                base = (base + bar_sz - 1) & ~(bar_sz - 1);
+                next_rmrr = find_next_rmrr(base);
+            }
         }
 
         bar_data |= (uint32_t)base;
-- 
2.7.4


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

Re: [PATCH v2] tools/hvmloader: Use base instead of pci_mem_start for find_next_rmrr()

[Jan Beulich]

>>> On 28.08.17 at 01:09, <xiong.y.zhang@intel.com> wrote:
> find_next_rmrr(base) is used to find the lowest RMRR ending above base
> but below 4G. Current method couldn't cover the following situation:
> a. two rmrr exist, small gap between them
> b. pci_mem_start and mem_resource.base is below the first rmrr.base
> c. find_next_rmrr(pci_mem_start) will find the first rmrr
> d. After aligning mem_resource.base to bar size,
>    first_rmrr.end < new_base < second_rmrr.base and
>    new_base + bar_sz > second_rmrr.base.
>    So the new bar will overlap with the second rmrr and doesn't overlap
> with the first rmrr.
> But the next_rmrr point to the first rmrr, then check_overlap() couldn't
> find the overlap. Finally assign a wrong address to bar.
> 
> This patch using aligned new base to find the next rmrr, could fix the
> above case and find all the overlapped rmrr with new base.
> 
> Signed-off-by: Xiong Zhang <xiong.y.zhang@intel.com>

Reviewed-by: Jan Beulich <jbeulich@suse.com>
albeit ...

> --- a/tools/firmware/hvmloader/pci.c
> +++ b/tools/firmware/hvmloader/pci.c
> @@ -84,7 +84,6 @@ void pci_setup(void)
>      uint32_t vga_devfn = 256;
>      uint16_t class, vendor_id, device_id;
>      unsigned int bar, pin, link, isa_irq;
> -    int next_rmrr;
>  
>      /* Resources assignable to PCI devices via BARs. */
>      struct resource {
> @@ -405,8 +404,6 @@ void pci_setup(void)
>      io_resource.base = 0xc000;
>      io_resource.max = 0x10000;
>  
> -    next_rmrr = find_next_rmrr(pci_mem_start);
> -
>      /* Assign iomem and ioport resources in descending order of size. */
>      for ( i = 0; i < nr_bars; i++ )
>      {
> @@ -464,15 +461,21 @@ void pci_setup(void)
>          base = (resource->base  + bar_sz - 1) & ~(uint64_t)(bar_sz - 1);
>  
>          /* If we're using mem_resource, check for RMRR conflicts. */
> -        while ( resource == &mem_resource &&
> -                next_rmrr >= 0 &&
> -                check_overlap(base, bar_sz,
> -                              memory_map.map[next_rmrr].addr,
> -                              memory_map.map[next_rmrr].size) )
> +        if ( resource == &mem_resource)
>          {
> -            base = memory_map.map[next_rmrr].addr + memory_map.map[next_rmrr].size;
> -            base = (base + bar_sz - 1) & ~(bar_sz - 1);
> +            int next_rmrr;
> +
>              next_rmrr = find_next_rmrr(base);

... this should have become the initializer of the variable now. If I
end up committing this, I'll try to remember to do the adjustment.

Jan


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