Re: [Xen-devel] [V8 PATCH 2/8] pvh dom0: construct_dom0 changes

["Jan Beulich" <JBeulich@xxxxxxxx>]

>>> On 05.04.14 at 02:53, <mukesh.rathor@xxxxxxxxxx> wrote:
> On Thu, 27 Mar 2014 16:30:37 +0100
> Tim Deegan <tim@xxxxxxx> wrote:
> 
>> At 15:04 +0000 on 27 Mar (1395929085), Jan Beulich wrote:
>> > >>> On 27.03.14 at 11:55, <george.dunlap@xxxxxxxxxxxxx> wrote:
>> > > On 03/27/2014 10:14 AM, Jan Beulich wrote:
>> > >>>>> On 26.03.14 at 20:05, <george.dunlap@xxxxxxxxxxxxx> wrote:
>> > >>> --- a/xen/arch/x86/mm/hap/hap.c
>> > >>> +++ b/xen/arch/x86/mm/hap/hap.c
>> > >>> @@ -589,6 +589,21 @@ int hap_domctl(struct domain *d,
>> > >>> xen_domctl_shadow_op_t 
>> > > *sc,
>> > >>>        }
>> > >>>    }
>> > >>>    
>> > >>> +void __init hap_set_pvh_alloc_for_dom0(struct domain *d,
>> > >>> +                                       unsigned long num_pages)
>> > >>> +{
>> > >>> +    int rc;
>> > >>> +    unsigned long memkb = num_pages * (PAGE_SIZE / 1024);
>> > >>> +
>> > >>> +    /* Copied from: libxl_get_required_shadow_memory() */
>> > >>> +    memkb = 4 * (256 * d->max_vcpus + 2 * (memkb / 1024));
>> > >>> +    num_pages = ( (memkb + 1023) / 1024) << (20 - PAGE_SHIFT);
>> > >>> +    paging_lock(d);
>> > >>> +    rc = hap_set_allocation(d, num_pages, NULL);
>> > >>> +    paging_unlock(d);
>> > >>>
>> > >>>
>> > >>> The calculation for how many pages of shadow memory are needed
>> > >>> logically belongs in domain_build.c, not hap.c.
>> > >> Iirc it was me requesting it to be moved here, on the basis that
>> > >> the calculation should match the one for other domains, and
>> > >> hence be done alongside that for other domains. domain_build.c
>> > >> shouldn't carry HAP-specific knowledge imo.
>> > > 
>> > > I thought that might be the case, which is why I apologized in
>> > > advance for not reading the previous thread.
>> > > 
>> > > The calculation should indeed match that done for other domains; 
>> > > however, as the comment clearly says, this calculation is done in
>> > > libxl, not in Xen at all.  Everything done to build dom0 in Xen
>> > > which corresponds to things done by the toolstack to build a domU
>> > > logically belongs in domain_build.c.
>> > > 
>> > > Putting it in hap.c would be wrong in any case; what would you do
>> > > when we enable shadow mode for HAP dom0?
>> > 
>> > The function calls hap_set_allocation(), so it's already
>> > HAP-specific.
>> 
>> It really ought to be calling a paging_set_allocation() wrapper;
>> there's nothing _inherently_ HAP-specific about PVH.  Right now that
>> wrapper doesn't exist, because that path goes via paging_domctl() for
>> other domains.
> 
> Correct, PVH requires HAP at present, and I like when the code makes
> that clear. In general, my approach is to make change in future when
> demanded, thus, when shadow is added, it can be rearranged with small
> effort. 
> 
> Anyways, I'm flexible. Jan, final word: Leave it as is (my first pref),
> move to domain_build.c, or put a wrapper(my last preference) ?

I'm fine with leaving it as is (in hap.c), but of course (as usually) my
first preference is to have it done cleanly (i.e. like Tim suggests) -
realizing that this is what would imply you doing more changes than
either of the other two options.

Jan


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