[PATCH v11 0/9] misc safety certification fixes

[PATCH v11 0/9] misc safety certification fixes

[Stefano Stabellini]

Hi all,

This version of the series makes use of the macro suggested by Jan with
few modifications. See each patch for a description of the changes.

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

for you to fetch changes up to 26fb02b5ae59b48b51ca788be91510d8df48ab0a:

  xen: explicit casts when DECLARE_BOUNDS cannot be used (2019-03-05 14:29:51 -0800)

----------------------------------------------------------------
Stefano Stabellini (9):
      xen: use __UINTPTR_TYPE__ for uintptr_t
      xen: introduce ptrdiff_t
      xen: introduce DECLARE_BOUNDS
      xen/arm: use DECLARE_BOUNDS as required
      xen/x86: use DECLARE_BOUNDS as required
      xen/common: use DECLARE_BOUNDS as required
      xen: use DECLARE_BOUNDS as required
      xen: use DECLARE_BOUNDS in alternative.c
      xen: explicit casts when DECLARE_BOUNDS cannot be used

 xen/arch/arm/alternative.c        | 16 +++----
 xen/arch/arm/arm32/livepatch.c    |  2 +-
 xen/arch/arm/arm64/livepatch.c    |  2 +-
 xen/arch/arm/device.c             | 14 ++++---
 xen/arch/arm/livepatch.c          |  4 +-
 xen/arch/arm/mm.c                 | 14 ++++---
 xen/arch/arm/percpu.c             | 11 ++---
 xen/arch/arm/platform.c           |  9 ++--
 xen/arch/arm/setup.c              |  5 ++-
 xen/arch/arm/xen.lds.S            |  3 +-
 xen/arch/x86/alternative.c        | 10 ++---
 xen/arch/x86/efi/efi-boot.h       | 10 +++--
 xen/arch/x86/percpu.c             | 11 ++---
 xen/arch/x86/setup.c              | 14 +++++--
 xen/arch/x86/smpboot.c            |  6 ++-
 xen/arch/x86/xen.lds.S            |  3 +-
 xen/common/kernel.c               | 13 ++++--
 xen/common/lib.c                  |  7 +++-
 xen/common/livepatch.c            |  5 ++-
 xen/common/schedule.c             | 11 ++++-
 xen/common/spinlock.c             |  8 ++--
 xen/common/version.c              |  9 ++--
 xen/common/virtual_region.c       |  4 +-
 xen/drivers/vpci/vpci.c           |  6 +--
 xen/include/asm-arm/alternative.h |  6 ++-
 xen/include/asm-arm/grant_table.h |  3 +-
 xen/include/asm-arm/percpu.h      |  4 +-
 xen/include/asm-x86/alternative.h |  7 +++-
 xen/include/asm-x86/percpu.h      |  6 ++-
 xen/include/xen/kernel.h          | 42 ++++++++++++-------
 xen/include/xen/lib.h             | 88 +++++++++++++++++++++++++++++++++++++++
 xen/include/xen/types.h           |  3 +-
 32 files changed, 259 insertions(+), 97 deletions(-)

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

[PATCH v11 1/9] xen: use __UINTPTR_TYPE__ for uintptr_t

[Stefano Stabellini]

Use __UINTPTR_TYPE__ to define uintptr_t. A later patch will make use of
__PTRDIFF_TYPE__ to define ptrdiff_t.

Signed-off-by: Stefano Stabellini <stefanos@xilinx.com>
Reviewed-by: Ian Jackson <ian.jackson@eu.citrix.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 v11:
- split patch
---
 xen/include/xen/types.h | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/xen/include/xen/types.h b/xen/include/xen/types.h
index 03f0fe6..1059fab 100644
--- a/xen/include/xen/types.h
+++ b/xen/include/xen/types.h
@@ -52,7 +52,7 @@ typedef __u32 __be32;
 typedef __u64 __le64;
 typedef __u64 __be64;
 
-typedef unsigned int __attribute__((__mode__(__pointer__))) uintptr_t;
+typedef __UINTPTR_TYPE__ uintptr_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 v11 2/9] xen: introduce ptrdiff_t

[Stefano Stabellini]

Introduce the new type "ptrdiff_t" which is defined as the signed
integer type of the result of subtracting two pointers. Use
__PTRDIFF_TYPE__ to define it.

Signed-off-by: Stefano Stabellini <stefanos@xilinx.com>
Reviewed-by: Ian Jackson <ian.jackson@eu.citrix.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 v11:
- split patch
---
 xen/include/xen/types.h | 1 +
 1 file changed, 1 insertion(+)

diff --git a/xen/include/xen/types.h b/xen/include/xen/types.h
index 1059fab..508a462 100644
--- a/xen/include/xen/types.h
+++ b/xen/include/xen/types.h
@@ -53,6 +53,7 @@ typedef __u64 __le64;
 typedef __u64 __be64;
 
 typedef __UINTPTR_TYPE__ uintptr_t;
+typedef __PTRDIFF_TYPE__ 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 v11 3/9] xen: introduce DECLARE_BOUNDS

[Stefano Stabellini]

Introduce a MACRO to be used to declare array variables corresponding to
linker symbols, plus two static inline functions to be used for
comparing and subtracting pointers with the linker symbols.

Note that the start and end symbols are declared of different types to
help avoid errors and misusing those variables.

Use a build-time assertion to check the proper alignment of the structs
passed as arguments to the static inline functions. Use BUILD_BUG_ON for
the implementation.

Suggested-by: Jan Beulich <JBeulich@suse.com>
Suggested-by: Ian Jackson <ian.jackson@citrix.com>
Signed-off-by: Stefano Stabellini <stefanos@xilinx.com>
---
Changes in v11:
- add ptrdiff_t casts in _diff macro
- improve comment
- add build-time assertion on struct alignment
- add _bytediff function
- move the macros to xen/lib.h
- rename DEFINE_SYMBOL to __DECLARE_BOUNDS
- add wrappers
---
 xen/include/xen/lib.h | 88 +++++++++++++++++++++++++++++++++++++++++++++++++++
 1 file changed, 88 insertions(+)

diff --git a/xen/include/xen/lib.h b/xen/include/xen/lib.h
index 972fc84..3b1a283 100644
--- a/xen/include/xen/lib.h
+++ b/xen/include/xen/lib.h
@@ -173,4 +173,92 @@ void init_constructors(void);
 void *bsearch(const void *key, const void *base, size_t num, size_t size,
               int (*cmp)(const void *key, const void *elt));
 
+ /*
+  * Declare start and end array variables in C corresponding to existing
+  * linker symbols.
+  *
+  * These macros, or an alternative technique, MUST be used any time
+  * linker symbols are imported into C via the `extern []' idiom.
+  *
+  *    __DECLARE_BOUNDS(TYPE, START, START, END)
+  *
+  *  introduces the following two constant expressions
+  *
+  *    const TYPE *START;
+  *    const struct abstract_NAME *END;
+  *
+  *  whose values are the linker symbols START and END; these
+  *  should be the start and end of a memory region.
+  *
+  *  You may then use these two inline functions:
+  *
+  *    bool NAME_lt(const TYPE *s1, const struct abstract_NAME *s2);
+  *    ptrdiff_t NAME_diff(const TYPE *s1, const struct abstract_NAME *s2);
+  *
+  *  lt returns true iff s1 < s2.
+  *  diff returns the s2-s1 in units of TYPE.
+  *
+  *
+  * You MUST NOT cast a struct abstract_NAME* to a TYPE*.  Doing so
+  * risks miscompilation.  If you need to operate on a struct
+  * abstract_NAME* in a way not supported here, you must provide
+  * a clear argument explaining why (i) the compiler will not
+  * misoptimise your code (ii) future programmers will not
+  * accidentally introduce errors.
+  *
+  * Rationale:
+  *
+  * This exists because compilers erroneously believe that no two
+  * external symbols can refer to the same array.  They deem
+  * operations (e.g. comparisons) which mix pointers from different
+  * linker symbols illegal and miscompile them.  We consider this a
+  * compiler bug (or standards bug) but are not in a position to make
+  * the compilers sane; so we must work around things.
+  *
+  * The workaround is to do arithmetic and comparisons on uintptr_t's
+  * derived from the pointers.  Arithmetic on uintptr_t is of course
+  * always defined. The conversion from a pointer is implementation
+  * defined, but Xen cannot run on a platform where the conversion is
+  * anything other than the usual bit pattern equivalence.
+  *
+  * Wrapping end in a new type prevents it being accidentally compared
+  * to or subtracted from pointers derived from start.
+  */
+#define __DECLARE_BOUNDS(type, name, start_name, end_name)                    \
+                                                                              \
+struct abstract_ ## name {                                                    \
+    type _;                                                                   \
+};                                                                            \
+                                                                              \
+extern const type start_name[];                                               \
+extern const struct abstract_ ## name end_name[];                             \
+                                                                              \
+static inline bool name ## _lt(const type s1[],                               \
+                               const struct abstract_ ## name s2[])           \
+{                                                                             \
+    BUILD_BUG_ON(alignof(*s1) != alignof(*s2));                               \
+    return (uintptr_t)s1 < (uintptr_t)s2;                                     \
+}                                                                             \
+                                                                              \
+static inline ptrdiff_t name ## _diff(const type s1[],                        \
+                                      const struct abstract_ ## name s2[])    \
+{                                                                             \
+    BUILD_BUG_ON(alignof(*s1) != alignof(*s2));                               \
+    return (ptrdiff_t)((uintptr_t)s2 - (uintptr_t)s1) /                       \
+           (ptrdiff_t)sizeof(*s1);                                            \
+}                                                                             \
+                                                                              \
+static inline ptrdiff_t name ## _bytediff(const type s1[],                    \
+                                          const struct abstract_ ## name s2[])\
+{                                                                             \
+    BUILD_BUG_ON(alignof(*s1) != alignof(*s2));                               \
+    return (ptrdiff_t)((uintptr_t)s2 - (uintptr_t)s1);                        \
+}
+
+#define DECLARE_BOUNDS(name, name_start, name_end)                            \
+        __DECLARE_BOUNDS(name ## _t, name, name_start, name_end);
+
+#define DECLARE_ARRAY_BOUNDS(name)                                            \
+        DECLARE_BOUNDS(name, __ ## name ## _start, __ ## name ## _end)
+
 #endif /* __LIB_H__ */
-- 
1.9.1


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

[PATCH v11 4/9] xen/arm: use DECLARE_BOUNDS as required

[Stefano Stabellini]

Use DECLARE_BOUNDS and the two static inline functions that come with it
for comparisons and subtractions of:

__init_begin, __init_end, __per_cpu_start, __per_cpu_data_end,
_splatform, _eplatform, _sdevice, _edevice, _asdevice, _aedevice.

M3CM: Rule-18.2: Subtraction between pointers shall only be applied to
pointers that address elements of the same array

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 v11:
- change p type to struct abstract_per_cpu *
- move changes to alternative.c to a new patch
- use DECLARE_BOUNDS

Changes in v10:
- use DEFINE_SYMBOL
- move changes for _start, _end, _stext, _etext, _srodata, _erodata,
  _sinittext, _einittext to a different patch

Changes in v9:
- use SYMBOLS_SUBTRACT and SYMBOLS_COMPARE
---
 xen/arch/arm/device.c        | 14 +++++++++-----
 xen/arch/arm/mm.c            |  8 +++++---
 xen/arch/arm/percpu.c        | 11 ++++++-----
 xen/arch/arm/platform.c      |  9 ++++++---
 xen/include/asm-arm/percpu.h |  4 +++-
 5 files changed, 29 insertions(+), 17 deletions(-)

diff --git a/xen/arch/arm/device.c b/xen/arch/arm/device.c
index 70cd6c1..2de71f6 100644
--- a/xen/arch/arm/device.c
+++ b/xen/arch/arm/device.c
@@ -22,8 +22,10 @@
 #include <xen/init.h>
 #include <xen/lib.h>
 
-extern const struct device_desc _sdevice[], _edevice[];
-extern const struct acpi_device_desc _asdevice[], _aedevice[];
+typedef struct device_desc device_desc_t;
+DECLARE_BOUNDS(device_desc, _sdevice, _edevice);
+typedef struct acpi_device_desc acpi_device_desc_t;
+DECLARE_BOUNDS(acpi_device_desc, _asdevice, _aedevice);
 
 int __init device_init(struct dt_device_node *dev, enum device_class class,
                        const void *data)
@@ -35,7 +37,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; device_desc_diff(desc, _edevice) != 0; desc++ )
     {
         if ( desc->class != class )
             continue;
@@ -56,7 +58,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;
+          acpi_device_desc_diff(desc, _aedevice) != 0;
+          desc++ )
     {
         if ( ( desc->class != class ) || ( desc->class_type != class_type ) )
             continue;
@@ -75,7 +79,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; device_desc_diff(desc, _edevice) != 0; desc++ )
     {
         if ( dt_match_node(desc->dt_match, dev) )
             return desc->class;
diff --git a/xen/arch/arm/mm.c b/xen/arch/arm/mm.c
index 01ae2cc..df52b26 100644
--- a/xen/arch/arm/mm.c
+++ b/xen/arch/arm/mm.c
@@ -157,7 +157,8 @@ unsigned long frametable_virt_end __read_mostly;
 unsigned long max_page;
 unsigned long total_pages;
 
-extern char __init_begin[], __init_end[];
+typedef char init_t;
+DECLARE_BOUNDS(init, __init_begin, __init_end);
 
 /* Checking VA memory layout alignment. */
 static inline void check_memory_layout_alignment_constraints(void) {
@@ -1122,7 +1123,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 = init_diff(__init_begin, __init_end);
     uint32_t insn;
     unsigned int i, nr = len / sizeof(insn);
     uint32_t *p;
@@ -1140,7 +1141,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",
+           init_diff(__init_begin, __init_end) >> 10);
 }
 
 void arch_dump_shared_mem_info(void)
diff --git a/xen/arch/arm/percpu.c b/xen/arch/arm/percpu.c
index 25442c4..874d52c 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(per_cpu_diff(__per_cpu_start,      \
+                                                        __per_cpu_data_end)))
 
 void __init percpu_init_areas(void)
 {
@@ -17,13 +18,13 @@ void __init percpu_init_areas(void)
 
 static int init_percpu_area(unsigned int cpu)
 {
-    char *p;
+    struct abstract_per_cpu *p;
     if ( __per_cpu_offset[cpu] != INVALID_PERCPU_AREA )
         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, per_cpu_diff(__per_cpu_start, __per_cpu_data_end));
+    __per_cpu_offset[cpu] = per_cpu_diff(__per_cpu_start, p);
     return 0;
 }
 
@@ -37,7 +38,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 *)__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 8eb0b6e..007527e 100644
--- a/xen/arch/arm/platform.c
+++ b/xen/arch/arm/platform.c
@@ -22,7 +22,8 @@
 #include <xen/init.h>
 #include <asm/psci.h>
 
-extern const struct platform_desc _splatform[], _eplatform[];
+typedef struct platform_desc platform_desc_t;
+DECLARE_BOUNDS(platform_desc, _splatform, _eplatform);
 
 /* Pointer to the current platform description */
 static const struct platform_desc *platform;
@@ -51,14 +52,16 @@ void __init platform_init(void)
     ASSERT(platform == NULL);
 
     /* Looking for the platform description */
-    for ( platform = _splatform; platform != _eplatform; platform++ )
+    for ( platform = _splatform;
+          platform_desc_diff(platform, _eplatform) != 0;
+          platform++ )
     {
         if ( platform_is_compatible(platform) )
             break;
     }
 
     /* We don't have specific operations for this platform */
-    if ( platform == _eplatform )
+    if ( platform_desc_diff(platform, _eplatform) == 0 )
     {
         /* TODO: dump DT machine compatible node */
         printk(XENLOG_INFO "Platform: Generic System\n");
diff --git a/xen/include/asm-arm/percpu.h b/xen/include/asm-arm/percpu.h
index 6263e77..3699dd3 100644
--- a/xen/include/asm-arm/percpu.h
+++ b/xen/include/asm-arm/percpu.h
@@ -3,10 +3,12 @@
 
 #ifndef __ASSEMBLY__
 
+#include <xen/lib.h>
 #include <xen/types.h>
 #include <asm/sysregs.h>
 
-extern char __per_cpu_start[], __per_cpu_data_end[];
+typedef char per_cpu_t;
+DECLARE_BOUNDS(per_cpu, __per_cpu_start, __per_cpu_data_end);
 extern unsigned long __per_cpu_offset[NR_CPUS];
 void percpu_init_areas(void);
 
-- 
1.9.1


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

[PATCH v11 5/9] xen/x86: use DECLARE_BOUNDS as required

[Stefano Stabellini]

Use DECLARE_BOUNDS and the two static inline functions that come with it
for comparisons and subtractions of:

__2M_rwdata_start, __2M_rwdata_end, __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

possible to use the provided static inline functions.
M3CM: Rule-18.2: Subtraction between pointers shall only be applied to
pointers that address elements of the same array.

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 v11:
- change p type to struct abstract_per_cpu *
- move changes to alternative.c to a new patch
- use DECLARE_BOUNDS

Changes in v10:
- use DEFINE_SYMBOL
- move changes for _start, _end, _stext, _etext, _srodata, _erodata,
  _sinittext, _einittext to a different patch

Changes in v9:
- use SYMBOLS_SUBTRACT and SYMBOLS_COMPARE
---
 xen/arch/x86/efi/efi-boot.h  | 10 ++++++----
 xen/arch/x86/percpu.c        | 11 ++++++-----
 xen/arch/x86/setup.c         |  8 ++++++--
 xen/arch/x86/smpboot.c       |  6 ++++--
 xen/drivers/vpci/vpci.c      |  6 +++---
 xen/include/asm-x86/percpu.h |  6 +++++-
 6 files changed, 30 insertions(+), 17 deletions(-)

diff --git a/xen/arch/x86/efi/efi-boot.h b/xen/arch/x86/efi/efi-boot.h
index 5789d2c..a674e98 100644
--- a/xen/arch/x86/efi/efi-boot.h
+++ b/xen/arch/x86/efi/efi-boot.h
@@ -98,8 +98,10 @@ static void __init efi_arch_relocate_image(unsigned long delta)
     }
 }
 
-extern const s32 __trampoline_rel_start[], __trampoline_rel_stop[];
-extern const s32 __trampoline_seg_start[], __trampoline_seg_stop[];
+typedef s32 trampoline_rel_t;
+DECLARE_BOUNDS(trampoline_rel, __trampoline_rel_start, __trampoline_rel_stop);
+typedef s32 trampoline_seg_t;
+DECLARE_BOUNDS(trampoline_seg, __trampoline_seg_start, __trampoline_seg_stop);
 
 static void __init relocate_trampoline(unsigned long phys)
 {
@@ -112,11 +114,11 @@ static void __init relocate_trampoline(unsigned long phys)
 
     /* Apply relocations to trampoline. */
     for ( trampoline_ptr = __trampoline_rel_start;
-          trampoline_ptr < __trampoline_rel_stop;
+          trampoline_rel_lt(trampoline_ptr, __trampoline_rel_stop);
           ++trampoline_ptr )
         *(u32 *)(*trampoline_ptr + (long)trampoline_ptr) += phys;
     for ( trampoline_ptr = __trampoline_seg_start;
-          trampoline_ptr < __trampoline_seg_stop;
+          trampoline_seg_lt(trampoline_ptr, __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..82ed5a2 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(per_cpu_diff(__per_cpu_start,     \
+                                                       __per_cpu_data_end))
 
 void __init percpu_init_areas(void)
 {
@@ -25,7 +26,7 @@ void __init percpu_init_areas(void)
 
 static int init_percpu_area(unsigned int cpu)
 {
-    char *p;
+    struct abstract_per_cpu *p;
 
     if ( __per_cpu_offset[cpu] != INVALID_PERCPU_AREA )
         return 0;
@@ -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, per_cpu_diff(__per_cpu_start, __per_cpu_data_end));
+    __per_cpu_offset[cpu] = per_cpu_lt(__per_cpu_start, p);
 
     return 0;
 }
@@ -49,7 +50,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 *)__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 06eb483..3264328 100644
--- a/xen/arch/x86/setup.c
+++ b/xen/arch/x86/setup.c
@@ -252,7 +252,9 @@ void __init discard_initial_images(void)
     initial_images = NULL;
 }
 
-extern char __init_begin[], __init_end[], __bss_start[], __bss_end[];
+typedef char init_t;
+DECLARE_BOUNDS(init, __init_begin, __init_end);
+extern char __bss_start[], __bss_end[];
 
 static void __init init_idle_domain(void)
 {
@@ -600,7 +602,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 = (char *)__init_begin;
+          init_lt(va, __init_end);
+          va += PAGE_SIZE )
         clear_page(va);
 
     /* Destroy Xen's mappings, and reuse the pages. */
diff --git a/xen/arch/x86/smpboot.c b/xen/arch/x86/smpboot.c
index 7d1226d..114c953 100644
--- a/xen/arch/x86/smpboot.c
+++ b/xen/arch/x86/smpboot.c
@@ -782,7 +782,8 @@ DEFINE_PER_CPU(root_pgentry_t *, root_pgt);
 
 static root_pgentry_t common_pgt;
 
-extern const char _stextentry[], _etextentry[];
+typedef char textentry_t;
+DECLARE_BOUNDS(textentry, _stextentry, _etextentry);
 
 static int setup_cpu_root_pgt(unsigned int cpu)
 {
@@ -811,7 +812,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 && textentry_lt(ptr, _etextentry);
+              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..a1d6817 100644
--- a/xen/drivers/vpci/vpci.c
+++ b/xen/drivers/vpci/vpci.c
@@ -31,9 +31,9 @@ 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)
+typedef vpci_register_init_t *const vpci_array_t;
+DECLARE_BOUNDS(vpci_array, __start_vpci_array, __end_vpci_array);
+#define NUM_VPCI_INIT (vpci_array_diff(__start_vpci_array, __end_vpci_array))
 
 void vpci_remove_device(struct pci_dev *pdev)
 {
diff --git a/xen/include/asm-x86/percpu.h b/xen/include/asm-x86/percpu.h
index 51562b9..67982de 100644
--- a/xen/include/asm-x86/percpu.h
+++ b/xen/include/asm-x86/percpu.h
@@ -2,7 +2,11 @@
 #define __X86_PERCPU_H__
 
 #ifndef __ASSEMBLY__
-extern char __per_cpu_start[], __per_cpu_data_end[];
+#include <xen/lib.h>
+#include <xen/types.h>
+
+typedef char per_cpu_t;
+DECLARE_BOUNDS(per_cpu, __per_cpu_start, __per_cpu_data_end);
 extern unsigned long __per_cpu_offset[NR_CPUS];
 void percpu_init_areas(void);
 #endif
-- 
1.9.1


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

[PATCH v11 6/9] xen/common: use DECLARE_BOUNDS as required

[Stefano Stabellini]

Use DECLARE_BOUNDS and the two static inline functions that come with it
for comparisons and subtractions of:

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

In the case of __initcall_start, __presmp_initcall_end, and
__initcall_end, turn the three variables into two proper ranges
introducing __presmp_initcall_start.

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.

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 v11:
- split (__initcall_start,__initcall_end) and
  (__initcall_start,__initcall_end)
- make use of elf_note_bytediff
- use DECLARE_BOUNDS

Changes in v10:
- use DEFINE_SYMBOL
- move changes for _start, _end, _stext, _etext, _srodata, _erodata,
  _sinittext, _einittext to a different patch

Changes in v9:
- use SYMBOLS_SUBTRACT and SYMBOLS_COMPARE
---
 xen/arch/arm/xen.lds.S |  3 ++-
 xen/arch/x86/xen.lds.S |  3 ++-
 xen/common/kernel.c    | 13 +++++++++----
 xen/common/lib.c       |  7 +++++--
 xen/common/schedule.c  | 11 +++++++++--
 xen/common/spinlock.c  |  8 +++++---
 xen/common/version.c   |  9 +++++----
 7 files changed, 37 insertions(+), 17 deletions(-)

diff --git a/xen/arch/arm/xen.lds.S b/xen/arch/arm/xen.lds.S
index 1e72906..7ec762e 100644
--- a/xen/arch/arm/xen.lds.S
+++ b/xen/arch/arm/xen.lds.S
@@ -155,9 +155,10 @@ SECTIONS
        *(.init.setup)
        __setup_end = .;
 
-       __initcall_start = .;
+       __presmp_initcall_start = .;
        *(.initcallpresmp.init)
        __presmp_initcall_end = .;
+       __initcall_start = .;
        *(.initcall1.init)
        __initcall_end = .;
 
diff --git a/xen/arch/x86/xen.lds.S b/xen/arch/x86/xen.lds.S
index 6e9bda5..5ec89ae 100644
--- a/xen/arch/x86/xen.lds.S
+++ b/xen/arch/x86/xen.lds.S
@@ -210,9 +210,10 @@ SECTIONS
        *(.init.setup)
        __setup_end = .;
 
-       __initcall_start = .;
+       __presmp_initcall_start = .;
        *(.initcallpresmp.init)
        __presmp_initcall_end = .;
+       __initcall_start = .;
        *(.initcall1.init)
        __initcall_end = .;
 
diff --git a/xen/common/kernel.c b/xen/common/kernel.c
index 5766a0f..1743939 100644
--- a/xen/common/kernel.c
+++ b/xen/common/kernel.c
@@ -306,20 +306,25 @@ void add_taint(unsigned int flag)
     tainted |= flag;
 }
 
-extern const initcall_t __initcall_start[], __presmp_initcall_end[],
-    __initcall_end[];
+DECLARE_ARRAY_BOUNDS(initcall);
+typedef initcall_t presmp_initcall_t;
+DECLARE_ARRAY_BOUNDS(presmp_initcall);
 
 void __init do_presmp_initcalls(void)
 {
     const initcall_t *call;
-    for ( call = __initcall_start; call < __presmp_initcall_end; call++ )
+    for ( call = __presmp_initcall_start;
+		  presmp_initcall_lt(call, __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 = __initcall_start;
+          initcall_lt(call, __initcall_end);
+          call++ )
         (*call)();
 }
 
diff --git a/xen/common/lib.c b/xen/common/lib.c
index 8ebec81..f4a2fd5 100644
--- a/xen/common/lib.c
+++ b/xen/common/lib.c
@@ -492,12 +492,15 @@ unsigned long long parse_size_and_unit(const char *s, const char **ps)
 }
 
 typedef void (*ctor_func_t)(void);
-extern const ctor_func_t __ctors_start[], __ctors_end[];
+DECLARE_BOUNDS(ctor_func, __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;
+          ctor_func_lt(f, __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..30e9b4a 100644
--- a/xen/common/schedule.c
+++ b/xen/common/schedule.c
@@ -67,8 +67,15 @@ 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)
+/*
+ * Cannot use typedef because it would make the schedulers array
+ * const: the pointers become const, instead of the pointed
+ * struct schedulers.
+ */
+#define schedulers_t struct scheduler*
+DECLARE_BOUNDS(schedulers, __start_schedulers_array, __end_schedulers_array);
+#define NUM_SCHEDULERS (schedulers_diff(__start_schedulers_array, \
+                                        __end_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..20439f7 100644
--- a/xen/common/spinlock.c
+++ b/xen/common/spinlock.c
@@ -318,8 +318,8 @@ struct lock_profile_anc {
 typedef void lock_profile_subfunc(
     struct lock_profile *, int32_t, int32_t, void *);
 
-extern struct lock_profile *__lock_profile_start;
-extern struct lock_profile *__lock_profile_end;
+typedef struct lock_profile* lock_profile_t;
+DECLARE_BOUNDS(lock_profile, __lock_profile_start, __lock_profile_end);
 
 static s_time_t lock_profile_start;
 static struct lock_profile_anc lock_profile_ancs[LOCKPROF_TYPE_N];
@@ -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;
+          lock_profile_lt(q, &__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..b4abf69 100644
--- a/xen/common/version.c
+++ b/xen/common/version.c
@@ -86,7 +86,8 @@ int xen_build_id(const void **p, unsigned int *len)
 
 #ifdef BUILD_ID
 /* Defined in linker script. */
-extern const Elf_Note __note_gnu_build_id_start[], __note_gnu_build_id_end[];
+typedef Elf_Note elf_note_t;
+DECLARE_BOUNDS(elf_note, __note_gnu_build_id_start, __note_gnu_build_id_end);
 
 int xen_build_id_check(const Elf_Note *n, unsigned int n_sz,
                        const void **p, unsigned int *len)
@@ -147,14 +148,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 ( !elf_note_lt(&n[0], __note_gnu_build_id_end) )
         return -ENODATA;
 
     /* Check for full Note header. */
-    if ( &n[1] >= __note_gnu_build_id_end )
+    if ( !elf_note_lt(&n[1], __note_gnu_build_id_end) )
         return -ENODATA;
 
-    sz = (void *)__note_gnu_build_id_end - (void *)n;
+    sz = elf_note_bytediff(n, __note_gnu_build_id_end);
 
     rc = xen_build_id_check(n, sz, &build_id_p, &build_id_len);
 
-- 
1.9.1


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

[PATCH v11 7/9] xen: use DECLARE_BOUNDS as required

[Stefano Stabellini]

Use DECLARE_BOUNDS and the two static inline functions that come with it
for comparisons and subtractions of:

_start, _end, _stext, _etext, _srodata, _erodata, _sinittext,
_einittext

Use explicit casts to uintptr_t when it is not possible to use the
provided static inline functions.

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.

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 v11:
- fix bug: a comma added by mistake
- use DECLARE_BOUNDS

Changes in v10:
- new patch
---
 xen/arch/arm/alternative.c        |  4 ++--
 xen/arch/arm/arm32/livepatch.c    |  2 +-
 xen/arch/arm/arm64/livepatch.c    |  2 +-
 xen/arch/arm/livepatch.c          |  4 ++--
 xen/arch/arm/mm.c                 |  6 +++---
 xen/arch/arm/setup.c              |  5 +++--
 xen/arch/x86/setup.c              |  3 ++-
 xen/include/asm-arm/grant_table.h |  3 ++-
 xen/include/xen/kernel.h          | 42 ++++++++++++++++++++++++---------------
 9 files changed, 42 insertions(+), 29 deletions(-)

diff --git a/xen/arch/arm/alternative.c b/xen/arch/arm/alternative.c
index 52ed7ed..6013110 100644
--- a/xen/arch/arm/alternative.c
+++ b/xen/arch/arm/alternative.c
@@ -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 = xen_diff(_start, _end);
         unsigned int xen_order = get_order_from_bytes(xen_size);
         void *xenmap;
 
@@ -206,7 +206,7 @@ static int __apply_alternatives_multi_stop(void *unused)
         region.begin = __alt_instructions;
         region.end = __alt_instructions_end;
 
-        ret = __apply_alternatives(&region, xenmap - (void *)_start);
+        ret = __apply_alternatives(&region, xen_diff(_start, xenmap));
         /* 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..83852d3 100644
--- a/xen/arch/arm/arm32/livepatch.c
+++ b/xen/arch/arm/arm32/livepatch.c
@@ -56,7 +56,7 @@ void arch_livepatch_apply(struct livepatch_func *func)
     else
         insn = 0xe1a00000; /* mov r0, r0 */
 
-    new_ptr = func->old_addr - (void *)_start + vmap_of_xen_text;
+    new_ptr = xen_diff(_start, func->old_addr) + 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..c63a0dd 100644
--- a/xen/arch/arm/arm64/livepatch.c
+++ b/xen/arch/arm/arm64/livepatch.c
@@ -43,7 +43,7 @@ void arch_livepatch_apply(struct livepatch_func *func)
     /* Verified in livepatch_verify_distance. */
     ASSERT(insn != AARCH64_BREAK_FAULT);
 
-    new_ptr = func->old_addr - (void *)_start + vmap_of_xen_text;
+    new_ptr = xen_diff(_start, func->old_addr) + vmap_of_xen_text;
     len = len / sizeof(uint32_t);
 
     /* PATCH! */
diff --git a/xen/arch/arm/livepatch.c b/xen/arch/arm/livepatch.c
index 279d52c..1ba068c 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(xen_diff(_start, _end));
 
     /*
      * The text section is read-only. So re-map Xen to be able to patch
@@ -78,7 +78,7 @@ void arch_livepatch_revert(const struct livepatch_func *func)
     uint32_t *new_ptr;
     unsigned int len;
 
-    new_ptr = func->old_addr - (void *)_start + vmap_of_xen_text;
+    new_ptr = xen_diff(_start, func->old_addr) + 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 df52b26..ed16824 100644
--- a/xen/arch/arm/mm.c
+++ b/xen/arch/arm/mm.c
@@ -1074,7 +1074,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 void *p, unsigned long l, enum mg mg)
 {
     lpae_t pte;
     int i;
@@ -1085,8 +1085,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 = xen_diff(_start, p) / PAGE_SIZE;
+          i < (xen_diff(_start, p) + l) / PAGE_SIZE;
           i++ )
     {
         pte = xen_xenmap[i];
diff --git a/xen/arch/arm/setup.c b/xen/arch/arm/setup.c
index 444857a..8d43943 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)(xen_diff(_start, _end) + 1),
+                             false);
     BUG_ON(!xen_bootmodule);
 
     setup_pagetables(boot_phys_offset);
diff --git a/xen/arch/x86/setup.c b/xen/arch/x86/setup.c
index 3264328..2ac7f62 100644
--- a/xen/arch/x86/setup.c
+++ b/xen/arch/x86/setup.c
@@ -1071,7 +1071,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,
+                        xen_diff(_start, _end), 1);
 
             /* Walk initial pagetables, relocating page directory entries. */
             pl4e = __va(__pa(idle_pg_table));
diff --git a/xen/include/asm-arm/grant_table.h b/xen/include/asm-arm/grant_table.h
index 816e3c6..1097791 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(text_diff(_stext, _etext)))
 
 #define gnttab_init_arch(gt)                                             \
 ({                                                                       \
diff --git a/xen/include/xen/kernel.h b/xen/include/xen/kernel.h
index 548b64d..c105a2a 100644
--- a/xen/include/xen/kernel.h
+++ b/xen/include/xen/kernel.h
@@ -5,6 +5,7 @@
  * 'kernel.h' contains some often-used function prototypes etc
  */
 
+#include <xen/lib.h>
 #include <xen/types.h>
 
 /*
@@ -65,28 +66,37 @@
 	1;                                      \
 })
 
-extern char _start[], _end[], start[];
-#define is_kernel(p) ({                         \
-    char *__p = (char *)(unsigned long)(p);     \
-    (__p >= _start) && (__p < _end);            \
+extern char start[];
+typedef char xen_t;
+DECLARE_BOUNDS(xen, _start, _end);
+#define is_kernel(p) ({                                             \
+    const char *p__ = (const char *)(unsigned long)(p);             \
+    ((uintptr_t)p__ >= (uintptr_t)_start &&                         \
+    xen_lt(p__, _end));                                             \
 })
 
-extern char _stext[], _etext[];
-#define is_kernel_text(p) ({                    \
-    char *__p = (char *)(unsigned long)(p);     \
-    (__p >= _stext) && (__p < _etext);          \
+typedef char text_t;
+DECLARE_BOUNDS(text, _stext, _etext);
+#define is_kernel_text(p) ({                                        \
+    const char *p__ = (const char *)(unsigned long)(p);             \
+    ((uintptr_t)p__ >= (uintptr_t) _stext &&                        \
+    text_lt(p__, _etext));                                          \
 })
 
-extern const char _srodata[], _erodata[];
-#define is_kernel_rodata(p) ({                  \
-    const char *__p = (const char *)(unsigned long)(p);     \
-    (__p >= _srodata) && (__p < _erodata);      \
+typedef char rodata_t;
+DECLARE_BOUNDS(rodata, _srodata, _erodata);
+#define is_kernel_rodata(p) ({                                      \
+    const char *p__ = (const char *)(unsigned long)(p);             \
+    ((uintptr_t)p__ >= (uintptr_t)_srodata &&                       \
+    rodata_lt(p__, _erodata));                                      \
 })
 
-extern char _sinittext[], _einittext[];
-#define is_kernel_inittext(p) ({                \
-    char *__p = (char *)(unsigned long)(p);     \
-    (__p >= _sinittext) && (__p < _einittext);  \
+typedef char inittext_t;
+DECLARE_BOUNDS(inittext, _sinittext, _einittext);
+#define is_kernel_inittext(p) ({                                    \
+    const char *p__ = (const char *)(unsigned long)(p);             \
+    ((uintptr_t)p__ >= (uintptr_t) _sinittext &&                    \
+    inittext_lt(p__, _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

[PATCH v11 8/9] xen: use DECLARE_BOUNDS in alternative.c

[Stefano Stabellini]

Use DECLARE_BOUNDS and the two static inline functions that come with it
for comparisons and subtractions of:

__alt_instructions, __alt_instructions_end

M3CM: Rule-18.2: Subtraction between pointers shall only be applied to
pointers that address elements of the same array

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 v11:
- new patch
---
 xen/arch/arm/alternative.c        | 12 +++++++-----
 xen/arch/x86/alternative.c        | 10 +++++-----
 xen/common/livepatch.c            |  5 +++--
 xen/include/asm-arm/alternative.h |  6 +++++-
 xen/include/asm-x86/alternative.h |  7 ++++++-
 5 files changed, 26 insertions(+), 14 deletions(-)

diff --git a/xen/arch/arm/alternative.c b/xen/arch/arm/alternative.c
index 6013110..32a03d6 100644
--- a/xen/arch/arm/alternative.c
+++ b/xen/arch/arm/alternative.c
@@ -38,11 +38,9 @@
 #undef virt_to_mfn
 #define virt_to_mfn(va) _mfn(__virt_to_mfn(va))
 
-extern const struct alt_instr __alt_instructions[], __alt_instructions_end[];
-
 struct alt_region {
     const struct alt_instr *begin;
-    const struct alt_instr *end;
+    const struct abstract_alt_instr *end;
 };
 
 /*
@@ -131,7 +129,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;
+          alt_instr_lt(alt, region->end);
+          alt++ )
     {
         int nr_inst;
 
@@ -236,7 +237,8 @@ void __init apply_alternatives_all(void)
     BUG_ON(ret);
 }
 
-int apply_alternatives(const struct alt_instr *start, const struct alt_instr *end)
+int apply_alternatives(const struct alt_instr *start,
+                       const struct abstract_alt_instr *end)
 {
     const struct alt_region region = {
         .begin = start,
diff --git a/xen/arch/x86/alternative.c b/xen/arch/x86/alternative.c
index b8c819a..30abf4e 100644
--- a/xen/arch/x86/alternative.c
+++ b/xen/arch/x86/alternative.c
@@ -29,8 +29,6 @@
 
 #define MAX_PATCH_LEN (255-1)
 
-extern struct alt_instr __alt_instructions[], __alt_instructions_end[];
-
 #ifdef K8_NOP1
 static const unsigned char k8nops[] init_or_livepatch_const = {
     K8_NOP1,
@@ -178,8 +176,8 @@ text_poke(void *addr, const void *opcode, size_t len)
  * APs have less capabilities than the boot processor are not handled.
  * Tough. Make sure you disable such features by hand.
  */
-void init_or_livepatch apply_alternatives(struct alt_instr *start,
-                                          struct alt_instr *end)
+void init_or_livepatch apply_alternatives(const struct alt_instr *start,
+                                          const struct abstract_alt_instr *end)
 {
     struct alt_instr *a, *base;
 
@@ -193,8 +191,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 = (struct alt_instr *)start; alt_instr_lt(a, end); a++ )
     {
         uint8_t *orig = ALT_ORIG_PTR(a);
         uint8_t *repl = ALT_REPL_PTR(a);
diff --git a/xen/common/livepatch.c b/xen/common/livepatch.c
index d6eaae6..e02f95d 100644
--- a/xen/common/livepatch.c
+++ b/xen/common/livepatch.c
@@ -661,7 +661,8 @@ static int prepare_payload(struct payload *payload,
     if ( sec )
     {
 #ifdef CONFIG_HAS_ALTERNATIVE
-        struct alt_instr *a, *start, *end;
+        struct alt_instr *a, *start;
+        struct abstract_alt_instr *end;
 
         if ( !section_ok(elf, sec, sizeof(*a)) )
             return -EINVAL;
@@ -669,7 +670,7 @@ static int prepare_payload(struct payload *payload,
         start = sec->load_addr;
         end = sec->load_addr + sec->sec->sh_size;
 
-        for ( a = start; a < end; a++ )
+        for ( a = start; alt_instr_lt(a, end); a++ )
         {
             const void *instr = ALT_ORIG_PTR(a);
             const void *replacement = ALT_REPL_PTR(a);
diff --git a/xen/include/asm-arm/alternative.h b/xen/include/asm-arm/alternative.h
index dedb6dd..68f848e 100644
--- a/xen/include/asm-arm/alternative.h
+++ b/xen/include/asm-arm/alternative.h
@@ -7,6 +7,7 @@
 
 #ifndef __ASSEMBLY__
 
+#include <xen/lib.h>
 #include <xen/types.h>
 #include <xen/stringify.h>
 
@@ -28,7 +29,10 @@ typedef void (*alternative_cb_t)(const struct alt_instr *alt,
 				 int nr_inst);
 
 void apply_alternatives_all(void);
-int apply_alternatives(const struct alt_instr *start, const struct alt_instr *end);
+typedef struct alt_instr alt_instr_t;
+DECLARE_BOUNDS(alt_instr, __alt_instructions, __alt_instructions_end);
+int apply_alternatives(const struct alt_instr *start,
+		               const struct abstract_alt_instr *end);
 
 #define ALTINSTR_ENTRY(feature, cb)					      \
 	" .word 661b - .\n"				/* label           */ \
diff --git a/xen/include/asm-x86/alternative.h b/xen/include/asm-x86/alternative.h
index d96411f..c0d65ab 100644
--- a/xen/include/asm-x86/alternative.h
+++ b/xen/include/asm-x86/alternative.h
@@ -4,6 +4,7 @@
 #ifdef __ASSEMBLY__
 #include <asm/alternative-asm.h>
 #else
+#include <xen/lib.h>
 #include <xen/stringify.h>
 #include <xen/types.h>
 #include <asm/asm-macros.h>
@@ -23,8 +24,12 @@ struct __packed alt_instr {
 #define ALT_REPL_PTR(a)     __ALT_PTR(a, repl_offset)
 
 extern void add_nops(void *insns, unsigned int len);
+
+typedef struct alt_instr alt_instr_t;
+DECLARE_BOUNDS(alt_instr, __alt_instructions, __alt_instructions_end);
 /* Similar to alternative_instructions except it can be run with IRQs enabled. */
-extern void apply_alternatives(struct alt_instr *start, struct alt_instr *end);
+void apply_alternatives(const struct alt_instr *start,
+	 	                const struct abstract_alt_instr *end);
 extern void alternative_instructions(void);
 
 #define alt_orig_len       "(.LXEN%=_orig_e - .LXEN%=_orig_s)"
-- 
1.9.1


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

[PATCH v11 9/9] xen: explicit casts when DECLARE_BOUNDS cannot be used

[Stefano Stabellini]

Sometimes the static inline functions provided by DECLARE_BOUNDS cannot
be used. This patch uses explicit casts to uintptr_t in those cases.

M3CM: Rule-18.2: Subtraction between pointers shall only be applied to
pointers that address elements of the same array

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
---
 xen/arch/x86/setup.c        | 3 ++-
 xen/common/virtual_region.c | 4 +++-
 2 files changed, 5 insertions(+), 2 deletions(-)

diff --git a/xen/arch/x86/setup.c b/xen/arch/x86/setup.c
index 2ac7f62..cb45b68 100644
--- a/xen/arch/x86/setup.c
+++ b/xen/arch/x86/setup.c
@@ -976,7 +976,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 = (uintptr_t)__2M_rwdata_end -
+                                       (uintptr_t) _stext;
     }
 
     modules_headroom = bzimage_headroom(bootstrap_map(mod), mod->mod_end);
diff --git a/xen/common/virtual_region.c b/xen/common/virtual_region.c
index aa23918..87ef33a 100644
--- a/xen/common/virtual_region.c
+++ b/xen/common/virtual_region.c
@@ -119,7 +119,9 @@ 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 = ((uintptr_t)bug_frames[i] - (uintptr_t)s) /
+             sizeof(struct bug_frame);
 
         core.frame[i - 1].n_bugs = sz;
         core.frame[i - 1].bugs = s;
-- 
1.9.1


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

Re: [PATCH v11 1/9] xen: use __UINTPTR_TYPE__ for uintptr_t

[Jan Beulich]

>>> On 05.03.19 at 23:38, <sstabellini@kernel.org> wrote:
> Use __UINTPTR_TYPE__ to define uintptr_t. A later patch will make use of
> __PTRDIFF_TYPE__ to define ptrdiff_t.
> 
> Signed-off-by: Stefano Stabellini <stefanos@xilinx.com>
> Reviewed-by: Ian Jackson <ian.jackson@eu.citrix.com>

As before - I object to this change without the description supplying
both a reason (which would better also explain why the current way
of defining uintptr_t is detrimental) and a discussion why it is okay for
us to use __UINTPTR_TYPE__, despite (at least) gcc making this
available only under certain conditions (i.e. it would need to be
confirmed that whatever the conditions they're always met for us).

With this I think it is pretty clear that you would better have split
the original patch the other way around - the uncontroversial one
first, as that one now wouldn't apply without the one here.

Jan



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

Re: [PATCH v11 3/9] xen: introduce DECLARE_BOUNDS

[Jan Beulich]

>>> On 05.03.19 at 23:38, <sstabellini@kernel.org> wrote:
> --- a/xen/include/xen/lib.h
> +++ b/xen/include/xen/lib.h
> @@ -173,4 +173,92 @@ void init_constructors(void);
>  void *bsearch(const void *key, const void *base, size_t num, size_t size,
>                int (*cmp)(const void *key, const void *elt));
>  
> + /*
> +  * Declare start and end array variables in C corresponding to existing
> +  * linker symbols.
> +  *
> +  * These macros, or an alternative technique, MUST be used any time
> +  * linker symbols are imported into C via the `extern []' idiom.
> +  *
> +  *    __DECLARE_BOUNDS(TYPE, START, START, END)

START used twice here makes it ambiguous which one is which in the
subsequent text.

> +  *  introduces the following two constant expressions
> +  *
> +  *    const TYPE *START;
> +  *    const struct abstract_NAME *END;

For one these are declarations, not (constant) expressions. And
then the declarations produce array types, not pointer types.
Please let's not have a comment which is out of sync with what
it describes from the very beginning.

> +  *  whose values are the linker symbols START and END; these
> +  *  should be the start and end of a memory region.
> +  *
> +  *  You may then use these two inline functions:
> +  *
> +  *    bool NAME_lt(const TYPE *s1, const struct abstract_NAME *s2);
> +  *    ptrdiff_t NAME_diff(const TYPE *s1, const struct abstract_NAME *s2);
> +  *
> +  *  lt returns true iff s1 < s2.
> +  *  diff returns the s2-s1 in units of TYPE.
> +  *
> +  *

Stray double blank comment lines and no mention of _bytediff.

> +static inline ptrdiff_t name ## _diff(const type s1[],                        \
> +                                      const struct abstract_ ## name s2[])    \
> +{                                                                             \
> +    BUILD_BUG_ON(alignof(*s1) != alignof(*s2));                               \
> +    return (ptrdiff_t)((uintptr_t)s2 - (uintptr_t)s1) /                       \
> +           (ptrdiff_t)sizeof(*s1);                                             \
> +}                                                                           

I had specifically asked for this to simply call _bytediff, to limit
redundancy and in particular the total number of casts.

> +static inline ptrdiff_t name ## _bytediff(const type s1[],                    \
> +                                          const struct abstract_ ## name s2[])\
> +{                                                                             \
> +    BUILD_BUG_ON(alignof(*s1) != alignof(*s2));                               \
> +    return (ptrdiff_t)((uintptr_t)s2 - (uintptr_t)s1);                        \
> +}

What's the value of the intermediate casting to uintptr_t? Why not
cast to ptrdiff_t right away?

I also don't think the BUILD_BUG_ON() is helpful in this latter case.

Jan



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

Re: [PATCH v11 5/9] xen/x86: use DECLARE_BOUNDS as required

[Jan Beulich]

>>> On 05.03.19 at 23:38, <sstabellini@kernel.org> wrote:
> --- 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(per_cpu_diff(__per_cpu_start,     \
> +                                                       __per_cpu_data_end))

Please use _bytediff() when bytes are meant (i.e. also below, and
perhaps elsewhere).

> @@ -600,7 +602,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 = (char *)__init_begin;
> +          init_lt(va, __init_end);
> +          va += PAGE_SIZE )

Is the line wrapping really needed here?

> --- a/xen/drivers/vpci/vpci.c
> +++ b/xen/drivers/vpci/vpci.c
> @@ -31,9 +31,9 @@ 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)
> +typedef vpci_register_init_t *const vpci_array_t;

You don't want to keep the const here - DECLARE_BOUNDS() will
suitably add it.

Also how about vcpi_init_t or vpci_reg_init_t or some such? The
defined type is not really an array after all.

> +DECLARE_BOUNDS(vpci_array, __start_vpci_array, __end_vpci_array);
> +#define NUM_VPCI_INIT (vpci_array_diff(__start_vpci_array, __end_vpci_array))

Unnecessary outermost parentheses.

Jan



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

Re: [PATCH v11 6/9] xen/common: use DECLARE_BOUNDS as required

[Jan Beulich]

>>> On 05.03.19 at 23:38, <sstabellini@kernel.org> wrote:
> --- a/xen/common/kernel.c
> +++ b/xen/common/kernel.c
> @@ -306,20 +306,25 @@ void add_taint(unsigned int flag)
>      tainted |= flag;
>  }
>  
> -extern const initcall_t __initcall_start[], __presmp_initcall_end[],
> -    __initcall_end[];
> +DECLARE_ARRAY_BOUNDS(initcall);
> +typedef initcall_t presmp_initcall_t;
> +DECLARE_ARRAY_BOUNDS(presmp_initcall);
>  
>  void __init do_presmp_initcalls(void)
>  {
>      const initcall_t *call;
> -    for ( call = __initcall_start; call < __presmp_initcall_end; call++ )
> +    for ( call = __presmp_initcall_start;
> +		  presmp_initcall_lt(call, __presmp_initcall_end);
> +		  call++ )

Hard tabs slipped in. Also would you mind adding the missing blank line
ahead of the one you modify?

>          (*call)();
>  }
>  
>  void __init do_initcalls(void)
>  {
>      const initcall_t *call;
> -    for ( call = __presmp_initcall_end; call < __initcall_end; call++ )
> +    for ( call = __initcall_start;
> +          initcall_lt(call, __initcall_end);
> +          call++ )

Again no need for splitting the line as it seems.

Jan



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

Re: [PATCH v11 5/9] xen/x86: use DECLARE_BOUNDS as required

[Jan Beulich]

>>> On 05.03.19 at 23:38, <sstabellini@kernel.org> wrote:
> @@ -600,7 +602,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 = (char *)__init_begin;
> +          init_lt(va, __init_end);
> +          va += PAGE_SIZE )
>          clear_page(va);

At the example of this - is casting away of const-ness not also a
potential certification hindrance?

Jan



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

Re: [PATCH v11 8/9] xen: use DECLARE_BOUNDS in alternative.c

[Jan Beulich]

>>> On 05.03.19 at 23:38, <sstabellini@kernel.org> wrote:
> @@ -193,8 +191,10 @@ void init_or_livepatch apply_alternatives(struct alt_instr *start,

Seeing this relevant part of the function parameters, ...

>       *
>       * 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 = (struct alt_instr *)start; alt_instr_lt(a, end); a++ )

... why the cast?

Jan



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

Re: [PATCH v11 3/9] xen: introduce DECLARE_BOUNDS

[Stefano Stabellini]

On Wed, 6 Mar 2019, Jan Beulich wrote:
> > +static inline ptrdiff_t name ## _diff(const type s1[],                        \
> > +                                      const struct abstract_ ## name s2[])    \
> > +{                                                                             \
> > +    BUILD_BUG_ON(alignof(*s1) != alignof(*s2));                               \
> > +    return (ptrdiff_t)((uintptr_t)s2 - (uintptr_t)s1) /                       \
> > +           (ptrdiff_t)sizeof(*s1);                                             \
> > +}                                                                           
> 
> I had specifically asked for this to simply call _bytediff, to limit
> redundancy and in particular the total number of casts.
> 
> > +static inline ptrdiff_t name ## _bytediff(const type s1[],                    \
> > +                                          const struct abstract_ ## name s2[])\
> > +{                                                                             \
> > +    BUILD_BUG_ON(alignof(*s1) != alignof(*s2));                               \
> > +    return (ptrdiff_t)((uintptr_t)s2 - (uintptr_t)s1);                        \
> > +}
> 
> What's the value of the intermediate casting to uintptr_t? Why not
> cast to ptrdiff_t right away?

uintptr_t is the integer type corresponding to a pointer, so we should
use uintptr_t first. ptrdiff_t is the difference type so we should cast
to it afterwards. Specifically, uintptr_t is unsigned and ptrdiff_t is
signed. So I don't think it would be correct to do:

  return (ptrdiff_t)((ptrdiff_t)s2 - (ptrdiff_t)s1);

Or am I missing your point?


I'll address all the other comments.


> I also don't think the BUILD_BUG_ON() is helpful in this latter case.

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

Re: [PATCH v11 5/9] xen/x86: use DECLARE_BOUNDS as required

[Stefano Stabellini]

On Wed, 6 Mar 2019, Jan Beulich wrote:
> >>> On 05.03.19 at 23:38, <sstabellini@kernel.org> wrote:
> > --- 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(per_cpu_diff(__per_cpu_start,     \
> > +                                                       __per_cpu_data_end))
> 
> Please use _bytediff() when bytes are meant (i.e. also below, and
> perhaps elsewhere).

OK


> > @@ -600,7 +602,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 = (char *)__init_begin;
> > +          init_lt(va, __init_end);
> > +          va += PAGE_SIZE )
> 
> Is the line wrapping really needed here?

It would end at 80 characters exactly otherwise. I am happy to do as you
prefer.


> > --- a/xen/drivers/vpci/vpci.c
> > +++ b/xen/drivers/vpci/vpci.c
> > @@ -31,9 +31,9 @@ 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)
> > +typedef vpci_register_init_t *const vpci_array_t;
> 
> You don't want to keep the const here - DECLARE_BOUNDS() will
> suitably add it.

OK


> Also how about vcpi_init_t or vpci_reg_init_t or some such? The
> defined type is not really an array after all.

OK


> > +DECLARE_BOUNDS(vpci_array, __start_vpci_array, __end_vpci_array);
> > +#define NUM_VPCI_INIT (vpci_array_diff(__start_vpci_array, __end_vpci_array))
> 
> Unnecessary outermost parentheses.

OK

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

Re: [PATCH v11 6/9] xen/common: use DECLARE_BOUNDS as required

[Stefano Stabellini]

On Wed, 6 Mar 2019, Jan Beulich wrote:
> >>> On 05.03.19 at 23:38, <sstabellini@kernel.org> wrote:
> > --- a/xen/common/kernel.c
> > +++ b/xen/common/kernel.c
> > @@ -306,20 +306,25 @@ void add_taint(unsigned int flag)
> >      tainted |= flag;
> >  }
> >  
> > -extern const initcall_t __initcall_start[], __presmp_initcall_end[],
> > -    __initcall_end[];
> > +DECLARE_ARRAY_BOUNDS(initcall);
> > +typedef initcall_t presmp_initcall_t;
> > +DECLARE_ARRAY_BOUNDS(presmp_initcall);
> >  
> >  void __init do_presmp_initcalls(void)
> >  {
> >      const initcall_t *call;
> > -    for ( call = __initcall_start; call < __presmp_initcall_end; call++ )
> > +    for ( call = __presmp_initcall_start;
> > +		  presmp_initcall_lt(call, __presmp_initcall_end);
> > +		  call++ )
> 
> Hard tabs slipped in. Also would you mind adding the missing blank line
> ahead of the one you modify?

Argh! I'll do.


> >          (*call)();
> >  }
> >  
> >  void __init do_initcalls(void)
> >  {
> >      const initcall_t *call;
> > -    for ( call = __presmp_initcall_end; call < __initcall_end; call++ )
> > +    for ( call = __initcall_start;
> > +          initcall_lt(call, __initcall_end);
> > +          call++ )
> 
> Again no need for splitting the line as it seems.

Actually it was 79 in the other case and 78 here, so I'll fix both.

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

Re: [PATCH v11 1/9] xen: use __UINTPTR_TYPE__ for uintptr_t

[Stefano Stabellini]

On Wed, 6 Mar 2019, Jan Beulich wrote:
> >>> On 05.03.19 at 23:38, <sstabellini@kernel.org> wrote:
> > Use __UINTPTR_TYPE__ to define uintptr_t. A later patch will make use of
> > __PTRDIFF_TYPE__ to define ptrdiff_t.
> > 
> > Signed-off-by: Stefano Stabellini <stefanos@xilinx.com>
> > Reviewed-by: Ian Jackson <ian.jackson@eu.citrix.com>
> 
> As before - I object to this change without the description supplying
> both a reason (which would better also explain why the current way
> of defining uintptr_t is detrimental) and a discussion why it is okay for
> us to use __UINTPTR_TYPE__, despite (at least) gcc making this
> available only under certain conditions (i.e. it would need to be
> confirmed that whatever the conditions they're always met for us).

Is the following good enough:

Use __UINTPTR_TYPE__ to define uintptr_t for consistency with ptrdiff_t.
__UINTPTR_TYPE__ is safe to use as it is a common predefined macro of
GNU C; it is defined to the correct underlying type as per GCC
documentation[1].

[1] https://gcc.gnu.org/onlinedocs/cpp/Common-Predefined-Macros.html

Also, it is not a good idea to use __mode__(__pointer__) to define
uintptr_t, because it relies on an obscurely defined GCC feature whose
semantics might be taken to imply that the things are really pointers.


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

Re: [PATCH v11 5/9] xen/x86: use DECLARE_BOUNDS as required

[Stefano Stabellini]

On Wed, 6 Mar 2019, Jan Beulich wrote:
> >>> On 05.03.19 at 23:38, <sstabellini@kernel.org> wrote:
> > @@ -600,7 +602,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 = (char *)__init_begin;
> > +          init_lt(va, __init_end);
> > +          va += PAGE_SIZE )
> >          clear_page(va);
> 
> At the example of this - is casting away of const-ness not also a
> potential certification hindrance?

Darn, well spotted! No, it is not permitted (Rule 11.8).
In this case const is not correct because we actually modify the object
few lines below:

  base->priv = 1;


This is problematic. We have also the following instances in this series
to deal with:

xen/arch/arm/percpu.c:_free_percpu_area
  char *p = (char *)__per_cpu_start + __per_cpu_offset[cpu];

xen/arch/x86/percpu.c:_free_percpu_area
  char *p = (char *)__per_cpu_start + __per_cpu_offset[cpu];

xen/arch/x86/setup.c:init_done
  for ( va = (char *)__init_begin; init_lt(va, __init_end); va += PAGE_SIZE )

xen/arch/x86/alternative.c:apply_alternatives
  for ( a = base = (struct alt_instr *)start; alt_instr_lt(a, end); a++ )


In all these cases we actually end up modifying the object. I suggest
we remove the const from either __DECLARE_BOUNDS (so from everywhere),
or just for per_cpu, init, and alt_instr by introducing another MACRO.

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

Re: [PATCH v11 8/9] xen: use DECLARE_BOUNDS in alternative.c

[Stefano Stabellini]

On Wed, 6 Mar 2019, Jan Beulich wrote:
> >>> On 05.03.19 at 23:38, <sstabellini@kernel.org> wrote:
> > @@ -193,8 +191,10 @@ void init_or_livepatch apply_alternatives(struct alt_instr *start,
> 
> Seeing this relevant part of the function parameters, ...
> 
> >       *
> >       * 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 = (struct alt_instr *)start; alt_instr_lt(a, end); a++ )
> 
> ... why the cast?

To remove const (see the other email about the problematic sites).

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

Re: [PATCH v11 1/9] xen: use __UINTPTR_TYPE__ for uintptr_t

[Jan Beulich]

>>> On 06.03.19 at 22:16, <sstabellini@kernel.org> wrote:
> On Wed, 6 Mar 2019, Jan Beulich wrote:
>> >>> On 05.03.19 at 23:38, <sstabellini@kernel.org> wrote:
>> > Use __UINTPTR_TYPE__ to define uintptr_t. A later patch will make use of
>> > __PTRDIFF_TYPE__ to define ptrdiff_t.
>> > 
>> > Signed-off-by: Stefano Stabellini <stefanos@xilinx.com>
>> > Reviewed-by: Ian Jackson <ian.jackson@eu.citrix.com>
>> 
>> As before - I object to this change without the description supplying
>> both a reason (which would better also explain why the current way
>> of defining uintptr_t is detrimental) and a discussion why it is okay for
>> us to use __UINTPTR_TYPE__, despite (at least) gcc making this
>> available only under certain conditions (i.e. it would need to be
>> confirmed that whatever the conditions they're always met for us).
> 
> Is the following good enough:

I'm afraid not, as I can't bring this ...

> Use __UINTPTR_TYPE__ to define uintptr_t for consistency with ptrdiff_t.
> __UINTPTR_TYPE__ is safe to use as it is a common predefined macro of
> GNU C; it is defined to the correct underlying type as per GCC
> documentation[1].

... in line with

void
c_stddef_cpp_builtins(void)
{
  builtin_define_with_value ("__SIZE_TYPE__", SIZE_TYPE, 0);
  builtin_define_with_value ("__PTRDIFF_TYPE__", PTRDIFF_TYPE, 0);
[...]
  if (INTPTR_TYPE)
    builtin_define_with_value ("__INTPTR_TYPE__", INTPTR_TYPE, 0);
  if (UINTPTR_TYPE)
    builtin_define_with_value ("__UINTPTR_TYPE__", UINTPTR_TYPE, 0);
}

> [1] https://gcc.gnu.org/onlinedocs/cpp/Common-Predefined-Macros.html 

Note that this also says "Some of these macros may not be defined on
particular systems if GCC does not provide a ‘stdint.h’ header on those
systems." I have absolutely no idea under what conditions gcc may not
(want to) provide stdint.h.

> Also, it is not a good idea to use __mode__(__pointer__) to define
> uintptr_t, because it relies on an obscurely defined GCC feature whose
> semantics might be taken to imply that the things are really pointers.

"Obscurely defined" is rather subjective, I'd say. The "pointer" mode
is precisely for this according to my understanding, and personally I find
"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" sufficiently clear, and not leaving room for any
(hidden) interpretation like "to imply that the things are really pointers".

Jan


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

Re: [PATCH v11 3/9] xen: introduce DECLARE_BOUNDS

[Jan Beulich]

>>> On 06.03.19 at 21:55, <sstabellini@kernel.org> wrote:
> On Wed, 6 Mar 2019, Jan Beulich wrote:
>> > +static inline ptrdiff_t name ## _bytediff(const type s1[],                    \
>> > +                                          const struct abstract_ ## name s2[])\
>> > +{                                                                             \
>> > +    BUILD_BUG_ON(alignof(*s1) != alignof(*s2));                               \
>> > +    return (ptrdiff_t)((uintptr_t)s2 - (uintptr_t)s1);                        \
>> > +}
>> 
>> What's the value of the intermediate casting to uintptr_t? Why not
>> cast to ptrdiff_t right away?
> 
> uintptr_t is the integer type corresponding to a pointer, so we should
> use uintptr_t first. ptrdiff_t is the difference type so we should cast
> to it afterwards. Specifically, uintptr_t is unsigned and ptrdiff_t is
> signed. So I don't think it would be correct to do:
> 
>   return (ptrdiff_t)((ptrdiff_t)s2 - (ptrdiff_t)s1);
> 
> Or am I missing your point?

Well, I really mean

   return (ptrdiff_t)s2 - (ptrdiff_t)s1;

But that aside - let's consider all three cases:

1) sizeof(ptrdiff_t) == sizeof(void *)

All fine. And you'll have some difficulty finding a platform where this
isn't the case.

2) sizeof(ptrdiff_t) > sizeof(void *)

Still all fine as long as the conversion void * -> ptrdiff_t doesn't differ
from the two step void * -> uintptr_t -> ptrdiff_t one. In fact in this
case yours would necessarily be wrong: You'd need to use intptr_t
instead, or else would-be-negative values wouldn't end up negative,
because the uintptr_t -> ptrdiff_t conversion is a zero extension then.
Whether my variant would suffer the same issue would depend on
whether void * -> ptrdiff_t is sign- or zero-extending.

Now if you went the intptr_t route, I'd be fine too, because then
again the outer cast can be dropped, which is all I'm after.

3) sizeof(ptrdiff_t) < sizeof(void *)

In this case neither variant will work for all possible pointer inputs.

Jan



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

Re: [PATCH v11 5/9] xen/x86: use DECLARE_BOUNDS as required

[Jan Beulich]

>>> On 06.03.19 at 22:05, <sstabellini@kernel.org> wrote:
> On Wed, 6 Mar 2019, Jan Beulich wrote:
>> >>> On 05.03.19 at 23:38, <sstabellini@kernel.org> wrote:
>> > @@ -600,7 +602,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 = (char *)__init_begin;
>> > +          init_lt(va, __init_end);
>> > +          va += PAGE_SIZE )
>> 
>> Is the line wrapping really needed here?
> 
> It would end at 80 characters exactly otherwise. I am happy to do as you
> prefer.

And 80 characters is what we permit.

Jan



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

Re: [PATCH v11 5/9] xen/x86: use DECLARE_BOUNDS as required

[Jan Beulich]

>>> On 06.03.19 at 22:38, <sstabellini@kernel.org> wrote:
> On Wed, 6 Mar 2019, Jan Beulich wrote:
>> >>> On 05.03.19 at 23:38, <sstabellini@kernel.org> wrote:
>> > @@ -600,7 +602,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 = (char *)__init_begin;
>> > +          init_lt(va, __init_end);
>> > +          va += PAGE_SIZE )
>> >          clear_page(va);
>> 
>> At the example of this - is casting away of const-ness not also a
>> potential certification hindrance?
> 
> Darn, well spotted! No, it is not permitted (Rule 11.8).
> In this case const is not correct because we actually modify the object
> few lines below:
> 
>   base->priv = 1;
> 
> 
> This is problematic. We have also the following instances in this series
> to deal with:
> 
> xen/arch/arm/percpu.c:_free_percpu_area
>   char *p = (char *)__per_cpu_start + __per_cpu_offset[cpu];
> 
> xen/arch/x86/percpu.c:_free_percpu_area
>   char *p = (char *)__per_cpu_start + __per_cpu_offset[cpu];
> 
> xen/arch/x86/setup.c:init_done
>   for ( va = (char *)__init_begin; init_lt(va, __init_end); va += PAGE_SIZE 
> )
> 
> xen/arch/x86/alternative.c:apply_alternatives
>   for ( a = base = (struct alt_instr *)start; alt_instr_lt(a, end); a++ )
> 
> 
> In all these cases we actually end up modifying the object. I suggest
> we remove the const from either __DECLARE_BOUNDS (so from everywhere),
> or just for per_cpu, init, and alt_instr by introducing another MACRO.

The core macro should just have another parameter, "modifier", which
is allowed to be empty. The "end" pointers should, if at all possible, be
const regardless, but the start pointer declarations should honor the
specified macro.

I'd like to note though that in the first two cases we don't alter the
declared object anyway, but instead a derived one; the declaration
should not use const for other reasons though. And the 3rd case is
fiddling with something that has lost its meaning as an object. In fact
we're about to free the underlying memory. In this case I'd prefer to
retain the const, but I won't insist as the symbol is non-const right
now as well.

Jan



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

Re: [PATCH v11 9/9] xen: explicit casts when DECLARE_BOUNDS cannot be used

[Jan Beulich]

>>> On 05.03.19 at 23:38, <sstabellini@kernel.org> wrote:
> --- a/xen/arch/x86/setup.c
> +++ b/xen/arch/x86/setup.c
> @@ -976,7 +976,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 = (uintptr_t)__2M_rwdata_end -
> +                                       (uintptr_t) _stext;

Did you consider changing __2M_rwdata_end's declaration and using
text_bytediff() here? Otherwise, if the casts are to stay, the stray
blank after the second one should go away.

> --- a/xen/common/virtual_region.c
> +++ b/xen/common/virtual_region.c
> @@ -119,7 +119,9 @@ 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 = ((uintptr_t)bug_frames[i] - (uintptr_t)s) /
> +             sizeof(struct bug_frame);

I disagree with the comment, and if you think it is correct, then no
matter what you do the behavior is undefined. Instead I view the
entirety of the .bug_frames.* sections as a single array, with
labels placed not only at start and end, but also in the middle. I
think the code here would better also be taken care of by the
DECLARE_BOUNDS() machinery, dividing the single array into
multiple smaller ones.

Jan



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

Re: [PATCH v11 5/9] xen/x86: use DECLARE_BOUNDS as required

[Ian Jackson]

Stefano Stabellini writes ("Re: [PATCH v11 5/9] xen/x86: use DECLARE_BOUNDS as required"):
> This is problematic. We have also the following instances in this series
> to deal with:
> 
> xen/arch/arm/percpu.c:_free_percpu_area
>   char *p = (char *)__per_cpu_start + __per_cpu_offset[cpu];
> 
> xen/arch/x86/percpu.c:_free_percpu_area
>   char *p = (char *)__per_cpu_start + __per_cpu_offset[cpu];
> 
> xen/arch/x86/setup.c:init_done
>   for ( va = (char *)__init_begin; init_lt(va, __init_end); va += PAGE_SIZE )
> 
> xen/arch/x86/alternative.c:apply_alternatives
>   for ( a = base = (struct alt_instr *)start; alt_instr_lt(a, end); a++ )

Presumably you will be writing some explanation as to why each of
these is OK ?

> In all these cases we actually end up modifying the object. I suggest
> we remove the const from either __DECLARE_BOUNDS (so from everywhere),
> or just for per_cpu, init, and alt_instr by introducing another MACRO.

My personal opinion is that you should:

 * Introduce a new macro DECLARE_BOUNDS_NONCONST.
 * Write a clear explanation of when DECLARE_BOUNDS_NONCONST is
   permitted and when DECLARE_BOUNDS must be used.
 * Implement both macros in terms of a common internal macro
   which takes an argument CONST which is empty or `const'.

Ian.

PS in prose, `macro' is written thus, not in all caps.  It is not an
acroynom; the etymology is from ancient Greek.

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

Re: [PATCH v11 5/9] xen/x86: use DECLARE_BOUNDS as required

[Ian Jackson]

Stefano Stabellini writes ("Re: [PATCH v11 5/9] xen/x86: use DECLARE_BOUNDS as required"):
> On Wed, 6 Mar 2019, Jan Beulich wrote:
> > Is the line wrapping really needed here?
> 
> It would end at 80 characters exactly otherwise. I am happy to do as you
> prefer.

Certainly I prefer lines to end strictly less than 80 characters and
preferably even shorter.  My mailer/editor produces wrap damage for
exactly-80-character lines.

I think this wrapping was introduced by Stefano after a prompt from
me.

Jan, it is quite unfortunate that you are replying to Stefano to
disagree with things that Stefano did because I suggested them, rather
than replying to my patch comments.  We must not put Stefano in the
middle of a disagreement between different committers.

On this style question, while I have an opinion, I don't consider
myself a maintainer, so the hypervisor maintainers' answer is
definitive.


Nevertheless, I will have one go at trying to convince Jan:

Note that:

 - When code is turned into a patch, an extra character is added for
   the diff +/-.  That means that 80-column code becomes 81 columns
   wide.

 - When a patch is quoted for review in email, two (usually) extra
   quoting characters are added ('> ') for each level of reply,
   so 80-column code becomes 83 or 85 (or more) columns wide.

 - One purpose of the line length limit is to fit within a
   conventional 80-column text terminal window (or at least, to
   minimise the number of lines which overflow such a window)

 - In an 80 column ssh session, simple representations are only
   capable of unambiguously displaying lines of up to 79 characters.

 - Therefore the total available code width which can be displayed
   unambiguously in an 80-column ssh session, within a singly quoted
   patch, is 76 characters.  Longer lines produce wrap damage.

To me would seem to imply that a *code* line length limit of 76 or 74
characters should be usual.  Certainly it seems churlish to object to
patches where the new code is wrapped to avoid lines >76.

Ian.

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

Re: [PATCH v11 3/9] xen: introduce DECLARE_BOUNDS

[Ian Jackson]

Jan Beulich writes ("Re: [PATCH v11 3/9] xen: introduce DECLARE_BOUNDS"):
> On 06.03.19 at 21:55, <sstabellini@kernel.org> wrote:
> > On Wed, 6 Mar 2019, Jan Beulich wrote:
> > uintptr_t is the integer type corresponding to a pointer, so we should
> > use uintptr_t first. ptrdiff_t is the difference type so we should cast
> > to it afterwards. Specifically, uintptr_t is unsigned and ptrdiff_t is
> > signed. So I don't think it would be correct to do:
> > 
> >   return (ptrdiff_t)((ptrdiff_t)s2 - (ptrdiff_t)s1);
> > 
> > Or am I missing your point?
> 
> Well, I really mean
> 
>    return (ptrdiff_t)s2 - (ptrdiff_t)s1;
> 
> But that aside - let's consider all three cases:
> 
> 1) sizeof(ptrdiff_t) == sizeof(void *)
> 
> All fine. And you'll have some difficulty finding a platform where this
> isn't the case.

No.  This is not fine.  Signed integer subtraction sometimes has UB.

To use signed integer subtraction safely in C it is necessary to prove
that it cannot overflow.  I doubt that such a proof would be a
tractable thing to try to write for a general macro like this.  It
would imply some complicated constraint on the arguments. [1]

OTOH, unsigned integer subtraction is always defined; and the
subsequent conversion to signed is also always defined.


Jan, I appreciate you looking at this stuff in detail.

But it is important tht these kind of review comments are actually
correct.  This is not the first occasion in the discussion of this
series where you have advocated a construct which contains a lurking
possibility of UB, and argued against the corresponding construct
which has no UB.


Conversely, I think the discussion of the sizes of these types is not
really relevant.  To port Xen it is necessary to have an environment
where
    sizeof(ptrdiff_t) == sizeof(uintptr_t)
    == sizeof(void*) == sizeof(struct maxalign*)
and I think there is little harm in further baking in those
assumptions.


Ian.

[1] In particular ISTM that on systems with unsigned pointers,
pointers around the 0x8bazillion boundary cause trouble.

Say, for any reason someone were to use the linker machinery to
produce two pointer constants 0x7f00.... and 0x8100.... .  Then that's
the two signed numbers 7f00.._16 and -7f00.._16 and the difference is
fe00.._16 which is not representable in the same sized signed integer.
But of course what the programmer wanted was to treat that mod 2^n and
get 200_16 ie 0x0200.. and that is what using unsigned arithmetic,
with last-minute conversion to signed, does.

To use signed arithmetic it would be necessary to somehow exclude such
uses.  Maybe they are fanciful because the two linker symbols would
often be in different notional segments but ISTM that this argument
depends on assumptions about the addressing layout and is in general
rather weak.  So a BUILD_BUG_ON would be needed.

All of this seems much more easily avoided by using uintptr_t.

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

Re: [PATCH v11 5/9] xen/x86: use DECLARE_BOUNDS as required

[George Dunlap]

On 3/7/19 2:02 PM, Ian Jackson wrote:
> Stefano Stabellini writes ("Re: [PATCH v11 5/9] xen/x86: use DECLARE_BOUNDS as required"):
>> On Wed, 6 Mar 2019, Jan Beulich wrote:
>>> Is the line wrapping really needed here?
>>
>> It would end at 80 characters exactly otherwise. I am happy to do as you
>> prefer.
> 
> Certainly I prefer lines to end strictly less than 80 characters and
> preferably even shorter.  My mailer/editor produces wrap damage for
> exactly-80-character lines.
> 
> I think this wrapping was introduced by Stefano after a prompt from
> me.
> 
> Jan, it is quite unfortunate that you are replying to Stefano to
> disagree with things that Stefano did because I suggested them, rather
> than replying to my patch comments.  We must not put Stefano in the
> middle of a disagreement between different committers.
> 
> On this style question, while I have an opinion, I don't consider
> myself a maintainer, so the hypervisor maintainers' answer is
> definitive.
> 
> 
> Nevertheless, I will have one go at trying to convince Jan:
> 
> Note that:
> 
>  - When code is turned into a patch, an extra character is added for
>    the diff +/-.  That means that 80-column code becomes 81 columns
>    wide.
> 
>  - When a patch is quoted for review in email, two (usually) extra
>    quoting characters are added ('> ') for each level of reply,
>    so 80-column code becomes 83 or 85 (or more) columns wide.
> 
>  - One purpose of the line length limit is to fit within a
>    conventional 80-column text terminal window (or at least, to
>    minimise the number of lines which overflow such a window)
> 
>  - In an 80 column ssh session, simple representations are only
>    capable of unambiguously displaying lines of up to 79 characters.
> 
>  - Therefore the total available code width which can be displayed
>    unambiguously in an 80-column ssh session, within a singly quoted
>    patch, is 76 characters.  Longer lines produce wrap damage.
> 
> To me would seem to imply that a *code* line length limit of 76 or 74
> characters should be usual.

Personally, I prefer to have wider lines.  My windows are all wider than
80 lines, and for my personal projects my code lines are much longer; 80
is a compromise "down" for me.  I'm happy not to argue that we should
raise the line limit higher than 80 to accommodate my own choice of
screen layout, but in return I'd rather not compromise further to
benefit people who choose to limit their windows to 80 columns.

>  Certainly it seems churlish to object to
> patches where the new code is wrapped to avoid lines >76.

I agree with this.

 -George

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

Re: [PATCH v11 5/9] xen/x86: use DECLARE_BOUNDS as required

[Ian Jackson]

Jan Beulich writes ("Re: [PATCH v11 5/9] xen/x86: use DECLARE_BOUNDS as required"):
> I'd like to note though that in the first two cases we don't alter the
> declared object anyway, but instead a derived one; the declaration
> should not use const for other reasons though. And the 3rd case is
> fiddling with something that has lost its meaning as an object. In fact
> we're about to free the underlying memory. In this case I'd prefer to
> retain the const, but I won't insist as the symbol is non-const right
> now as well.

I think if you do this

  extern const struct blah blah_start[];

it is not safe to cast the const away later.

From C99+TC1+TC2, 6.7.3 5:

  5  If an attempt is made to modify an object defned with a
     const-qualifed type through use of an lvalue with
     non-const-qualifed type, the behavior is undefned. ...

Of course `extern const struct blah blah_start[]' is only a
declaration, not a definition.

But all the declarations/definitions even in different translation
units must be `compatible' (or UB, 6.2.7 2) and for types to be
`compatible' they must be identically qualified (6.7.3 9).

So the compiler authors will say that

  `extern const struct blah blah_start[]'

implies a definition somewhere of an array 

  `extern const struct blah blah_start[ somesize ]

and therefore any accesses `to blah_start' (which in their mind means
any accesses via a pointer whose provenance is blah_start) via
non-const lvalues (even for reading!) is UB.

Ian.

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

Re: [PATCH v11 9/9] xen: explicit casts when DECLARE_BOUNDS cannot be used [and 1 more messages]

[Ian Jackson]

Stefano Stabellini writes ("[PATCH v11 9/9] xen: explicit casts when DECLARE_BOUNDS cannot be used"):
> Sometimes the static inline functions provided by DECLARE_BOUNDS cannot
> be used. This patch uses explicit casts to uintptr_t in those cases.
> 
> M3CM: Rule-18.2: Subtraction between pointers shall only be applied to
> pointers that address elements of the same array

IMO these ad-hoc workarounds *must* be accompanied by a clear
explanation of why the DECLARE_BOUNDS arragement is not suitable.


In general I am very strongly of the opinion that:

 * Code should be written so that it does not introduce the risk of
   accidentally inducing UB in correct-looking constructs elsewhere.

 * In particular, declarations of linker symbols should always be
   written in a way that means that plain correct-looking code will
   either *be* correct or produce a compile error.

 * Exceptions of whatever kind should be fully justified, in the
   code.  Usually this will mean a clear and detailed and rigorous
   comment which contains a proof (or similar argument) that
   the approach taken is sound.


Considering these two in reverse order.

bug_frames
----------

What appears to be going on is this:

setup_virtual_regions contains a static const array of pointers to
struct bug_frame.  These struct bug_frame* values are themselves
linker symbol values.

Because the compiler has visibility of all of this, it is allowed to
assume that these values do not change.  It will therefore wrongly
assume that they are incomparable.

(All of this ought to have been explained in comments in the code.
Given that it wasn't, a comment ought to have been in your patch.
I shouldn't have had to go and dig into the code.)

And I'm afraid I do not like your patch.  The declaration in eg
xen/include/asm-arm/bug.h

  extern const struct bug_frame __start_bug_frames[],
                                __stop_bug_frames_0[],
                                __stop_bug_frames_1[],
                                __stop_bug_frames_2[];

is a clear violation of the rule in the DECLARE_BOUNDS comment:

+  * These macros, or an alternative technique, MUST be used any time
+  * linker symbols are imported into C via the `extern []' idiom.

I think an `alternative technique' should be applied *at the time of
the declaration* and it should suffice to *mostly avoid the risk of
dangerous uses*.

Jan has one suggestion, which I don't fully understand (see below).
Alternatively I suggest the following ad-hoc approach:

  * Rename __start_bug_frames etc. to
      __UBDANGER_*bug_frame*
    (and provide an accompanying comment saying what the UB danger is
    and how UB must be avoided).

  * Change the definition of bug_frames to
      static const uintptr_t __initconstrel bug_frames[] ...

  * Provide an arch-independent helper macro to both declare
    __UBDANGER_*bug_frame*, converting the pointers to
    uintptr_t, and initialise the array.

  * In setup_virtual_regions, cast the uintptr_t to a pointer.  This
    needs to be accompanied by a substantial comment explaining why
    this is safe.  Elements of the proof are
      - stating that the thing came from a pointer which came
        from a linker symbol, so it is a valid pointer value
      - some kind of argument that no code elsewhere will
        compare pointers from different core_init.frame
        and core_init.ex entries.
    I observe that with the current code I think making such an
    argument may involve doing something about core_init.ex and
    core_init.ex_end.  Not sure.

An alternative would be to construct this table in a different
translation unit, in which case the compiler compiling
setup_virtual_regions cannot `prove' the falsehood .  (Note that we
must be disabling whole-program optimisation.  I hope we are!)


Jan writes:

> I disagree with the comment,

I also disagree with the wording of the comment.  It is seriously
misleading.  These symbols do in fact refer to the same object!
The problem is that the compiler thinks otherwise.  You need wording
like that in DECLARE_BOUNDS.  (Or a reference to it.)

> and if you think it is correct, then no
> matter what you do the behavior is undefined. Instead I view the
> entirety of the .bug_frames.* sections as a single array, with
> labels placed not only at start and end, but also in the middle. I
> think the code here would better also be taken care of by the
> DECLARE_BOUNDS() machinery, dividing the single array into
> multiple smaller ones.

Jan, I'm not sure exactly what you are suggesting.  Currently the
array has one pointer per element.  Are you suggesting it should have
two pointers (start and end), with different notional types ?

If that is OK from a perf point of view then it is an easy answer
(although a bit tiresome since more linker symbols will have to be
generated).


__start_xen
-----------

> @@ -976,7 +976,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 = (uintptr_t)__2M_rwdata_end -
> +                                       (uintptr_t) _stext;

__2M_rwdata_end and _stext are both declared in header files using the
deprecated pattern.

  xen/include/xen/kernel.h:extern char _stext[], _etext[];
  xen/include/asm-x86/setup.h:extern char __2M_rwdata_start[], __2M_rwdata_end[];

According to the comment for DEFINE_BOUNDS, it ought to be used here,
or some other mechanism.  But AFAICT hyou have not changed the
declarations ?

If you changed the declarations then (i) mistakes would be avoided
(ii) you would still have to use explicit casts to compare these
pointers to different sections, but you could write a clear
explanation of (a) why this is needed (b) why it is safe.

You have done neither.


I hope this makes sense.

Thanks,
Ian.

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

Re: [PATCH v11 1/9] xen: use __UINTPTR_TYPE__ for uintptr_t

[Ian Jackson]

Jan Beulich writes ("Re: [PATCH v11 1/9] xen: use __UINTPTR_TYPE__ for uintptr_t"):
> On 06.03.19 at 22:16, <sstabellini@kernel.org> wrote:
> > Also, it is not a good idea to use __mode__(__pointer__) to define
> > uintptr_t, because it relies on an obscurely defined GCC feature whose
> > semantics might be taken to imply that the things are really pointers.
> 
> "Obscurely defined" is rather subjective, I'd say. The "pointer" mode
> is precisely for this according to my understanding,

Unfortunately it is not your understanding that is relevant, but the
possible future understanding of compiler authors.

> and personally I find "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" sufficiently
> clear, and not leaving room for any (hidden) interpretation like "to
> imply that the things are really pointers".

The problem is the word `mode'.  I cannot find a definition of that in
this context in the GCC manual.  The only thing is the extremely
informal description you quote.  Normally in English `mode' would mean
something other than merely `size'.

I think in fact the word `mode' here is a reference to a concept in
the GCC compiler internals.  This is really not a situation we should
be optimistic about, particularly given that we are in serious dispute
with compiler authors about the meanings of specifications and that
compiler authors seem quite willing to engage in rank sophistry.
Relying on our good faith interpretation of some vague text is a
seriously bad idea.

On the other hand, the cpp documentation has this:

  __UINTPTR_TYPE__

     These macros are defined to the correct underlying types for the
     'size_t', 'ptrdiff_t', 'wchar_t', 'wint_t', 'intmax_t', ...

This is completely clear and unambiguous.

If I were Stefano I would have quoted these manual sections in my
commit message or a code comment.  I would have hoped that to be
conclusive.

> ... in line with
> 
> void
> c_stddef_cpp_builtins(void)
> {
>   builtin_define_with_value ("__SIZE_TYPE__", SIZE_TYPE, 0);
>   builtin_define_with_value ("__PTRDIFF_TYPE__", PTRDIFF_TYPE, 0);
> [...]
>   if (INTPTR_TYPE)
>     builtin_define_with_value ("__INTPTR_TYPE__", INTPTR_TYPE, 0);
>   if (UINTPTR_TYPE)
>     builtin_define_with_value ("__UINTPTR_TYPE__", UINTPTR_TYPE, 0);
> }

I have no idea what this is.  Is this some piece of the cpp source
code ?  What is your point in quoting it ?

> > [1] https://gcc.gnu.org/onlinedocs/cpp/Common-Predefined-Macros.html 
> 
> Note that this also says "Some of these macros may not be defined on
> particular systems if GCC does not provide a ‘stdint.h’ header on those
> systems." I have absolutely no idea under what conditions gcc may not
> (want to) provide stdint.h.

We do not need to worry about this.

What this means is that if the underlying gcc platform does not have a
uintptr_t then we don't get __UINTPTR_T__.  If that is the case then
we need to fix GCC, not use some random other ad-hoc compiler feature
which we hope, without clear justification, will DTRT.

Furthermore, the possible failure case with __UINTPTR_T__is an
undefined type error.  Failures due to use of the mode attribute are
unknowable because there is no real specification for it.

Ian.

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

Re: [PATCH v11 9/9] xen: explicit casts when DECLARE_BOUNDS cannot be used [and 1 more messages]

[Stefano Stabellini]

On Thu, 7 Mar 2019, Ian Jackson wrote:
> bug_frames
> ----------
> 
> What appears to be going on is this:
> 
> setup_virtual_regions contains a static const array of pointers to
> struct bug_frame.  These struct bug_frame* values are themselves
> linker symbol values.
> 
> Because the compiler has visibility of all of this, it is allowed to
> assume that these values do not change.  It will therefore wrongly
> assume that they are incomparable.
> 
> (All of this ought to have been explained in comments in the code.
> Given that it wasn't, a comment ought to have been in your patch.
> I shouldn't have had to go and dig into the code.)
> 
> And I'm afraid I do not like your patch.  The declaration in eg
> xen/include/asm-arm/bug.h
> 
>   extern const struct bug_frame __start_bug_frames[],
>                                 __stop_bug_frames_0[],
>                                 __stop_bug_frames_1[],
>                                 __stop_bug_frames_2[];
> 
> is a clear violation of the rule in the DECLARE_BOUNDS comment:
> 
> +  * These macros, or an alternative technique, MUST be used any time
> +  * linker symbols are imported into C via the `extern []' idiom.
> 
> I think an `alternative technique' should be applied *at the time of
> the declaration* and it should suffice to *mostly avoid the risk of
> dangerous uses*.
> 
> Jan has one suggestion, which I don't fully understand (see below).
> Alternatively I suggest the following ad-hoc approach:
> 
>   * Rename __start_bug_frames etc. to
>       __UBDANGER_*bug_frame*
>     (and provide an accompanying comment saying what the UB danger is
>     and how UB must be avoided).
> 
>   * Change the definition of bug_frames to
>       static const uintptr_t __initconstrel bug_frames[] ...
> 
>   * Provide an arch-independent helper macro to both declare
>     __UBDANGER_*bug_frame*, converting the pointers to
>     uintptr_t, and initialise the array.

I don't follow why this macro is needed and what more concretely you
have in mind for the array initialization.


>   * In setup_virtual_regions, cast the uintptr_t to a pointer.  This
>     needs to be accompanied by a substantial comment explaining why
>     this is safe.  Elements of the proof are
>       - stating that the thing came from a pointer which came
>         from a linker symbol, so it is a valid pointer value
>       - some kind of argument that no code elsewhere will
>         compare pointers from different core_init.frame
>         and core_init.ex entries.
>     I observe that with the current code I think making such an
>     argument may involve doing something about core_init.ex and
>     core_init.ex_end.  Not sure.

I am sure it will take a couple of tries to get the comment right, but I
can give a try at a first draft.


> An alternative would be to construct this table in a different
> translation unit, in which case the compiler compiling
> setup_virtual_regions cannot `prove' the falsehood .  (Note that we
> must be disabling whole-program optimisation.  I hope we are!)
> 
> 
> Jan writes:
> 
> > I disagree with the comment,
> 
> I also disagree with the wording of the comment.  It is seriously
> misleading.  These symbols do in fact refer to the same object!
> The problem is that the compiler thinks otherwise.  You need wording
> like that in DECLARE_BOUNDS.  (Or a reference to it.)
> 
> > and if you think it is correct, then no
> > matter what you do the behavior is undefined. Instead I view the
> > entirety of the .bug_frames.* sections as a single array, with
> > labels placed not only at start and end, but also in the middle. I
> > think the code here would better also be taken care of by the
> > DECLARE_BOUNDS() machinery, dividing the single array into
> > multiple smaller ones.
> 
> Jan, I'm not sure exactly what you are suggesting.  Currently the
> array has one pointer per element.  Are you suggesting it should have
> two pointers (start and end), with different notional types ?

I am not fully understanding the suggestion either.

Even if we split it into multiple start-end pairs, still we won't be
able to compare start/end of a pair with start/end of different pair
without casts.


> If that is OK from a perf point of view then it is an easy answer
> (although a bit tiresome since more linker symbols will have to be
> generated).
> 
> 
> __start_xen
> -----------
> 
> > @@ -976,7 +976,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 = (uintptr_t)__2M_rwdata_end -
> > +                                       (uintptr_t) _stext;
> 
> __2M_rwdata_end and _stext are both declared in header files using the
> deprecated pattern.
> 
>   xen/include/xen/kernel.h:extern char _stext[], _etext[];
>   xen/include/asm-x86/setup.h:extern char __2M_rwdata_start[], __2M_rwdata_end[];
> 
> According to the comment for DEFINE_BOUNDS, it ought to be used here,
> or some other mechanism.  But AFAICT hyou have not changed the
> declarations ?
> 
> If you changed the declarations then (i) mistakes would be avoided
> (ii) you would still have to use explicit casts to compare these
> pointers to different sections, but you could write a clear
> explanation of (a) why this is needed (b) why it is safe.
> 
> You have done neither.
> 
> 
> I hope this makes sense.
 
This makes sense. Also, I understand _stext and __2M_rwdata_end better
than the bug_frames and I should be able to write an half-decent
explanation here. FYI _stext is already converted to DEFINE_BOUNDS in
this series, but you are right that __2M_rwdata_end is not. I'll fix
that.

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

Re: [PATCH v11 1/9] xen: use __UINTPTR_TYPE__ for uintptr_t

[Jan Beulich]

>>> Ian Jackson <ian.jackson@citrix.com> 03/07/19 4:44 PM >>>
>Jan Beulich writes ("Re: [PATCH v11 1/9] xen: use __UINTPTR_TYPE__ for uintptr_t"):
>> On 06.03.19 at 22:16, <sstabellini@kernel.org> wrote:
>> > Also, it is not a good idea to use __mode__(__pointer__) to define
>> > uintptr_t, because it relies on an obscurely defined GCC feature whose
>> > semantics might be taken to imply that the things are really pointers.
>> 
>> "Obscurely defined" is rather subjective, I'd say. The "pointer" mode
>> is precisely for this according to my understanding,
>
>Unfortunately it is not your understanding that is relevant, but the
>possible future understanding of compiler authors.

Well, I'm trying to claim that I sort of understand what their understanding
is, ...


>> and personally I find "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" sufficiently
>> clear, and not leaving room for any (hidden) interpretation like "to
>> imply that the things are really pointers".
>
>The problem is the word `mode'.  I cannot find a definition of that in
>this context in the GCC manual.  The only thing is the extremely
>informal description you quote.  Normally in English `mode' would mean
>something other than merely `size'.
>
>I think in fact the word `mode' here is a reference to a concept in
>the GCC compiler internals.

... knowing some (hopefully enough) of these internals, first and foremost
because I had done the entire tool chain port for an experimental OS of
ours years ago. Arguably this may not be enough, but I hope it explains
why I'm saying what I say.


>This is really not a situation we should
>be optimistic about, particularly given that we are in serious dispute
>with compiler authors about the meanings of specifications and that
>compiler authors seem quite willing to engage in rank sophistry.
>Relying on our good faith interpretation of some vague text is a
>seriously bad idea.

I'm not relying on just the what you call "vague" text. I'm having a rather
hard time imagining why the compiler folks would change a fundamental
concept of how the compiler (internally, and exposed to users via the
__attribute__(())) works.


>On the other hand, the cpp documentation has this:
>
>__UINTPTR_TYPE__
>
>These macros are defined to the correct underlying types for the
>'size_t', 'ptrdiff_t', 'wchar_t', 'wint_t', 'intmax_t', ...
>
>This is completely clear and unambiguous.
>
>If I were Stefano I would have quoted these manual sections in my
>commit message or a code comment.  I would have hoped that to be
>conclusive.

He sort of did that, by pointing at them instead of quoting.


>> ... in line with
>> 
>> void
>> c_stddef_cpp_builtins(void)
>> {
>>   builtin_define_with_value ("__SIZE_TYPE__", SIZE_TYPE, 0);
>>   builtin_define_with_value ("__PTRDIFF_TYPE__", PTRDIFF_TYPE, 0);
>> [...]
>>   if (INTPTR_TYPE)
>>     builtin_define_with_value ("__INTPTR_TYPE__", INTPTR_TYPE, 0);
>>   if (UINTPTR_TYPE)
>>     builtin_define_with_value ("__UINTPTR_TYPE__", UINTPTR_TYPE, 0);
>> }
>
>I have no idea what this is.  Is this some piece of the cpp source
>code ?  What is your point in quoting it ?

The fact that the symbol is not defined unconditionally, whereas
__attribute__((__mode__(__pointer__))) is always available.


>> > [1] https://gcc.gnu.org/onlinedocs/cpp/Common-Predefined-Macros.html 
>> 
>> Note that this also says "Some of these macros may not be defined on
>> particular systems if GCC does not provide a ‘stdint.h’ header on those
>> systems." I have absolutely no idea under what conditions gcc may not
>> (want to) provide stdint.h.
>
>We do not need to worry about this.
>
>What this means is that if the underlying gcc platform does not have a
>uintptr_t then we don't get __UINTPTR_T__.  If that is the case then
>we need to fix GCC, not use >
>Furthermore, the possible failure case with __UINTPTR_T__is an
>undefined type error.  Failures due to use of the mode attribute are
>unknowable because there is no real specification for it.

Well, I accept this to be one possible and valid standpoint. I don't accept
this to be the only one.

Jan




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

Re: [PATCH v11 3/9] xen: introduce DECLARE_BOUNDS

[Jan Beulich]

>>> Ian Jackson <ian.jackson@citrix.com> 03/07/19 3:16 PM >>>
>Jan Beulich writes ("Re: [PATCH v11 3/9] xen: introduce DECLARE_BOUNDS"):
>> On 06.03.19 at 21:55, <sstabellini@kernel.org> wrote:
>> > On Wed, 6 Mar 2019, Jan Beulich wrote:
>> > uintptr_t is the integer type corresponding to a pointer, so we should
>> > use uintptr_t first. ptrdiff_t is the difference type so we should cast
>> > to it afterwards. Specifically, uintptr_t is unsigned and ptrdiff_t is
>> > signed. So I don't think it would be correct to do:
>> > 
>> >   return (ptrdiff_t)((ptrdiff_t)s2 - (ptrdiff_t)s1);
>> > 
>> > Or am I missing your point?
>> 
>> Well, I really mean
>> 
>>    return (ptrdiff_t)s2 - (ptrdiff_t)s1;
>> 
>> But that aside - let's consider all three cases:
>> 
>> 1) sizeof(ptrdiff_t) == sizeof(void *)
>> 
>> All fine. And you'll have some difficulty finding a platform where this
>> isn't the case.
>
>No.  This is not fine.  Signed integer subtraction sometimes has UB.
>
>To use signed integer subtraction safely in C it is necessary to prove
>that it cannot overflow.  I doubt that such a proof would be a
>tractable thing to try to write for a general macro like this.  It
>would imply some complicated constraint on the arguments. [1]
>
>OTOH, unsigned integer subtraction is always defined; and the
>subsequent conversion to signed is also always defined.

I've spent an hour trying to find the relevant parts of the spec, but I'm
afraid I've once again failed at finding all necessary pieces. In the
Conversions section there is

"Otherwise, if the new type is unsigned, the value is converted by
 repeatedly adding or subtracting one more than the maximum value
 that can be represented in the new type until the value is in the
 range of the new type."


But I can't take this as applicable to other than conversions.

Whereas one of the general clauses in the Expressions section says

"If an exceptional condition occurs during the evaluation of an expression
 (that is, if the result is not mathematically defined or not in the range of
 representable values for its type), the behavior is undefined."

This does not make any provisions for unsigned types being special.

And there's nothing in the Additive operators section afaics.

As a result, I'd appreciate if you could help me out and point at what
exact sections you're referring to.

As to the unsigned -> signed conversion, according to, again in the
Conversions section,


"Otherwise, the new type is signed and the value cannot be represented
 in it; either the result is implementation-defined or an implementation-
 defined signal is raised."


this is implementation defined according to my understanding (but I take
it that we're fine with this).


>Jan, I appreciate you looking at this stuff in detail.
>
>But it is important tht these kind of review comments are actually
>correct.  This is not the first occasion in the discussion of this
>series where you have advocated a construct which contains a lurking
>possibility of UB, and argued against the corresponding construct
>which has no UB.

I'm sorry for this. I have to admit that (see above) the scattering around of
rules in the C spec isn't too helpful here.


>Conversely, I think the discussion of the sizes of these types is not
>really relevant.  To port Xen it is necessary to have an environment
>where
>sizeof(ptrdiff_t) == sizeof(uintptr_t)
>== sizeof(void*) == sizeof(struct maxalign*)
>and I think there is little harm in further baking in those
>assumptions.

Good (except that I don't understand the struct maxalign* part - neither
why you use a pointer there, nor - assuming there was no pointer - how
that would be in line with __{,u}int128_t or with vector types).


Jan


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

Re: [PATCH v11 5/9] xen/x86: use DECLARE_BOUNDS as required

[Jan Beulich]

>>> Ian Jackson <ian.jackson@citrix.com> 03/07/19 3:44 PM >>>
>Jan Beulich writes ("Re: [PATCH v11 5/9] xen/x86: use DECLARE_BOUNDS as required"):
>> I'd like to note though that in the first two cases we don't alter the
>> declared object anyway, but instead a derived one; the declaration
>> should not use const for other reasons though. And the 3rd case is
>> fiddling with something that has lost its meaning as an object. In fact
>> we're about to free the underlying memory. In this case I'd prefer to
>> retain the const, but I won't insist as the symbol is non-const right
>> now as well.
>
>I think if you do this
>
>extern const struct blah blah_start[];
>
>it is not safe to cast the const away later.
>
>From C99+TC1+TC2, 6.7.3 5:
>
>5  If an attempt is made to modify an object defned with a
>const-qualifed type through use of an lvalue with
>non-const-qualifed type, the behavior is undefned. ...
>
>Of course `extern const struct blah blah_start[]' is only a
>declaration, not a definition.
>
>But all the declarations/definitions even in different translation
>units must be `compatible' (or UB, 6.2.7 2) and for types to be
>`compatible' they must be identically qualified (6.7.3 9).
>
>So the compiler authors will say that
>
>`extern const struct blah blah_start[]'
>
>implies a definition somewhere of an array 
>
>`extern const struct blah blah_start[ somesize ]
>
>and therefore any accesses `to blah_start' (which in their mind means
>any accesses via a pointer whose provenance is blah_start) via
>non-const lvalues (even for reading!) is UB.

It looks to me as if we were in agreement then. I was talking about freeing
an object that was const-qualified, not modifying it. The scrubbing of the
memory could be considered part of the freeing, it just ought to occur up
front because of otherwise possible races, and because we have no means
to tell the freeing function to do the scrubbing (in fact I should say "we used
to have no means, as I think right now the memory would be scrubbed
implicitly, so the explicit scrubbing could probably be dropped).


Jan




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

Re: [PATCH v11 9/9] xen: explicit casts when DECLARE_BOUNDS cannot be used [and 1 more messages]

[Jan Beulich]

>>> Ian Jackson <ian.jackson@citrix.com> 03/07/19 4:26 PM >>>
>Jan writes:
>
>> I disagree with the comment,
>
>I also disagree with the wording of the comment.  It is seriously
>misleading.  These symbols do in fact refer to the same object!
>The problem is that the compiler thinks otherwise.  You need wording
>like that in DECLARE_BOUNDS.  (Or a reference to it.)
>
>> and if you think it is correct, then no
>> matter what you do the behavior is undefined. Instead I view the
>> entirety of the .bug_frames.* sections as a single array, with
>> labels placed not only at start and end, but also in the middle. I
>> think the code here would better also be taken care of by the
>> DECLARE_BOUNDS() machinery, dividing the single array into
>> multiple smaller ones.
>
>Jan, I'm not sure exactly what you are suggesting.  Currently the
>array has one pointer per element.  Are you suggesting it should have
>two pointers (start and end), with different notional types ?

Yes.


>If that is OK from a perf point of view then it is an easy answer
>(although a bit tiresome since more linker symbols will have to be
>generated).

This is init-time code and init-time data, so to me neither the performance
aspect nor the doubled storage requirements would really matter. Both
could perhaps even be eliminated by using an array of unions.


Jan


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

Re: [PATCH v11 5/9] xen/x86: use DECLARE_BOUNDS as required

[Jan Beulich]

>>> Ian Jackson <ian.jackson@citrix.com> 03/07/19 3:02 PM >>>
>Stefano Stabellini writes ("Re: [PATCH v11 5/9] xen/x86: use DECLARE_BOUNDS as required"):
>> On Wed, 6 Mar 2019, Jan Beulich wrote:
>> > Is the line wrapping really needed here?
>> 
>> It would end at 80 characters exactly otherwise. I am happy to do as you
>> prefer.
>
>Certainly I prefer lines to end strictly less than 80 characters and
>preferably even shorter.  My mailer/editor produces wrap damage for
>exactly-80-character lines.
>
>I think this wrapping was introduced by Stefano after a prompt from
>me.
>
>Jan, it is quite unfortunate that you are replying to Stefano to
>disagree with things that Stefano did because I suggested them, rather
>than replying to my patch comments.  We must not put Stefano in the
>middle of a disagreement between different committers.

I'm sorry, but I may have easily overlooked this earlier comment of yours.
Anyway, a later reply by Stefano suggests that the line would end up
being 79 chars, which is in line with ./CODING_STYLE.


>On this style question, while I have an opinion, I don't consider
>myself a maintainer, so the hypervisor maintainers' answer is
>definitive.
>
>
>Nevertheless, I will have one go at trying to convince Jan:
>
>Note that:
>
>- When code is turned into a patch, an extra character is added for
>the diff +/-.  That means that 80-column code becomes 81 columns
>wide.
>
>- When a patch is quoted for review in email, two (usually) extra
>quoting characters are added ('> ') for each level of reply,
>so 80-column code becomes 83 or 85 (or more) columns wide.
>
>- One purpose of the line length limit is to fit within a
>conventional 80-column text terminal window (or at least, to
>minimise the number of lines which overflow such a window)
>
>- In an 80 column ssh session, simple representations are only
>capable of unambiguously displaying lines of up to 79 characters.
>
>- Therefore the total available code width which can be displayed
>unambiguously in an 80-column ssh session, within a singly quoted
>patch, is 76 characters.  Longer lines produce wrap damage.
>
>To me would seem to imply that a *code* line length limit of 76 or 74
>characters should be usual.  Certainly it seems churlish to object to
>patches where the new code is wrapped to avoid lines >76.

./CODING_STYLE is pretty clear about it being "less than 80 characters".
If you want a lower limit, I think you'd have to propose a patch to that file
(which I'd likely try to prevent from going in). Wrapping in particular for(;;)
(as was the case here iirc) is always weighing length vs readability. I for
one consider for(;;) easier to read when it's all on one line. Therefore I'd
prefer if no "early" wrapping was done. But as always - if a majority thinks
differently, so be it.


Jan




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

Re: [PATCH v11 5/9] xen/x86: use DECLARE_BOUNDS as required

[Juergen Gross]

On 08/03/2019 09:46, Jan Beulich wrote:
>>>> Ian Jackson <ian.jackson@citrix.com> 03/07/19 3:02 PM >>>
>> Stefano Stabellini writes ("Re: [PATCH v11 5/9] xen/x86: use DECLARE_BOUNDS as required"):
>>> On Wed, 6 Mar 2019, Jan Beulich wrote:
>>>> Is the line wrapping really needed here?
>>>
>>> It would end at 80 characters exactly otherwise. I am happy to do as you
>>> prefer.
>>
>> Certainly I prefer lines to end strictly less than 80 characters and
>> preferably even shorter.  My mailer/editor produces wrap damage for
>> exactly-80-character lines.
>>
>> I think this wrapping was introduced by Stefano after a prompt from
>> me.
>>
>> Jan, it is quite unfortunate that you are replying to Stefano to
>> disagree with things that Stefano did because I suggested them, rather
>> than replying to my patch comments.  We must not put Stefano in the
>> middle of a disagreement between different committers.
> 
> I'm sorry, but I may have easily overlooked this earlier comment of yours.
> Anyway, a later reply by Stefano suggests that the line would end up
> being 79 chars, which is in line with ./CODING_STYLE.
> 
> 
>> On this style question, while I have an opinion, I don't consider
>> myself a maintainer, so the hypervisor maintainers' answer is
>> definitive.
>>
>>
>> Nevertheless, I will have one go at trying to convince Jan:
>>
>> Note that:
>>
>> - When code is turned into a patch, an extra character is added for
>> the diff +/-.  That means that 80-column code becomes 81 columns
>> wide.
>>
>> - When a patch is quoted for review in email, two (usually) extra
>> quoting characters are added ('> ') for each level of reply,
>> so 80-column code becomes 83 or 85 (or more) columns wide.
>>
>> - One purpose of the line length limit is to fit within a
>> conventional 80-column text terminal window (or at least, to
>> minimise the number of lines which overflow such a window)
>>
>> - In an 80 column ssh session, simple representations are only
>> capable of unambiguously displaying lines of up to 79 characters.
>>
>> - Therefore the total available code width which can be displayed
>> unambiguously in an 80-column ssh session, within a singly quoted
>> patch, is 76 characters.  Longer lines produce wrap damage.
>>
>> To me would seem to imply that a *code* line length limit of 76 or 74
>> characters should be usual.  Certainly it seems churlish to object to
>> patches where the new code is wrapped to avoid lines >76.
> 
> ./CODING_STYLE is pretty clear about it being "less than 80 characters".
> If you want a lower limit, I think you'd have to propose a patch to that file
> (which I'd likely try to prevent from going in). Wrapping in particular for(;;)
> (as was the case here iirc) is always weighing length vs readability. I for
> one consider for(;;) easier to read when it's all on one line. Therefore I'd
> prefer if no "early" wrapping was done. But as always - if a majority thinks
> differently, so be it.

-2 for lowering the limit.


Juergen

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

Re: [PATCH v11 1/9] xen: use __UINTPTR_TYPE__ for uintptr_t

[Andrew Cooper]

On 05/03/2019 22:38, Stefano Stabellini wrote:
> Use __UINTPTR_TYPE__ to define uintptr_t. A later patch will make use of
> __PTRDIFF_TYPE__ to define ptrdiff_t.
>
> Signed-off-by: Stefano Stabellini <stefanos@xilinx.com>
> Reviewed-by: Ian Jackson <ian.jackson@eu.citrix.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 v11:
> - split patch
> ---
>  xen/include/xen/types.h | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/xen/include/xen/types.h b/xen/include/xen/types.h
> index 03f0fe6..1059fab 100644
> --- a/xen/include/xen/types.h
> +++ b/xen/include/xen/types.h
> @@ -52,7 +52,7 @@ typedef __u32 __be32;
>  typedef __u64 __le64;
>  typedef __u64 __be64;
>  
> -typedef unsigned int __attribute__((__mode__(__pointer__))) uintptr_t;
> +typedef __UINTPTR_TYPE__ uintptr_t;

I've already raised an objected to this once, so I'm going to be very
blunt and crystal clear.

NACK.

Stop messing around with GCC-isms and use the spec-compliant way of
getting uintptr_t (and others).

#include <stdint.h>

That way, *any* compiler will give you a working uintptr_t, not just
those which are emulating GCC's internals.

This isn't rocket science, and I know all of us have better things to be
doing that wasting time arguing over aspects which are unrelated to
actually fixing the problem.

~Andrew

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

Re: [PATCH v11 1/9] xen: use __UINTPTR_TYPE__ for uintptr_t

[Ian Jackson]

Andrew Cooper writes ("Re: [PATCH v11 1/9] xen: use __UINTPTR_TYPE__ for uintptr_t"):
> NACK.
> 
> Stop messing around with GCC-isms and use the spec-compliant way of
> getting uintptr_t (and others).
> 
> #include <stdint.h>

If everything is working correctly, stdint.h is provided by the
compiler (eg by libgcc) and this will DTRT.

However, if things are not working correctly, we will pick up the host
operating system <stdint.h>.  I consider it unacceptable that a build
system issue would be able to cause us to compile a hypervisor with a
busted uintptr_t.  (This is particularly bad given that the hypervisor
currently hardly uses uintptr_t at all, which means such a
miscompilation might even go largely undetected.)

I would tolerate this approach if it were accompanied by an
appropriate set of BUILD_BUG_ON which will detect if this has
occurred, for example by checking that sizeof(void*) ==
sizeof(uintptr_t).


> That way, *any* compiler will give you a working uintptr_t, not just
> those which are emulating GCC's internals.

It has been conventional for many years in embedded programming to,
effectively, briefly put on the hat of the C language implementor and
provide one's own uintptr_t et al.

For this reason all compilers have always provided compiler-defined
types like __UINTPTR_TYPE__.

It is not conceivable that we would succeed in porting Xen to a
compiler which was so strange that it did not provide any internal
type aliases for these standard types.  (Indeed such a compiler
authors would have to do more work to implement <stdint.h>!)  It is
highly unlikely that we would want to try porting Xen to a compiler
which didn't provide these with these now-conventional names, given
that any contemporary compiler author can trivially provide them.

And the (vanishingly unlikely) failure mode is a completely obvious
missing type error.

So __UINTPTR_TYPE__ is strictly superior: it is de jure correct
according to the manual for the compiler that invented the name; it is
currently implemented everywhere we care about (including, right now,
llvm), and will almost certainly remain available, and unlike
<stdint.h> there is absolutely no risk of it meaning anything else.


To put it another way, <stdint.h> is a layer of indirection to
__UINTPTR_TYPE__.  It is provided by libgcc (and other compilers'
equivalents) for the benefit of programs which need to run on much
wider variety of platforms than Xen, and to provide a set of more
convenient names which are not available by default (for compatibility
reasons).

We do need the convenient and standard names, but we can provide them
ourselves.  Relying on an external layer of indirection, however,
exposes us to needless additional risk that the indirection ends up
pointing to the wrong place.


> This isn't rocket science, and I know all of us have better things to be
> doing that wasting time arguing over aspects which are unrelated to
> actually fixing the problem.

I suggest we compromise on <stdint.h> + BUILD_BUG_ON since at least
two of us seem to be able to tolerate that.


Ian.

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

Re: SRSL People... [PATCH v11 0/9] misc safety certification fixes

[Andrew Cooper]

On 05/03/2019 22:38, Stefano Stabellini wrote:
> Hi all,
>
> This version of the series makes use of the macro suggested by Jan with
> few modifications. See each patch for a description of the changes.
>
> 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-11
>
> for you to fetch changes up to 26fb02b5ae59b48b51ca788be91510d8df48ab0a:
>
>   xen: explicit casts when DECLARE_BOUNDS cannot be used (2019-03-05 14:29:51 -0800)
>
> ----------------------------------------------------------------
> Stefano Stabellini (9):
>       xen: use __UINTPTR_TYPE__ for uintptr_t
>       xen: introduce ptrdiff_t
>       xen: introduce DECLARE_BOUNDS
>       xen/arm: use DECLARE_BOUNDS as required
>       xen/x86: use DECLARE_BOUNDS as required
>       xen/common: use DECLARE_BOUNDS as required
>       xen: use DECLARE_BOUNDS as required
>       xen: use DECLARE_BOUNDS in alternative.c
>       xen: explicit casts when DECLARE_BOUNDS cannot be used

Seriously.  Everyone take a step back from their keyboards and apply
some common sense.

This argument has gone on far beyond the only answer which matters.  WTF
is still continuing for? (This is a rhetorical question - I don't expect
an answer.)


The rational for this series is to satisfy MISRA.  MISRA have said in no
uncertain terms that all of these tricks are unacceptable, and have
identified the one acceptable option.  By not doing what MISRA said,
this series doesn't move Xen any closer to passing certification.

Therefore, we either do nothing, or we do the single thing which MISRA
have identified as acceptable.  Obviously, "do nothing" is off the cards
in the context of this series.


Before any further arguing continues (especially concerning the details
of how to proceed), are there any concerns or queries with the points
laid out here?

~Andrew

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

Re: [PATCH v11 3/9] xen: introduce DECLARE_BOUNDS

[Ian Jackson]

Jan Beulich writes ("Re: [PATCH v11 3/9] xen: introduce DECLARE_BOUNDS"):
> >No.  This is not fine.  Signed integer subtraction sometimes has UB.
...
> I've spent an hour trying to find the relevant parts of the spec, but I'm
> afraid I've once again failed at finding all necessary pieces.

The spec is obtuse indeed.

> This does not make any provisions for unsigned types being special.

6.2.5 9
   .... A computation involving unsigned operands can never overfow,
   because a result that cannot be represented by the resulting
   unsigned integer type is reduced modulo the number that is one
   greater than the largest value that can be represented by the
   resulting type.

> As to the unsigned -> signed conversion, according to, again in the
> Conversions section,
> 
> "Otherwise, the new type is signed and the value cannot be represented
>  in it; either the result is implementation-defined or an implementation-
>  defined signal is raised."
> 
> this is implementation defined according to my understanding (but I take
> it that we're fine with this).

Yes.  On the platforms currently supported by Xen, the
`implementation' `defines' that the result is simply the same bit
pattern.  I say `implementation' in quotes because really we mean
compiler, and `defines' because while no doubt there is a formal
conformance statement somewhere, what we are relying on is instead
something much more general:

The community of C programmers for `normal' architectures effectively
require that conversion to signed integer work this way.  A platform
where that didn't work would be strange and would produce a lot of
other porting difficulties too.  And because the result is *defined*,
the compiler authors can't use this as an optimisation opportunity so
aren't motivated to sophistic justifications for misbehaviour.

> >Conversely, I think the discussion of the sizes of these types is not
> >really relevant.  To port Xen it is necessary to have an environment
> >where
> >sizeof(ptrdiff_t) == sizeof(uintptr_t)
> >== sizeof(void*) == sizeof(struct maxalign*)
> >and I think there is little harm in further baking in those
> >assumptions.
> 
> Good (except that I don't understand the struct maxalign* part - neither
> why you use a pointer there, nor - assuming there was no pointer - how
> that would be in line with __{,u}int128_t or with vector types).

On some machines, pointers to different types have different
representations and even different sizes.  Xen cannot be ported to
such machines - at least, not without an awful lot of work.

Xen assumes that all pointers have the same representation.

`struct maxalign*' was a somewhat-informal way of referring to the
type of a pointer to an object type with the maximal alignment.

Ian.

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

Re: [PATCH v11 5/9] xen/x86: use DECLARE_BOUNDS as required

[Ian Jackson]

Jan Beulich writes ("Re: [PATCH v11 5/9] xen/x86: use DECLARE_BOUNDS as required"):
> Ian Jackson <ian.jackson@citrix.com> 03/07/19 3:02 PM
> >Jan, it is quite unfortunate that you are replying to Stefano to
> >disagree with things that Stefano did because I suggested them, rather
> >than replying to my patch comments.  We must not put Stefano in the
> >middle of a disagreement between different committers.
> 
> I'm sorry, but I may have easily overlooked this earlier comment of yours.
> Anyway, a later reply by Stefano suggests that the line would end up
> being 79 chars, which is in line with ./CODING_STYLE.

Right.  Given others' replies, I won't press this point further.

Of course `less than 80 characters' would mean that the maximum was 79
but if you all decide to increase it by one then grump but whatever.

Ian.

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

Re: [PATCH v11 5/9] xen/x86: use DECLARE_BOUNDS as required

[Ian Jackson]

Jan Beulich writes ("Re: [PATCH v11 5/9] xen/x86: use DECLARE_BOUNDS as required"):
> Ian Jackson <ian.jackson@citrix.com> 03/07/19 3:44 PM >>>
> >Jan Beulich writes ("Re: [PATCH v11 5/9] xen/x86: use DECLARE_BOUNDS as required"):
> >> I'd like to note though that in the first two cases we don't alter the
> >> declared object anyway, but instead a derived one; the declaration
> >> should not use const for other reasons though. And the 3rd case is
> >> fiddling with something that has lost its meaning as an object. In fact
> >> we're about to free the underlying memory. In this case I'd prefer to
> >> retain the const, but I won't insist as the symbol is non-const right
> >> now as well.
> >
> >I think if you do this
> >extern const struct blah blah_start[];
> >it is not safe to cast the const away later.
...
> It looks to me as if we were in agreement then.

Perhaps so.

> I was talking about freeing an object that was const-qualified, not
> modifying it.

I'm not sure what you mean, precisely.  Do you mean this:

   extern const struct blah blah_start[];
   ...
   free(blah_start);

?  But I'm sure the hypervisor doesn't have a function free().

> The scrubbing of the memory could be considered part
> of the freeing, it just ought to occur up front because of otherwise
> possible races, and because we have no means to tell the freeing
> function to do the scrubbing (in fact I should say "we used to have
> no means, as I think right now the memory would be scrubbed
> implicitly, so the explicit scrubbing could probably be dropped).

Maybe you mean this:

   extern const struct blah blah_start[];
   ...
   memset(blah_start, 0, size of blah array);

?  That is clearly forbidden, regardless of whether the cost
is cast away.  But I think this:

   extern struct blah blah_start[];
   ...
   memset(blah_start, 0, size of blah array);

is fine.

Ian.

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

Re: [PATCH v11 9/9] xen: explicit casts when DECLARE_BOUNDS cannot be used [and 1 more messages] [and 1 more messages]

[Ian Jackson]

Jan Beulich writes ("Re: [PATCH v11 9/9] xen: explicit casts when DECLARE_BOUNDS cannot be used [and 1 more messages]"):
> Ian Jackson <ian.jackson@citrix.com> 03/07/19 4:26 PM >>>
> >Jan, I'm not sure exactly what you are suggesting.  Currently the
> >array has one pointer per element.  Are you suggesting it should have
> >two pointers (start and end), with different notional types ?
> 
> Yes.

Right.

> >If that is OK from a perf point of view then it is an easy answer
> >(although a bit tiresome since more linker symbols will have to be
> >generated).
> 
> This is init-time code and init-time data, so to me neither the performance
> aspect nor the doubled storage requirements would really matter.

Jolly good.

> Both could perhaps even be eliminated by using an array of unions.

If there are no compelling perf reassons to do otherwise, let us do
something which is obviously correct, rather than complicating
matters.


Stefano Stabellini writes ("Re: [PATCH v11 9/9] xen: explicit casts when DECLARE_BOUNDS cannot be used [and 1 more messages]"):
> On Thu, 7 Mar 2019, Ian Jackson wrote:
> > Jan has one suggestion, which I don't fully understand (see below).
> > Alternatively I suggest the following ad-hoc approach:

Probably, we should follow Jan's suggestion and not mine.


> > I also disagree with the wording of the comment.  It is seriously
> > misleading.  These symbols do in fact refer to the same object!
> > The problem is that the compiler thinks otherwise.  You need wording
> > like that in DECLARE_BOUNDS.  (Or a reference to it.)
> > 
> > > and if you think it is correct, then no
> > > matter what you do the behavior is undefined. Instead I view the
> > > entirety of the .bug_frames.* sections as a single array, with
> > > labels placed not only at start and end, but also in the middle. I
> > > think the code here would better also be taken care of by the
> > > DECLARE_BOUNDS() machinery, dividing the single array into
> > > multiple smaller ones.
> > 
> > Jan, I'm not sure exactly what you are suggesting.  Currently the
> > array has one pointer per element.  Are you suggesting it should have
> > two pointers (start and end), with different notional types ?
> 
> I am not fully understanding the suggestion either.
> 
> Even if we split it into multiple start-end pairs, still we won't be
> able to compare start/end of a pair with start/end of different pair
> without casts.

I don't think this bug_frames code does that ?  Maybe I have misread
it.


> This makes sense. Also, I understand _stext and __2M_rwdata_end better
> than the bug_frames and I should be able to write an half-decent
> explanation here. FYI _stext is already converted to DEFINE_BOUNDS in
> this series, but you are right that __2M_rwdata_end is not. I'll fix
> that.

Thanks.

Ian.

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

Re: [PATCH v11 5/9] xen/x86: use DECLARE_BOUNDS as required

[Ian Jackson]

Stefano Stabellini writes ("[PATCH v11 5/9] xen/x86: use DECLARE_BOUNDS as required"):
> Use DECLARE_BOUNDS and the two static inline functions that come with it
> for comparisons and subtractions of:
> 
> __2M_rwdata_start, __2M_rwdata_end, __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
> 
> possible to use the provided static inline functions.
> M3CM: Rule-18.2: Subtraction between pointers shall only be applied to
> pointers that address elements of the same array.

Hi.  I picked one of these to briefly look at the detail of and I
spotted this:

> @@ -49,7 +50,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 *)__per_cpu_start + __per_cpu_offset[cpu];

This is dangerous because it turns __per_cpu_start which is a magic
linker symbol pointer into one which you might try to compare with
other pointers.

I can't see from your patch what else is done to `p'.  Something ought
to be done to prevent future programmers from comparing p to other
pointers.

Some extended comment, or something, is likely to be needed every time
you introduce a cast.

>      free_xenheap_pages(p, PERCPU_ORDER);

JOOI, why does free_xenheap_pages not take a void* ?

Thanks,
Ian.

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

Re: SRSL People... [PATCH v11 0/9] misc safety certification fixes

[Ian Jackson]

Andrew Cooper writes ("Re: SRSL People... [PATCH v11 0/9] misc safety certification fixes"):
> The rational for this series is to satisfy MISRA.  MISRA have said in no
> uncertain terms that all of these tricks are unacceptable, and have
> identified the one acceptable option.  By not doing what MISRA said,
> this series doesn't move Xen any closer to passing certification.

Can you please point me to the message from the MISRA folks ?  I seem
to have missed it.

Ian.

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

Re: SRSL People... [PATCH v11 0/9] misc safety certification fixes

[Jan Beulich]

>>> On 08.03.19 at 16:26, <andrew.cooper3@citrix.com> wrote:
> On 05/03/2019 22:38, Stefano Stabellini wrote:
>> Hi all,
>>
>> This version of the series makes use of the macro suggested by Jan with
>> few modifications. See each patch for a description of the changes.
>>
>> 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-11
>>
>> for you to fetch changes up to 26fb02b5ae59b48b51ca788be91510d8df48ab0a:
>>
>>   xen: explicit casts when DECLARE_BOUNDS cannot be used (2019-03-05 
> 14:29:51 -0800)
>>
>> ----------------------------------------------------------------
>> Stefano Stabellini (9):
>>       xen: use __UINTPTR_TYPE__ for uintptr_t
>>       xen: introduce ptrdiff_t
>>       xen: introduce DECLARE_BOUNDS
>>       xen/arm: use DECLARE_BOUNDS as required
>>       xen/x86: use DECLARE_BOUNDS as required
>>       xen/common: use DECLARE_BOUNDS as required
>>       xen: use DECLARE_BOUNDS as required
>>       xen: use DECLARE_BOUNDS in alternative.c
>>       xen: explicit casts when DECLARE_BOUNDS cannot be used
> 
> Seriously.  Everyone take a step back from their keyboards and apply
> some common sense.
> 
> This argument has gone on far beyond the only answer which matters.  WTF
> is still continuing for? (This is a rhetorical question - I don't expect
> an answer.)
> 
> 
> The rational for this series is to satisfy MISRA.  MISRA have said in no
> uncertain terms that all of these tricks are unacceptable, and have
> identified the one acceptable option.  By not doing what MISRA said,
> this series doesn't move Xen any closer to passing certification.

Iirc they said to use casts in a central place. Am I misremembering?
Because that's what the series does.

Jan



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

Re: [PATCH v11 5/9] xen/x86: use DECLARE_BOUNDS as required

[Jan Beulich]

>>> On 08.03.19 at 16:43, <ian.jackson@citrix.com> wrote:
> Stefano Stabellini writes ("[PATCH v11 5/9] xen/x86: use DECLARE_BOUNDS as 
> required"):
>>      free_xenheap_pages(p, PERCPU_ORDER);
> 
> JOOI, why does free_xenheap_pages not take a void* ?

It does. It's the const that gets in the way here, not the char. And
(as said earlier on this lengthy discussion) my proposal to add const
to the freeing functions' parameters was disliked years ago (if I'm
remembering right also by you).

Jan



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

Re: [PATCH v11 5/9] xen/x86: use DECLARE_BOUNDS as required

[Jan Beulich]

>>> On 08.03.19 at 16:36, <ian.jackson@citrix.com> wrote:
> Jan Beulich writes ("Re: [PATCH v11 5/9] xen/x86: use DECLARE_BOUNDS as required"):
>> Ian Jackson <ian.jackson@citrix.com> 03/07/19 3:44 PM >>>
>> >Jan Beulich writes ("Re: [PATCH v11 5/9] xen/x86: use DECLARE_BOUNDS as required"):
>> >> I'd like to note though that in the first two cases we don't alter the
>> >> declared object anyway, but instead a derived one; the declaration
>> >> should not use const for other reasons though. And the 3rd case is
>> >> fiddling with something that has lost its meaning as an object. In fact
>> >> we're about to free the underlying memory. In this case I'd prefer to
>> >> retain the const, but I won't insist as the symbol is non-const right
>> >> now as well.
>> >
>> >I think if you do this
>> >extern const struct blah blah_start[];
>> >it is not safe to cast the const away later.
> ...
>> It looks to me as if we were in agreement then.
> 
> Perhaps so.
> 
>> I was talking about freeing an object that was const-qualified, not
>> modifying it.
> 
> I'm not sure what you mean, precisely.  Do you mean this:
> 
>    extern const struct blah blah_start[];
>    ...
>    free(blah_start);
> 
> ?  But I'm sure the hypervisor doesn't have a function free().

It has xfree(), free_xenheap_pages() and a few more.

>> The scrubbing of the memory could be considered part
>> of the freeing, it just ought to occur up front because of otherwise
>> possible races, and because we have no means to tell the freeing
>> function to do the scrubbing (in fact I should say "we used to have
>> no means, as I think right now the memory would be scrubbed
>> implicitly, so the explicit scrubbing could probably be dropped).
> 
> Maybe you mean this:
> 
>    extern const struct blah blah_start[];
>    ...
>    memset(blah_start, 0, size of blah array);
> 
> ?  That is clearly forbidden, regardless of whether the cost
> is cast away.  But I think this:
> 
>    extern struct blah blah_start[];
>    ...
>    memset(blah_start, 0, size of blah array);
> 
> is fine.

Of course. What I was meaning though is entirely different.
Assume we had

void my_free(const void *);

I could then

extern const struct blah blah_start[];
...
my_free(blah_start);

my_free() might internally scrub the memory, no matter that it
was handed a pointer to const, because at that point the
memory doesn't underly any particular object anymore - it's just
plain memory. The context here was

    /* Zero the .init code and data. */
    for ( va = __init_begin; va < _p(__init_end); va += PAGE_SIZE )
        clear_page(va);

    /* Destroy Xen's mappings, and reuse the pages. */
    if ( using_2M_mapping() )
    {
        start = (unsigned long)&__2M_init_start,
        end   = (unsigned long)&__2M_init_end;
    }
    else
    {
        start = (unsigned long)&__init_begin;
        end   = (unsigned long)&__init_end;
    }

    destroy_xen_mappings(start, end);
    init_xenheap_pages(__pa(start), __pa(end));

and what I was trying to explain is that here we're simply doing
the scrubbing ahead of calling the freeing function
(init_xenheap_pages()) because, at least back when this code
was written, no scrubbing would have been done inside the
page allocator itself in this case.

Jan



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

Re: SRSL People... [PATCH v11 0/9] misc safety certification fixes

[Stefano Stabellini]

On Fri, 8 Mar 2019, Jan Beulich wrote:
> >>> On 08.03.19 at 16:26, <andrew.cooper3@citrix.com> wrote:
> > On 05/03/2019 22:38, Stefano Stabellini wrote:
> >> Hi all,
> >>
> >> This version of the series makes use of the macro suggested by Jan with
> >> few modifications. See each patch for a description of the changes.
> >>
> >> 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-11
> >>
> >> for you to fetch changes up to 26fb02b5ae59b48b51ca788be91510d8df48ab0a:
> >>
> >>   xen: explicit casts when DECLARE_BOUNDS cannot be used (2019-03-05 
> > 14:29:51 -0800)
> >>
> >> ----------------------------------------------------------------
> >> Stefano Stabellini (9):
> >>       xen: use __UINTPTR_TYPE__ for uintptr_t
> >>       xen: introduce ptrdiff_t
> >>       xen: introduce DECLARE_BOUNDS
> >>       xen/arm: use DECLARE_BOUNDS as required
> >>       xen/x86: use DECLARE_BOUNDS as required
> >>       xen/common: use DECLARE_BOUNDS as required
> >>       xen: use DECLARE_BOUNDS as required
> >>       xen: use DECLARE_BOUNDS in alternative.c
> >>       xen: explicit casts when DECLARE_BOUNDS cannot be used
> > 
> > Seriously.  Everyone take a step back from their keyboards and apply
> > some common sense.
> > 
> > This argument has gone on far beyond the only answer which matters.  WTF
> > is still continuing for? (This is a rhetorical question - I don't expect
> > an answer.)
> > 
> > 
> > The rational for this series is to satisfy MISRA.  MISRA have said in no
> > uncertain terms that all of these tricks are unacceptable, and have
> > identified the one acceptable option.  By not doing what MISRA said,
> > this series doesn't move Xen any closer to passing certification.
> 
> Iirc they said to use casts in a central place. Am I misremembering?
> Because that's what the series does.

Yes, the answer was in a private thread, but it was as Jan wrote. In
short, they said to use casts to uintptr_t for comparisons/subtractions.
They also said ideally to do the casts only once, because the fewer
casts the better. 

So, as far as I understand, the general approach taken by this series
should satisfy both C99 and MISRAC.

The remaining questions are on the implementation details, such as what
to do for the few cases where the DECLARE_BOUNDS approach doesn't work
well without modifications.

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