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

[Andrew Cooper <andrew.cooper3@citrix.com>]

On 02/10/15 10:44, Juergen Gross wrote:
> 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.

It would appear that a lot of the naming around there is confusing.

Would "unmapped_initrd" be a better name then?

If not, I am not too fussed, seeing as it matches the (questionably
named) ELF note.

~Andrew