Re: [v10][PATCH 06/16] hvmloader/pci: Try to avoid placing BARs in RMRRs

[George Dunlap <george.dunlap@citrix.com>]

On 07/20/2015 12:30 PM, Jan Beulich wrote:
>>>> On 20.07.15 at 08:16, <tiejun.chen@intel.com> wrote:
>> --- a/tools/firmware/hvmloader/pci.c
>> +++ b/tools/firmware/hvmloader/pci.c
>> @@ -38,6 +38,43 @@ uint64_t pci_hi_mem_start = 0, pci_hi_mem_end = 0;
>>  enum virtual_vga virtual_vga = VGA_none;
>>  unsigned long igd_opregion_pgbase = 0;
>>  
>> +/* Check if any conflicts with all reserved device memory. */
> 
> /* Check if the specified range conflicts with any reserved device memory. */
> 
> (and the "any" could perhaps be left out)
> 
>> +static bool check_overlap_all(uint64_t start, uint64_t size)
>> +{
>> +    unsigned int i;
>> +
>> +    for ( i = 0; i < memory_map.nr_map; i++ )
>> +    {
>> +        if ( memory_map.map[i].type == E820_RESERVED &&
>> +             check_overlap(start, size,
>> +                           memory_map.map[i].addr,
>> +                           memory_map.map[i].size) )
>> +            return true;
>> +    }
>> +
>> +    return false;
>> +}
>> +
>> +/* Find the lowest RMRR higher than base. */
>> +static int find_next_rmrr(uint32_t base)
>> +{
>> +    unsigned int i;
>> +    int next_rmrr = -1;
>> +    uint64_t min_base = (1ull << 32);
>> +
>> +    for ( i = 0; i < memory_map.nr_map ; i++ )
>> +    {
>> +        if ( memory_map.map[i].type == E820_RESERVED &&
>> +             memory_map.map[i].addr > base &&
>> +             memory_map.map[i].addr < min_base )
>> +        {
>> +            next_rmrr = i;
>> +            min_base = memory_map.map[i].addr;
>> +        }
>> +    }
>> +    return next_rmrr;
>> +}
> 
> Considering _both_ callers, I think the function should actually return
> the lowest RMRR higher than or equal to base. 

You mean instead of strictly greater than the base.

> Or wait - we actually
> need to find the lowest RMRR the _end_ of which is higher than base.

Yes, you're right: there's always a risk that pci_mem_start will *start*
in the middle of a range.  Looking for the next *end* is more robust.

>> @@ -407,6 +456,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 &&
> 
>> = I think.

Yes, > is a typo; I certainly meant to type >=.

> 
>> +                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) & ~(uint64_t)(bar_sz - 1);
> 
> Pointless cast.

This was copy-and-pasted from the line above.  I had assumed that bar_sz
was uint32_t (perhaps it was at some point), but I guess now it's
pointless there too. :-)

 -George