Hi Jan,
One question below to make a decision on the way forward.
On Mon, 14 Jan 2019, Jan Beulich wrote:
> >>> On 14.01.19 at 04:45, <stewart.hildebr...@dornerworks.com> wrote:
> > 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:
>
> Global symbols starting with underscores should already be shouting
> enough. But what's worse - the whole idea if using array types is to
> avoid the intermediate variables.
>
> > --- 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;
>
> This and ...
>
> > @@ -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;
>
> ... this violates what the symbol names say. And if you want to
> avoid issues, you'd want to keep out of C files uses of those
> symbols altogether anyway, and you easily can: In any
> assembly file, have
>
> _start: .long _start_linker_assigned_dont_use_me
> _end: .long _end_linker_assigned_dont_use_me
>
> In particular, they don't need to be runtime initialized, saving
> you from needing to set them before first use. But as said -
> things are the way they are precisely to avoid such variables.
>
> >> 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.
>
> Why not? Simply specify that compilers with implementation defined
> behavior not matching our expectations are unsuitable. And btw, I
> suppose this is just the tiny tip of the iceberg of our reliance on
> implementation defined behavior.
The reason is that relying on undefined behavior is not reliable, it is
not C compliant, it is not allowed by MISRA-C, and not guaranteed to
work with any compiler. Yes, this instance is only the tip of the
iceberg, we have a long road ahead, but we shouldn't really give up
because it is going to be difficult :-) Stewart's approach would
actually be compliant and help toward reducing reliance on undefined
behavior.
Would you be OK if I rework the series to follow his approach using
intermediate variables? See the attached patch as a reference, it only
"converts" _start and _end as an example. Fortunately, it will be
textually similar to the previous SYMBOL returning unsigned long version
of the series.
If you are OK with it, do you have any suggestions on how would you like
the intermediate variables to be called? I went with _start/start_ and
_end/end_ but I am open to suggestions. Also to which assembly file you
would like the new variables being added -- I created a new one for the
purpose named var.S in the attached example.diff --git a/xen/arch/arm/alternative.c b/xen/arch/arm/alternative.c
index 52ed7ed..b79536d 100644
--- a/xen/arch/arm/alternative.c
+++ b/xen/arch/arm/alternative.c
@@ -187,8 +187,8 @@ static int __apply_alternatives_multi_stop(void *unused)
{
int ret;
struct alt_region region;
- mfn_t xen_mfn = virt_to_mfn(_start);
- paddr_t xen_size = _end - _start;
+ mfn_t xen_mfn = virt_to_mfn(start_);
+ paddr_t xen_size = end_ - start_;
unsigned int xen_order = get_order_from_bytes(xen_size);
void *xenmap;
@@ -206,7 +206,7 @@ static int __apply_alternatives_multi_stop(void *unused)
region.begin = __alt_instructions;
region.end = __alt_instructions_end;
- ret = __apply_alternatives(®ion, xenmap - (void *)_start);
+ ret = __apply_alternatives(®ion, (uintptr_t)xenmap - start_);
/* The patching is not expected to fail during boot. */
BUG_ON(ret != 0);
diff --git a/xen/arch/arm/arm32/Makefile b/xen/arch/arm/arm32/Makefile
index 0ac254f..983fb82 100644
--- a/xen/arch/arm/arm32/Makefile
+++ b/xen/arch/arm/arm32/Makefile
@@ -10,4 +10,5 @@ obj-y += proc-v7.o proc-caxx.o
obj-y += smpboot.o
obj-y += traps.o
obj-y += vfp.o
+obj-y += var.o
diff --git a/xen/arch/arm/arm32/livepatch.c b/xen/arch/arm/arm32/livepatch.c
index 41378a5..ad9e9ca 100644
--- a/xen/arch/arm/arm32/livepatch.c
+++ b/xen/arch/arm/arm32/livepatch.c
@@ -56,7 +56,7 @@ void arch_livepatch_apply(struct livepatch_func *func)
else
insn = 0xe1a00000; /* mov r0, r0 */
- new_ptr = func->old_addr - (void *)_start + vmap_of_xen_text;
+ new_ptr = (uintptr_t)func->old_addr - start_ + (uintptr_t)vmap_of_xen_text;
len = len / sizeof(uint32_t);
/* PATCH! */
diff --git a/xen/arch/arm/arm32/var.S b/xen/arch/arm/arm32/var.S
new file mode 100644
index 0000000..f8b94a1
--- /dev/null
+++ b/xen/arch/arm/arm32/var.S
@@ -0,0 +1,4 @@
+GLOBAL(start_)
+ .long _start
+GLOBAL(end_)
+ .long _end
diff --git a/xen/arch/arm/arm64/Makefile b/xen/arch/arm/arm64/Makefile
index c4f3a28..a5d9558 100644
--- a/xen/arch/arm/arm64/Makefile
+++ b/xen/arch/arm/arm64/Makefile
@@ -13,3 +13,4 @@ obj-y += smpboot.o
obj-y += traps.o
obj-y += vfp.o
obj-y += vsysreg.o
+obj-y += var.o
diff --git a/xen/arch/arm/arm64/livepatch.c b/xen/arch/arm/arm64/livepatch.c
index 2247b92..60616d6 100644
--- a/xen/arch/arm/arm64/livepatch.c
+++ b/xen/arch/arm/arm64/livepatch.c
@@ -43,7 +43,7 @@ void arch_livepatch_apply(struct livepatch_func *func)
/* Verified in livepatch_verify_distance. */
ASSERT(insn != AARCH64_BREAK_FAULT);
- new_ptr = func->old_addr - (void *)_start + vmap_of_xen_text;
+ new_ptr = (uintptr_t)func->old_addr - start_ + (uintptr_t)vmap_of_xen_text;
len = len / sizeof(uint32_t);
/* PATCH! */
diff --git a/xen/arch/arm/arm64/var.S b/xen/arch/arm/arm64/var.S
new file mode 100644
index 0000000..566e06c
--- /dev/null
+++ b/xen/arch/arm/arm64/var.S
@@ -0,0 +1,4 @@
+GLOBAL(start_)
+ .quad _start
+GLOBAL(end_)
+ .quad _end
diff --git a/xen/arch/arm/livepatch.c b/xen/arch/arm/livepatch.c
index 279d52c..7ab66d3 100644
--- a/xen/arch/arm/livepatch.c
+++ b/xen/arch/arm/livepatch.c
@@ -26,8 +26,8 @@ int arch_livepatch_quiesce(void)
if ( vmap_of_xen_text )
return -EINVAL;
- text_mfn = virt_to_mfn(_start);
- text_order = get_order_from_bytes(_end - _start);
+ text_mfn = virt_to_mfn(start_);
+ text_order = get_order_from_bytes(end_ - start_);
/*
* The text section is read-only. So re-map Xen to be able to patch
@@ -78,7 +78,7 @@ void arch_livepatch_revert(const struct livepatch_func *func)
uint32_t *new_ptr;
unsigned int len;
- new_ptr = func->old_addr - (void *)_start + vmap_of_xen_text;
+ new_ptr = (uintptr_t)func->old_addr - start_ + (uintptr_t)vmap_of_xen_text;
len = livepatch_insn_len(func);
memcpy(new_ptr, func->opaque, len);
diff --git a/xen/arch/arm/mm.c b/xen/arch/arm/mm.c
index 01ae2cc..2a083be 100644
--- a/xen/arch/arm/mm.c
+++ b/xen/arch/arm/mm.c
@@ -1073,7 +1073,7 @@ 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 uintptr_t p, unsigned long l, enum mg
mg)
{
lpae_t pte;
int i;
@@ -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 - start_) / PAGE_SIZE;
+ i < (p + l - start_) / PAGE_SIZE;
i++ )
{
pte = xen_xenmap[i];
@@ -1127,7 +1127,7 @@ void free_init_memory(void)
unsigned int i, nr = len / sizeof(insn);
uint32_t *p;
- set_pte_flags_on_range(__init_begin, len, mg_rw);
+ set_pte_flags_on_range((uintptr_t)__init_begin, len, mg_rw);
#ifdef CONFIG_ARM_32
/* udf instruction i.e (see A8.8.247 in ARM DDI 0406C.c) */
insn = 0xe7f000f0;
@@ -1138,7 +1138,7 @@ void free_init_memory(void)
for ( i = 0; i < nr; i++ )
*(p + i) = insn;
- set_pte_flags_on_range(__init_begin, len, mg_clear);
+ set_pte_flags_on_range((uintptr_t)__init_begin, len, mg_clear);
init_domheap_pages(pa, pa + len);
printk("Freed %ldkB init memory.\n", (long)(__init_end-__init_begin)>>10);
}
diff --git a/xen/arch/arm/setup.c b/xen/arch/arm/setup.c
index 444857a..9a0df43 100644
--- a/xen/arch/arm/setup.c
+++ b/xen/arch/arm/setup.c
@@ -772,8 +772,8 @@ void __init start_xen(unsigned long boot_phys_offset,
/* 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);
+ (start_ + boot_phys_offset),
+ (end_ - start_ + 1), false);
BUG_ON(!xen_bootmodule);
setup_pagetables(boot_phys_offset);
diff --git a/xen/arch/x86/boot/Makefile b/xen/arch/x86/boot/Makefile
index e103882..8327d2a 100644
--- a/xen/arch/x86/boot/Makefile
+++ b/xen/arch/x86/boot/Makefile
@@ -1,4 +1,5 @@
obj-bin-y += head.o
+obj-bin-y += var.o
DEFS_H_DEPS = defs.h $(BASEDIR)/include/xen/stdbool.h
diff --git a/xen/arch/x86/boot/var.S b/xen/arch/x86/boot/var.S
new file mode 100644
index 0000000..566e06c
--- /dev/null
+++ b/xen/arch/x86/boot/var.S
@@ -0,0 +1,4 @@
+GLOBAL(start_)
+ .quad _start
+GLOBAL(end_)
+ .quad _end
diff --git a/xen/arch/x86/setup.c b/xen/arch/x86/setup.c
index 06eb483..7d6c3ec 100644
--- a/xen/arch/x86/setup.c
+++ b/xen/arch/x86/setup.c
@@ -1039,7 +1039,7 @@ void __init noreturn __start_xen(unsigned long mbi_p)
* Is the region size greater than zero and does it begin
* at or above the end of current Xen image placement?
*/
- if ( (end > s) && (end - reloc_size + XEN_IMG_OFFSET >= __pa(_end)) )
+ if ( (end > s) && (end - reloc_size + XEN_IMG_OFFSET >= __pa(end_)) )
{
l4_pgentry_t *pl4e;
l3_pgentry_t *pl3e;
@@ -1067,7 +1067,7 @@ void __init noreturn __start_xen(unsigned long mbi_p)
* data until after we have switched to the relocated pagetables!
*/
barrier();
- move_memory(e + XEN_IMG_OFFSET, XEN_IMG_OFFSET, _end - _start, 1);
+ move_memory(e + XEN_IMG_OFFSET, XEN_IMG_OFFSET, end_ - start_, 1);
/* Walk initial pagetables, relocating page directory entries. */
pl4e = __va(__pa(idle_pg_table));
@@ -1382,7 +1382,7 @@ void __init noreturn __start_xen(unsigned long mbi_p)
}
#endif
- xen_virt_end = ((unsigned long)_end + (1UL << L2_PAGETABLE_SHIFT) - 1) &
+ xen_virt_end = (end_ + (1UL << L2_PAGETABLE_SHIFT) - 1) &
~((1UL << L2_PAGETABLE_SHIFT) - 1);
destroy_xen_mappings(xen_virt_end, XEN_VIRT_START + BOOTSTRAP_MAP_BASE);
diff --git a/xen/arch/x86/x86_64/machine_kexec.c
b/xen/arch/x86/x86_64/machine_kexec.c
index f4a005c..cf435ac 100644
--- a/xen/arch/x86/x86_64/machine_kexec.c
+++ b/xen/arch/x86/x86_64/machine_kexec.c
@@ -13,8 +13,8 @@
int machine_kexec_get_xen(xen_kexec_range_t *range)
{
- range->start = virt_to_maddr(_start);
- range->size = virt_to_maddr(_end) - (unsigned long)range->start;
+ range->start = virt_to_maddr(start_);
+ range->size = virt_to_maddr(end_) - (unsigned long)range->start;
return 0;
}
diff --git a/xen/include/asm-arm/mm.h b/xen/include/asm-arm/mm.h
index eafa26f..e72ffb2 100644
--- a/xen/include/asm-arm/mm.h
+++ b/xen/include/asm-arm/mm.h
@@ -151,8 +151,8 @@ extern vaddr_t xenheap_virt_start;
#endif
#define is_xen_fixed_mfn(mfn) \
- ((pfn_to_paddr(mfn) >= virt_to_maddr(&_start)) && \
- (pfn_to_paddr(mfn) <= virt_to_maddr(&_end)))
+ ((pfn_to_paddr(mfn) >= virt_to_maddr(&start_)) && \
+ (pfn_to_paddr(mfn) <= virt_to_maddr(&end_)))
#define page_get_owner(_p) (_p)->v.inuse.domain
#define page_set_owner(_p,_d) ((_p)->v.inuse.domain = (_d))
diff --git a/xen/include/xen/kernel.h b/xen/include/xen/kernel.h
index 548b64d..b508f65 100644
--- a/xen/include/xen/kernel.h
+++ b/xen/include/xen/kernel.h
@@ -65,10 +65,11 @@
1; \
})
-extern char _start[], _end[], start[];
-#define is_kernel(p) ({ \
- char *__p = (char *)(unsigned long)(p); \
- (__p >= _start) && (__p < _end); \
+extern char start[];
+extern uintptr_t start_, end_;
+#define is_kernel(p) ({ \
+ const uintptr_t p__ = (const uintptr_t)(p); \
+ (p__ >= start_) && (p__ < end_); \
})
extern char _stext[], _etext[];
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel
