* [PATCH v10 0/6] misc safety certification fixes
@ 2019-02-25 20:49 Stefano Stabellini
2019-02-25 20:50 ` [PATCH v10 1/6] xen: introduce ptrdiff_t Stefano Stabellini
` (5 more replies)
0 siblings, 6 replies; 35+ messages in thread
From: Stefano Stabellini @ 2019-02-25 20:49 UTC (permalink / raw)
To: xen-devel
Cc: sstabellini, andrew.cooper3, George.Dunlap, julien.grall,
JBeulich, ian.jackson
Hi all,
This version of the series makes use of the macro suggested by Jan with
few modifications.
The main changes are:
- introduce the WHATEVER macro, named DEFINE_SYMBOL
- make use of the static inline functions to subtract and compare
pointers when possible
- when not possible, cast to uintptr_t
- use __UINTPTR_TYPE__ and __PTRDIFF_TYPE__ to define uintptr_t and
ptrdiff_t
Let me premise that I think that using two different types for start and
end limits the usability of the static inlines, so overall v9 is better
than this version of the series in my opinion.
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-10
for you to fetch changes up to 6d95b9dbb2c8dd1ad635ae84c56087c0f815e69b:
xen: use DEFINE_SYMBOL as required (2019-02-25 12:42:47 -0800)
----------------------------------------------------------------
Stefano Stabellini (6):
xen: introduce ptrdiff_t
xen: introduce DEFINE_SYMBOL
xen/arm: use DEFINE_SYMBOL as required
xen/x86: use DEFINE_SYMBOL as required
xen/common: use DEFINE_SYMBOL as required
xen: use DEFINE_SYMBOL as required
xen/arch/arm/alternative.c | 15 ++++++++++-----
xen/arch/arm/arm32/livepatch.c | 14 +++++++++++++-
xen/arch/arm/arm64/livepatch.c | 14 +++++++++++++-
xen/arch/arm/device.c | 13 ++++++++-----
xen/arch/arm/livepatch.c | 16 ++++++++++++++--
xen/arch/arm/mm.c | 11 ++++++-----
xen/arch/arm/percpu.c | 9 +++++----
xen/arch/arm/platform.c | 9 ++++++---
xen/arch/arm/setup.c | 5 +++--
xen/arch/x86/alternative.c | 8 +++++++-
xen/arch/x86/efi/efi-boot.h | 10 ++++++----
xen/arch/x86/percpu.c | 9 +++++----
xen/arch/x86/setup.c | 13 +++++++++----
xen/arch/x86/smpboot.c | 5 +++--
xen/common/kernel.c | 12 ++++++++++--
xen/common/lib.c | 7 +++++--
xen/common/schedule.c | 6 ++++--
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/grant_table.h | 3 ++-
xen/include/asm-arm/percpu.h | 2 +-
xen/include/asm-x86/percpu.h | 4 +++-
xen/include/xen/compiler.h | 32 ++++++++++++++++++++++++++++++++
xen/include/xen/kernel.h | 37 +++++++++++++++++++++----------------
xen/include/xen/types.h | 3 ++-
27 files changed, 204 insertions(+), 80 deletions(-)
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel
^ permalink raw reply [flat|nested] 35+ messages in thread
* [PATCH v10 1/6] xen: introduce ptrdiff_t
2019-02-25 20:49 [PATCH v10 0/6] misc safety certification fixes Stefano Stabellini
@ 2019-02-25 20:50 ` Stefano Stabellini
2019-02-26 10:54 ` Jan Beulich
2019-02-26 16:24 ` Ian Jackson
2019-02-25 20:50 ` [PATCH v10 2/6] xen: introduce DEFINE_SYMBOL Stefano Stabellini
` (4 subsequent siblings)
5 siblings, 2 replies; 35+ messages in thread
From: Stefano Stabellini @ 2019-02-25 20:50 UTC (permalink / raw)
To: xen-devel
Cc: Stefano Stabellini, sstabellini, wei.liu2, konrad.wilk,
George.Dunlap, andrew.cooper3, ian.jackson, George.Dunlap, tim,
julien.grall, jbeulich, ian.jackson
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.
Also, use __UINTPTR_TYPE__ for uintptr_t for consistency.
Signed-off-by: Stefano Stabellini <stefanos@xilinx.com>
CC: jbeulich@suse.com
CC: andrew.cooper3@citrix.com
CC: julien.grall@arm.com
CC: George.Dunlap@eu.citrix.com
CC: ian.jackson@eu.citrix.com
CC: konrad.wilk@oracle.com
CC: tim@xen.org
CC: wei.liu2@citrix.com
---
Changes in v10:
- use __UINTPTR_TYPE__ and __PTRDIFF_TYPE__
Changes in v9:
- new patch
---
xen/include/xen/types.h | 3 ++-
1 file changed, 2 insertions(+), 1 deletion(-)
diff --git a/xen/include/xen/types.h b/xen/include/xen/types.h
index 03f0fe6..508a462 100644
--- a/xen/include/xen/types.h
+++ b/xen/include/xen/types.h
@@ -52,7 +52,8 @@ typedef __u32 __be32;
typedef __u64 __le64;
typedef __u64 __be64;
-typedef unsigned int __attribute__((__mode__(__pointer__))) uintptr_t;
+typedef __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
^ permalink raw reply related [flat|nested] 35+ messages in thread
* [PATCH v10 2/6] xen: introduce DEFINE_SYMBOL
2019-02-25 20:49 [PATCH v10 0/6] misc safety certification fixes Stefano Stabellini
2019-02-25 20:50 ` [PATCH v10 1/6] xen: introduce ptrdiff_t Stefano Stabellini
@ 2019-02-25 20:50 ` Stefano Stabellini
2019-02-26 15:20 ` Jan Beulich
2019-02-26 16:46 ` Ian Jackson
2019-02-25 20:50 ` [PATCH v10 3/6] xen/arm: use DEFINE_SYMBOL as required Stefano Stabellini
` (3 subsequent siblings)
5 siblings, 2 replies; 35+ messages in thread
From: Stefano Stabellini @ 2019-02-25 20:50 UTC (permalink / raw)
To: xen-devel
Cc: Stefano Stabellini, sstabellini, andrew.cooper3, George.Dunlap,
julien.grall, JBeulich, ian.jackson
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.
Suggested-by: Jan Beulich <JBeulich@suse.com>
Signed-off-by: Stefano Stabellini <stefanos@xilinx.com>
---
xen/include/xen/compiler.h | 32 ++++++++++++++++++++++++++++++++
1 file changed, 32 insertions(+)
diff --git a/xen/include/xen/compiler.h b/xen/include/xen/compiler.h
index ff6c0f5..99da107 100644
--- a/xen/include/xen/compiler.h
+++ b/xen/include/xen/compiler.h
@@ -99,6 +99,38 @@
__asm__ ("" : "=r"(__ptr) : "0"(ptr)); \
(typeof(ptr)) (__ptr + (off)); })
+
+/*
+ * Declare start and end array variables in C corresponding to existing
+ * linker symbols.
+ *
+ * Two static inline functions are declared to do comparisons and
+ * subtractions between these variables.
+ *
+ * The end variable is declared with a different type to make sure that
+ * the static inline functions cannot be misused.
+ */
+#define DEFINE_SYMBOL(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[]) \
+{ \
+ return (uintptr_t)s1 < (uintptr_t)s2; \
+} \
+ \
+static inline ptrdiff_t name ## _diff(const type s1[], \
+ const struct abstract_ ## name s2[]) \
+{ \
+ return ((uintptr_t)s2 - (uintptr_t)s1) / sizeof(*s1); \
+}
+
#ifdef __GCC_ASM_FLAG_OUTPUTS__
# define ASM_FLAG_OUT(yes, no) yes
#else
--
1.9.1
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel
^ permalink raw reply related [flat|nested] 35+ messages in thread
* [PATCH v10 3/6] xen/arm: use DEFINE_SYMBOL as required
2019-02-25 20:49 [PATCH v10 0/6] misc safety certification fixes Stefano Stabellini
2019-02-25 20:50 ` [PATCH v10 1/6] xen: introduce ptrdiff_t Stefano Stabellini
2019-02-25 20:50 ` [PATCH v10 2/6] xen: introduce DEFINE_SYMBOL Stefano Stabellini
@ 2019-02-25 20:50 ` Stefano Stabellini
2019-02-26 17:11 ` Ian Jackson
2019-02-25 20:50 ` [PATCH v10 4/6] xen/x86: " Stefano Stabellini
` (2 subsequent siblings)
5 siblings, 1 reply; 35+ messages in thread
From: Stefano Stabellini @ 2019-02-25 20:50 UTC (permalink / raw)
To: xen-devel
Cc: Stefano Stabellini, sstabellini, andrew.cooper3, George.Dunlap,
julien.grall, JBeulich, ian.jackson
Use DEFINE_SYMBOL and the two static inline functions that come with it
for comparisons and subtractions of:
__init_begin, __init_end, __alt_instructions, __alt_instructions_end,
__per_cpu_start, __per_cpu_data_end, _splatform, _eplatform, _sdevice,
_edevice, _asdevice, _aedevice.
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
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 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/alternative.c | 10 +++++++---
xen/arch/arm/device.c | 13 ++++++++-----
xen/arch/arm/mm.c | 7 ++++---
xen/arch/arm/percpu.c | 9 +++++----
xen/arch/arm/platform.c | 9 ++++++---
xen/include/asm-arm/percpu.h | 2 +-
6 files changed, 31 insertions(+), 19 deletions(-)
diff --git a/xen/arch/arm/alternative.c b/xen/arch/arm/alternative.c
index 52ed7ed..cef9cca 100644
--- a/xen/arch/arm/alternative.c
+++ b/xen/arch/arm/alternative.c
@@ -38,7 +38,8 @@
#undef virt_to_mfn
#define virt_to_mfn(va) _mfn(__virt_to_mfn(va))
-extern const struct alt_instr __alt_instructions[], __alt_instructions_end[];
+DEFINE_SYMBOL(struct alt_instr, alt_instr, __alt_instructions,
+ __alt_instructions_end);
struct alt_region {
const struct alt_instr *begin;
@@ -131,7 +132,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;
+ (uintptr_t)alt < (uintptr_t)region->end;
+ alt++ )
{
int nr_inst;
@@ -204,7 +208,7 @@ static int __apply_alternatives_multi_stop(void *unused)
BUG_ON(!xenmap);
region.begin = __alt_instructions;
- region.end = __alt_instructions_end;
+ region.end = (struct alt_instr *)__alt_instructions_end;
ret = __apply_alternatives(®ion, xenmap - (void *)_start);
/* The patching is not expected to fail during boot. */
diff --git a/xen/arch/arm/device.c b/xen/arch/arm/device.c
index 70cd6c1..1ecb9b7 100644
--- a/xen/arch/arm/device.c
+++ b/xen/arch/arm/device.c
@@ -22,8 +22,9 @@
#include <xen/init.h>
#include <xen/lib.h>
-extern const struct device_desc _sdevice[], _edevice[];
-extern const struct acpi_device_desc _asdevice[], _aedevice[];
+DEFINE_SYMBOL(struct device_desc, device_desc, _sdevice, _edevice);
+DEFINE_SYMBOL(struct acpi_device_desc, acpi_device_desc, _asdevice,
+ _aedevice);
int __init device_init(struct dt_device_node *dev, enum device_class class,
const void *data)
@@ -35,7 +36,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 +57,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 +78,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..9f31e81 100644
--- a/xen/arch/arm/mm.c
+++ b/xen/arch/arm/mm.c
@@ -157,7 +157,7 @@ unsigned long frametable_virt_end __read_mostly;
unsigned long max_page;
unsigned long total_pages;
-extern char __init_begin[], __init_end[];
+DEFINE_SYMBOL(char, init, __init_begin, __init_end);
/* Checking VA memory layout alignment. */
static inline void check_memory_layout_alignment_constraints(void) {
@@ -1122,7 +1122,7 @@ static void set_pte_flags_on_range(const char *p, unsigned long l, enum mg mg)
void free_init_memory(void)
{
paddr_t pa = virt_to_maddr(__init_begin);
- unsigned long len = __init_end - __init_begin;
+ unsigned long len = init_diff(__init_begin, __init_end);
uint32_t insn;
unsigned int i, nr = len / sizeof(insn);
uint32_t *p;
@@ -1140,7 +1140,8 @@ void free_init_memory(void)
set_pte_flags_on_range(__init_begin, len, mg_clear);
init_domheap_pages(pa, pa + len);
- printk("Freed %ldkB init memory.\n", (long)(__init_end-__init_begin)>>10);
+ printk("Freed %ldkB init memory.\n",
+ 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..502739d 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)
{
@@ -22,8 +23,8 @@ static int init_percpu_area(unsigned int cpu)
return -EBUSY;
if ( (p = alloc_xenheap_pages(PERCPU_ORDER, 0)) == NULL )
return -ENOMEM;
- memset(p, 0, __per_cpu_data_end - __per_cpu_start);
- __per_cpu_offset[cpu] = p - __per_cpu_start;
+ memset(p, 0, per_cpu_diff(__per_cpu_start, __per_cpu_data_end));
+ __per_cpu_offset[cpu] = (uintptr_t)p - (uintptr_t)__per_cpu_start;
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..1da33e7 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[];
+DEFINE_SYMBOL(struct platform_desc, 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..a589c1c 100644
--- a/xen/include/asm-arm/percpu.h
+++ b/xen/include/asm-arm/percpu.h
@@ -6,7 +6,7 @@
#include <xen/types.h>
#include <asm/sysregs.h>
-extern char __per_cpu_start[], __per_cpu_data_end[];
+DEFINE_SYMBOL(char, 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
^ permalink raw reply related [flat|nested] 35+ messages in thread
* [PATCH v10 4/6] xen/x86: use DEFINE_SYMBOL as required
2019-02-25 20:49 [PATCH v10 0/6] misc safety certification fixes Stefano Stabellini
` (2 preceding siblings ...)
2019-02-25 20:50 ` [PATCH v10 3/6] xen/arm: use DEFINE_SYMBOL as required Stefano Stabellini
@ 2019-02-25 20:50 ` Stefano Stabellini
2019-02-26 17:13 ` Ian Jackson
2019-02-26 17:16 ` Ian Jackson
2019-02-25 20:50 ` [PATCH v10 5/6] xen/common: " Stefano Stabellini
2019-02-25 20:50 ` [PATCH v10 6/6] xen: " Stefano Stabellini
5 siblings, 2 replies; 35+ messages in thread
From: Stefano Stabellini @ 2019-02-25 20:50 UTC (permalink / raw)
To: xen-devel
Cc: Stefano Stabellini, sstabellini, andrew.cooper3, George.Dunlap,
julien.grall, JBeulich, ian.jackson
Use SYMBOLS_SUBTRACT and SYMBOLS_COMPARE in cases of 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
Use explicit casts to uintptr_t when it is not possible to use the
provided static inline functions.
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 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/alternative.c | 8 +++++++-
xen/arch/x86/efi/efi-boot.h | 10 ++++++----
xen/arch/x86/percpu.c | 9 +++++----
xen/arch/x86/setup.c | 7 +++++--
xen/arch/x86/smpboot.c | 5 +++--
xen/drivers/vpci/vpci.c | 6 +++---
xen/include/asm-x86/percpu.h | 4 +++-
7 files changed, 32 insertions(+), 17 deletions(-)
diff --git a/xen/arch/x86/alternative.c b/xen/arch/x86/alternative.c
index b8c819a..f81759f 100644
--- a/xen/arch/x86/alternative.c
+++ b/xen/arch/x86/alternative.c
@@ -29,6 +29,10 @@
#define MAX_PATCH_LEN (255-1)
+/*
+ * Cannot use DEFINE_SYMBOL because of the way they are passed to
+ * apply_alternatives.
+ */
extern struct alt_instr __alt_instructions[], __alt_instructions_end[];
#ifdef K8_NOP1
@@ -193,8 +197,10 @@ void init_or_livepatch apply_alternatives(struct alt_instr *start,
*
* So be careful if you want to change the scan order to any other
* order.
+ *
+ * start and end could be pointers to different objects.
*/
- for ( a = base = start; a < end; a++ )
+ for ( a = base = start; (uintptr_t)a < (uintptr_t)end; a++ )
{
uint8_t *orig = ALT_ORIG_PTR(a);
uint8_t *repl = ALT_REPL_PTR(a);
diff --git a/xen/arch/x86/efi/efi-boot.h b/xen/arch/x86/efi/efi-boot.h
index 5789d2c..f798b7b 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[];
+DEFINE_SYMBOL(s32, trampoline_rel, __trampoline_rel_start,
+ __trampoline_rel_stop);
+DEFINE_SYMBOL(s32, 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..2efb180 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)
{
@@ -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] = (uintptr_t)p - (uintptr_t)__per_cpu_start;
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..b024339 100644
--- a/xen/arch/x86/setup.c
+++ b/xen/arch/x86/setup.c
@@ -252,7 +252,8 @@ void __init discard_initial_images(void)
initial_images = NULL;
}
-extern char __init_begin[], __init_end[], __bss_start[], __bss_end[];
+DEFINE_SYMBOL(char, init, __init_begin, __init_end);
+extern char __bss_start[], __bss_end[];
static void __init init_idle_domain(void)
{
@@ -600,7 +601,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..b51bead 100644
--- a/xen/arch/x86/smpboot.c
+++ b/xen/arch/x86/smpboot.c
@@ -782,7 +782,7 @@ DEFINE_PER_CPU(root_pgentry_t *, root_pgt);
static root_pgentry_t common_pgt;
-extern const char _stextentry[], _etextentry[];
+DEFINE_SYMBOL(char, textentry, _stextentry, _etextentry);
static int setup_cpu_root_pgt(unsigned int cpu)
{
@@ -811,7 +811,8 @@ static int setup_cpu_root_pgt(unsigned int cpu)
const char *ptr;
for ( rc = 0, ptr = _stextentry;
- !rc && ptr < _etextentry; ptr += PAGE_SIZE )
+ !rc && 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..1ea6d7f 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)
+DEFINE_SYMBOL(vpci_register_init_t *const, 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..8778ed1 100644
--- a/xen/include/asm-x86/percpu.h
+++ b/xen/include/asm-x86/percpu.h
@@ -2,7 +2,9 @@
#define __X86_PERCPU_H__
#ifndef __ASSEMBLY__
-extern char __per_cpu_start[], __per_cpu_data_end[];
+#include <xen/types.h>
+
+DEFINE_SYMBOL(char, 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
^ permalink raw reply related [flat|nested] 35+ messages in thread
* [PATCH v10 5/6] xen/common: use DEFINE_SYMBOL as required
2019-02-25 20:49 [PATCH v10 0/6] misc safety certification fixes Stefano Stabellini
` (3 preceding siblings ...)
2019-02-25 20:50 ` [PATCH v10 4/6] xen/x86: " Stefano Stabellini
@ 2019-02-25 20:50 ` Stefano Stabellini
2019-02-26 15:13 ` Jan Beulich
2019-02-25 20:50 ` [PATCH v10 6/6] xen: " Stefano Stabellini
5 siblings, 1 reply; 35+ messages in thread
From: Stefano Stabellini @ 2019-02-25 20:50 UTC (permalink / raw)
To: xen-devel
Cc: Stefano Stabellini, sstabellini, andrew.cooper3, George.Dunlap,
julien.grall, JBeulich, ian.jackson
Use DEFINE_SYMBOL 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,
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 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/common/kernel.c | 12 ++++++++++--
xen/common/lib.c | 7 +++++--
xen/common/schedule.c | 6 ++++--
xen/common/spinlock.c | 8 +++++---
xen/common/version.c | 9 +++++----
xen/common/virtual_region.c | 4 +++-
6 files changed, 32 insertions(+), 14 deletions(-)
diff --git a/xen/common/kernel.c b/xen/common/kernel.c
index 5766a0f..f6c8d80 100644
--- a/xen/common/kernel.c
+++ b/xen/common/kernel.c
@@ -306,20 +306,28 @@ void add_taint(unsigned int flag)
tainted |= flag;
}
+/*
+ * We cannot use DEFINE_SYMBOL because we have three variables instead
+ * of two
+ */
extern const initcall_t __initcall_start[], __presmp_initcall_end[],
__initcall_end[];
void __init do_presmp_initcalls(void)
{
const initcall_t *call;
- for ( call = __initcall_start; call < __presmp_initcall_end; call++ )
+ for ( call = __initcall_start;
+ (uintptr_t)call < (uintptr_t)__presmp_initcall_end;
+ call++ )
(*call)();
}
void __init do_initcalls(void)
{
const initcall_t *call;
- for ( call = __presmp_initcall_end; call < __initcall_end; call++ )
+ for ( call = __presmp_initcall_end;
+ (uintptr_t)call < (uintptr_t)__initcall_end;
+ call++ )
(*call)();
}
diff --git a/xen/common/lib.c b/xen/common/lib.c
index 8ebec81..0b2105a 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[];
+DEFINE_SYMBOL(ctor_func_t, 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..ac3f9af 100644
--- a/xen/common/schedule.c
+++ b/xen/common/schedule.c
@@ -67,8 +67,10 @@ DEFINE_PER_CPU(struct scheduler *, scheduler);
/* Scratch space for cpumasks. */
DEFINE_PER_CPU(cpumask_t, cpumask_scratch);
-extern const struct scheduler *__start_schedulers_array[], *__end_schedulers_array[];
-#define NUM_SCHEDULERS (__end_schedulers_array - __start_schedulers_array)
+DEFINE_SYMBOL(struct scheduler *, 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..2d04bf8 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;
+DEFINE_SYMBOL(struct lock_profile *, 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..1303fe7 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[];
+DEFINE_SYMBOL(const Elf_Note, 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 = (uintptr_t)__note_gnu_build_id_end - (uintptr_t)n;
rc = xen_build_id_check(n, sz, &build_id_p, &build_id_len);
diff --git a/xen/common/virtual_region.c b/xen/common/virtual_region.c
index aa23918..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
^ permalink raw reply related [flat|nested] 35+ messages in thread
* [PATCH v10 6/6] xen: use DEFINE_SYMBOL as required
2019-02-25 20:49 [PATCH v10 0/6] misc safety certification fixes Stefano Stabellini
` (4 preceding siblings ...)
2019-02-25 20:50 ` [PATCH v10 5/6] xen/common: " Stefano Stabellini
@ 2019-02-25 20:50 ` Stefano Stabellini
2019-02-26 15:28 ` Jan Beulich
5 siblings, 1 reply; 35+ messages in thread
From: Stefano Stabellini @ 2019-02-25 20:50 UTC (permalink / raw)
To: xen-devel
Cc: Stefano Stabellini, sstabellini, andrew.cooper3, George.Dunlap,
julien.grall, JBeulich, ian.jackson
Use DEFINE_SYMBOL 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 v10:
- new patch
---
xen/arch/arm/alternative.c | 5 +++--
xen/arch/arm/arm32/livepatch.c | 14 +++++++++++++-
xen/arch/arm/arm64/livepatch.c | 14 +++++++++++++-
xen/arch/arm/livepatch.c | 16 ++++++++++++++--
xen/arch/arm/mm.c | 4 ++--
xen/arch/arm/setup.c | 5 +++--
xen/arch/x86/setup.c | 6 ++++--
xen/include/asm-arm/grant_table.h | 3 ++-
xen/include/xen/kernel.h | 37 +++++++++++++++++++++----------------
9 files changed, 75 insertions(+), 29 deletions(-)
diff --git a/xen/arch/arm/alternative.c b/xen/arch/arm/alternative.c
index cef9cca..7a1fe5a 100644
--- a/xen/arch/arm/alternative.c
+++ b/xen/arch/arm/alternative.c
@@ -192,7 +192,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;
@@ -210,7 +210,8 @@ static int __apply_alternatives_multi_stop(void *unused)
region.begin = __alt_instructions;
region.end = (struct alt_instr *)__alt_instructions_end;
- ret = __apply_alternatives(®ion, xenmap - (void *)_start);
+ ret = __apply_alternatives(®ion, (uintptr_t)xenmap -
+ (uintptr_t)_start);
/* The patching is not expected to fail during boot. */
BUG_ON(ret != 0);
diff --git a/xen/arch/arm/arm32/livepatch.c b/xen/arch/arm/arm32/livepatch.c
index 41378a5..5f7390e 100644
--- a/xen/arch/arm/arm32/livepatch.c
+++ b/xen/arch/arm/arm32/livepatch.c
@@ -56,7 +56,19 @@ void arch_livepatch_apply(struct livepatch_func *func)
else
insn = 0xe1a00000; /* mov r0, r0 */
- new_ptr = func->old_addr - (void *)_start + vmap_of_xen_text;
+ /*
+ * We need to calculate the offset of the address from _start, and
+ * apply that to our own map, to find where we have this mapped.
+ * Doing these kind of games directly with pointers is contrary to
+ * the C rules for what pointers may be compared and computed. So
+ * we do the offset calculation with integers, which is always
+ * legal. The subsequent addition of the offset to the
+ * vmap_of_xen_text pointer is legal because the computed pointer is
+ * indeed a valid part of the object referred to by vmap_of_xen_text
+ * - namely, the byte array of our mapping of the Xen text.
+ */
+ new_ptr = ((uintptr_t)func->old_addr, - (uintptr_t)_start) +
+ vmap_of_xen_text;
len = len / sizeof(uint32_t);
/* PATCH! */
diff --git a/xen/arch/arm/arm64/livepatch.c b/xen/arch/arm/arm64/livepatch.c
index 2247b92..362235c 100644
--- a/xen/arch/arm/arm64/livepatch.c
+++ b/xen/arch/arm/arm64/livepatch.c
@@ -43,7 +43,19 @@ void arch_livepatch_apply(struct livepatch_func *func)
/* Verified in livepatch_verify_distance. */
ASSERT(insn != AARCH64_BREAK_FAULT);
- new_ptr = func->old_addr - (void *)_start + vmap_of_xen_text;
+ /*
+ * We need to calculate the offset of the address from _start, and
+ * apply that to our own map, to find where we have this mapped.
+ * Doing these kind of games directly with pointers is contrary to
+ * the C rules for what pointers may be compared and computed. So
+ * we do the offset calculation with integers, which is always
+ * legal. The subsequent addition of the offset to the
+ * vmap_of_xen_text pointer is legal because the computed pointer is
+ * indeed a valid part of the object referred to by vmap_of_xen_text
+ * - namely, the byte array of our mapping of the Xen text.
+ */
+ new_ptr = ((uintptr_t)func->old_addr, - (uintptr_t)_start) +
+ vmap_of_xen_text;
len = len / sizeof(uint32_t);
/* PATCH! */
diff --git a/xen/arch/arm/livepatch.c b/xen/arch/arm/livepatch.c
index 279d52c..af4411a 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,19 @@ void arch_livepatch_revert(const struct livepatch_func *func)
uint32_t *new_ptr;
unsigned int len;
- new_ptr = func->old_addr - (void *)_start + vmap_of_xen_text;
+ /*
+ * We need to calculate the offset of the address from _start, and
+ * apply that to our own map, to find where we have this mapped.
+ * Doing these kind of games directly with pointers is contrary to
+ * the C rules for what pointers may be compared and computed. So
+ * we do the offset calculation with integers, which is always
+ * legal. The subsequent addition of the offset to the
+ * vmap_of_xen_text pointer is legal because the computed pointer is
+ * indeed a valid part of the object referred to by vmap_of_xen_text
+ * - namely, the byte array of our mapping of the Xen text.
+ */
+ new_ptr = ((uintptr_t)func->old_addr, - (uintptr_t)_start) +
+ vmap_of_xen_text;
len = livepatch_insn_len(func);
memcpy(new_ptr, func->opaque, len);
diff --git a/xen/arch/arm/mm.c b/xen/arch/arm/mm.c
index 9f31e81..7f9a309 100644
--- a/xen/arch/arm/mm.c
+++ b/xen/arch/arm/mm.c
@@ -1084,8 +1084,8 @@ static void set_pte_flags_on_range(const char *p, unsigned long l, enum mg mg)
ASSERT(!((unsigned long) p & ~PAGE_MASK));
ASSERT(!(l & ~PAGE_MASK));
- for ( i = (p - _start) / PAGE_SIZE;
- i < (p + l - _start) / PAGE_SIZE;
+ for ( i = ((uintptr_t)p - (uintptr_t)_start) / PAGE_SIZE;
+ i < ((uintptr_t)p + l - (uintptr_t)_start) / 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 b024339..7ede826 100644
--- a/xen/arch/x86/setup.c
+++ b/xen/arch/x86/setup.c
@@ -975,7 +975,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);
@@ -1070,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..15ace6d 100644
--- a/xen/include/xen/kernel.h
+++ b/xen/include/xen/kernel.h
@@ -65,28 +65,33 @@
1; \
})
-extern char _start[], _end[], start[];
-#define is_kernel(p) ({ \
- char *__p = (char *)(unsigned long)(p); \
- (__p >= _start) && (__p < _end); \
+extern char start[];
+DEFINE_SYMBOL(char, 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); \
+DEFINE_SYMBOL(char, 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); \
+DEFINE_SYMBOL(char, 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); \
+DEFINE_SYMBOL(char, 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
^ permalink raw reply related [flat|nested] 35+ messages in thread
* Re: [PATCH v10 1/6] xen: introduce ptrdiff_t
2019-02-25 20:50 ` [PATCH v10 1/6] xen: introduce ptrdiff_t Stefano Stabellini
@ 2019-02-26 10:54 ` Jan Beulich
2019-02-26 16:24 ` Ian Jackson
1 sibling, 0 replies; 35+ messages in thread
From: Jan Beulich @ 2019-02-26 10:54 UTC (permalink / raw)
To: Stefano Stabellini
Cc: Stefano Stabellini, Wei Liu, Konrad Rzeszutek Wilk,
George Dunlap, Andrew Cooper, Ian Jackson, george.dunlap,
Tim Deegan, Julien Grall, xen-devel, ian.jackson
>>> On 25.02.19 at 21:50, <sstabellini@kernel.org> wrote:
> 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.
>
> Also, use __UINTPTR_TYPE__ for uintptr_t for consistency.
Please can you not mix adjustments to existing type definitions
into a patch adding a new type? __UINTPTR_TYPE__, as said in
prior discussions and other than __PTRDIFF_TYPE__, is not
provided unconditionally by gcc (and I didn't try to work out the
condition(s) so far), so use of it may introduce a regression
(manifesting as a build failure somewhere). Therefore, if we are
to indeed go this route, we should at least leave ourselves the
option of easily reverting one change without the other.
Jan
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel
^ permalink raw reply [flat|nested] 35+ messages in thread
* Re: [PATCH v10 5/6] xen/common: use DEFINE_SYMBOL as required
2019-02-25 20:50 ` [PATCH v10 5/6] xen/common: " Stefano Stabellini
@ 2019-02-26 15:13 ` Jan Beulich
2019-02-26 21:14 ` Stefano Stabellini
0 siblings, 1 reply; 35+ messages in thread
From: Jan Beulich @ 2019-02-26 15:13 UTC (permalink / raw)
To: Stefano Stabellini
Cc: Stefano Stabellini, Andrew Cooper, george.dunlap, Julien Grall,
ian.jackson, xen-devel
>>> On 25.02.19 at 21:50, <sstabellini@kernel.org> wrote:
> --- a/xen/common/kernel.c
> +++ b/xen/common/kernel.c
> @@ -306,20 +306,28 @@ void add_taint(unsigned int flag)
> tainted |= flag;
> }
>
> +/*
> + * We cannot use DEFINE_SYMBOL because we have three variables instead
> + * of two
> + */
> extern const initcall_t __initcall_start[], __presmp_initcall_end[],
> __initcall_end[];
But I did mention this specific case on the phone call we had: You
simply want to split this into two proper ranges,
(__initcall_start,__initcall_end) and
(__presmp_initcall_start,__presmp_initcall_end). Otherwise ...
> void __init do_presmp_initcalls(void)
> {
> const initcall_t *call;
> - for ( call = __initcall_start; call < __presmp_initcall_end; call++ )
> + for ( call = __initcall_start;
> + (uintptr_t)call < (uintptr_t)__presmp_initcall_end;
> + call++ )
> (*call)();
... you could as well use the open coded casting approach everywhere
(which I then would again grumble about).
> --- 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[];
> +DEFINE_SYMBOL(ctor_func_t, ctor_func, __ctors_start, __ctors_end);
At the example of this, there's too much redundancy here for my
taste. At least the _start and _end suffixes should be made
consistent across the code base (maybe except for the pseudo-
standard symbols like _etext and _edata). I'd also prefer if what
is now the first parameter would simply become <second>##_t.
There's nothing wrong with adding a few typedefs for this to work
in the common case.
> @@ -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 = (uintptr_t)__note_gnu_build_id_end - (uintptr_t)n;
This is another example of undue open coding. As also said on
the call we've had, it may be helpful to have a second diff
function for this, producing the byte difference instead of the
element one. In fact I did suggest to make the latter use the
former, such that the casting was truly limited to as few places
as possible.
Jan
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel
^ permalink raw reply [flat|nested] 35+ messages in thread
* Re: [PATCH v10 2/6] xen: introduce DEFINE_SYMBOL
2019-02-25 20:50 ` [PATCH v10 2/6] xen: introduce DEFINE_SYMBOL Stefano Stabellini
@ 2019-02-26 15:20 ` Jan Beulich
2019-02-26 18:54 ` Stefano Stabellini
2019-02-26 16:46 ` Ian Jackson
1 sibling, 1 reply; 35+ messages in thread
From: Jan Beulich @ 2019-02-26 15:20 UTC (permalink / raw)
To: Stefano Stabellini
Cc: Stefano Stabellini, Andrew Cooper, george.dunlap, Julien Grall,
ian.jackson, xen-devel
>>> On 25.02.19 at 21:50, <sstabellini@kernel.org> wrote:
> --- a/xen/include/xen/compiler.h
> +++ b/xen/include/xen/compiler.h
> @@ -99,6 +99,38 @@
> __asm__ ("" : "=r"(__ptr) : "0"(ptr)); \
> (typeof(ptr)) (__ptr + (off)); })
>
> +
> +/*
> + * Declare start and end array variables in C corresponding to existing
> + * linker symbols.
You validly say "declare" here, so why ...
> + * Two static inline functions are declared to do comparisons and
> + * subtractions between these variables.
> + *
> + * The end variable is declared with a different type to make sure that
> + * the static inline functions cannot be misused.
> + */
> +#define DEFINE_SYMBOL(type, name, start_name, end_name) \
... do you use DEFINE here?
How about DECLARE_ARRAY_BOUNDS(tag, name) using
tag ## _t as type, tag ## _lt etc as function names, and
name ## _start / name ## _end as start / end symbols. To
accommodate things like _etext, the above could in fact expand
to DECLARE_BOUNDS(tag, name ## _start, name ## _end)
allowing this second macro then to also be used like
DECLARE_BOUNDS(text, _stext, _etext).
Jan
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel
^ permalink raw reply [flat|nested] 35+ messages in thread
* Re: [PATCH v10 6/6] xen: use DEFINE_SYMBOL as required
2019-02-25 20:50 ` [PATCH v10 6/6] xen: " Stefano Stabellini
@ 2019-02-26 15:28 ` Jan Beulich
2019-02-26 21:22 ` Stefano Stabellini
0 siblings, 1 reply; 35+ messages in thread
From: Jan Beulich @ 2019-02-26 15:28 UTC (permalink / raw)
To: Stefano Stabellini
Cc: Stefano Stabellini, Andrew Cooper, george.dunlap, Julien Grall,
ian.jackson, xen-devel
>>> On 25.02.19 at 21:50, <sstabellini@kernel.org> wrote:
> @@ -210,7 +210,8 @@ static int __apply_alternatives_multi_stop(void *unused)
> region.begin = __alt_instructions;
> region.end = (struct alt_instr *)__alt_instructions_end;
>
> - ret = __apply_alternatives(®ion, xenmap - (void *)_start);
> + ret = __apply_alternatives(®ion, (uintptr_t)xenmap -
> + (uintptr_t)_start);
Undesirable (but in this case maybe indeed unavoidable) casting
instead. I don't think this belongs in a patch with the given title
though.
> @@ -78,7 +78,19 @@ void arch_livepatch_revert(const struct livepatch_func *func)
> uint32_t *new_ptr;
> unsigned int len;
>
> - new_ptr = func->old_addr - (void *)_start + vmap_of_xen_text;
> + /*
> + * We need to calculate the offset of the address from _start, and
> + * apply that to our own map, to find where we have this mapped.
> + * Doing these kind of games directly with pointers is contrary to
> + * the C rules for what pointers may be compared and computed. So
> + * we do the offset calculation with integers, which is always
> + * legal. The subsequent addition of the offset to the
> + * vmap_of_xen_text pointer is legal because the computed pointer is
> + * indeed a valid part of the object referred to by vmap_of_xen_text
> + * - namely, the byte array of our mapping of the Xen text.
> + */
> + new_ptr = ((uintptr_t)func->old_addr, - (uintptr_t)_start) +
> + vmap_of_xen_text;
You not using the intended helper inlines has allowed for a bug to
slip in that the compiler can't even help notice, due to - being both
a valid unary and a valid binary operator.
Jan
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel
^ permalink raw reply [flat|nested] 35+ messages in thread
* Re: [PATCH v10 1/6] xen: introduce ptrdiff_t
2019-02-25 20:50 ` [PATCH v10 1/6] xen: introduce ptrdiff_t Stefano Stabellini
2019-02-26 10:54 ` Jan Beulich
@ 2019-02-26 16:24 ` Ian Jackson
1 sibling, 0 replies; 35+ messages in thread
From: Ian Jackson @ 2019-02-26 16:24 UTC (permalink / raw)
To: Stefano Stabellini
Cc: Stefano Stabellini, Wei Liu, konrad.wilk, Andrew Cooper,
Tim (Xen.org),
George Dunlap, julien.grall, JBeulich, xen-devel
Stefano Stabellini writes ("[PATCH v10 1/6] xen: introduce ptrdiff_t"):
> 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.
>
> Also, use __UINTPTR_TYPE__ for uintptr_t for consistency.
>
> Signed-off-by: Stefano Stabellini <stefanos@xilinx.com>
Reviewed-by: Ian Jackson <ian.jackson@eu.citrix.com>
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel
^ permalink raw reply [flat|nested] 35+ messages in thread
* Re: [PATCH v10 2/6] xen: introduce DEFINE_SYMBOL
2019-02-25 20:50 ` [PATCH v10 2/6] xen: introduce DEFINE_SYMBOL Stefano Stabellini
2019-02-26 15:20 ` Jan Beulich
@ 2019-02-26 16:46 ` Ian Jackson
2019-02-26 17:13 ` Jan Beulich
1 sibling, 1 reply; 35+ messages in thread
From: Ian Jackson @ 2019-02-26 16:46 UTC (permalink / raw)
To: Stefano Stabellini
Cc: Stefano Stabellini, Andrew Cooper, George Dunlap, julien.grall,
JBeulich, Ian Jackson, xen-devel
Stefano Stabellini writes ("[PATCH v10 2/6] xen: introduce DEFINE_SYMBOL"):
> 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.
Firstly, sorry, but a formatting grumble:
The \ cause wrap damage on my 80-column terminal even before I quote
the email as I am doing now. Right now it looks like this:
> +extern const type start_name[]; \
\
> +extern const struct abstract_ ## name end_name[]; \
\
which is just awful.
I will remove some spaces from the quoted text so I can review it.
I'm not a hypervisor maintainer but I would appreciate it if you could
make the whole thing fit in 75 columns or so.
> +
> +/*
> + * Declare start and end array variables in C corresponding to existing
> + * linker symbols.
> + *
> + * Two static inline functions are declared to do comparisons and
> + * subtractions between these variables.
> + *
> + * The end variable is declared with a different type to make sure that
> + * the static inline functions cannot be misused.
> + */
> +#define DEFINE_SYMBOL(type, name, start_name, end_name) \
> + \
> +struct abstract_ ## name { \
> + type _; \
> +}; \
> + \
> +extern const type start_name[]; \
> +extern const struct abstract_ ## name end_name[]; \
I have thought of a problem with this approach.
This goes wrong unless `type' is a struct type. Because the compiler
is allowed to assume that end_name has the correct alignment for its
type. And in some ABIs, the alignment of a struct containing (say) a
char is bigger than that of a char. AIUI in some of the actual use
cases the linker-generated symbols may not be struct aligned.
I am not aware of a standard C type which could be used instead of
this struct. But I think you can use the `packed' attribute to get
the right behaviour. The GCC manual says:
| Alignment can be decreased by specifying the 'packed' attribute.
| See below.
Bizarrely, this seems only to be stated, slightly elliptically like
this, in the section on the `aligned' attribute; it's not mentioned in
`packed'. I suggest we couple this with a compile-time assertion that
alignof is the struct is the same as alignof the type.
(FTR I think this is a largely theoretical concern because most
current ABIs including all of Xen's specify that structs inherit the
alignment of the coarsest member.)
> +static inline bool name ## _lt(const type s1[], \
> + const struct abstract_ ## name s2[]) \
> +{ \
> + return (uintptr_t)s1 < (uintptr_t)s2; \
> +} \
This seems right to me.
> +static inline ptrdiff_t name ## _diff(const type s1[], \
> + const struct abstract_ ## name s2[])\
> +{ \
> + return ((uintptr_t)s2 - (uintptr_t)s1) / sizeof(*s1); \
This is wrong. The conversion to ptrdiff_t (currently done implicitly
by return) must come before the division. Otherwise it will give the
wrong answer when s1 > s2.
Suppose 32-bit, s1=0x00000040 s2=0x00000020 sizeof=0x10, Then
s2-s1=0xffffffe0, and unsigned division gives
(s2-s1)/sizeof=0x0ffffffe. Converstion to ptrdiff_t does not change
the bit pattern. But we wanted 0xffffffe.
Signed integer division by a positive divisor is always defined (and
always fits) so doing the conversion first is fine.
Regards,
Ian.
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel
^ permalink raw reply [flat|nested] 35+ messages in thread
* Re: [PATCH v10 3/6] xen/arm: use DEFINE_SYMBOL as required
2019-02-25 20:50 ` [PATCH v10 3/6] xen/arm: use DEFINE_SYMBOL as required Stefano Stabellini
@ 2019-02-26 17:11 ` Ian Jackson
2019-02-26 19:20 ` Stefano Stabellini
0 siblings, 1 reply; 35+ messages in thread
From: Ian Jackson @ 2019-02-26 17:11 UTC (permalink / raw)
To: Stefano Stabellini
Cc: Stefano Stabellini, Andrew Cooper, George Dunlap, julien.grall,
JBeulich, xen-devel
Stefano Stabellini writes ("[PATCH v10 3/6] xen/arm: use DEFINE_SYMBOL as required"):
> Use DEFINE_SYMBOL and the two static inline functions that come with it
> for comparisons and subtractions of:
>
> __init_begin, __init_end, __alt_instructions, __alt_instructions_end,
> __per_cpu_start, __per_cpu_data_end, _splatform, _eplatform, _sdevice,
> _edevice, _asdevice, _aedevice.
>
> 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
...
> -extern const struct alt_instr __alt_instructions[], __alt_instructions_end[];
> +DEFINE_SYMBOL(struct alt_instr, alt_instr, __alt_instructions,
> + __alt_instructions_end);
>
> struct alt_region {
> const struct alt_instr *begin;
> @@ -204,7 +208,7 @@ static int __apply_alternatives_multi_stop(void *unused)
> BUG_ON(!xenmap);
>
> region.begin = __alt_instructions;
> - region.end = __alt_instructions_end;
> + region.end = (struct alt_instr *)__alt_instructions_end;
I disapprove of this. It is hazardous to convert one of these
linker-generated end pointers to the underlying pointer type, because
that would allow accidental direct pointer comparison.
So, for example, here:
> @@ -131,7 +132,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;
> + (uintptr_t)alt < (uintptr_t)region->end;
> + alt++ )
If you missed out this hunk, the code would still compile. Also I
find the comment you have used opaque.
It is, in fact, false: the two pointers always do in fact point to the
same object. However, we know that compilers erroneously believe that
they do not. Obviously we don't want to put a complete discussion of
this issue next to each relevant use site.
I think what this demonstrates is that the macros in your patch 2 need
a big doc comment explaining (1) why this exists (2) what the rules
are. I suggest replacing the doc comment by the macro definition with
something like this:
/*
* 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.
*
* DEFINE_SYMBOL(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 errnoeously believe that no two
* external symbols can refer to the same array. They deem
* operations (eg 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 comparions 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.
*/
Having written all that down (what a palaver), we can see that your
cast, above, is a breach of the rules. Instead you can just pass the
struct abstract_alt_instr*, and all is well. Then you don't need a
comment at the use site, either, since you are doing things which are
entirely supported and explained.
> - 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] = (uintptr_t)p - (uintptr_t)__per_cpu_start;
If per_cpu_diff(__per_cpu_start, __per_cpu_data_end) gives the right
value for memset, isn't it the right value for offset[cpu] too ?
Ie I don't know why you are using uintptr_t here.
> @@ -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];
I also don't know why you are casting to char* here if you didn't need
to do so before.
The rest in this patch looks fine to me.
Thanks,
Ian.
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel
^ permalink raw reply [flat|nested] 35+ messages in thread
* Re: [PATCH v10 2/6] xen: introduce DEFINE_SYMBOL
2019-02-26 16:46 ` Ian Jackson
@ 2019-02-26 17:13 ` Jan Beulich
2019-02-26 17:32 ` Ian Jackson
0 siblings, 1 reply; 35+ messages in thread
From: Jan Beulich @ 2019-02-26 17:13 UTC (permalink / raw)
To: Ian Jackson
Cc: Stefano Stabellini, Stefano Stabellini, Andrew Cooper,
george.dunlap, Julien Grall, xen-devel
>>> On 26.02.19 at 17:46, <ian.jackson@citrix.com> wrote:
> Stefano Stabellini writes ("[PATCH v10 2/6] xen: introduce DEFINE_SYMBOL"):
>> +/*
>> + * Declare start and end array variables in C corresponding to existing
>> + * linker symbols.
>> + *
>> + * Two static inline functions are declared to do comparisons and
>> + * subtractions between these variables.
>> + *
>> + * The end variable is declared with a different type to make sure that
>> + * the static inline functions cannot be misused.
>> + */
>> +#define DEFINE_SYMBOL(type, name, start_name, end_name) \
>> + \
>> +struct abstract_ ## name { \
>> + type _; \
>> +}; \
>> + \
>> +extern const type start_name[]; \
>> +extern const struct abstract_ ## name end_name[]; \
>
> I have thought of a problem with this approach.
>
> This goes wrong unless `type' is a struct type. Because the compiler
> is allowed to assume that end_name has the correct alignment for its
> type. And in some ABIs, the alignment of a struct containing (say) a
> char is bigger than that of a char. AIUI in some of the actual use
> cases the linker-generated symbols may not be struct aligned.
>
> I am not aware of a standard C type which could be used instead of
> this struct. But I think you can use the `packed' attribute to get
> the right behaviour. The GCC manual says:
>
> | Alignment can be decreased by specifying the 'packed' attribute.
> | See below.
>
> Bizarrely, this seems only to be stated, slightly elliptically like
> this, in the section on the `aligned' attribute; it's not mentioned in
> `packed'. I suggest we couple this with a compile-time assertion that
> alignof is the struct is the same as alignof the type.
Until I've looked at this (again) now, I wasn't even aware that
one can combine packed and aligned attributes in a sensible
way. May I suggest that, because of this being a theoretical
issue only at this point, we limit ourselves to the build time
assertion you suggest?
>> +static inline bool name ## _lt(const type s1[], \
>> + const struct abstract_ ## name s2[])
> \
>> +{ \
>> + return (uintptr_t)s1 < (uintptr_t)s2; \
>> +} \
>
> This seems right to me.
>
>> +static inline ptrdiff_t name ## _diff(const type s1[], \
>> + const struct abstract_ ## name s2[])\
>> +{ \
>> + return ((uintptr_t)s2 - (uintptr_t)s1) / sizeof(*s1); \
>
> This is wrong. The conversion to ptrdiff_t (currently done implicitly
> by return) must come before the division. Otherwise it will give the
> wrong answer when s1 > s2.
>
> Suppose 32-bit, s1=0x00000040 s2=0x00000020 sizeof=0x10, Then
> s2-s1=0xffffffe0, and unsigned division gives
> (s2-s1)/sizeof=0x0ffffffe. Converstion to ptrdiff_t does not change
> the bit pattern. But we wanted 0xffffffe.
>
> Signed integer division by a positive divisor is always defined (and
> always fits) so doing the conversion first is fine.
Well, this would come as a side effect if there first was a function
producing the byte delta, and then the function here would call
that other function, doing the division on the result.
There's another caveat here though: Even by doing the cast first,
the division will still be unsigned as long as the sizeof() doesn't also
get cast to ptrdiff_t.
One question though is whether we actually care about the case
when s1 > s2 in the first place. But perhaps it's better to consider
it right away than getting bitten later on.
Jan
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel
^ permalink raw reply [flat|nested] 35+ messages in thread
* Re: [PATCH v10 4/6] xen/x86: use DEFINE_SYMBOL as required
2019-02-25 20:50 ` [PATCH v10 4/6] xen/x86: " Stefano Stabellini
@ 2019-02-26 17:13 ` Ian Jackson
2019-02-26 19:23 ` Stefano Stabellini
2019-02-26 17:16 ` Ian Jackson
1 sibling, 1 reply; 35+ messages in thread
From: Ian Jackson @ 2019-02-26 17:13 UTC (permalink / raw)
To: Stefano Stabellini
Cc: Stefano Stabellini, Andrew Cooper, George Dunlap, julien.grall,
JBeulich, xen-devel
Stefano Stabellini writes ("[PATCH v10 4/6] xen/x86: use DEFINE_SYMBOL as required"):
> Use SYMBOLS_SUBTRACT and SYMBOLS_COMPARE in cases of comparisons and
> subtractions of:
...
> Use explicit casts to uintptr_t when it is not possible to use the
> provided static inline functions.
Why is it not possible ? You write:
> +/*
> + * Cannot use DEFINE_SYMBOL because of the way they are passed to
> + * apply_alternatives.
> + */
> extern struct alt_instr __alt_instructions[], __alt_instructions_end[];
But I don't know why you can't pass a `struct abstract_alt_instr*' to
apply_alternatives.
IMO it should be strictly forbidden to ever write this formulation, as
you have above. See my proposed rule comment for DEFINE_SYMBOL.
Even if you can't use the macros at some particular calculation site,
you should still ensure that ..._end has a different type, to make
sure that no unsafe uses escape.
FTR, I have not reviewed the rest of this patch yet.
Ian.
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel
^ permalink raw reply [flat|nested] 35+ messages in thread
* Re: [PATCH v10 4/6] xen/x86: use DEFINE_SYMBOL as required
2019-02-25 20:50 ` [PATCH v10 4/6] xen/x86: " Stefano Stabellini
2019-02-26 17:13 ` Ian Jackson
@ 2019-02-26 17:16 ` Ian Jackson
1 sibling, 0 replies; 35+ messages in thread
From: Ian Jackson @ 2019-02-26 17:16 UTC (permalink / raw)
To: Stefano Stabellini
Cc: Stefano Stabellini, Andrew Cooper, George Dunlap, julien.grall,
JBeulich, xen-devel
Stefano Stabellini writes ("[PATCH v10 4/6] xen/x86: use DEFINE_SYMBOL as required"):
> Use SYMBOLS_SUBTRACT and SYMBOLS_COMPARE in cases of comparisons and
> subtractions of:
Oh and the commit message still mentions old macro names :-).
FTR I think your definition macro should be called SYMBOLS_DEFINE
since it defines two symbols rather than one, but I think this is a
very minor point and if you don't feel like changing it I certainly
won't object.
Regards,
Ian.
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel
^ permalink raw reply [flat|nested] 35+ messages in thread
* Re: [PATCH v10 2/6] xen: introduce DEFINE_SYMBOL
2019-02-26 17:13 ` Jan Beulich
@ 2019-02-26 17:32 ` Ian Jackson
2019-02-26 18:43 ` Stefano Stabellini
2019-02-27 8:27 ` Jan Beulich
0 siblings, 2 replies; 35+ messages in thread
From: Ian Jackson @ 2019-02-26 17:32 UTC (permalink / raw)
To: Jan Beulich
Cc: Stefano Stabellini, Stefano Stabellini, Andrew Cooper,
George Dunlap, Julien Grall, xen-devel
Jan Beulich writes ("Re: [PATCH v10 2/6] xen: introduce DEFINE_SYMBOL"):
> On 26.02.19 at 17:46, <ian.jackson@citrix.com> wrote:
> > I am not aware of a standard C type which could be used instead of
> > this struct. But I think you can use the `packed' attribute to get
> > the right behaviour. The GCC manual says:
> >
> > | Alignment can be decreased by specifying the 'packed' attribute.
> > | See below.
...
> Until I've looked at this (again) now, I wasn't even aware that
> one can combine packed and aligned attributes in a sensible
> way. May I suggest that, because of this being a theoretical
> issue only at this point, we limit ourselves to the build time
> assertion you suggest?
I am not suggesting combining `packed' and `aligned'. I am suggesting
only `packed' (but based on text which is in the manual section for
`aligned'). But I am happy with a build-time assertion if you don't
want to add `packed'. That is just as safe.
> > This is wrong. The conversion to ptrdiff_t (currently done implicitly
> > by return) must come before the division. Otherwise it will give the
> > wrong answer when s1 > s2.
> >
> > Suppose 32-bit, s1=0x00000040 s2=0x00000020 sizeof=0x10, Then
> > s2-s1=0xffffffe0, and unsigned division gives
> > (s2-s1)/sizeof=0x0ffffffe. Converstion to ptrdiff_t does not change
> > the bit pattern. But we wanted 0xffffffe.
> >
> > Signed integer division by a positive divisor is always defined (and
> > always fits) so doing the conversion first is fine.
>
> Well, this would come as a side effect if there first was a function
> producing the byte delta, and then the function here would call
> that other function, doing the division on the result.
I don't mind if someone wants to provide a byte delta function. It
ought to have a different name to `blah_diff' though. `blah_bytediff'
maybe.
> There's another caveat here though: Even by doing the cast first,
> the division will still be unsigned as long as the sizeof() doesn't also
> get cast to ptrdiff_t.
Yes, the sizeof would have to be cast to ptrdiff_t too.
> One question though is whether we actually care about the case
> when s1 > s2 in the first place. But perhaps it's better to consider
> it right away than getting bitten later on.
Having a thing which silently goes wrong giving bizarre and large
answers is clearly not acceptable...
Ian.
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel
^ permalink raw reply [flat|nested] 35+ messages in thread
* Re: [PATCH v10 2/6] xen: introduce DEFINE_SYMBOL
2019-02-26 17:32 ` Ian Jackson
@ 2019-02-26 18:43 ` Stefano Stabellini
2019-02-27 9:46 ` Jan Beulich
2019-02-27 8:27 ` Jan Beulich
1 sibling, 1 reply; 35+ messages in thread
From: Stefano Stabellini @ 2019-02-26 18:43 UTC (permalink / raw)
To: Ian Jackson
Cc: Stefano Stabellini, Stefano Stabellini, Andrew Cooper,
George Dunlap, Julien Grall, Jan Beulich, xen-devel
On Tue, 26 Feb 2019, Ian Jackson wrote:
> Jan Beulich writes ("Re: [PATCH v10 2/6] xen: introduce DEFINE_SYMBOL"):
> > On 26.02.19 at 17:46, <ian.jackson@citrix.com> wrote:
> > > I am not aware of a standard C type which could be used instead of
> > > this struct. But I think you can use the `packed' attribute to get
> > > the right behaviour. The GCC manual says:
> > >
> > > | Alignment can be decreased by specifying the 'packed' attribute.
> > > | See below.
> ...
> > Until I've looked at this (again) now, I wasn't even aware that
> > one can combine packed and aligned attributes in a sensible
> > way. May I suggest that, because of this being a theoretical
> > issue only at this point, we limit ourselves to the build time
> > assertion you suggest?
>
> I am not suggesting combining `packed' and `aligned'. I am suggesting
> only `packed' (but based on text which is in the manual section for
> `aligned'). But I am happy with a build-time assertion if you don't
> want to add `packed'. That is just as safe.
Could you please provide a rough example of the build-time assertion you
are thinking about? I am happy to add it.
> > > This is wrong. The conversion to ptrdiff_t (currently done implicitly
> > > by return) must come before the division. Otherwise it will give the
> > > wrong answer when s1 > s2.
> > >
> > > Suppose 32-bit, s1=0x00000040 s2=0x00000020 sizeof=0x10, Then
> > > s2-s1=0xffffffe0, and unsigned division gives
> > > (s2-s1)/sizeof=0x0ffffffe. Converstion to ptrdiff_t does not change
> > > the bit pattern. But we wanted 0xffffffe.
> > >
> > > Signed integer division by a positive divisor is always defined (and
> > > always fits) so doing the conversion first is fine.
> >
> > Well, this would come as a side effect if there first was a function
> > producing the byte delta, and then the function here would call
> > that other function, doing the division on the result.
>
> I don't mind if someone wants to provide a byte delta function. It
> ought to have a different name to `blah_diff' though. `blah_bytediff'
> maybe.
>
> > There's another caveat here though: Even by doing the cast first,
> > the division will still be unsigned as long as the sizeof() doesn't also
> > get cast to ptrdiff_t.
>
> Yes, the sizeof would have to be cast to ptrdiff_t too.
OK, it makes sense.
> > One question though is whether we actually care about the case
> > when s1 > s2 in the first place. But perhaps it's better to consider
> > it right away than getting bitten later on.
>
> Having a thing which silently goes wrong giving bizarre and large
> answers is clearly not acceptable...
Right
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel
^ permalink raw reply [flat|nested] 35+ messages in thread
* Re: [PATCH v10 2/6] xen: introduce DEFINE_SYMBOL
2019-02-26 15:20 ` Jan Beulich
@ 2019-02-26 18:54 ` Stefano Stabellini
2019-02-27 8:29 ` Jan Beulich
0 siblings, 1 reply; 35+ messages in thread
From: Stefano Stabellini @ 2019-02-26 18:54 UTC (permalink / raw)
To: Jan Beulich
Cc: Stefano Stabellini, Stefano Stabellini, Andrew Cooper,
george.dunlap, Julien Grall, ian.jackson, xen-devel
On Tue, 26 Feb 2019, Jan Beulich wrote:
> >>> On 25.02.19 at 21:50, <sstabellini@kernel.org> wrote:
> > --- a/xen/include/xen/compiler.h
> > +++ b/xen/include/xen/compiler.h
> > @@ -99,6 +99,38 @@
> > __asm__ ("" : "=r"(__ptr) : "0"(ptr)); \
> > (typeof(ptr)) (__ptr + (off)); })
> >
> > +
> > +/*
> > + * Declare start and end array variables in C corresponding to existing
> > + * linker symbols.
>
> You validly say "declare" here, so why ...
>
> > + * Two static inline functions are declared to do comparisons and
> > + * subtractions between these variables.
> > + *
> > + * The end variable is declared with a different type to make sure that
> > + * the static inline functions cannot be misused.
> > + */
> > +#define DEFINE_SYMBOL(type, name, start_name, end_name) \
>
> ... do you use DEFINE here?
>
> How about DECLARE_ARRAY_BOUNDS(tag, name) using
> tag ## _t as type, tag ## _lt etc as function names, and
> name ## _start / name ## _end as start / end symbols. To
> accommodate things like _etext, the above could in fact expand
> to DECLARE_BOUNDS(tag, name ## _start, name ## _end)
> allowing this second macro then to also be used like
> DECLARE_BOUNDS(text, _stext, _etext).
I am fine with renaming the macro. It is also fine to provide a wrapper
macro which automatically sets the most common variable names.
However, in your example both DECLARE_BOUNDS and DECLARE_ARRAY_BOUNDS
end up assuming that the type is tag ## _t. Currently many of our
variable types don't follow this naming pattern (struct alt_instr,
struct device_desc, struct acpi_device_desc, just to name the first
three I found). I don't think it is a good idea to introduce even more
renaming as part of this series. I suggest to do this instead:
#define DECLARE_ARRAY_BOUNDS(type, name) DECLARE_BOUNDS(type, name, name ## _start, name ## _end)
Where type is the type, name is used for abstract_ ## name, name ## _lt
and name ## _diff.
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel
^ permalink raw reply [flat|nested] 35+ messages in thread
* Re: [PATCH v10 3/6] xen/arm: use DEFINE_SYMBOL as required
2019-02-26 17:11 ` Ian Jackson
@ 2019-02-26 19:20 ` Stefano Stabellini
2019-02-27 9:56 ` Jan Beulich
0 siblings, 1 reply; 35+ messages in thread
From: Stefano Stabellini @ 2019-02-26 19:20 UTC (permalink / raw)
To: Ian Jackson
Cc: Stefano Stabellini, Stefano Stabellini, Andrew Cooper,
George Dunlap, julien.grall, JBeulich, xen-devel
On Tue, 26 Feb 2019, Ian Jackson wrote:
> Having written all that down (what a palaver), we can see that your
> cast, above, is a breach of the rules. Instead you can just pass the
> struct abstract_alt_instr*, and all is well. Then you don't need a
> comment at the use site, either, since you are doing things which are
> entirely supported and explained.
The in-code comment is great, and I have added it to the right patch.
Let me get a clarification on the rest of the suggestion: I would
have to change the type of region->end to const struct
abstract_alt_instr* (see xen/arch/arm/alternative.c).
If I do that, I get a compilation failure at:
alternative.c:245:16: error: initialization from incompatible pointer type [-Werror=incompatible-pointer-types]
.end = end,
because apply_alternatives currently expects two struct alt_instr* as
parameters. I cannot change the type of the second parameter of
apply_alternatives, because actually it might not be a linker symbol, it
might not be a struct abstract_alt_instr*. So I would still have to cast
to struct abstract_alt_instr* somewhere?
> > - 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] = (uintptr_t)p - (uintptr_t)__per_cpu_start;
>
> If per_cpu_diff(__per_cpu_start, __per_cpu_data_end) gives the right
> value for memset, isn't it the right value for offset[cpu] too ?
> Ie I don't know why you are using uintptr_t here.
I am using uintptr_t because p is not a abstract_per_cpu type. I could
do:
__per_cpu_offset[cpu] = per_cpu_diff(__per_cpu_start,
(struct abstract_per_cpu *)p);
I didn't think it was better though. What do you think?
> > @@ -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];
>
> I also don't know why you are casting to char* here if you didn't need
> to do so before.
That is because __per_cpu_start is now const, it wasn't before.
> The rest in this patch looks fine to me.
Thanks again for your help!
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel
^ permalink raw reply [flat|nested] 35+ messages in thread
* Re: [PATCH v10 4/6] xen/x86: use DEFINE_SYMBOL as required
2019-02-26 17:13 ` Ian Jackson
@ 2019-02-26 19:23 ` Stefano Stabellini
2019-02-27 9:45 ` Jan Beulich
0 siblings, 1 reply; 35+ messages in thread
From: Stefano Stabellini @ 2019-02-26 19:23 UTC (permalink / raw)
To: Ian Jackson
Cc: Stefano Stabellini, Stefano Stabellini, Andrew Cooper,
George Dunlap, julien.grall, JBeulich, xen-devel
On Tue, 26 Feb 2019, Ian Jackson wrote:
> Stefano Stabellini writes ("[PATCH v10 4/6] xen/x86: use DEFINE_SYMBOL as required"):
> > Use SYMBOLS_SUBTRACT and SYMBOLS_COMPARE in cases of comparisons and
> > subtractions of:
> ...
> > Use explicit casts to uintptr_t when it is not possible to use the
> > provided static inline functions.
>
> Why is it not possible ? You write:
>
> > +/*
> > + * Cannot use DEFINE_SYMBOL because of the way they are passed to
> > + * apply_alternatives.
> > + */
> > extern struct alt_instr __alt_instructions[], __alt_instructions_end[];
>
> But I don't know why you can't pass a `struct abstract_alt_instr*' to
> apply_alternatives.
>
> IMO it should be strictly forbidden to ever write this formulation, as
> you have above. See my proposed rule comment for DEFINE_SYMBOL.
>
> Even if you can't use the macros at some particular calculation site,
> you should still ensure that ..._end has a different type, to make
> sure that no unsafe uses escape.
Unfortunately __apply_alternatives is called from
__apply_alternatives_multi_stop, where it would be fine to use struct
abstract_alt_instr*, and also from apply_alternatives which in a
xen/common interface called from xen/common/livepatch.c and doesn't work
with linker symbols AFAICT.
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel
^ permalink raw reply [flat|nested] 35+ messages in thread
* Re: [PATCH v10 5/6] xen/common: use DEFINE_SYMBOL as required
2019-02-26 15:13 ` Jan Beulich
@ 2019-02-26 21:14 ` Stefano Stabellini
2019-02-27 8:32 ` Jan Beulich
0 siblings, 1 reply; 35+ messages in thread
From: Stefano Stabellini @ 2019-02-26 21:14 UTC (permalink / raw)
To: Jan Beulich
Cc: Stefano Stabellini, Stefano Stabellini, Andrew Cooper,
george.dunlap, Julien Grall, ian.jackson, xen-devel
On Tue, 26 Feb 2019, Jan Beulich wrote:
> >>> On 25.02.19 at 21:50, <sstabellini@kernel.org> wrote:
> > --- a/xen/common/kernel.c
> > +++ b/xen/common/kernel.c
> > @@ -306,20 +306,28 @@ void add_taint(unsigned int flag)
> > tainted |= flag;
> > }
> >
> > +/*
> > + * We cannot use DEFINE_SYMBOL because we have three variables instead
> > + * of two
> > + */
> > extern const initcall_t __initcall_start[], __presmp_initcall_end[],
> > __initcall_end[];
>
> But I did mention this specific case on the phone call we had: You
> simply want to split this into two proper ranges,
> (__initcall_start,__initcall_end) and
> (__presmp_initcall_start,__presmp_initcall_end). Otherwise ...
>
> > void __init do_presmp_initcalls(void)
> > {
> > const initcall_t *call;
> > - for ( call = __initcall_start; call < __presmp_initcall_end; call++ )
> > + for ( call = __initcall_start;
> > + (uintptr_t)call < (uintptr_t)__presmp_initcall_end;
> > + call++ )
> > (*call)();
>
> ... you could as well use the open coded casting approach everywhere
> (which I then would again grumble about).
I'll follow your suggestion of splitting the ranges.
> > --- 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[];
> > +DEFINE_SYMBOL(ctor_func_t, ctor_func, __ctors_start, __ctors_end);
>
> At the example of this, there's too much redundancy here for my
> taste. At least the _start and _end suffixes should be made
> consistent across the code base (maybe except for the pseudo-
> standard symbols like _etext and _edata). I'd also prefer if what
> is now the first parameter would simply become <second>##_t.
> There's nothing wrong with adding a few typedefs for this to work
> in the common case.
I understand your point but I would prefer to avoid changing the
existing types or names. Instead, I could add a wrapper around
DEFINE_SYMBOL or DECLARE_BOUNDS as you suggested, see my other reply
https://marc.info/?l=xen-devel&m=155120735032147.
However, this example doesn't actually follow the regular pattern
unfortunately (__ctors_start != ctors_func_start). I would like to avoid
making all names/types follow the regular pattern as part of this
series. I could do as a clean-up afterwards.
> > @@ -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 = (uintptr_t)__note_gnu_build_id_end - (uintptr_t)n;
>
> This is another example of undue open coding. As also said on
> the call we've had, it may be helpful to have a second diff
> function for this, producing the byte difference instead of the
> element one. In fact I did suggest to make the latter use the
> former, such that the casting was truly limited to as few places
> as possible.
I considered adding a second function, but this is the only instance we
have today, so I decided to skip it. I am OK with adding the separate
function though, let me know.
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel
^ permalink raw reply [flat|nested] 35+ messages in thread
* Re: [PATCH v10 6/6] xen: use DEFINE_SYMBOL as required
2019-02-26 15:28 ` Jan Beulich
@ 2019-02-26 21:22 ` Stefano Stabellini
2019-02-27 8:43 ` Jan Beulich
0 siblings, 1 reply; 35+ messages in thread
From: Stefano Stabellini @ 2019-02-26 21:22 UTC (permalink / raw)
To: Jan Beulich
Cc: Stefano Stabellini, Stefano Stabellini, Andrew Cooper,
george.dunlap, Julien Grall, ian.jackson, xen-devel
On Tue, 26 Feb 2019, Jan Beulich wrote:
> >>> On 25.02.19 at 21:50, <sstabellini@kernel.org> wrote:
> > @@ -210,7 +210,8 @@ static int __apply_alternatives_multi_stop(void *unused)
> > region.begin = __alt_instructions;
> > region.end = (struct alt_instr *)__alt_instructions_end;
> >
> > - ret = __apply_alternatives(®ion, xenmap - (void *)_start);
> > + ret = __apply_alternatives(®ion, (uintptr_t)xenmap -
> > + (uintptr_t)_start);
>
> Undesirable (but in this case maybe indeed unavoidable) casting
> instead. I don't think this belongs in a patch with the given title
> though.
It's in this patch because this is the patch dealing with _start and
_end. Let me know how would you like the patches to be split.
> > @@ -78,7 +78,19 @@ void arch_livepatch_revert(const struct livepatch_func *func)
> > uint32_t *new_ptr;
> > unsigned int len;
> >
> > - new_ptr = func->old_addr - (void *)_start + vmap_of_xen_text;
> > + /*
> > + * We need to calculate the offset of the address from _start, and
> > + * apply that to our own map, to find where we have this mapped.
> > + * Doing these kind of games directly with pointers is contrary to
> > + * the C rules for what pointers may be compared and computed. So
> > + * we do the offset calculation with integers, which is always
> > + * legal. The subsequent addition of the offset to the
> > + * vmap_of_xen_text pointer is legal because the computed pointer is
> > + * indeed a valid part of the object referred to by vmap_of_xen_text
> > + * - namely, the byte array of our mapping of the Xen text.
> > + */
> > + new_ptr = ((uintptr_t)func->old_addr, - (uintptr_t)_start) +
> > + vmap_of_xen_text;
>
> You not using the intended helper inlines has allowed for a bug to
> slip in that the compiler can't even help notice, due to - being both
> a valid unary and a valid binary operator.
Well spotted! I'll fix the bug. I would also be happy to use the helper
inlines, but we discussed not to use them in cases like this, with three
operators. Even if I wanted to use them, none of the inline helpers fit
this case. Or do you suggest:
new_ptr = xen_diff(_start, (struct abstract_xen *)func->old_addr) +
vmap_of_xen_text;
Is that what you are asking?
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel
^ permalink raw reply [flat|nested] 35+ messages in thread
* Re: [PATCH v10 2/6] xen: introduce DEFINE_SYMBOL
2019-02-26 17:32 ` Ian Jackson
2019-02-26 18:43 ` Stefano Stabellini
@ 2019-02-27 8:27 ` Jan Beulich
1 sibling, 0 replies; 35+ messages in thread
From: Jan Beulich @ 2019-02-27 8:27 UTC (permalink / raw)
To: Ian Jackson
Cc: Stefano Stabellini, Stefano Stabellini, Andrew Cooper,
george.dunlap, Julien Grall, xen-devel
>>> On 26.02.19 at 18:32, <ian.jackson@citrix.com> wrote:
> Jan Beulich writes ("Re: [PATCH v10 2/6] xen: introduce DEFINE_SYMBOL"):
>> On 26.02.19 at 17:46, <ian.jackson@citrix.com> wrote:
>> > I am not aware of a standard C type which could be used instead of
>> > this struct. But I think you can use the `packed' attribute to get
>> > the right behaviour. The GCC manual says:
>> >
>> > | Alignment can be decreased by specifying the 'packed' attribute.
>> > | See below.
> ...
>> Until I've looked at this (again) now, I wasn't even aware that
>> one can combine packed and aligned attributes in a sensible
>> way. May I suggest that, because of this being a theoretical
>> issue only at this point, we limit ourselves to the build time
>> assertion you suggest?
>
> I am not suggesting combining `packed' and `aligned'. I am suggesting
> only `packed' (but based on text which is in the manual section for
> `aligned'). But I am happy with a build-time assertion if you don't
> want to add `packed'. That is just as safe.
But "packed" alone reduces alignment to the minimum, i.e. one byte.
That would immediately break the build time assertion you've suggested
to add.
Jan
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel
^ permalink raw reply [flat|nested] 35+ messages in thread
* Re: [PATCH v10 2/6] xen: introduce DEFINE_SYMBOL
2019-02-26 18:54 ` Stefano Stabellini
@ 2019-02-27 8:29 ` Jan Beulich
0 siblings, 0 replies; 35+ messages in thread
From: Jan Beulich @ 2019-02-27 8:29 UTC (permalink / raw)
To: Stefano Stabellini
Cc: Stefano Stabellini, Andrew Cooper, george.dunlap, Julien Grall,
ian.jackson, xen-devel
>>> On 26.02.19 at 19:54, <sstabellini@kernel.org> wrote:
> On Tue, 26 Feb 2019, Jan Beulich wrote:
>> >>> On 25.02.19 at 21:50, <sstabellini@kernel.org> wrote:
>> > --- a/xen/include/xen/compiler.h
>> > +++ b/xen/include/xen/compiler.h
>> > @@ -99,6 +99,38 @@
>> > __asm__ ("" : "=r"(__ptr) : "0"(ptr)); \
>> > (typeof(ptr)) (__ptr + (off)); })
>> >
>> > +
>> > +/*
>> > + * Declare start and end array variables in C corresponding to existing
>> > + * linker symbols.
>>
>> You validly say "declare" here, so why ...
>>
>> > + * Two static inline functions are declared to do comparisons and
>> > + * subtractions between these variables.
>> > + *
>> > + * The end variable is declared with a different type to make sure that
>> > + * the static inline functions cannot be misused.
>> > + */
>> > +#define DEFINE_SYMBOL(type, name, start_name, end_name)
> \
>>
>> ... do you use DEFINE here?
>>
>> How about DECLARE_ARRAY_BOUNDS(tag, name) using
>> tag ## _t as type, tag ## _lt etc as function names, and
>> name ## _start / name ## _end as start / end symbols. To
>> accommodate things like _etext, the above could in fact expand
>> to DECLARE_BOUNDS(tag, name ## _start, name ## _end)
>> allowing this second macro then to also be used like
>> DECLARE_BOUNDS(text, _stext, _etext).
>
> I am fine with renaming the macro. It is also fine to provide a wrapper
> macro which automatically sets the most common variable names.
>
> However, in your example both DECLARE_BOUNDS and DECLARE_ARRAY_BOUNDS
> end up assuming that the type is tag ## _t. Currently many of our
> variable types don't follow this naming pattern (struct alt_instr,
> struct device_desc, struct acpi_device_desc, just to name the first
> three I found). I don't think it is a good idea to introduce even more
> renaming as part of this series.
I didn't suggest renaming anything. Instead I've suggested to add
typedef-s as needed.
Jan
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel
^ permalink raw reply [flat|nested] 35+ messages in thread
* Re: [PATCH v10 5/6] xen/common: use DEFINE_SYMBOL as required
2019-02-26 21:14 ` Stefano Stabellini
@ 2019-02-27 8:32 ` Jan Beulich
0 siblings, 0 replies; 35+ messages in thread
From: Jan Beulich @ 2019-02-27 8:32 UTC (permalink / raw)
To: Stefano Stabellini
Cc: Stefano Stabellini, Andrew Cooper, george.dunlap, Julien Grall,
ian.jackson, xen-devel
>>> On 26.02.19 at 22:14, <sstabellini@kernel.org> wrote:
> On Tue, 26 Feb 2019, Jan Beulich wrote:
>> >>> On 25.02.19 at 21:50, <sstabellini@kernel.org> wrote:
>> > --- 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[];
>> > +DEFINE_SYMBOL(ctor_func_t, ctor_func, __ctors_start, __ctors_end);
>>
>> At the example of this, there's too much redundancy here for my
>> taste. At least the _start and _end suffixes should be made
>> consistent across the code base (maybe except for the pseudo-
>> standard symbols like _etext and _edata). I'd also prefer if what
>> is now the first parameter would simply become <second>##_t.
>> There's nothing wrong with adding a few typedefs for this to work
>> in the common case.
>
> I understand your point but I would prefer to avoid changing the
> existing types or names. Instead, I could add a wrapper around
> DEFINE_SYMBOL or DECLARE_BOUNDS as you suggested, see my other reply
> https://marc.info/?l=xen-devel&m=155120735032147.
>
> However, this example doesn't actually follow the regular pattern
> unfortunately (__ctors_start != ctors_func_start). I would like to avoid
> making all names/types follow the regular pattern as part of this
> series. I could do as a clean-up afterwards.
Personally I think the bringing in line with the intended common
pattern should be a prereq patch (or several of them if need be)
in this series.
>> > @@ -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 = (uintptr_t)__note_gnu_build_id_end - (uintptr_t)n;
>>
>> This is another example of undue open coding. As also said on
>> the call we've had, it may be helpful to have a second diff
>> function for this, producing the byte difference instead of the
>> element one. In fact I did suggest to make the latter use the
>> former, such that the casting was truly limited to as few places
>> as possible.
>
> I considered adding a second function, but this is the only instance we
> have today, so I decided to skip it. I am OK with adding the separate
> function though, let me know.
Well, as per the discussion also with Ian on patch 2, there's
independent benefit of having that extra function. So yes, I
continue to think it should be introduced.
Jan
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel
^ permalink raw reply [flat|nested] 35+ messages in thread
* Re: [PATCH v10 6/6] xen: use DEFINE_SYMBOL as required
2019-02-26 21:22 ` Stefano Stabellini
@ 2019-02-27 8:43 ` Jan Beulich
2019-03-01 21:48 ` Stefano Stabellini
0 siblings, 1 reply; 35+ messages in thread
From: Jan Beulich @ 2019-02-27 8:43 UTC (permalink / raw)
To: Stefano Stabellini
Cc: Stefano Stabellini, Andrew Cooper, george.dunlap, Julien Grall,
ian.jackson, xen-devel
>>> On 26.02.19 at 22:22, <sstabellini@kernel.org> wrote:
> On Tue, 26 Feb 2019, Jan Beulich wrote:
>> >>> On 25.02.19 at 21:50, <sstabellini@kernel.org> wrote:
>> > @@ -210,7 +210,8 @@ static int __apply_alternatives_multi_stop(void *unused)
>> > region.begin = __alt_instructions;
>> > region.end = (struct alt_instr *)__alt_instructions_end;
>> >
>> > - ret = __apply_alternatives(®ion, xenmap - (void *)_start);
>> > + ret = __apply_alternatives(®ion, (uintptr_t)xenmap -
>> > + (uintptr_t)_start);
>>
>> Undesirable (but in this case maybe indeed unavoidable) casting
>> instead. I don't think this belongs in a patch with the given title
>> though.
>
> It's in this patch because this is the patch dealing with _start and
> _end. Let me know how would you like the patches to be split.
Well, I can see the general possible need for additional changes
due to the type adjustments. I don't see though why the original
code in this example would break with the other adjustments done
here. Things need to build and work after each patch, but changes
not strictly needed and not related to the subject of a patch would
better be split out (in this case into the [or another] Arm-specific
patch).
>> > @@ -78,7 +78,19 @@ void arch_livepatch_revert(const struct livepatch_func *func)
>> > uint32_t *new_ptr;
>> > unsigned int len;
>> >
>> > - new_ptr = func->old_addr - (void *)_start + vmap_of_xen_text;
>> > + /*
>> > + * We need to calculate the offset of the address from _start, and
>> > + * apply that to our own map, to find where we have this mapped.
>> > + * Doing these kind of games directly with pointers is contrary to
>> > + * the C rules for what pointers may be compared and computed. So
>> > + * we do the offset calculation with integers, which is always
>> > + * legal. The subsequent addition of the offset to the
>> > + * vmap_of_xen_text pointer is legal because the computed pointer is
>> > + * indeed a valid part of the object referred to by vmap_of_xen_text
>> > + * - namely, the byte array of our mapping of the Xen text.
>> > + */
>> > + new_ptr = ((uintptr_t)func->old_addr, - (uintptr_t)_start) +
>> > + vmap_of_xen_text;
>>
>> You not using the intended helper inlines has allowed for a bug to
>> slip in that the compiler can't even help notice, due to - being both
>> a valid unary and a valid binary operator.
>
> Well spotted! I'll fix the bug. I would also be happy to use the helper
> inlines, but we discussed not to use them in cases like this, with three
> operators. Even if I wanted to use them, none of the inline helpers fit
> this case. Or do you suggest:
>
> new_ptr = xen_diff(_start, (struct abstract_xen *)func->old_addr) +
> vmap_of_xen_text;
>
> Is that what you are asking?
No matter what, it looks like you're wanting (and perhaps needing) to
stick to some form of cast here. But that's what you're specifically
trying to get away from, aren't you? What is MISRA's position on
casts from void * to a type? This is not a generally "safe" operation
after all, because the casted to type could be out of sync with the
object the pointer points at.
Note that struct livepatch_func only happens to live in the public
interface, but the type of old_addr ought to be freely changeable as
long as it remains a pointer. Did you check whether changing it would
help avoid all casting?
Jan
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel
^ permalink raw reply [flat|nested] 35+ messages in thread
* Re: [PATCH v10 4/6] xen/x86: use DEFINE_SYMBOL as required
2019-02-26 19:23 ` Stefano Stabellini
@ 2019-02-27 9:45 ` Jan Beulich
0 siblings, 0 replies; 35+ messages in thread
From: Jan Beulich @ 2019-02-27 9:45 UTC (permalink / raw)
To: Ian Jackson
Cc: Stefano Stabellini, Stefano Stabellini, Andrew Cooper,
george.dunlap, Julien Grall, xen-devel
>>> On 26.02.19 at 20:23, <sstabellini@kernel.org> wrote:
> On Tue, 26 Feb 2019, Ian Jackson wrote:
>> Stefano Stabellini writes ("[PATCH v10 4/6] xen/x86: use DEFINE_SYMBOL as required"):
>> > Use SYMBOLS_SUBTRACT and SYMBOLS_COMPARE in cases of comparisons and
>> > subtractions of:
>> ...
>> > Use explicit casts to uintptr_t when it is not possible to use the
>> > provided static inline functions.
>>
>> Why is it not possible ? You write:
>>
>> > +/*
>> > + * Cannot use DEFINE_SYMBOL because of the way they are passed to
>> > + * apply_alternatives.
>> > + */
>> > extern struct alt_instr __alt_instructions[], __alt_instructions_end[];
>>
>> But I don't know why you can't pass a `struct abstract_alt_instr*' to
>> apply_alternatives.
>>
>> IMO it should be strictly forbidden to ever write this formulation, as
>> you have above. See my proposed rule comment for DEFINE_SYMBOL.
>>
>> Even if you can't use the macros at some particular calculation site,
>> you should still ensure that ..._end has a different type, to make
>> sure that no unsafe uses escape.
>
> Unfortunately __apply_alternatives is called from
> __apply_alternatives_multi_stop, where it would be fine to use struct
> abstract_alt_instr*, and also from apply_alternatives which in a
> xen/common interface called from xen/common/livepatch.c and doesn't work
> with linker symbols AFAICT.
As Ian says, it's only a matter of correctly defining the type of "end" at
the call site.
Jan
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel
^ permalink raw reply [flat|nested] 35+ messages in thread
* Re: [PATCH v10 2/6] xen: introduce DEFINE_SYMBOL
2019-02-26 18:43 ` Stefano Stabellini
@ 2019-02-27 9:46 ` Jan Beulich
2019-02-28 0:05 ` Stefano Stabellini
0 siblings, 1 reply; 35+ messages in thread
From: Jan Beulich @ 2019-02-27 9:46 UTC (permalink / raw)
To: Stefano Stabellini
Cc: Stefano Stabellini, Andrew Cooper, george.dunlap, Julien Grall,
Ian Jackson, xen-devel
>>> On 26.02.19 at 19:43, <sstabellini@kernel.org> wrote:
> On Tue, 26 Feb 2019, Ian Jackson wrote:
>> Jan Beulich writes ("Re: [PATCH v10 2/6] xen: introduce DEFINE_SYMBOL"):
>> > On 26.02.19 at 17:46, <ian.jackson@citrix.com> wrote:
>> > > I am not aware of a standard C type which could be used instead of
>> > > this struct. But I think you can use the `packed' attribute to get
>> > > the right behaviour. The GCC manual says:
>> > >
>> > > | Alignment can be decreased by specifying the 'packed' attribute.
>> > > | See below.
>> ...
>> > Until I've looked at this (again) now, I wasn't even aware that
>> > one can combine packed and aligned attributes in a sensible
>> > way. May I suggest that, because of this being a theoretical
>> > issue only at this point, we limit ourselves to the build time
>> > assertion you suggest?
>>
>> I am not suggesting combining `packed' and `aligned'. I am suggesting
>> only `packed' (but based on text which is in the manual section for
>> `aligned'). But I am happy with a build-time assertion if you don't
>> want to add `packed'. That is just as safe.
>
> Could you please provide a rough example of the build-time assertion you
> are thinking about? I am happy to add it.
BUILD_BUG_ON(alignof(*s1) != alignof(*s2));
Jan
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel
^ permalink raw reply [flat|nested] 35+ messages in thread
* Re: [PATCH v10 3/6] xen/arm: use DEFINE_SYMBOL as required
2019-02-26 19:20 ` Stefano Stabellini
@ 2019-02-27 9:56 ` Jan Beulich
2019-02-28 23:43 ` Stefano Stabellini
0 siblings, 1 reply; 35+ messages in thread
From: Jan Beulich @ 2019-02-27 9:56 UTC (permalink / raw)
To: Stefano Stabellini
Cc: Stefano Stabellini, Andrew Cooper, george.dunlap, Julien Grall,
Ian Jackson, xen-devel
>>> On 26.02.19 at 20:20, <sstabellini@kernel.org> wrote:
> On Tue, 26 Feb 2019, Ian Jackson wrote:
>> Having written all that down (what a palaver), we can see that your
>> cast, above, is a breach of the rules. Instead you can just pass the
>> struct abstract_alt_instr*, and all is well. Then you don't need a
>> comment at the use site, either, since you are doing things which are
>> entirely supported and explained.
>
> The in-code comment is great, and I have added it to the right patch.
>
> Let me get a clarification on the rest of the suggestion: I would
> have to change the type of region->end to const struct
> abstract_alt_instr* (see xen/arch/arm/alternative.c).
>
> If I do that, I get a compilation failure at:
>
> alternative.c:245:16: error: initialization from incompatible pointer type
> [-Werror=incompatible-pointer-types]
> .end = end,
>
> because apply_alternatives currently expects two struct alt_instr* as
> parameters. I cannot change the type of the second parameter of
> apply_alternatives, because actually it might not be a linker symbol, it
> might not be a struct abstract_alt_instr*. So I would still have to cast
> to struct abstract_alt_instr* somewhere?
I don't think so, no. In livepatch.c we have
end = sec->load_addr + sec->sec->sh_size;
which (a) is effectively a linker defined symbol, just that we don't
obtain it through a linker script and it also has no name associated
with it and (b) will be fine without any casts thanks to load_addr
being of type void * (i.e. the type of "end" can freely change).
>> > - 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] = (uintptr_t)p - (uintptr_t)__per_cpu_start;
>>
>> If per_cpu_diff(__per_cpu_start, __per_cpu_data_end) gives the right
>> value for memset, isn't it the right value for offset[cpu] too ?
>> Ie I don't know why you are using uintptr_t here.
>
> I am using uintptr_t because p is not a abstract_per_cpu type. I could
> do:
>
> __per_cpu_offset[cpu] = per_cpu_diff(__per_cpu_start,
> (struct abstract_per_cpu *)p);
>
> I didn't think it was better though. What do you think?
Did you try changing p's type? That said, the calculation isn't going
to be universally correct (in MISRA's sense), no matter what you do:
We _deliberately_ subtract two entities here which are _guaranteed_
not to point to the same object (or one past an array's last element).
>> > @@ -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];
>>
>> I also don't know why you are casting to char* here if you didn't need
>> to do so before.
>
> That is because __per_cpu_start is now const, it wasn't before.
That's why I'm advocating that free()-style functions should take
pointers to const. A patch to this effect wasn't liked, though.
Jan
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel
^ permalink raw reply [flat|nested] 35+ messages in thread
* Re: [PATCH v10 2/6] xen: introduce DEFINE_SYMBOL
2019-02-27 9:46 ` Jan Beulich
@ 2019-02-28 0:05 ` Stefano Stabellini
2019-02-28 10:33 ` Jan Beulich
0 siblings, 1 reply; 35+ messages in thread
From: Stefano Stabellini @ 2019-02-28 0:05 UTC (permalink / raw)
To: Jan Beulich
Cc: Stefano Stabellini, Stefano Stabellini, Andrew Cooper,
george.dunlap, Julien Grall, Ian Jackson, xen-devel
On Wed, 27 Feb 2019, Jan Beulich wrote:
> >>> On 26.02.19 at 19:43, <sstabellini@kernel.org> wrote:
> > On Tue, 26 Feb 2019, Ian Jackson wrote:
> >> Jan Beulich writes ("Re: [PATCH v10 2/6] xen: introduce DEFINE_SYMBOL"):
> >> > On 26.02.19 at 17:46, <ian.jackson@citrix.com> wrote:
> >> > > I am not aware of a standard C type which could be used instead of
> >> > > this struct. But I think you can use the `packed' attribute to get
> >> > > the right behaviour. The GCC manual says:
> >> > >
> >> > > | Alignment can be decreased by specifying the 'packed' attribute.
> >> > > | See below.
> >> ...
> >> > Until I've looked at this (again) now, I wasn't even aware that
> >> > one can combine packed and aligned attributes in a sensible
> >> > way. May I suggest that, because of this being a theoretical
> >> > issue only at this point, we limit ourselves to the build time
> >> > assertion you suggest?
> >>
> >> I am not suggesting combining `packed' and `aligned'. I am suggesting
> >> only `packed' (but based on text which is in the manual section for
> >> `aligned'). But I am happy with a build-time assertion if you don't
> >> want to add `packed'. That is just as safe.
> >
> > Could you please provide a rough example of the build-time assertion you
> > are thinking about? I am happy to add it.
>
> BUILD_BUG_ON(alignof(*s1) != alignof(*s2));
Thanks! I noticed that BUILD_BUG_ON requires xen/lib.h which cannot be
included from xen/compiler.h. Actually, we were already erroneously
using BUILD_BUG_ON_ZERO in xen/compiler.h without including xen/lib.h.
My suggestion would be to move the definitions of BUG_ON, WARN_ON,
BUILD_BUG_ON and BUILD_BUG_ON_ZERO from xen/lib.h to compiler.h.
Everything works fine if I do that, and it seems a better fit.
Are you OK with this?
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel
^ permalink raw reply [flat|nested] 35+ messages in thread
* Re: [PATCH v10 2/6] xen: introduce DEFINE_SYMBOL
2019-02-28 0:05 ` Stefano Stabellini
@ 2019-02-28 10:33 ` Jan Beulich
0 siblings, 0 replies; 35+ messages in thread
From: Jan Beulich @ 2019-02-28 10:33 UTC (permalink / raw)
To: Stefano Stabellini
Cc: Stefano Stabellini, Andrew Cooper, george.dunlap, Julien Grall,
Ian Jackson, xen-devel
>>> On 28.02.19 at 01:05, <sstabellini@kernel.org> wrote:
> On Wed, 27 Feb 2019, Jan Beulich wrote:
>> >>> On 26.02.19 at 19:43, <sstabellini@kernel.org> wrote:
>> > On Tue, 26 Feb 2019, Ian Jackson wrote:
>> >> Jan Beulich writes ("Re: [PATCH v10 2/6] xen: introduce DEFINE_SYMBOL"):
>> >> > On 26.02.19 at 17:46, <ian.jackson@citrix.com> wrote:
>> >> > > I am not aware of a standard C type which could be used instead of
>> >> > > this struct. But I think you can use the `packed' attribute to get
>> >> > > the right behaviour. The GCC manual says:
>> >> > >
>> >> > > | Alignment can be decreased by specifying the 'packed' attribute.
>> >> > > | See below.
>> >> ...
>> >> > Until I've looked at this (again) now, I wasn't even aware that
>> >> > one can combine packed and aligned attributes in a sensible
>> >> > way. May I suggest that, because of this being a theoretical
>> >> > issue only at this point, we limit ourselves to the build time
>> >> > assertion you suggest?
>> >>
>> >> I am not suggesting combining `packed' and `aligned'. I am suggesting
>> >> only `packed' (but based on text which is in the manual section for
>> >> `aligned'). But I am happy with a build-time assertion if you don't
>> >> want to add `packed'. That is just as safe.
>> >
>> > Could you please provide a rough example of the build-time assertion you
>> > are thinking about? I am happy to add it.
>>
>> BUILD_BUG_ON(alignof(*s1) != alignof(*s2));
>
> Thanks! I noticed that BUILD_BUG_ON requires xen/lib.h which cannot be
> included from xen/compiler.h. Actually, we were already erroneously
> using BUILD_BUG_ON_ZERO in xen/compiler.h without including xen/lib.h.
>
> My suggestion would be to move the definitions of BUG_ON, WARN_ON,
> BUILD_BUG_ON and BUILD_BUG_ON_ZERO from xen/lib.h to compiler.h.
> Everything works fine if I do that, and it seems a better fit.
>
> Are you OK with this?
Better fit or not, if it becomes a requirement, such movement ought to
be okay. However, why would you use these in compiler.h? The new
macro you define doesn't belong there (as it has nothing to do with
how we interface with the compiler) - did I overlook something there?
xen/lib.h would actually seem to be one sensible place to put it; there
may be other sensible candidates.
Jan
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel
^ permalink raw reply [flat|nested] 35+ messages in thread
* Re: [PATCH v10 3/6] xen/arm: use DEFINE_SYMBOL as required
2019-02-27 9:56 ` Jan Beulich
@ 2019-02-28 23:43 ` Stefano Stabellini
0 siblings, 0 replies; 35+ messages in thread
From: Stefano Stabellini @ 2019-02-28 23:43 UTC (permalink / raw)
To: Jan Beulich
Cc: Stefano Stabellini, Stefano Stabellini, Andrew Cooper,
george.dunlap, Julien Grall, Ian Jackson, xen-devel
On Wed, 27 Feb 2019, Jan Beulich wrote:
> >>> On 26.02.19 at 20:20, <sstabellini@kernel.org> wrote:
> > On Tue, 26 Feb 2019, Ian Jackson wrote:
> >> Having written all that down (what a palaver), we can see that your
> >> cast, above, is a breach of the rules. Instead you can just pass the
> >> struct abstract_alt_instr*, and all is well. Then you don't need a
> >> comment at the use site, either, since you are doing things which are
> >> entirely supported and explained.
> >
> > The in-code comment is great, and I have added it to the right patch.
> >
> > Let me get a clarification on the rest of the suggestion: I would
> > have to change the type of region->end to const struct
> > abstract_alt_instr* (see xen/arch/arm/alternative.c).
> >
> > If I do that, I get a compilation failure at:
> >
> > alternative.c:245:16: error: initialization from incompatible pointer type
> > [-Werror=incompatible-pointer-types]
> > .end = end,
> >
> > because apply_alternatives currently expects two struct alt_instr* as
> > parameters. I cannot change the type of the second parameter of
> > apply_alternatives, because actually it might not be a linker symbol, it
> > might not be a struct abstract_alt_instr*. So I would still have to cast
> > to struct abstract_alt_instr* somewhere?
>
> I don't think so, no. In livepatch.c we have
>
> end = sec->load_addr + sec->sec->sh_size;
>
> which (a) is effectively a linker defined symbol, just that we don't
> obtain it through a linker script and it also has no name associated
> with it and (b) will be fine without any casts thanks to load_addr
> being of type void * (i.e. the type of "end" can freely change).
Yes, good suggestion.
> >> > - 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] = (uintptr_t)p - (uintptr_t)__per_cpu_start;
> >>
> >> If per_cpu_diff(__per_cpu_start, __per_cpu_data_end) gives the right
> >> value for memset, isn't it the right value for offset[cpu] too ?
> >> Ie I don't know why you are using uintptr_t here.
> >
> > I am using uintptr_t because p is not a abstract_per_cpu type. I could
> > do:
> >
> > __per_cpu_offset[cpu] = per_cpu_diff(__per_cpu_start,
> > (struct abstract_per_cpu *)p);
> >
> > I didn't think it was better though. What do you think?
>
> Did you try changing p's type? That said, the calculation isn't going
> to be universally correct (in MISRA's sense), no matter what you do:
> We _deliberately_ subtract two entities here which are _guaranteed_
> not to point to the same object (or one past an array's last element).
>
No I haven't tried changing p's type -- I have been trying to avoid
using abstract_blah_blah in the code, my assumption is that it was
supposed to be hidden within the depths of the macro, not used
explicitly. But I can do that and it gets rid of these casts.
> >> > @@ -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];
> >>
> >> I also don't know why you are casting to char* here if you didn't need
> >> to do so before.
> >
> > That is because __per_cpu_start is now const, it wasn't before.
>
> That's why I'm advocating that free()-style functions should take
> pointers to const. A patch to this effect wasn't liked, though.
Changes like this are very few in this series, only a couple. I would
hope that it won't be a problem. Especially considering that most of the
linker symbols are declared as non-const today. I.e. we are not making
things worse.
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel
^ permalink raw reply [flat|nested] 35+ messages in thread
* Re: [PATCH v10 6/6] xen: use DEFINE_SYMBOL as required
2019-02-27 8:43 ` Jan Beulich
@ 2019-03-01 21:48 ` Stefano Stabellini
0 siblings, 0 replies; 35+ messages in thread
From: Stefano Stabellini @ 2019-03-01 21:48 UTC (permalink / raw)
To: Jan Beulich
Cc: Stefano Stabellini, Stefano Stabellini, Andrew Cooper,
george.dunlap, Julien Grall, ian.jackson, xen-devel
On Wed, 27 Feb 2019, Jan Beulich wrote:
> >>> On 26.02.19 at 22:22, <sstabellini@kernel.org> wrote:
> > On Tue, 26 Feb 2019, Jan Beulich wrote:
> >> >>> On 25.02.19 at 21:50, <sstabellini@kernel.org> wrote:
> >> > @@ -210,7 +210,8 @@ static int __apply_alternatives_multi_stop(void *unused)
> >> > region.begin = __alt_instructions;
> >> > region.end = (struct alt_instr *)__alt_instructions_end;
> >> >
> >> > - ret = __apply_alternatives(®ion, xenmap - (void *)_start);
> >> > + ret = __apply_alternatives(®ion, (uintptr_t)xenmap -
> >> > + (uintptr_t)_start);
> >>
> >> Undesirable (but in this case maybe indeed unavoidable) casting
> >> instead. I don't think this belongs in a patch with the given title
> >> though.
> >
> > It's in this patch because this is the patch dealing with _start and
> > _end. Let me know how would you like the patches to be split.
>
> Well, I can see the general possible need for additional changes
> due to the type adjustments. I don't see though why the original
> code in this example would break with the other adjustments done
> here. Things need to build and work after each patch, but changes
> not strictly needed and not related to the subject of a patch would
> better be split out (in this case into the [or another] Arm-specific
> patch).
I'll add a patch at the end with all the explicit casts. Much easier to
talk about how to solve the odd ends that way. This specific instance
can be resolved though, I'll do that.
> >> > @@ -78,7 +78,19 @@ void arch_livepatch_revert(const struct livepatch_func *func)
> >> > uint32_t *new_ptr;
> >> > unsigned int len;
> >> >
> >> > - new_ptr = func->old_addr - (void *)_start + vmap_of_xen_text;
> >> > + /*
> >> > + * We need to calculate the offset of the address from _start, and
> >> > + * apply that to our own map, to find where we have this mapped.
> >> > + * Doing these kind of games directly with pointers is contrary to
> >> > + * the C rules for what pointers may be compared and computed. So
> >> > + * we do the offset calculation with integers, which is always
> >> > + * legal. The subsequent addition of the offset to the
> >> > + * vmap_of_xen_text pointer is legal because the computed pointer is
> >> > + * indeed a valid part of the object referred to by vmap_of_xen_text
> >> > + * - namely, the byte array of our mapping of the Xen text.
> >> > + */
> >> > + new_ptr = ((uintptr_t)func->old_addr, - (uintptr_t)_start) +
> >> > + vmap_of_xen_text;
> >>
> >> You not using the intended helper inlines has allowed for a bug to
> >> slip in that the compiler can't even help notice, due to - being both
> >> a valid unary and a valid binary operator.
> >
> > Well spotted! I'll fix the bug. I would also be happy to use the helper
> > inlines, but we discussed not to use them in cases like this, with three
> > operators. Even if I wanted to use them, none of the inline helpers fit
> > this case. Or do you suggest:
> >
> > new_ptr = xen_diff(_start, (struct abstract_xen *)func->old_addr) +
> > vmap_of_xen_text;
> >
> > Is that what you are asking?
>
> No matter what, it looks like you're wanting (and perhaps needing) to
> stick to some form of cast here. But that's what you're specifically
> trying to get away from, aren't you? What is MISRA's position on
> casts from void * to a type? This is not a generally "safe" operation
> after all, because the casted to type could be out of sync with the
> object the pointer points at.
>
> Note that struct livepatch_func only happens to live in the public
> interface, but the type of old_addr ought to be freely changeable as
> long as it remains a pointer. Did you check whether changing it would
> help avoid all casting?
I am trying to get away from comparisons/subtractions between pointers
pointing to different objects. I am not trying to get away from casts
completely, only trying to minimize them while reaching the main
objective.
I checked MISRAC and it looks like void* pointers conversions are
allowed, so I can use xen_diff here.
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel
^ permalink raw reply [flat|nested] 35+ messages in thread
end of thread, other threads:[~2019-03-01 21:48 UTC | newest]
Thread overview: 35+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2019-02-25 20:49 [PATCH v10 0/6] misc safety certification fixes Stefano Stabellini
2019-02-25 20:50 ` [PATCH v10 1/6] xen: introduce ptrdiff_t Stefano Stabellini
2019-02-26 10:54 ` Jan Beulich
2019-02-26 16:24 ` Ian Jackson
2019-02-25 20:50 ` [PATCH v10 2/6] xen: introduce DEFINE_SYMBOL Stefano Stabellini
2019-02-26 15:20 ` Jan Beulich
2019-02-26 18:54 ` Stefano Stabellini
2019-02-27 8:29 ` Jan Beulich
2019-02-26 16:46 ` Ian Jackson
2019-02-26 17:13 ` Jan Beulich
2019-02-26 17:32 ` Ian Jackson
2019-02-26 18:43 ` Stefano Stabellini
2019-02-27 9:46 ` Jan Beulich
2019-02-28 0:05 ` Stefano Stabellini
2019-02-28 10:33 ` Jan Beulich
2019-02-27 8:27 ` Jan Beulich
2019-02-25 20:50 ` [PATCH v10 3/6] xen/arm: use DEFINE_SYMBOL as required Stefano Stabellini
2019-02-26 17:11 ` Ian Jackson
2019-02-26 19:20 ` Stefano Stabellini
2019-02-27 9:56 ` Jan Beulich
2019-02-28 23:43 ` Stefano Stabellini
2019-02-25 20:50 ` [PATCH v10 4/6] xen/x86: " Stefano Stabellini
2019-02-26 17:13 ` Ian Jackson
2019-02-26 19:23 ` Stefano Stabellini
2019-02-27 9:45 ` Jan Beulich
2019-02-26 17:16 ` Ian Jackson
2019-02-25 20:50 ` [PATCH v10 5/6] xen/common: " Stefano Stabellini
2019-02-26 15:13 ` Jan Beulich
2019-02-26 21:14 ` Stefano Stabellini
2019-02-27 8:32 ` Jan Beulich
2019-02-25 20:50 ` [PATCH v10 6/6] xen: " Stefano Stabellini
2019-02-26 15:28 ` Jan Beulich
2019-02-26 21:22 ` Stefano Stabellini
2019-02-27 8:43 ` Jan Beulich
2019-03-01 21:48 ` Stefano Stabellini
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.