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

[Juergen Gross <jgross@suse.com>]

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.

I guess it would be a good idea to add something like mod_start and
mod_len to struct xc_dom_image and init those according to the
allocation (virtual or physical). start_info can then be filled from
those new members.


Juergen