Re: [PATCH] libxc: Defer initialization of start_page for HVM guests

[Ian Campbell <ian.campbell@citrix.com>]

On Tue, 2015-12-22 at 09:48 -0500, Boris Ostrovsky wrote:
> On 12/22/2015 09:36 AM, Roger Pau Monné wrote:
> > El 22/12/15 a les 15.12, Boris Ostrovsky ha escrit:
> > > On 12/22/2015 04:42 AM, Roger Pau Monné wrote:
> > > > Also, why is everything done inside of alloc_magic_pages_hvm moved
> > > > to
> > > > start_info_hvm? AFAICT we should only need to move the code that
> > > > fills
> > > > the hvm_start_info struct, but the rest of the code already present
> > > > in
> > > > alloc_magic_pages_hvm could be left as-is.
> > > That's what I was referring to in the commit message (and in the
> > > comment
> > > below it): most, if not all, of the stuff that
> > > alloc_magic_pages_hvm()
> > > currently does really has nothing to do with magic pages allocation.
> > > E.g. hvm_params settting, special pages initialization, etc. So I
> > > figured I could move all of this to a later point. But, as I said in
> > > the
> > > comment, I am not convinced that start_info() is the right place
> > > either.
> > Some of it has to do with magic pages IMHO (although I have to admit
> > this is probably a question of taste), for example it makes sense from
> > my PoV to allocate the hvm_info_page, the special pages (xenstore,
> > console...), the ioreq server pages and possibly the EPT identity map.
> > Moving the start_info stuff into it's own function also makes sense.
> 
> Since there are many ways to separate this I'll wait for maintainers to 
> express their preference.

So AIUI the issue here is that alloc_magic_pages_hvm has become something
of a dumping ground for stuff that needs to (or can) happen at this
particular point in time, some of which is certainly not "magic pages" and
others for which it is debatable.

One particular piece of this is the start_info which is not a magic page
either and which as it happens needs to be done later. (Much later it
seems?)

As part of fixing that you essentially moved the "dumping ground" from
alloc_magic_pages_hvm to start_info_hvm (which is a new callback on an
existing hook), since, why not.

Looking at xc_dom_boot_image (the caller of the start_info hook) I see that
it also calls, not long after the start_info hook, other hooks such as
->bootlate ("/* misc x86 stuff */").

So if all the random stuff which this patch moves from ->alloc_magic to
->start_info can really be safely moved like that (which I didn't check)
I'd expect it could equally well move to ->bootlate (the only thing in the
middle is setup_hypercall_page() and xc_dom_log_memory_footprint(), and
that stuff does seem much more in keeping with "misc x86 stuff" than
->start_info or ->alloc_magic.

Maybe the xc_dom_log_memory_footprint call would need to move after the
->bootlate, that doesn't seem like the end of the world.

Of course if there is functionality which seems worthy of its own new
callback or which would be best placed in some other existing callback I'm
sure that would be fine too.

> 
> (I don't know who is around now and I will disappear too on Friday until 
> January so this may have to wait until then).
> 
> -boris

_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
http://lists.xen.org/xen-devel