Re: [PATCH v3 2/3] XENVER_build_id: Provide ld-embedded build-ids (v8)

From: Konrad Rzeszutek Wilk <konrad.wilk@oracle.com>
To: Jan Beulich <JBeulich@suse.com>
Cc: wei.liu2@citrix.com, ian.campbell@citrix.com,
	andrew.cooper3@citrix.com, ian.jackson@eu.citrix.com,
	mpohlack@amazon.de, xen-devel@lists.xenproject.org,
	dgdegra@tycho.nsa.gov
Subject: Re: [PATCH v3 2/3] XENVER_build_id: Provide ld-embedded build-ids (v8)
Date: Tue, 12 Jan 2016 11:43:01 -0500	[thread overview]
Message-ID: <20160112164301.GC17685@char.us.oracle.com> (raw)
In-Reply-To: <5695370702000078000C6047@prv-mh.provo.novell.com>

On Tue, Jan 12, 2016 at 09:25:27AM -0700, Jan Beulich wrote:
> >>> On 08.01.16 at 03:25, <konrad.wilk@oracle.com> wrote:
> > The mechanism to get this is via the XENVER hypercall and
> > we add a new sub-command to retrieve the binary build-id
> > called XENVER_build_id. The sub-hypercall parameter
> > allows an arbitrary size (the buffer and len is provided
> > to the hypervisor). A NULL parameter will probe the hypervisor
> > for the length of the build-id.
> > 
> > One can also retrieve the value of the build-id by doing
> > 'readelf -h xen-syms'.
> 
> Does this or something similar also work on xen.gz and xen.efi?

Sadly no.
> I ask because xen-syms isn't present on the boot partition, and
> may not be present anywhere on an otherwise functioning
> system.

Right. Some later patches (xsplice related) hook the build-id printing
to a keyboard handler. Would that work ?

Or are you suggesting that perhaps the kernel should at boot time
print the build-id (like it does the changset)?

> 
> > --- a/Config.mk
> > +++ b/Config.mk
> > @@ -126,6 +126,17 @@ endef
> >  check-$(gcc) = $(call cc-ver-check,CC,0x040100,"Xen requires at least gcc-4.1")
> >  $(eval $(check-y))
> >  
> > +ld-ver = $(shell if [ $$((`$(1) --version | head -1 | sed 's/[^0-9]/ /g' | awk \
> > +           '{ printf "0x%02x%02x", $$1, $$2}'`)) -ge $$(($(2))) ]; \
> > +           then echo y; else echo n; fi ;)
> > +
> > +# binutils 2.18 implement build-id.
> > +ifeq ($(call ld-ver,$(LD),0x0212),n)
> > +build_id :=
> > +else
> > +build_id := --build-id=sha1
> > +endif
> 
> Wouldn't it be better to probe the linker for recognizing the --build-id
> command line option, along the lines of $(cc-option)?

Let me see what happens with that.
> 
> In any event the option should be added to LDFLAGS unless this
> conflicts with something else.
> 
> > --- a/xen/arch/x86/xen.lds.S
> > +++ b/xen/arch/x86/xen.lds.S
> > @@ -67,6 +67,12 @@ SECTIONS
> >         *(.rodata.*)
> >    } :text
> >  
> > +  .note.gnu.build-id : {
> > +      __note_gnu_build_id_start = .;
> > +      *(.note.gnu.build-id)
> > +      __note_gnu_build_id_end = .;
> > +  } :text
> 
> Wouldn't this better be a generic .note section, with .note.gnu.build-id
> just special cased ahead of *(.note) *(.note.*)?
> 
> > --- a/xen/common/kernel.c
> > +++ b/xen/common/kernel.c
> > @@ -366,6 +366,41 @@ DO(xen_version)(int cmd, XEN_GUEST_HANDLE_PARAM(void) arg)
> >              return -EFAULT;
> >          return 0;
> >      }
> > +    case XENVER_build_id:
> 
> Blank line ahead of case statements please when most/all others
> have it that way in a particular switch().
> 
> > +    {
> > +        xen_build_id_t build_id;
> > +        unsigned int sz = 0;
> > +        int rc = 0;
> > +        char *p = NULL;
> > +
> > +        if ( deny )
> > +            return -EPERM;
> > +
> > +        /* Only return size. */
> > +        if ( !guest_handle_is_null(arg) )
> > +        {
> > +            if ( copy_from_guest(&build_id, arg, 1) )
> > +                return -EFAULT;
> > +
> > +            if ( build_id.len == 0 )
> > +                return -EINVAL;
> > +        }
> > +
> > +        rc = xen_build_id(&p, &sz);
> 
> Perhaps use the helpers from xen/err.h to limit the amount of
> indirection needed?
> 
> > +        if ( rc )
> > +            return rc;
> > +
> > +        if ( guest_handle_is_null(arg) )
> > +            return sz;
> > +
> > +        if ( sz > build_id.len )
> > +            return -ENOBUFS;
> 
> And how will the caller know how much is needed?

Duh. I shall update the build_id.len with the appropiate value.
> 
> > +        if ( copy_to_guest_offset(arg, offsetof(xen_build_id_t, buf), p, sz) )
> > +            return -EFAULT;
> > +
> > +        return sz;
> > +    }
> 
> Or how much got copied?

That one is easy - we do return 'sz' which tells us how much got copied.

Or are you suggesting that the build_id.len should also be updated?
> 
> > +int xen_build_id(char **p, unsigned int *len)
> > +{
> > +    const Elf_Note *n = __note_gnu_build_id_start;
> > +    static bool_t checked = 0;
> 
> Pointless initializer.
> 
> Jan
> 