[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

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



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

 


Rackspace

Lists.xenproject.org is hosted with RackSpace, monitoring our
servers 24x7x365 and backed by RackSpace's Fanatical Support®.