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 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?


> > 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?

Ian.