[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index] Re: [Xen-devel] [V8 PATCH 2/8] pvh dom0: construct_dom0 changes
>>> 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
|
Lists.xenproject.org is hosted with RackSpace, monitoring our |