From mboxrd@z Thu Jan 1 00:00:00 1970 From: George Dunlap Subject: Re: [V8 PATCH 2/8] pvh dom0: construct_dom0 changes Date: Mon, 7 Apr 2014 10:27:14 +0100 Message-ID: <53426F72.4080206@eu.citrix.com> References: <1395452357-1598-1-git-send-email-mukesh.rathor@oracle.com> <1395452357-1598-3-git-send-email-mukesh.rathor@oracle.com> <533324EB.2030409@eu.citrix.com> <533408220200007800002BA4@nat28.tlf.novell.com> <5334039F.3010400@eu.citrix.com> <53344C1D0200007800002F3B@nat28.tlf.novell.com> <20140327153037.GB91261@deinos.phlegethon.org> <20140404175337.18aad83c@mantra.us.oracle.com> Mime-Version: 1.0 Content-Type: text/plain; charset="us-ascii"; Format="flowed" Content-Transfer-Encoding: 7bit Return-path: Received: from mail6.bemta4.messagelabs.com ([85.158.143.247]) by lists.xen.org with esmtp (Exim 4.72) (envelope-from ) id 1WX5py-0004hu-4M for xen-devel@lists.xenproject.org; Mon, 07 Apr 2014 09:27:30 +0000 In-Reply-To: <20140404175337.18aad83c@mantra.us.oracle.com> List-Unsubscribe: , List-Post: List-Help: List-Subscribe: , Sender: xen-devel-bounces@lists.xen.org Errors-To: xen-devel-bounces@lists.xen.org To: Mukesh Rathor , Tim Deegan Cc: xen-devel@lists.xenproject.org, keir.xen@gmail.com, Jan Beulich List-Id: xen-devel@lists.xenproject.org On 04/05/2014 01:53 AM, Mukesh Rathor wrote: > On Thu, 27 Mar 2014 16:30:37 +0100 > Tim Deegan wrote: > >> At 15:04 +0000 on 27 Mar (1395929085), Jan Beulich wrote: >>>>>> On 27.03.14 at 11:55, wrote: >>>> On 03/27/2014 10:14 AM, Jan Beulich wrote: >>>>>>>> On 26.03.14 at 20:05, 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 the code in domain_build.c calling hap_set_allocation() directly for the time being, and implementing a paging_set_allocation() wrapper later; but I think the code doing the calculation should be in domain_build.c. -George