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

[Juergen Gross <jgross@suse.com>]

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

> 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