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

[Ian Campbell <ian.campbell@xxxxxxxxxx>]

On Fri, 2015-10-02 at 12:01 +0200, Juergen Gross wrote:
> On 10/02/2015 11:53 AM, Andrew Cooper wrote:
> > 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@xxxxxxx>
> > > > > Cc: Jan Beulich <jbeulich@xxxxxxxx>
> > > > > Cc: Andrew Cooper <andrew.cooper3@xxxxxxxxxx>
> > > > > Cc: Tim Deegan <tim@xxxxxxx>
> > > > > Suggested-by: Ian Campbell <ian.campbell@xxxxxxxxxx>
> > > > > Signed-off-by: Juergen Gross <jgross@xxxxxxxx>
> > > > > ---
> > > > >    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?
> 
> Hmm, in theory, yes.
> 
> The question is, whether the name should fit to the elf note's name or
> to it's semantics. The name of the elf note will be used in libelf and
> in the Linux kernel (and possibly other kernels as well), while the
> flag name will be used in the domain builders (hypervisor and tools).
> 
> Which flag name will be less confusing?

My Â0.02:

Given that this is in effect a kind of abstraction over the implementation
of what is in the ELF notes I think it would be fine and somewhat desirable
to have a clearer name at this layer (accepting that the layering is a bit
vague/implicit/etc).

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

_______________________________________________
Xen-devel mailing list
Xen-devel@xxxxxxxxxxxxx
http://lists.xen.org/xen-devel