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 18:28:32 +0200 Message-ID: <560EB0B0.6040702@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> <560E9F01.6000301@suse.com> <1443799263.11707.137.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: <1443799263.11707.137.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 05:21 PM, Ian Campbell wrote: > On Fri, 2015-10-02 at 17:13 +0200, Juergen Gross wrote: >> 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. > > So are vend and vstart frame numbers then? > > They had better be, because otherwise the length you are computing is in > bytes, so adding it to a frame number in vstart makes no sense. The length is always in bytes. The start is either a virtual address or a frame number. vend is making only sense if you subtract vstart. I think we can stop discussing this now, as the next version of the series should be much clearer in this regard. Juergen