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

[Juergen Gross <jgross@xxxxxxxx>]

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@xxxxxxxx>
> > ---
> >   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).

> 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. :-)

> 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. I think I'll change this when I'm
modifying the allocation interface (see my answer to patch 5).

> I'm also not clear how/where the virtual mapping is avoided, given that
> this code here does strictly more than the original code above which is now
> made conditional does.
The page tables are built before. So they don't cover the initrd memory.


Juergen


_______________________________________________
Xen-devel mailing list
Xen-devel@xxxxxxxxxxxxx
http://lists.xen.org/xen-devel