[PATCH v4 0/2] misc safety certification fixes

[PATCH v4 0/2] misc safety certification fixes

[Stefano Stabellini]

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

[PATCH v4 1/2] xen: introduce SYMBOL

[Stefano Stabellini]

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

[PATCH v4 2/2] xen: use SYMBOL when required

[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(&region, xenmap - (void *)_start);
+        ret = __apply_alternatives(&region,
+                                   (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

Re: [PATCH v4 2/2] xen: use SYMBOL when required

[Jan Beulich]

>>> 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

Re: [PATCH v4 2/2] xen: use SYMBOL when required

[Julien Grall]

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

Re: [PATCH v4 2/2] xen: use SYMBOL when required

[Jan Beulich]

>>> 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

Re: [PATCH v4 2/2] xen: use SYMBOL when required

[Stefano Stabellini]

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

Re: [PATCH v4 2/2] xen: use SYMBOL when required

[Jan Beulich]

>>> 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

Re: [PATCH v4 0/2] misc safety certification fixes

[Julien Grall]

(+ 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

Re: [PATCH v4 0/2] misc safety certification fixes

[Jan Beulich]

>>> 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

Re: [PATCH v4 0/2] misc safety certification fixes

[Julien Grall]

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

Re: [PATCH v4 0/2] misc safety certification fixes

[Stefano Stabellini]

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

Re: [PATCH v4 2/2] xen: use SYMBOL when required

[Stefano Stabellini]

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

Re: [PATCH v4 2/2] xen: use SYMBOL when required

[Stefano Stabellini]

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

Re: [PATCH v4 2/2] xen: use SYMBOL when required

[Stefano Stabellini]

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

Re: [PATCH v4 2/2] xen: use SYMBOL when required

[Stefano Stabellini]

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

Re: [PATCH v4 2/2] xen: use SYMBOL when required

[Jan Beulich]

>>> 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

Re: [PATCH v4 2/2] xen: use SYMBOL when required

[Stefano Stabellini]

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

Re: [PATCH v4 2/2] xen: use SYMBOL when required

[Jan Beulich]

>>> 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

Re: [PATCH v4 2/2] xen: use SYMBOL when required

[Stefano Stabellini]

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

Re: [PATCH v4 2/2] xen: use SYMBOL when required

[Jan Beulich]

>>> 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

Re: [PATCH v4 2/2] xen: use SYMBOL when required

[Stefano Stabellini]

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

Re: [PATCH v4 2/2] xen: use SYMBOL when required

[Stefano Stabellini]

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

Re: [PATCH v4 2/2] xen: use SYMBOL when required

[Julien Grall]

[-- 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

Re: [PATCH v4 2/2] xen: use SYMBOL when required

[Jan Beulich]

>>> 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

Re: [PATCH v4 2/2] xen: use SYMBOL when required

[Stefano Stabellini]

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