Re: [Xen-devel] [PATCH v2] make use of .startof.() and .sizeof.() assembler expressions

[Julien Grall <julien.grall@xxxxxxx>]

Hi Jan,

On 30/08/16 14:09, Jan Beulich wrote:
> > > > On 30.08.16 at 14:50, <julien.grall@xxxxxxx> wrote:
> > On 24/08/16 08:44, Jan Beulich wrote:
> > > --- a/xen/arch/arm/device.c
> > > +++ b/xen/arch/arm/device.c
> > > @@ -21,8 +21,8 @@
> > >  #include <xen/errno.h>
> > >  #include <xen/lib.h>
> > >
> > > -extern const struct device_desc _sdevice[], _edevice[];
> > > -extern const struct acpi_device_desc _asdevice[], _aedevice[];
> > > +extern const struct device_desc _sdevice[] asm(".startof.(.dev.info)");
> > > +extern const struct device_desc _edevice[];
> > >
> > >  int __init device_init(struct dt_device_node *dev, enum device_class class,
> > >                         const void *data)
> > > @@ -51,6 +51,11 @@ int __init device_init(struct dt_device_
> > >      return -EBADF;
> > >  }
> > >
> > > +#ifdef CONFIG_ACPI
> > Why did you add the #ifdef CONFIG_ACPI here? It is not explained in the
> > commit message.
> .startof.(section_which_not_exist) gives a linker error. And ld
> doesn't add to the section table any sections mentioned in the
> linker script, but without constituents.
This looks quite fragile, with this patch you are now assuming that all 
the sections will not be empty. As we go towards a modular Xen, this may 
lead to more linker error.
For instance, it might be possible in the future to not compiled 
unnecessary platform specific code. As not all the boards requires 
platform specific code, the section may get empty.
> > If you want to add it, it should be a separate patch and we should go
> > further by removing the section in the linker script.
> While I don't think this needs to be broken out, it certainly can (but
> then really there should have been a conditional like the one I
> introduce from the beginning, to avoid having dead code in a non-
> ACPI enabled binary).
It was an oversight while reviewing the patch adding acpi_device_init 
and I thought you added CONFIG_ACPI just for comestic.
> > > --- a/xen/arch/arm/xen.lds.S
> > > +++ b/xen/arch/arm/xen.lds.S
> > > @@ -31,7 +31,6 @@ SECTIONS
> > >    . = XEN_VIRT_START;
> > >    _start = .;
> > >    .text : /* XXX should be AT ( XEN_PHYS_START ) */ {
> > > -        _stext = .;            /* Text section */
> > I don't see any removal of _stext in xen/arch/arm/alternative.c.
> Why should that be removed? _stext doesn't cease to exist:
Oh, sorry I got confused.

> --- a/xen/include/xen/kernel.h
> +++ b/xen/include/xen/kernel.h
> @@ -71,24 +71,34 @@ extern char _start[], _end[], start[];
>      (__p >= _start) && (__p < _end);            \
>  })
>
> -extern char _stext[], _etext[];
> +extern const char _stext[] asm(".startof.(.text)");
> +extern const char _etext[];
>  #define is_kernel_text(p) ({                    \
>
> Jan
Regards,

--
Julien Grall

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