* [PATCH v4 0/2] misc safety certification fixes
@ 2018-11-12 23:06 Stefano Stabellini
2018-11-12 23:06 ` [PATCH v4 1/2] xen: introduce SYMBOL Stefano Stabellini
` (2 more replies)
0 siblings, 3 replies; 26+ messages in thread
From: Stefano Stabellini @ 2018-11-12 23:06 UTC (permalink / raw)
To: xen-devel; +Cc: andrew.cooper3, julien.grall, sstabellini, JBeulich
Hi all,
The first patch introduces a new macro that is used throughout the code
in patch #2 to access _stext, _etext pointers and friends.
Cheers,
Stefano
Stefano Stabellini (2):
xen: introduce SYMBOL
xen: use SYMBOL when required
xen/arch/arm/alternative.c | 7 ++++---
xen/arch/arm/arm32/livepatch.c | 3 ++-
xen/arch/arm/arm64/livepatch.c | 3 ++-
xen/arch/arm/device.c | 6 +++---
xen/arch/arm/livepatch.c | 5 +++--
xen/arch/arm/mm.c | 19 ++++++++++---------
xen/arch/arm/percpu.c | 8 ++++----
xen/arch/arm/platform.c | 6 ++++--
xen/arch/arm/setup.c | 8 +++++---
xen/arch/x86/alternative.c | 2 +-
xen/arch/x86/efi/efi-boot.h | 4 ++--
xen/arch/x86/percpu.c | 8 ++++----
xen/arch/x86/setup.c | 11 +++++++----
xen/arch/x86/smpboot.c | 2 +-
xen/common/kernel.c | 8 ++++++--
xen/common/lib.c | 2 +-
xen/common/schedule.c | 2 +-
xen/common/spinlock.c | 4 +++-
xen/common/version.c | 6 +++---
xen/common/virtual_region.c | 2 +-
xen/drivers/vpci/vpci.c | 2 +-
xen/include/asm-arm/grant_table.h | 3 ++-
xen/include/asm-arm/mm.h | 2 +-
xen/include/xen/compiler.h | 6 ++++++
xen/include/xen/kernel.h | 24 ++++++++++++------------
25 files changed, 89 insertions(+), 64 deletions(-)
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel
^ permalink raw reply [flat|nested] 26+ messages in thread* [PATCH v4 1/2] xen: introduce SYMBOL 2018-11-12 23:06 [PATCH v4 0/2] misc safety certification fixes Stefano Stabellini @ 2018-11-12 23:06 ` Stefano Stabellini 2018-11-12 23:06 ` [PATCH v4 2/2] xen: use SYMBOL when required Stefano Stabellini 2018-12-20 17:26 ` [PATCH v4 0/2] misc safety certification fixes Julien Grall 2 siblings, 0 replies; 26+ messages in thread From: Stefano Stabellini @ 2018-11-12 23:06 UTC (permalink / raw) To: xen-devel Cc: Stefano Stabellini, sstabellini, wei.liu2, andrew.cooper3, julien.grall, JBeulich Introduce a macro, SYMBOL, which is a simple wrapper around RELOC_HIDE to be used everywhere symbols such as _stext and _etext are used in the code. RELOC_HIDE is needed when accessing symbols such as _stext and _etext because the C standard forbids for both comparisons and substraction (see C Standard, 6.5.6 [ISO/IEC 9899:2011] and [1]). forbids comparisons between pointers pointing to different objects. _stext, _etext, etc. are all pointers to different objects from ANCI C point of view. To work around potential C compiler issues (which have actually been found, see the comment on top of RELOC_HIDE in Linux), and to help with certifications, let's introduce some syntactic sugar to be used in following patches. [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 Signed-off-by: Stefano Stabellini <stefanos@xilinx.com> Acked-by: Jan Beulich <jbeulich@suse.com> Acked-by: Julien Grall <julien.grall@arm.com> CC: JBeulich@suse.com CC: andrew.cooper3@citrix.com CC: wei.liu2@citrix.com --- Changes in v4: - add acked-bys - remove unneeded parenthesis Changes in v3: - improve commit message - rename __symbol to SYMBOL to avoid name space violations Changes in v2: - do not cast return to char* - move to common header --- xen/include/xen/compiler.h | 6 ++++++ 1 file changed, 6 insertions(+) diff --git a/xen/include/xen/compiler.h b/xen/include/xen/compiler.h index ff6c0f5..344efff 100644 --- a/xen/include/xen/compiler.h +++ b/xen/include/xen/compiler.h @@ -99,6 +99,12 @@ __asm__ ("" : "=r"(__ptr) : "0"(ptr)); \ (typeof(ptr)) (__ptr + (off)); }) +/* + * Use RELOC_HIDE with symbols such as _stext and _etext to avoid + * undefined behavior comparing pointers to different objects + */ +#define SYMBOL(x) RELOC_HIDE((unsigned long)(x), 0) + #ifdef __GCC_ASM_FLAG_OUTPUTS__ # define ASM_FLAG_OUT(yes, no) yes #else -- 1.9.1 _______________________________________________ Xen-devel mailing list Xen-devel@lists.xenproject.org https://lists.xenproject.org/mailman/listinfo/xen-devel ^ permalink raw reply related [flat|nested] 26+ messages in thread
* [PATCH v4 2/2] xen: use SYMBOL when required 2018-11-12 23:06 [PATCH v4 0/2] misc safety certification fixes Stefano Stabellini 2018-11-12 23:06 ` [PATCH v4 1/2] xen: introduce SYMBOL Stefano Stabellini @ 2018-11-12 23:06 ` Stefano Stabellini 2018-11-13 12:56 ` Jan Beulich 2018-12-20 17:26 ` [PATCH v4 0/2] misc safety certification fixes Julien Grall 2 siblings, 1 reply; 26+ messages in thread From: Stefano Stabellini @ 2018-11-12 23:06 UTC (permalink / raw) To: xen-devel Cc: andrew.cooper3, julien.grall, sstabellini, JBeulich, Stefano Stabellini Use SYMBOL in cases of comparisons and subtractions of: _start, _end, __init_begin, __init_end, __2M_text_end, __2M_rodata_start, __2M_rodata_end, __2M_init_start,__2M_init_end, __2M_rwdata_start, __2M_rwdata_end, _stext, _etext, _srodata, _erodata, __end_vpci_array, __start_vpci_array, _sinittext, _einittext, _stextentry, _etextentry, __start_bug_frames, __stop_bug_frames_0, __stop_bug_frames_1, __stop_bug_frames_2,__stop_bug_frames_3, __note_gnu_build_id_start, __note_gnu_build_id_end, __start___ex_table, __stop___ex_table, __start___pre_ex_table, __stop___pre_ex_table, __lock_profile_start, __lock_profile_end, __param_start, __param_end, __setup_start, __setup_end, __initcall_start, __initcall_end, __presmp_initcall_end, __trampoline_rel_start, __trampoline_rel_stop, __trampoline_seg_start, __trampoline_seg_stop __alt_instructions, __alt_instructions_end, __ctors_start, __ctors_end, __end_schedulers_array, __start_schedulers_array, __bss_start, __bss_end, __per_cpu_start, __per_cpu_data_end, _splatform, _eplatform, _sdevice, _edevice, _asdevice, _aedevice, __proc_info_start, __proc_info_end, _sdtb as by the C standard [1]. 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 Signed-off-by: Stefano Stabellini <stefanos@xilinx.com> CC: JBeulich@suse.com CC: andrew.cooper3@citrix.com --- Changes in v4: - only use SYMBOL where necessary, not "everywhere": comparisons and subtractions - improve commit message - remove some unnecessary casts - fix some still unsafe casts - extend checks to all symbols in xen/arch/x86/xen.lds.S and xen/arch/arm/xen.lds.S 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 | 3 ++- xen/arch/arm/arm64/livepatch.c | 3 ++- xen/arch/arm/device.c | 6 +++--- xen/arch/arm/livepatch.c | 5 +++-- xen/arch/arm/mm.c | 19 ++++++++++--------- xen/arch/arm/percpu.c | 8 ++++---- xen/arch/arm/platform.c | 6 ++++-- xen/arch/arm/setup.c | 8 +++++--- xen/arch/x86/alternative.c | 2 +- xen/arch/x86/efi/efi-boot.h | 4 ++-- xen/arch/x86/percpu.c | 8 ++++---- xen/arch/x86/setup.c | 11 +++++++---- xen/arch/x86/smpboot.c | 2 +- xen/common/kernel.c | 8 ++++++-- xen/common/lib.c | 2 +- xen/common/schedule.c | 2 +- xen/common/spinlock.c | 4 +++- xen/common/version.c | 6 +++--- xen/common/virtual_region.c | 2 +- xen/drivers/vpci/vpci.c | 2 +- xen/include/asm-arm/grant_table.h | 3 ++- xen/include/asm-arm/mm.h | 2 +- xen/include/xen/kernel.h | 24 ++++++++++++------------ 24 files changed, 83 insertions(+), 64 deletions(-) diff --git a/xen/arch/arm/alternative.c b/xen/arch/arm/alternative.c index 52ed7ed..b740cfe 100644 --- a/xen/arch/arm/alternative.c +++ b/xen/arch/arm/alternative.c @@ -131,7 +131,7 @@ static int __apply_alternatives(const struct alt_region *region, printk(XENLOG_INFO "alternatives: Patching with alt table %p -> %p\n", region->begin, region->end); - for ( alt = region->begin; alt < region->end; alt++ ) + for ( alt = region->begin; SYMBOL(alt) < SYMBOL(region->end); alt++ ) { int nr_inst; @@ -188,7 +188,7 @@ 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; + 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..83f443f 100644 --- a/xen/arch/arm/arm32/livepatch.c +++ b/xen/arch/arm/arm32/livepatch.c @@ -56,7 +56,8 @@ 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 = (uint32_t *)((unsigned long)func->old_addr - SYMBOL(_start) + + (unsigned long)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..d7a1d03 100644 --- a/xen/arch/arm/arm64/livepatch.c +++ b/xen/arch/arm/arm64/livepatch.c @@ -43,7 +43,8 @@ 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 = (uint32_t *)((unsigned long)func->old_addr - SYMBOL(_start) + + (unsigned long)vmap_of_xen_text); len = len / sizeof(uint32_t); /* PATCH! */ diff --git a/xen/arch/arm/device.c b/xen/arch/arm/device.c index a0072c1..6204858 100644 --- a/xen/arch/arm/device.c +++ b/xen/arch/arm/device.c @@ -34,7 +34,7 @@ int __init device_init(struct dt_device_node *dev, enum device_class class, if ( !dt_device_is_available(dev) || dt_device_for_passthrough(dev) ) return -ENODEV; - for ( desc = _sdevice; desc != _edevice; desc++ ) + for ( desc = _sdevice; SYMBOL(desc) != SYMBOL(_edevice); desc++ ) { if ( desc->class != class ) continue; @@ -55,7 +55,7 @@ int __init acpi_device_init(enum device_class class, const void *data, int class { const struct acpi_device_desc *desc; - for ( desc = _asdevice; desc != _aedevice; desc++ ) + for ( desc = _asdevice; SYMBOL(desc) != SYMBOL(_aedevice); desc++ ) { if ( ( desc->class != class ) || ( desc->class_type != class_type ) ) continue; @@ -74,7 +74,7 @@ enum device_class device_get_class(const struct dt_device_node *dev) ASSERT(dev != NULL); - for ( desc = _sdevice; desc != _edevice; desc++ ) + for ( desc = _sdevice; SYMBOL(desc) != SYMBOL(_edevice); desc++ ) { if ( dt_match_node(desc->dt_match, dev) ) return desc->class; diff --git a/xen/arch/arm/livepatch.c b/xen/arch/arm/livepatch.c index 279d52c..1bab21d 100644 --- a/xen/arch/arm/livepatch.c +++ b/xen/arch/arm/livepatch.c @@ -27,7 +27,7 @@ int arch_livepatch_quiesce(void) return -EINVAL; text_mfn = virt_to_mfn(_start); - text_order = get_order_from_bytes(_end - _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,8 @@ 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 = (uint32_t *)((unsigned long)func->old_addr - SYMBOL(_start) + + (unsigned long)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..0bea016 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,7 @@ 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, _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 +1089,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 +1100,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 +1138,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,9 +1154,10 @@ 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); + printk("Freed %ldkB init memory.\n", + (long)(SYMBOL(__init_end)-SYMBOL(__init_begin))>>10); } void arch_dump_shared_mem_info(void) diff --git a/xen/arch/arm/percpu.c b/xen/arch/arm/percpu.c index 25442c4..dace118 100644 --- a/xen/arch/arm/percpu.c +++ b/xen/arch/arm/percpu.c @@ -6,7 +6,7 @@ unsigned long __per_cpu_offset[NR_CPUS]; #define INVALID_PERCPU_AREA (-(long)__per_cpu_start) -#define PERCPU_ORDER (get_order_from_bytes(__per_cpu_data_end-__per_cpu_start)) +#define PERCPU_ORDER (get_order_from_bytes(SYMBOL(__per_cpu_data_end) - SYMBOL(__per_cpu_start))) void __init percpu_init_areas(void) { @@ -22,8 +22,8 @@ static int init_percpu_area(unsigned int cpu) return -EBUSY; if ( (p = alloc_xenheap_pages(PERCPU_ORDER, 0)) == NULL ) return -ENOMEM; - memset(p, 0, __per_cpu_data_end - __per_cpu_start); - __per_cpu_offset[cpu] = p - __per_cpu_start; + memset(p, 0, SYMBOL(__per_cpu_data_end) - SYMBOL(__per_cpu_start)); + __per_cpu_offset[cpu] = (unsigned long)p - SYMBOL(__per_cpu_start); return 0; } @@ -37,7 +37,7 @@ static void _free_percpu_area(struct rcu_head *head) { struct free_info *info = container_of(head, struct free_info, rcu); unsigned int cpu = info->cpu; - char *p = __per_cpu_start + __per_cpu_offset[cpu]; + char *p = (char *)(SYMBOL(__per_cpu_start) + __per_cpu_offset[cpu]); free_xenheap_pages(p, PERCPU_ORDER); __per_cpu_offset[cpu] = INVALID_PERCPU_AREA; } diff --git a/xen/arch/arm/platform.c b/xen/arch/arm/platform.c index 6989e58..e3e0df6 100644 --- a/xen/arch/arm/platform.c +++ b/xen/arch/arm/platform.c @@ -51,14 +51,16 @@ void __init platform_init(void) ASSERT(platform == NULL); /* Looking for the platform description */ - for ( platform = _splatform; platform != _eplatform; platform++ ) + for ( platform = _splatform; + SYMBOL(platform) != SYMBOL(_eplatform); + platform++ ) { if ( platform_is_compatible(platform) ) break; } /* We don't have specific operations for this platform */ - if ( platform == _eplatform ) + if ( SYMBOL(platform) == SYMBOL(_eplatform) ) { /* TODO: dump DT machine compatible node */ printk(XENLOG_INFO "Platform: Generic System\n"); diff --git a/xen/arch/arm/setup.c b/xen/arch/arm/setup.c index 80f0028..7cb232f 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++ ) @@ -728,8 +729,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)(SYMBOL(_start) + boot_phys_offset), + (paddr_t)(SYMBOL(_end) - + SYMBOL(_start) + 1), NULL); BUG_ON(!xen_bootmodule); xen_paddr = get_xen_paddr(); diff --git a/xen/arch/x86/alternative.c b/xen/arch/x86/alternative.c index b8c819a..5193f51 100644 --- a/xen/arch/x86/alternative.c +++ b/xen/arch/x86/alternative.c @@ -194,7 +194,7 @@ void init_or_livepatch apply_alternatives(struct alt_instr *start, * So be careful if you want to change the scan order to any other * order. */ - for ( a = base = start; a < end; a++ ) + for ( a = base = start; SYMBOL(a) < SYMBOL(end); a++ ) { uint8_t *orig = ALT_ORIG_PTR(a); uint8_t *repl = ALT_REPL_PTR(a); diff --git a/xen/arch/x86/efi/efi-boot.h b/xen/arch/x86/efi/efi-boot.h index 5789d2c..e67e80f 100644 --- a/xen/arch/x86/efi/efi-boot.h +++ b/xen/arch/x86/efi/efi-boot.h @@ -112,11 +112,11 @@ static void __init relocate_trampoline(unsigned long phys) /* Apply relocations to trampoline. */ for ( trampoline_ptr = __trampoline_rel_start; - trampoline_ptr < __trampoline_rel_stop; + SYMBOL(trampoline_ptr) < SYMBOL(__trampoline_rel_stop); ++trampoline_ptr ) *(u32 *)(*trampoline_ptr + (long)trampoline_ptr) += phys; for ( trampoline_ptr = __trampoline_seg_start; - trampoline_ptr < __trampoline_seg_stop; + SYMBOL(trampoline_ptr) < SYMBOL(__trampoline_seg_stop); ++trampoline_ptr ) *(u16 *)(*trampoline_ptr + (long)trampoline_ptr) = phys >> 4; } diff --git a/xen/arch/x86/percpu.c b/xen/arch/x86/percpu.c index 8be4ebd..5e87e91 100644 --- a/xen/arch/x86/percpu.c +++ b/xen/arch/x86/percpu.c @@ -13,7 +13,7 @@ unsigned long __per_cpu_offset[NR_CPUS]; * context of PV guests. */ #define INVALID_PERCPU_AREA (0x8000000000000000L - (long)__per_cpu_start) -#define PERCPU_ORDER get_order_from_bytes(__per_cpu_data_end - __per_cpu_start) +#define PERCPU_ORDER get_order_from_bytes(SYMBOL(__per_cpu_data_end) - SYMBOL(__per_cpu_start)) void __init percpu_init_areas(void) { @@ -33,8 +33,8 @@ static int init_percpu_area(unsigned int cpu) if ( (p = alloc_xenheap_pages(PERCPU_ORDER, 0)) == NULL ) return -ENOMEM; - memset(p, 0, __per_cpu_data_end - __per_cpu_start); - __per_cpu_offset[cpu] = p - __per_cpu_start; + memset(p, 0, SYMBOL(__per_cpu_data_end) - SYMBOL(__per_cpu_start)); + __per_cpu_offset[cpu] = (unsigned long)p - SYMBOL(__per_cpu_start); return 0; } @@ -49,7 +49,7 @@ static void _free_percpu_area(struct rcu_head *head) { struct free_info *info = container_of(head, struct free_info, rcu); unsigned int cpu = info->cpu; - char *p = __per_cpu_start + __per_cpu_offset[cpu]; + char *p = (char *)(SYMBOL(__per_cpu_start) + __per_cpu_offset[cpu]); free_xenheap_pages(p, PERCPU_ORDER); __per_cpu_offset[cpu] = INVALID_PERCPU_AREA; diff --git a/xen/arch/x86/setup.c b/xen/arch/x86/setup.c index 55a288f..c99a608 100644 --- a/xen/arch/x86/setup.c +++ b/xen/arch/x86/setup.c @@ -970,7 +970,7 @@ void __init noreturn __start_xen(unsigned long mbi_p) * 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_end = SYMBOL(__2M_rwdata_end) - SYMBOL(_stext); } modules_headroom = bzimage_headroom(bootstrap_map(mod), mod->mod_end); @@ -1037,7 +1037,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(_end)) ) { l4_pgentry_t *pl4e; l3_pgentry_t *pl3e; @@ -1065,7 +1066,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)); @@ -1380,7 +1382,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); diff --git a/xen/arch/x86/smpboot.c b/xen/arch/x86/smpboot.c index 567cece..cac4ddf 100644 --- a/xen/arch/x86/smpboot.c +++ b/xen/arch/x86/smpboot.c @@ -811,7 +811,7 @@ static int setup_cpu_root_pgt(unsigned int cpu) const char *ptr; for ( rc = 0, ptr = _stextentry; - !rc && ptr < _etextentry; ptr += PAGE_SIZE ) + !rc && SYMBOL(ptr) < SYMBOL(_etextentry); ptr += PAGE_SIZE ) rc = clone_mapping(ptr, rpt); if ( rc ) diff --git a/xen/common/kernel.c b/xen/common/kernel.c index 5766a0f..7599e29 100644 --- a/xen/common/kernel.c +++ b/xen/common/kernel.c @@ -312,14 +312,18 @@ extern const initcall_t __initcall_start[], __presmp_initcall_end[], void __init do_presmp_initcalls(void) { const initcall_t *call; - for ( call = __initcall_start; call < __presmp_initcall_end; call++ ) + for ( call = __initcall_start; + SYMBOL(call) < SYMBOL(__presmp_initcall_end); + call++ ) (*call)(); } void __init do_initcalls(void) { const initcall_t *call; - for ( call = __presmp_initcall_end; call < __initcall_end; call++ ) + for ( call = __presmp_initcall_end; + SYMBOL(call) < SYMBOL(__initcall_end); + call++ ) (*call)(); } diff --git a/xen/common/lib.c b/xen/common/lib.c index 6233020..ac5c90a 100644 --- a/xen/common/lib.c +++ b/xen/common/lib.c @@ -493,7 +493,7 @@ extern const ctor_func_t __ctors_start[], __ctors_end[]; void __init init_constructors(void) { const ctor_func_t *f; - for ( f = __ctors_start; f < __ctors_end; ++f ) + for ( f = __ctors_start; SYMBOL(f) < SYMBOL(__ctors_end); ++f ) (*f)(); /* Putting this here seems as good (or bad) as any other place. */ diff --git a/xen/common/schedule.c b/xen/common/schedule.c index a957c5e..063086e 100644 --- a/xen/common/schedule.c +++ b/xen/common/schedule.c @@ -68,7 +68,7 @@ DEFINE_PER_CPU(struct scheduler *, scheduler); DEFINE_PER_CPU(cpumask_t, cpumask_scratch); extern const struct scheduler *__start_schedulers_array[], *__end_schedulers_array[]; -#define NUM_SCHEDULERS (__end_schedulers_array - __start_schedulers_array) +#define NUM_SCHEDULERS (SYMBOL(__end_schedulers_array) - SYMBOL(__start_schedulers_array)) #define schedulers __start_schedulers_array static struct scheduler __read_mostly ops; diff --git a/xen/common/spinlock.c b/xen/common/spinlock.c index 6bc52d7..d3e9d3a 100644 --- a/xen/common/spinlock.c +++ b/xen/common/spinlock.c @@ -474,7 +474,9 @@ static int __init lock_prof_init(void) { struct lock_profile **q; - for ( q = &__lock_profile_start; q < &__lock_profile_end; q++ ) + for ( q = &__lock_profile_start; + SYMBOL(q) < SYMBOL(&__lock_profile_end); + q++ ) { (*q)->next = lock_profile_glb_q.elem_q; lock_profile_glb_q.elem_q = *q; diff --git a/xen/common/version.c b/xen/common/version.c index 223cb52..dc1d43e 100644 --- a/xen/common/version.c +++ b/xen/common/version.c @@ -147,14 +147,14 @@ static int __init xen_build_init(void) int rc; /* --build-id invoked with wrong parameters. */ - if ( __note_gnu_build_id_end <= &n[0] ) + if ( SYMBOL(__note_gnu_build_id_end) <= SYMBOL(&n[0]) ) return -ENODATA; /* Check for full Note header. */ - if ( &n[1] >= __note_gnu_build_id_end ) + if ( SYMBOL(&n[1]) >= SYMBOL(__note_gnu_build_id_end) ) return -ENODATA; - sz = (void *)__note_gnu_build_id_end - (void *)n; + sz = SYMBOL(__note_gnu_build_id_end) - SYMBOL(n); rc = xen_build_id_check(n, sz, &build_id_p, &build_id_len); diff --git a/xen/common/virtual_region.c b/xen/common/virtual_region.c index aa23918..da99bfd 100644 --- a/xen/common/virtual_region.c +++ b/xen/common/virtual_region.c @@ -119,7 +119,7 @@ void __init setup_virtual_regions(const struct exception_table_entry *start, const struct bug_frame *s; s = bug_frames[i - 1]; - sz = bug_frames[i] - s; + sz = SYMBOL(bug_frames[i]) - SYMBOL(s); core.frame[i - 1].n_bugs = sz; core.frame[i - 1].bugs = s; diff --git a/xen/drivers/vpci/vpci.c b/xen/drivers/vpci/vpci.c index 82607bd..bc0cca6 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) { 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..a21c81a 100644 --- a/xen/include/asm-arm/mm.h +++ b/xen/include/asm-arm/mm.h @@ -151,7 +151,7 @@ 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(&_start)) && \ (pfn_to_paddr(mfn) <= virt_to_maddr(&_end))) #define page_get_owner(_p) (_p)->v.inuse.domain 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 { -- 1.9.1 _______________________________________________ Xen-devel mailing list Xen-devel@lists.xenproject.org https://lists.xenproject.org/mailman/listinfo/xen-devel ^ permalink raw reply related [flat|nested] 26+ messages in thread
* Re: [PATCH v4 2/2] xen: use SYMBOL when required 2018-11-12 23:06 ` [PATCH v4 2/2] xen: use SYMBOL when required Stefano Stabellini @ 2018-11-13 12:56 ` Jan Beulich 2018-11-13 13:17 ` Julien Grall 2019-01-02 18:20 ` Stefano Stabellini 0 siblings, 2 replies; 26+ messages in thread From: Jan Beulich @ 2018-11-13 12:56 UTC (permalink / raw) To: Stefano Stabellini Cc: Andrew Cooper, Julien Grall, Stefano Stabellini, xen-devel >>> On 13.11.18 at 00:06, <sstabellini@kernel.org> wrote: > --- a/xen/arch/x86/alternative.c > +++ b/xen/arch/x86/alternative.c > @@ -194,7 +194,7 @@ void init_or_livepatch apply_alternatives(struct alt_instr *start, > * So be careful if you want to change the scan order to any other > * order. > */ > - for ( a = base = start; a < end; a++ ) > + for ( a = base = start; SYMBOL(a) < SYMBOL(end); a++ ) At this point all is fine: end is allowed to point to the end of start[]. If anything you want to change the invocations (where the questionable symbols are used). I'm also not convinced you need to touch both sides of the comparison or subtraction expressions. In order for people to not start wondering what the purpose of SYMBOL() is at any of its use sites, you really want to use it on the problematic symbols themselves, not somewhere on a derived variable or parameter. I also think review would be helped if you at least split this patch into an Arm, and x86, and a common code patch. > @@ -33,8 +33,8 @@ static int init_percpu_area(unsigned int cpu) > if ( (p = alloc_xenheap_pages(PERCPU_ORDER, 0)) == NULL ) > return -ENOMEM; > > - memset(p, 0, __per_cpu_data_end - __per_cpu_start); > - __per_cpu_offset[cpu] = p - __per_cpu_start; > + memset(p, 0, SYMBOL(__per_cpu_data_end) - SYMBOL(__per_cpu_start)); > + __per_cpu_offset[cpu] = (unsigned long)p - SYMBOL(__per_cpu_start); Can't you make SYMBOL() retain the original type, such that casts like this one aren't needed? As soon as the compiler doesn't know anymore that particular globals (or statics) are used, it can't infer anymore that two pointers can't possibly point into the same array. > @@ -1037,7 +1037,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(_end)) ) Only re-flowing? If no change is meant to be done to this use of _end, please omit the change. > --- a/xen/include/asm-arm/mm.h > +++ b/xen/include/asm-arm/mm.h > @@ -151,7 +151,7 @@ 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(&_start)) && \ > (pfn_to_paddr(mfn) <= virt_to_maddr(&_end))) Unnecessary or incomplete change again? Jan _______________________________________________ Xen-devel mailing list Xen-devel@lists.xenproject.org https://lists.xenproject.org/mailman/listinfo/xen-devel ^ permalink raw reply [flat|nested] 26+ messages in thread
* Re: [PATCH v4 2/2] xen: use SYMBOL when required 2018-11-13 12:56 ` Jan Beulich @ 2018-11-13 13:17 ` Julien Grall 2018-11-13 13:27 ` Jan Beulich 2019-01-02 18:20 ` Stefano Stabellini 1 sibling, 1 reply; 26+ messages in thread From: Julien Grall @ 2018-11-13 13:17 UTC (permalink / raw) To: Jan Beulich, Stefano Stabellini Cc: Andrew Cooper, Stefano Stabellini, nd, xen-devel Hi, On 13/11/2018 12:56, Jan Beulich wrote: >>>> On 13.11.18 at 00:06, <sstabellini@kernel.org> wrote: >> --- a/xen/arch/x86/alternative.c >> +++ b/xen/arch/x86/alternative.c >> @@ -194,7 +194,7 @@ void init_or_livepatch apply_alternatives(struct alt_instr *start, >> * So be careful if you want to change the scan order to any other >> * order. >> */ >> - for ( a = base = start; a < end; a++ ) >> + for ( a = base = start; SYMBOL(a) < SYMBOL(end); a++ ) > > At this point all is fine: end is allowed to point to the end of start[]. But it can also point to somewhere else. The compiler cannot know it, right? >> @@ -33,8 +33,8 @@ static int init_percpu_area(unsigned int cpu) >> if ( (p = alloc_xenheap_pages(PERCPU_ORDER, 0)) == NULL ) >> return -ENOMEM; >> >> - memset(p, 0, __per_cpu_data_end - __per_cpu_start); >> - __per_cpu_offset[cpu] = p - __per_cpu_start; >> + memset(p, 0, SYMBOL(__per_cpu_data_end) - SYMBOL(__per_cpu_start)); >> + __per_cpu_offset[cpu] = (unsigned long)p - SYMBOL(__per_cpu_start); > > Can't you make SYMBOL() retain the original type, such that casts > like this one aren't needed? As soon as the compiler doesn't know > anymore that particular globals (or statics) are used, it can't infer > anymore that two pointers can't possibly point into the same array. If SYMBOL() keeps the original type, then you will still substract 2 pointers. If the compiler can't infer the cannot possibly point into the same array, it also cannot infer they point to the same. So that would be undefined, right? Cheers, -- Julien Grall _______________________________________________ Xen-devel mailing list Xen-devel@lists.xenproject.org https://lists.xenproject.org/mailman/listinfo/xen-devel ^ permalink raw reply [flat|nested] 26+ messages in thread
* Re: [PATCH v4 2/2] xen: use SYMBOL when required 2018-11-13 13:17 ` Julien Grall @ 2018-11-13 13:27 ` Jan Beulich 2018-11-13 22:02 ` Stefano Stabellini 0 siblings, 1 reply; 26+ messages in thread From: Jan Beulich @ 2018-11-13 13:27 UTC (permalink / raw) To: Julien Grall Cc: Andrew Cooper, Stefano Stabellini, nd, Stefano Stabellini, xen-devel >>> On 13.11.18 at 14:17, <Julien.Grall@arm.com> wrote: > On 13/11/2018 12:56, Jan Beulich wrote: >>>>> On 13.11.18 at 00:06, <sstabellini@kernel.org> wrote: >>> --- a/xen/arch/x86/alternative.c >>> +++ b/xen/arch/x86/alternative.c >>> @@ -194,7 +194,7 @@ void init_or_livepatch apply_alternatives(struct alt_instr *start, >>> * So be careful if you want to change the scan order to any other >>> * order. >>> */ >>> - for ( a = base = start; a < end; a++ ) >>> + for ( a = base = start; SYMBOL(a) < SYMBOL(end); a++ ) >> >> At this point all is fine: end is allowed to point to the end of start[]. > > But it can also point to somewhere else. The compiler cannot know it, right? Correct. My point is that at this point the compiler cannot use its knowledge of what is (il)legal to "optimize" generated code. >>> @@ -33,8 +33,8 @@ static int init_percpu_area(unsigned int cpu) >>> if ( (p = alloc_xenheap_pages(PERCPU_ORDER, 0)) == NULL ) >>> return -ENOMEM; >>> >>> - memset(p, 0, __per_cpu_data_end - __per_cpu_start); >>> - __per_cpu_offset[cpu] = p - __per_cpu_start; >>> + memset(p, 0, SYMBOL(__per_cpu_data_end) - SYMBOL(__per_cpu_start)); >>> + __per_cpu_offset[cpu] = (unsigned long)p - SYMBOL(__per_cpu_start); >> >> Can't you make SYMBOL() retain the original type, such that casts >> like this one aren't needed? As soon as the compiler doesn't know >> anymore that particular globals (or statics) are used, it can't infer >> anymore that two pointers can't possibly point into the same array. > > If SYMBOL() keeps the original type, then you will still substract 2 > pointers. If the compiler can't infer the cannot possibly point into the > same array, it also cannot infer they point to the same. So that would > be undefined, right? Undefined behavior results if you _actually_ subtract pointers pointing into different objects. Subtracting of pointers is not generally undefined. The compiler can use undefined-ness only if it can prove that both pointers do point into different objects. Jan _______________________________________________ Xen-devel mailing list Xen-devel@lists.xenproject.org https://lists.xenproject.org/mailman/listinfo/xen-devel ^ permalink raw reply [flat|nested] 26+ messages in thread
* Re: [PATCH v4 2/2] xen: use SYMBOL when required 2018-11-13 13:27 ` Jan Beulich @ 2018-11-13 22:02 ` Stefano Stabellini 2018-11-14 7:44 ` Jan Beulich 0 siblings, 1 reply; 26+ messages in thread From: Stefano Stabellini @ 2018-11-13 22:02 UTC (permalink / raw) To: Jan Beulich Cc: Stefano Stabellini, Stefano Stabellini, Andrew Cooper, Julien Grall, xen-devel, nd On Tue, 13 Nov 2018, Jan Beulich wrote: > >>> On 13.11.18 at 14:17, <Julien.Grall@arm.com> wrote: > > On 13/11/2018 12:56, Jan Beulich wrote: > >>>>> On 13.11.18 at 00:06, <sstabellini@kernel.org> wrote: > >>> --- a/xen/arch/x86/alternative.c > >>> +++ b/xen/arch/x86/alternative.c > >>> @@ -194,7 +194,7 @@ void init_or_livepatch apply_alternatives(struct alt_instr *start, > >>> * So be careful if you want to change the scan order to any other > >>> * order. > >>> */ > >>> - for ( a = base = start; a < end; a++ ) > >>> + for ( a = base = start; SYMBOL(a) < SYMBOL(end); a++ ) > >> > >> At this point all is fine: end is allowed to point to the end of start[]. > > > > But it can also point to somewhere else. The compiler cannot know it, right? > > Correct. My point is that at this point the compiler cannot use its > knowledge of what is (il)legal to "optimize" generated code. > > >>> @@ -33,8 +33,8 @@ static int init_percpu_area(unsigned int cpu) > >>> if ( (p = alloc_xenheap_pages(PERCPU_ORDER, 0)) == NULL ) > >>> return -ENOMEM; > >>> > >>> - memset(p, 0, __per_cpu_data_end - __per_cpu_start); > >>> - __per_cpu_offset[cpu] = p - __per_cpu_start; > >>> + memset(p, 0, SYMBOL(__per_cpu_data_end) - SYMBOL(__per_cpu_start)); > >>> + __per_cpu_offset[cpu] = (unsigned long)p - SYMBOL(__per_cpu_start); > >> > >> Can't you make SYMBOL() retain the original type, such that casts > >> like this one aren't needed? As soon as the compiler doesn't know > >> anymore that particular globals (or statics) are used, it can't infer > >> anymore that two pointers can't possibly point into the same array. > > > > If SYMBOL() keeps the original type, then you will still substract 2 > > pointers. If the compiler can't infer the cannot possibly point into the > > same array, it also cannot infer they point to the same. So that would > > be undefined, right? > > Undefined behavior results if you _actually_ subtract pointers pointing > into different objects. Subtracting of pointers is not generally undefined. > The compiler can use undefined-ness only if it can prove that both > pointers do point into different objects. Let's remember that we are not trying to work-around the compiler, we are trying to make our code C standard compliant :-) The compiler might not be able to infer anymore that two pointers can't possibly point into the same array, but we would still be not-compliant. It doesn't solve our problem, especially in regards to certifications. I is safer to use unsigned long as return type for SYMBOL and avoid pointers comparisons completely. The code impact is very limited and then we don't have to prove same or different "objectness" at all. _______________________________________________ Xen-devel mailing list Xen-devel@lists.xenproject.org https://lists.xenproject.org/mailman/listinfo/xen-devel ^ permalink raw reply [flat|nested] 26+ messages in thread
* Re: [PATCH v4 2/2] xen: use SYMBOL when required 2018-11-13 22:02 ` Stefano Stabellini @ 2018-11-14 7:44 ` Jan Beulich 2019-01-02 18:20 ` Stefano Stabellini 0 siblings, 1 reply; 26+ messages in thread From: Jan Beulich @ 2018-11-14 7:44 UTC (permalink / raw) To: Stefano Stabellini Cc: Andrew Cooper, Julien Grall, nd, Stefano Stabellini, xen-devel >>> On 13.11.18 at 23:02, <sstabellini@kernel.org> wrote: > On Tue, 13 Nov 2018, Jan Beulich wrote: >> >>> On 13.11.18 at 14:17, <Julien.Grall@arm.com> wrote: >> > On 13/11/2018 12:56, Jan Beulich wrote: >> >>>>> On 13.11.18 at 00:06, <sstabellini@kernel.org> wrote: >> >>> @@ -33,8 +33,8 @@ static int init_percpu_area(unsigned int cpu) >> >>> if ( (p = alloc_xenheap_pages(PERCPU_ORDER, 0)) == NULL ) >> >>> return -ENOMEM; >> >>> >> >>> - memset(p, 0, __per_cpu_data_end - __per_cpu_start); >> >>> - __per_cpu_offset[cpu] = p - __per_cpu_start; >> >>> + memset(p, 0, SYMBOL(__per_cpu_data_end) - SYMBOL(__per_cpu_start)); >> >>> + __per_cpu_offset[cpu] = (unsigned long)p - SYMBOL(__per_cpu_start); >> >> >> >> Can't you make SYMBOL() retain the original type, such that casts >> >> like this one aren't needed? As soon as the compiler doesn't know >> >> anymore that particular globals (or statics) are used, it can't infer >> >> anymore that two pointers can't possibly point into the same array. >> > >> > If SYMBOL() keeps the original type, then you will still substract 2 >> > pointers. If the compiler can't infer the cannot possibly point into the >> > same array, it also cannot infer they point to the same. So that would >> > be undefined, right? >> >> Undefined behavior results if you _actually_ subtract pointers pointing >> into different objects. Subtracting of pointers is not generally undefined. >> The compiler can use undefined-ness only if it can prove that both >> pointers do point into different objects. > > Let's remember that we are not trying to work-around the compiler, we > are trying to make our code C standard compliant :-) The compiler might > not be able to infer anymore that two pointers can't possibly point into > the same array, but we would still be not-compliant. It doesn't solve > our problem, especially in regards to certifications. But then this entire patch is pointless: SYMBOL() is exclusively about deluding the compiler. To make the code standard compliant, you'd have to e.g. do away with all combined (start and end) uses (in C files) of symbols bounding sections. I at least cannot think of a standard compliant way of expressing these. Oddly enough I had once tried to deal with this issue (for other reasons), but the patch wasn't liked: https://lists.xenproject.org/archives/html/xen-devel/2016-08/msg02718.html All the remaining end symbols then could obviously go away in favor of using the size expressions, but as you see further C limitations make it necessary to use asm() for the ones which get converted. Talking of asm()-s: C standard compliance, in a strict sense, would require dropping all of them as well. I'm afraid that when writing special purpose code like OS kernels or hypervisors are, if you want to avoid to resort extensively to assembly code, you'll have to accept to bend some of the language rules, just making sure that the compiler won't have means to mis-interpret the constructs used. > I is safer to use unsigned long as return type for SYMBOL and avoid > pointers comparisons completely. The code impact is very limited and > then we don't have to prove same or different "objectness" at all. Well, that's one perspective to take. The other is that hidden or explicit casts are always a risk (and hence when reviewing code I'm quite picky about any ones introduced anew or even just retained without reason). Making constructs needing to cast things at least finally cast back to the original type often at least lowers this risk. Jan _______________________________________________ Xen-devel mailing list Xen-devel@lists.xenproject.org https://lists.xenproject.org/mailman/listinfo/xen-devel ^ permalink raw reply [flat|nested] 26+ messages in thread
* Re: [PATCH v4 2/2] xen: use SYMBOL when required 2018-11-14 7:44 ` Jan Beulich @ 2019-01-02 18:20 ` Stefano Stabellini 2019-01-04 8:48 ` Jan Beulich 0 siblings, 1 reply; 26+ messages in thread From: Stefano Stabellini @ 2019-01-02 18:20 UTC (permalink / raw) To: Jan Beulich Cc: Stefano Stabellini, Stefano Stabellini, Andrew Cooper, Julien Grall, xen-devel, nd On Wed, 14 Nov 2018, Jan Beulich wrote: > >>> On 13.11.18 at 23:02, <sstabellini@kernel.org> wrote: > > On Tue, 13 Nov 2018, Jan Beulich wrote: > >> >>> On 13.11.18 at 14:17, <Julien.Grall@arm.com> wrote: > >> > On 13/11/2018 12:56, Jan Beulich wrote: > >> >>>>> On 13.11.18 at 00:06, <sstabellini@kernel.org> wrote: > >> >>> @@ -33,8 +33,8 @@ static int init_percpu_area(unsigned int cpu) > >> >>> if ( (p = alloc_xenheap_pages(PERCPU_ORDER, 0)) == NULL ) > >> >>> return -ENOMEM; > >> >>> > >> >>> - memset(p, 0, __per_cpu_data_end - __per_cpu_start); > >> >>> - __per_cpu_offset[cpu] = p - __per_cpu_start; > >> >>> + memset(p, 0, SYMBOL(__per_cpu_data_end) - SYMBOL(__per_cpu_start)); > >> >>> + __per_cpu_offset[cpu] = (unsigned long)p - SYMBOL(__per_cpu_start); > >> >> > >> >> Can't you make SYMBOL() retain the original type, such that casts > >> >> like this one aren't needed? As soon as the compiler doesn't know > >> >> anymore that particular globals (or statics) are used, it can't infer > >> >> anymore that two pointers can't possibly point into the same array. > >> > > >> > If SYMBOL() keeps the original type, then you will still substract 2 > >> > pointers. If the compiler can't infer the cannot possibly point into the > >> > same array, it also cannot infer they point to the same. So that would > >> > be undefined, right? > >> > >> Undefined behavior results if you _actually_ subtract pointers pointing > >> into different objects. Subtracting of pointers is not generally undefined. > >> The compiler can use undefined-ness only if it can prove that both > >> pointers do point into different objects. > > > > Let's remember that we are not trying to work-around the compiler, we > > are trying to make our code C standard compliant :-) The compiler might > > not be able to infer anymore that two pointers can't possibly point into > > the same array, but we would still be not-compliant. It doesn't solve > > our problem, especially in regards to certifications. > > But then this entire patch is pointless: SYMBOL() is exclusively about > deluding the compiler. To make the code standard compliant, you'd > have to e.g. do away with all combined (start and end) uses (in C > files) of symbols bounding sections. I at least cannot think of a > standard compliant way of expressing these. Oddly enough I had > once tried to deal with this issue (for other reasons), but the patch > wasn't liked: > https://lists.xenproject.org/archives/html/xen-devel/2016-08/msg02718.html > All the remaining end symbols then could obviously go away in favor > of using the size expressions, but as you see further C limitations > make it necessary to use asm() for the ones which get converted. > > Talking of asm()-s: C standard compliance, in a strict sense, would > require dropping all of them as well. I'm afraid that when writing > special purpose code like OS kernels or hypervisors are, if you > want to avoid to resort extensively to assembly code, you'll have > to accept to bend some of the language rules, just making sure > that the compiler won't have means to mis-interpret the constructs > used. > > > I is safer to use unsigned long as return type for SYMBOL and avoid > > pointers comparisons completely. The code impact is very limited and > > then we don't have to prove same or different "objectness" at all. > > Well, that's one perspective to take. The other is that hidden or > explicit casts are always a risk (and hence when reviewing code > I'm quite picky about any ones introduced anew or even just > retained without reason). Making constructs needing to cast > things at least finally cast back to the original type often at least > lowers this risk. The new casts added actually cancel themselves out with the ones been removed (some casts to unsigned long have been removed). I went through the patch, these are the stats: arch/arm: +4 arch/x86: 0 common: -4 Overall the impact is basically null. Given the plus side of not having to prove same or different "objectness", I think it is the best compromise in this case. My preference is to use unsigned long as return type, as done in this version of the patch. _______________________________________________ Xen-devel mailing list Xen-devel@lists.xenproject.org https://lists.xenproject.org/mailman/listinfo/xen-devel ^ permalink raw reply [flat|nested] 26+ messages in thread
* Re: [PATCH v4 2/2] xen: use SYMBOL when required 2019-01-02 18:20 ` Stefano Stabellini @ 2019-01-04 8:48 ` Jan Beulich 2019-01-04 17:05 ` Stefano Stabellini 0 siblings, 1 reply; 26+ messages in thread From: Jan Beulich @ 2019-01-04 8:48 UTC (permalink / raw) To: Stefano Stabellini Cc: Andrew Cooper, Julien Grall, nd, Stefano Stabellini, xen-devel >>> On 02.01.19 at 19:20, <sstabellini@kernel.org> wrote: > On Wed, 14 Nov 2018, Jan Beulich wrote: >> >>> On 13.11.18 at 23:02, <sstabellini@kernel.org> wrote: >> > On Tue, 13 Nov 2018, Jan Beulich wrote: >> >> >>> On 13.11.18 at 14:17, <Julien.Grall@arm.com> wrote: >> >> > On 13/11/2018 12:56, Jan Beulich wrote: >> >> >>>>> On 13.11.18 at 00:06, <sstabellini@kernel.org> wrote: >> >> >>> @@ -33,8 +33,8 @@ static int init_percpu_area(unsigned int cpu) >> >> >>> if ( (p = alloc_xenheap_pages(PERCPU_ORDER, 0)) == NULL ) >> >> >>> return -ENOMEM; >> >> >>> >> >> >>> - memset(p, 0, __per_cpu_data_end - __per_cpu_start); >> >> >>> - __per_cpu_offset[cpu] = p - __per_cpu_start; >> >> >>> + memset(p, 0, SYMBOL(__per_cpu_data_end) - SYMBOL(__per_cpu_start)); >> >> >>> + __per_cpu_offset[cpu] = (unsigned long)p - SYMBOL(__per_cpu_start); >> >> >> >> >> >> Can't you make SYMBOL() retain the original type, such that casts >> >> >> like this one aren't needed? As soon as the compiler doesn't know >> >> >> anymore that particular globals (or statics) are used, it can't infer >> >> >> anymore that two pointers can't possibly point into the same array. >> >> > >> >> > If SYMBOL() keeps the original type, then you will still substract 2 >> >> > pointers. If the compiler can't infer the cannot possibly point into the >> >> > same array, it also cannot infer they point to the same. So that would >> >> > be undefined, right? >> >> >> >> Undefined behavior results if you _actually_ subtract pointers pointing >> >> into different objects. Subtracting of pointers is not generally undefined. >> >> The compiler can use undefined-ness only if it can prove that both >> >> pointers do point into different objects. >> > >> > Let's remember that we are not trying to work-around the compiler, we >> > are trying to make our code C standard compliant :-) The compiler might >> > not be able to infer anymore that two pointers can't possibly point into >> > the same array, but we would still be not-compliant. It doesn't solve >> > our problem, especially in regards to certifications. >> >> But then this entire patch is pointless: SYMBOL() is exclusively about >> deluding the compiler. To make the code standard compliant, you'd >> have to e.g. do away with all combined (start and end) uses (in C >> files) of symbols bounding sections. I at least cannot think of a >> standard compliant way of expressing these. Oddly enough I had >> once tried to deal with this issue (for other reasons), but the patch >> wasn't liked: >> https://lists.xenproject.org/archives/html/xen-devel/2016-08/msg02718.html >> All the remaining end symbols then could obviously go away in favor >> of using the size expressions, but as you see further C limitations >> make it necessary to use asm() for the ones which get converted. >> >> Talking of asm()-s: C standard compliance, in a strict sense, would >> require dropping all of them as well. I'm afraid that when writing >> special purpose code like OS kernels or hypervisors are, if you >> want to avoid to resort extensively to assembly code, you'll have >> to accept to bend some of the language rules, just making sure >> that the compiler won't have means to mis-interpret the constructs >> used. >> >> > I is safer to use unsigned long as return type for SYMBOL and avoid >> > pointers comparisons completely. The code impact is very limited and >> > then we don't have to prove same or different "objectness" at all. >> >> Well, that's one perspective to take. The other is that hidden or >> explicit casts are always a risk (and hence when reviewing code >> I'm quite picky about any ones introduced anew or even just >> retained without reason). Making constructs needing to cast >> things at least finally cast back to the original type often at least >> lowers this risk. > > The new casts added actually cancel themselves out with the ones been > removed (some casts to unsigned long have been removed). I went through > the patch, these are the stats: > > arch/arm: +4 > arch/x86: 0 > common: -4 > > Overall the impact is basically null. Given the plus side of not having to > prove same or different "objectness", I think it is the best compromise > in this case. My preference is to use unsigned long as return type, as > done in this version of the patch. But if I'm not misremembering there could be an overall win if you casted the result back to the original type, as suggested on the other sub-thread. Jan _______________________________________________ Xen-devel mailing list Xen-devel@lists.xenproject.org https://lists.xenproject.org/mailman/listinfo/xen-devel ^ permalink raw reply [flat|nested] 26+ messages in thread
* Re: [PATCH v4 2/2] xen: use SYMBOL when required 2019-01-04 8:48 ` Jan Beulich @ 2019-01-04 17:05 ` Stefano Stabellini 2019-01-07 7:39 ` Jan Beulich 0 siblings, 1 reply; 26+ messages in thread From: Stefano Stabellini @ 2019-01-04 17:05 UTC (permalink / raw) To: Jan Beulich Cc: Stefano Stabellini, Stefano Stabellini, Andrew Cooper, Julien Grall, xen-devel, nd On Fri, 4 Jan 2019, Jan Beulich wrote: > >>> On 02.01.19 at 19:20, <sstabellini@kernel.org> wrote: > > On Wed, 14 Nov 2018, Jan Beulich wrote: > >> >>> On 13.11.18 at 23:02, <sstabellini@kernel.org> wrote: > >> > On Tue, 13 Nov 2018, Jan Beulich wrote: > >> >> >>> On 13.11.18 at 14:17, <Julien.Grall@arm.com> wrote: > >> >> > On 13/11/2018 12:56, Jan Beulich wrote: > >> >> >>>>> On 13.11.18 at 00:06, <sstabellini@kernel.org> wrote: > >> >> >>> @@ -33,8 +33,8 @@ static int init_percpu_area(unsigned int cpu) > >> >> >>> if ( (p = alloc_xenheap_pages(PERCPU_ORDER, 0)) == NULL ) > >> >> >>> return -ENOMEM; > >> >> >>> > >> >> >>> - memset(p, 0, __per_cpu_data_end - __per_cpu_start); > >> >> >>> - __per_cpu_offset[cpu] = p - __per_cpu_start; > >> >> >>> + memset(p, 0, SYMBOL(__per_cpu_data_end) - SYMBOL(__per_cpu_start)); > >> >> >>> + __per_cpu_offset[cpu] = (unsigned long)p - SYMBOL(__per_cpu_start); > >> >> >> > >> >> >> Can't you make SYMBOL() retain the original type, such that casts > >> >> >> like this one aren't needed? As soon as the compiler doesn't know > >> >> >> anymore that particular globals (or statics) are used, it can't infer > >> >> >> anymore that two pointers can't possibly point into the same array. > >> >> > > >> >> > If SYMBOL() keeps the original type, then you will still substract 2 > >> >> > pointers. If the compiler can't infer the cannot possibly point into the > >> >> > same array, it also cannot infer they point to the same. So that would > >> >> > be undefined, right? > >> >> > >> >> Undefined behavior results if you _actually_ subtract pointers pointing > >> >> into different objects. Subtracting of pointers is not generally undefined. > >> >> The compiler can use undefined-ness only if it can prove that both > >> >> pointers do point into different objects. > >> > > >> > Let's remember that we are not trying to work-around the compiler, we > >> > are trying to make our code C standard compliant :-) The compiler might > >> > not be able to infer anymore that two pointers can't possibly point into > >> > the same array, but we would still be not-compliant. It doesn't solve > >> > our problem, especially in regards to certifications. > >> > >> But then this entire patch is pointless: SYMBOL() is exclusively about > >> deluding the compiler. To make the code standard compliant, you'd > >> have to e.g. do away with all combined (start and end) uses (in C > >> files) of symbols bounding sections. I at least cannot think of a > >> standard compliant way of expressing these. Oddly enough I had > >> once tried to deal with this issue (for other reasons), but the patch > >> wasn't liked: > >> https://lists.xenproject.org/archives/html/xen-devel/2016-08/msg02718.html > >> All the remaining end symbols then could obviously go away in favor > >> of using the size expressions, but as you see further C limitations > >> make it necessary to use asm() for the ones which get converted. > >> > >> Talking of asm()-s: C standard compliance, in a strict sense, would > >> require dropping all of them as well. I'm afraid that when writing > >> special purpose code like OS kernels or hypervisors are, if you > >> want to avoid to resort extensively to assembly code, you'll have > >> to accept to bend some of the language rules, just making sure > >> that the compiler won't have means to mis-interpret the constructs > >> used. > >> > >> > I is safer to use unsigned long as return type for SYMBOL and avoid > >> > pointers comparisons completely. The code impact is very limited and > >> > then we don't have to prove same or different "objectness" at all. > >> > >> Well, that's one perspective to take. The other is that hidden or > >> explicit casts are always a risk (and hence when reviewing code > >> I'm quite picky about any ones introduced anew or even just > >> retained without reason). Making constructs needing to cast > >> things at least finally cast back to the original type often at least > >> lowers this risk. > > > > The new casts added actually cancel themselves out with the ones been > > removed (some casts to unsigned long have been removed). I went through > > the patch, these are the stats: > > > > arch/arm: +4 > > arch/x86: 0 > > common: -4 > > > > Overall the impact is basically null. Given the plus side of not having to > > prove same or different "objectness", I think it is the best compromise > > in this case. My preference is to use unsigned long as return type, as > > done in this version of the patch. > > But if I'm not misremembering there could be an overall win if you > casted the result back to the original type, as suggested on the > other sub-thread. I think you are right. I am not saying that SYMBOL returning unsigned long reduces the number of casts compared to SYMBOL returning original type. I am only saying that compared to before this series, the number of casts required to introduce SYMBOL returning unsigned long is small. Given that it is preferable to make unsigned long comparisons rather than pointers comparisons, I think it is the best way forward. In other words, with unsigned long comparisons Xen is more C compliant, it helps with MISRA-C, it helps with us not having to figure out when pointers point to different objects. All for the price of very few new unsigned long casts (in fact, zero new casts in this version of the series). I realize that you are not convinced by these arguments, but let's find a way forward. My preference would be to have SYMBOL returning unsigned long and do unsigned long comparisons when pointers pointing to different objects are involved. If you are strongly opposed to it, I'll change the code to have SYMBOL returning the original type and do pointers comparisons. It is not clear to me whether this approach is actually C compliant and MISRA-C compliant, but it is still better than what we have today. At the very least it clearly highlights all the problematic sites. _______________________________________________ Xen-devel mailing list Xen-devel@lists.xenproject.org https://lists.xenproject.org/mailman/listinfo/xen-devel ^ permalink raw reply [flat|nested] 26+ messages in thread
* Re: [PATCH v4 2/2] xen: use SYMBOL when required 2019-01-04 17:05 ` Stefano Stabellini @ 2019-01-07 7:39 ` Jan Beulich 2019-01-07 18:29 ` Stefano Stabellini 0 siblings, 1 reply; 26+ messages in thread From: Jan Beulich @ 2019-01-07 7:39 UTC (permalink / raw) To: Stefano Stabellini Cc: Andrew Cooper, Julien Grall, nd, Stefano Stabellini, xen-devel >>> On 04.01.19 at 18:05, <sstabellini@kernel.org> wrote: > I realize that you are not convinced by these arguments, but let's find > a way forward. My preference would be to have SYMBOL returning unsigned > long and do unsigned long comparisons when pointers pointing to > different objects are involved. I continue to fail to see how suitable hiding of the connection to the original symbol from the compiler makes code less standard compliant when comparing pointers: The compiler simply can't know whether the underlying object is (in the extreme case) an array spanning the entire address space. Jan _______________________________________________ Xen-devel mailing list Xen-devel@lists.xenproject.org https://lists.xenproject.org/mailman/listinfo/xen-devel ^ permalink raw reply [flat|nested] 26+ messages in thread
* Re: [PATCH v4 2/2] xen: use SYMBOL when required 2019-01-07 7:39 ` Jan Beulich @ 2019-01-07 18:29 ` Stefano Stabellini 2019-01-08 8:03 ` Jan Beulich 0 siblings, 1 reply; 26+ messages in thread From: Stefano Stabellini @ 2019-01-07 18:29 UTC (permalink / raw) To: Jan Beulich Cc: Stefano Stabellini, Stefano Stabellini, Andrew Cooper, Julien Grall, xen-devel, nd On Mon, 7 Jan 2019, Jan Beulich wrote: > >>> On 04.01.19 at 18:05, <sstabellini@kernel.org> wrote: > > I realize that you are not convinced by these arguments, but let's find > > a way forward. My preference would be to have SYMBOL returning unsigned > > long and do unsigned long comparisons when pointers pointing to > > different objects are involved. > > I continue to fail to see how suitable hiding of the connection to the > original symbol from the compiler makes code less standard compliant > when comparing pointers: The compiler simply can't know whether > the underlying object is (in the extreme case) an array spanning the > entire address space. That is because the requirement I am trying to address is MISRA-C compliance, which in turns requires C language compliance for C language (I think it allows mixing C with assembly code). I don't particularly care whether the compiler can or cannot find the connection to the original symbol. The important thing for me is to avoid comparisons (and subtractions) between pointers pointing to different objects. If we compare unsigned longs, it is easier to prove that the comparison is not between pointers pointing to different objects, even if somehow the numeric values indirectly come from pointers. If we compare pointers, even if they went through some sort of assembly conversions, we are still comparing pointers pointing to different objects. The compiler might not be able to figure it out, but a MISRA-C compliance scanning tool, or a human, might. _______________________________________________ Xen-devel mailing list Xen-devel@lists.xenproject.org https://lists.xenproject.org/mailman/listinfo/xen-devel ^ permalink raw reply [flat|nested] 26+ messages in thread
* Re: [PATCH v4 2/2] xen: use SYMBOL when required 2019-01-07 18:29 ` Stefano Stabellini @ 2019-01-08 8:03 ` Jan Beulich 2019-01-08 17:36 ` Stefano Stabellini 0 siblings, 1 reply; 26+ messages in thread From: Jan Beulich @ 2019-01-08 8:03 UTC (permalink / raw) To: Stefano Stabellini Cc: Andrew Cooper, Julien Grall, nd, Stefano Stabellini, xen-devel >>> On 07.01.19 at 19:29, <sstabellini@kernel.org> wrote: > On Mon, 7 Jan 2019, Jan Beulich wrote: >> >>> On 04.01.19 at 18:05, <sstabellini@kernel.org> wrote: >> > I realize that you are not convinced by these arguments, but let's find >> > a way forward. My preference would be to have SYMBOL returning unsigned >> > long and do unsigned long comparisons when pointers pointing to >> > different objects are involved. >> >> I continue to fail to see how suitable hiding of the connection to the >> original symbol from the compiler makes code less standard compliant >> when comparing pointers: The compiler simply can't know whether >> the underlying object is (in the extreme case) an array spanning the >> entire address space. > > That is because the requirement I am trying to address is MISRA-C > compliance, which in turns requires C language compliance for C language > (I think it allows mixing C with assembly code). I don't particularly > care whether the compiler can or cannot find the connection to the > original symbol. > > The important thing for me is to avoid comparisons (and subtractions) > between pointers pointing to different objects. If we compare unsigned > longs, it is easier to prove that the comparison is not between pointers > pointing to different objects, even if somehow the numeric values > indirectly come from pointers. If we compare pointers, even if they went > through some sort of assembly conversions, we are still comparing > pointers pointing to different objects. The compiler might not be able > to figure it out, but a MISRA-C compliance scanning tool, or a human, > might. This is absurd: We are similarly still comparing pointers to different objects when comparing their values casted to unsigned long. The cast is as much of a hiding technique as any other one. If you want to be C language compliant without any compromises, you'll have to do away with all *_end symbols. You may recall that I did propose a patch doing so (for an entirely different reason), using the tool chain's .sizeof.() operator (and then for consistency also its .startof.() counterpart). Jan _______________________________________________ Xen-devel mailing list Xen-devel@lists.xenproject.org https://lists.xenproject.org/mailman/listinfo/xen-devel ^ permalink raw reply [flat|nested] 26+ messages in thread
* Re: [PATCH v4 2/2] xen: use SYMBOL when required 2019-01-08 8:03 ` Jan Beulich @ 2019-01-08 17:36 ` Stefano Stabellini 2019-01-08 18:08 ` Stefano Stabellini 0 siblings, 1 reply; 26+ messages in thread From: Stefano Stabellini @ 2019-01-08 17:36 UTC (permalink / raw) To: Jan Beulich Cc: Stefano Stabellini, Stefano Stabellini, Andrew Cooper, Julien Grall, xen-devel, nd On Tue, 8 Jan 2019, Jan Beulich wrote: > >>> On 07.01.19 at 19:29, <sstabellini@kernel.org> wrote: > > On Mon, 7 Jan 2019, Jan Beulich wrote: > >> >>> On 04.01.19 at 18:05, <sstabellini@kernel.org> wrote: > >> > I realize that you are not convinced by these arguments, but let's find > >> > a way forward. My preference would be to have SYMBOL returning unsigned > >> > long and do unsigned long comparisons when pointers pointing to > >> > different objects are involved. > >> > >> I continue to fail to see how suitable hiding of the connection to the > >> original symbol from the compiler makes code less standard compliant > >> when comparing pointers: The compiler simply can't know whether > >> the underlying object is (in the extreme case) an array spanning the > >> entire address space. > > > > That is because the requirement I am trying to address is MISRA-C > > compliance, which in turns requires C language compliance for C language > > (I think it allows mixing C with assembly code). I don't particularly > > care whether the compiler can or cannot find the connection to the > > original symbol. > > > > The important thing for me is to avoid comparisons (and subtractions) > > between pointers pointing to different objects. If we compare unsigned > > longs, it is easier to prove that the comparison is not between pointers > > pointing to different objects, even if somehow the numeric values > > indirectly come from pointers. If we compare pointers, even if they went > > through some sort of assembly conversions, we are still comparing > > pointers pointing to different objects. The compiler might not be able > > to figure it out, but a MISRA-C compliance scanning tool, or a human, > > might. > > This is absurd: We are similarly still comparing pointers to different > objects when comparing their values casted to unsigned long. The > cast is as much of a hiding technique as any other one. If you want > to be C language compliant without any compromises, you'll have to > do away with all *_end symbols. Basically, this is a matter of interpretation of the spec: it seems to me that coming back from asm-land with pointers and comparing pointers would be a worse offense than a (almost) harmless unsigned long comparison of values returned from asm-land. But I am not particularly knowledgeable about MISRA-C compliance and their interpretation of the rules. So, this is what I am going to do: I'll send a series update according to your suggestion, with SYMBOL returning the native pointer type. As I wrote earlier, although weaker, it is still an improvement from my point of view. In the future, we can revisit this decision if necessary, and at the very least this series will help with easily spotting all the troublesome sites, clearly marked by the SYMBOL macro. If we do have to revisit it, your suggestion below is also something to keep in mind. > You may recall that I did propose > a patch doing so (for an entirely different reason), using the tool > chain's .sizeof.() operator (and then for consistency also its > .startof.() counterpart). _______________________________________________ Xen-devel mailing list Xen-devel@lists.xenproject.org https://lists.xenproject.org/mailman/listinfo/xen-devel ^ permalink raw reply [flat|nested] 26+ messages in thread
* Re: [PATCH v4 2/2] xen: use SYMBOL when required 2019-01-08 17:36 ` Stefano Stabellini @ 2019-01-08 18:08 ` Stefano Stabellini 2019-01-08 18:43 ` Julien Grall 2019-01-09 9:39 ` Jan Beulich 0 siblings, 2 replies; 26+ messages in thread From: Stefano Stabellini @ 2019-01-08 18:08 UTC (permalink / raw) To: Stefano Stabellini Cc: Stefano Stabellini, Andrew Cooper, Julien Grall, Jan Beulich, xen-devel, nd On Tue, 8 Jan 2019, Stefano Stabellini wrote: > On Tue, 8 Jan 2019, Jan Beulich wrote: > > >>> On 07.01.19 at 19:29, <sstabellini@kernel.org> wrote: > > > On Mon, 7 Jan 2019, Jan Beulich wrote: > > >> >>> On 04.01.19 at 18:05, <sstabellini@kernel.org> wrote: > > >> > I realize that you are not convinced by these arguments, but let's find > > >> > a way forward. My preference would be to have SYMBOL returning unsigned > > >> > long and do unsigned long comparisons when pointers pointing to > > >> > different objects are involved. > > >> > > >> I continue to fail to see how suitable hiding of the connection to the > > >> original symbol from the compiler makes code less standard compliant > > >> when comparing pointers: The compiler simply can't know whether > > >> the underlying object is (in the extreme case) an array spanning the > > >> entire address space. > > > > > > That is because the requirement I am trying to address is MISRA-C > > > compliance, which in turns requires C language compliance for C language > > > (I think it allows mixing C with assembly code). I don't particularly > > > care whether the compiler can or cannot find the connection to the > > > original symbol. > > > > > > The important thing for me is to avoid comparisons (and subtractions) > > > between pointers pointing to different objects. If we compare unsigned > > > longs, it is easier to prove that the comparison is not between pointers > > > pointing to different objects, even if somehow the numeric values > > > indirectly come from pointers. If we compare pointers, even if they went > > > through some sort of assembly conversions, we are still comparing > > > pointers pointing to different objects. The compiler might not be able > > > to figure it out, but a MISRA-C compliance scanning tool, or a human, > > > might. > > > > This is absurd: We are similarly still comparing pointers to different > > objects when comparing their values casted to unsigned long. The > > cast is as much of a hiding technique as any other one. If you want > > to be C language compliant without any compromises, you'll have to > > do away with all *_end symbols. > > Basically, this is a matter of interpretation of the spec: it seems to > me that coming back from asm-land with pointers and comparing pointers > would be a worse offense than a (almost) harmless unsigned long > comparison of values returned from asm-land. > > But I am not particularly knowledgeable about MISRA-C compliance and > their interpretation of the rules. > > So, this is what I am going to do: I'll send a series update according > to your suggestion, with SYMBOL returning the native pointer type. As I > wrote earlier, although weaker, it is still an improvement from my point > of view. There is a problem with this though I didn't foresee :-( The native type of _start is not char* -- it is char[]. So I cannot actually return the native type from SYMBOL because I cannot cast to (char[]). I didn't notice it until I actually tried it. See the implementation of RELOC_HIDE: #define RELOC_HIDE(ptr, off) \ ({ unsigned long __ptr; \ __asm__ ("" : "=r"(__ptr) : "0"(ptr)); \ (typeof(ptr)) (__ptr + (off)); }) It casts to the type at the end, the error is: error: cast specifies array type (typeof(ptr)) (__ptr + (off)); }) We have a few options: 1) use unsigned long as in this version of the series (the Linux kernel also uses this technique) Sorry if I insist, it is still the best I think :-) 2) casts the parameters of SYMBOL to the corresponding pointer type For instance: SYMBOL((char *)_start) SYMBOL((struct alt_instr *)__alt_instructions_end) This works, but it is ugly, I would say it makes the code worse than option 1) 2) always return void* from SYMBOL I don't think it is a good idea to use void* as a workaround here 3) pass the desired return type to SYMBOL For instance: SYMBOL(_start, char *) SYMBOL(__alt_instructions_end, struct alt_instr *) Then SYMBOL would automatically cast the return type to char * and struct alt_instr * according to the second parameter. Do you have any other suggestions? _______________________________________________ Xen-devel mailing list Xen-devel@lists.xenproject.org https://lists.xenproject.org/mailman/listinfo/xen-devel ^ permalink raw reply [flat|nested] 26+ messages in thread
* Re: [PATCH v4 2/2] xen: use SYMBOL when required 2019-01-08 18:08 ` Stefano Stabellini @ 2019-01-08 18:43 ` Julien Grall 2019-01-09 9:39 ` Jan Beulich 1 sibling, 0 replies; 26+ messages in thread From: Julien Grall @ 2019-01-08 18:43 UTC (permalink / raw) To: Stefano Stabellini Cc: Stefano Stabellini, Andrew Cooper, Julien Grall, Jan Beulich, xen-devel, nd [-- Attachment #1.1: Type: text/plain, Size: 4973 bytes --] Hi, Sorry for the formatting. On Tue, 8 Jan 2019, 13:09 Stefano Stabellini, <sstabellini@kernel.org> wrote: > On Tue, 8 Jan 2019, Stefano Stabellini wrote: > > On Tue, 8 Jan 2019, Jan Beulich wrote: > > > >>> On 07.01.19 at 19:29, <sstabellini@kernel.org> wrote: > > > > On Mon, 7 Jan 2019, Jan Beulich wrote: > > > >> >>> On 04.01.19 at 18:05, <sstabellini@kernel.org> wrote: > > > >> > I realize that you are not convinced by these arguments, but > let's find > > > >> > a way forward. My preference would be to have SYMBOL returning > unsigned > > > >> > long and do unsigned long comparisons when pointers pointing to > > > >> > different objects are involved. > > > >> > > > >> I continue to fail to see how suitable hiding of the connection to > the > > > >> original symbol from the compiler makes code less standard compliant > > > >> when comparing pointers: The compiler simply can't know whether > > > >> the underlying object ills (in the extreme case) an array spanning > the > > > >> entire address space. > > > > > > > > That is because the requirement I am trying to address is MISRA-C > > > > compliance, which in turns requires C language compliance for C > language > > > > (I think it allows mixing C with assembly code). I don't particularly > > > > care whether the compiler can or cannot find the connection to the > > > > original symbol. > > > > > > > > The important thing for me is to avoid comparisons (and subtractions) > > > > between pointers pointing to different objects. If we compare > unsigned > > > > longs, it is easier to prove that the comparison is not between > pointers > > > > pointing to different objects, even if somehow the numeric values > > > > indirectly come from pointers. If we compare pointers, even if they > went > > > > through some sort of assembly conversions, we are still comparing > > > > pointers pointing to different objects. The compiler might not be > able > > > > to figure it out, but a MISRA-C compliance scanning tool, or a human, > > > > might. > > > > > > This is absurd: We are similarly still comparing pointers to different > > > objects when comparing their values casted to unsigned long. The > > > cast is as much of a hiding technique as any other one. If you want > > > to be C language compliant without any compromises, you'll have to > > > do away with all *_end symbols. > > > > Basically, this is a matter of interpretation of the spec: it seems to > > me that coming back from asm-land with pointers and comparing pointers > > would be a worse offense than a (almost) harmless unsigned long > > comparison of values returned from asm-land. > > > > But I am not particularly knowledgeable about MISRA-C compliance and > > their interpretation of the rules. > > > > So, this is what I am going to do: I'll send a series update according > > to your suggestion, with SYMBOL returning the native pointer type. As I > > wrote earlier, although weaker, it is still an improvement from my point > > of view. > > There is a problem with this though I didn't foresee :-( > > The native type of _start is not char* -- it is char[]. So I cannot > actually return the native type from SYMBOL because I cannot cast to > (char[]). I didn't notice it until I actually tried it. > > See the implementation of RELOC_HIDE: > > #define RELOC_HIDE(ptr, off) \ > ({ unsigned long __ptr; \ > __asm__ ("" : "=r"(__ptr) : "0"(ptr)); \ > (typeof(ptr)) (__ptr + (off)); }) > > It casts to the type at the end, the error is: > > error: cast specifies array type > (typeof(ptr)) (__ptr + (off)); }) > > We have a few options: > > 1) use unsigned long as in this version of the series (the Linux kernel > also uses this technique) > Sorry if I insist, it is still the best I think :-) > > 2) casts the parameters of SYMBOL to the corresponding pointer type > For instance: > SYMBOL((char *)_start) > SYMBOL((struct alt_instr *)__alt_instructions_end) > This works, but it is ugly, I would say it makes the code worse than > option 1) > > 2) always return void* from SYMBOL > I don't think it is a good idea to use void* as a workaround here > > 3) pass the desired return type to SYMBOL > For instance: > SYMBOL(_start, char *) > SYMBOL(__alt_instructions_end, struct alt_instr *) > Then SYMBOL would automatically cast the return type to char * and > struct alt_instr * according to the second parameter. > > Do you have any other suggestions? > Reading [1], I think casting back to the initial type is pointless and not going to help the static analyzer or compiler. After all, you still compare/substract 2 pointers... So, I think the only solution is 1). Cheers, [1] https://kristerw.blogspot.com/2016/12/pointer-comparison-invalid-optimization.html?m=1 > _______________________________________________ > Xen-devel mailing list > Xen-devel@lists.xenproject.org > https://lists.xenproject.org/mailman/listinfo/xen-devel [-- Attachment #1.2: Type: text/html, Size: 6798 bytes --] [-- Attachment #2: Type: text/plain, Size: 157 bytes --] _______________________________________________ Xen-devel mailing list Xen-devel@lists.xenproject.org https://lists.xenproject.org/mailman/listinfo/xen-devel ^ permalink raw reply [flat|nested] 26+ messages in thread
* Re: [PATCH v4 2/2] xen: use SYMBOL when required 2019-01-08 18:08 ` Stefano Stabellini 2019-01-08 18:43 ` Julien Grall @ 2019-01-09 9:39 ` Jan Beulich 2019-01-09 23:50 ` Stefano Stabellini 1 sibling, 1 reply; 26+ messages in thread From: Jan Beulich @ 2019-01-09 9:39 UTC (permalink / raw) To: Stefano Stabellini Cc: Andrew Cooper, Julien Grall, nd, Stefano Stabellini, xen-devel >>> On 08.01.19 at 19:08, <sstabellini@kernel.org> wrote: > On Tue, 8 Jan 2019, Stefano Stabellini wrote: >> So, this is what I am going to do: I'll send a series update according >> to your suggestion, with SYMBOL returning the native pointer type. As I >> wrote earlier, although weaker, it is still an improvement from my point >> of view. > > There is a problem with this though I didn't foresee :-( > > The native type of _start is not char* -- it is char[]. So I cannot > actually return the native type from SYMBOL because I cannot cast to > (char[]). I didn't notice it until I actually tried it. > > See the implementation of RELOC_HIDE: > > #define RELOC_HIDE(ptr, off) \ > ({ unsigned long __ptr; \ > __asm__ ("" : "=r"(__ptr) : "0"(ptr)); \ > (typeof(ptr)) (__ptr + (off)); }) > > It casts to the type at the end, the error is: > > error: cast specifies array type > (typeof(ptr)) (__ptr + (off)); }) > > We have a few options: > > 1) use unsigned long as in this version of the series (the Linux kernel > also uses this technique) > Sorry if I insist, it is still the best I think :-) > > 2) casts the parameters of SYMBOL to the corresponding pointer type > For instance: > SYMBOL((char *)_start) > SYMBOL((struct alt_instr *)__alt_instructions_end) > This works, but it is ugly, I would say it makes the code worse than > option 1) > > 2) always return void* from SYMBOL > I don't think it is a good idea to use void* as a workaround here > > 3) pass the desired return type to SYMBOL > For instance: > SYMBOL(_start, char *) > SYMBOL(__alt_instructions_end, struct alt_instr *) > Then SYMBOL would automatically cast the return type to char * and > struct alt_instr * according to the second parameter. > > Do you have any other suggestions? 4) #define RELOC_HIDE(ptr, off) \ ({ unsigned long ptr_; \ __asm__ ("" : "=r"(ptr_) : "0" (ptr)); \ (typeof(*(ptr)) *) (ptr_ + (off)); }) or, if not suitable for RELOC_HIDE() itself, clone the macro into one that fits SYMBOL()'s needs. Jan _______________________________________________ Xen-devel mailing list Xen-devel@lists.xenproject.org https://lists.xenproject.org/mailman/listinfo/xen-devel ^ permalink raw reply [flat|nested] 26+ messages in thread
* Re: [PATCH v4 2/2] xen: use SYMBOL when required 2019-01-09 9:39 ` Jan Beulich @ 2019-01-09 23:50 ` Stefano Stabellini 0 siblings, 0 replies; 26+ messages in thread From: Stefano Stabellini @ 2019-01-09 23:50 UTC (permalink / raw) To: Jan Beulich Cc: Stefano Stabellini, Stefano Stabellini, Andrew Cooper, Julien Grall, xen-devel, nd On Wed, 9 Jan 2019, Jan Beulich wrote: > >>> On 08.01.19 at 19:08, <sstabellini@kernel.org> wrote: > > On Tue, 8 Jan 2019, Stefano Stabellini wrote: > >> So, this is what I am going to do: I'll send a series update according > >> to your suggestion, with SYMBOL returning the native pointer type. As I > >> wrote earlier, although weaker, it is still an improvement from my point > >> of view. > > > > There is a problem with this though I didn't foresee :-( > > > > The native type of _start is not char* -- it is char[]. So I cannot > > actually return the native type from SYMBOL because I cannot cast to > > (char[]). I didn't notice it until I actually tried it. > > > > See the implementation of RELOC_HIDE: > > > > #define RELOC_HIDE(ptr, off) \ > > ({ unsigned long __ptr; \ > > __asm__ ("" : "=r"(__ptr) : "0"(ptr)); \ > > (typeof(ptr)) (__ptr + (off)); }) > > > > It casts to the type at the end, the error is: > > > > error: cast specifies array type > > (typeof(ptr)) (__ptr + (off)); }) > > > > We have a few options: > > > > 1) use unsigned long as in this version of the series (the Linux kernel > > also uses this technique) > > Sorry if I insist, it is still the best I think :-) > > > > 2) casts the parameters of SYMBOL to the corresponding pointer type > > For instance: > > SYMBOL((char *)_start) > > SYMBOL((struct alt_instr *)__alt_instructions_end) > > This works, but it is ugly, I would say it makes the code worse than > > option 1) > > > > 2) always return void* from SYMBOL > > I don't think it is a good idea to use void* as a workaround here > > > > 3) pass the desired return type to SYMBOL > > For instance: > > SYMBOL(_start, char *) > > SYMBOL(__alt_instructions_end, struct alt_instr *) > > Then SYMBOL would automatically cast the return type to char * and > > struct alt_instr * according to the second parameter. > > > > Do you have any other suggestions? > > 4) > > #define RELOC_HIDE(ptr, off) \ > ({ unsigned long ptr_; \ > __asm__ ("" : "=r"(ptr_) : "0" (ptr)); \ > (typeof(*(ptr)) *) (ptr_ + (off)); }) > > or, if not suitable for RELOC_HIDE() itself, clone the macro into one > that fits SYMBOL()'s needs. OK. I still don't think that this is a good idea. Nonetheless, I have just sent a patch series that uses this trick in the implementation of SYMBOL to return the native type. _______________________________________________ Xen-devel mailing list Xen-devel@lists.xenproject.org https://lists.xenproject.org/mailman/listinfo/xen-devel ^ permalink raw reply [flat|nested] 26+ messages in thread
* Re: [PATCH v4 2/2] xen: use SYMBOL when required 2018-11-13 12:56 ` Jan Beulich 2018-11-13 13:17 ` Julien Grall @ 2019-01-02 18:20 ` Stefano Stabellini 2019-01-02 21:04 ` Stefano Stabellini 1 sibling, 1 reply; 26+ messages in thread From: Stefano Stabellini @ 2019-01-02 18:20 UTC (permalink / raw) To: Jan Beulich Cc: Andrew Cooper, Julien Grall, Stefano Stabellini, Stefano Stabellini, xen-devel On Tue, 13 Nov 2018, Jan Beulich wrote: > >>> On 13.11.18 at 00:06, <sstabellini@kernel.org> wrote: > > --- a/xen/arch/x86/alternative.c > > +++ b/xen/arch/x86/alternative.c > > @@ -194,7 +194,7 @@ void init_or_livepatch apply_alternatives(struct alt_instr *start, > > * So be careful if you want to change the scan order to any other > > * order. > > */ > > - for ( a = base = start; a < end; a++ ) > > + for ( a = base = start; SYMBOL(a) < SYMBOL(end); a++ ) > > At this point all is fine: end is allowed to point to the end of start[]. > If anything you want to change the invocations (where the > questionable symbols are used). I'm also not convinced you need > to touch both sides of the comparison or subtraction expressions. > > In order for people to not start wondering what the purpose of > SYMBOL() is at any of its use sites, you really want to use it on > the problematic symbols themselves, not somewhere on a derived > variable or parameter. I wasn't sure about what to do about derived variables and decided to err on the safe side. I am happy to remove those changes, because I agree that it would be far clearer if SYMBOL() is only used on the problematic symbols. > I also think review would be helped if you at least split this patch > into an Arm, and x86, and a common code patch. I'll do > > @@ -33,8 +33,8 @@ static int init_percpu_area(unsigned int cpu) > > if ( (p = alloc_xenheap_pages(PERCPU_ORDER, 0)) == NULL ) > > return -ENOMEM; > > > > - memset(p, 0, __per_cpu_data_end - __per_cpu_start); > > - __per_cpu_offset[cpu] = p - __per_cpu_start; > > + memset(p, 0, SYMBOL(__per_cpu_data_end) - SYMBOL(__per_cpu_start)); > > + __per_cpu_offset[cpu] = (unsigned long)p - SYMBOL(__per_cpu_start); > > Can't you make SYMBOL() retain the original type, such that casts > like this one aren't needed? As soon as the compiler doesn't know > anymore that particular globals (or statics) are used, it can't infer > anymore that two pointers can't possibly point into the same array. I'll reply to your point to later email in the thread. > > @@ -1037,7 +1037,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(_end)) ) > > Only re-flowing? If no change is meant to be done to this use of _end, > please omit the change. Sorry about the spurious change, I'll remove it. > > --- a/xen/include/asm-arm/mm.h > > +++ b/xen/include/asm-arm/mm.h > > @@ -151,7 +151,7 @@ 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(&_start)) && \ > > (pfn_to_paddr(mfn) <= virt_to_maddr(&_end))) > > Unnecessary or incomplete change again? Same again _______________________________________________ Xen-devel mailing list Xen-devel@lists.xenproject.org https://lists.xenproject.org/mailman/listinfo/xen-devel ^ permalink raw reply [flat|nested] 26+ messages in thread
* Re: [PATCH v4 2/2] xen: use SYMBOL when required 2019-01-02 18:20 ` Stefano Stabellini @ 2019-01-02 21:04 ` Stefano Stabellini 2019-01-03 19:22 ` Stefano Stabellini 0 siblings, 1 reply; 26+ messages in thread From: Stefano Stabellini @ 2019-01-02 21:04 UTC (permalink / raw) To: Stefano Stabellini Cc: Andrew Cooper, Julien Grall, Stefano Stabellini, Jan Beulich, xen-devel On Wed, 2 Jan 2019, Stefano Stabellini wrote: > On Tue, 13 Nov 2018, Jan Beulich wrote: > > >>> On 13.11.18 at 00:06, <sstabellini@kernel.org> wrote: > > > --- a/xen/arch/x86/alternative.c > > > +++ b/xen/arch/x86/alternative.c > > > @@ -194,7 +194,7 @@ void init_or_livepatch apply_alternatives(struct alt_instr *start, > > > * So be careful if you want to change the scan order to any other > > > * order. > > > */ > > > - for ( a = base = start; a < end; a++ ) > > > + for ( a = base = start; SYMBOL(a) < SYMBOL(end); a++ ) > > > > At this point all is fine: end is allowed to point to the end of start[]. > > If anything you want to change the invocations (where the > > questionable symbols are used). I'm also not convinced you need > > to touch both sides of the comparison or subtraction expressions. > > > > In order for people to not start wondering what the purpose of > > SYMBOL() is at any of its use sites, you really want to use it on > > the problematic symbols themselves, not somewhere on a derived > > variable or parameter. > > I wasn't sure about what to do about derived variables and decided to > err on the safe side. I am happy to remove those changes, because I > agree that it would be far clearer if SYMBOL() is only used on the > problematic symbols. I replied too quickly. I agree that changing only the problematic symbols, and not the derived variables, is easier to understand from the code point of view, and should still be safe and compliant. However, I would also prefer to keep the comparisons as unsigned long comparisons, like it is done in this patch. I think it is safer and more compliant to do unsigned long comparisons to avoid completely the issue of having to understand when pointers point to same or different objects. It is certainly more in the spirit of the spec. So, in the case you mentioned above (and a bunch of similar cases), to do unsigned long comparisons only and apply SYMBOL() only to the problematic symbols, we need one more cast to unsigned long at the derived variable side: for ( a = base = start; (unsigned long)a < SYMBOL(end); a++ ) FYI I realized I was using SYMBOL() conversions more than strictly necessary at the derived variables side, but I though it would be nicer to have: SYMBOL(derived) < SYMBOL(problem_variable) rather than (unsigned long)derived < SYMBOL(problem_variable) everywhere. I am happy either way as both should be compliant. _______________________________________________ Xen-devel mailing list Xen-devel@lists.xenproject.org https://lists.xenproject.org/mailman/listinfo/xen-devel ^ permalink raw reply [flat|nested] 26+ messages in thread
* Re: [PATCH v4 2/2] xen: use SYMBOL when required 2019-01-02 21:04 ` Stefano Stabellini @ 2019-01-03 19:22 ` Stefano Stabellini 0 siblings, 0 replies; 26+ messages in thread From: Stefano Stabellini @ 2019-01-03 19:22 UTC (permalink / raw) To: Stefano Stabellini Cc: Andrew Cooper, Julien Grall, Stefano Stabellini, Jan Beulich, xen-devel On Wed, 2 Jan 2019, Stefano Stabellini wrote: > On Wed, 2 Jan 2019, Stefano Stabellini wrote: > > On Tue, 13 Nov 2018, Jan Beulich wrote: > > > >>> On 13.11.18 at 00:06, <sstabellini@kernel.org> wrote: > > > > --- a/xen/arch/x86/alternative.c > > > > +++ b/xen/arch/x86/alternative.c > > > > @@ -194,7 +194,7 @@ void init_or_livepatch apply_alternatives(struct alt_instr *start, > > > > * So be careful if you want to change the scan order to any other > > > > * order. > > > > */ > > > > - for ( a = base = start; a < end; a++ ) > > > > + for ( a = base = start; SYMBOL(a) < SYMBOL(end); a++ ) > > > > > > At this point all is fine: end is allowed to point to the end of start[]. > > > If anything you want to change the invocations (where the > > > questionable symbols are used). I'm also not convinced you need > > > to touch both sides of the comparison or subtraction expressions. > > > > > > In order for people to not start wondering what the purpose of > > > SYMBOL() is at any of its use sites, you really want to use it on > > > the problematic symbols themselves, not somewhere on a derived > > > variable or parameter. > > > > I wasn't sure about what to do about derived variables and decided to > > err on the safe side. I am happy to remove those changes, because I > > agree that it would be far clearer if SYMBOL() is only used on the > > problematic symbols. > > I replied too quickly. I agree that changing only the problematic > symbols, and not the derived variables, is easier to understand from the > code point of view, and should still be safe and compliant. > > However, I would also prefer to keep the comparisons as unsigned long > comparisons, like it is done in this patch. I think it is safer and more > compliant to do unsigned long comparisons to avoid completely the issue > of having to understand when pointers point to same or different > objects. It is certainly more in the spirit of the spec. > > So, in the case you mentioned above (and a bunch of similar cases), to > do unsigned long comparisons only and apply SYMBOL() only to the > problematic symbols, we need one more cast to unsigned long at the > derived variable side: > > for ( a = base = start; (unsigned long)a < SYMBOL(end); a++ ) > > > FYI I realized I was using SYMBOL() conversions more than strictly > necessary at the derived variables side, but I though it would be nicer > to have: > > SYMBOL(derived) < SYMBOL(problem_variable) > > rather than > > (unsigned long)derived < SYMBOL(problem_variable) > > everywhere. I am happy either way as both should be compliant. With the hope of getting it merge before the code freeze and to save time, given that I also had other comments to address and a rebase to do, I'll send a new series update using the latter of these two options. See: https://marc.info/?l=xen-devel&m=154654323311302 We can further discuss the issue there -- I am happy to change things again as necessary. _______________________________________________ Xen-devel mailing list Xen-devel@lists.xenproject.org https://lists.xenproject.org/mailman/listinfo/xen-devel ^ permalink raw reply [flat|nested] 26+ messages in thread
* Re: [PATCH v4 0/2] misc safety certification fixes 2018-11-12 23:06 [PATCH v4 0/2] misc safety certification fixes Stefano Stabellini 2018-11-12 23:06 ` [PATCH v4 1/2] xen: introduce SYMBOL Stefano Stabellini 2018-11-12 23:06 ` [PATCH v4 2/2] xen: use SYMBOL when required Stefano Stabellini @ 2018-12-20 17:26 ` Julien Grall 2018-12-21 9:27 ` Jan Beulich 2 siblings, 1 reply; 26+ messages in thread From: Julien Grall @ 2018-12-20 17:26 UTC (permalink / raw) To: Stefano Stabellini, xen-devel; +Cc: Juergen Gross, andrew.cooper3, Jan Beulich (+ Juergen) On 11/12/18 11:06 PM, Stefano Stabellini wrote: > Hi all, Hi, Discussing with Stefano today, he is aiming to get this series for Xen 4.12. I will be away until the x86/common code freeze. I agree with him that I will waive my ack if it gets reviewed by any committers. Cheers, > > The first patch introduces a new macro that is used throughout the code > in patch #2 to access _stext, _etext pointers and friends. > > Cheers, > > Stefano > > Stefano Stabellini (2): > xen: introduce SYMBOL > xen: use SYMBOL when required > > xen/arch/arm/alternative.c | 7 ++++--- > xen/arch/arm/arm32/livepatch.c | 3 ++- > xen/arch/arm/arm64/livepatch.c | 3 ++- > xen/arch/arm/device.c | 6 +++--- > xen/arch/arm/livepatch.c | 5 +++-- > xen/arch/arm/mm.c | 19 ++++++++++--------- > xen/arch/arm/percpu.c | 8 ++++---- > xen/arch/arm/platform.c | 6 ++++-- > xen/arch/arm/setup.c | 8 +++++--- > xen/arch/x86/alternative.c | 2 +- > xen/arch/x86/efi/efi-boot.h | 4 ++-- > xen/arch/x86/percpu.c | 8 ++++---- > xen/arch/x86/setup.c | 11 +++++++---- > xen/arch/x86/smpboot.c | 2 +- > xen/common/kernel.c | 8 ++++++-- > xen/common/lib.c | 2 +- > xen/common/schedule.c | 2 +- > xen/common/spinlock.c | 4 +++- > xen/common/version.c | 6 +++--- > xen/common/virtual_region.c | 2 +- > xen/drivers/vpci/vpci.c | 2 +- > xen/include/asm-arm/grant_table.h | 3 ++- > xen/include/asm-arm/mm.h | 2 +- > xen/include/xen/compiler.h | 6 ++++++ > xen/include/xen/kernel.h | 24 ++++++++++++------------ > 25 files changed, 89 insertions(+), 64 deletions(-) > -- Julien Grall _______________________________________________ Xen-devel mailing list Xen-devel@lists.xenproject.org https://lists.xenproject.org/mailman/listinfo/xen-devel ^ permalink raw reply [flat|nested] 26+ messages in thread
* Re: [PATCH v4 0/2] misc safety certification fixes 2018-12-20 17:26 ` [PATCH v4 0/2] misc safety certification fixes Julien Grall @ 2018-12-21 9:27 ` Jan Beulich 2018-12-21 10:34 ` Julien Grall 0 siblings, 1 reply; 26+ messages in thread From: Jan Beulich @ 2018-12-21 9:27 UTC (permalink / raw) To: Julien Grall; +Cc: Juergen Gross, Andrew Cooper, Stefano Stabellini, xen-devel >>> On 20.12.18 at 18:26, <julien.grall@arm.com> wrote: > On 11/12/18 11:06 PM, Stefano Stabellini wrote: > Discussing with Stefano today, he is aiming to get this series for Xen > 4.12. I will be away until the x86/common code freeze. > > I agree with him that I will waive my ack if it gets reviewed by any > committers. Well, discussion on patch 2 was abandoned rather than finished afaict, which means Stefano either lost interest or is meaning to submit v5 with the comments addressed. Jan _______________________________________________ Xen-devel mailing list Xen-devel@lists.xenproject.org https://lists.xenproject.org/mailman/listinfo/xen-devel ^ permalink raw reply [flat|nested] 26+ messages in thread
* Re: [PATCH v4 0/2] misc safety certification fixes 2018-12-21 9:27 ` Jan Beulich @ 2018-12-21 10:34 ` Julien Grall 2018-12-21 17:15 ` Stefano Stabellini 0 siblings, 1 reply; 26+ messages in thread From: Julien Grall @ 2018-12-21 10:34 UTC (permalink / raw) To: Jan Beulich; +Cc: Juergen Gross, Andrew Cooper, Stefano Stabellini, xen-devel Hi, On 21/12/2018 09:27, Jan Beulich wrote: >>>> On 20.12.18 at 18:26, <julien.grall@arm.com> wrote: >> On 11/12/18 11:06 PM, Stefano Stabellini wrote: >> Discussing with Stefano today, he is aiming to get this series for Xen >> 4.12. I will be away until the x86/common code freeze. >> >> I agree with him that I will waive my ack if it gets reviewed by any >> committers. > > Well, discussion on patch 2 was abandoned rather than finished > afaict, which means Stefano either lost interest or is meaning to > submit v5 with the comments addressed. He is planning to send a new version while I am on holidays. My request applies for any new version of this providing it gets reviewed by any committers. Cheers, -- Julien Grall _______________________________________________ Xen-devel mailing list Xen-devel@lists.xenproject.org https://lists.xenproject.org/mailman/listinfo/xen-devel ^ permalink raw reply [flat|nested] 26+ messages in thread
* Re: [PATCH v4 0/2] misc safety certification fixes 2018-12-21 10:34 ` Julien Grall @ 2018-12-21 17:15 ` Stefano Stabellini 0 siblings, 0 replies; 26+ messages in thread From: Stefano Stabellini @ 2018-12-21 17:15 UTC (permalink / raw) To: Julien Grall Cc: Juergen Gross, Andrew Cooper, Stefano Stabellini, Jan Beulich, xen-devel On Fri, 21 Dec 2018, Julien Grall wrote: > Hi, > > On 21/12/2018 09:27, Jan Beulich wrote: > > > > > On 20.12.18 at 18:26, <julien.grall@arm.com> wrote: > > > On 11/12/18 11:06 PM, Stefano Stabellini wrote: > > > Discussing with Stefano today, he is aiming to get this series for Xen > > > 4.12. I will be away until the x86/common code freeze. > > > > > > I agree with him that I will waive my ack if it gets reviewed by any > > > committers. > > > > Well, discussion on patch 2 was abandoned rather than finished > > afaict, which means Stefano either lost interest or is meaning to > > submit v5 with the comments addressed. > > He is planning to send a new version while I am on holidays. My request > applies for any new version of this providing it gets reviewed by any > committers. That's right: I haven't had the bandwidth to continue the discussion, but I intend to resume work on it on the 1st week of Jan. The idea is that even if Julien will be on holidays, if I manage to make enough progress and get your reviewed-by the series could go in anyway before the freeze, if that's OK for you. _______________________________________________ Xen-devel mailing list Xen-devel@lists.xenproject.org https://lists.xenproject.org/mailman/listinfo/xen-devel ^ permalink raw reply [flat|nested] 26+ messages in thread
end of thread, other threads:[~2019-01-09 23:50 UTC | newest] Thread overview: 26+ messages (download: mbox.gz / follow: Atom feed) -- links below jump to the message on this page -- 2018-11-12 23:06 [PATCH v4 0/2] misc safety certification fixes Stefano Stabellini 2018-11-12 23:06 ` [PATCH v4 1/2] xen: introduce SYMBOL Stefano Stabellini 2018-11-12 23:06 ` [PATCH v4 2/2] xen: use SYMBOL when required Stefano Stabellini 2018-11-13 12:56 ` Jan Beulich 2018-11-13 13:17 ` Julien Grall 2018-11-13 13:27 ` Jan Beulich 2018-11-13 22:02 ` Stefano Stabellini 2018-11-14 7:44 ` Jan Beulich 2019-01-02 18:20 ` Stefano Stabellini 2019-01-04 8:48 ` Jan Beulich 2019-01-04 17:05 ` Stefano Stabellini 2019-01-07 7:39 ` Jan Beulich 2019-01-07 18:29 ` Stefano Stabellini 2019-01-08 8:03 ` Jan Beulich 2019-01-08 17:36 ` Stefano Stabellini 2019-01-08 18:08 ` Stefano Stabellini 2019-01-08 18:43 ` Julien Grall 2019-01-09 9:39 ` Jan Beulich 2019-01-09 23:50 ` Stefano Stabellini 2019-01-02 18:20 ` Stefano Stabellini 2019-01-02 21:04 ` Stefano Stabellini 2019-01-03 19:22 ` Stefano Stabellini 2018-12-20 17:26 ` [PATCH v4 0/2] misc safety certification fixes Julien Grall 2018-12-21 9:27 ` Jan Beulich 2018-12-21 10:34 ` Julien Grall 2018-12-21 17:15 ` Stefano Stabellini
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.