Re: [PATCH v3 6/9] libxc: create unmapped initrd in domain builder if supported

[Juergen Gross <jgross@suse.com>]

On 10/28/2015 05:11 PM, Wei Liu wrote:
> On Tue, Oct 13, 2015 at 03:11:15PM +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/include/xc_dom.h |  5 +++++
>>   tools/libxc/xc_dom_core.c    | 19 +++++++++++++++++--
>>   tools/libxc/xc_dom_x86.c     |  8 ++++----
>>   3 files changed, 26 insertions(+), 6 deletions(-)
>>
>> diff --git a/tools/libxc/include/xc_dom.h b/tools/libxc/include/xc_dom.h
>> index b0120a6..fa772a9 100644
>> --- a/tools/libxc/include/xc_dom.h
>> +++ b/tools/libxc/include/xc_dom.h
>> @@ -94,6 +94,11 @@ struct xc_dom_image {
>>       xen_pfn_t pfn_alloc_end;
>>       xen_vaddr_t virt_alloc_end;
>>       xen_vaddr_t bsd_symtab_start;
>> +
>> +    /* initrd parameters as specified in start_info page */
>> +    unsigned long initrd_start;
>> +    unsigned long initrd_len;
>> +
>>       unsigned int alloc_bootstack;
>>       xen_vaddr_t virt_pgtab_end;
>>
>> diff --git a/tools/libxc/xc_dom_core.c b/tools/libxc/xc_dom_core.c
>> index 8e1e17f..15e9fa3 100644
>> --- a/tools/libxc/xc_dom_core.c
>> +++ b/tools/libxc/xc_dom_core.c
>> @@ -1041,6 +1041,7 @@ static int xc_dom_build_ramdisk(struct xc_dom_image *dom)
>>   int xc_dom_build_image(struct xc_dom_image *dom)
>>   {
>>       unsigned int page_size;
>> +    bool unmapped_initrd;
>>
>>       DOMPRINTF_CALLED(dom->xch);
>>
>> @@ -1064,11 +1065,15 @@ 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. */
>> +    unmapped_initrd = dom->parms.unmapped_initrd && !dom->ramdisk_seg.vstart;
>
> A minor suggestion, the comment is describing the reverse logic of the
> statement that immediately follows it, can you make the comment and
> statement match?

Sure.

> I think I manage to work this out: if ramdisk_seg.vstart is set that
> means upper layer wants the ramdisk to be mapped at that address (ARM is
> doing that), so in that case even if kernel supports unmapped initrd we
> should still map the ramdisk. Correct me if I'm wrong.

That's how it was meant to be.

> The rest of code looks correct. With that understand (and whether you
> follow my minor suggestion or not):
>
> Acked-by: Wei Liu <wei.liu2@citrix.com>
>

Thanks,

Juergen