Re: [Xen-devel] [PATCH v6 1/4] xen: introduce SYMBOL

["Jan Beulich" <JBeulich@xxxxxxxx>]

>>> On 14.01.19 at 04:45, <Stewart.Hildebrand@xxxxxxxxxxxxxxx> wrote:
> So let's keep the linker-accessible variable as a type that works for the
> linker (which really could be anything as long as you use the address, not
> the value), but name it something else - a name that screams "DON'T USE ME
> UNLESS YOU KNOW WHAT YOU'RE DOING". And then before the first use, copy
> that value to "uintptr_t _start;".
> 
> The following is a quick proof of concept for aarch64. I changed the type
> of _start and _end, and added code to copy the linker-assigned value to
> _start and _end. Upon booting, I see the correct values:

Global symbols starting with underscores should already be shouting
enough. But what's worse - the whole idea if using array types is to
avoid the intermediate variables.

> --- a/xen/arch/arm/setup.c
> +++ b/xen/arch/arm/setup.c
> @@ -726,6 +726,12 @@ static void __init setup_mm(unsigned long dtb_paddr, 
> size_t dtb_size)
>  
>  size_t __read_mostly dcache_line_bytes;
>  
> +typedef char TYPE_DOESNT_MATTER;
> +extern TYPE_DOESNT_MATTER _start_linker_assigned_dont_use_me,
> +                          _end_linker_assigned_dont_use_me;

This and ...

> @@ -770,10 +776,17 @@ void __init start_xen(unsigned long boot_phys_offset,
>      printk("Command line: %s\n", cmdline);
>      cmdline_parse(cmdline);
>  
> +    _start = (uintptr_t)&_start_linker_assigned_dont_use_me;
> +    _end = (uintptr_t)&_end_linker_assigned_dont_use_me;

... this violates what the symbol names say. And if you want to
avoid issues, you'd want to keep out of C files uses of those
symbols altogether anyway, and you easily can: In any
assembly file, have

_start: .long _start_linker_assigned_dont_use_me
_end:   .long _end_linker_assigned_dont_use_me

In particular, they don't need to be runtime initialized, saving
you from needing to set them before first use. But as said -
things are the way they are precisely to avoid such variables.

> --- a/xen/Rules.mk
> +++ b/xen/Rules.mk
> @@ -54,7 +54,7 @@ CFLAGS += -fomit-frame-pointer
>  endif
>  
>  CFLAGS += -nostdinc -fno-builtin -fno-common
> -CFLAGS += -Werror -Wredundant-decls -Wno-pointer-arith
> +CFLAGS += -Wredundant-decls -Wno-pointer-arith

This I would consider bad even in a PoC. If you make a change
which causes compiler warnings, surely you now violate something
else.

>> But, instead of converting _start to unsigned long via SYMBOL_HIDE, we
>> could convert it to uintptr_t instead, it would be a trivial change on
>> top of the existing unsigned long series. Not sure if it is beneficial.
> 
> The difference would be whether we want to rely on implementation-defined
> behavior or not.

Why not? Simply specify that compilers with implementation defined
behavior not matching our expectations are unsuitable. And btw, I
suppose this is just the tiny tip of the iceberg of our reliance on
implementation defined behavior.

> In this case, whether "unsigned long" is wide enough to
> hold a pointer value or not.

This is a basic assumption of UNIX and its derivatives, afaik.

Jan



_______________________________________________
Xen-devel mailing list
Xen-devel@xxxxxxxxxxxxxxxxxxxx
https://lists.xenproject.org/mailman/listinfo/xen-devel