Re: [v7][PATCH 06/16] hvmloader/pci: skip reserved ranges

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

>> -    } *resource, mem_resource, high_mem_resource, io_resource;
>> +    } *resource, mem_resource, high_mem_resource, io_resource, exp_mem_resource;
>
> Despite having gone through description and the rest of the patch I
> can't seem to be able to guess what "exp_mem" stands for.
> Meaningful variable names are quite helpful though, often avoiding
> the need for comments.

exp_mem_resource() is the expanded mem_resource in the case of 
populating RAM.

Maybe I should use the whole word, expand_mem_resource.

>
>>       /* Create a list of device BARs in descending order of size. */

[snip]

>> @@ -309,29 +339,31 @@ void pci_setup(void)
>>       }
>>
>>       /* Relocate RAM that overlaps PCI space (in 64k-page chunks). */
>> +    cur_pci_mem_start = pci_mem_start;
>>       while ( (pci_mem_start >> PAGE_SHIFT) < hvm_info->low_mem_pgend )
>> +        relocate_ram_for_pci_memory(cur_pci_mem_start);
>
> Please be consistent which variable to want to use in the loop
> (pci_mem_start vs cur_pci_mem_start).

Overall I just call relocate_ram_for_pci_memory() twice and each I 
always pass cur_pci_mem_start. Any inconsistent place?

>
> Also, this being the first substantial change to the function makes
> clear that you _still_ leave the sizing loop untouched, and instead
> make the allocation logic below more complicated. I said before a

But this may be more reasonable than it used to do. In my point of view 
we always need to first allocate 32bit mmio and then allocate 64bit mmio 
since as you said we don't want to expand high memory if possible.

> number of times that I don't think this helps maintainability of this
> already convoluted code. Among other things this manifests itself
> in your second call to relocate_ram_for_pci_memory() in no way
> playing by the constraints explained a few lines up from here in an
> extensive comment.

Can't all variables/comments express what I intend to do here? Except 
for that exp_mem_resource.
               /* 

              * We have to populate more RAM to further allocate 

              * the remaining 32bars. 

              */ 

             if ( mmio32_unallocated_total ) 

             { 

                 cur_pci_mem_start = pci_mem_start - 
mmio32_unallocated_total;
                 relocate_ram_for_pci_memory(cur_pci_mem_start); 

                 exp_mem_resource.base = cur_pci_mem_start; 

                 exp_mem_resource.max = pci_mem_start; 

             }

>
> Therefore I'll not make any further comments on the rest of the
> patch, but instead outline an allocation model that I think would
> fit our needs: Subject to the constraints mentioned above, set up
> a bitmap (maximum size 64k [2Gb = 2^^19 pages needing 2^^19
> bits], i.e. reasonably small a memory block). Each bit represents a
> page usable for MMIO: First of all you remove the range from
> PCI_MEM_END upwards. Then remove all RDM pages. Now do a
> first pass over all devices, allocating (in the bitmap) space for only
> the 32-bit MMIO BARs, starting with the biggest one(s), by finding
> a best fit (i.e. preferably a range not usable by any bigger BAR)
> from top down. For example, if you have available
>
> [f0000000,f8000000)
> [f9000000,f9001000)
> [fa000000,fa003000)
> [fa010000,fa012000)
>
> and you're looking for a single page slot, you should end up
> picking fa002000.

Why is this [f9000000,f9001000]? Just one page in this slot.

>
> After this pass you should be able to do RAM relocation in a
> single attempt just like we do today (you may still grow the MMIO
> window if you know you need to and can fit some of the 64-bit
> BARs in there, subject to said constraints; this is in an attempt
> to help OSes not comfortable with 64-bit resources).
>
> In a 2nd pass you'd then assign 64-bit resources: If you can fit
> them below 4G (you still have the bitmap left of what you've got
> available), put them there. Allocation strategy could be the same

I think basically, your logic is similar to what I did as I described in 
changelog,

   1>. The first allocation round just to 32bit-bar

   If we can finish allocating all 32bit-bar, we just go to allocate 
64bit-bar
   with all remaining resources including low pci memory.

   If not, we need to calculate how much RAM should be populated to 
allocate the
   remaining 32bit-bars, then populate sufficient RAM as 
exp_mem_resource to go
   to the second allocation round 2>.

   2>. The second allocation round to the remaining 32bit-bar

   We should can finish allocating all 32bit-bar in theory, then go to 
the third
   allocation round 3>.

   3>. The third allocation round to 64bit-bar

   We'll try to first allocate from the remaining low memory resource. 
If that
   isn't enough, we try to expand highmem to allocate for 64bit-bar. 
This process
   should be same as the original.

> as above (biggest first), perhaps allowing for some factoring out
> of logic, but here smallest first probably could work equally well.
> The main thought to decide between the two is whether it is
> better to fit as many (small) or as big (in total) as possible a set
> under 4G. I'd generally expect the former (as many as possible,
> leaving only a few huge ones to go above 4G) to be the better
> approach, but that's more a gut feeling than based on hard data.
>

I think bitmap mechanism is a good idea but honestly, its not easy to 
cover all requirements here. And just like bootmem on Linux side, so its 
a little complicated to implement this entirely. So I prefer not to 
introduce this way in current phase.

Thanks
Tiejun