Re: [PATCH v2 3/5] libxc: create unmapped initrd in domain builder if supported

[Ian Campbell <ian.campbell@citrix.com>]

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 <jgross@suse.com>
> > > > > ---
> > > > >    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.

Ian.