From mboxrd@z Thu Jan  1 00:00:00 1970
From: George Dunlap <george.dunlap@citrix.com>
Subject: Re: [v10][PATCH 06/16] hvmloader/pci: Try to avoid
 placing BARs in RMRRs
Date: Mon, 20 Jul 2015 13:52:11 +0100
Message-ID: <55ACEEFB.1050707@citrix.com>
References: <1437373023-14884-1-git-send-email-tiejun.chen@intel.com>
	<1437373023-14884-7-git-send-email-tiejun.chen@intel.com>
	<55ACF7EF020000780009305B@mail.emea.novell.com>
Mime-Version: 1.0
Content-Type: text/plain; charset="us-ascii"
Content-Transfer-Encoding: 7bit
Return-path: <xen-devel-bounces@lists.xen.org>
In-Reply-To: <55ACF7EF020000780009305B@mail.emea.novell.com>
List-Unsubscribe: <http://lists.xen.org/cgi-bin/mailman/options/xen-devel>,
	<mailto:xen-devel-request@lists.xen.org?subject=unsubscribe>
List-Post: <mailto:xen-devel@lists.xen.org>
List-Help: <mailto:xen-devel-request@lists.xen.org?subject=help>
List-Subscribe: <http://lists.xen.org/cgi-bin/mailman/listinfo/xen-devel>,
	<mailto:xen-devel-request@lists.xen.org?subject=subscribe>
Sender: xen-devel-bounces@lists.xen.org
Errors-To: xen-devel-bounces@lists.xen.org
To: Jan Beulich <JBeulich@suse.com>, George Dunlap <George.Dunlap@eu.citrix.com>, Tiejun Chen <tiejun.chen@intel.com>
Cc: xen-devel@lists.xen.org
List-Id: xen-devel@lists.xenproject.org

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