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

[Julien Grall <julien.grall@xxxxxxx>]

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,

--
Julien Grall

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