[PATCH v9 0/5] misc safety certification fixes

[PATCH v9 0/5] misc safety certification fixes

[Stefano Stabellini]

Hi all,

This version of the series introduces two macros to compare and subtract
linker symbols. Note that it has been suggested on the list to add type
checking inside the macros, but that is not done in this version of the
series.

Cheers,

Stefano


The following changes since commit 808cff4c2af66afd61973451aeb7e708732abf90:

  sched/credit2: remove stale comment (2019-01-09 15:46:05 +0100)

are available in the git repository at:

  http://xenbits.xenproject.org/git-http/people/sstabellini/xen-unstable.git certifications-9

for you to fetch changes up to 8d3f511c83d026109e64babc5fb5eec9abfdcd14:

  xen/common: use SYMBOLS_SUBTRACT and SYMBOLS_COMPARE as required (2019-02-11 17:10:30 -0800)

----------------------------------------------------------------
Stefano Stabellini (5):
      xen: introduce ptrdiff_t, fix uintptr_t
      xen: introduce SYMBOLS_SUBTRACT and SYMBOLS_COMPARE
      xen/arm: use SYMBOLS_SUBTRACT and SYMBOLS_COMPARE as required
      xen/x86: use SYMBOLS_SUBTRACT and SYMBOLS_COMPARE as required
      xen/common: use SYMBOLS_SUBTRACT and SYMBOLS_COMPARE as required

 xen/arch/arm/alternative.c        | 10 +++++++---
 xen/arch/arm/arm32/livepatch.c    | 14 +++++++++++++-
 xen/arch/arm/arm64/livepatch.c    | 14 +++++++++++++-
 xen/arch/arm/device.c             |  8 +++++---
 xen/arch/arm/livepatch.c          | 16 ++++++++++++++--
 xen/arch/arm/mm.c                 |  9 +++++----
 xen/arch/arm/percpu.c             |  7 ++++---
 xen/arch/arm/platform.c           |  6 ++++--
 xen/arch/arm/setup.c              |  5 +++--
 xen/arch/x86/alternative.c        |  4 +++-
 xen/arch/x86/efi/efi-boot.h       |  4 ++--
 xen/arch/x86/percpu.c             |  7 ++++---
 xen/arch/x86/setup.c              | 10 +++++++---
 xen/arch/x86/smpboot.c            |  3 ++-
 xen/common/kernel.c               |  8 ++++++--
 xen/common/lib.c                  |  5 ++++-
 xen/common/schedule.c             |  6 ++++--
 xen/common/spinlock.c             |  4 +++-
 xen/common/version.c              |  6 +++---
 xen/common/virtual_region.c       |  3 ++-
 xen/drivers/vpci/vpci.c           |  2 +-
 xen/include/asm-arm/grant_table.h |  3 ++-
 xen/include/xen/compiler.h        | 22 ++++++++++++++++++++++
 xen/include/xen/kernel.h          | 28 ++++++++++++++++------------
 xen/include/xen/types.h           |  3 ++-
 25 files changed, 151 insertions(+), 56 deletions(-)

_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel

[PATCH v9 1/5] xen: introduce ptrdiff_t, fix uintptr_t

[Stefano Stabellini]

uintptr_t should be "unsigned long" -- same size as a pointer.

Introduce the new type "ptrdiff_t" which is defined as the signed
integer type of the result of subtracting two pointers.

Signed-off-by: Stefano Stabellini <stefanos@xilinx.com>
CC: jbeulich@suse.com
CC: andrew.cooper3@citrix.com
CC: julien.grall@arm.com
CC: George.Dunlap@eu.citrix.com
CC: ian.jackson@eu.citrix.com
CC: konrad.wilk@oracle.com
CC: tim@xen.org
CC: wei.liu2@citrix.com
---
Changes in v9:
- new patch
---
 xen/include/xen/types.h | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/xen/include/xen/types.h b/xen/include/xen/types.h
index 03f0fe6..e70adac 100644
--- a/xen/include/xen/types.h
+++ b/xen/include/xen/types.h
@@ -52,7 +52,8 @@ typedef __u32 __be32;
 typedef __u64 __le64;
 typedef __u64 __be64;
 
-typedef unsigned int __attribute__((__mode__(__pointer__))) uintptr_t;
+typedef unsigned long __attribute__((__mode__(__pointer__))) uintptr_t;
+typedef long ptrdiff_t;
 
 typedef bool bool_t;
 #define test_and_set_bool(b)   xchg(&(b), true)
-- 
1.9.1


_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel

[PATCH v9 2/5] xen: introduce SYMBOLS_SUBTRACT and SYMBOLS_COMPARE

[Stefano Stabellini]

Introduce two macros meant used everywhere symbols such as _stext and
_etext are compared or subtracted in the code.

They are needed because the C standard forbids for both comparisons and
substraction (see C Standard, 6.5.6 [ISO/IEC 9899:2011] and [1]) between
pointers pointing to different objects. _stext, _etext, etc. are all
pointers to different objects from ANCI C point of view.

[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>
CC: JBeulich@suse.com
CC: andrew.cooper3@citrix.com
CC: wei.liu2@citrix.com
---
Note that the MACROs don't do any type checking at the moment.
Specifically, they don't check that either parameter is a linker symbol.

---
Changes in v9:
- introduce SYMBOLS_SUBTRACT and SYMBOLS_COMPARE
---
 xen/include/xen/compiler.h | 22 ++++++++++++++++++++++
 1 file changed, 22 insertions(+)

diff --git a/xen/include/xen/compiler.h b/xen/include/xen/compiler.h
index ff6c0f5..43620eb 100644
--- a/xen/include/xen/compiler.h
+++ b/xen/include/xen/compiler.h
@@ -99,6 +99,28 @@
     __asm__ ("" : "=r"(__ptr) : "0"(ptr));      \
     (typeof(ptr)) (__ptr + (off)); })
 
+/*
+ * Calculate (end - start), where start and/or end are linker symbols,
+ * returning a ptrdiff_t. The size is in units of start's referent.
+ */
+#define SYMBOLS_SUBTRACT(end, start) ({                                       \
+    (ptrdiff_t)((uintptr_t)(end) - (uintptr_t)(start)) / sizeof(*start);      \
+})
+
+/*
+ * Given two pointers A, B of arbitrary types, returns the difference
+ * B - A in bytes.  Can be used for comparisons:
+ *   If A < B, returns a negative number
+ *   if A == B, returns zero
+ *   If A > B, returns a positive number
+ *
+ * Legal even if the pointers are to different objects. It can be used
+ * when A and/or B are linker symbols.
+ */
+#define SYMBOLS_COMPARE(A, B) ({                                        \
+    (ptrdiff_t)((uintptr_t)(A) - (uintptr_t)(B));                       \
+})
+
 #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 v9 3/5] xen/arm: use SYMBOLS_SUBTRACT and SYMBOLS_COMPARE as required

[Stefano Stabellini]

Use SYMBOLS_SUBTRACT and SYMBOLS_COMPARE in cases of comparisons and
subtractions of:

_start, _end, __init_begin, __init_end, _stext, _etext,
__alt_instructions, __alt_instructions_end, __per_cpu_start,
__per_cpu_data_end, _splatform, _eplatform, _sdevice, _edevice,
_asdevice, _aedevice.

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 v9:
- use SYMBOLS_SUBTRACT and SYMBOLS_COMPARE
---
 xen/arch/arm/alternative.c        | 10 +++++++---
 xen/arch/arm/arm32/livepatch.c    | 14 +++++++++++++-
 xen/arch/arm/arm64/livepatch.c    | 14 +++++++++++++-
 xen/arch/arm/device.c             |  8 +++++---
 xen/arch/arm/livepatch.c          | 16 ++++++++++++++--
 xen/arch/arm/mm.c                 |  9 +++++----
 xen/arch/arm/percpu.c             |  7 ++++---
 xen/arch/arm/platform.c           |  6 ++++--
 xen/arch/arm/setup.c              |  5 +++--
 xen/include/asm-arm/grant_table.h |  3 ++-
 10 files changed, 70 insertions(+), 22 deletions(-)

diff --git a/xen/arch/arm/alternative.c b/xen/arch/arm/alternative.c
index 52ed7ed..2cb66d0 100644
--- a/xen/arch/arm/alternative.c
+++ b/xen/arch/arm/alternative.c
@@ -131,7 +131,10 @@ 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++ )
+    /* region->begin and region->end might point to different objects. */
+    for ( alt = region->begin;
+          SYMBOLS_COMPARE(alt, region->end) < 0;
+          alt++ )
     {
         int nr_inst;
 
@@ -188,7 +191,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 = SYMBOLS_SUBTRACT(_end, _start);
         unsigned int xen_order = get_order_from_bytes(xen_size);
         void *xenmap;
 
@@ -206,7 +209,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,
+                                   SYMBOLS_SUBTRACT(xenmap, _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..03ae84b 100644
--- a/xen/arch/arm/arm32/livepatch.c
+++ b/xen/arch/arm/arm32/livepatch.c
@@ -56,7 +56,19 @@ 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;
+    /*
+     * We need to calculate the offset of the address from _start, and
+     * apply that to our own map, to find where we have this mapped.
+     * Doing these kind of games directly with pointers is contrary to
+     * the C rules for what pointers may be compared and computed.  So
+     * we do the offset calculation with integers, which is always
+     * legal.  The subsequent addition of the offset to the
+     * vmap_of_xen_text pointer is legal because the computed pointer is
+     * indeed a valid part of the object referred to by vmap_of_xen_text
+     * - namely, the byte array of our mapping of the Xen text.
+     */
+    new_ptr = (((uintptr_t))func->old_addr, - ((uintptr_t))_start) +
+              vmap_of_xen_text;
     len = len / sizeof(uint32_t);
 
     /* PATCH! */
diff --git a/xen/arch/arm/arm64/livepatch.c b/xen/arch/arm/arm64/livepatch.c
index 2247b92..ef362c3 100644
--- a/xen/arch/arm/arm64/livepatch.c
+++ b/xen/arch/arm/arm64/livepatch.c
@@ -43,7 +43,19 @@ 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;
+    /*
+     * We need to calculate the offset of the address from _start, and
+     * apply that to our own map, to find where we have this mapped.
+     * Doing these kind of games directly with pointers is contrary to
+     * the C rules for what pointers may be compared and computed.  So
+     * we do the offset calculation with integers, which is always
+     * legal.  The subsequent addition of the offset to the
+     * vmap_of_xen_text pointer is legal because the computed pointer is
+     * indeed a valid part of the object referred to by vmap_of_xen_text
+     * - namely, the byte array of our mapping of the Xen text.
+     */
+    new_ptr = (((uintptr_t))func->old_addr, - ((uintptr_t))_start) +
+              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 70cd6c1..0f0bb77 100644
--- a/xen/arch/arm/device.c
+++ b/xen/arch/arm/device.c
@@ -35,7 +35,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; SYMBOLS_COMPARE(desc, _edevice) != 0; desc++ )
     {
         if ( desc->class != class )
             continue;
@@ -56,7 +56,9 @@ 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;
+          SYMBOLS_COMPARE(desc, _aedevice) != 0;
+          desc++ )
     {
         if ( ( desc->class != class ) || ( desc->class_type != class_type ) )
             continue;
@@ -75,7 +77,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; SYMBOLS_COMPARE(desc, _edevice) != 0; 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..fb733a4 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(SYMBOLS_SUBTRACT(_end, _start));
 
     /*
      * The text section is read-only. So re-map Xen to be able to patch
@@ -78,7 +78,19 @@ 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;
+    /*
+     * We need to calculate the offset of the address from _start, and
+     * apply that to our own map, to find where we have this mapped.
+     * Doing these kind of games directly with pointers is contrary to
+     * the C rules for what pointers may be compared and computed.  So
+     * we do the offset calculation with integers, which is always
+     * legal.  The subsequent addition of the offset to the
+     * vmap_of_xen_text pointer is legal because the computed pointer is
+     * indeed a valid part of the object referred to by vmap_of_xen_text
+     * - namely, the byte array of our mapping of the Xen text.
+     */
+    new_ptr = (((uintptr_t))func->old_addr, - ((uintptr_t))_start) +
+              vmap_of_xen_text;
 
     len = livepatch_insn_len(func);
     memcpy(new_ptr, func->opaque, len);
diff --git a/xen/arch/arm/mm.c b/xen/arch/arm/mm.c
index 01ae2cc..bde57fd 100644
--- a/xen/arch/arm/mm.c
+++ b/xen/arch/arm/mm.c
@@ -1084,8 +1084,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 = (SYMBOLS_SUBTRACT(p, _start)) / PAGE_SIZE;
+          i < ((uintptr_t)p + l - (uintptr_t)_start) / PAGE_SIZE;
           i++ )
     {
         pte = xen_xenmap[i];
@@ -1122,7 +1122,7 @@ 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 = SYMBOLS_SUBTRACT(__init_end, __init_begin);
     uint32_t insn;
     unsigned int i, nr = len / sizeof(insn);
     uint32_t *p;
@@ -1140,7 +1140,8 @@ void free_init_memory(void)
 
     set_pte_flags_on_range(__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",
+           SYMBOLS_SUBTRACT(__init_end, __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..5733a1c 100644
--- a/xen/arch/arm/percpu.c
+++ b/xen/arch/arm/percpu.c
@@ -6,7 +6,8 @@
 
 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(SYMBOLS_SUBTRACT(__per_cpu_data_end, \
+                                           __per_cpu_start)))
 
 void __init percpu_init_areas(void)
 {
@@ -22,8 +23,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, SYMBOLS_SUBTRACT(__per_cpu_data_end, __per_cpu_start));
+    __per_cpu_offset[cpu] = SYMBOLS_SUBTRACT(p, __per_cpu_start);
     return 0;
 }
 
diff --git a/xen/arch/arm/platform.c b/xen/arch/arm/platform.c
index 8eb0b6e..664b573 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;
+          SYMBOLS_COMPARE(platform, _eplatform) != 0;
+          platform++ )
     {
         if ( platform_is_compatible(platform) )
             break;
     }
 
     /* We don't have specific operations for this platform */
-    if ( platform == _eplatform )
+    if ( SYMBOLS_COMPARE(platform, _eplatform) == 0 )
     {
         /* 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 444857a..95fc7aa 100644
--- a/xen/arch/arm/setup.c
+++ b/xen/arch/arm/setup.c
@@ -772,8 +772,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), false);
+                             (paddr_t)(_start + boot_phys_offset),
+                             (paddr_t)(SYMBOLS_SUBTRACT(_end, _start) + 1),
+                             false);
     BUG_ON(!xen_bootmodule);
 
     setup_pagetables(boot_phys_offset);
diff --git a/xen/include/asm-arm/grant_table.h b/xen/include/asm-arm/grant_table.h
index 816e3c6..fffe934 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(SYMBOLS_SUBTRACT(_etext, _stext)))
 
 #define gnttab_init_arch(gt)                                             \
 ({                                                                       \
-- 
1.9.1


_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel

[PATCH v9 4/5] xen/x86: use SYMBOLS_SUBTRACT and SYMBOLS_COMPARE as required

[Stefano Stabellini]

Use SYMBOLS_SUBTRACT and SYMBOLS_COMPARE in cases of comparisons and
subtractions of:

_start, _end, __2M_rwdata_start, __2M_rwdata_end, _stext, _etext,
__end_vpci_array, __start_vpci_array, _stextentry, _etextentry,
__trampoline_rel_start, __trampoline_rel_stop, __trampoline_seg_start,
__trampoline_seg_stop __per_cpu_start, __per_cpu_data_end

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 v9:
- use SYMBOLS_SUBTRACT and SYMBOLS_COMPARE
---
 xen/arch/x86/alternative.c  |  4 +++-
 xen/arch/x86/efi/efi-boot.h |  4 ++--
 xen/arch/x86/percpu.c       |  7 ++++---
 xen/arch/x86/setup.c        | 10 +++++++---
 xen/arch/x86/smpboot.c      |  3 ++-
 xen/drivers/vpci/vpci.c     |  2 +-
 6 files changed, 19 insertions(+), 11 deletions(-)

diff --git a/xen/arch/x86/alternative.c b/xen/arch/x86/alternative.c
index b8c819a..56c3710 100644
--- a/xen/arch/x86/alternative.c
+++ b/xen/arch/x86/alternative.c
@@ -193,8 +193,10 @@ 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.
+     *
+     * start and end could be pointers to different objects.
      */
-    for ( a = base = start; a < end; a++ )
+    for ( a = base = start; SYMBOLS_COMPARE(a, end) < 0; 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..12709e1 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;
+          SYMBOLS_COMPARE(trampoline_ptr, __trampoline_rel_stop) < 0;
           ++trampoline_ptr )
         *(u32 *)(*trampoline_ptr + (long)trampoline_ptr) += phys;
     for ( trampoline_ptr = __trampoline_seg_start;
-          trampoline_ptr < __trampoline_seg_stop;
+          SYMBOLS_COMPARE(trampoline_ptr, __trampoline_seg_stop) < 0;
           ++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..17a9c90 100644
--- a/xen/arch/x86/percpu.c
+++ b/xen/arch/x86/percpu.c
@@ -13,7 +13,8 @@ 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(SYMBOLS_SUBTRACT(__per_cpu_data_end, \
+                                                           __per_cpu_start))
 
 void __init percpu_init_areas(void)
 {
@@ -33,8 +34,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, SYMBOLS_SUBTRACT(__per_cpu_data_end, __per_cpu_start));
+    __per_cpu_offset[cpu] = SYMBOLS_SUBTRACT(p, __per_cpu_start);
 
     return 0;
 }
diff --git a/xen/arch/x86/setup.c b/xen/arch/x86/setup.c
index 06eb483..11d26d7 100644
--- a/xen/arch/x86/setup.c
+++ b/xen/arch/x86/setup.c
@@ -600,7 +600,9 @@ static void noinline init_done(void)
     unregister_init_virtual_region();
 
     /* Zero the .init code and data. */
-    for ( va = __init_begin; va < _p(__init_end); va += PAGE_SIZE )
+    for ( va = __init_begin;
+          SYMBOLS_COMPARE(va, __init_end) < 0;
+          va += PAGE_SIZE )
         clear_page(va);
 
     /* Destroy Xen's mappings, and reuse the pages. */
@@ -972,7 +974,8 @@ 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 = SYMBOLS_SUBTRACT(__2M_rwdata_end,
+                                                        _stext);
     }
 
     modules_headroom = bzimage_headroom(bootstrap_map(mod), mod->mod_end);
@@ -1067,7 +1070,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,
+                        SYMBOLS_SUBTRACT(_end, _start), 1);
 
             /* Walk initial pagetables, relocating page directory entries. */
             pl4e = __va(__pa(idle_pg_table));
diff --git a/xen/arch/x86/smpboot.c b/xen/arch/x86/smpboot.c
index 7d1226d..0bfd4a8 100644
--- a/xen/arch/x86/smpboot.c
+++ b/xen/arch/x86/smpboot.c
@@ -811,7 +811,8 @@ static int setup_cpu_root_pgt(unsigned int cpu)
         const char *ptr;
 
         for ( rc = 0, ptr = _stextentry;
-              !rc && ptr < _etextentry; ptr += PAGE_SIZE )
+              !rc && SYMBOLS_COMPARE(ptr, _etextentry) < 0;
+              ptr += PAGE_SIZE )
             rc = clone_mapping(ptr, rpt);
 
         if ( rc )
diff --git a/xen/drivers/vpci/vpci.c b/xen/drivers/vpci/vpci.c
index 82607bd..88054da 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 (SYMBOLS_SUBTRACT(__end_vpci_array, __start_vpci_array))
 
 void vpci_remove_device(struct pci_dev *pdev)
 {
-- 
1.9.1


_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel

[PATCH v9 5/5] xen/common: use SYMBOLS_SUBTRACT and SYMBOLS_COMPARE as required

[Stefano Stabellini]

Use SYMBOLS_SUBTRACT and SYMBOLS_COMPARE in cases of comparisons and
subtractions of:

_start, _end, _stext, _etext, _srodata, _erodata, _sinittext,
_einittext, __note_gnu_build_id_start, __note_gnu_build_id_end,
__lock_profile_start, __lock_profile_end, __initcall_start,
__initcall_end, __presmp_initcall_end, __ctors_start, __ctors_end,
__end_schedulers_array, __start_schedulers_array, __start_bug_frames,
__stop_bug_frames_0, __stop_bug_frames_1, __stop_bug_frames_2,
__stop_bug_frames_3,

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

Since we are changing the body of is_kernel_text and friends, take the
opportunity to remove the leading underscores in the local variables
names, which are violationg namespace rules. Also make the local p__
variable const.

[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>
Reviewed-by: Jan Beulich <jbeulich@suse.com>
CC: JBeulich@suse.com
CC: andrew.cooper3@citrix.com
---
Changes in v9:
- use SYMBOLS_SUBTRACT and SYMBOLS_COMPARE
---
 xen/common/kernel.c         |  8 ++++++--
 xen/common/lib.c            |  5 ++++-
 xen/common/schedule.c       |  6 ++++--
 xen/common/spinlock.c       |  4 +++-
 xen/common/version.c        |  6 +++---
 xen/common/virtual_region.c |  3 ++-
 xen/include/xen/kernel.h    | 28 ++++++++++++++++------------
 7 files changed, 38 insertions(+), 22 deletions(-)

diff --git a/xen/common/kernel.c b/xen/common/kernel.c
index 5766a0f..b1122a1 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;
+          SYMBOLS_COMPARE(call, __presmp_initcall_end) < 0;
+          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;
+          SYMBOLS_COMPARE(call, __initcall_end) < 0;
+          call++ )
         (*call)();
 }
 
diff --git a/xen/common/lib.c b/xen/common/lib.c
index 8ebec81..ea32367 100644
--- a/xen/common/lib.c
+++ b/xen/common/lib.c
@@ -497,7 +497,10 @@ 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;
+          SYMBOLS_COMPARE(f, __ctors_end) < 0;
+          ++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..0181275 100644
--- a/xen/common/schedule.c
+++ b/xen/common/schedule.c
@@ -67,8 +67,10 @@ DEFINE_PER_CPU(struct scheduler *, scheduler);
 /* Scratch space for cpumasks. */
 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)
+extern const struct scheduler *__start_schedulers_array[],
+                              *__end_schedulers_array[];
+#define NUM_SCHEDULERS (SYMBOLS_SUBTRACT(__end_schedulers_array, \
+                                         __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..1fe292f 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;
+          SYMBOLS_COMPARE(q, &__lock_profile_end) < 0;
+          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..cdf2e56 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 ( SYMBOLS_COMPARE(__note_gnu_build_id_end, &n[0]) <= 0 )
         return -ENODATA;
 
     /* Check for full Note header. */
-    if ( &n[1] >= __note_gnu_build_id_end )
+    if ( SYMBOLS_COMPARE(&n[1], __note_gnu_build_id_end) >= 0 )
         return -ENODATA;
 
-    sz = (void *)__note_gnu_build_id_end - (void *)n;
+    sz = SYMBOLS_SUBTRACT(__note_gnu_build_id_end, (void *)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..b3afcb9 100644
--- a/xen/common/virtual_region.c
+++ b/xen/common/virtual_region.c
@@ -119,7 +119,8 @@ 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;
+        /* bug_frame[i] and s are pointers to different objects. */
+        sz = SYMBOLS_SUBTRACT(bug_frames[i], s);
 
         core.frame[i - 1].n_bugs = sz;
         core.frame[i - 1].bugs = s;
diff --git a/xen/include/xen/kernel.h b/xen/include/xen/kernel.h
index 548b64d..826fc32 100644
--- a/xen/include/xen/kernel.h
+++ b/xen/include/xen/kernel.h
@@ -66,27 +66,31 @@
 })
 
 extern char _start[], _end[], start[];
-#define is_kernel(p) ({                         \
-    char *__p = (char *)(unsigned long)(p);     \
-    (__p >= _start) && (__p < _end);            \
+#define is_kernel(p) ({                                             \
+    const char *p__ = (const char *)(unsigned long)(p);             \
+    (SYMBOLS_COMPARE(p__, _start) >= 0 &&                           \
+    SYMBOLS_COMPARE(p__, _end) < 0);                                \
 })
 
 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 char *p__ = (const char *)(unsigned long)(p);             \
+    (SYMBOLS_COMPARE(p__, _stext) >= 0 &&                           \
+    SYMBOLS_COMPARE(p__, _etext) < 0);                              \
 })
 
 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 char *p__ = (const char *)(unsigned long)(p);             \
+    (SYMBOLS_COMPARE(p__, _srodata) >= 0 &&                         \
+    SYMBOLS_COMPARE(p__, _erodata) < 0);                            \
 })
 
 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 char *p__ = (const char *)(unsigned long)(p);             \
+    (SYMBOLS_COMPARE(p__, _sinittext) >= 0 &&                       \
+    SYMBOLS_COMPARE(p__, _einittext) < 0);                          \
 })
 
 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 v9 2/5] xen: introduce SYMBOLS_SUBTRACT and SYMBOLS_COMPARE

[Ian Jackson]

I would particularly welcome the opinion of hypervisor maintainers on
my type safety point, below.


Stefano Stabellini writes ("[Xen-devel] [PATCH v9 2/5] xen: introduce SYMBOLS_SUBTRACT and SYMBOLS_COMPARE"):
> +/*
> + * Calculate (end - start), where start and/or end are linker symbols,
> + * returning a ptrdiff_t. The size is in units of start's referent.
> + */
> +#define SYMBOLS_SUBTRACT(end, start) ({                                       \
> +    (ptrdiff_t)((uintptr_t)(end) - (uintptr_t)(start)) / sizeof(*start);      \
> +})

I'm afraid I have several problems with the details of these macros.

Firstly, I really don't like the lack a type check, which was present
in my proposal.  For example, given:
     extern struct wombat _foos_start[];
     extern char _end[];
and your macro definition, the two expressions
      SYMBOLS_SUBTRACT(_foos_start, _end)
     -SYMBOLS_SUBTRACT(_end, _foos_start)
produce different values because one is divided by sizeof(struct
wombat) and the other by sizeof(char).  This is hardly desirable.


Secondly, I find the argument ordering extremely confusing.  With your
macros DIFFERENCE(start,end) is negative but COMPARE(start,end) is
positive.  I suggest that you call the macro DIFFERENCE and have
DIFFERENCE(start,end) be positive.


Thirdly, in an earlier exchange:

> On Thu, 7 Feb 2019, Ian Jackson wrote:
> > FAOD, I think you should expect people to declare the linker symbols
> > either as I suggested:
> > 
> >      extern const struct wombat _wombats_start[];
> >      extern const struct abstract_symbol _wombats_end[];
> > 
> > (or along the lines of Jan's suggestion, but frankly I think that is
> > going to be too hard to sort out now.)
> 
> Yes, they are already declared this way, I would prefer to avoid
> changing the declaration as part of this series.

Are you sure things are not currently declared like this
     extern const struct wombat _wombats_start[];
     extern const struct wombat _wombats_end[];
?  Looking at your v9 series, that seems to be the case.

I think using the `abstract_symbol' version (or Jan's refinement of
it) is important because it arranges that accidental raw pointer
comparisons, which are UB, are a type error.

IOW this approach moves and centralises the burden of remembering (on
pain of miscompilation) that something is special about these
pointers, to the point where the linker symbols are introduced into
the C environment.  That is one place, and one where it is easier to
remember because something odd is already going on.

Unless you want to propose some other approach which will reliably
find these kind of linker symbol pointer comparison rule violations
and ensure that they are not deployed in production code ?


This type difference is why my proposal had two macros.

If both start and end have the same type, only one macro is necessary.
It should be one which checks the types are identical and returns a
signed value in units of the type.  That can be used for comparisons.
If desired, we could enhance the macro so that the compiler can prove
that the division can be removed when the result is compared for
inequality with 0.

But even with two types, it may be that there is only one macro needed
because the vast majority of use sites are formulaic.  You said
earlier:

> Most of the call sites only do things like (_end - _start) or (p >
> _end). I wanted to bring up cases that are not trivial.

When designing a general scheme for macros like this it is best to
consider the usual case and make it straightforward to use, and
bulletproof.  Ie presenting unusual cases as your examples is not
helpful to the design process for a macro.

IMO the situation you describe in the snipped I quote is what the
purpose of SYMBOL_DIFFERENCE is.  For the two examples you give,
always one of the arguments is _end.  So if we make the type of _end
be struct abstract_symbol[], SYMBOL_DIFFERENCE can (i) check that _end
is of that type (ii) return an answer in units of its other (first)
argument.

For pointers derived from _start, it is actually straightforwardly
legal to compare them with _start, or subtract _start from them.  So
no macrology is needed in that case.


Stepping back a bit, it is indeed the second symbol referrring to the
same memory object that triggers the compiler bug: the compiler
wrongly assumes that two extern declarations must refer to two
different objects.  Making the two declarations have different types
will simply prevent *all* triggers for this bug, because raw
comparisons or arithmetic between differently typed pointers is a
compile error.  Then all that is needed is to embed the correct usage
pattern into a macro (or, a sufficiently general set of correct usage
patterns that ad-hoc approaches are rare).


You also write:

> We have a couple of cases where we are "punning wildly between pointers
> and integers", for instance:
> 
> xen/arch/arm/arm64/livepatch.c:arch_livepatch_apply
> xen/arch/arm/setup.c:start_xen  line 772
> xen/arch/x86/setup.c:__start_xen  line 1382
> 
> I think it is OK to manually cast to (uintptr_t) in those cases as you
> suggest.

I haven't looked at these, but IMO whatever technique is used comes
with a proof obligation.

In the case of a macro, the proof obligation attaches to the author of
the macro.  The proof should show not only that correct use of the
macro will result in correct code; but there should also be discussion
of the risks of incorrect use of the macro - such incorrect use should
be prevented if that's reasonably possible.

In the case of an ad-hoc technique, the proof obligation comes with
the author of the exciting code.


Ian.

_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel

Re: [PATCH v9 2/5] xen: introduce SYMBOLS_SUBTRACT and SYMBOLS_COMPARE

[Jan Beulich]

>>> On 12.02.19 at 13:01, <ian.jackson@citrix.com> wrote:
> I would particularly welcome the opinion of hypervisor maintainers on
> my type safety point, below.

I agree with the requirements you put forward; I think I'd
prefer the inline function versions I had suggested (or
something similar) over macros though, not the least because
they come with "built-in" type safety, rather than grafted one
(by adding "pseudo" comparisons).

> Stefano Stabellini writes ("[Xen-devel] [PATCH v9 2/5] xen: introduce 
> SYMBOLS_SUBTRACT and SYMBOLS_COMPARE"):
>> +/*
>> + * Calculate (end - start), where start and/or end are linker symbols,
>> + * returning a ptrdiff_t. The size is in units of start's referent.
>> + */
>> +#define SYMBOLS_SUBTRACT(end, start) ({                                       \
>> +    (ptrdiff_t)((uintptr_t)(end) - (uintptr_t)(start)) / sizeof(*start);      \
>> +})
> 
> I'm afraid I have several problems with the details of these macros.
> 
> Firstly, I really don't like the lack a type check, which was present
> in my proposal.  For example, given:
>      extern struct wombat _foos_start[];
>      extern char _end[];
> and your macro definition, the two expressions
>       SYMBOLS_SUBTRACT(_foos_start, _end)
>      -SYMBOLS_SUBTRACT(_end, _foos_start)
> produce different values because one is divided by sizeof(struct
> wombat) and the other by sizeof(char).  This is hardly desirable.
> 
> 
> Secondly, I find the argument ordering extremely confusing.  With your
> macros DIFFERENCE(start,end) is negative but COMPARE(start,end) is
> positive.  I suggest that you call the macro DIFFERENCE and have
> DIFFERENCE(start,end) be positive.

Indeed having to put end before start in either macro invocation
is prone to be got wrong. In the common case this will be noticed
quickly, but even then it's likely one extra compile and test run to
notice that there's something wrong.

However, I realize this is to keep use sites look more like
"end - start", which has its merits as well.

> Thirdly, in an earlier exchange:
> [...]

And fourth, looking at just what's left in context, I see that the
macro argument uses are incompletely parenthesized.

Furthermore - do we really need both a subtract and a compare
construct? The result subtract produces can be used for
comparison purposes as well, after all (just like all CPUs I know
details of implement [integer] compare as a subtract discarding
its numeric result, instead [or only] updating certain status flags).

Jan



_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel

Re: [PATCH v9 2/5] xen: introduce SYMBOLS_SUBTRACT and SYMBOLS_COMPARE

[Ian Jackson]

Jan Beulich writes ("Re: [Xen-devel] [PATCH v9 2/5] xen: introduce SYMBOLS_SUBTRACT and SYMBOLS_COMPARE"):
> On 12.02.19 at 13:01, <ian.jackson@citrix.com> wrote:
> > I would particularly welcome the opinion of hypervisor maintainers on
> > my type safety point, below.
> 
> I agree with the requirements you put forward; I think I'd
> prefer the inline function versions I had suggested (or
> something similar) over macros though, not the least because
> they come with "built-in" type safety, rather than grafted one
> (by adding "pseudo" comparisons).

I didn't see your proposed inline function, but don't think it can
work correctly because it won't be type-generic.  Ie, the requirement
is to use the same `SYMBOLS_DIFFERENCE(p, _foo_end)' for various
different `struct foo *p'.  p is perhaps of different types in the
different invocations and the return value needs to have been divided
by sizeof(*p).


I hate to suggest this now, but as an alternative it would be possible
to do this:

 /*
  *    T *ALTER_PROVENANCE(T *value, T *provenance);
  *
  *    Returns a pointer with the value of `value' but the provenance
  *    of `provenance'; that is, which is considered by the compiler
  *    to be part of same object as `provenance' and not (necessarily)
  *    part of the same object as `value'.
  *
  *    `value' MUST be within whatever the compiler thinks the size
  *    of `provenance' is (if the compiler has any way to know the
  *    size of `provenance').  That is, `value' must be one that can
  *    legally be constructed by indexing within `provenance'.
  *
  *    `value' and `provenance' are evaluated only once each.
  */
 #define ALTER_PROVENANCE(value,provenance) ({
      (void)( typeof((value))0      == (void*)0           );
      (void)( typeof((provenance))0 == (void*)0           );
      (void)( typeof((provenance))0 == typeof((value))0   );
      typeof((provenance)) const alter_provenance__copy = (provenance);
      (typeof((provenance)))(
         (char*)(alter_provenance__copy) +
         ( (uintptr_t)(char*)(value) -
           (uintptr_t)(char*)(alter_provenance__copy) )
      )
 })

(\ omitted for clarity).

But you would still want _foo_end to have a different type to
_foo_start so you would have to wrap ALTER_PROVENANCE in another
macro.


> > Secondly, I find the argument ordering extremely confusing.  With your
> > macros DIFFERENCE(start,end) is negative but COMPARE(start,end) is
> > positive.  I suggest that you call the macro DIFFERENCE and have
> > DIFFERENCE(start,end) be positive.
> 
> Indeed having to put end before start in either macro invocation
> is prone to be got wrong. In the common case this will be noticed
> quickly, but even then it's likely one extra compile and test run to
> notice that there's something wrong.
> 
> However, I realize this is to keep use sites look more like
> "end - start", which has its merits as well.

Yes.  The problem is that with the `-' disappearing, the need to swap
the arguments (as is needed with `-') is much less evident.


> Furthermore - do we really need both a subtract and a compare
> construct? The result subtract produces can be used for
> comparison purposes as well, after all (just like all CPUs I know
> details of implement [integer] compare as a subtract discarding
> its numeric result, instead [or only] updating certain status flags).

I think we only need one macro construct to deal with comparisons with
_foo_end.

The other cases seem very few and don't fall into a convenient pattern
anyway.

Ian.

_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel

Re: [PATCH v9 1/5] xen: introduce ptrdiff_t, fix uintptr_t

[Jan Beulich]

>>> On 12.02.19 at 02:13, <sstabellini@kernel.org> wrote:
> --- a/xen/include/xen/types.h
> +++ b/xen/include/xen/types.h
> @@ -52,7 +52,8 @@ typedef __u32 __be32;
>  typedef __u64 __le64;
>  typedef __u64 __be64;
>  
> -typedef unsigned int __attribute__((__mode__(__pointer__))) uintptr_t;
> +typedef unsigned long __attribute__((__mode__(__pointer__))) uintptr_t;

I don't understand this change: The mode attribute discards any width
association derived from the specified base type.

> +typedef long ptrdiff_t;

FTR I'm unconvinced of this, or the need to use uintptr_t in the first
place in the macros you introduce: The entire UNIX world implies
sizeof(long) == sizeof(void*) afaik.

But if we really want to have ptrdiff_t, then I think we should either
follow the uintptr_t model and use attribute((mode())), or we should
leverage the compiler's __PTRDIFF_TYPE__ (and then also
__UINTPTR_TYPE__ for uintptr_t, at least if available - not sure what
its availability depends on, but it's conditional in gcc's
c_stddef_cpp_builtins()).

Jan



_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel

Re: [PATCH v9 1/5] xen: introduce ptrdiff_t, fix uintptr_t

[Ian Jackson]

Jan Beulich writes ("Re: [PATCH v9 1/5] xen: introduce ptrdiff_t, fix uintptr_t"):
> On 12.02.19 at 02:13, <sstabellini@kernel.org> wrote:
> > -typedef unsigned int __attribute__((__mode__(__pointer__))) uintptr_t;
> > +typedef unsigned long __attribute__((__mode__(__pointer__))) uintptr_t;
> 
> I don't understand this change: The mode attribute discards any width
> association derived from the specified base type.

I looked for the reference documentation for the semantics of
__attribute__((__mode__(__pointer__))).  I found this in my GCC 6
manual:

  'mode (MODE)'
       This attribute specifies the data type for the
       declaration--whichever type corresponds to the mode MODE.  This in
       effect lets you request an integer or floating-point type according
       to its width.

       You may also specify a mode of 'byte' or '__byte__' to indicate the
       mode corresponding to a one-byte integer, 'word' or '__word__' for
       the mode of a one-word integer, and 'pointer' or '__pointer__' for
       the mode used to represent pointers.

This wording is extremely obscure, especially when read in the context
of C's insane pointer provenance rules and compiler authors' insane
interpretations of those rules.

If our goal is to avoid malicious compiler optimisation nonsense we
should not use a feature like this whose semantics might be taken to
imply that the things are really pointers.


> > +typedef long ptrdiff_t;
> 
> FTR I'm unconvinced of this, or the need to use uintptr_t in the first
> place in the macros you introduce: The entire UNIX world implies
> sizeof(long) == sizeof(void*) afaik.

Please provide a reference for this assertion about sizeof(long).

Looking at http://www.unix.org/whitepapers/64bit.html LLP64 (32-bit
long, 64-bit long long, 64-bit void*) was at least contemplated in
some circles, although I haven't been able to find any Unix-like
examples besides Minix, which runs on machines with 16-bit pointers
(and its longs have to be >=32-bit).

> But if we really want to have ptrdiff_t, then I think we should either
> follow the uintptr_t model and use attribute((mode())), or we should
> leverage the compiler's __PTRDIFF_TYPE__ (and then also
> __UINTPTR_TYPE__ for uintptr_t, at least if available - not sure what
> its availability depends on, but it's conditional in gcc's
> c_stddef_cpp_builtins()).

It is not unusual for porting something like Xen to a new architecture
to involve writing a short header file with these kind of type
definitions.  I don't know why we couldn't take that approach.

Ian.

_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel

Re: [PATCH v9 1/5] xen: introduce ptrdiff_t, fix uintptr_t

[Andrew Cooper]

On 12/02/2019 15:26, Ian Jackson wrote:
>
>> But if we really want to have ptrdiff_t, then I think we should either
>> follow the uintptr_t model and use attribute((mode())), or we should
>> leverage the compiler's __PTRDIFF_TYPE__ (and then also
>> __UINTPTR_TYPE__ for uintptr_t, at least if available - not sure what
>> its availability depends on, but it's conditional in gcc's
>> c_stddef_cpp_builtins()).
> It is not unusual for porting something like Xen to a new architecture
> to involve writing a short header file with these kind of type
> definitions.  I don't know why we couldn't take that approach.

stdint.h and inttypes.h are a freestanding header files, and are
intended for uses just like this.

Lets stop second guessing our build environment, and use the solution to
the problem given to us by the C specification.

And to be crystal clear.  This means including <stdint.h> and
<inttypes.h> in xen/types.h and deleting all of these typedefs

~Andrew

_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel

Re: [PATCH v9 2/5] xen: introduce SYMBOLS_SUBTRACT and SYMBOLS_COMPARE

[Jan Beulich]

>>> On 12.02.19 at 15:47, <ian.jackson@citrix.com> wrote:
> Jan Beulich writes ("Re: [Xen-devel] [PATCH v9 2/5] xen: introduce 
> SYMBOLS_SUBTRACT and SYMBOLS_COMPARE"):
>> On 12.02.19 at 13:01, <ian.jackson@citrix.com> wrote:
>> > I would particularly welcome the opinion of hypervisor maintainers on
>> > my type safety point, below.
>> 
>> I agree with the requirements you put forward; I think I'd
>> prefer the inline function versions I had suggested (or
>> something similar) over macros though, not the least because
>> they come with "built-in" type safety, rather than grafted one
>> (by adding "pseudo" comparisons).
> 
> I didn't see your proposed inline function, but don't think it can
> work correctly because it won't be type-generic.  Ie, the requirement
> is to use the same `SYMBOLS_DIFFERENCE(p, _foo_end)' for various
> different `struct foo *p'.  p is perhaps of different types in the
> different invocations and the return value needs to have been divided
> by sizeof(*p).

Well, it wasn't a single inline function, but a macro defining an
inline function suitable for a certain (start,end) tuple, at the
same time giving both start and end suitable types which then
also can't be used in the wrong context:
https://lists.xenproject.org/archives/html/xen-devel/2019-02/msg00440.html

> I hate to suggest this now, but as an alternative it would be possible
> to do this:
> 
>  /*
>   *    T *ALTER_PROVENANCE(T *value, T *provenance);
>   *
>   *    Returns a pointer with the value of `value' but the provenance
>   *    of `provenance'; that is, which is considered by the compiler
>   *    to be part of same object as `provenance' and not (necessarily)
>   *    part of the same object as `value'.
>   *
>   *    `value' MUST be within whatever the compiler thinks the size
>   *    of `provenance' is (if the compiler has any way to know the
>   *    size of `provenance').  That is, `value' must be one that can
>   *    legally be constructed by indexing within `provenance'.
>   *
>   *    `value' and `provenance' are evaluated only once each.
>   */
>  #define ALTER_PROVENANCE(value,provenance) ({
>       (void)( typeof((value))0      == (void*)0           );
>       (void)( typeof((provenance))0 == (void*)0           );
>       (void)( typeof((provenance))0 == typeof((value))0   );
>       typeof((provenance)) const alter_provenance__copy = (provenance);
>       (typeof((provenance)))(
>          (char*)(alter_provenance__copy) +
>          ( (uintptr_t)(char*)(value) -
>            (uintptr_t)(char*)(alter_provenance__copy) )
>       )
>  })

I'm having trouble seeing how this would help with the issues
at hand. Would you mind giving an example of how you'd see
this used?

Jan



_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel

Re: [PATCH v9 1/5] xen: introduce ptrdiff_t, fix uintptr_t

[Jan Beulich]

>>> On 12.02.19 at 16:26, <ian.jackson@citrix.com> wrote:
> Jan Beulich writes ("Re: [PATCH v9 1/5] xen: introduce ptrdiff_t, fix 
> uintptr_t"):
>> On 12.02.19 at 02:13, <sstabellini@kernel.org> wrote:
>> > -typedef unsigned int __attribute__((__mode__(__pointer__))) uintptr_t;
>> > +typedef unsigned long __attribute__((__mode__(__pointer__))) uintptr_t;
>> 
>> I don't understand this change: The mode attribute discards any width
>> association derived from the specified base type.
> 
> I looked for the reference documentation for the semantics of
> __attribute__((__mode__(__pointer__))).  I found this in my GCC 6
> manual:
> 
>   'mode (MODE)'
>        This attribute specifies the data type for the
>        declaration--whichever type corresponds to the mode MODE.  This in
>        effect lets you request an integer or floating-point type according
>        to its width.
> 
>        You may also specify a mode of 'byte' or '__byte__' to indicate the
>        mode corresponding to a one-byte integer, 'word' or '__word__' for
>        the mode of a one-word integer, and 'pointer' or '__pointer__' for
>        the mode used to represent pointers.
> 
> This wording is extremely obscure, especially when read in the context
> of C's insane pointer provenance rules and compiler authors' insane
> interpretations of those rules.
> 
> If our goal is to avoid malicious compiler optimisation nonsense we
> should not use a feature like this whose semantics might be taken to
> imply that the things are really pointers.

I can understand your reservations, albeit I don't share them. But
this is why I've suggested other options.

>> > +typedef long ptrdiff_t;
>> 
>> FTR I'm unconvinced of this, or the need to use uintptr_t in the first
>> place in the macros you introduce: The entire UNIX world implies
>> sizeof(long) == sizeof(void*) afaik.
> 
> Please provide a reference for this assertion about sizeof(long).

Well, my (limited) familiarity with UNIX in general (hence the "afaik")
goes back to the times when the IA64 ABI was formulated, and
when (just like a few years later for x86-64) this was called out as
the main difference to the Windows world. I don't think I can
provide anything that would come close to a "reference", other
than the processor specific ABIs that have emerged from that
discussion.

What's clear though is that both Linux and Xen (having inherited
this from Linux) make this assumption in numerous places (sadly
in Linux this also gets violated every now and then by people
introducing casts between pointers and uint64_t, but once the
warnings get noticed in 32-bit builds things usually get fixed).

>> But if we really want to have ptrdiff_t, then I think we should either
>> follow the uintptr_t model and use attribute((mode())), or we should
>> leverage the compiler's __PTRDIFF_TYPE__ (and then also
>> __UINTPTR_TYPE__ for uintptr_t, at least if available - not sure what
>> its availability depends on, but it's conditional in gcc's
>> c_stddef_cpp_builtins()).
> 
> It is not unusual for porting something like Xen to a new architecture
> to involve writing a short header file with these kind of type
> definitions.  I don't know why we couldn't take that approach.

If we think we need the extra types, I'd be fine going that
route (as much as I would be going the other one; the only
thing I'm not fine with is a mix of approaches). But as with
Stefano's changes, for the moment I'm unconvinced we
actually need uintptr_t or ptrdiff_t here, when - as said - we
already heavily rely on long <-> pointer conversions anyway.

Jan



_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel

Re: [PATCH v9 2/5] xen: introduce SYMBOLS_SUBTRACT and SYMBOLS_COMPARE

[Ian Jackson]

Jan Beulich writes ("Re: [Xen-devel] [PATCH v9 2/5] xen: introduce SYMBOLS_SUBTRACT and SYMBOLS_COMPARE"):
> On 12.02.19 at 15:47, <ian.jackson@citrix.com> wrote:
> > I didn't see your proposed inline function, but don't think it can
> > work correctly because it won't be type-generic.  Ie, the requirement
> > is to use the same `SYMBOLS_DIFFERENCE(p, _foo_end)' for various
> > different `struct foo *p'.  p is perhaps of different types in the
> > different invocations and the return value needs to have been divided
> > by sizeof(*p).
> 
> Well, it wasn't a single inline function, but a macro defining an
> inline function suitable for a certain (start,end) tuple, at the
> same time giving both start and end suitable types which then
> also can't be used in the wrong context:
> https://lists.xenproject.org/archives/html/xen-devel/2019-02/msg00440.html

Ah.  Right.  Yes, that macro looks OK to me.

Although I really think it should use uintpr_t, not `unsigned long'.
IIRC there are special C language spec rules for conversion of
pointers to and from uintptr_t's.  (They are probably applied anyway
for all pointer-to-sufficiently-large-integer conversions but I would
prefer not to bet on that.)

> >  #define ALTER_PROVENANCE(value,provenance) ({
...
> I'm having trouble seeing how this would help with the issues
> at hand. Would you mind giving an example of how you'd see
> this used?

It may be a red herring.  But you could, perhaps, have your WHATEVER
macro do something like this:

  extern const type pfx##_end__wrong_provenance[];
  static const type *const pfx##_end =
     ALTER_PROVENANCE(pfx##_end__wrong_provenance, pfx##_start);

Ian.

_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel

Re: [PATCH v9 1/5] xen: introduce ptrdiff_t, fix uintptr_t

[Jan Beulich]

>>> On 12.02.19 at 16:32, <andrew.cooper3@citrix.com> wrote:
> On 12/02/2019 15:26, Ian Jackson wrote:
>>
>>> But if we really want to have ptrdiff_t, then I think we should either
>>> follow the uintptr_t model and use attribute((mode())), or we should
>>> leverage the compiler's __PTRDIFF_TYPE__ (and then also
>>> __UINTPTR_TYPE__ for uintptr_t, at least if available - not sure what
>>> its availability depends on, but it's conditional in gcc's
>>> c_stddef_cpp_builtins()).
>> It is not unusual for porting something like Xen to a new architecture
>> to involve writing a short header file with these kind of type
>> definitions.  I don't know why we couldn't take that approach.
> 
> stdint.h and inttypes.h are a freestanding header files, and are
> intended for uses just like this.
> 
> Lets stop second guessing our build environment, and use the solution to
> the problem given to us by the C specification.
> 
> And to be crystal clear.  This means including <stdint.h> and
> <inttypes.h> in xen/types.h and deleting all of these typedefs

Well, this would certainly be a viable route if
- these headers were truly freestanding (rather than coming in their
  own incarnation with every gcc version, and perhaps also with
  any other compiler)
- were guaranteed bug free on every distro we care about building on

As an aside, inttypes.h does not seem to be part of the headers
required to be provided by a free standing compiler implementation.
Yet I think no good can come from out of sync inttypes.h and
stdint.h.

I assume you did read chapter 2 "Language Standards Supported
by GCC" of the gcc documentation, just to see their statement of
what they (mean to) support in such a case (and what they don't).

Jan



_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel

Re: [PATCH v9 1/5] xen: introduce ptrdiff_t, fix uintptr_t

[Stefano Stabellini]

On Tue, 12 Feb 2019, Jan Beulich wrote:
> >>> On 12.02.19 at 02:13, <sstabellini@kernel.org> wrote:
> > --- a/xen/include/xen/types.h
> > +++ b/xen/include/xen/types.h
> > @@ -52,7 +52,8 @@ typedef __u32 __be32;
> >  typedef __u64 __le64;
> >  typedef __u64 __be64;
> >  
> > -typedef unsigned int __attribute__((__mode__(__pointer__))) uintptr_t;
> > +typedef unsigned long __attribute__((__mode__(__pointer__))) uintptr_t;
> 
> I don't understand this change: The mode attribute discards any width
> association derived from the specified base type.

Sorry Jan, I muddled the waters even further with this change. :-/

I genuinely thought that it was required to make the whole thing work,
but it turns I was confused. I'll drop this change, except for the
below.


> > +typedef long ptrdiff_t;
> 
> FTR I'm unconvinced of this, or the need to use uintptr_t in the first
> place in the macros you introduce: The entire UNIX world implies
> sizeof(long) == sizeof(void*) afaik.
> 
> But if we really want to have ptrdiff_t, then I think we should either
> follow the uintptr_t model and use attribute((mode())), or we should
> leverage the compiler's __PTRDIFF_TYPE__ (and then also
> __UINTPTR_TYPE__ for uintptr_t, at least if available - not sure what
> its availability depends on, but it's conditional in gcc's
> c_stddef_cpp_builtins()).

I like the idea of using ptrdiff_t, it seems appropriate to me. However,
I have also read the other replies about not using
__attribute__((__mode__(__pointer__))). I don't have an opinion on that,
but we don't have to clean-up the whole of Xen with this series alone,
which is already changing a lot of code.

So, I'll go with the path of least resistance. It looks like
__PTRDIFF_TYPE__ and __UINTPTR_TYPE__ are a good way to do it, so I'll
go with that. But I am also OK with dropping this patch from the series
and using "long" and "unsigned long" instead. Like you wrote, whether
it's right or wrong, the assumption sizeof(void*)==sizeof(unsigned long)
is everywhere in the Xen code.

_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel

Re: [PATCH v9 2/5] xen: introduce SYMBOLS_SUBTRACT and SYMBOLS_COMPARE

[Stefano Stabellini]

On Tue, 12 Feb 2019, Jan Beulich wrote:
> >>> On 12.02.19 at 13:01, <ian.jackson@citrix.com> wrote:
> > I would particularly welcome the opinion of hypervisor maintainers on
> > my type safety point, below.
> 
> I agree with the requirements you put forward; I think I'd
> prefer the inline function versions I had suggested (or
> something similar) over macros though, not the least because
> they come with "built-in" type safety, rather than grafted one
> (by adding "pseudo" comparisons).

I don't mind the type checks in principle, I didn't add them to this
version because, as I wrote in a previous email, with have occurrences
of all three this possible calls:

  SYMBOL_COMPARE/SUBTRACT( symbol, symbol )
  SYMBOL_COMPARE/SUBTRACT( symbol, non-symbol )
  SYMBOL_COMPARE/SUBTRACT( non-symbol, symbol )

If you look through the patches you should be able to spot all three.
As you know "non-symbol" and "symbol" are of different type:

  non-symbol would be like a "struct wombat*"
  symbol would be like a "struct wombat[]"

However, it is not possible to have symbol as struct wombar[] and
non-symbol as something entirely different like char*.

So, my question is: do we need three different variations of these
macros for the types checks?


I don't understand from IanJ email whether the suggestion is to change
the type of all the linker symbols. If so, why are we doing this instead
of the var.S approach? If we go and change the type of all the linker
symbols in C-land, this series will become much bigger and at least as
invasive as the var.S approach, but with added weird macros. It is kind
of a lose - lose situation.

Similarly I would prefer to avoid Jan's proposed inline function approach
because we have a few different array types for the linker symbols
(vpci_register_init_t*, struct scheduler *, etc.), it looks far more
work, and I am already waaaay over-budget for this series (as in 700%
over budget). I would be very happy to "gift it" to somebody else
willing to take it over :-)

Seriously, now that all the calls sites are marked appropriately and we
all agree on the compare/subtract macro approach, it wouldn't be hard
for somebody else to jump in and write the macros in their favorite way.
Let me know if you would like to volunteer!




> > Stefano Stabellini writes ("[Xen-devel] [PATCH v9 2/5] xen: introduce 
> > SYMBOLS_SUBTRACT and SYMBOLS_COMPARE"):
> >> +/*
> >> + * Calculate (end - start), where start and/or end are linker symbols,
> >> + * returning a ptrdiff_t. The size is in units of start's referent.
> >> + */
> >> +#define SYMBOLS_SUBTRACT(end, start) ({                                       \
> >> +    (ptrdiff_t)((uintptr_t)(end) - (uintptr_t)(start)) / sizeof(*start);      \
> >> +})
> > 
> > I'm afraid I have several problems with the details of these macros.
> > 
> > Firstly, I really don't like the lack a type check, which was present
> > in my proposal.  For example, given:
> >      extern struct wombat _foos_start[];
> >      extern char _end[];
> > and your macro definition, the two expressions
> >       SYMBOLS_SUBTRACT(_foos_start, _end)
> >      -SYMBOLS_SUBTRACT(_end, _foos_start)
> > produce different values because one is divided by sizeof(struct
> > wombat) and the other by sizeof(char).  This is hardly desirable.
> > 
> > 
> > Secondly, I find the argument ordering extremely confusing.  With your
> > macros DIFFERENCE(start,end) is negative but COMPARE(start,end) is
> > positive.  I suggest that you call the macro DIFFERENCE and have
> > DIFFERENCE(start,end) be positive.
> 
> Indeed having to put end before start in either macro invocation
> is prone to be got wrong. In the common case this will be noticed
> quickly, but even then it's likely one extra compile and test run to
> notice that there's something wrong.
> 
> However, I realize this is to keep use sites look more like
> "end - start", which has its merits as well.

Yes, I attempted to keep the same ordering as before. I can change it
without too much troubles if that is what we want.


> > Thirdly, in an earlier exchange:
> > [...]
> 
> And fourth, looking at just what's left in context, I see that the
> macro argument uses are incompletely parenthesized.
> 
> Furthermore - do we really need both a subtract and a compare
> construct? The result subtract produces can be used for
> comparison purposes as well, after all (just like all CPUs I know
> details of implement [integer] compare as a subtract discarding
> its numeric result, instead [or only] updating certain status flags).

No, we don't. In my first attempt I only had one macro. I am happy to
follow your suggestion and keep only SUBTRACT.

_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel

Re: [PATCH v9 2/5] xen: introduce SYMBOLS_SUBTRACT and SYMBOLS_COMPARE

[Jan Beulich]

>>> On 13.02.19 at 02:17, <sstabellini@kernel.org> wrote:
> On Tue, 12 Feb 2019, Jan Beulich wrote:
>> >>> On 12.02.19 at 13:01, <ian.jackson@citrix.com> wrote:
>> > I would particularly welcome the opinion of hypervisor maintainers on
>> > my type safety point, below.
>> 
>> I agree with the requirements you put forward; I think I'd
>> prefer the inline function versions I had suggested (or
>> something similar) over macros though, not the least because
>> they come with "built-in" type safety, rather than grafted one
>> (by adding "pseudo" comparisons).
> 
> I don't mind the type checks in principle, I didn't add them to this
> version because, as I wrote in a previous email, with have occurrences
> of all three this possible calls:
> 
>   SYMBOL_COMPARE/SUBTRACT( symbol, symbol )
>   SYMBOL_COMPARE/SUBTRACT( symbol, non-symbol )
>   SYMBOL_COMPARE/SUBTRACT( non-symbol, symbol )
> 
> If you look through the patches you should be able to spot all three.
> As you know "non-symbol" and "symbol" are of different type:
> 
>   non-symbol would be like a "struct wombat*"
>   symbol would be like a "struct wombat[]"

These are declared types. Effective type when used as rvalue
(i.e. also when passed as arguments to functions) is struct
wombat * in both cases.

The important aspect (and new idea) Ian has been introducing
really is that the "end" symbols intentionally no longer be of
the same type as the start ones (but some type derived
thereof).

> However, it is not possible to have symbol as struct wombar[] and
> non-symbol as something entirely different like char*.
> 
> So, my question is: do we need three different variations of these
> macros for the types checks?
> 
> 
> I don't understand from IanJ email whether the suggestion is to change
> the type of all the linker symbols. If so, why are we doing this instead
> of the var.S approach? If we go and change the type of all the linker
> symbols in C-land, this series will become much bigger and at least as
> invasive as the var.S approach, but with added weird macros. It is kind
> of a lose - lose situation.
> 
> Similarly I would prefer to avoid Jan's proposed inline function approach
> because we have a few different array types for the linker symbols
> (vpci_register_init_t*, struct scheduler *, etc.), it looks far more
> work, and I am already waaaay over-budget for this series (as in 700%
> over budget). I would be very happy to "gift it" to somebody else
> willing to take it over :-)
> 
> Seriously, now that all the calls sites are marked appropriately and we
> all agree on the compare/subtract macro approach, it wouldn't be hard
> for somebody else to jump in and write the macros in their favorite way.
> Let me know if you would like to volunteer!

I've indeed been considering this already, as I expected the point
would come up sooner or later. Thing is though that, in particular
with Adrian not having replied at all so far, I'm still unconvinced that
we need to make this many changes (i.e. other than to work around
compiler deficiencies, which would boil down to using SYMBOL_HIDE()
from v7, but only in places where it is known certain compiler versions
might mis-optimize it, and with a clear reference to the involved
compiler bug/versions) at all. It's just that what we're now discussing
is the approach I have the least problem following _if_ such a global
"marking" of linker symbol uses ends up being necessary.

>> Furthermore - do we really need both a subtract and a compare
>> construct? The result subtract produces can be used for
>> comparison purposes as well, after all (just like all CPUs I know
>> details of implement [integer] compare as a subtract discarding
>> its numeric result, instead [or only] updating certain status flags).
> 
> No, we don't. In my first attempt I only had one macro. I am happy to
> follow your suggestion and keep only SUBTRACT.

Except that, as I think Ian has also suggested, DIFFERENCE() (or
SYMBOL_DIFFERENCE()) might be better, as it (hopefully)
reduces the connections to the - operator, and hence the risk
of possibly getting the argument order wrong. Otoh with the
type safety added wrong argument order would cause a
compile time error.

Jan


_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel

Re: [PATCH v9 1/5] xen: introduce ptrdiff_t, fix uintptr_t

[George Dunlap]

On 2/12/19 4:03 PM, Jan Beulich wrote:
>>>> On 12.02.19 at 16:32, <andrew.cooper3@citrix.com> wrote:
>> On 12/02/2019 15:26, Ian Jackson wrote:
>>>
>>>> But if we really want to have ptrdiff_t, then I think we should either
>>>> follow the uintptr_t model and use attribute((mode())), or we should
>>>> leverage the compiler's __PTRDIFF_TYPE__ (and then also
>>>> __UINTPTR_TYPE__ for uintptr_t, at least if available - not sure what
>>>> its availability depends on, but it's conditional in gcc's
>>>> c_stddef_cpp_builtins()).
>>> It is not unusual for porting something like Xen to a new architecture
>>> to involve writing a short header file with these kind of type
>>> definitions.  I don't know why we couldn't take that approach.
>>
>> stdint.h and inttypes.h are a freestanding header files, and are
>> intended for uses just like this.
>>
>> Lets stop second guessing our build environment, and use the solution to
>> the problem given to us by the C specification.
>>
>> And to be crystal clear.  This means including <stdint.h> and
>> <inttypes.h> in xen/types.h and deleting all of these typedefs
> 
> Well, this would certainly be a viable route if
> - these headers were truly freestanding (rather than coming in their
>   own incarnation with every gcc version, and perhaps also with
>   any other compiler)

Why would this be a problem?  Isn't that a feature -- that your
compiler/header file combination provides you with the proper runes to
get a working uintptr_t?  That seems a lot more robust and reliable than
trying to keep our own header that works on all possible compiler
combinations.

> - were guaranteed bug free on every distro we care about building on

This argument seems kind of backwards to me.

You are implicitly asserting that there are distros we care about
building on where the system stdint.h and inttypes.h have bugs.  It's
not up to others to show that this is false, but up to you to show that
it's true; by providing at least some examples (even if historical).

Or if you don't have examples ready to hand, saying something like:

"I seem to recall some distros having bugs in stdint.h in the past.  We
should make sure to test it with all distro versions we care about
before committing to it."  This should be somewhat easier now that we
have the docker build-testing infrastructure.

 -George

_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel

Re: [PATCH v9 1/5] xen: introduce ptrdiff_t, fix uintptr_t

[Jan Beulich]

>>> On 13.02.19 at 11:28, <george.dunlap@citrix.com> wrote:
> On 2/12/19 4:03 PM, Jan Beulich wrote:
>>>>> On 12.02.19 at 16:32, <andrew.cooper3@citrix.com> wrote:
>>> On 12/02/2019 15:26, Ian Jackson wrote:
>>>>
>>>>> But if we really want to have ptrdiff_t, then I think we should either
>>>>> follow the uintptr_t model and use attribute((mode())), or we should
>>>>> leverage the compiler's __PTRDIFF_TYPE__ (and then also
>>>>> __UINTPTR_TYPE__ for uintptr_t, at least if available - not sure what
>>>>> its availability depends on, but it's conditional in gcc's
>>>>> c_stddef_cpp_builtins()).
>>>> It is not unusual for porting something like Xen to a new architecture
>>>> to involve writing a short header file with these kind of type
>>>> definitions.  I don't know why we couldn't take that approach.
>>>
>>> stdint.h and inttypes.h are a freestanding header files, and are
>>> intended for uses just like this.
>>>
>>> Lets stop second guessing our build environment, and use the solution to
>>> the problem given to us by the C specification.
>>>
>>> And to be crystal clear.  This means including <stdint.h> and
>>> <inttypes.h> in xen/types.h and deleting all of these typedefs
>> 
>> Well, this would certainly be a viable route if
>> - these headers were truly freestanding (rather than coming in their
>>   own incarnation with every gcc version, and perhaps also with
>>   any other compiler)
> 
> Why would this be a problem?  Isn't that a feature -- that your
> compiler/header file combination provides you with the proper runes to
> get a working uintptr_t?  That seems a lot more robust and reliable than
> trying to keep our own header that works on all possible compiler
> combinations.

Well, that depends on which instance of the header one ends up
including - the one in /usr/include/, or the specific one coming
with the specific compiler version. It is certainly possible that I'm
too biased due to my DOS / OS/2 / Windows / NetWare heritage,
where compilers didn't necessarily know where to take headers
from without being told, but I'm simply uncertain whether of all
compilers we care about (and also all ones we want to care
about in the future) we can expect that they'll find the right
headers without our help, and they won't fall back to e.g. the
ones in /usr/include/, or not find anything at all.

>> - were guaranteed bug free on every distro we care about building on
> 
> This argument seems kind of backwards to me.
> 
> You are implicitly asserting that there are distros we care about
> building on where the system stdint.h and inttypes.h have bugs.  It's
> not up to others to show that this is false, but up to you to show that
> it's true; by providing at least some examples (even if historical).
> 
> Or if you don't have examples ready to hand, saying something like:
> 
> "I seem to recall some distros having bugs in stdint.h in the past.  We
> should make sure to test it with all distro versions we care about
> before committing to it."  This should be somewhat easier now that we
> have the docker build-testing infrastructure.

Oh, that's pretty easy: Why would gcc care to produce fixed up
host headers (see the fixincludes/ subdirectory in its sources and
how it's used by the build process), if there wasn't the problem of
distros getting their headers wrong?

But then again I don't think I agree with you considering this
backwards: Bugs exist, there's no question. For something as
fundamental as producing correct basic types (which _can_ be
got right without relying on any headers outside of a project's
source tree), the mere risk of there possibly being bugs is bad
enough imo.

Jan



_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel

Re: [PATCH v9 2/5] xen: introduce SYMBOLS_SUBTRACT and SYMBOLS_COMPARE

[Stefano Stabellini]

On Wed, 13 Feb 2019, Jan Beulich wrote:
> >>> On 13.02.19 at 02:17, <sstabellini@kernel.org> wrote:
> > On Tue, 12 Feb 2019, Jan Beulich wrote:
> >> >>> On 12.02.19 at 13:01, <ian.jackson@citrix.com> wrote:
> >> > I would particularly welcome the opinion of hypervisor maintainers on
> >> > my type safety point, below.
> >> 
> >> I agree with the requirements you put forward; I think I'd
> >> prefer the inline function versions I had suggested (or
> >> something similar) over macros though, not the least because
> >> they come with "built-in" type safety, rather than grafted one
> >> (by adding "pseudo" comparisons).
> > 
> > I don't mind the type checks in principle, I didn't add them to this
> > version because, as I wrote in a previous email, with have occurrences
> > of all three this possible calls:
> > 
> >   SYMBOL_COMPARE/SUBTRACT( symbol, symbol )
> >   SYMBOL_COMPARE/SUBTRACT( symbol, non-symbol )
> >   SYMBOL_COMPARE/SUBTRACT( non-symbol, symbol )
> > 
> > If you look through the patches you should be able to spot all three.
> > As you know "non-symbol" and "symbol" are of different type:
> > 
> >   non-symbol would be like a "struct wombat*"
> >   symbol would be like a "struct wombat[]"
> 
> These are declared types. Effective type when used as rvalue
> (i.e. also when passed as arguments to functions) is struct
> wombat * in both cases.

I see..


> The important aspect (and new idea) Ian has been introducing
> really is that the "end" symbols intentionally no longer be of
> the same type as the start ones (but some type derived
> thereof).

I missed that part of his proposal. Reading back your suggested static
inline functions and WHATEVER macro I can see that it works. But I don't
understand why is it desirable to have the "end" symbols intentionally
no longer be of the same type as the start ones.

Also, I would ask to consinder the impact of WHATEVER on the code versus
the var.S approach, which at least has the benefit of being simpler.


> > However, it is not possible to have symbol as struct wombar[] and
> > non-symbol as something entirely different like char*.
> > 
> > So, my question is: do we need three different variations of these
> > macros for the types checks?
> > 
> > 
> > I don't understand from IanJ email whether the suggestion is to change
> > the type of all the linker symbols. If so, why are we doing this instead
> > of the var.S approach? If we go and change the type of all the linker
> > symbols in C-land, this series will become much bigger and at least as
> > invasive as the var.S approach, but with added weird macros. It is kind
> > of a lose - lose situation.
> > 
> > Similarly I would prefer to avoid Jan's proposed inline function approach
> > because we have a few different array types for the linker symbols
> > (vpci_register_init_t*, struct scheduler *, etc.), it looks far more
> > work, and I am already waaaay over-budget for this series (as in 700%
> > over budget). I would be very happy to "gift it" to somebody else
> > willing to take it over :-)
> > 
> > Seriously, now that all the calls sites are marked appropriately and we
> > all agree on the compare/subtract macro approach, it wouldn't be hard
> > for somebody else to jump in and write the macros in their favorite way.
> > Let me know if you would like to volunteer!
> 
> I've indeed been considering this already, as I expected the point
> would come up sooner or later. Thing is though that, in particular
> with Adrian not having replied at all so far, I'm still unconvinced that
> we need to make this many changes (i.e. other than to work around
> compiler deficiencies, which would boil down to using SYMBOL_HIDE()
> from v7, but only in places where it is known certain compiler versions
> might mis-optimize it, and with a clear reference to the involved
> compiler bug/versions) at all. It's just that what we're now discussing
> is the approach I have the least problem following _if_ such a global
> "marking" of linker symbol uses ends up being necessary.

Thank you for taking this into consideration, I really appreciate it!
When/If we decide that we need a global "marking", and we also want
"fancy" type-checking, I would really appreciate your help.


> >> Furthermore - do we really need both a subtract and a compare
> >> construct? The result subtract produces can be used for
> >> comparison purposes as well, after all (just like all CPUs I know
> >> details of implement [integer] compare as a subtract discarding
> >> its numeric result, instead [or only] updating certain status flags).
> > 
> > No, we don't. In my first attempt I only had one macro. I am happy to
> > follow your suggestion and keep only SUBTRACT.
> 
> Except that, as I think Ian has also suggested, DIFFERENCE() (or
> SYMBOL_DIFFERENCE()) might be better, as it (hopefully)
> reduces the connections to the - operator, and hence the risk
> of possibly getting the argument order wrong. Otoh with the
> type safety added wrong argument order would cause a
> compile time error.

I don't mind either way. The good thing about the way is done in this
series is that all the comparison signs (<, >, <=, >=, etc) didn't have
to be changed. It makes it much easier to review and check it's correct.

_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel

Re: [PATCH v9 2/5] xen: introduce SYMBOLS_SUBTRACT and SYMBOLS_COMPARE

[Jan Beulich]

>>> On 14.02.19 at 00:30, <sstabellini@kernel.org> wrote:
> On Wed, 13 Feb 2019, Jan Beulich wrote:
>> >>> On 13.02.19 at 02:17, <sstabellini@kernel.org> wrote:
>> > On Tue, 12 Feb 2019, Jan Beulich wrote:
>> >> >>> On 12.02.19 at 13:01, <ian.jackson@citrix.com> wrote:
>> >> > I would particularly welcome the opinion of hypervisor maintainers on
>> >> > my type safety point, below.
>> >> 
>> >> I agree with the requirements you put forward; I think I'd
>> >> prefer the inline function versions I had suggested (or
>> >> something similar) over macros though, not the least because
>> >> they come with "built-in" type safety, rather than grafted one
>> >> (by adding "pseudo" comparisons).
>> > 
>> > I don't mind the type checks in principle, I didn't add them to this
>> > version because, as I wrote in a previous email, with have occurrences
>> > of all three this possible calls:
>> > 
>> >   SYMBOL_COMPARE/SUBTRACT( symbol, symbol )
>> >   SYMBOL_COMPARE/SUBTRACT( symbol, non-symbol )
>> >   SYMBOL_COMPARE/SUBTRACT( non-symbol, symbol )
>> > 
>> > If you look through the patches you should be able to spot all three.
>> > As you know "non-symbol" and "symbol" are of different type:
>> > 
>> >   non-symbol would be like a "struct wombat*"
>> >   symbol would be like a "struct wombat[]"
>> 
>> These are declared types. Effective type when used as rvalue
>> (i.e. also when passed as arguments to functions) is struct
>> wombat * in both cases.
> 
> I see..
> 
> 
>> The important aspect (and new idea) Ian has been introducing
>> really is that the "end" symbols intentionally no longer be of
>> the same type as the start ones (but some type derived
>> thereof).
> 
> I missed that part of his proposal. Reading back your suggested static
> inline functions and WHATEVER macro I can see that it works. But I don't
> understand why is it desirable to have the "end" symbols intentionally
> no longer be of the same type as the start ones.

As said - this is to make it impossible to mistakenly invert start and
end in a function (or macro) invocation (without the compiler pointing
out the issue).

> Also, I would ask to consinder the impact of WHATEVER on the code versus
> the var.S approach, which at least has the benefit of being simpler.

I'm not sure of this.

>> >> Furthermore - do we really need both a subtract and a compare
>> >> construct? The result subtract produces can be used for
>> >> comparison purposes as well, after all (just like all CPUs I know
>> >> details of implement [integer] compare as a subtract discarding
>> >> its numeric result, instead [or only] updating certain status flags).
>> > 
>> > No, we don't. In my first attempt I only had one macro. I am happy to
>> > follow your suggestion and keep only SUBTRACT.
>> 
>> Except that, as I think Ian has also suggested, DIFFERENCE() (or
>> SYMBOL_DIFFERENCE()) might be better, as it (hopefully)
>> reduces the connections to the - operator, and hence the risk
>> of possibly getting the argument order wrong. Otoh with the
>> type safety added wrong argument order would cause a
>> compile time error.
> 
> I don't mind either way. The good thing about the way is done in this
> series is that all the comparison signs (<, >, <=, >=, etc) didn't have
> to be changed. It makes it much easier to review and check it's correct.

Yeah, I've realized this as well meanwhile: With

#define DIFFERENCE(s, e) ((e) - (s))

(shortened to just show the essential aspect here) at least one of

    if ( DIFFERENCE(start, end) > 0 )

and

   diff = DIFFERENCE(start, end);

becomes confusing to read. Specifying the arguments the other
way around seems counterintuitive, and swapping the operands of
- would improve the comparison use, but the difference calculation
would then require an un-obvious explicit negation of the result.

Ian, do you have any further thoughts here?

Jan



_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel