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

[Boris Ostrovsky <boris.ostrovsky@xxxxxxxxxx>]

On 01/05/2016 09:06 AM, Ian Campbell wrote:
> 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 didn't notice bootlate --- it looks to me like a place where we'd do 
things we couldn't do earlier, which is exactly what I was trying to 
achieve.
So I'll use it (and see if I can keep some of the things Roger thought 
are magic pages-related in alloc_magic_pages_hvm()).
-boris

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