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

[Boris Ostrovsky <boris.ostrovsky@oracle.com>]

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@lists.xen.org
http://lists.xen.org/xen-devel