[PATCH v3 0/4] misc safety certification fixes

[PATCH v3 0/4] misc safety certification fixes

[Stefano Stabellini]

Hi all,

This short patch series fixes a few issues discovered by the safety
certifications code scanner. The first two patches address simple
variable initializations concerns. The third patch introduces a new
macro that is used throughout the code in patch #4 to access _stext,
_etext pointers and friends.

Cheers,

Stefano


Stefano Stabellini (4):
      xen/arm: initialize target
      xen/arm: initialize access
      xen: introduce SYMBOL
      xen: use SYMBOL everywhere

 xen/arch/arm/alternative.c          |  7 ++--
 xen/arch/arm/arm32/livepatch.c      |  2 +-
 xen/arch/arm/arm64/livepatch.c      |  2 +-
 xen/arch/arm/domain_build.c         |  2 +-
 xen/arch/arm/livepatch.c            |  6 +--
 xen/arch/arm/mem_access.c           |  1 +
 xen/arch/arm/mm.c                   | 17 ++++----
 xen/arch/arm/setup.c                |  8 ++--
 xen/arch/arm/vgic-v2.c              |  2 +-
 xen/arch/arm/vgic-v3.c              |  2 +-
 xen/arch/x86/setup.c                | 79 +++++++++++++++++++------------------
 xen/arch/x86/tboot.c                | 12 +++---
 xen/arch/x86/x86_64/machine_kexec.c |  4 +-
 xen/drivers/vpci/vpci.c             |  7 +++-
 xen/include/asm-arm/grant_table.h   |  3 +-
 xen/include/asm-arm/mm.h            |  4 +-
 xen/include/asm-x86/mm.h            |  4 +-
 xen/include/xen/compiler.h          |  6 +++
 xen/include/xen/kernel.h            | 24 +++++------
 19 files changed, 106 insertions(+), 86 deletions(-)

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

[PATCH v3 1/4] xen/arm: initialize target

[Stefano Stabellini]

Initialize variable target before passing it as a parameter.
It makes the code a bit nicer and it is a safety certification
requirement.

M3CM Rule-9.1: The value of an object with automatic storage duration
shall not be read before it has been set

QAVerify: 2972
Signed-off-by: Stefano Stabellini <stefanos@xilinx.com>
---
Changes in v3:
- add more info to commit messagte

Changes in v2:
- improve comment
---
 xen/arch/arm/vgic-v2.c | 2 +-
 xen/arch/arm/vgic-v3.c | 2 +-
 2 files changed, 2 insertions(+), 2 deletions(-)

diff --git a/xen/arch/arm/vgic-v2.c b/xen/arch/arm/vgic-v2.c
index f6c11f1..0099fcf 100644
--- a/xen/arch/arm/vgic-v2.c
+++ b/xen/arch/arm/vgic-v2.c
@@ -379,6 +379,7 @@ static bool vgic_v2_to_sgi(struct vcpu *v, register_t sgir)
     enum gic_sgi_mode sgi_mode;
     struct sgi_target target;
 
+    sgi_target_init(&target);
     irqmode = (sgir & GICD_SGI_TARGET_LIST_MASK) >> GICD_SGI_TARGET_LIST_SHIFT;
     virq = (sgir & GICD_SGI_INTID_MASK);
 
@@ -386,7 +387,6 @@ static bool vgic_v2_to_sgi(struct vcpu *v, register_t sgir)
     switch ( irqmode )
     {
     case GICD_SGI_TARGET_LIST_VAL:
-        sgi_target_init(&target);
         target.list = (sgir & GICD_SGI_TARGET_MASK) >> GICD_SGI_TARGET_SHIFT;
         sgi_mode = SGI_TARGET_LIST;
         break;
diff --git a/xen/arch/arm/vgic-v3.c b/xen/arch/arm/vgic-v3.c
index efe824c..c14bcd8 100644
--- a/xen/arch/arm/vgic-v3.c
+++ b/xen/arch/arm/vgic-v3.c
@@ -1474,6 +1474,7 @@ static bool vgic_v3_to_sgi(struct vcpu *v, register_t sgir)
     enum gic_sgi_mode sgi_mode;
     struct sgi_target target;
 
+    sgi_target_init(&target);
     irqmode = (sgir >> ICH_SGI_IRQMODE_SHIFT) & ICH_SGI_IRQMODE_MASK;
     virq = (sgir >> ICH_SGI_IRQ_SHIFT ) & ICH_SGI_IRQ_MASK;
 
@@ -1481,7 +1482,6 @@ static bool vgic_v3_to_sgi(struct vcpu *v, register_t sgir)
     switch ( irqmode )
     {
     case ICH_SGI_TARGET_LIST:
-        sgi_target_init(&target);
         /* We assume that only AFF1 is used in ICC_SGI1R_EL1. */
         target.aff1 = (sgir >> ICH_SGI_AFFINITY_LEVEL(1)) & ICH_SGI_AFFx_MASK;
         target.list = sgir & ICH_SGI_TARGETLIST_MASK;
-- 
1.9.1


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

[PATCH v3 2/4] xen/arm: initialize access

[Stefano Stabellini]

Initialize variable *access before returning it back to the caller.
It makes the code a bit nicer and it is a safety certification
requirement.

M3CM Rule-9.1: The value of an object with automatic storage duration
shall not be read before it has been set

QAVerify: 2962
Signed-off-by: Stefano Stabellini <stefanos@xilinx.com>
Acked-by: Razvan Cojocaru <rcojocaru@bitdefender.com>
CC: rcojocaru@bitdefender.com
CC: Tamas K Lengyel <tamas@tklengyel.com>
---
Changes in v3:
- add more info to commit messagte

Changes in v2:
- improve comment
- use p2m->default_access
---
 xen/arch/arm/mem_access.c | 1 +
 1 file changed, 1 insertion(+)

diff --git a/xen/arch/arm/mem_access.c b/xen/arch/arm/mem_access.c
index ba4ec78..86f0882 100644
--- a/xen/arch/arm/mem_access.c
+++ b/xen/arch/arm/mem_access.c
@@ -47,6 +47,7 @@ static int __p2m_get_mem_access(struct domain *d, gfn_t gfn,
     };
 
     ASSERT(p2m_is_locked(p2m));
+    *access = p2m->default_access;
 
     /* If no setting was ever set, just return rwx. */
     if ( !p2m->mem_access_enabled )
-- 
1.9.1


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

[PATCH v3 3/4] xen: introduce SYMBOL

[Stefano Stabellini]

Introduce a macro, SYMBOL, which is a simple wrapper around RELOC_HIDE
to be used everywhere symbols such as _stext and _etext are used in the
code.

RELOC_HIDE is needed when accessing symbols such as _stext and _etext
because the C standard forbids for both comparisons and substraction
(see C Standard, 6.5.6 [ISO/IEC 9899:2011] and [1]).

forbids comparisons between pointers pointing to
different objects. _stext, _etext, etc. are all pointers to different
objects from ANCI C point of view.

To work around potential C compiler issues (which have actually
been found, see the comment on top of RELOC_HIDE in Linux), and to help
with certifications, let's introduce some syntactic sugar to be used in
following patches.

[1] https://wiki.sei.cmu.edu/confluence/display/c/ARR36-C.+Do+not+subtract+or+compare+two+pointers+that+do+not+refer+to+the+same+array

Signed-off-by: Stefano Stabellini <stefanos@xilinx.com>
CC: JBeulich@suse.com
CC: andrew.cooper3@citrix.com
CC: wei.liu2@citrix.com
---
Changes in v3:
- improve commit message
- rename __symbol to SYMBOL to avoid name space violations

Changes in v2:
- do not cast return to char*
- move to common header
---
 xen/include/xen/compiler.h | 6 ++++++
 1 file changed, 6 insertions(+)

diff --git a/xen/include/xen/compiler.h b/xen/include/xen/compiler.h
index ff6c0f5..b3375f6 100644
--- a/xen/include/xen/compiler.h
+++ b/xen/include/xen/compiler.h
@@ -99,6 +99,12 @@
     __asm__ ("" : "=r"(__ptr) : "0"(ptr));      \
     (typeof(ptr)) (__ptr + (off)); })
 
+/*
+ * Use RELOC_HIDE with symbols such as _stext and _etext to avoid errors
+ * on comparing pointers to different objects
+ */
+#define SYMBOL(x)         (RELOC_HIDE((unsigned long)(x), 0))
+
 #ifdef __GCC_ASM_FLAG_OUTPUTS__
 # define ASM_FLAG_OUT(yes, no) yes
 #else
-- 
1.9.1


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

[PATCH v3 4/4] xen: use SYMBOL everywhere

[Stefano Stabellini]

Use SYMBOL everywhere _stext, _etext, etc. are used. Technically, it
is required when comparing and subtracting pointers [1], but use it
everywhere to avoid confusion.

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

[1] https://wiki.sei.cmu.edu/confluence/display/c/ARR36-C.+Do+not+subtract+or+compare+two+pointers+that+do+not+refer+to+the+same+array

QAVerify: 2761 (and many others)
Signed-off-by: Stefano Stabellini <stefanos@xilinx.com>
CC: JBeulich@suse.com
CC: andrew.cooper3@citrix.com
---
Changes in v3:
- improve commit message
- no hard tabs
- rename __symbol to SYMBOL
- fix __end_vpci_array and __start_vpci_array
- avoid all comparisons between pointers: including (void *) casted
  returns from SYMBOL()
- remove useless casts to (unsigned long)

Changes in v2:
- cast return of SYMBOL to char* when required
- define __pa as unsigned long in is_kernel* functions
---
 xen/arch/arm/alternative.c          |  7 ++--
 xen/arch/arm/arm32/livepatch.c      |  2 +-
 xen/arch/arm/arm64/livepatch.c      |  2 +-
 xen/arch/arm/domain_build.c         |  2 +-
 xen/arch/arm/livepatch.c            |  6 +--
 xen/arch/arm/mm.c                   | 17 ++++----
 xen/arch/arm/setup.c                |  8 ++--
 xen/arch/x86/setup.c                | 79 +++++++++++++++++++------------------
 xen/arch/x86/tboot.c                | 12 +++---
 xen/arch/x86/x86_64/machine_kexec.c |  4 +-
 xen/drivers/vpci/vpci.c             |  7 +++-
 xen/include/asm-arm/grant_table.h   |  3 +-
 xen/include/asm-arm/mm.h            |  4 +-
 xen/include/asm-x86/mm.h            |  4 +-
 xen/include/xen/kernel.h            | 24 +++++------
 15 files changed, 97 insertions(+), 84 deletions(-)

diff --git a/xen/arch/arm/alternative.c b/xen/arch/arm/alternative.c
index 52ed7ed..2efa9ca 100644
--- a/xen/arch/arm/alternative.c
+++ b/xen/arch/arm/alternative.c
@@ -187,8 +187,8 @@ 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;
+        mfn_t xen_mfn = virt_to_mfn(SYMBOL(_start));
+        paddr_t xen_size = SYMBOL(_end) - SYMBOL(_start);
         unsigned int xen_order = get_order_from_bytes(xen_size);
         void *xenmap;
 
@@ -206,7 +206,8 @@ static int __apply_alternatives_multi_stop(void *unused)
         region.begin = __alt_instructions;
         region.end = __alt_instructions_end;
 
-        ret = __apply_alternatives(&region, xenmap - (void *)_start);
+        ret = __apply_alternatives(&region,
+                                   (unsigned long)xenmap - SYMBOL(_start));
         /* The patching is not expected to fail during boot. */
         BUG_ON(ret != 0);
 
diff --git a/xen/arch/arm/arm32/livepatch.c b/xen/arch/arm/arm32/livepatch.c
index 41378a5..6bf9132 100644
--- a/xen/arch/arm/arm32/livepatch.c
+++ b/xen/arch/arm/arm32/livepatch.c
@@ -56,7 +56,7 @@ void arch_livepatch_apply(struct livepatch_func *func)
     else
         insn = 0xe1a00000; /* mov r0, r0 */
 
-    new_ptr = func->old_addr - (void *)_start + vmap_of_xen_text;
+    new_ptr = func->old_addr - (void *)SYMBOL(_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..fb1477d 100644
--- a/xen/arch/arm/arm64/livepatch.c
+++ b/xen/arch/arm/arm64/livepatch.c
@@ -43,7 +43,7 @@ void arch_livepatch_apply(struct livepatch_func *func)
     /* Verified in livepatch_verify_distance. */
     ASSERT(insn != AARCH64_BREAK_FAULT);
 
-    new_ptr = func->old_addr - (void *)_start + vmap_of_xen_text;
+    new_ptr = func->old_addr - SYMBOL(_start) + vmap_of_xen_text;
     len = len / sizeof(uint32_t);
 
     /* PATCH! */
diff --git a/xen/arch/arm/domain_build.c b/xen/arch/arm/domain_build.c
index f552154..6c03996 100644
--- a/xen/arch/arm/domain_build.c
+++ b/xen/arch/arm/domain_build.c
@@ -2090,7 +2090,7 @@ static void __init find_gnttab_region(struct domain *d,
      * Only use the text section as it's always present and will contain
      * enough space for a large grant table
      */
-    kinfo->gnttab_start = __pa(_stext);
+    kinfo->gnttab_start = __pa(SYMBOL(_stext));
     kinfo->gnttab_size = gnttab_dom0_frames() << PAGE_SHIFT;
 
 #ifdef CONFIG_ARM_32
diff --git a/xen/arch/arm/livepatch.c b/xen/arch/arm/livepatch.c
index 279d52c..609ab35 100644
--- a/xen/arch/arm/livepatch.c
+++ b/xen/arch/arm/livepatch.c
@@ -26,8 +26,8 @@ int arch_livepatch_quiesce(void)
     if ( vmap_of_xen_text )
         return -EINVAL;
 
-    text_mfn = virt_to_mfn(_start);
-    text_order = get_order_from_bytes(_end - _start);
+    text_mfn = virt_to_mfn(SYMBOL(_start));
+    text_order = get_order_from_bytes(SYMBOL(_end) - SYMBOL(_start));
 
     /*
      * The text section is read-only. So re-map Xen to be able to patch
@@ -78,7 +78,7 @@ void arch_livepatch_revert(const struct livepatch_func *func)
     uint32_t *new_ptr;
     unsigned int len;
 
-    new_ptr = func->old_addr - (void *)_start + vmap_of_xen_text;
+    new_ptr = func->old_addr - SYMBOL(_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 7a06a33..16a8afc 100644
--- a/xen/arch/arm/mm.c
+++ b/xen/arch/arm/mm.c
@@ -620,7 +620,7 @@ void __init setup_pagetables(unsigned long boot_phys_offset, paddr_t xen_paddr)
     int i;
 
     /* Calculate virt-to-phys offset for the new location */
-    phys_offset = xen_paddr - (unsigned long) _start;
+    phys_offset = xen_paddr - SYMBOL(_start);
 
 #ifdef CONFIG_ARM_64
     p = (void *) xen_pgtable;
@@ -681,7 +681,8 @@ void __init setup_pagetables(unsigned long boot_phys_offset, paddr_t xen_paddr)
     ttbr = (uintptr_t) cpu0_pgtable + phys_offset;
 #endif
 
-    relocate_xen(ttbr, _start, (void*)dest_va, _end - _start);
+    relocate_xen(ttbr, (void*)SYMBOL(_start), (void*)dest_va,
+                 SYMBOL(_end) - SYMBOL(_start));
 
     /* Clear the copy of the boot pagetables. Each secondary CPU
      * rebuilds these itself (see head.S) */
@@ -1089,7 +1090,7 @@ int modify_xen_mappings(unsigned long s, unsigned long e, unsigned int flags)
 }
 
 enum mg { mg_clear, mg_ro, mg_rw, mg_rx };
-static void set_pte_flags_on_range(const char *p, unsigned long l, enum mg mg)
+static void set_pte_flags_on_range(const unsigned long p, unsigned long l, enum mg mg)
 {
     lpae_t pte;
     int i;
@@ -1100,8 +1101,8 @@ static void set_pte_flags_on_range(const char *p, unsigned long l, enum mg mg)
     ASSERT(!((unsigned long) p & ~PAGE_MASK));
     ASSERT(!(l & ~PAGE_MASK));
 
-    for ( i = (p - _start) / PAGE_SIZE; 
-          i < (p + l - _start) / PAGE_SIZE; 
+    for ( i = (p - SYMBOL(_start)) / PAGE_SIZE; 
+          i < (p + l - SYMBOL(_start)) / PAGE_SIZE; 
           i++ )
     {
         pte = xen_xenmap[i];
@@ -1138,12 +1139,12 @@ static void set_pte_flags_on_range(const char *p, unsigned long l, enum mg mg)
 void free_init_memory(void)
 {
     paddr_t pa = virt_to_maddr(__init_begin);
-    unsigned long len = __init_end - __init_begin;
+    unsigned long len = SYMBOL(__init_end) - SYMBOL(__init_begin);
     uint32_t insn;
     unsigned int i, nr = len / sizeof(insn);
     uint32_t *p;
 
-    set_pte_flags_on_range(__init_begin, len, mg_rw);
+    set_pte_flags_on_range(SYMBOL(__init_begin), len, mg_rw);
 #ifdef CONFIG_ARM_32
     /* udf instruction i.e (see A8.8.247 in ARM DDI 0406C.c) */
     insn = 0xe7f000f0;
@@ -1154,7 +1155,7 @@ void free_init_memory(void)
     for ( i = 0; i < nr; i++ )
         *(p + i) = insn;
 
-    set_pte_flags_on_range(__init_begin, len, mg_clear);
+    set_pte_flags_on_range(SYMBOL(__init_begin), len, mg_clear);
     init_domheap_pages(pa, pa + len);
     printk("Freed %ldkB init memory.\n", (long)(__init_end-__init_begin)>>10);
 }
diff --git a/xen/arch/arm/setup.c b/xen/arch/arm/setup.c
index ea2495a..e3adddf 100644
--- a/xen/arch/arm/setup.c
+++ b/xen/arch/arm/setup.c
@@ -394,7 +394,8 @@ static paddr_t __init get_xen_paddr(void)
     paddr_t paddr = 0;
     int i;
 
-    min_size = (_end - _start + (XEN_PADDR_ALIGN-1)) & ~(XEN_PADDR_ALIGN-1);
+    min_size = (SYMBOL(_end) - SYMBOL(_start) +
+                (XEN_PADDR_ALIGN-1)) & ~(XEN_PADDR_ALIGN-1);
 
     /* Find the highest bank with enough space. */
     for ( i = 0; i < mi->nr_banks; i++ )
@@ -727,8 +728,9 @@ void __init start_xen(unsigned long boot_phys_offset,
 
     /* Register Xen's load address as a boot module. */
     xen_bootmodule = add_boot_module(BOOTMOD_XEN,
-                             (paddr_t)(uintptr_t)(_start + boot_phys_offset),
-                             (paddr_t)(uintptr_t)(_end - _start + 1), NULL);
+                             (paddr_t)(uintptr_t)(SYMBOL(_start) + boot_phys_offset),
+                             (paddr_t)(uintptr_t)(SYMBOL(_end) -
+                                                  SYMBOL(_start) + 1), NULL);
     BUG_ON(!xen_bootmodule);
 
     xen_paddr = get_xen_paddr();
diff --git a/xen/arch/x86/setup.c b/xen/arch/x86/setup.c
index 93b79c7..1a02b30 100644
--- a/xen/arch/x86/setup.c
+++ b/xen/arch/x86/setup.c
@@ -578,13 +578,13 @@ static void __init kexec_reserve_area(struct e820map *e820)
 
 static inline bool using_2M_mapping(void)
 {
-    return !l1_table_offset((unsigned long)__2M_text_end) &&
-           !l1_table_offset((unsigned long)__2M_rodata_start) &&
-           !l1_table_offset((unsigned long)__2M_rodata_end) &&
-           !l1_table_offset((unsigned long)__2M_init_start) &&
-           !l1_table_offset((unsigned long)__2M_init_end) &&
-           !l1_table_offset((unsigned long)__2M_rwdata_start) &&
-           !l1_table_offset((unsigned long)__2M_rwdata_end);
+    return !l1_table_offset(SYMBOL(__2M_text_end)) &&
+           !l1_table_offset(SYMBOL(__2M_rodata_start)) &&
+           !l1_table_offset(SYMBOL(__2M_rodata_end)) &&
+           !l1_table_offset(SYMBOL(__2M_init_start)) &&
+           !l1_table_offset(SYMBOL(__2M_init_end)) &&
+           !l1_table_offset(SYMBOL(__2M_rwdata_start)) &&
+           !l1_table_offset(SYMBOL(__2M_rwdata_end));
 }
 
 static void noinline init_done(void)
@@ -606,13 +606,13 @@ static void noinline init_done(void)
     /* Destroy Xen's mappings, and reuse the pages. */
     if ( using_2M_mapping() )
     {
-        start = (unsigned long)&__2M_init_start,
-        end   = (unsigned long)&__2M_init_end;
+        start = SYMBOL(&__2M_init_start),
+        end   = SYMBOL(&__2M_init_end);
     }
     else
     {
-        start = (unsigned long)&__init_begin;
-        end   = (unsigned long)&__init_end;
+        start = SYMBOL(&__init_begin);
+        end   = SYMBOL(&__init_end);
     }
 
     destroy_xen_mappings(start, end);
@@ -966,8 +966,8 @@ void __init noreturn __start_xen(unsigned long mbi_p)
          * This needs to remain in sync with xen_in_range() and the
          * 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_start = virt_to_mfn(SYMBOL(_stext));
+        mod[mbi->mods_count].mod_end = SYMBOL(__2M_rwdata_end) - SYMBOL(_stext);
     }
 
     modules_headroom = bzimage_headroom(bootstrap_map(mod), mod->mod_end);
@@ -1018,7 +1018,7 @@ void __init noreturn __start_xen(unsigned long mbi_p)
                      1UL << (PAGE_SHIFT + 32)) )
             e = min(HYPERVISOR_VIRT_END - DIRECTMAP_VIRT_START,
                     1UL << (PAGE_SHIFT + 32));
-#define reloc_size ((__pa(__2M_rwdata_end) + mask) & ~mask)
+#define reloc_size ((__pa(SYMBOL(__2M_rwdata_end)) + mask) & ~mask)
         /* Is the region suitable for relocating Xen? */
         if ( !xen_phys_start && e <= limit )
         {
@@ -1034,7 +1034,8 @@ void __init noreturn __start_xen(unsigned long mbi_p)
          * Is the region size greater than zero and does it begin
          * at or above the end of current Xen image placement?
          */
-        if ( (end > s) && (end - reloc_size + XEN_IMG_OFFSET >= __pa(_end)) )
+        if ( (end > s) && (end - reloc_size + XEN_IMG_OFFSET >=
+             __pa(SYMBOL(_end))) )
         {
             l4_pgentry_t *pl4e;
             l3_pgentry_t *pl3e;
@@ -1062,7 +1063,8 @@ void __init noreturn __start_xen(unsigned long mbi_p)
              * data until after we have switched to the relocated pagetables!
              */
             barrier();
-            move_memory(e + XEN_IMG_OFFSET, XEN_IMG_OFFSET, _end - _start, 1);
+            move_memory(e + XEN_IMG_OFFSET, XEN_IMG_OFFSET,
+                        SYMBOL(_end) - SYMBOL(_start), 1);
 
             /* Walk initial pagetables, relocating page directory entries. */
             pl4e = __va(__pa(idle_pg_table));
@@ -1103,8 +1105,8 @@ void __init noreturn __start_xen(unsigned long mbi_p)
              * is contained in this PTE.
              */
             BUG_ON(using_2M_mapping() &&
-                   l2_table_offset((unsigned long)_erodata) ==
-                   l2_table_offset((unsigned long)_stext));
+                   l2_table_offset(SYMBOL(_erodata)) ==
+                   l2_table_offset(SYMBOL(_stext)));
             *pl2e++ = l2e_from_pfn(xen_phys_start >> PAGE_SHIFT,
                                    PAGE_HYPERVISOR_RX | _PAGE_PSE);
             for ( i = 1; i < L2_PAGETABLE_ENTRIES; i++, pl2e++ )
@@ -1122,22 +1124,22 @@ void __init noreturn __start_xen(unsigned long mbi_p)
                     continue;
                 }
 
-                if ( i < l2_table_offset((unsigned long)&__2M_text_end) )
+                if ( i < l2_table_offset((unsigned long)SYMBOL(&__2M_text_end)) )
                 {
                     flags = PAGE_HYPERVISOR_RX | _PAGE_PSE;
                 }
-                else if ( i >= l2_table_offset((unsigned long)&__2M_rodata_start) &&
-                          i <  l2_table_offset((unsigned long)&__2M_rodata_end) )
+                else if ( i >= l2_table_offset(SYMBOL(&__2M_rodata_start)) &&
+                          i <  l2_table_offset(SYMBOL(&__2M_rodata_end)) )
                 {
                     flags = PAGE_HYPERVISOR_RO | _PAGE_PSE;
                 }
-                else if ( i >= l2_table_offset((unsigned long)&__2M_init_start) &&
-                          i <  l2_table_offset((unsigned long)&__2M_init_end) )
+                else if ( i >= l2_table_offset(SYMBOL(&__2M_init_start)) &&
+                          i <  l2_table_offset(SYMBOL(&__2M_init_end)) )
                 {
                     flags = PAGE_HYPERVISOR_RWX | _PAGE_PSE;
                 }
-                else if ( (i >= l2_table_offset((unsigned long)&__2M_rwdata_start) &&
-                           i <  l2_table_offset((unsigned long)&__2M_rwdata_end)) )
+                else if ( (i >= l2_table_offset(SYMBOL(&__2M_rwdata_start)) &&
+                           i <  l2_table_offset(SYMBOL(&__2M_rwdata_end))) )
                 {
                     flags = PAGE_HYPERVISOR_RW | _PAGE_PSE;
                 }
@@ -1234,7 +1236,7 @@ void __init noreturn __start_xen(unsigned long mbi_p)
         panic("Not enough memory to relocate Xen\n");
 
     /* This needs to remain in sync with xen_in_range(). */
-    reserve_e820_ram(&boot_e820, __pa(_stext), __pa(__2M_rwdata_end));
+    reserve_e820_ram(&boot_e820, __pa(SYMBOL(_stext)), __pa(SYMBOL(__2M_rwdata_end)));
 
     /* Late kexec reservation (dynamic start address). */
     kexec_reserve_area(&boot_e820);
@@ -1377,7 +1379,8 @@ void __init noreturn __start_xen(unsigned long mbi_p)
     }
 #endif
 
-    xen_virt_end = ((unsigned long)_end + (1UL << L2_PAGETABLE_SHIFT) - 1) &
+    xen_virt_end = (SYMBOL(_end) +
+                    (1UL << L2_PAGETABLE_SHIFT) - 1) &
                    ~((1UL << L2_PAGETABLE_SHIFT) - 1);
     destroy_xen_mappings(xen_virt_end, XEN_VIRT_START + BOOTSTRAP_MAP_BASE);
 
@@ -1390,22 +1393,22 @@ void __init noreturn __start_xen(unsigned long mbi_p)
     {
         /* Mark .text as RX (avoiding the first 2M superpage). */
         modify_xen_mappings(XEN_VIRT_START + MB(2),
-                            (unsigned long)&__2M_text_end,
+                            SYMBOL(&__2M_text_end),
                             PAGE_HYPERVISOR_RX);
 
         /* Mark .rodata as RO. */
-        modify_xen_mappings((unsigned long)&__2M_rodata_start,
-                            (unsigned long)&__2M_rodata_end,
+        modify_xen_mappings(SYMBOL(&__2M_rodata_start),
+                            SYMBOL(&__2M_rodata_end),
                             PAGE_HYPERVISOR_RO);
 
         /* Mark .data and .bss as RW. */
-        modify_xen_mappings((unsigned long)&__2M_rwdata_start,
-                            (unsigned long)&__2M_rwdata_end,
+        modify_xen_mappings(SYMBOL(&__2M_rwdata_start),
+                            SYMBOL(&__2M_rwdata_end),
                             PAGE_HYPERVISOR_RW);
 
         /* Drop the remaining mappings in the shattered superpage. */
-        destroy_xen_mappings((unsigned long)&__2M_rwdata_end,
-                             ROUNDUP((unsigned long)&__2M_rwdata_end, MB(2)));
+        destroy_xen_mappings(SYMBOL(&__2M_rwdata_end),
+                             ROUNDUP(SYMBOL(&__2M_rwdata_end), MB(2)));
     }
 
     nr_pages = 0;
@@ -1860,11 +1863,11 @@ int __hwdom_init xen_in_range(unsigned long mfn)
          */
 
         /* hypervisor .text + .rodata */
-        xen_regions[region_ro].s = __pa(&_stext);
-        xen_regions[region_ro].e = __pa(&__2M_rodata_end);
+        xen_regions[region_ro].s = __pa(SYMBOL(&_stext));
+        xen_regions[region_ro].e = __pa(SYMBOL(&__2M_rodata_end));
         /* hypervisor .data + .bss */
-        xen_regions[region_rw].s = __pa(&__2M_rwdata_start);
-        xen_regions[region_rw].e = __pa(&__2M_rwdata_end);
+        xen_regions[region_rw].s = __pa(SYMBOL(&__2M_rwdata_start));
+        xen_regions[region_rw].e = __pa(SYMBOL(&__2M_rwdata_end));
     }
 
     start = (paddr_t)mfn << PAGE_SHIFT;
diff --git a/xen/arch/x86/tboot.c b/xen/arch/x86/tboot.c
index f3fdee4..e355232 100644
--- a/xen/arch/x86/tboot.c
+++ b/xen/arch/x86/tboot.c
@@ -373,13 +373,13 @@ void tboot_shutdown(uint32_t shutdown_type)
         g_tboot_shared->mac_regions[0].size = bootsym_phys(trampoline_end) -
                                               bootsym_phys(trampoline_start);
         /* hypervisor .text + .rodata */
-        g_tboot_shared->mac_regions[1].start = (uint64_t)__pa(&_stext);
-        g_tboot_shared->mac_regions[1].size = __pa(&__2M_rodata_end) -
-                                              __pa(&_stext);
+        g_tboot_shared->mac_regions[1].start = (uint64_t)__pa(SYMBOL(&_stext));
+        g_tboot_shared->mac_regions[1].size = __pa(SYMBOL(&__2M_rodata_end)) -
+                                              __pa(SYMBOL(&_stext));
         /* hypervisor .data + .bss */
-        g_tboot_shared->mac_regions[2].start = (uint64_t)__pa(&__2M_rwdata_start);
-        g_tboot_shared->mac_regions[2].size = __pa(&__2M_rwdata_end) -
-                                              __pa(&__2M_rwdata_start);
+        g_tboot_shared->mac_regions[2].start = (uint64_t)__pa(SYMBOL(&__2M_rwdata_start));
+        g_tboot_shared->mac_regions[2].size = __pa(SYMBOL(&__2M_rwdata_end)) -
+                                              __pa(SYMBOL(&__2M_rwdata_start));
 
         /*
          * MAC domains and other Xen memory
diff --git a/xen/arch/x86/x86_64/machine_kexec.c b/xen/arch/x86/x86_64/machine_kexec.c
index f4a005c..91936db 100644
--- a/xen/arch/x86/x86_64/machine_kexec.c
+++ b/xen/arch/x86/x86_64/machine_kexec.c
@@ -13,8 +13,8 @@
 
 int machine_kexec_get_xen(xen_kexec_range_t *range)
 {
-        range->start = virt_to_maddr(_start);
-        range->size = virt_to_maddr(_end) - (unsigned long)range->start;
+        range->start = virt_to_maddr(SYMBOL(_start));
+        range->size = virt_to_maddr(SYMBOL(_end)) - (unsigned long)range->start;
         return 0;
 }
 
diff --git a/xen/drivers/vpci/vpci.c b/xen/drivers/vpci/vpci.c
index 82607bd..df2ca47 100644
--- a/xen/drivers/vpci/vpci.c
+++ b/xen/drivers/vpci/vpci.c
@@ -33,7 +33,7 @@ struct vpci_register {
 #ifdef __XEN__
 extern vpci_register_init_t *const __start_vpci_array[];
 extern vpci_register_init_t *const __end_vpci_array[];
-#define NUM_VPCI_INIT (__end_vpci_array - __start_vpci_array)
+#define NUM_VPCI_INIT (SYMBOL(__end_vpci_array) - SYMBOL(__start_vpci_array))
 
 void vpci_remove_device(struct pci_dev *pdev)
 {
@@ -71,6 +71,11 @@ int __hwdom_init vpci_add_handlers(struct pci_dev *pdev)
 
     for ( i = 0; i < NUM_VPCI_INIT; i++ )
     {
+        /*
+         * We should use SYMBOL here, but it would make the code awkward
+         * and it is not required due because there are no pointers
+         * comparison. Leave it as is.
+         */
         rc = __start_vpci_array[i](pdev);
         if ( rc )
             break;
diff --git a/xen/include/asm-arm/grant_table.h b/xen/include/asm-arm/grant_table.h
index 37415b7..6c5edcc 100644
--- a/xen/include/asm-arm/grant_table.h
+++ b/xen/include/asm-arm/grant_table.h
@@ -31,7 +31,8 @@ void gnttab_mark_dirty(struct domain *d, mfn_t mfn);
  * enough space for a large grant table
  */
 #define gnttab_dom0_frames()                                             \
-    min_t(unsigned int, opt_max_grant_frames, PFN_DOWN(_etext - _stext))
+    min_t(unsigned int, opt_max_grant_frames,                            \
+          PFN_DOWN(SYMBOL(_etext) - SYMBOL(_stext)))
 
 #define gnttab_init_arch(gt)                                             \
 ({                                                                       \
diff --git a/xen/include/asm-arm/mm.h b/xen/include/asm-arm/mm.h
index 940b74b..61983f6 100644
--- a/xen/include/asm-arm/mm.h
+++ b/xen/include/asm-arm/mm.h
@@ -151,8 +151,8 @@ extern vaddr_t xenheap_virt_start;
 #endif
 
 #define is_xen_fixed_mfn(mfn)                                   \
-    ((pfn_to_paddr(mfn) >= virt_to_maddr(&_start)) &&       \
-     (pfn_to_paddr(mfn) <= virt_to_maddr(&_end)))
+    ((pfn_to_paddr(mfn) >= virt_to_maddr(SYMBOL(&_start))) && \
+     (pfn_to_paddr(mfn) <= virt_to_maddr(SYMBOL(&_end))))
 
 #define page_get_owner(_p)    (_p)->v.inuse.domain
 #define page_set_owner(_p,_d) ((_p)->v.inuse.domain = (_d))
diff --git a/xen/include/asm-x86/mm.h b/xen/include/asm-x86/mm.h
index 6e45651..82018b2 100644
--- a/xen/include/asm-x86/mm.h
+++ b/xen/include/asm-x86/mm.h
@@ -273,8 +273,8 @@ struct page_info
 #define is_xen_heap_mfn(mfn) \
     (__mfn_valid(mfn) && is_xen_heap_page(mfn_to_page(_mfn(mfn))))
 #define is_xen_fixed_mfn(mfn)                     \
-    ((((mfn) << PAGE_SHIFT) >= __pa(&_stext)) &&  \
-     (((mfn) << PAGE_SHIFT) <= __pa(&__2M_rwdata_end)))
+    ((((mfn) << PAGE_SHIFT) >= __pa(SYMBOL(&_stext))) &&  \
+     (((mfn) << PAGE_SHIFT) <= __pa(SYMBOL(&__2M_rwdata_end))))
 
 #define PRtype_info "016lx"/* should only be used for printk's */
 
diff --git a/xen/include/xen/kernel.h b/xen/include/xen/kernel.h
index 548b64d..cd27030 100644
--- a/xen/include/xen/kernel.h
+++ b/xen/include/xen/kernel.h
@@ -66,27 +66,27 @@
 })
 
 extern char _start[], _end[], start[];
-#define is_kernel(p) ({                         \
-    char *__p = (char *)(unsigned long)(p);     \
-    (__p >= _start) && (__p < _end);            \
+#define is_kernel(p) ({                                             \
+    const unsigned long __p = (unsigned long)(p);                   \
+    (__p >= SYMBOL(_start)) && (__p < SYMBOL(_end));            \
 })
 
 extern char _stext[], _etext[];
-#define is_kernel_text(p) ({                    \
-    char *__p = (char *)(unsigned long)(p);     \
-    (__p >= _stext) && (__p < _etext);          \
+#define is_kernel_text(p) ({                                        \
+    const unsigned long __p = (unsigned long)(p);                   \
+    (__p >= SYMBOL(_stext)) && (__p < SYMBOL(_etext));          \
 })
 
 extern const char _srodata[], _erodata[];
-#define is_kernel_rodata(p) ({                  \
-    const char *__p = (const char *)(unsigned long)(p);     \
-    (__p >= _srodata) && (__p < _erodata);      \
+#define is_kernel_rodata(p) ({                                      \
+    const unsigned long __p = (unsigned long)(p);                   \
+    (__p >= SYMBOL(_srodata)) && (__p < SYMBOL(_erodata));      \
 })
 
 extern char _sinittext[], _einittext[];
-#define is_kernel_inittext(p) ({                \
-    char *__p = (char *)(unsigned long)(p);     \
-    (__p >= _sinittext) && (__p < _einittext);  \
+#define is_kernel_inittext(p) ({                                    \
+    const unsigned long __p = (unsigned long)(p);                   \
+    (__p >= SYMBOL(_sinittext)) && (__p < SYMBOL(_einittext));  \
 })
 
 extern enum system_state {
-- 
1.9.1


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

Re: [PATCH v3 3/4] xen: introduce SYMBOL

[Jan Beulich]

>>> On 06.11.18 at 23:05, <sstabellini@kernel.org> wrote:
> --- a/xen/include/xen/compiler.h
> +++ b/xen/include/xen/compiler.h
> @@ -99,6 +99,12 @@
>      __asm__ ("" : "=r"(__ptr) : "0"(ptr));      \
>      (typeof(ptr)) (__ptr + (off)); })
>  
> +/*
> + * Use RELOC_HIDE with symbols such as _stext and _etext to avoid errors
> + * on comparing pointers to different objects
> + */
> +#define SYMBOL(x)         (RELOC_HIDE((unsigned long)(x), 0))

I'm not overly happy with this rather generic name, but I have no better
suggestion. I'd appreciate though if you dropped the unnecessary
outermost pair of parentheses.

Acked-by: Jan Beulich <jbeulich@suse.com>

Jan



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

Re: [PATCH v3 4/4] xen: use SYMBOL everywhere

[Jan Beulich]

>>> On 06.11.18 at 23:05, <sstabellini@kernel.org> wrote:
> Use SYMBOL everywhere _stext, _etext, etc. are used. Technically, it
> is required when comparing and subtracting pointers [1], but use it
> everywhere to avoid confusion.

I think using it when not needed is causing more confusion. Also
why would you then not use it on all other data symbols? The
patch will end up quite a bit more reasonable in size once you drop
the unnecessary changes.

> ---
>  xen/arch/arm/alternative.c          |  7 ++--
>  xen/arch/arm/arm32/livepatch.c      |  2 +-
>  xen/arch/arm/arm64/livepatch.c      |  2 +-
>  xen/arch/arm/domain_build.c         |  2 +-
>  xen/arch/arm/livepatch.c            |  6 +--
>  xen/arch/arm/mm.c                   | 17 ++++----
>  xen/arch/arm/setup.c                |  8 ++--
>  xen/arch/x86/setup.c                | 79 +++++++++++++++++++------------------
>  xen/arch/x86/tboot.c                | 12 +++---
>  xen/arch/x86/x86_64/machine_kexec.c |  4 +-
>  xen/drivers/vpci/vpci.c             |  7 +++-
>  xen/include/asm-arm/grant_table.h   |  3 +-
>  xen/include/asm-arm/mm.h            |  4 +-
>  xen/include/asm-x86/mm.h            |  4 +-
>  xen/include/xen/kernel.h            | 24 +++++------
>  15 files changed, 97 insertions(+), 84 deletions(-)

Just like for v2: Did you really check you caught them all? The vPCI
ones I had pointed at back then were only an example. Another
example now is xen/common/kernel.c:_cmdline_parse().

Jan



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

Re: [PATCH v3 3/4] xen: introduce SYMBOL

[Stefano Stabellini]

On Thu, 8 Nov 2018, Jan Beulich wrote:
> >>> On 06.11.18 at 23:05, <sstabellini@kernel.org> wrote:
> > --- a/xen/include/xen/compiler.h
> > +++ b/xen/include/xen/compiler.h
> > @@ -99,6 +99,12 @@
> >      __asm__ ("" : "=r"(__ptr) : "0"(ptr));      \
> >      (typeof(ptr)) (__ptr + (off)); })
> >  
> > +/*
> > + * Use RELOC_HIDE with symbols such as _stext and _etext to avoid errors
> > + * on comparing pointers to different objects
> > + */
> > +#define SYMBOL(x)         (RELOC_HIDE((unsigned long)(x), 0))
> 
> I'm not overly happy with this rather generic name, but I have no better
> suggestion. I'd appreciate though if you dropped the unnecessary
> outermost pair of parentheses.

I'll do, and thanks!

> Acked-by: Jan Beulich <jbeulich@suse.com>


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

Re: [PATCH v3 4/4] xen: use SYMBOL everywhere

[Stefano Stabellini]

On Thu, 8 Nov 2018, Jan Beulich wrote:
> >>> On 06.11.18 at 23:05, <sstabellini@kernel.org> wrote:
> > Use SYMBOL everywhere _stext, _etext, etc. are used. Technically, it
> > is required when comparing and subtracting pointers [1], but use it
> > everywhere to avoid confusion.
> 
> I think using it when not needed is causing more confusion. Also
> why would you then not use it on all other data symbols? The
> patch will end up quite a bit more reasonable in size once you drop
> the unnecessary changes.

OK, I am happy to do that. It will probably be better that way.


> > ---
> >  xen/arch/arm/alternative.c          |  7 ++--
> >  xen/arch/arm/arm32/livepatch.c      |  2 +-
> >  xen/arch/arm/arm64/livepatch.c      |  2 +-
> >  xen/arch/arm/domain_build.c         |  2 +-
> >  xen/arch/arm/livepatch.c            |  6 +--
> >  xen/arch/arm/mm.c                   | 17 ++++----
> >  xen/arch/arm/setup.c                |  8 ++--
> >  xen/arch/x86/setup.c                | 79 +++++++++++++++++++------------------
> >  xen/arch/x86/tboot.c                | 12 +++---
> >  xen/arch/x86/x86_64/machine_kexec.c |  4 +-
> >  xen/drivers/vpci/vpci.c             |  7 +++-
> >  xen/include/asm-arm/grant_table.h   |  3 +-
> >  xen/include/asm-arm/mm.h            |  4 +-
> >  xen/include/asm-x86/mm.h            |  4 +-
> >  xen/include/xen/kernel.h            | 24 +++++------
> >  15 files changed, 97 insertions(+), 84 deletions(-)
> 
> Just like for v2: Did you really check you caught them all? The vPCI
> ones I had pointed at back then were only an example. Another
> example now is xen/common/kernel.c:_cmdline_parse().

It is difficult to catch them all. Any suggestion on how to make sure
there are no leftover (other than waiting for the next QAVerify scan)?
Maybe, I should just remove the word "everywhere" from the commit
message?

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

Re: [PATCH v3 4/4] xen: use SYMBOL everywhere

[Jan Beulich]

>>> On 08.11.18 at 23:27, <sstabellini@kernel.org> wrote:
> On Thu, 8 Nov 2018, Jan Beulich wrote:
>> >>> On 06.11.18 at 23:05, <sstabellini@kernel.org> wrote:
>> > ---
>> >  xen/arch/arm/alternative.c          |  7 ++--
>> >  xen/arch/arm/arm32/livepatch.c      |  2 +-
>> >  xen/arch/arm/arm64/livepatch.c      |  2 +-
>> >  xen/arch/arm/domain_build.c         |  2 +-
>> >  xen/arch/arm/livepatch.c            |  6 +--
>> >  xen/arch/arm/mm.c                   | 17 ++++----
>> >  xen/arch/arm/setup.c                |  8 ++--
>> >  xen/arch/x86/setup.c                | 79 +++++++++++++++++++------------------
>> >  xen/arch/x86/tboot.c                | 12 +++---
>> >  xen/arch/x86/x86_64/machine_kexec.c |  4 +-
>> >  xen/drivers/vpci/vpci.c             |  7 +++-
>> >  xen/include/asm-arm/grant_table.h   |  3 +-
>> >  xen/include/asm-arm/mm.h            |  4 +-
>> >  xen/include/asm-x86/mm.h            |  4 +-
>> >  xen/include/xen/kernel.h            | 24 +++++------
>> >  15 files changed, 97 insertions(+), 84 deletions(-)
>> 
>> Just like for v2: Did you really check you caught them all? The vPCI
>> ones I had pointed at back then were only an example. Another
>> example now is xen/common/kernel.c:_cmdline_parse().
> 
> It is difficult to catch them all. Any suggestion on how to make sure
> there are no leftover (other than waiting for the next QAVerify scan)?

First an foremost go through all symbols generated by linker scripts.
Most if not all will bound either side of a section.

> Maybe, I should just remove the word "everywhere" from the commit
> message?

That would help too, together with enumerating what patterns
you've looked for.

Jan



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

Re: [PATCH v3 1/4] xen/arm: initialize target

[Julien Grall]

Hi Stefano,

On 06/11/2018 22:05, Stefano Stabellini wrote:
> Initialize variable target before passing it as a parameter.
> It makes the code a bit nicer and it is a safety certification
> requirement.
> 
> M3CM Rule-9.1: The value of an object with automatic storage duration
> shall not be read before it has been set
> 
> QAVerify: 2972
> Signed-off-by: Stefano Stabellini <stefanos@xilinx.com>

Acked-by: Julien Grall <julien.grall@arm.com>

Cheers,

-- 
Julien Grall

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

Re: [PATCH v3 2/4] xen/arm: initialize access

[Julien Grall]

Hi Stefano,

On 06/11/2018 22:05, Stefano Stabellini wrote:
> Initialize variable *access before returning it back to the caller.
> It makes the code a bit nicer and it is a safety certification
> requirement.
> 
> M3CM Rule-9.1: The value of an object with automatic storage duration
> shall not be read before it has been set
> 
> QAVerify: 2962
> Signed-off-by: Stefano Stabellini <stefanos@xilinx.com>
> Acked-by: Razvan Cojocaru <rcojocaru@bitdefender.com>

Acked-by: Julien Grall <julien.grall@arm.com>

Cheers,

-- 
Julien Grall

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

Re: [PATCH v3 3/4] xen: introduce SYMBOL

[Julien Grall]

Hi Stefano,

On 06/11/2018 22:05, Stefano Stabellini wrote:
> Introduce a macro, SYMBOL, which is a simple wrapper around RELOC_HIDE
> to be used everywhere symbols such as _stext and _etext are used in the
> code.
> 
> RELOC_HIDE is needed when accessing symbols such as _stext and _etext
> because the C standard forbids for both comparisons and substraction
> (see C Standard, 6.5.6 [ISO/IEC 9899:2011] and [1]).
> 
> forbids comparisons between pointers pointing to
> different objects. _stext, _etext, etc. are all pointers to different
> objects from ANCI C point of view.
> 
> To work around potential C compiler issues (which have actually
> been found, see the comment on top of RELOC_HIDE in Linux), and to help
> with certifications, let's introduce some syntactic sugar to be used in
> following patches.
> 
> [1] https://wiki.sei.cmu.edu/confluence/display/c/ARR36-C.+Do+not+subtract+or+compare+two+pointers+that+do+not+refer+to+the+same+array
> 
> Signed-off-by: Stefano Stabellini <stefanos@xilinx.com>
> CC: JBeulich@suse.com
> CC: andrew.cooper3@citrix.com
> CC: wei.liu2@citrix.com
> ---
> Changes in v3:
> - improve commit message
> - rename __symbol to SYMBOL to avoid name space violations
> 
> Changes in v2:
> - do not cast return to char*
> - move to common header
> ---
>   xen/include/xen/compiler.h | 6 ++++++
>   1 file changed, 6 insertions(+)
> 
> diff --git a/xen/include/xen/compiler.h b/xen/include/xen/compiler.h
> index ff6c0f5..b3375f6 100644
> --- a/xen/include/xen/compiler.h
> +++ b/xen/include/xen/compiler.h
> @@ -99,6 +99,12 @@
>       __asm__ ("" : "=r"(__ptr) : "0"(ptr));      \
>       (typeof(ptr)) (__ptr + (off)); })
>   
> +/*
> + * Use RELOC_HIDE with symbols such as _stext and _etext to avoid errors

It is not clear what "errors" means here. I think it would be better to use 
"undefined behavior" instead.

With that:

Acked-by: Julien Grall <julien.grall@arm.com>

Cheers,

> + * on comparing pointers to different objects
> + */
> +#define SYMBOL(x)         (RELOC_HIDE((unsigned long)(x), 0))
> +
>   #ifdef __GCC_ASM_FLAG_OUTPUTS__
>   # define ASM_FLAG_OUT(yes, no) yes
>   #else
> 

-- 
Julien Grall

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

Re: [PATCH v3 4/4] xen: use SYMBOL everywhere

[Julien Grall]

Hi Stefano,

On 06/11/2018 22:05, Stefano Stabellini wrote:
> Use SYMBOL everywhere _stext, _etext, etc. are used. Technically, it
> is required when comparing and subtracting pointers [1], but use it
> everywhere to avoid confusion.
> 
> M3CM: Rule-18.2: Subtraction between pointers shall only be applied to
> pointers that address elements of the same array
> 
> [1] https://wiki.sei.cmu.edu/confluence/display/c/ARR36-C.+Do+not+subtract+or+compare+two+pointers+that+do+not+refer+to+the+same+array
> 
> QAVerify: 2761 (and many others)
> Signed-off-by: Stefano Stabellini <stefanos@xilinx.com>
> CC: JBeulich@suse.com
> CC: andrew.cooper3@citrix.com
> ---
> Changes in v3:
> - improve commit message
> - no hard tabs
> - rename __symbol to SYMBOL
> - fix __end_vpci_array and __start_vpci_array
> - avoid all comparisons between pointers: including (void *) casted
>    returns from SYMBOL()
> - remove useless casts to (unsigned long)
> 
> Changes in v2:
> - cast return of SYMBOL to char* when required
> - define __pa as unsigned long in is_kernel* functions
> ---
>   xen/arch/arm/alternative.c          |  7 ++--
>   xen/arch/arm/arm32/livepatch.c      |  2 +-
>   xen/arch/arm/arm64/livepatch.c      |  2 +-
>   xen/arch/arm/domain_build.c         |  2 +-
>   xen/arch/arm/livepatch.c            |  6 +--
>   xen/arch/arm/mm.c                   | 17 ++++----
>   xen/arch/arm/setup.c                |  8 ++--
>   xen/arch/x86/setup.c                | 79 +++++++++++++++++++------------------
>   xen/arch/x86/tboot.c                | 12 +++---
>   xen/arch/x86/x86_64/machine_kexec.c |  4 +-
>   xen/drivers/vpci/vpci.c             |  7 +++-
>   xen/include/asm-arm/grant_table.h   |  3 +-
>   xen/include/asm-arm/mm.h            |  4 +-
>   xen/include/asm-x86/mm.h            |  4 +-
>   xen/include/xen/kernel.h            | 24 +++++------
>   15 files changed, 97 insertions(+), 84 deletions(-)
> 
> diff --git a/xen/arch/arm/alternative.c b/xen/arch/arm/alternative.c
> index 52ed7ed..2efa9ca 100644
> --- a/xen/arch/arm/alternative.c
> +++ b/xen/arch/arm/alternative.c
> @@ -187,8 +187,8 @@ 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;
> +        mfn_t xen_mfn = virt_to_mfn(SYMBOL(_start));
> +        paddr_t xen_size = SYMBOL(_end) - SYMBOL(_start);
>           unsigned int xen_order = get_order_from_bytes(xen_size);
>           void *xenmap;
>   
> @@ -206,7 +206,8 @@ static int __apply_alternatives_multi_stop(void *unused)
>           region.begin = __alt_instructions;
>           region.end = __alt_instructions_end;
>   
> -        ret = __apply_alternatives(&region, xenmap - (void *)_start);
> +        ret = __apply_alternatives(&region,
> +                                   (unsigned long)xenmap - SYMBOL(_start));
>           /* The patching is not expected to fail during boot. */
>           BUG_ON(ret != 0);
>   
> diff --git a/xen/arch/arm/arm32/livepatch.c b/xen/arch/arm/arm32/livepatch.c
> index 41378a5..6bf9132 100644
> --- a/xen/arch/arm/arm32/livepatch.c
> +++ b/xen/arch/arm/arm32/livepatch.c
> @@ -56,7 +56,7 @@ void arch_livepatch_apply(struct livepatch_func *func)
>       else
>           insn = 0xe1a00000; /* mov r0, r0 */
>   
> -    new_ptr = func->old_addr - (void *)_start + vmap_of_xen_text;
> +    new_ptr = func->old_addr - (void *)SYMBOL(_start) + vmap_of_xen_text;

You cast again to (void *) and old_addr is a pointer as well. How is it safe?

>       len = len / sizeof(uint32_t);
>   
>       /* PATCH! */
> diff --git a/xen/arch/arm/arm64/livepatch.c b/xen/arch/arm/arm64/livepatch.c
> index 2247b92..fb1477d 100644
> --- a/xen/arch/arm/arm64/livepatch.c
> +++ b/xen/arch/arm/arm64/livepatch.c
> @@ -43,7 +43,7 @@ void arch_livepatch_apply(struct livepatch_func *func)
>       /* Verified in livepatch_verify_distance. */
>       ASSERT(insn != AARCH64_BREAK_FAULT);
>   
> -    new_ptr = func->old_addr - (void *)_start + vmap_of_xen_text;
> +    new_ptr = func->old_addr - SYMBOL(_start) + vmap_of_xen_text;

Here vmap_of_xen_text is a pointer and old_addr is a pointer too. How is it safe?

>       len = len / sizeof(uint32_t);
>   
>       /* PATCH! */
> diff --git a/xen/arch/arm/domain_build.c b/xen/arch/arm/domain_build.c
> index f552154..6c03996 100644
> --- a/xen/arch/arm/domain_build.c
> +++ b/xen/arch/arm/domain_build.c
> @@ -2090,7 +2090,7 @@ static void __init find_gnttab_region(struct domain *d,
>        * Only use the text section as it's always present and will contain
>        * enough space for a large grant table
>        */
> -    kinfo->gnttab_start = __pa(_stext);
> +    kinfo->gnttab_start = __pa(SYMBOL(_stext));

Would it make sense to introduce __pa_symbol here?

>       kinfo->gnttab_size = gnttab_dom0_frames() << PAGE_SHIFT;
>   
>   #ifdef CONFIG_ARM_32
> diff --git a/xen/arch/arm/livepatch.c b/xen/arch/arm/livepatch.c
> index 279d52c..609ab35 100644
> --- a/xen/arch/arm/livepatch.c
> +++ b/xen/arch/arm/livepatch.c
> @@ -26,8 +26,8 @@ int arch_livepatch_quiesce(void)
>       if ( vmap_of_xen_text )
>           return -EINVAL;
>   
> -    text_mfn = virt_to_mfn(_start);
> -    text_order = get_order_from_bytes(_end - _start);
> +    text_mfn = virt_to_mfn(SYMBOL(_start));
> +    text_order = get_order_from_bytes(SYMBOL(_end) - SYMBOL(_start));
>   
>       /*
>        * The text section is read-only. So re-map Xen to be able to patch
> @@ -78,7 +78,7 @@ void arch_livepatch_revert(const struct livepatch_func *func)
>       uint32_t *new_ptr;
>       unsigned int len;
>   
> -    new_ptr = func->old_addr - (void *)_start + vmap_of_xen_text;
> +    new_ptr = func->old_addr - SYMBOL(_start) + vmap_of_xen_text;

Same question as the previous old_addr.

>   
>       len = livepatch_insn_len(func);
>       memcpy(new_ptr, func->opaque, len);
> diff --git a/xen/arch/arm/mm.c b/xen/arch/arm/mm.c
> index 7a06a33..16a8afc 100644
> --- a/xen/arch/arm/mm.c
> +++ b/xen/arch/arm/mm.c
> @@ -620,7 +620,7 @@ void __init setup_pagetables(unsigned long boot_phys_offset, paddr_t xen_paddr)
>       int i;
>   
>       /* Calculate virt-to-phys offset for the new location */
> -    phys_offset = xen_paddr - (unsigned long) _start;
> +    phys_offset = xen_paddr - SYMBOL(_start);
>   
>   #ifdef CONFIG_ARM_64
>       p = (void *) xen_pgtable;
> @@ -681,7 +681,8 @@ void __init setup_pagetables(unsigned long boot_phys_offset, paddr_t xen_paddr)
>       ttbr = (uintptr_t) cpu0_pgtable + phys_offset;
>   #endif
>   
> -    relocate_xen(ttbr, _start, (void*)dest_va, _end - _start);
> +    relocate_xen(ttbr, (void*)SYMBOL(_start), (void*)dest_va,
> +                 SYMBOL(_end) - SYMBOL(_start));
>   
>       /* Clear the copy of the boot pagetables. Each secondary CPU
>        * rebuilds these itself (see head.S) */
> @@ -1089,7 +1090,7 @@ int modify_xen_mappings(unsigned long s, unsigned long e, unsigned int flags)
>   }
>   
>   enum mg { mg_clear, mg_ro, mg_rw, mg_rx };
> -static void set_pte_flags_on_range(const char *p, unsigned long l, enum mg mg)
> +static void set_pte_flags_on_range(const unsigned long p, unsigned long l, enum mg mg)
>   {
>       lpae_t pte;
>       int i;
> @@ -1100,8 +1101,8 @@ static void set_pte_flags_on_range(const char *p, unsigned long l, enum mg mg)
>       ASSERT(!((unsigned long) p & ~PAGE_MASK));
>       ASSERT(!(l & ~PAGE_MASK));
>   
> -    for ( i = (p - _start) / PAGE_SIZE;
> -          i < (p + l - _start) / PAGE_SIZE;
> +    for ( i = (p - SYMBOL(_start)) / PAGE_SIZE;
> +          i < (p + l - SYMBOL(_start)) / PAGE_SIZE;
>             i++ )
>       {
>           pte = xen_xenmap[i];
> @@ -1138,12 +1139,12 @@ static void set_pte_flags_on_range(const char *p, unsigned long l, enum mg mg)
>   void free_init_memory(void)
>   {
>       paddr_t pa = virt_to_maddr(__init_begin);
> -    unsigned long len = __init_end - __init_begin;
> +    unsigned long len = SYMBOL(__init_end) - SYMBOL(__init_begin);
>       uint32_t insn;
>       unsigned int i, nr = len / sizeof(insn);
>       uint32_t *p;
>   
> -    set_pte_flags_on_range(__init_begin, len, mg_rw);
> +    set_pte_flags_on_range(SYMBOL(__init_begin), len, mg_rw);
>   #ifdef CONFIG_ARM_32
>       /* udf instruction i.e (see A8.8.247 in ARM DDI 0406C.c) */
>       insn = 0xe7f000f0;
> @@ -1154,7 +1155,7 @@ void free_init_memory(void)
>       for ( i = 0; i < nr; i++ )
>           *(p + i) = insn;
>   
> -    set_pte_flags_on_range(__init_begin, len, mg_clear);
> +    set_pte_flags_on_range(SYMBOL(__init_begin), len, mg_clear);
>       init_domheap_pages(pa, pa + len);
>       printk("Freed %ldkB init memory.\n", (long)(__init_end-__init_begin)>>10);
>   }
> diff --git a/xen/arch/arm/setup.c b/xen/arch/arm/setup.c
> index ea2495a..e3adddf 100644
> --- a/xen/arch/arm/setup.c
> +++ b/xen/arch/arm/setup.c
> @@ -394,7 +394,8 @@ static paddr_t __init get_xen_paddr(void)
>       paddr_t paddr = 0;
>       int i;
>   
> -    min_size = (_end - _start + (XEN_PADDR_ALIGN-1)) & ~(XEN_PADDR_ALIGN-1);
> +    min_size = (SYMBOL(_end) - SYMBOL(_start) +
> +                (XEN_PADDR_ALIGN-1)) & ~(XEN_PADDR_ALIGN-1);
>   
>       /* Find the highest bank with enough space. */
>       for ( i = 0; i < mi->nr_banks; i++ )
> @@ -727,8 +728,9 @@ void __init start_xen(unsigned long boot_phys_offset,
>   
>       /* Register Xen's load address as a boot module. */
>       xen_bootmodule = add_boot_module(BOOTMOD_XEN,
> -                             (paddr_t)(uintptr_t)(_start + boot_phys_offset),
> -                             (paddr_t)(uintptr_t)(_end - _start + 1), NULL);
> +                             (paddr_t)(uintptr_t)(SYMBOL(_start) + boot_phys_offset),
> +                             (paddr_t)(uintptr_t)(SYMBOL(_end) -
> +                                                  SYMBOL(_start) + 1), NULL);

Are the casts necessary when you use SYMBOL?

>       BUG_ON(!xen_bootmodule);
>   
>       xen_paddr = get_xen_paddr();
> diff --git a/xen/arch/x86/setup.c b/xen/arch/x86/setup.c
> index 93b79c7..1a02b30 100644
> --- a/xen/arch/x86/setup.c
> +++ b/xen/arch/x86/setup.c
> @@ -578,13 +578,13 @@ static void __init kexec_reserve_area(struct e820map *e820)
>   
>   static inline bool using_2M_mapping(void)
>   {
> -    return !l1_table_offset((unsigned long)__2M_text_end) &&
> -           !l1_table_offset((unsigned long)__2M_rodata_start) &&
> -           !l1_table_offset((unsigned long)__2M_rodata_end) &&
> -           !l1_table_offset((unsigned long)__2M_init_start) &&
> -           !l1_table_offset((unsigned long)__2M_init_end) &&
> -           !l1_table_offset((unsigned long)__2M_rwdata_start) &&
> -           !l1_table_offset((unsigned long)__2M_rwdata_end);
> +    return !l1_table_offset(SYMBOL(__2M_text_end)) &&
> +           !l1_table_offset(SYMBOL(__2M_rodata_start)) &&
> +           !l1_table_offset(SYMBOL(__2M_rodata_end)) &&
> +           !l1_table_offset(SYMBOL(__2M_init_start)) &&
> +           !l1_table_offset(SYMBOL(__2M_init_end)) &&
> +           !l1_table_offset(SYMBOL(__2M_rwdata_start)) &&
> +           !l1_table_offset(SYMBOL(__2M_rwdata_end));
>   }
>   
>   static void noinline init_done(void)
> @@ -606,13 +606,13 @@ static void noinline init_done(void)
>       /* Destroy Xen's mappings, and reuse the pages. */
>       if ( using_2M_mapping() )
>       {
> -        start = (unsigned long)&__2M_init_start,
> -        end   = (unsigned long)&__2M_init_end;
> +        start = SYMBOL(&__2M_init_start),
> +        end   = SYMBOL(&__2M_init_end);
>       }
>       else
>       {
> -        start = (unsigned long)&__init_begin;
> -        end   = (unsigned long)&__init_end;
> +        start = SYMBOL(&__init_begin);
> +        end   = SYMBOL(&__init_end);
>       }
>   
>       destroy_xen_mappings(start, end);
> @@ -966,8 +966,8 @@ void __init noreturn __start_xen(unsigned long mbi_p)
>            * This needs to remain in sync with xen_in_range() and the
>            * 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_start = virt_to_mfn(SYMBOL(_stext));
> +        mod[mbi->mods_count].mod_end = SYMBOL(__2M_rwdata_end) - SYMBOL(_stext);
>       }
>   
>       modules_headroom = bzimage_headroom(bootstrap_map(mod), mod->mod_end);
> @@ -1018,7 +1018,7 @@ void __init noreturn __start_xen(unsigned long mbi_p)
>                        1UL << (PAGE_SHIFT + 32)) )
>               e = min(HYPERVISOR_VIRT_END - DIRECTMAP_VIRT_START,
>                       1UL << (PAGE_SHIFT + 32));
> -#define reloc_size ((__pa(__2M_rwdata_end) + mask) & ~mask)
> +#define reloc_size ((__pa(SYMBOL(__2M_rwdata_end)) + mask) & ~mask)
>           /* Is the region suitable for relocating Xen? */
>           if ( !xen_phys_start && e <= limit )
>           {
> @@ -1034,7 +1034,8 @@ void __init noreturn __start_xen(unsigned long mbi_p)
>            * Is the region size greater than zero and does it begin
>            * at or above the end of current Xen image placement?
>            */
> -        if ( (end > s) && (end - reloc_size + XEN_IMG_OFFSET >= __pa(_end)) )
> +        if ( (end > s) && (end - reloc_size + XEN_IMG_OFFSET >=
> +             __pa(SYMBOL(_end))) )
>           {
>               l4_pgentry_t *pl4e;
>               l3_pgentry_t *pl3e;
> @@ -1062,7 +1063,8 @@ void __init noreturn __start_xen(unsigned long mbi_p)
>                * data until after we have switched to the relocated pagetables!
>                */
>               barrier();
> -            move_memory(e + XEN_IMG_OFFSET, XEN_IMG_OFFSET, _end - _start, 1);
> +            move_memory(e + XEN_IMG_OFFSET, XEN_IMG_OFFSET,
> +                        SYMBOL(_end) - SYMBOL(_start), 1);
>   
>               /* Walk initial pagetables, relocating page directory entries. */
>               pl4e = __va(__pa(idle_pg_table));
> @@ -1103,8 +1105,8 @@ void __init noreturn __start_xen(unsigned long mbi_p)
>                * is contained in this PTE.
>                */
>               BUG_ON(using_2M_mapping() &&
> -                   l2_table_offset((unsigned long)_erodata) ==
> -                   l2_table_offset((unsigned long)_stext));
> +                   l2_table_offset(SYMBOL(_erodata)) ==
> +                   l2_table_offset(SYMBOL(_stext)));
>               *pl2e++ = l2e_from_pfn(xen_phys_start >> PAGE_SHIFT,
>                                      PAGE_HYPERVISOR_RX | _PAGE_PSE);
>               for ( i = 1; i < L2_PAGETABLE_ENTRIES; i++, pl2e++ )
> @@ -1122,22 +1124,22 @@ void __init noreturn __start_xen(unsigned long mbi_p)
>                       continue;
>                   }
>   
> -                if ( i < l2_table_offset((unsigned long)&__2M_text_end) )
> +                if ( i < l2_table_offset((unsigned long)SYMBOL(&__2M_text_end)) )
>                   {
>                       flags = PAGE_HYPERVISOR_RX | _PAGE_PSE;
>                   }
> -                else if ( i >= l2_table_offset((unsigned long)&__2M_rodata_start) &&
> -                          i <  l2_table_offset((unsigned long)&__2M_rodata_end) )
> +                else if ( i >= l2_table_offset(SYMBOL(&__2M_rodata_start)) &&
> +                          i <  l2_table_offset(SYMBOL(&__2M_rodata_end)) )
>                   {
>                       flags = PAGE_HYPERVISOR_RO | _PAGE_PSE;
>                   }
> -                else if ( i >= l2_table_offset((unsigned long)&__2M_init_start) &&
> -                          i <  l2_table_offset((unsigned long)&__2M_init_end) )
> +                else if ( i >= l2_table_offset(SYMBOL(&__2M_init_start)) &&
> +                          i <  l2_table_offset(SYMBOL(&__2M_init_end)) )
>                   {
>                       flags = PAGE_HYPERVISOR_RWX | _PAGE_PSE;
>                   }
> -                else if ( (i >= l2_table_offset((unsigned long)&__2M_rwdata_start) &&
> -                           i <  l2_table_offset((unsigned long)&__2M_rwdata_end)) )
> +                else if ( (i >= l2_table_offset(SYMBOL(&__2M_rwdata_start)) &&
> +                           i <  l2_table_offset(SYMBOL(&__2M_rwdata_end))) )
>                   {
>                       flags = PAGE_HYPERVISOR_RW | _PAGE_PSE;
>                   }
> @@ -1234,7 +1236,7 @@ void __init noreturn __start_xen(unsigned long mbi_p)
>           panic("Not enough memory to relocate Xen\n");
>   
>       /* This needs to remain in sync with xen_in_range(). */
> -    reserve_e820_ram(&boot_e820, __pa(_stext), __pa(__2M_rwdata_end));
> +    reserve_e820_ram(&boot_e820, __pa(SYMBOL(_stext)), __pa(SYMBOL(__2M_rwdata_end)));
>   
>       /* Late kexec reservation (dynamic start address). */
>       kexec_reserve_area(&boot_e820);
> @@ -1377,7 +1379,8 @@ void __init noreturn __start_xen(unsigned long mbi_p)
>       }
>   #endif
>   
> -    xen_virt_end = ((unsigned long)_end + (1UL << L2_PAGETABLE_SHIFT) - 1) &
> +    xen_virt_end = (SYMBOL(_end) +
> +                    (1UL << L2_PAGETABLE_SHIFT) - 1) &
>                      ~((1UL << L2_PAGETABLE_SHIFT) - 1);
>       destroy_xen_mappings(xen_virt_end, XEN_VIRT_START + BOOTSTRAP_MAP_BASE);
>   
> @@ -1390,22 +1393,22 @@ void __init noreturn __start_xen(unsigned long mbi_p)
>       {
>           /* Mark .text as RX (avoiding the first 2M superpage). */
>           modify_xen_mappings(XEN_VIRT_START + MB(2),
> -                            (unsigned long)&__2M_text_end,
> +                            SYMBOL(&__2M_text_end),
>                               PAGE_HYPERVISOR_RX);
>   
>           /* Mark .rodata as RO. */
> -        modify_xen_mappings((unsigned long)&__2M_rodata_start,
> -                            (unsigned long)&__2M_rodata_end,
> +        modify_xen_mappings(SYMBOL(&__2M_rodata_start),
> +                            SYMBOL(&__2M_rodata_end),
>                               PAGE_HYPERVISOR_RO);
>   
>           /* Mark .data and .bss as RW. */
> -        modify_xen_mappings((unsigned long)&__2M_rwdata_start,
> -                            (unsigned long)&__2M_rwdata_end,
> +        modify_xen_mappings(SYMBOL(&__2M_rwdata_start),
> +                            SYMBOL(&__2M_rwdata_end),
>                               PAGE_HYPERVISOR_RW);
>   
>           /* Drop the remaining mappings in the shattered superpage. */
> -        destroy_xen_mappings((unsigned long)&__2M_rwdata_end,
> -                             ROUNDUP((unsigned long)&__2M_rwdata_end, MB(2)));
> +        destroy_xen_mappings(SYMBOL(&__2M_rwdata_end),
> +                             ROUNDUP(SYMBOL(&__2M_rwdata_end), MB(2)));
>       }
>   
>       nr_pages = 0;
> @@ -1860,11 +1863,11 @@ int __hwdom_init xen_in_range(unsigned long mfn)
>            */
>   
>           /* hypervisor .text + .rodata */
> -        xen_regions[region_ro].s = __pa(&_stext);
> -        xen_regions[region_ro].e = __pa(&__2M_rodata_end);
> +        xen_regions[region_ro].s = __pa(SYMBOL(&_stext));
> +        xen_regions[region_ro].e = __pa(SYMBOL(&__2M_rodata_end));
>           /* hypervisor .data + .bss */
> -        xen_regions[region_rw].s = __pa(&__2M_rwdata_start);
> -        xen_regions[region_rw].e = __pa(&__2M_rwdata_end);
> +        xen_regions[region_rw].s = __pa(SYMBOL(&__2M_rwdata_start));
> +        xen_regions[region_rw].e = __pa(SYMBOL(&__2M_rwdata_end));
>       }
>   
>       start = (paddr_t)mfn << PAGE_SHIFT;
> diff --git a/xen/arch/x86/tboot.c b/xen/arch/x86/tboot.c
> index f3fdee4..e355232 100644
> --- a/xen/arch/x86/tboot.c
> +++ b/xen/arch/x86/tboot.c
> @@ -373,13 +373,13 @@ void tboot_shutdown(uint32_t shutdown_type)
>           g_tboot_shared->mac_regions[0].size = bootsym_phys(trampoline_end) -
>                                                 bootsym_phys(trampoline_start);
>           /* hypervisor .text + .rodata */
> -        g_tboot_shared->mac_regions[1].start = (uint64_t)__pa(&_stext);
> -        g_tboot_shared->mac_regions[1].size = __pa(&__2M_rodata_end) -
> -                                              __pa(&_stext);
> +        g_tboot_shared->mac_regions[1].start = (uint64_t)__pa(SYMBOL(&_stext));
> +        g_tboot_shared->mac_regions[1].size = __pa(SYMBOL(&__2M_rodata_end)) -
> +                                              __pa(SYMBOL(&_stext));
>           /* hypervisor .data + .bss */
> -        g_tboot_shared->mac_regions[2].start = (uint64_t)__pa(&__2M_rwdata_start);
> -        g_tboot_shared->mac_regions[2].size = __pa(&__2M_rwdata_end) -
> -                                              __pa(&__2M_rwdata_start);
> +        g_tboot_shared->mac_regions[2].start = (uint64_t)__pa(SYMBOL(&__2M_rwdata_start));
> +        g_tboot_shared->mac_regions[2].size = __pa(SYMBOL(&__2M_rwdata_end)) -
> +                                              __pa(SYMBOL(&__2M_rwdata_start));
>   
>           /*
>            * MAC domains and other Xen memory
> diff --git a/xen/arch/x86/x86_64/machine_kexec.c b/xen/arch/x86/x86_64/machine_kexec.c
> index f4a005c..91936db 100644
> --- a/xen/arch/x86/x86_64/machine_kexec.c
> +++ b/xen/arch/x86/x86_64/machine_kexec.c
> @@ -13,8 +13,8 @@
>   
>   int machine_kexec_get_xen(xen_kexec_range_t *range)
>   {
> -        range->start = virt_to_maddr(_start);
> -        range->size = virt_to_maddr(_end) - (unsigned long)range->start;
> +        range->start = virt_to_maddr(SYMBOL(_start));
> +        range->size = virt_to_maddr(SYMBOL(_end)) - (unsigned long)range->start;
>           return 0;
>   }
>   
> diff --git a/xen/drivers/vpci/vpci.c b/xen/drivers/vpci/vpci.c
> index 82607bd..df2ca47 100644
> --- a/xen/drivers/vpci/vpci.c
> +++ b/xen/drivers/vpci/vpci.c
> @@ -33,7 +33,7 @@ struct vpci_register {
>   #ifdef __XEN__
>   extern vpci_register_init_t *const __start_vpci_array[];
>   extern vpci_register_init_t *const __end_vpci_array[];
> -#define NUM_VPCI_INIT (__end_vpci_array - __start_vpci_array)
> +#define NUM_VPCI_INIT (SYMBOL(__end_vpci_array) - SYMBOL(__start_vpci_array))
>   
>   void vpci_remove_device(struct pci_dev *pdev)
>   {
> @@ -71,6 +71,11 @@ int __hwdom_init vpci_add_handlers(struct pci_dev *pdev)
>   
>       for ( i = 0; i < NUM_VPCI_INIT; i++ )
>       {
> +        /*
> +         * We should use SYMBOL here, but it would make the code awkward
> +         * and it is not required due because there are no pointers
> +         * comparison. Leave it as is.
> +         */
>           rc = __start_vpci_array[i](pdev);
>           if ( rc )
>               break;
> diff --git a/xen/include/asm-arm/grant_table.h b/xen/include/asm-arm/grant_table.h
> index 37415b7..6c5edcc 100644
> --- a/xen/include/asm-arm/grant_table.h
> +++ b/xen/include/asm-arm/grant_table.h
> @@ -31,7 +31,8 @@ void gnttab_mark_dirty(struct domain *d, mfn_t mfn);
>    * enough space for a large grant table
>    */
>   #define gnttab_dom0_frames()                                             \
> -    min_t(unsigned int, opt_max_grant_frames, PFN_DOWN(_etext - _stext))
> +    min_t(unsigned int, opt_max_grant_frames,                            \
> +          PFN_DOWN(SYMBOL(_etext) - SYMBOL(_stext)))
>   
>   #define gnttab_init_arch(gt)                                             \
>   ({                                                                       \
> diff --git a/xen/include/asm-arm/mm.h b/xen/include/asm-arm/mm.h
> index 940b74b..61983f6 100644
> --- a/xen/include/asm-arm/mm.h
> +++ b/xen/include/asm-arm/mm.h
> @@ -151,8 +151,8 @@ extern vaddr_t xenheap_virt_start;
>   #endif
>   
>   #define is_xen_fixed_mfn(mfn)                                   \
> -    ((pfn_to_paddr(mfn) >= virt_to_maddr(&_start)) &&       \
> -     (pfn_to_paddr(mfn) <= virt_to_maddr(&_end)))
> +    ((pfn_to_paddr(mfn) >= virt_to_maddr(SYMBOL(&_start))) && \
> +     (pfn_to_paddr(mfn) <= virt_to_maddr(SYMBOL(&_end))))

__pa_symbol would help here.

>   
>   #define page_get_owner(_p)    (_p)->v.inuse.domain
>   #define page_set_owner(_p,_d) ((_p)->v.inuse.domain = (_d))
> diff --git a/xen/include/asm-x86/mm.h b/xen/include/asm-x86/mm.h
> index 6e45651..82018b2 100644
> --- a/xen/include/asm-x86/mm.h
> +++ b/xen/include/asm-x86/mm.h
> @@ -273,8 +273,8 @@ struct page_info
>   #define is_xen_heap_mfn(mfn) \
>       (__mfn_valid(mfn) && is_xen_heap_page(mfn_to_page(_mfn(mfn))))
>   #define is_xen_fixed_mfn(mfn)                     \
> -    ((((mfn) << PAGE_SHIFT) >= __pa(&_stext)) &&  \
> -     (((mfn) << PAGE_SHIFT) <= __pa(&__2M_rwdata_end)))
> +    ((((mfn) << PAGE_SHIFT) >= __pa(SYMBOL(&_stext))) &&  \
> +     (((mfn) << PAGE_SHIFT) <= __pa(SYMBOL(&__2M_rwdata_end))))

Same here.

>   
>   #define PRtype_info "016lx"/* should only be used for printk's */
>   
> diff --git a/xen/include/xen/kernel.h b/xen/include/xen/kernel.h
> index 548b64d..cd27030 100644
> --- a/xen/include/xen/kernel.h
> +++ b/xen/include/xen/kernel.h
> @@ -66,27 +66,27 @@
>   })
>   
>   extern char _start[], _end[], start[];
> -#define is_kernel(p) ({                         \
> -    char *__p = (char *)(unsigned long)(p);     \
> -    (__p >= _start) && (__p < _end);            \
> +#define is_kernel(p) ({                                             \
> +    const unsigned long __p = (unsigned long)(p);                   \
> +    (__p >= SYMBOL(_start)) && (__p < SYMBOL(_end));            \
>   })
>   
>   extern char _stext[], _etext[];
> -#define is_kernel_text(p) ({                    \
> -    char *__p = (char *)(unsigned long)(p);     \
> -    (__p >= _stext) && (__p < _etext);          \
> +#define is_kernel_text(p) ({                                        \
> +    const unsigned long __p = (unsigned long)(p);                   \
> +    (__p >= SYMBOL(_stext)) && (__p < SYMBOL(_etext));          \
>   })
>   
>   extern const char _srodata[], _erodata[];
> -#define is_kernel_rodata(p) ({                  \
> -    const char *__p = (const char *)(unsigned long)(p);     \
> -    (__p >= _srodata) && (__p < _erodata);      \
> +#define is_kernel_rodata(p) ({                                      \
> +    const unsigned long __p = (unsigned long)(p);                   \
> +    (__p >= SYMBOL(_srodata)) && (__p < SYMBOL(_erodata));      \
>   })
>   
>   extern char _sinittext[], _einittext[];
> -#define is_kernel_inittext(p) ({                \
> -    char *__p = (char *)(unsigned long)(p);     \
> -    (__p >= _sinittext) && (__p < _einittext);  \
> +#define is_kernel_inittext(p) ({                                    \
> +    const unsigned long __p = (unsigned long)(p);                   \
> +    (__p >= SYMBOL(_sinittext)) && (__p < SYMBOL(_einittext));  \
>   })
>   
>   extern enum system_state {
> 

Cheers,

-- 
Julien Grall

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

Re: [PATCH v3 0/4] misc safety certification fixes

[Julien Grall]

On 06/11/2018 22:05, Stefano Stabellini wrote:
> Hi all,

Hi,

> This short patch series fixes a few issues discovered by the safety
> certifications code scanner. The first two patches address simple
> variable initializations concerns. The third patch introduces a new
> macro that is used throughout the code in patch #4 to access _stext,
> _etext pointers and friends.
> 
> Cheers,
> 
> Stefano
> 
> 
> Stefano Stabellini (4):
>        xen/arm: initialize target
>        xen/arm: initialize access

I have merged the 2 patches above. The rest will require a respin.

Cheers,

-- 
Julien Grall

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

Re: [PATCH v3 0/4] misc safety certification fixes

[Stefano Stabellini]

On Fri, 9 Nov 2018, Julien Grall wrote:
> On 06/11/2018 22:05, Stefano Stabellini wrote:
> > Hi all,
> 
> Hi,
> 
> > This short patch series fixes a few issues discovered by the safety
> > certifications code scanner. The first two patches address simple
> > variable initializations concerns. The third patch introduces a new
> > macro that is used throughout the code in patch #4 to access _stext,
> > _etext pointers and friends.
> > 
> > Cheers,
> > 
> > Stefano
> > 
> > 
> > Stefano Stabellini (4):
> >        xen/arm: initialize target
> >        xen/arm: initialize access
> 
> I have merged the 2 patches above. The rest will require a respin.

Thanks!

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

Re: [PATCH v3 4/4] xen: use SYMBOL everywhere

[Stefano Stabellini]

On Fri, 9 Nov 2018, Julien Grall wrote:
> Hi Stefano,
> 
> On 06/11/2018 22:05, Stefano Stabellini wrote:
> > Use SYMBOL everywhere _stext, _etext, etc. are used. Technically, it
> > is required when comparing and subtracting pointers [1], but use it
> > everywhere to avoid confusion.
> > 
> > M3CM: Rule-18.2: Subtraction between pointers shall only be applied to
> > pointers that address elements of the same array
> > 
> > [1]
> > https://wiki.sei.cmu.edu/confluence/display/c/ARR36-C.+Do+not+subtract+or+compare+two+pointers+that+do+not+refer+to+the+same+array
> > 
> > QAVerify: 2761 (and many others)
> > Signed-off-by: Stefano Stabellini <stefanos@xilinx.com>
> > CC: JBeulich@suse.com
> > CC: andrew.cooper3@citrix.com
> > ---
> > Changes in v3:
> > - improve commit message
> > - no hard tabs
> > - rename __symbol to SYMBOL
> > - fix __end_vpci_array and __start_vpci_array
> > - avoid all comparisons between pointers: including (void *) casted
> >    returns from SYMBOL()
> > - remove useless casts to (unsigned long)
> > 
> > Changes in v2:
> > - cast return of SYMBOL to char* when required
> > - define __pa as unsigned long in is_kernel* functions
> > ---
> >   xen/arch/arm/alternative.c          |  7 ++--
> >   xen/arch/arm/arm32/livepatch.c      |  2 +-
> >   xen/arch/arm/arm64/livepatch.c      |  2 +-
> >   xen/arch/arm/domain_build.c         |  2 +-
> >   xen/arch/arm/livepatch.c            |  6 +--
> >   xen/arch/arm/mm.c                   | 17 ++++----
> >   xen/arch/arm/setup.c                |  8 ++--
> >   xen/arch/x86/setup.c                | 79
> > +++++++++++++++++++------------------
> >   xen/arch/x86/tboot.c                | 12 +++---
> >   xen/arch/x86/x86_64/machine_kexec.c |  4 +-
> >   xen/drivers/vpci/vpci.c             |  7 +++-
> >   xen/include/asm-arm/grant_table.h   |  3 +-
> >   xen/include/asm-arm/mm.h            |  4 +-
> >   xen/include/asm-x86/mm.h            |  4 +-
> >   xen/include/xen/kernel.h            | 24 +++++------
> >   15 files changed, 97 insertions(+), 84 deletions(-)
> > 
> > diff --git a/xen/arch/arm/alternative.c b/xen/arch/arm/alternative.c
> > index 52ed7ed..2efa9ca 100644
> > --- a/xen/arch/arm/alternative.c
> > +++ b/xen/arch/arm/alternative.c
> > @@ -187,8 +187,8 @@ 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;
> > +        mfn_t xen_mfn = virt_to_mfn(SYMBOL(_start));
> > +        paddr_t xen_size = SYMBOL(_end) - SYMBOL(_start);
> >           unsigned int xen_order = get_order_from_bytes(xen_size);
> >           void *xenmap;
> >   @@ -206,7 +206,8 @@ static int __apply_alternatives_multi_stop(void
> > *unused)
> >           region.begin = __alt_instructions;
> >           region.end = __alt_instructions_end;
> >   -        ret = __apply_alternatives(&region, xenmap - (void *)_start);
> > +        ret = __apply_alternatives(&region,
> > +                                   (unsigned long)xenmap - SYMBOL(_start));
> >           /* The patching is not expected to fail during boot. */
> >           BUG_ON(ret != 0);
> >   diff --git a/xen/arch/arm/arm32/livepatch.c
> > b/xen/arch/arm/arm32/livepatch.c
> > index 41378a5..6bf9132 100644
> > --- a/xen/arch/arm/arm32/livepatch.c
> > +++ b/xen/arch/arm/arm32/livepatch.c
> > @@ -56,7 +56,7 @@ void arch_livepatch_apply(struct livepatch_func *func)
> >       else
> >           insn = 0xe1a00000; /* mov r0, r0 */
> >   -    new_ptr = func->old_addr - (void *)_start + vmap_of_xen_text;
> > +    new_ptr = func->old_addr - (void *)SYMBOL(_start) + vmap_of_xen_text;
> 
> You cast again to (void *) and old_addr is a pointer as well. How is it safe?

It is not. I caught it after sending this version of the series to the
list. Now I am using the following:

    new_ptr = (uint32_t *)((unsigned long)func->old_addr - SYMBOL(_start) +
              (unsigned long)vmap_of_xen_text);

There are a couple of other instances exactly like this.


> >       len = len / sizeof(uint32_t);
> >         /* PATCH! */
> > diff --git a/xen/arch/arm/arm64/livepatch.c b/xen/arch/arm/arm64/livepatch.c
> > index 2247b92..fb1477d 100644
> > --- a/xen/arch/arm/arm64/livepatch.c
> > +++ b/xen/arch/arm/arm64/livepatch.c
> > @@ -43,7 +43,7 @@ void arch_livepatch_apply(struct livepatch_func *func)
> >       /* Verified in livepatch_verify_distance. */
> >       ASSERT(insn != AARCH64_BREAK_FAULT);
> >   -    new_ptr = func->old_addr - (void *)_start + vmap_of_xen_text;
> > +    new_ptr = func->old_addr - SYMBOL(_start) + vmap_of_xen_text;
> 
> Here vmap_of_xen_text is a pointer and old_addr is a pointer too. How is it
> safe?

Yes, same here as above.


> >       len = len / sizeof(uint32_t);
> >         /* PATCH! */
> > diff --git a/xen/arch/arm/domain_build.c b/xen/arch/arm/domain_build.c
> > index f552154..6c03996 100644
> > --- a/xen/arch/arm/domain_build.c
> > +++ b/xen/arch/arm/domain_build.c
> > @@ -2090,7 +2090,7 @@ static void __init find_gnttab_region(struct domain
> > *d,
> >        * Only use the text section as it's always present and will contain
> >        * enough space for a large grant table
> >        */
> > -    kinfo->gnttab_start = __pa(_stext);
> > +    kinfo->gnttab_start = __pa(SYMBOL(_stext));
> 
> Would it make sense to introduce __pa_symbol here?

Following Jan's suggestion, all the __pa(SYMBOL(_stext)) will disappear
in the next version. I have already made the change. That is because
this is not a required change (no points comparisons or subtractions.)


> >       kinfo->gnttab_size = gnttab_dom0_frames() << PAGE_SHIFT;
> >     #ifdef CONFIG_ARM_32
> > diff --git a/xen/arch/arm/livepatch.c b/xen/arch/arm/livepatch.c
> > index 279d52c..609ab35 100644
> > --- a/xen/arch/arm/livepatch.c
> > +++ b/xen/arch/arm/livepatch.c
> > @@ -26,8 +26,8 @@ int arch_livepatch_quiesce(void)
> >       if ( vmap_of_xen_text )
> >           return -EINVAL;
> >   -    text_mfn = virt_to_mfn(_start);
> > -    text_order = get_order_from_bytes(_end - _start);
> > +    text_mfn = virt_to_mfn(SYMBOL(_start));
> > +    text_order = get_order_from_bytes(SYMBOL(_end) - SYMBOL(_start));
> >         /*
> >        * The text section is read-only. So re-map Xen to be able to patch
> > @@ -78,7 +78,7 @@ void arch_livepatch_revert(const struct livepatch_func
> > *func)
> >       uint32_t *new_ptr;
> >       unsigned int len;
> >   -    new_ptr = func->old_addr - (void *)_start + vmap_of_xen_text;
> > +    new_ptr = func->old_addr - SYMBOL(_start) + vmap_of_xen_text;
> 
> Same question as the previous old_addr.

Yes, I fixed it.


> >         len = livepatch_insn_len(func);
> >       memcpy(new_ptr, func->opaque, len);
> > diff --git a/xen/arch/arm/mm.c b/xen/arch/arm/mm.c
> > index 7a06a33..16a8afc 100644
> > --- a/xen/arch/arm/mm.c
> > +++ b/xen/arch/arm/mm.c
> > @@ -620,7 +620,7 @@ void __init setup_pagetables(unsigned long
> > boot_phys_offset, paddr_t xen_paddr)
> >       int i;
> >         /* Calculate virt-to-phys offset for the new location */
> > -    phys_offset = xen_paddr - (unsigned long) _start;
> > +    phys_offset = xen_paddr - SYMBOL(_start);
> >     #ifdef CONFIG_ARM_64
> >       p = (void *) xen_pgtable;
> > @@ -681,7 +681,8 @@ void __init setup_pagetables(unsigned long
> > boot_phys_offset, paddr_t xen_paddr)
> >       ttbr = (uintptr_t) cpu0_pgtable + phys_offset;
> >   #endif
> >   -    relocate_xen(ttbr, _start, (void*)dest_va, _end - _start);
> > +    relocate_xen(ttbr, (void*)SYMBOL(_start), (void*)dest_va,
> > +                 SYMBOL(_end) - SYMBOL(_start));
> >         /* Clear the copy of the boot pagetables. Each secondary CPU
> >        * rebuilds these itself (see head.S) */
> > @@ -1089,7 +1090,7 @@ int modify_xen_mappings(unsigned long s, unsigned long
> > e, unsigned int flags)
> >   }
> >     enum mg { mg_clear, mg_ro, mg_rw, mg_rx };
> > -static void set_pte_flags_on_range(const char *p, unsigned long l, enum mg
> > mg)
> > +static void set_pte_flags_on_range(const unsigned long p, unsigned long l,
> > enum mg mg)
> >   {
> >       lpae_t pte;
> >       int i;
> > @@ -1100,8 +1101,8 @@ static void set_pte_flags_on_range(const char *p,
> > unsigned long l, enum mg mg)
> >       ASSERT(!((unsigned long) p & ~PAGE_MASK));
> >       ASSERT(!(l & ~PAGE_MASK));
> >   -    for ( i = (p - _start) / PAGE_SIZE;
> > -          i < (p + l - _start) / PAGE_SIZE;
> > +    for ( i = (p - SYMBOL(_start)) / PAGE_SIZE;
> > +          i < (p + l - SYMBOL(_start)) / PAGE_SIZE;
> >             i++ )
> >       {
> >           pte = xen_xenmap[i];
> > @@ -1138,12 +1139,12 @@ static void set_pte_flags_on_range(const char *p,
> > unsigned long l, enum mg mg)
> >   void free_init_memory(void)
> >   {
> >       paddr_t pa = virt_to_maddr(__init_begin);
> > -    unsigned long len = __init_end - __init_begin;
> > +    unsigned long len = SYMBOL(__init_end) - SYMBOL(__init_begin);
> >       uint32_t insn;
> >       unsigned int i, nr = len / sizeof(insn);
> >       uint32_t *p;
> >   -    set_pte_flags_on_range(__init_begin, len, mg_rw);
> > +    set_pte_flags_on_range(SYMBOL(__init_begin), len, mg_rw);
> >   #ifdef CONFIG_ARM_32
> >       /* udf instruction i.e (see A8.8.247 in ARM DDI 0406C.c) */
> >       insn = 0xe7f000f0;
> > @@ -1154,7 +1155,7 @@ void free_init_memory(void)
> >       for ( i = 0; i < nr; i++ )
> >           *(p + i) = insn;
> >   -    set_pte_flags_on_range(__init_begin, len, mg_clear);
> > +    set_pte_flags_on_range(SYMBOL(__init_begin), len, mg_clear);
> >       init_domheap_pages(pa, pa + len);
> >       printk("Freed %ldkB init memory.\n",
> > (long)(__init_end-__init_begin)>>10);
> >   }
> > diff --git a/xen/arch/arm/setup.c b/xen/arch/arm/setup.c
> > index ea2495a..e3adddf 100644
> > --- a/xen/arch/arm/setup.c
> > +++ b/xen/arch/arm/setup.c
> > @@ -394,7 +394,8 @@ static paddr_t __init get_xen_paddr(void)
> >       paddr_t paddr = 0;
> >       int i;
> >   -    min_size = (_end - _start + (XEN_PADDR_ALIGN-1)) &
> > ~(XEN_PADDR_ALIGN-1);
> > +    min_size = (SYMBOL(_end) - SYMBOL(_start) +
> > +                (XEN_PADDR_ALIGN-1)) & ~(XEN_PADDR_ALIGN-1);
> >         /* Find the highest bank with enough space. */
> >       for ( i = 0; i < mi->nr_banks; i++ )
> > @@ -727,8 +728,9 @@ void __init start_xen(unsigned long boot_phys_offset,
> >         /* Register Xen's load address as a boot module. */
> >       xen_bootmodule = add_boot_module(BOOTMOD_XEN,
> > -                             (paddr_t)(uintptr_t)(_start +
> > boot_phys_offset),
> > -                             (paddr_t)(uintptr_t)(_end - _start + 1),
> > NULL);
> > +                             (paddr_t)(uintptr_t)(SYMBOL(_start) +
> > boot_phys_offset),
> > +                             (paddr_t)(uintptr_t)(SYMBOL(_end) -
> > +                                                  SYMBOL(_start) + 1),
> > NULL);
> 
> Are the casts necessary when you use SYMBOL?
 
I can remove the cast to (uintptr_t) that is unnecessary


> >       BUG_ON(!xen_bootmodule);
> >         xen_paddr = get_xen_paddr();
> > diff --git a/xen/arch/x86/setup.c b/xen/arch/x86/setup.c
> > index 93b79c7..1a02b30 100644
> > --- a/xen/arch/x86/setup.c
> > +++ b/xen/arch/x86/setup.c
> > @@ -578,13 +578,13 @@ static void __init kexec_reserve_area(struct e820map
> > *e820)
> >     static inline bool using_2M_mapping(void)
> >   {
> > -    return !l1_table_offset((unsigned long)__2M_text_end) &&
> > -           !l1_table_offset((unsigned long)__2M_rodata_start) &&
> > -           !l1_table_offset((unsigned long)__2M_rodata_end) &&
> > -           !l1_table_offset((unsigned long)__2M_init_start) &&
> > -           !l1_table_offset((unsigned long)__2M_init_end) &&
> > -           !l1_table_offset((unsigned long)__2M_rwdata_start) &&
> > -           !l1_table_offset((unsigned long)__2M_rwdata_end);
> > +    return !l1_table_offset(SYMBOL(__2M_text_end)) &&
> > +           !l1_table_offset(SYMBOL(__2M_rodata_start)) &&
> > +           !l1_table_offset(SYMBOL(__2M_rodata_end)) &&
> > +           !l1_table_offset(SYMBOL(__2M_init_start)) &&
> > +           !l1_table_offset(SYMBOL(__2M_init_end)) &&
> > +           !l1_table_offset(SYMBOL(__2M_rwdata_start)) &&
> > +           !l1_table_offset(SYMBOL(__2M_rwdata_end));
> >   }
> >     static void noinline init_done(void)
> > @@ -606,13 +606,13 @@ static void noinline init_done(void)
> >       /* Destroy Xen's mappings, and reuse the pages. */
> >       if ( using_2M_mapping() )
> >       {
> > -        start = (unsigned long)&__2M_init_start,
> > -        end   = (unsigned long)&__2M_init_end;
> > +        start = SYMBOL(&__2M_init_start),
> > +        end   = SYMBOL(&__2M_init_end);
> >       }
> >       else
> >       {
> > -        start = (unsigned long)&__init_begin;
> > -        end   = (unsigned long)&__init_end;
> > +        start = SYMBOL(&__init_begin);
> > +        end   = SYMBOL(&__init_end);
> >       }
> >         destroy_xen_mappings(start, end);
> > @@ -966,8 +966,8 @@ void __init noreturn __start_xen(unsigned long mbi_p)
> >            * This needs to remain in sync with xen_in_range() and the
> >            * 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_start = virt_to_mfn(SYMBOL(_stext));
> > +        mod[mbi->mods_count].mod_end = SYMBOL(__2M_rwdata_end) -
> > SYMBOL(_stext);
> >       }
> >         modules_headroom = bzimage_headroom(bootstrap_map(mod),
> > mod->mod_end);
> > @@ -1018,7 +1018,7 @@ void __init noreturn __start_xen(unsigned long mbi_p)
> >                        1UL << (PAGE_SHIFT + 32)) )
> >               e = min(HYPERVISOR_VIRT_END - DIRECTMAP_VIRT_START,
> >                       1UL << (PAGE_SHIFT + 32));
> > -#define reloc_size ((__pa(__2M_rwdata_end) + mask) & ~mask)
> > +#define reloc_size ((__pa(SYMBOL(__2M_rwdata_end)) + mask) & ~mask)
> >           /* Is the region suitable for relocating Xen? */
> >           if ( !xen_phys_start && e <= limit )
> >           {
> > @@ -1034,7 +1034,8 @@ void __init noreturn __start_xen(unsigned long mbi_p)
> >            * Is the region size greater than zero and does it begin
> >            * at or above the end of current Xen image placement?
> >            */
> > -        if ( (end > s) && (end - reloc_size + XEN_IMG_OFFSET >= __pa(_end))
> > )
> > +        if ( (end > s) && (end - reloc_size + XEN_IMG_OFFSET >=
> > +             __pa(SYMBOL(_end))) )
> >           {
> >               l4_pgentry_t *pl4e;
> >               l3_pgentry_t *pl3e;
> > @@ -1062,7 +1063,8 @@ void __init noreturn __start_xen(unsigned long mbi_p)
> >                * data until after we have switched to the relocated
> > pagetables!
> >                */
> >               barrier();
> > -            move_memory(e + XEN_IMG_OFFSET, XEN_IMG_OFFSET, _end - _start,
> > 1);
> > +            move_memory(e + XEN_IMG_OFFSET, XEN_IMG_OFFSET,
> > +                        SYMBOL(_end) - SYMBOL(_start), 1);
> >                 /* Walk initial pagetables, relocating page directory
> > entries. */
> >               pl4e = __va(__pa(idle_pg_table));
> > @@ -1103,8 +1105,8 @@ void __init noreturn __start_xen(unsigned long mbi_p)
> >                * is contained in this PTE.
> >                */
> >               BUG_ON(using_2M_mapping() &&
> > -                   l2_table_offset((unsigned long)_erodata) ==
> > -                   l2_table_offset((unsigned long)_stext));
> > +                   l2_table_offset(SYMBOL(_erodata)) ==
> > +                   l2_table_offset(SYMBOL(_stext)));
> >               *pl2e++ = l2e_from_pfn(xen_phys_start >> PAGE_SHIFT,
> >                                      PAGE_HYPERVISOR_RX | _PAGE_PSE);
> >               for ( i = 1; i < L2_PAGETABLE_ENTRIES; i++, pl2e++ )
> > @@ -1122,22 +1124,22 @@ void __init noreturn __start_xen(unsigned long
> > mbi_p)
> >                       continue;
> >                   }
> >   -                if ( i < l2_table_offset((unsigned long)&__2M_text_end) )
> > +                if ( i < l2_table_offset((unsigned
> > long)SYMBOL(&__2M_text_end)) )
> >                   {
> >                       flags = PAGE_HYPERVISOR_RX | _PAGE_PSE;
> >                   }
> > -                else if ( i >= l2_table_offset((unsigned
> > long)&__2M_rodata_start) &&
> > -                          i <  l2_table_offset((unsigned
> > long)&__2M_rodata_end) )
> > +                else if ( i >= l2_table_offset(SYMBOL(&__2M_rodata_start))
> > &&
> > +                          i <  l2_table_offset(SYMBOL(&__2M_rodata_end)) )
> >                   {
> >                       flags = PAGE_HYPERVISOR_RO | _PAGE_PSE;
> >                   }
> > -                else if ( i >= l2_table_offset((unsigned
> > long)&__2M_init_start) &&
> > -                          i <  l2_table_offset((unsigned
> > long)&__2M_init_end) )
> > +                else if ( i >= l2_table_offset(SYMBOL(&__2M_init_start)) &&
> > +                          i <  l2_table_offset(SYMBOL(&__2M_init_end)) )
> >                   {
> >                       flags = PAGE_HYPERVISOR_RWX | _PAGE_PSE;
> >                   }
> > -                else if ( (i >= l2_table_offset((unsigned
> > long)&__2M_rwdata_start) &&
> > -                           i <  l2_table_offset((unsigned
> > long)&__2M_rwdata_end)) )
> > +                else if ( (i >= l2_table_offset(SYMBOL(&__2M_rwdata_start))
> > &&
> > +                           i <  l2_table_offset(SYMBOL(&__2M_rwdata_end)))
> > )
> >                   {
> >                       flags = PAGE_HYPERVISOR_RW | _PAGE_PSE;
> >                   }
> > @@ -1234,7 +1236,7 @@ void __init noreturn __start_xen(unsigned long mbi_p)
> >           panic("Not enough memory to relocate Xen\n");
> >         /* This needs to remain in sync with xen_in_range(). */
> > -    reserve_e820_ram(&boot_e820, __pa(_stext), __pa(__2M_rwdata_end));
> > +    reserve_e820_ram(&boot_e820, __pa(SYMBOL(_stext)),
> > __pa(SYMBOL(__2M_rwdata_end)));
> >         /* Late kexec reservation (dynamic start address). */
> >       kexec_reserve_area(&boot_e820);
> > @@ -1377,7 +1379,8 @@ void __init noreturn __start_xen(unsigned long mbi_p)
> >       }
> >   #endif
> >   -    xen_virt_end = ((unsigned long)_end + (1UL << L2_PAGETABLE_SHIFT) -
> > 1) &
> > +    xen_virt_end = (SYMBOL(_end) +
> > +                    (1UL << L2_PAGETABLE_SHIFT) - 1) &
> >                      ~((1UL << L2_PAGETABLE_SHIFT) - 1);
> >       destroy_xen_mappings(xen_virt_end, XEN_VIRT_START +
> > BOOTSTRAP_MAP_BASE);
> >   @@ -1390,22 +1393,22 @@ void __init noreturn __start_xen(unsigned long
> > mbi_p)
> >       {
> >           /* Mark .text as RX (avoiding the first 2M superpage). */
> >           modify_xen_mappings(XEN_VIRT_START + MB(2),
> > -                            (unsigned long)&__2M_text_end,
> > +                            SYMBOL(&__2M_text_end),
> >                               PAGE_HYPERVISOR_RX);
> >             /* Mark .rodata as RO. */
> > -        modify_xen_mappings((unsigned long)&__2M_rodata_start,
> > -                            (unsigned long)&__2M_rodata_end,
> > +        modify_xen_mappings(SYMBOL(&__2M_rodata_start),
> > +                            SYMBOL(&__2M_rodata_end),
> >                               PAGE_HYPERVISOR_RO);
> >             /* Mark .data and .bss as RW. */
> > -        modify_xen_mappings((unsigned long)&__2M_rwdata_start,
> > -                            (unsigned long)&__2M_rwdata_end,
> > +        modify_xen_mappings(SYMBOL(&__2M_rwdata_start),
> > +                            SYMBOL(&__2M_rwdata_end),
> >                               PAGE_HYPERVISOR_RW);
> >             /* Drop the remaining mappings in the shattered superpage. */
> > -        destroy_xen_mappings((unsigned long)&__2M_rwdata_end,
> > -                             ROUNDUP((unsigned long)&__2M_rwdata_end,
> > MB(2)));
> > +        destroy_xen_mappings(SYMBOL(&__2M_rwdata_end),
> > +                             ROUNDUP(SYMBOL(&__2M_rwdata_end), MB(2)));
> >       }
> >         nr_pages = 0;
> > @@ -1860,11 +1863,11 @@ int __hwdom_init xen_in_range(unsigned long mfn)
> >            */
> >             /* hypervisor .text + .rodata */
> > -        xen_regions[region_ro].s = __pa(&_stext);
> > -        xen_regions[region_ro].e = __pa(&__2M_rodata_end);
> > +        xen_regions[region_ro].s = __pa(SYMBOL(&_stext));
> > +        xen_regions[region_ro].e = __pa(SYMBOL(&__2M_rodata_end));
> >           /* hypervisor .data + .bss */
> > -        xen_regions[region_rw].s = __pa(&__2M_rwdata_start);
> > -        xen_regions[region_rw].e = __pa(&__2M_rwdata_end);
> > +        xen_regions[region_rw].s = __pa(SYMBOL(&__2M_rwdata_start));
> > +        xen_regions[region_rw].e = __pa(SYMBOL(&__2M_rwdata_end));
> >       }
> >         start = (paddr_t)mfn << PAGE_SHIFT;
> > diff --git a/xen/arch/x86/tboot.c b/xen/arch/x86/tboot.c
> > index f3fdee4..e355232 100644
> > --- a/xen/arch/x86/tboot.c
> > +++ b/xen/arch/x86/tboot.c
> > @@ -373,13 +373,13 @@ void tboot_shutdown(uint32_t shutdown_type)
> >           g_tboot_shared->mac_regions[0].size = bootsym_phys(trampoline_end)
> > -
> >                                                 bootsym_phys(trampoline_start);
> >           /* hypervisor .text + .rodata */
> > -        g_tboot_shared->mac_regions[1].start = (uint64_t)__pa(&_stext);
> > -        g_tboot_shared->mac_regions[1].size = __pa(&__2M_rodata_end) -
> > -                                              __pa(&_stext);
> > +        g_tboot_shared->mac_regions[1].start =
> > (uint64_t)__pa(SYMBOL(&_stext));
> > +        g_tboot_shared->mac_regions[1].size =
> > __pa(SYMBOL(&__2M_rodata_end)) -
> > +                                              __pa(SYMBOL(&_stext));
> >           /* hypervisor .data + .bss */
> > -        g_tboot_shared->mac_regions[2].start =
> > (uint64_t)__pa(&__2M_rwdata_start);
> > -        g_tboot_shared->mac_regions[2].size = __pa(&__2M_rwdata_end) -
> > -                                              __pa(&__2M_rwdata_start);
> > +        g_tboot_shared->mac_regions[2].start =
> > (uint64_t)__pa(SYMBOL(&__2M_rwdata_start));
> > +        g_tboot_shared->mac_regions[2].size =
> > __pa(SYMBOL(&__2M_rwdata_end)) -
> > +
> > __pa(SYMBOL(&__2M_rwdata_start));
> >             /*
> >            * MAC domains and other Xen memory
> > diff --git a/xen/arch/x86/x86_64/machine_kexec.c
> > b/xen/arch/x86/x86_64/machine_kexec.c
> > index f4a005c..91936db 100644
> > --- a/xen/arch/x86/x86_64/machine_kexec.c
> > +++ b/xen/arch/x86/x86_64/machine_kexec.c
> > @@ -13,8 +13,8 @@
> >     int machine_kexec_get_xen(xen_kexec_range_t *range)
> >   {
> > -        range->start = virt_to_maddr(_start);
> > -        range->size = virt_to_maddr(_end) - (unsigned long)range->start;
> > +        range->start = virt_to_maddr(SYMBOL(_start));
> > +        range->size = virt_to_maddr(SYMBOL(_end)) - (unsigned
> > long)range->start;
> >           return 0;
> >   }
> >   diff --git a/xen/drivers/vpci/vpci.c b/xen/drivers/vpci/vpci.c
> > index 82607bd..df2ca47 100644
> > --- a/xen/drivers/vpci/vpci.c
> > +++ b/xen/drivers/vpci/vpci.c
> > @@ -33,7 +33,7 @@ struct vpci_register {
> >   #ifdef __XEN__
> >   extern vpci_register_init_t *const __start_vpci_array[];
> >   extern vpci_register_init_t *const __end_vpci_array[];
> > -#define NUM_VPCI_INIT (__end_vpci_array - __start_vpci_array)
> > +#define NUM_VPCI_INIT (SYMBOL(__end_vpci_array) -
> > SYMBOL(__start_vpci_array))
> >     void vpci_remove_device(struct pci_dev *pdev)
> >   {
> > @@ -71,6 +71,11 @@ int __hwdom_init vpci_add_handlers(struct pci_dev *pdev)
> >         for ( i = 0; i < NUM_VPCI_INIT; i++ )
> >       {
> > +        /*
> > +         * We should use SYMBOL here, but it would make the code awkward
> > +         * and it is not required due because there are no pointers
> > +         * comparison. Leave it as is.
> > +         */
> >           rc = __start_vpci_array[i](pdev);
> >           if ( rc )
> >               break;
> > diff --git a/xen/include/asm-arm/grant_table.h
> > b/xen/include/asm-arm/grant_table.h
> > index 37415b7..6c5edcc 100644
> > --- a/xen/include/asm-arm/grant_table.h
> > +++ b/xen/include/asm-arm/grant_table.h
> > @@ -31,7 +31,8 @@ void gnttab_mark_dirty(struct domain *d, mfn_t mfn);
> >    * enough space for a large grant table
> >    */
> >   #define gnttab_dom0_frames()                                             \
> > -    min_t(unsigned int, opt_max_grant_frames, PFN_DOWN(_etext - _stext))
> > +    min_t(unsigned int, opt_max_grant_frames,                            \
> > +          PFN_DOWN(SYMBOL(_etext) - SYMBOL(_stext)))
> >     #define gnttab_init_arch(gt)
> > \
> >   ({                                                                       \
> > diff --git a/xen/include/asm-arm/mm.h b/xen/include/asm-arm/mm.h
> > index 940b74b..61983f6 100644
> > --- a/xen/include/asm-arm/mm.h
> > +++ b/xen/include/asm-arm/mm.h
> > @@ -151,8 +151,8 @@ extern vaddr_t xenheap_virt_start;
> >   #endif
> >     #define is_xen_fixed_mfn(mfn)                                   \
> > -    ((pfn_to_paddr(mfn) >= virt_to_maddr(&_start)) &&       \
> > -     (pfn_to_paddr(mfn) <= virt_to_maddr(&_end)))
> > +    ((pfn_to_paddr(mfn) >= virt_to_maddr(SYMBOL(&_start))) && \
> > +     (pfn_to_paddr(mfn) <= virt_to_maddr(SYMBOL(&_end))))
> 
> __pa_symbol would help here.

I have removed these changes.


> >     #define page_get_owner(_p)    (_p)->v.inuse.domain
> >   #define page_set_owner(_p,_d) ((_p)->v.inuse.domain = (_d))
> > diff --git a/xen/include/asm-x86/mm.h b/xen/include/asm-x86/mm.h
> > index 6e45651..82018b2 100644
> > --- a/xen/include/asm-x86/mm.h
> > +++ b/xen/include/asm-x86/mm.h
> > @@ -273,8 +273,8 @@ struct page_info
> >   #define is_xen_heap_mfn(mfn) \
> >       (__mfn_valid(mfn) && is_xen_heap_page(mfn_to_page(_mfn(mfn))))
> >   #define is_xen_fixed_mfn(mfn)                     \
> > -    ((((mfn) << PAGE_SHIFT) >= __pa(&_stext)) &&  \
> > -     (((mfn) << PAGE_SHIFT) <= __pa(&__2M_rwdata_end)))
> > +    ((((mfn) << PAGE_SHIFT) >= __pa(SYMBOL(&_stext))) &&  \
> > +     (((mfn) << PAGE_SHIFT) <= __pa(SYMBOL(&__2M_rwdata_end))))
> 
> Same here.

These changes have also been removed


> >     #define PRtype_info "016lx"/* should only be used for printk's */
> >   diff --git a/xen/include/xen/kernel.h b/xen/include/xen/kernel.h
> > index 548b64d..cd27030 100644
> > --- a/xen/include/xen/kernel.h
> > +++ b/xen/include/xen/kernel.h
> > @@ -66,27 +66,27 @@
> >   })
> >     extern char _start[], _end[], start[];
> > -#define is_kernel(p) ({                         \
> > -    char *__p = (char *)(unsigned long)(p);     \
> > -    (__p >= _start) && (__p < _end);            \
> > +#define is_kernel(p) ({                                             \
> > +    const unsigned long __p = (unsigned long)(p);                   \
> > +    (__p >= SYMBOL(_start)) && (__p < SYMBOL(_end));            \
> >   })
> >     extern char _stext[], _etext[];
> > -#define is_kernel_text(p) ({                    \
> > -    char *__p = (char *)(unsigned long)(p);     \
> > -    (__p >= _stext) && (__p < _etext);          \
> > +#define is_kernel_text(p) ({                                        \
> > +    const unsigned long __p = (unsigned long)(p);                   \
> > +    (__p >= SYMBOL(_stext)) && (__p < SYMBOL(_etext));          \
> >   })
> >     extern const char _srodata[], _erodata[];
> > -#define is_kernel_rodata(p) ({                  \
> > -    const char *__p = (const char *)(unsigned long)(p);     \
> > -    (__p >= _srodata) && (__p < _erodata);      \
> > +#define is_kernel_rodata(p) ({                                      \
> > +    const unsigned long __p = (unsigned long)(p);                   \
> > +    (__p >= SYMBOL(_srodata)) && (__p < SYMBOL(_erodata));      \
> >   })
> >     extern char _sinittext[], _einittext[];
> > -#define is_kernel_inittext(p) ({                \
> > -    char *__p = (char *)(unsigned long)(p);     \
> > -    (__p >= _sinittext) && (__p < _einittext);  \
> > +#define is_kernel_inittext(p) ({                                    \
> > +    const unsigned long __p = (unsigned long)(p);                   \
> > +    (__p >= SYMBOL(_sinittext)) && (__p < SYMBOL(_einittext));  \
> >   })
> >     extern enum system_state {
> > 
> 
> Cheers,
> 
> -- 
> Julien Grall
> 

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

Re: [PATCH v3 4/4] xen: use SYMBOL everywhere

[Julien Grall]

Hi Stefano,

On 11/8/18 10:27 PM, Stefano Stabellini wrote:
> On Thu, 8 Nov 2018, Jan Beulich wrote:
>>>>> On 06.11.18 at 23:05, <sstabellini@kernel.org> wrote:
>>> Use SYMBOL everywhere _stext, _etext, etc. are used. Technically, it
>>> is required when comparing and subtracting pointers [1], but use it
>>> everywhere to avoid confusion.
>>
>> I think using it when not needed is causing more confusion. Also
>> why would you then not use it on all other data symbols? The
>> patch will end up quite a bit more reasonable in size once you drop
>> the unnecessary changes.
> 
> OK, I am happy to do that. It will probably be better that way.
> 
> 
>>> ---
>>>   xen/arch/arm/alternative.c          |  7 ++--
>>>   xen/arch/arm/arm32/livepatch.c      |  2 +-
>>>   xen/arch/arm/arm64/livepatch.c      |  2 +-
>>>   xen/arch/arm/domain_build.c         |  2 +-
>>>   xen/arch/arm/livepatch.c            |  6 +--
>>>   xen/arch/arm/mm.c                   | 17 ++++----
>>>   xen/arch/arm/setup.c                |  8 ++--
>>>   xen/arch/x86/setup.c                | 79 +++++++++++++++++++------------------
>>>   xen/arch/x86/tboot.c                | 12 +++---
>>>   xen/arch/x86/x86_64/machine_kexec.c |  4 +-
>>>   xen/drivers/vpci/vpci.c             |  7 +++-
>>>   xen/include/asm-arm/grant_table.h   |  3 +-
>>>   xen/include/asm-arm/mm.h            |  4 +-
>>>   xen/include/asm-x86/mm.h            |  4 +-
>>>   xen/include/xen/kernel.h            | 24 +++++------
>>>   15 files changed, 97 insertions(+), 84 deletions(-)
>>
>> Just like for v2: Did you really check you caught them all? The vPCI
>> ones I had pointed at back then were only an example. Another
>> example now is xen/common/kernel.c:_cmdline_parse().
> 
> It is difficult to catch them all. Any suggestion on how to make sure
> there are no leftover (other than waiting for the next QAVerify scan)?

The webpage [1] seems to suggest coverity would be able to catch the 
undefined behavior fixed in that patch.

I am not sure what version of coverity is used to analyze Xen, but it 
probably worth to have a try.

Cheers,

[1] 
https://wiki.sei.cmu.edu/confluence/display/c/ARR36-C.+Do+not+subtract+or+compare+two+pointers+that+do+not+refer+to+the+same+array

-- 
Julien Grall

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

Re: [PATCH v3 4/4] xen: use SYMBOL everywhere

[Stefano Stabellini]

On Mon, 12 Nov 2018, Julien Grall wrote:
> Hi Stefano,
> 
> On 11/8/18 10:27 PM, Stefano Stabellini wrote:
> > On Thu, 8 Nov 2018, Jan Beulich wrote:
> > > > > > On 06.11.18 at 23:05, <sstabellini@kernel.org> wrote:
> > > > Use SYMBOL everywhere _stext, _etext, etc. are used. Technically, it
> > > > is required when comparing and subtracting pointers [1], but use it
> > > > everywhere to avoid confusion.
> > > 
> > > I think using it when not needed is causing more confusion. Also
> > > why would you then not use it on all other data symbols? The
> > > patch will end up quite a bit more reasonable in size once you drop
> > > the unnecessary changes.
> > 
> > OK, I am happy to do that. It will probably be better that way.
> > 
> > 
> > > > ---
> > > >   xen/arch/arm/alternative.c          |  7 ++--
> > > >   xen/arch/arm/arm32/livepatch.c      |  2 +-
> > > >   xen/arch/arm/arm64/livepatch.c      |  2 +-
> > > >   xen/arch/arm/domain_build.c         |  2 +-
> > > >   xen/arch/arm/livepatch.c            |  6 +--
> > > >   xen/arch/arm/mm.c                   | 17 ++++----
> > > >   xen/arch/arm/setup.c                |  8 ++--
> > > >   xen/arch/x86/setup.c                | 79
> > > > +++++++++++++++++++------------------
> > > >   xen/arch/x86/tboot.c                | 12 +++---
> > > >   xen/arch/x86/x86_64/machine_kexec.c |  4 +-
> > > >   xen/drivers/vpci/vpci.c             |  7 +++-
> > > >   xen/include/asm-arm/grant_table.h   |  3 +-
> > > >   xen/include/asm-arm/mm.h            |  4 +-
> > > >   xen/include/asm-x86/mm.h            |  4 +-
> > > >   xen/include/xen/kernel.h            | 24 +++++------
> > > >   15 files changed, 97 insertions(+), 84 deletions(-)
> > > 
> > > Just like for v2: Did you really check you caught them all? The vPCI
> > > ones I had pointed at back then were only an example. Another
> > > example now is xen/common/kernel.c:_cmdline_parse().
> > 
> > It is difficult to catch them all. Any suggestion on how to make sure
> > there are no leftover (other than waiting for the next QAVerify scan)?
> 
> The webpage [1] seems to suggest coverity would be able to catch the undefined
> behavior fixed in that patch.
> 
> I am not sure what version of coverity is used to analyze Xen, but it probably
> worth to have a try.
> 
> Cheers,
> 
> [1]
> https://wiki.sei.cmu.edu/confluence/display/c/ARR36-C.+Do+not+subtract+or+compare+two+pointers+that+do+not+refer+to+the+same+array

To do this, I would need to be able to create a special branch with my
fixes for Coverity to use. However, I don't have "write access" to any
Xen Project Coverity instances at the moment.

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