Re: [PATCH v2 2/5] xen: add generic flag to elf_dom_parms indicating support of unmapped initrd

[Juergen Gross <jgross@suse.com>]

On 10/02/2015 11:37 AM, Andrew Cooper wrote:
> On 02/10/15 06:49, Juergen Gross wrote:
>> Support of an unmapped initrd is indicated by the kernel of the domain
>> via elf notes. In order not to have to use raw elf data in the tools
>> for support of an unmapped initrd add a flag to the parsed data area
>> to indicate the kernel supporting this feature.
>>
>> Switch using this flag in the hypervisor domain builder.
>>
>> Cc: Keir Fraser <keir@xen.org>
>> Cc: Jan Beulich <jbeulich@suse.com>
>> Cc: Andrew Cooper <andrew.cooper3@citrix.com>
>> Cc: Tim Deegan <tim@xen.org>
>> Suggested-by: Ian Campbell <ian.campbell@citrix.com>
>> Signed-off-by: Juergen Gross <jgross@suse.com>
>> ---
>>   xen/arch/x86/domain_build.c        | 4 ++--
>>   xen/common/libelf/libelf-dominfo.c | 3 +++
>>   xen/include/xen/libelf.h           | 1 +
>>   3 files changed, 6 insertions(+), 2 deletions(-)
>>
>> diff --git a/xen/arch/x86/domain_build.c b/xen/arch/x86/domain_build.c
>> index c2ef87a..a02c9fb 100644
>> --- a/xen/arch/x86/domain_build.c
>> +++ b/xen/arch/x86/domain_build.c
>> @@ -353,7 +353,7 @@ static unsigned long __init compute_dom0_nr_pages(
>>
>>           vstart = parms->virt_base;
>>           vend = round_pgup(parms->virt_kend);
>> -        if ( !parms->elf_notes[XEN_ELFNOTE_MOD_START_PFN].data.num )
>> +        if ( !parms->mod_start_pfn )
>>               vend += round_pgup(initrd_len);
>>           end = vend + nr_pages * sizeof_long;
>>
>> @@ -1037,7 +1037,7 @@ int __init construct_dom0(
>>       v_start          = parms.virt_base;
>>       vkern_start      = parms.virt_kstart;
>>       vkern_end        = parms.virt_kend;
>> -    if ( parms.elf_notes[XEN_ELFNOTE_MOD_START_PFN].data.num )
>> +    if ( parms.mod_start_pfn )
>>       {
>>           vinitrd_start  = vinitrd_end = 0;
>>           vphysmap_start = round_pgup(vkern_end);
>> diff --git a/xen/common/libelf/libelf-dominfo.c b/xen/common/libelf/libelf-dominfo.c
>> index 3de1c23..31d8436 100644
>> --- a/xen/common/libelf/libelf-dominfo.c
>> +++ b/xen/common/libelf/libelf-dominfo.c
>> @@ -190,6 +190,9 @@ elf_errorstatus elf_xen_parse_note(struct elf_binary *elf,
>>       case XEN_ELFNOTE_INIT_P2M:
>>           parms->p2m_base = val;
>>           break;
>> +    case XEN_ELFNOTE_MOD_START_PFN:
>> +        parms->mod_start_pfn = !!val;
>> +        break;
>>       case XEN_ELFNOTE_PADDR_OFFSET:
>>           parms->elf_paddr_offset = val;
>>           break;
>> diff --git a/xen/include/xen/libelf.h b/xen/include/xen/libelf.h
>> index d7045f6..07b96a3 100644
>> --- a/xen/include/xen/libelf.h
>> +++ b/xen/include/xen/libelf.h
>> @@ -423,6 +423,7 @@ struct elf_dom_parms {
>>       char loader[16];
>>       enum xen_pae_type pae;
>>       bool bsd_symtab;
>> +    bool mod_start_pfn;
>
> The _pfn suffix here is confusing given the type of bool.
>
> Perhaps "has_initrd" is a better choice of name?  The rest of the patch
> looks fine.

Hmm, I just followed the naming of the note index in the raw data:
XEN_ELFNOTE_MOD_START_PFN. "has_initrd" would be completely misleading:
the flag doesn't indicate the support of an initrd, but the support of
an initrd (or more general: module as understood by grub) not covered by
the initial kernel mapping and due to this specified by it's starting
pfn instead it's virtual address.


Juergen