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

[Juergen Gross <jgross@suse.com>]

On 10/02/2015 05:21 PM, Ian Campbell wrote:
> 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.

The length is always in bytes. The start is either a virtual address or
a frame number. vend is making only sense if you subtract vstart.

I think we can stop discussing this now, as the next version of the
series should be much clearer in this regard.


Juergen