From mboxrd@z Thu Jan 1 00:00:00 1970 From: Juergen Gross Subject: Re: [PATCH v2 3/5] libxc: create unmapped initrd in domain builder if supported Date: Fri, 2 Oct 2015 17:13:05 +0200 Message-ID: <560E9F01.6000301@suse.com> References: <1443764987-23639-1-git-send-email-jgross@suse.com> <1443764987-23639-4-git-send-email-jgross@suse.com> <1443790779.11707.95.camel@citrix.com> <560E98DC.4030805@suse.com> <1443797802.11707.134.camel@citrix.com> Mime-Version: 1.0 Content-Type: text/plain; charset="us-ascii"; Format="flowed" Content-Transfer-Encoding: 7bit Return-path: In-Reply-To: <1443797802.11707.134.camel@citrix.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: Ian Campbell , xen-devel@lists.xen.org, ian.jackson@eu.citrix.com, stefano.stabellini@eu.citrix.com, wei.liu2@citrix.com List-Id: xen-devel@lists.xenproject.org On 10/02/2015 04:56 PM, Ian Campbell wrote: > On Fri, 2015-10-02 at 16:46 +0200, Juergen Gross wrote: >> On 10/02/2015 02:59 PM, Ian Campbell wrote: >>> On Fri, 2015-10-02 at 07:49 +0200, Juergen Gross wrote: >>>> In case the kernel of a new pv-domU indicates it is supporting an >>>> unmapped initrd, don't waste precious virtual space for the initrd, >>>> but allocate only guest physical memory for it. >>>> >>>> Signed-off-by: Juergen Gross >>>> --- >>>> tools/libxc/xc_dom_core.c | 22 ++++++++++++++++++++-- >>>> 1 file changed, 20 insertions(+), 2 deletions(-) >>>> >>>> diff --git a/tools/libxc/xc_dom_core.c b/tools/libxc/xc_dom_core.c >>>> index b510bbd..85b531a 100644 >>>> --- a/tools/libxc/xc_dom_core.c >>>> +++ b/tools/libxc/xc_dom_core.c >>>> @@ -1019,8 +1019,9 @@ int xc_dom_build_image(struct xc_dom_image >>>> *dom) >>>> if ( dom->kernel_loader->loader(dom) != 0 ) >>>> goto err; >>>> >>>> - /* load ramdisk */ >>>> - if ( dom->ramdisk_blob ) >>>> + /* Load ramdisk if initial mapping required. */ >>>> + if ( dom->ramdisk_blob && >>>> + (!dom->parms.mod_start_pfn || dom->ramdisk_seg.vstart) ) >>>> { >>>> if ( xc_dom_build_ramdisk(dom) != 0 ) >>>> goto err; >>>> @@ -1063,6 +1064,23 @@ int xc_dom_build_image(struct xc_dom_image >>>> *dom) >>>> __FUNCTION__, dom->virt_alloc_end); >>>> DOMPRINTF("%-20s: virt_pgtab_end : 0x%" PRIx64 "", >>>> __FUNCTION__, dom->virt_pgtab_end); >>>> + >>>> + /* Prepare allocating unmapped memory. */ >>>> + if ( dom->virt_pgtab_end ) >>>> + dom->virt_alloc_end = dom->virt_pgtab_end; >>>> + >>>> + /* Load ramdisk if no initial mapping required. */ >>>> + if ( dom->ramdisk_blob && !dom->ramdisk_seg.vstart && >>>> + dom->parms.mod_start_pfn ) >>>> + { >>>> + if ( xc_dom_build_ramdisk(dom) != 0 ) >>>> + goto err; >>>> + dom->flags |= SIF_MOD_START_PFN; >>>> + dom->ramdisk_seg.vend = dom->ramdisk_seg.vend - dom >>>> ->ramdisk_seg.vstart; >>>> + dom->ramdisk_seg.vstart = dom->ramdisk_seg.pfn; >>>> + dom->ramdisk_seg.vend += dom->ramdisk_seg.vstart; >>> >>> This seems like it is trying to do something clever, like partially >>> reversing something which the xc_dom_alloc_segment call in >>> xc_dom_build_ramdisk has done. >> >> It's just changing the boundaries of the initrd to fit the interface >> for it being not mapped (indicated by the SIF_MOD_START_PFN flag). > > So something has initialised these fields as if it were mapped and we are > now undoing it because in reality it is not? IOW something has arranged > things such that vstart and vend are invalid before this adjustment. > > So whatever is making these allocations and mapping (or not) them should > instead be fixed to just do things correctly from the start. > > Am I correct that after this the vstart and vend actually contain physical > addresses? Does anything use them that way, and how does it know if they > are physical or virtual? > > Perhaps the right answer is to add pstart and pend and to initialise those > correctly (for everything) while leaving the v* ones set to some sentinel > value to indicate when things are unmapped? I want to do something like this, yes. >>> It looks like the vend handling in particular is just a complicated way >>> of >>> subtracting vstart and adding pfn, with the aim of rebasing from virt >>> to >>> phys world. >> >> Hmm, not more complicated as: >> >> len = dom->ramdisk_seg.vend - dom->ramdisk_seg.vstart; >> dom->ramdisk_seg.vstart = dom->ramdisk_seg.pfn; >> dom->ramdisk_seg.vend = dom->ramdisk_seg.pfn + len; >> >> I have to admit that above variant might be easier to understand. :-) > > It is, much easier. :-) >>> Plus vstart/end are addresses, while presumably pfn is a page number, >>> so >>> I'm confused about that as well. >> >> The naming is irritating, yes. > > Are you saying that pfn is actually a physical address? It's a page number. The start_info page data regarding the initrd is taken from dom->ramdisk_seg. The length is computed from vend - vstart and the start of the initrd is either a virtual address or a pfn. I guess it would be a good idea to add something like mod_start and mod_len to struct xc_dom_image and init those according to the allocation (virtual or physical). start_info can then be filled from those new members. Juergen