On Mon, 2020-02-03 at 14:00 +0000, Julien Grall wrote:
> Hi David,
> 
> On 01/02/2020 00:33, David Woodhouse wrote:
> > From: David Woodhouse <dwmw@amazon.co.uk>
> 
> I am a bit concerned with this change, particularly the consequence this 
> have for the page-tables. There is an assumption that intermediate 
> page-tables allocated via the boot allocator will never be freed.
> 
> On x86, a call to vunmap() will not free page-tables, but a subsequent 
> call to vmap() may free it depending on the mapping size. So we would 
> call free_domheap_pages() rather than init_heap_pages().
> 
> I am not entirely sure what is the full consequence, but I think this is 
> a call for investigation and write it down a summary in the commit message.

This isn't just about page tables, right? It's about *any* allocation
given out by the boot allocator, being freed with free_heap_pages() ? 

Given the amount of code that has conditionals in both alloc and free
paths along the lines of…

  if (system_state > SYS_STATE_boot)
      use xenheap
  else
      use boot allocator

… I'm not sure I'd really trust the assumption that such a thing never
happens; that no pages are ever allocated from the boot allocator and
then freed into the heap.

In fact it does work fine except for some esoteric corner cases,
because init_heap_pages() is mostly just a trivial loop over
free_heap_pages().

The corner cases are if you call free_heap_pages() on boot-allocated
memory which matches one or more of the following criteria:

 • Includes MFN #0,

 • Includes the first page the heap has seen on a given node, so
   init_node_heap() has to be called, or

 • High-order allocations crossing from one node to another.

The first is trivial to fix by lifting the check for MFN#0 up into
free_heap_pages(). The second isn't hard either.

I'll have to think about the third a little longer... one option might
be to declare that it's OK to call free_heap_pages() on a *single* page
that was allocated from the boot allocator, but not higher orders (in
case they cross nodes). That would at least cover the specific concern
about page table pages and vm_init()/vmap().