[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

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

-    } *resource, mem_resource, high_mem_resource, io_resource;
+    } *resource, mem_resource, high_mem_resource, io_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. */


@@ -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


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.


Xen-devel mailing list



Lists.xenproject.org is hosted with RackSpace, monitoring our
servers 24x7x365 and backed by RackSpace's Fanatical Support®.