Re: [RFC][PATCH 12/13] hvmloader/pci: skip reserved ranges

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

On 2015/5/15 16:31, Jan Beulich wrote:
>>>> On 15.05.15 at 10:16, <tiejun.chen@intel.com> wrote:
>> On 2015/5/15 15:44, Jan Beulich wrote:
>>>>>> On 15.05.15 at 09:34, <tiejun.chen@intel.com> wrote:
>>>> So I think we may need to adjust pci_mem_start like this,
>>>>
>>>> @@ -301,6 +301,19 @@ 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;
>>>> +        }
>>>> +    }
>>>> +
>>>>         if ( mmio_total > (pci_mem_end - pci_mem_start) )
>>>>         {
>>>>             printf("Low MMIO hole not large enough for all devices,"
>>>>
>>>> Right?
>>>
>>> I think that gets you in the right direction, but isn't enough, as it
>>> doesn't account for (unavoidable) gaps (BARs are always a power
>>> of 2 in size and accordingly aligned).
>>
>> Right.
>>
>> But as you see, we always take this action, >> PAGE_SHIFT, so this means
>> its always a sort of power of 2.
>
> No, certainly not.

rdm start or size? Or anything else I'm missing?

>
>> Additionally, lets go back here,
>>
>>           if ( (base < resource->base) || (base > resource->max) )
>>           {
>>               printf("pci dev %02x:%x bar %02x size "PRIllx": no space for "
>>                      "resource!\n", devfn>>3, devfn&7, bar_reg,
>>                      PRIllx_arg(bar_sz));
>>               continue;
>>           }
>>
>> I mean even without rdm, the original codes don't consider handling this
>> lack of space from a alignment in advance, right?
>
> Correct. But your change increases the chances of this getting
> used. Also I think you may want to carefully look at under what
> conditions this path gets taken without and with your patches.
>

Sure.

I think I can record that max bar_sz to improve this like,

@@ -60,7 +60,7 @@ void pci_setup(void)
          uint64_t bar_sz;
      } *bars = (struct bars *)scratch_start;
      unsigned int i, j, nr_bars = 0;
-    uint64_t mmio_hole_size = 0, reserved_end;
+    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) ||
@@ -311,6 +313,8 @@ void pci_setup(void)
                                 memory_map.map[j].addr,
                                 memory_map.map[j].size) )
                  pci_mem_start -= memory_map.map[j].size >> PAGE_SHIFT;
+                pci_mem_start = (pci_mem_start + max_bar_sz - 1) &
+                                    ~(uint64_t)(max_bar_sz - 1);
          }
      }


Note you also can take close look at this change in next revision if 
this is not that bad with your glance :)

Thanks
Tiejun