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

[Julien Grall <julien.grall@xxxxxxx>]

Hi,

On 1/15/19 11:46 AM, Julien Grall wrote:
> Hi Stefano,
>
> On 1/11/19 9:37 PM, Stefano Stabellini wrote:
> > On Fri, 11 Jan 2019, Stewart Hildebrand wrote:
> > > On Friday, January 11, 2019 3:36 PM, Julien Grall wrote:
> > > > On Fri, 11 Jan 2019, 12:53 Stewart Hildebrand wrote:
> > > > > Why don't we change the type of _start so it's not a pointer type?
> > > > Can you suggest a type that would be suitable?
> > > >
> > > > Cheers,
> > > Yes. My opinion is that the "sufficient-width integer type" should be a
> > > "uintptr_t" or "intptr_t", since those types by definition are 
> > > *integer* types
> > > wide enough to hold a value converted from a void pointer. While 
> > > "unsigned
> > > long" seems to work for Linux, the definition of that type doesn't 
> > > provide the
> > > same guarantee. Since uintptr_t is an *integer* type by definition 
> > > (and not a
> > > pointer type), my interpretation of the C standard is that
> > > subtraction/comparison of uintptr_t types won't be subject to the 
> > > potential
> > > "pointer to object" issues in question.
> > >
> > > If I had to choose between "uintptr_t" or "intptr_t" I guess I would 
> > > choose
> > > "uintptr_t" since that type is already used in various places in the Xen
> > > codebase. And the Linux workaround is also using an unsigned integer 
> > > type.
> > On changing type of _start & friends: we cannot declare _start as
> > uintptr_t, the linker won't be able to set the value. It needs to be an
> > array type. At that point, it is basically a pointer, it doesn't matter
> > if it is a char[] or uintptr_t[]. It won't help.
> Are you sure about this? I wrote a quick patch (see below) to switch 
> _start/_end to uintptr_t and didn't notice any specific linker issue. I 
> borrowed the idea from ATF which have been using uintptr_t for linker 
> symbol.
> diff --git a/xen/arch/arm/mm.c b/xen/arch/arm/mm.c
> index 340a1d1548..ab98cabbb7 100644
> --- a/xen/arch/arm/mm.c
> +++ b/xen/arch/arm/mm.c
> @@ -1073,10 +1073,11 @@ int modify_xen_mappings(unsigned long s, 
> unsigned long e, unsigned int flags)
>   }
>
>   enum mg { mg_clear, mg_ro, mg_rw, mg_rx };
> -static void set_pte_flags_on_range(const char *p, unsigned long l, enum 
> mg mg)
> +static void set_pte_flags_on_range(const char *__p, unsigned long l, 
> enum mg mg)
>   {
>       lpae_t pte;
>       int i;
> +    uintptr_t p = (uintptr_t)__p;
>
>       ASSERT(is_kernel(p) && is_kernel(p + l));
>
> diff --git a/xen/include/xen/kernel.h b/xen/include/xen/kernel.h
> index 548b64da9f..94bb08fc65 100644
> --- a/xen/include/xen/kernel.h
> +++ b/xen/include/xen/kernel.h
> @@ -65,9 +65,9 @@
>          1;                                      \
>   })
>
> -extern char _start[], _end[], start[];
> +extern uintptr_t _start, _end, start;
>   #define is_kernel(p) ({                         \
> -    char *__p = (char *)(unsigned long)(p);     \
> +    uintptr_t __p = (uintptr_t)(p);             \
>       (__p >= _start) && (__p < _end);            \
>   })
>
> Cheers,
Please ignore this patch, I was wrong here. Sorry for the noise.

Cheers,

--
Julien Grall

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