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

["Jan Beulich" <JBeulich@xxxxxxxx>]

>>> On 08.01.16 at 03:25, <konrad.wilk@xxxxxxxxxx> 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?
I ask because xen-syms isn't present on the boot partition, and
may not be present anywhere on an otherwise functioning
system.

> --- 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)?

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?

> +        if ( copy_to_guest_offset(arg, offsetof(xen_build_id_t, buf), p, sz) 
> )
> +            return -EFAULT;
> +
> +        return sz;
> +    }

Or how much got copied?

> +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


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