Re: open-coded page list manipulation in shadow code

From: Tim Deegan <tim@xen.org>
To: Jan Beulich <JBeulich@suse.com>
Cc: xen-devel <xen-devel@lists.xenproject.org>
Subject: Re: open-coded page list manipulation in shadow code
Date: Tue, 27 Jan 2015 11:50:26 +0100	[thread overview]
Message-ID: <20150127105026.GA38811@deinos.phlegethon.org> (raw)
In-Reply-To: <54C67AC802000078000598CE@mail.emea.novell.com>

At 16:35 +0000 on 26 Jan (1422286504), Jan Beulich wrote:
> Tim,
> 
> in the course of adding >16Tb support to the hypervisor, I ran into
> issues with you having added several open-coded page list accesses
> while breaking up non-order-0 allocations there. I looked at that
> code for quite a while, but wasn't really able to figure how to
> properly convert these. In particular I'm struggling with the list
> terminations (would using NULL for PAGE_LIST_NULL be correct
> when using ordinary struct list_head-s for struct page_info's list
> member?)

Yes, I think so.  The fragmentary lists would terminate in NULL in
both directions.

> and the implications of this comment
> 
>     /* Page lists don't have pointers back to the head structure, so
>      * it's safe to use a head structure on the stack to link the pages
>      * together. */

Yep; we'd have to make sure that we explicitly NULL'ed out the first
page's prev and the last page's next.  This all seems a bit
unsatisfactory though.

Maybe it would be better to piggy-back these partial lists into the
hash-table's linked list (i.e. the 'next_shadow' field) instead of
the pinned list.  That list is much more active, though.

Let me have a think about it on Thursday (assuming I don't have too
many patch series to review by then!)

> in shadow_alloc(), but then again I also didn't look very closely at
> the other places that also fail to compile.
> 
> To help myself test the new code, I invented a shadow stub which
> provides just enough functionality to build and not crash with all
> the other shadow code compiled out. I wonder whether such a
> build mode might actually be useful for other purposes, and hence
> whether it might be worthwhile extracting that from the patch into
> a standalone one.

If it's not too much work, yes please.  There are already use cases
where people don't need shadow pagetable support, and once PVH is
stable that will be more common.  Having the option to compile out
7kloc of twisty MMU code seems good. :)

Tim.