Re: [v3][PATCH 07/16] hvmloader/pci: skip reserved ranges

["Chen, Tiejun" <tiejun.chen@intel.com>]

On 2015/6/11 17:51, Tian, Kevin wrote:
>> From: Chen, Tiejun
>> Sent: Thursday, June 11, 2015 9:15 AM
>>
>> When allocating mmio address for PCI bars, we need to make
>> sure they don't overlap with reserved regions.
>>
>> Signed-off-by: Tiejun Chen <tiejun.chen@intel.com>
>> ---
>>   tools/firmware/hvmloader/pci.c | 36
>> ++++++++++++++++++++++++++++++++++--
>>   1 file changed, 34 insertions(+), 2 deletions(-)
>>
>> diff --git a/tools/firmware/hvmloader/pci.c b/tools/firmware/hvmloader/pci.c
>> index 5ff87a7..98af568 100644
>> --- a/tools/firmware/hvmloader/pci.c
>> +++ b/tools/firmware/hvmloader/pci.c
>> @@ -59,8 +59,8 @@ void pci_setup(void)
>>           uint32_t bar_reg;
>>           uint64_t bar_sz;
>>       } *bars = (struct bars *)scratch_start;
>> -    unsigned int i, nr_bars = 0;
>> -    uint64_t mmio_hole_size = 0;
>> +    unsigned int i, j, nr_bars = 0;
>> +    uint64_t mmio_hole_size = 0, reserved_end, max_bar_sz = 0;
>>
>>       const char *s;
>>       /*
>> @@ -226,6 +226,8 @@ void pci_setup(void)
>>               bars[i].devfn   = devfn;
>>               bars[i].bar_reg = bar_reg;
>>               bars[i].bar_sz  = bar_sz;
>> +            if ( bar_sz > max_bar_sz )
>> +                max_bar_sz = bar_sz;
>>
>>               if ( ((bar_data & PCI_BASE_ADDRESS_SPACE) ==
>>                     PCI_BASE_ADDRESS_SPACE_MEMORY) ||
>> @@ -301,6 +303,21 @@ void pci_setup(void)
>>               pci_mem_start <<= 1;
>>       }
>>
>> +    /* Relocate PCI memory that overlaps reserved space, like RDM. */
>> +    for ( j = 0; j < memory_map.nr_map ; j++ )
>> +    {
>> +        if ( memory_map.map[j].type != E820_RAM )
>> +        {
>> +            reserved_end = memory_map.map[j].addr + memory_map.map[j].size;
>> +            if ( check_overlap(pci_mem_start, pci_mem_end,
>> +                               memory_map.map[j].addr,
>> +                               memory_map.map[j].size) )
>> +                pci_mem_start -= memory_map.map[j].size >> PAGE_SHIFT;
>
> what's the point of subtracting reserved size here? I think you want
> to move pci_mem_start higher instead of lower to avoid conflict, right?

No.

>
>> +                pci_mem_start = (pci_mem_start + max_bar_sz - 1) &
>> +                                    ~(uint64_t)(max_bar_sz - 1);
>
> better have some comment to explain what exactly you're trying to
> achieve here.

Actually I didn't have this code fragment. Here I add this chunk of 
codes to address one concern that Jan raised. Please see the below.

>
>> +        }
>> +    }
>> +
>>       if ( mmio_total > (pci_mem_end - pci_mem_start) )
>>       {
>>           printf("Low MMIO hole not large enough for all devices,"
>> @@ -407,8 +424,23 @@ void pci_setup(void)
>>           }
>>
>>           base = (resource->base  + bar_sz - 1) & ~(uint64_t)(bar_sz - 1);
>> + reallocate_mmio:
>
> In earlier comment you said:
>
>> +    /* Relocate PCI memory that overlaps reserved space, like RDM. */
>
> If pci_mem_start has been relocated to avoid overlapping, how will actual
> allocation here will conflict again? Sorry I may miss the two relocations here...
>
>>           bar_data |= (uint32_t)base;
>>           bar_data_upper = (uint32_t)(base >> 32);
>> +        for ( j = 0; j < memory_map.nr_map ; j++ )
>> +        {
>> +            if ( memory_map.map[j].type != E820_RAM )
>> +            {
>> +                reserved_end = memory_map.map[j].addr +
>> memory_map.map[j].size;
>> +                if ( check_overlap(base, bar_sz,
>> +                                   memory_map.map[j].addr,
>> +                                   memory_map.map[j].size) )
>> +                {
>> +                    base = (reserved_end  + bar_sz - 1) & ~(uint64_t)(bar_sz - 1);
>> +                    goto reallocate_mmio;

That is because our previous implementation is just skipping that 
conflict region,

"But you do nothing to make sure the MMIO regions all fit in the
available window (see the code ahead of this relocating RAM if
necessary)." and "...it simply skips assigning resources. Your changes 
potentially growing the space needed to fit all MMIO BARs therefore also 
needs to adjust the up front calculation, such that if necessary more 
RAM can be relocated to make the hole large enough."

And then I replied as follows,

"You're right.

Just think about we're always trying to check pci_mem_start to populate 
more RAM to obtain enough PCI mempry,

     /* Relocate RAM that overlaps PCI space (in 64k-page chunks). */
     while ( (pci_mem_start >> PAGE_SHIFT) < hvm_info->low_mem_pgend )
     {
         struct xen_add_to_physmap xatp;
         unsigned int nr_pages = min_t(
             unsigned int,
             hvm_info->low_mem_pgend - (pci_mem_start >> PAGE_SHIFT),
             (1u << 16) - 1);
         if ( hvm_info->high_mem_pgend == 0 )
             hvm_info->high_mem_pgend = 1ull << (32 - PAGE_SHIFT);
         hvm_info->low_mem_pgend -= nr_pages;
         printf("Relocating 0x%x pages from "PRIllx" to "PRIllx\
                " for lowmem MMIO hole\n",
                nr_pages,
                PRIllx_arg(((uint64_t)hvm_info->low_mem_pgend)<<PAGE_SHIFT),

PRIllx_arg(((uint64_t)hvm_info->high_mem_pgend)<<PAGE_SHIFT));
         xatp.domid = DOMID_SELF;
         xatp.space = XENMAPSPACE_gmfn_range;
         xatp.idx   = hvm_info->low_mem_pgend;
         xatp.gpfn  = hvm_info->high_mem_pgend;
         xatp.size  = nr_pages;
         if ( hypercall_memory_op(XENMEM_add_to_physmap, &xatp) != 0 )
             BUG();
         hvm_info->high_mem_pgend += nr_pages;
     }
"

I hope this can help you understand this background. And I will update 
that code comment like this,

     /*
      * We'll skip all space overlapping with reserved memory later,
      * so we need to decrease pci_mem_start to populate more RAM
      * to compensate them.
      */

Thanks
Tiejun

>> +                }
>> +            }
>> +        }
>>           base += bar_sz;
>>
>>           if ( (base < resource->base) || (base > resource->max) )
>> --
>> 1.9.1
>
>
>