From: Stefano Stabellini <sstabellini@kernel.org>
To: Julien Grall <julien.grall@arm.com>
Cc: xen-devel@lists.xenproject.org,
Stefano Stabellini <stefanos@xilinx.com>,
Stefano Stabellini <sstabellini@kernel.org>,
JBeulich@suse.com, andrew.cooper3@citrix.com
Subject: Re: [PATCH v3 4/4] xen: use SYMBOL everywhere
Date: Fri, 9 Nov 2018 13:44:44 -0800 (PST) [thread overview]
Message-ID: <alpine.DEB.2.10.1811091341130.8259@sstabellini-ThinkPad-X260> (raw)
In-Reply-To: <cc13557b-ae1c-1ef4-432f-4a83a1805538@arm.com>
On Fri, 9 Nov 2018, Julien Grall wrote:
> Hi Stefano,
>
> On 06/11/2018 22:05, Stefano Stabellini wrote:
> > Use SYMBOL everywhere _stext, _etext, etc. are used. Technically, it
> > is required when comparing and subtracting pointers [1], but use it
> > everywhere to avoid confusion.
> >
> > M3CM: Rule-18.2: Subtraction between pointers shall only be applied to
> > pointers that address elements of the same array
> >
> > [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
> >
> > QAVerify: 2761 (and many others)
> > Signed-off-by: Stefano Stabellini <stefanos@xilinx.com>
> > CC: JBeulich@suse.com
> > CC: andrew.cooper3@citrix.com
> > ---
> > Changes in v3:
> > - improve commit message
> > - no hard tabs
> > - rename __symbol to SYMBOL
> > - fix __end_vpci_array and __start_vpci_array
> > - avoid all comparisons between pointers: including (void *) casted
> > returns from SYMBOL()
> > - remove useless casts to (unsigned long)
> >
> > 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 | 7 ++--
> > 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/drivers/vpci/vpci.c | 7 +++-
> > 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 +++++------
> > 15 files changed, 97 insertions(+), 84 deletions(-)
> >
> > diff --git a/xen/arch/arm/alternative.c b/xen/arch/arm/alternative.c
> > index 52ed7ed..2efa9ca 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,8 @@ 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,
> > + (unsigned long)xenmap - 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..6bf9132 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 = func->old_addr - (void *)SYMBOL(_start) + vmap_of_xen_text;
>
> You cast again to (void *) and old_addr is a pointer as well. How is it safe?
It is not. I caught it after sending this version of the series to the
list. Now I am using the following:
new_ptr = (uint32_t *)((unsigned long)func->old_addr - SYMBOL(_start) +
(unsigned long)vmap_of_xen_text);
There are a couple of other instances exactly like this.
> > len = len / sizeof(uint32_t);
> > /* PATCH! */
> > diff --git a/xen/arch/arm/arm64/livepatch.c b/xen/arch/arm/arm64/livepatch.c
> > index 2247b92..fb1477d 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 - SYMBOL(_start) + vmap_of_xen_text;
>
> Here vmap_of_xen_text is a pointer and old_addr is a pointer too. How is it
> safe?
Yes, same here as above.
> > len = len / sizeof(uint32_t);
> > /* PATCH! */
> > diff --git a/xen/arch/arm/domain_build.c b/xen/arch/arm/domain_build.c
> > index f552154..6c03996 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));
>
> Would it make sense to introduce __pa_symbol here?
Following Jan's suggestion, all the __pa(SYMBOL(_stext)) will disappear
in the next version. I have already made the change. That is because
this is not a required change (no points comparisons or subtractions.)
> > 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..609ab35 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 - SYMBOL(_start) + vmap_of_xen_text;
>
> Same question as the previous old_addr.
Yes, I fixed it.
> > 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..16a8afc 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 - 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));
> > /* Clear the copy of the boot pagetables. Each secondary CPU
> > * rebuilds these itself (see head.S) */
> > @@ -1089,7 +1090,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 unsigned long p, unsigned long l,
> > enum mg mg)
> > {
> > lpae_t pte;
> > int i;
> > @@ -1100,8 +1101,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 - SYMBOL(_start)) / PAGE_SIZE;
> > + i < (p + l - SYMBOL(_start)) / PAGE_SIZE;
> > i++ )
> > {
> > pte = xen_xenmap[i];
> > @@ -1138,12 +1139,12 @@ static void set_pte_flags_on_range(const char *p,
> > unsigned long l, enum mg mg)
> > void free_init_memory(void)
> > {
> > paddr_t pa = virt_to_maddr(__init_begin);
> > - unsigned long len = __init_end - __init_begin;
> > + unsigned long len = SYMBOL(__init_end) - SYMBOL(__init_begin);
> > uint32_t insn;
> > 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(SYMBOL(__init_begin), len, mg_rw);
> > #ifdef CONFIG_ARM_32
> > /* udf instruction i.e (see A8.8.247 in ARM DDI 0406C.c) */
> > insn = 0xe7f000f0;
> > @@ -1154,7 +1155,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(SYMBOL(__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 ea2495a..e3adddf 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);
>
> Are the casts necessary when you use SYMBOL?
I can remove the cast to (uintptr_t) that is unnecessary
> > 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..1a02b30 100644
> > --- a/xen/arch/x86/setup.c
> > +++ b/xen/arch/x86/setup.c
> > @@ -578,13 +578,13 @@ static void __init kexec_reserve_area(struct e820map
> > *e820)
> > static inline bool using_2M_mapping(void)
> > {
> > - return !l1_table_offset((unsigned long)__2M_text_end) &&
> > - !l1_table_offset((unsigned long)__2M_rodata_start) &&
> > - !l1_table_offset((unsigned long)__2M_rodata_end) &&
> > - !l1_table_offset((unsigned long)__2M_init_start) &&
> > - !l1_table_offset((unsigned long)__2M_init_end) &&
> > - !l1_table_offset((unsigned long)__2M_rwdata_start) &&
> > - !l1_table_offset((unsigned long)__2M_rwdata_end);
> > + return !l1_table_offset(SYMBOL(__2M_text_end)) &&
> > + !l1_table_offset(SYMBOL(__2M_rodata_start)) &&
> > + !l1_table_offset(SYMBOL(__2M_rodata_end)) &&
> > + !l1_table_offset(SYMBOL(__2M_init_start)) &&
> > + !l1_table_offset(SYMBOL(__2M_init_end)) &&
> > + !l1_table_offset(SYMBOL(__2M_rwdata_start)) &&
> > + !l1_table_offset(SYMBOL(__2M_rwdata_end));
> > }
> > static void noinline init_done(void)
> > @@ -606,13 +606,13 @@ static void noinline init_done(void)
> > /* Destroy Xen's mappings, and reuse the pages. */
> > if ( using_2M_mapping() )
> > {
> > - start = (unsigned long)&__2M_init_start,
> > - end = (unsigned long)&__2M_init_end;
> > + start = SYMBOL(&__2M_init_start),
> > + end = SYMBOL(&__2M_init_end);
> > }
> > else
> > {
> > - start = (unsigned long)&__init_begin;
> > - end = (unsigned long)&__init_end;
> > + start = SYMBOL(&__init_begin);
> > + end = SYMBOL(&__init_end);
> > }
> > destroy_xen_mappings(start, end);
> > @@ -966,8 +966,8 @@ void __init noreturn __start_xen(unsigned long mbi_p)
> > * This needs to remain in sync with xen_in_range() and the
> > * respective reserve_e820_ram() invocation below.
> > */
> > - mod[mbi->mods_count].mod_start = virt_to_mfn(_stext);
> > - mod[mbi->mods_count].mod_end = __2M_rwdata_end - _stext;
> > + mod[mbi->mods_count].mod_start = virt_to_mfn(SYMBOL(_stext));
> > + mod[mbi->mods_count].mod_end = SYMBOL(__2M_rwdata_end) -
> > SYMBOL(_stext);
> > }
> > modules_headroom = bzimage_headroom(bootstrap_map(mod),
> > mod->mod_end);
> > @@ -1018,7 +1018,7 @@ void __init noreturn __start_xen(unsigned long mbi_p)
> > 1UL << (PAGE_SHIFT + 32)) )
> > e = min(HYPERVISOR_VIRT_END - DIRECTMAP_VIRT_START,
> > 1UL << (PAGE_SHIFT + 32));
> > -#define reloc_size ((__pa(__2M_rwdata_end) + mask) & ~mask)
> > +#define reloc_size ((__pa(SYMBOL(__2M_rwdata_end)) + mask) & ~mask)
> > /* Is the region suitable for relocating Xen? */
> > if ( !xen_phys_start && e <= limit )
> > {
> > @@ -1034,7 +1034,8 @@ 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(SYMBOL(_end))) )
> > {
> > l4_pgentry_t *pl4e;
> > l3_pgentry_t *pl3e;
> > @@ -1062,7 +1063,8 @@ 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,
> > + SYMBOL(_end) - SYMBOL(_start), 1);
> > /* Walk initial pagetables, relocating page directory
> > entries. */
> > pl4e = __va(__pa(idle_pg_table));
> > @@ -1103,8 +1105,8 @@ void __init noreturn __start_xen(unsigned long mbi_p)
> > * is contained in this PTE.
> > */
> > BUG_ON(using_2M_mapping() &&
> > - l2_table_offset((unsigned long)_erodata) ==
> > - l2_table_offset((unsigned long)_stext));
> > + l2_table_offset(SYMBOL(_erodata)) ==
> > + l2_table_offset(SYMBOL(_stext)));
> > *pl2e++ = l2e_from_pfn(xen_phys_start >> PAGE_SHIFT,
> > PAGE_HYPERVISOR_RX | _PAGE_PSE);
> > for ( i = 1; i < L2_PAGETABLE_ENTRIES; i++, pl2e++ )
> > @@ -1122,22 +1124,22 @@ void __init noreturn __start_xen(unsigned long
> > mbi_p)
> > continue;
> > }
> > - if ( i < l2_table_offset((unsigned long)&__2M_text_end) )
> > + if ( i < l2_table_offset((unsigned
> > long)SYMBOL(&__2M_text_end)) )
> > {
> > flags = PAGE_HYPERVISOR_RX | _PAGE_PSE;
> > }
> > - else if ( i >= l2_table_offset((unsigned
> > long)&__2M_rodata_start) &&
> > - i < l2_table_offset((unsigned
> > long)&__2M_rodata_end) )
> > + else if ( i >= l2_table_offset(SYMBOL(&__2M_rodata_start))
> > &&
> > + i < l2_table_offset(SYMBOL(&__2M_rodata_end)) )
> > {
> > flags = PAGE_HYPERVISOR_RO | _PAGE_PSE;
> > }
> > - else if ( i >= l2_table_offset((unsigned
> > long)&__2M_init_start) &&
> > - i < l2_table_offset((unsigned
> > long)&__2M_init_end) )
> > + else if ( i >= l2_table_offset(SYMBOL(&__2M_init_start)) &&
> > + i < l2_table_offset(SYMBOL(&__2M_init_end)) )
> > {
> > flags = PAGE_HYPERVISOR_RWX | _PAGE_PSE;
> > }
> > - else if ( (i >= l2_table_offset((unsigned
> > long)&__2M_rwdata_start) &&
> > - i < l2_table_offset((unsigned
> > long)&__2M_rwdata_end)) )
> > + else if ( (i >= l2_table_offset(SYMBOL(&__2M_rwdata_start))
> > &&
> > + i < l2_table_offset(SYMBOL(&__2M_rwdata_end)))
> > )
> > {
> > flags = PAGE_HYPERVISOR_RW | _PAGE_PSE;
> > }
> > @@ -1234,7 +1236,7 @@ void __init noreturn __start_xen(unsigned long mbi_p)
> > panic("Not enough memory to relocate Xen\n");
> > /* This needs to remain in sync with xen_in_range(). */
> > - reserve_e820_ram(&boot_e820, __pa(_stext), __pa(__2M_rwdata_end));
> > + reserve_e820_ram(&boot_e820, __pa(SYMBOL(_stext)),
> > __pa(SYMBOL(__2M_rwdata_end)));
> > /* Late kexec reservation (dynamic start address). */
> > kexec_reserve_area(&boot_e820);
> > @@ -1377,7 +1379,8 @@ void __init noreturn __start_xen(unsigned long mbi_p)
> > }
> > #endif
> > - xen_virt_end = ((unsigned long)_end + (1UL << L2_PAGETABLE_SHIFT) -
> > 1) &
> > + xen_virt_end = (SYMBOL(_end) +
> > + (1UL << L2_PAGETABLE_SHIFT) - 1) &
> > ~((1UL << L2_PAGETABLE_SHIFT) - 1);
> > destroy_xen_mappings(xen_virt_end, XEN_VIRT_START +
> > BOOTSTRAP_MAP_BASE);
> > @@ -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,
> > + 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(SYMBOL(&__2M_rodata_start),
> > + SYMBOL(&__2M_rodata_end),
> > PAGE_HYPERVISOR_RO);
> > /* Mark .data and .bss as RW. */
> > - modify_xen_mappings((unsigned long)&__2M_rwdata_start,
> > - (unsigned long)&__2M_rwdata_end,
> > + modify_xen_mappings(SYMBOL(&__2M_rwdata_start),
> > + SYMBOL(&__2M_rwdata_end),
> > PAGE_HYPERVISOR_RW);
> > /* Drop the remaining mappings in the shattered superpage. */
> > - destroy_xen_mappings((unsigned long)&__2M_rwdata_end,
> > - ROUNDUP((unsigned long)&__2M_rwdata_end,
> > MB(2)));
> > + destroy_xen_mappings(SYMBOL(&__2M_rwdata_end),
> > + ROUNDUP(SYMBOL(&__2M_rwdata_end), MB(2)));
> > }
> > nr_pages = 0;
> > @@ -1860,11 +1863,11 @@ int __hwdom_init xen_in_range(unsigned long mfn)
> > */
> > /* hypervisor .text + .rodata */
> > - xen_regions[region_ro].s = __pa(&_stext);
> > - xen_regions[region_ro].e = __pa(&__2M_rodata_end);
> > + xen_regions[region_ro].s = __pa(SYMBOL(&_stext));
> > + xen_regions[region_ro].e = __pa(SYMBOL(&__2M_rodata_end));
> > /* hypervisor .data + .bss */
> > - xen_regions[region_rw].s = __pa(&__2M_rwdata_start);
> > - xen_regions[region_rw].e = __pa(&__2M_rwdata_end);
> > + xen_regions[region_rw].s = __pa(SYMBOL(&__2M_rwdata_start));
> > + xen_regions[region_rw].e = __pa(SYMBOL(&__2M_rwdata_end));
> > }
> > start = (paddr_t)mfn << PAGE_SHIFT;
> > diff --git a/xen/arch/x86/tboot.c b/xen/arch/x86/tboot.c
> > index f3fdee4..e355232 100644
> > --- a/xen/arch/x86/tboot.c
> > +++ b/xen/arch/x86/tboot.c
> > @@ -373,13 +373,13 @@ void tboot_shutdown(uint32_t shutdown_type)
> > g_tboot_shared->mac_regions[0].size = bootsym_phys(trampoline_end)
> > -
> > bootsym_phys(trampoline_start);
> > /* hypervisor .text + .rodata */
> > - g_tboot_shared->mac_regions[1].start = (uint64_t)__pa(&_stext);
> > - g_tboot_shared->mac_regions[1].size = __pa(&__2M_rodata_end) -
> > - __pa(&_stext);
> > + g_tboot_shared->mac_regions[1].start =
> > (uint64_t)__pa(SYMBOL(&_stext));
> > + g_tboot_shared->mac_regions[1].size =
> > __pa(SYMBOL(&__2M_rodata_end)) -
> > + __pa(SYMBOL(&_stext));
> > /* hypervisor .data + .bss */
> > - g_tboot_shared->mac_regions[2].start =
> > (uint64_t)__pa(&__2M_rwdata_start);
> > - g_tboot_shared->mac_regions[2].size = __pa(&__2M_rwdata_end) -
> > - __pa(&__2M_rwdata_start);
> > + g_tboot_shared->mac_regions[2].start =
> > (uint64_t)__pa(SYMBOL(&__2M_rwdata_start));
> > + g_tboot_shared->mac_regions[2].size =
> > __pa(SYMBOL(&__2M_rwdata_end)) -
> > +
> > __pa(SYMBOL(&__2M_rwdata_start));
> > /*
> > * MAC domains and other Xen memory
> > diff --git a/xen/arch/x86/x86_64/machine_kexec.c
> > b/xen/arch/x86/x86_64/machine_kexec.c
> > index f4a005c..91936db 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(SYMBOL(_start));
> > + range->size = virt_to_maddr(SYMBOL(_end)) - (unsigned
> > long)range->start;
> > return 0;
> > }
> > diff --git a/xen/drivers/vpci/vpci.c b/xen/drivers/vpci/vpci.c
> > index 82607bd..df2ca47 100644
> > --- a/xen/drivers/vpci/vpci.c
> > +++ b/xen/drivers/vpci/vpci.c
> > @@ -33,7 +33,7 @@ struct vpci_register {
> > #ifdef __XEN__
> > extern vpci_register_init_t *const __start_vpci_array[];
> > extern vpci_register_init_t *const __end_vpci_array[];
> > -#define NUM_VPCI_INIT (__end_vpci_array - __start_vpci_array)
> > +#define NUM_VPCI_INIT (SYMBOL(__end_vpci_array) -
> > SYMBOL(__start_vpci_array))
> > void vpci_remove_device(struct pci_dev *pdev)
> > {
> > @@ -71,6 +71,11 @@ int __hwdom_init vpci_add_handlers(struct pci_dev *pdev)
> > for ( i = 0; i < NUM_VPCI_INIT; i++ )
> > {
> > + /*
> > + * We should use SYMBOL here, but it would make the code awkward
> > + * and it is not required due because there are no pointers
> > + * comparison. Leave it as is.
> > + */
> > rc = __start_vpci_array[i](pdev);
> > if ( rc )
> > break;
> > diff --git a/xen/include/asm-arm/grant_table.h
> > b/xen/include/asm-arm/grant_table.h
> > index 37415b7..6c5edcc 100644
> > --- a/xen/include/asm-arm/grant_table.h
> > +++ b/xen/include/asm-arm/grant_table.h
> > @@ -31,7 +31,8 @@ void gnttab_mark_dirty(struct domain *d, mfn_t mfn);
> > * enough space for a large grant table
> > */
> > #define gnttab_dom0_frames() \
> > - min_t(unsigned int, opt_max_grant_frames, PFN_DOWN(_etext - _stext))
> > + min_t(unsigned int, opt_max_grant_frames, \
> > + PFN_DOWN(SYMBOL(_etext) - SYMBOL(_stext)))
> > #define gnttab_init_arch(gt)
> > \
> > ({ \
> > diff --git a/xen/include/asm-arm/mm.h b/xen/include/asm-arm/mm.h
> > index 940b74b..61983f6 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(SYMBOL(&_start))) && \
> > + (pfn_to_paddr(mfn) <= virt_to_maddr(SYMBOL(&_end))))
>
> __pa_symbol would help here.
I have removed these changes.
> > #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/asm-x86/mm.h b/xen/include/asm-x86/mm.h
> > index 6e45651..82018b2 100644
> > --- a/xen/include/asm-x86/mm.h
> > +++ b/xen/include/asm-x86/mm.h
> > @@ -273,8 +273,8 @@ struct page_info
> > #define is_xen_heap_mfn(mfn) \
> > (__mfn_valid(mfn) && is_xen_heap_page(mfn_to_page(_mfn(mfn))))
> > #define is_xen_fixed_mfn(mfn) \
> > - ((((mfn) << PAGE_SHIFT) >= __pa(&_stext)) && \
> > - (((mfn) << PAGE_SHIFT) <= __pa(&__2M_rwdata_end)))
> > + ((((mfn) << PAGE_SHIFT) >= __pa(SYMBOL(&_stext))) && \
> > + (((mfn) << PAGE_SHIFT) <= __pa(SYMBOL(&__2M_rwdata_end))))
>
> Same here.
These changes have also been removed
> > #define PRtype_info "016lx"/* should only be used for printk's */
> > diff --git a/xen/include/xen/kernel.h b/xen/include/xen/kernel.h
> > index 548b64d..cd27030 100644
> > --- a/xen/include/xen/kernel.h
> > +++ b/xen/include/xen/kernel.h
> > @@ -66,27 +66,27 @@
> > })
> > extern char _start[], _end[], start[];
> > -#define is_kernel(p) ({ \
> > - char *__p = (char *)(unsigned long)(p); \
> > - (__p >= _start) && (__p < _end); \
> > +#define is_kernel(p) ({ \
> > + const unsigned long __p = (unsigned long)(p); \
> > + (__p >= SYMBOL(_start)) && (__p < SYMBOL(_end)); \
> > })
> > extern char _stext[], _etext[];
> > -#define is_kernel_text(p) ({ \
> > - char *__p = (char *)(unsigned long)(p); \
> > - (__p >= _stext) && (__p < _etext); \
> > +#define is_kernel_text(p) ({ \
> > + const unsigned long __p = (unsigned long)(p); \
> > + (__p >= SYMBOL(_stext)) && (__p < SYMBOL(_etext)); \
> > })
> > extern const char _srodata[], _erodata[];
> > -#define is_kernel_rodata(p) ({ \
> > - const char *__p = (const char *)(unsigned long)(p); \
> > - (__p >= _srodata) && (__p < _erodata); \
> > +#define is_kernel_rodata(p) ({ \
> > + const unsigned long __p = (unsigned long)(p); \
> > + (__p >= SYMBOL(_srodata)) && (__p < SYMBOL(_erodata)); \
> > })
> > extern char _sinittext[], _einittext[];
> > -#define is_kernel_inittext(p) ({ \
> > - char *__p = (char *)(unsigned long)(p); \
> > - (__p >= _sinittext) && (__p < _einittext); \
> > +#define is_kernel_inittext(p) ({ \
> > + const unsigned long __p = (unsigned long)(p); \
> > + (__p >= SYMBOL(_sinittext)) && (__p < SYMBOL(_einittext)); \
> > })
> > extern enum system_state {
> >
>
> Cheers,
>
> --
> Julien Grall
>
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel
next prev parent reply other threads:[~2018-11-09 21:44 UTC|newest]
Thread overview: 19+ messages / expand[flat|nested] mbox.gz Atom feed top
2018-11-06 22:05 [PATCH v3 0/4] misc safety certification fixes Stefano Stabellini
2018-11-06 22:05 ` [PATCH v3 1/4] xen/arm: initialize target Stefano Stabellini
2018-11-09 11:24 ` Julien Grall
2018-11-06 22:05 ` [PATCH v3 2/4] xen/arm: initialize access Stefano Stabellini
2018-11-09 11:24 ` Julien Grall
2018-11-06 22:05 ` [PATCH v3 3/4] xen: introduce SYMBOL Stefano Stabellini
2018-11-08 14:45 ` Jan Beulich
2018-11-08 22:24 ` Stefano Stabellini
2018-11-09 11:25 ` Julien Grall
2018-11-06 22:05 ` [PATCH v3 4/4] xen: use SYMBOL everywhere Stefano Stabellini
2018-11-08 14:51 ` Jan Beulich
2018-11-08 22:27 ` Stefano Stabellini
2018-11-09 7:07 ` Jan Beulich
2018-11-12 12:25 ` Julien Grall
2018-11-12 18:05 ` Stefano Stabellini
2018-11-09 11:48 ` Julien Grall
2018-11-09 21:44 ` Stefano Stabellini [this message]
2018-11-09 11:54 ` [PATCH v3 0/4] misc safety certification fixes Julien Grall
2018-11-09 16:45 ` Stefano Stabellini
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=alpine.DEB.2.10.1811091341130.8259@sstabellini-ThinkPad-X260 \
--to=sstabellini@kernel.org \
--cc=JBeulich@suse.com \
--cc=andrew.cooper3@citrix.com \
--cc=julien.grall@arm.com \
--cc=stefanos@xilinx.com \
--cc=xen-devel@lists.xenproject.org \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.