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@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel