Re: [Xen-devel] [PATCH v2 4/4] xen: use __symbol everywhere
On Wed, 17 Oct 2018, Julien Grall wrote:
> Hi,
>
> On 17/10/2018 15:31, Stefano Stabellini wrote:
> > Use __symbol everywhere _stext, _etext, etc. are used. Technically, it
> > is only required when comparing pointers, but use it everywhere to avoid
>
> Well no. It is also required when substracting pointers (see [1]).
>
> > confusion.
>
> [...]
>
> > diff --git a/xen/arch/arm/alternative.c b/xen/arch/arm/alternative.c
> > index 52ed7ed..305b337 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(__symbol(_start));
> > +paddr_t xen_size = __symbol(_end) - __symbol(_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(, xenmap - (void *)_start);
> > +ret = __apply_alternatives(, xenmap - (void
> > *)__symbol(_start));
>
> For instance, I think this line would be contain undefined behavior. There are
> probably others below.
I'll fix
> [...]
>
> > diff --git a/xen/arch/arm/mm.c b/xen/arch/arm/mm.c
> > index 7a06a33..d9d3948 100644
> > --- a/xen/arch/arm/mm.c
> > +++ b/xen/arch/arm/mm.c
> > @@ -620,7 +620,7 @@ void __init setup_pagetables(unsigned long
> > boot_phys_offset, paddr_t xen_paddr)
> > int i;
> > /* Calculate virt-to-phys offset for the new location */
> > -phys_offset = xen_paddr - (unsigned long) _start;
> > +phys_offset = xen_paddr - (unsigned long) __symbol(_start);
> > #ifdef CONFIG_ARM_64
> > p = (void *) xen_pgtable;
> > @@ -681,7 +681,8 @@ void __init setup_pagetables(unsigned long
> > boot_phys_offset, paddr_t xen_paddr)
> > ttbr = (uintptr_t) cpu0_pgtable + phys_offset;
> > #endif
> > -relocate_xen(ttbr, _start, (void*)dest_va, _end - _start);
> > +relocate_xen(ttbr, (void*)__symbol(_start), (void*)dest_va,
> > +__symbol(_end) - __symbol(_start));
>
> No hard tab.
>
> [...]
>
> > diff --git a/xen/arch/arm/setup.c b/xen/arch/arm/setup.c
> > index ea2495a..9d0b915 100644
> > --- a/xen/arch/arm/setup.c
> > +++ b/xen/arch/arm/setup.c
> > @@ -394,7 +394,8 @@ static paddr_t __init get_xen_paddr(void)
> > paddr_t paddr = 0;
> > int i;
> > -min_size = (_end - _start + (XEN_PADDR_ALIGN-1)) &
> > ~(XEN_PADDR_ALIGN-1);
> > +min_size = (__symbol(_end) - __symbol(_start) +
> > + (XEN_PADDR_ALIGN-1)) & ~(XEN_PADDR_ALIGN-1);
> > /* Find the highest bank with enough space. */
> > for ( i = 0; i < mi->nr_banks; i++ )
> > @@ -727,8 +728,9 @@ 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),
> > NULL);
> > + (paddr_t)(uintptr_t)(__symbol(_start) +
> > boot_phys_offset),
> > + (paddr_t)(uintptr_t)(__symbol(_end) -
> > +
> > __symbol(_start) + 1), NULL);
>
> No hard tab.
>
> > BUG_ON(!xen_bootmodule);
> > xen_paddr = get_xen_paddr();
> > diff --git a/xen/arch/x86/setup.c b/xen/arch/x86/setup.c
> > index 93b79c7..0a7d6c0 100644
> > --- a/xen/arch/x86/setup.c
> > +++ b/xen/arch/x86/setup.c
>
> [...]
>
> > @@ -1390,22 +1393,22 @@ void __init noreturn __start_xen(unsigned long
> > mbi_p)
> > {
> > /* Mark .text as RX (avoiding the first 2M superpage). */
> > modify_xen_mappings(XEN_VIRT_START + MB(2),
> > -(unsigned long)&__2M_text_end,
> > +(unsigned long)__symbol(&__2M_text_end),
> > PAGE_HYPERVISOR_RX);
> > /* Mark .rodata as RO. */
> > -modify_xen_mappings((unsigned long)&__2M_rodata_start,
> > -(unsigned long)&__2M_rodata_end,
> > +modify_xen_mappings((unsigned long)__symbol(&__2M_rodata_start),
> > +(unsigned long)__symbol(&__2M_rodata_end),
>
> AFAICT the return of __symbol should be unsigned long, right? If so, the cast
> could be removed.
I'll fix
> Cheers,
>
> [1]
> https://wiki.sei.cmu.edu/confluence/display/c/ARR36-C.+Do+not+subtract+or+compare+two+pointers+that+do+not+refer+to+the+same+array
>
> --
> Julien Grall
>
___
Xen-devel mailing list
Re: [Xen-devel] [PATCH v2 4/4] xen: use __symbol everywhere
Hi,
On 17/10/2018 15:31, Stefano Stabellini wrote:
Use __symbol everywhere _stext, _etext, etc. are used. Technically, it
is only required when comparing pointers, but use it everywhere to avoid
Well no. It is also required when substracting pointers (see [1]).
confusion.
[...]
diff --git a/xen/arch/arm/alternative.c b/xen/arch/arm/alternative.c
index 52ed7ed..305b337 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(__symbol(_start));
+paddr_t xen_size = __symbol(_end) - __symbol(_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(, xenmap - (void *)_start);
+ret = __apply_alternatives(, xenmap - (void *)__symbol(_start));
For instance, I think this line would be contain undefined behavior.
There are probably others below.
[...]
diff --git a/xen/arch/arm/mm.c b/xen/arch/arm/mm.c
index 7a06a33..d9d3948 100644
--- a/xen/arch/arm/mm.c
+++ b/xen/arch/arm/mm.c
@@ -620,7 +620,7 @@ void __init setup_pagetables(unsigned long
boot_phys_offset, paddr_t xen_paddr)
int i;
/* Calculate virt-to-phys offset for the new location */
-phys_offset = xen_paddr - (unsigned long) _start;
+phys_offset = xen_paddr - (unsigned long) __symbol(_start);
#ifdef CONFIG_ARM_64
p = (void *) xen_pgtable;
@@ -681,7 +681,8 @@ void __init setup_pagetables(unsigned long
boot_phys_offset, paddr_t xen_paddr)
ttbr = (uintptr_t) cpu0_pgtable + phys_offset;
#endif
-relocate_xen(ttbr, _start, (void*)dest_va, _end - _start);
+relocate_xen(ttbr, (void*)__symbol(_start), (void*)dest_va,
+__symbol(_end) - __symbol(_start));
No hard tab.
[...]
diff --git a/xen/arch/arm/setup.c b/xen/arch/arm/setup.c
index ea2495a..9d0b915 100644
--- a/xen/arch/arm/setup.c
+++ b/xen/arch/arm/setup.c
@@ -394,7 +394,8 @@ static paddr_t __init get_xen_paddr(void)
paddr_t paddr = 0;
int i;
-min_size = (_end - _start + (XEN_PADDR_ALIGN-1)) & ~(XEN_PADDR_ALIGN-1);
+min_size = (__symbol(_end) - __symbol(_start) +
+ (XEN_PADDR_ALIGN-1)) & ~(XEN_PADDR_ALIGN-1);
/* Find the highest bank with enough space. */
for ( i = 0; i < mi->nr_banks; i++ )
@@ -727,8 +728,9 @@ 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), NULL);
+ (paddr_t)(uintptr_t)(__symbol(_start) +
boot_phys_offset),
+ (paddr_t)(uintptr_t)(__symbol(_end) -
+
__symbol(_start) + 1), NULL);
No hard tab.
BUG_ON(!xen_bootmodule);
xen_paddr = get_xen_paddr();
diff --git a/xen/arch/x86/setup.c b/xen/arch/x86/setup.c
index 93b79c7..0a7d6c0 100644
--- a/xen/arch/x86/setup.c
+++ b/xen/arch/x86/setup.c
[...]
@@ -1390,22 +1393,22 @@ void __init noreturn __start_xen(unsigned long mbi_p)
{
/* Mark .text as RX (avoiding the first 2M superpage). */
modify_xen_mappings(XEN_VIRT_START + MB(2),
-(unsigned long)&__2M_text_end,
+(unsigned long)__symbol(&__2M_text_end),
PAGE_HYPERVISOR_RX);
/* Mark .rodata as RO. */
-modify_xen_mappings((unsigned long)&__2M_rodata_start,
-(unsigned long)&__2M_rodata_end,
+modify_xen_mappings((unsigned long)__symbol(&__2M_rodata_start),
+(unsigned long)__symbol(&__2M_rodata_end),
AFAICT the return of __symbol should be unsigned long, right? If so, the
cast could be removed.
Cheers,
[1]
https://wiki.sei.cmu.edu/confluence/display/c/ARR36-C.+Do+not+subtract+or+compare+two+pointers+that+do+not+refer+to+the+same+array
--
Julien Grall
___
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel
[Xen-devel] [PATCH v2 4/4] xen: use __symbol everywhere
Use __symbol everywhere _stext, _etext, etc. are used. Technically, it
is only required when comparing pointers, but use it everywhere to avoid
confusion.
Signed-off-by: Stefano Stabellini
CC: jbeul...@suse.com
CC: andrew.coop...@citrix.com
---
Changes in v2:
- cast return of __symbol to char* when required
- define __pa as unsigned long in is_kernel* functions
---
xen/arch/arm/alternative.c | 6 +--
xen/arch/arm/arm32/livepatch.c | 2 +-
xen/arch/arm/arm64/livepatch.c | 2 +-
xen/arch/arm/domain_build.c | 2 +-
xen/arch/arm/livepatch.c| 6 +--
xen/arch/arm/mm.c | 17
xen/arch/arm/setup.c| 8 ++--
xen/arch/x86/setup.c| 79 +++--
xen/arch/x86/tboot.c| 12 +++---
xen/arch/x86/x86_64/machine_kexec.c | 4 +-
xen/include/asm-arm/grant_table.h | 3 +-
xen/include/asm-arm/mm.h| 4 +-
xen/include/asm-x86/mm.h| 4 +-
xen/include/xen/kernel.h| 24 +--
14 files changed, 90 insertions(+), 83 deletions(-)
diff --git a/xen/arch/arm/alternative.c b/xen/arch/arm/alternative.c
index 52ed7ed..305b337 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(__symbol(_start));
+paddr_t xen_size = __symbol(_end) - __symbol(_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(, xenmap - (void *)_start);
+ret = __apply_alternatives(, xenmap - (void *)__symbol(_start));
/* The patching is not expected to fail during boot. */
BUG_ON(ret != 0);
diff --git a/xen/arch/arm/arm32/livepatch.c b/xen/arch/arm/arm32/livepatch.c
index 41378a5..e71764c 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 = 0xe1a0; /* mov r0, r0 */
-new_ptr = func->old_addr - (void *)_start + vmap_of_xen_text;
+new_ptr = func->old_addr - (void *)__symbol(_start) + vmap_of_xen_text;
len = len / sizeof(uint32_t);
/* PATCH! */
diff --git a/xen/arch/arm/arm64/livepatch.c b/xen/arch/arm/arm64/livepatch.c
index 2247b92..4a31026 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 = func->old_addr - (void *)__symbol(_start) + vmap_of_xen_text;
len = len / sizeof(uint32_t);
/* PATCH! */
diff --git a/xen/arch/arm/domain_build.c b/xen/arch/arm/domain_build.c
index f552154..40968d0 100644
--- a/xen/arch/arm/domain_build.c
+++ b/xen/arch/arm/domain_build.c
@@ -2090,7 +2090,7 @@ static void __init find_gnttab_region(struct domain *d,
* Only use the text section as it's always present and will contain
* enough space for a large grant table
*/
-kinfo->gnttab_start = __pa(_stext);
+kinfo->gnttab_start = __pa(__symbol(_stext));
kinfo->gnttab_size = gnttab_dom0_frames() << PAGE_SHIFT;
#ifdef CONFIG_ARM_32
diff --git a/xen/arch/arm/livepatch.c b/xen/arch/arm/livepatch.c
index 279d52c..006dff8 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(__symbol(_start));
+text_order = get_order_from_bytes(__symbol(_end) - __symbol(_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 = func->old_addr - (void *)__symbol(_start) + 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 7a06a33..d9d3948 100644
--- a/xen/arch/arm/mm.c
+++ b/xen/arch/arm/mm.c
@@ -620,7 +620,7 @@ void __init setup_pagetables(unsigned long
boot_phys_offset, paddr_t xen_paddr)
int i;
/* Calculate virt-to-phys offset for the new location */
-phys_offset = xen_paddr - (unsigned long) _start;
