Re: [Xen-devel] [PATCH 1/2] xen/mm: fold PGC_broken into PGC_state bits

From: Julien Grall <julien@xen.org>
To: David Woodhouse <dwmw2@infradead.org>, Jan Beulich <jbeulich@suse.com>
Cc: Stefano Stabellini <sstabellini@kernel.org>, Wei Liu <wl@xen.org>,
	Konrad Rzeszutek Wilk <konrad.wilk@oracle.com>,
	George Dunlap <george.dunlap@eu.citrix.com>,
	Andrew Cooper <andrew.cooper3@citrix.com>,
	Ian Jackson <ian.jackson@eu.citrix.com>,
	George Dunlap <george.dunlap@citrix.com>,
	Jeff Kubascik <jeff.kubascik@dornerworks.com>,
	Stewart Hildebrand <stewart.hildebrand@dornerworks.com>,
	xen-devel@lists.xenproject.org
Subject: Re: [Xen-devel] [PATCH 1/2] xen/mm: fold PGC_broken into PGC_state bits
Date: Sun, 9 Feb 2020 17:59:50 +0000	[thread overview]
Message-ID: <087ba1dc-8838-91f3-058c-28e6b798fd85@xen.org> (raw)
In-Reply-To: <d0072bd4be2151e1ad768f948f541694bb062d94.camel@infradead.org>

On 09/02/2020 13:22, David Woodhouse wrote:
> On Fri, 2020-02-07 at 20:27 +0000, Julien Grall wrote:
>> Could you please send the series in a separate thread? So we don't mix
>> the two discussions (i.e merge and free on boot allocated page) together.
> 
> Well, they're all part of the same mess, really.

Sending a series in the middle of another series is always more 
difficult to track :). The more if they are handled by two different 
person...

> 
> There are cases where pages end up in free_heap_pages() which were
> never vetted by init_heap_pages(). While that normally works just fine
> — to the extent that we've never really cared — the hack excluding MFN0
> is one of the things that gets 'missed' for such pages.
> 
> I was only looking at this because the early vmap support makes it a
> tiny bit more likely that some pages will be freed that way after being
> given out by the boot allocator, but there were plenty of reasons it
> might happen already.
> 
> These patches basically legitimise that — we make it OK for
> free_heap_pages() to be given a range which was never in the heap in
> the first place. And in so doing, we fix the MFN0/zone merge trick even
> when (for example) MFN0 was part of the dom0 initramfs and assigned
> directly, but gets freed up later.
> 
> But sure, having failed to elicit any screams of "don't do it like
> that", I'll fix up the things you pointed out and I'll repost it as
> part of the series containing the early vmap() support, since that's
> why I'm working on it.
> 
> Although at this point I'm half tempted to declare that there are *so*
> many ways this can happen already, that the tiny little bit that it's
> made more likely by the early vmap() support is basically in the noise.
> 
> In that case we can separate these patches out as an independent fix
> rather than a dependency of early vmap.

I hadn't realize how messy it was because I had Arm in mind and wasn't 
expected x86 to abuse so much the interface.

For x86, I agree that this is noise as they are abusing the interface 
pretty much everywhere.

However, on Arm there is only one place that is abusing the interface. 
It is in the ACPI code, although I think it will be just a leak given 
the implementation of acpi_os_free_memory(). As we don't free 
page-tables yet on Arm, the introduction of the early vmap would not 
introduce any more abuse on Arm.

It would obviously nice to fix it, but as you said this is noise on x86. 
So that's really up to the x86 folks (Andrew, George, Jan) to see 
whether yet another abuse is ok for them :).

Cheers,

-- 
Julien Grall

_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel