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

[Tim Deegan <tim@xxxxxxx>]

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.

I'd be happy with the allocation runes being in x86/mm/paging.c or in
domain_build.c, with a mild preferecen for the latter.

Tim.

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