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

["Jan Beulich" <JBeulich@xxxxxxxx>]

>>> On 28.08.17 at 01:09, <xiong.y.zhang@xxxxxxxxx> 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@xxxxxxxxx>

Reviewed-by: Jan Beulich <jbeulich@xxxxxxxx>
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@xxxxxxxxxxxxx
https://lists.xen.org/xen-devel