Re: [Xen-devel] [PATCH v2 4/4] xen: use __symbol everywhere

[Stefano Stabellini <sstabellini@xxxxxxxxxx>]

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(&region, xenmap - (void *)_start);
> > +        ret = __apply_alternatives(&region, 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
Xen-devel@xxxxxxxxxxxxxxxxxxxx
https://lists.xenproject.org/mailman/listinfo/xen-devel