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

[Stewart Hildebrand <Stewart.Hildebrand@xxxxxxxxxxxxxxx>]

On Friday, January 11, 2019 4:38 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.

Ah, I see. OK. See [1] for further explanation of why this is the case. So
I guess we'll just have to work around that.

I don't mean a uintptr_t[], because that's an array/pointer type. I mean
"uintptr_t" the integer type. I recognize that there are risks of going
from a pointer type to an integer type, since any operations that relied
on pointer arithmetic have to be changed to account for integer
arithmetic.

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:

(XEN) sizeof(uintptr_t): 8
(XEN) _start: 0x00200000
(XEN) _end:   0x00320d80

(please keep reading after the patch)

From: Stewart Hildebrand <stewart.hildebrand@xxxxxxxxxxxxxxx>
Date: Sun, 13 Jan 2019 21:10:43 -0500
Subject: [PATCH RFC] Proof of concept: change _start/_end to uintptr_t

Signed-off-by: Stewart Hildebrand <stewart.hildebrand@xxxxxxxxxxxxxxx>

---
 xen/include/xen/kernel.h |  3 ++-
 xen/arch/arm/xen.lds.S   |  4 ++--
 xen/arch/arm/setup.c     | 17 +++++++++++++++--
 xen/arch/arm/mm.c        |  4 ++--
 xen/Rules.mk             |  2 +-
 5 files changed, 22 insertions(+), 8 deletions(-)

diff --git a/xen/include/xen/kernel.h b/xen/include/xen/kernel.h
index 548b64da9f..ec7d10bb75 100644
--- a/xen/include/xen/kernel.h
+++ b/xen/include/xen/kernel.h
@@ -65,7 +65,8 @@
        1;                                      \
 })
 
-extern char _start[], _end[], start[];
+extern uintptr_t _start, _end;
+extern char start[];
 #define is_kernel(p) ({                         \
     char *__p = (char *)(unsigned long)(p);     \
     (__p >= _start) && (__p < _end);            \
diff --git a/xen/arch/arm/xen.lds.S b/xen/arch/arm/xen.lds.S
index 1e72906477..c837dd534a 100644
--- a/xen/arch/arm/xen.lds.S
+++ b/xen/arch/arm/xen.lds.S
@@ -28,7 +28,7 @@ PHDRS
 SECTIONS
 {
   . = XEN_VIRT_START;
-  _start = .;
+  _start_linker_assigned_dont_use_me = .;
   .text : {
         _stext = .;            /* Text section */
        *(.text)
@@ -205,7 +205,7 @@ SECTIONS
        __per_cpu_data_end = .;
        __bss_end = .;
   } :text
-  _end = . ;
+  _end_linker_assigned_dont_use_me = . ;
 
 #ifdef CONFIG_DTB_FILE
   /* Section for the device tree blob (if any). */
diff --git a/xen/arch/arm/setup.c b/xen/arch/arm/setup.c
index 444857a967..fe17a86384 100644
--- 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;
+
+uintptr_t _start, _end;
+
 /* C entry point for boot CPU */
 void __init start_xen(unsigned long boot_phys_offset,
                       unsigned long fdt_paddr,
@@ -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;
+
+    printk("sizeof(uintptr_t): %ld\n", sizeof(uintptr_t));
+    printk("_start: 0x%08" PRIxPTR "\n", _start);
+    printk("_end:   0x%08" PRIxPTR "\n", _end);
+
     /* Register Xen's load address as a boot module. */
     xen_bootmodule = add_boot_module(BOOTMOD_XEN,
-                             (paddr_t)(uintptr_t)(_start + boot_phys_offset),
-                             (paddr_t)(uintptr_t)(_end - _start + 1), false);
+        (paddr_t)(uintptr_t)(_start + boot_phys_offset * sizeof(char*)),
+        (paddr_t)(uintptr_t)(_end - _start + sizeof(char*)), false);
     BUG_ON(!xen_bootmodule);
 
     setup_pagetables(boot_phys_offset);
diff --git a/xen/arch/arm/mm.c b/xen/arch/arm/mm.c
index 01ae2cccc0..b4fd0381d1 100644
--- a/xen/arch/arm/mm.c
+++ b/xen/arch/arm/mm.c
@@ -1084,8 +1084,8 @@ static void set_pte_flags_on_range(const char *p, 
unsigned long l, enum mg mg)
     ASSERT(!((unsigned long) p & ~PAGE_MASK));
     ASSERT(!(l & ~PAGE_MASK));
 
-    for ( i = (p - _start) / PAGE_SIZE; 
-          i < (p + l - _start) / PAGE_SIZE; 
+    for ( i = (p - (const char *)_start) / PAGE_SIZE;
+          i < (p + l - (const char *)_start) / PAGE_SIZE;
           i++ )
     {
         pte = xen_xenmap[i];
diff --git a/xen/Rules.mk b/xen/Rules.mk
index a151b3f625..a05ceec1e5 100644
--- 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
 $(call cc-option-add,CFLAGS,CC,-Wvla)
 CFLAGS += -pipe -D__XEN__ -include $(BASEDIR)/include/xen/config.h
 CFLAGS-$(CONFIG_DEBUG_INFO) += -g
-- 
2.17.1



I'm not sure if start_xen() is the first actual use of _start/_end, but it
seemed good enough to verify that _start and _end were being assigned
properly. I removed -Werror due to "comparison between pointer and
integer" warnings. Additionally, since this is a switch from pointer
arithmetic to integer arithmetic, we need to add
"* sizeof(some_pointer_type)" in a few places. I only added this in one
place for the proof of concept, so as you might expect, it didn't finish
booting.

Does it make sense to change the type of all variables that could be
considered "pointers to different objects"? If we intend to do any sort of
subtraction/comparison between them (i.e. violate MISRA rules and venture
into the territory of undefined behavior), then yes, they should all be
changed. I believe that's a small price to pay to take a step toward MISRA
conformance and not risk the compiler making certain optimizations that
could break the code.

> 
> 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. In this case, whether "unsigned long" is wide enough to
hold a pointer value or not. I recognize that in most implementations and
architectures it is, but it's still not strictly guaranteed per the
definition of the type of "unsigned long" as it is with "uintptr_t".

Lastly, also possibly of interest, while playing around with the proof of
concept, I did also manage to get GCC 7.3 to emit this warning (by
removing the "extern" declaration and adding back the array declaration):
setup.c:731:27: warning: array ‘_end_linker_assigned_dont_use_me’ assumed to 
have one element
                           _end_linker_assigned_dont_use_me[];
                           ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~

Stew

[1] 
http://docs.adacore.com/live/wave/binutils-stable/html/ld/ld.html#Source-Code-Reference
_______________________________________________
Xen-devel mailing list
Xen-devel@xxxxxxxxxxxxxxxxxxxx
https://lists.xenproject.org/mailman/listinfo/xen-devel