Re: [PATCH kvm-unit-tests v2 07/11] s390x: initialize the physical allocator

From: David Hildenbrand <david@redhat.com>
To: Andrew Jones <drjones@redhat.com>
Cc: kvm@vger.kernel.org, "Paolo Bonzini" <pbonzini@redhat.com>,
	"Radim Krčmář" <rkrcmar@redhat.com>,
	"Thomas Huth" <thuth@redhat.com>,
	"Christian Borntraeger" <borntraeger@de.ibm.com>,
	"Cornelia Huck" <cohuck@redhat.com>
Subject: Re: [PATCH kvm-unit-tests v2 07/11] s390x: initialize the physical allocator
Date: Mon, 15 Jan 2018 11:21:59 +0100	[thread overview]
Message-ID: <9e24c7c0-4dfb-d8e3-0d38-f6e98e28f35d@redhat.com> (raw)
In-Reply-To: <20180115101614.7zazuegmbmu3p6ku@kamzik.brq.redhat.com>

On 15.01.2018 11:16, Andrew Jones wrote:
> On Fri, Jan 12, 2018 at 03:34:13PM +0100, David Hildenbrand wrote:
>> We now have the range of free memory, let's initialize the physical
>> allocator. It is now possible to use alloc_page()/alloc_pages().
> 
> Commit message isn't quite right. After this patch it's possible to
> use malloc and memalign, based on the early-ops, but alloc_pages()
> requires the freelist to be initialized first by free_pages(), done
> by setup_vm().

That's interesting, as I am using alloc_pages() during setup_vm() I
haven't noticed it.

> 
> Actually, since we have four states of memory management setup,
> then I think we either need three setup calls:
> 
> state 1: no setup done, no call
> state 2: phys-alloc is set up by the phys_alloc_init() call
> state 3: alloc_page() is set up, need call that allocates chunk with the
>                                  phys-allocator for free_pages()
> 
> (Instead of a chunk, all memory could be given, but then either a
>  new set of early-ops should be provided that are based on
>  alloc_page(), or all early memory allocations must use alloc_page())
> 
> state 4: virtual memory is set up by the setup_vm() call
> 
> Or, simply remove state 2 and its code and alloc-ops, changing all early
> allocations to use alloc_page(). If each arch provides an mmu_enabled()
> call, then the alloc_ops->memalign call in memalign could be replaced
> with

I think I'd prefer that. Unless somebody needs the flexibility of this
extra state.

But let's hear what the others say.

> 
>  if (mmu_enabled())
>     vm_memalign(...)
>  else
>     /* fallback to alloc_pages() */
> 
> I probably should have written all this in a separate thread, because
> I wouldn't want to hold this series up on a rework of the general
> memory management framework. But, anyway, thoughts on that? Paolo?>
> Thanks,
> drew
>

-- 

Thanks,

David / dhildenb