Re: [PATCH RFCv2 14/15] xen/arm: mm: Rework setup_xenheap_mappings()

From: Stefano Stabellini <sstabellini@kernel.org>
To: Julien Grall <julien@xen.org>
Cc: Stefano Stabellini <sstabellini@kernel.org>,
	 xen-devel@lists.xenproject.org, Wei.Chen@arm.com,
	Henry.Wang@arm.com,  Penny.Zheng@arm.com,
	Bertrand.Marquis@arm.com,  Julien Grall <julien.grall@arm.com>,
	 Volodymyr Babchuk <Volodymyr_Babchuk@epam.com>,
	 Julien Grall <jgrall@amazon.com>
Subject: Re: [PATCH RFCv2 14/15] xen/arm: mm: Rework setup_xenheap_mappings()
Date: Mon, 17 May 2021 17:50:30 -0700 (PDT)	[thread overview]
Message-ID: <alpine.DEB.2.21.2105171738320.14426@sstabellini-ThinkPad-T480s> (raw)
In-Reply-To: <8cda6d2d-7f3c-fef6-c534-2fadabed1bad@xen.org>

On Sat, 15 May 2021, Julien Grall wrote:
> Hi,
> 
> On 15/05/2021 00:51, Stefano Stabellini wrote:
> > On Sun, 25 Apr 2021, Julien Grall wrote:
> > > From: Julien Grall <julien.grall@arm.com>
> > > 
> > > A few issues have been reported with setup_xenheap_mappings() over the
> > > last couple of years. The main ones are:
> > >      - It will break on platform supporting more than 512GB of RAM
> > >        because the memory allocated by the boot allocator is not yet
> > >        mapped.
> > >      - Aligning all the regions to 1GB may lead to unexpected result
> > >        because we may alias non-cacheable region (such as device or
> > > reserved
> > >        regions).
> > > 
> > > map_pages_to_xen() was recently reworked to allow superpage mappings and
> > > deal with the use of page-tables before they are mapped.
> > > 
> > > Most of the code in setup_xenheap_mappings() is now replaced with a
> > > single call to map_pages_to_xen().
> > > 
> > > This also require to re-order the steps setup_mm() so the regions are
> > > given to the boot allocator first and then we setup the xenheap
> > > mappings.
> > 
> > I know this is paranoia but doesn't this introduce a requirement on the
> > size of the first bank in bootinfo.mem.bank[] ?
> > 
> > It should be at least 8KB?
> 
> AFAIK, the current requirement is 4KB because of the 1GB mapping. Long term,
> it would be 8KB.
> 
> > 
> > I know it is unlikely but it is theoretically possible to have a first
> > bank of just 1KB.
> 
> All the page allocators are working at the page granularity level. I am not
> entirely sure whether the current Xen would ignore the region or break.
> 
> Note that setup_xenheap_mappings() is taking a base MFN and a number of pages
> to map. So this doesn't look to be a new problem here.

Yeah... the example of the first bank being 1KB is wrong because, like
you wrote, it wouldn't work before your patches either, and probably it
will never work.

Maybe a better example is a first bank of 4KB exactly.

> For the 8KB requirement, we can look at first all the pages to the boot
> allocator and then do the mapping.
> 
> > 
> > I think we should write the requirement down with an in-code comment?
> > Or better maybe we should write a message about it in the panic below?
> 
> I can write an in-code comment but I think writing down in the panic() would
> be wrong because there is no promise this is going to be the only reason it
> fails (imagine there is a bug in the code...).

Maybe it is sufficient to print out the error code (EINVAL / ENOMEM etc)
in the panic. If you see -12 you know what to look for.

Looking into it I realize that we are not returning -ENOMEM in case of
alloc_boot_pages failures in create_xen_table. It looks like we would
hit a BUG() in the implementation of alloc_boot_pages. Maybe that's good
enough as is then.