Re: [Xen-devel] [PATCH 2/2] xen/mm: Introduce PGC_state_uninitialised

[Julien Grall <julien@xen.org>]

Hi David,

On 20/03/2020 15:17, David Woodhouse wrote:
> On Fri, 2020-03-20 at 13:33 +0000, Paul Durrant wrote:
>>> -----Original Message-----
>>> From: Xen-devel <xen-devel-bounces@lists.xenproject.org> On Behalf Of David Woodhouse
>>> Sent: 19 March 2020 21:22
>>> To: xen-devel@lists.xenproject.org
>>> Cc: Stefano Stabellini <sstabellini@kernel.org>; Julien Grall <julien@xen.org>; Wei Liu <wl@xen.org>;
>>> Andrew Cooper <andrew.cooper3@citrix.com>; Ian Jackson <ian.jackson@eu.citrix.com>; George Dunlap
>>> <george.dunlap@citrix.com>; hongyxia@amazon.com; Jan Beulich <jbeulich@suse.com>; Volodymyr Babchuk
>>> <Volodymyr_Babchuk@epam.com>; Roger Pau Monné <roger.pau@citrix.com>
>>> Subject: [Xen-devel] [PATCH 2/2] xen/mm: Introduce PGC_state_uninitialised
>>>
>>> From: David Woodhouse <dwmw@amazon.co.uk>
>>>
>>> It is possible for pages to enter general circulation without ever
>>> being process by init_heap_pages().
>>>
>>> For example, pages of the multiboot module containing the initramfs may
>>> be assigned via assign_pages() to dom0 as it is created. And some code
>>> including map_pages_to_xen() has checks on 'system_state' to determine
>>> whether to use the boot or the heap allocator, but it seems impossible
>>> to prove that pages allocated by the boot allocator are not subsequently
>>> freed with free_heap_pages().
>>>
>>> This actually works fine in the majority of cases; there are only a few
>>> esoteric corner cases which init_heap_pages() handles before handing the
>>> page range off to free_heap_pages():
>>>   • Excluding MFN #0 to avoid inappropriate cross-zone merging.
>>>   • Ensuring that the node information structures exist, when the first
>>>     page(s) of a given node are handled.
>>>   • High order allocations crossing from one node to another.
>>>
>>> To handle this case, shift PG_state_inuse from its current value of
>>> zero, to another value. Use zero, which is the initial state of the
>>> entire frame table, as PG_state_uninitialised.
>>>
>>> Fix a couple of assertions which were assuming that PG_state_inuse is
>>> zero, and make them cope with the PG_state_uninitialised case too where
>>> appopriate.
>>>
>>> Finally, make free_heap_pages() call through to init_heap_pages() when
>>> given a page range which has not been initialised. This cannot keep
>>> recursing because init_heap_pages() will set each page state to
>>> PGC_state_inuse before passing it back to free_heap_pages() for the
>>> second time.
>>>
>>> Signed-off-by: David Woodhouse <dwmw@amazon.co.uk>
>>> ---
>>>   xen/arch/x86/mm.c        |  3 ++-
>>>   xen/common/page_alloc.c  | 44 +++++++++++++++++++++++++++++-----------
>>>   xen/include/asm-arm/mm.h |  3 ++-
>>>   xen/include/asm-x86/mm.h |  3 ++-
>>>   4 files changed, 38 insertions(+), 15 deletions(-)
>>>
>>> diff --git a/xen/arch/x86/mm.c b/xen/arch/x86/mm.c
>>> index 62507ca651..5f0581c072 100644
>>> --- a/xen/arch/x86/mm.c
>>> +++ b/xen/arch/x86/mm.c
>>> @@ -491,7 +491,8 @@ void share_xen_page_with_guest(struct page_info *page, struct domain *d,
>>>
>>>       page_set_owner(page, d);
>>>       smp_wmb(); /* install valid domain ptr before updating refcnt. */
>>> -    ASSERT((page->count_info & ~PGC_xen_heap) == 0);
>>> +    ASSERT((page->count_info & ~PGC_xen_heap) == PGC_state_inuse ||
>>> +           (page->count_info & ~PGC_xen_heap) == PGC_state_uninitialised);
>>
>> Could the page state perhaps be bumped to inuse in this case? It
>> seems odd to leave state uninitialized yet succeed in sharing with a
>> guest.
> 
> No, that doesn't really work.
> 
> You can't just *declare* that the page was properly initialised,
> because it isn't true. There's a pathological case where the zone
> hasn't been initialised at all, because *all* the pages in that zone
> got handed out by the boot allocator or used for initrd etc.
> 
> The first pages 'freed' in that zone end up being used (in
> init_heap_pages) to create the zone structures.
> 
> Likewise, it could include a page which init_heap_pages() doesn't
> actually *put* into the buddy allocator, to work around the cross-zone
> merge problem. It's fine to use that page and share it with a guest,
> but it can't ever be freed into the buddy allocator.

For liveupdate, we will need a way to initialize a page but mark it as 
already inuse (i.e in the same state as they would be if allocated 
normally).

It feels to me, this is also what we want in this case. The page would 
be first initialize and then we can use it normally including freeing 
later on.

Would it make sense to introduce an helper for this purpose?

Cheers,

-- 
Julien Grall

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