[PATCH 0/4] misc safety certification fixes

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


Jan, Andrew, as you know my goal is to fix arm and common code. I think
it would be best to do the same for the x86 code as well, but let me
know if you think otherwise.


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          |  6 +--
 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                   |  9 +++--
 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/include/asm-arm/grant_table.h   |  3 +-
 xen/include/asm-arm/mm.h            | 10 ++++-
 xen/include/asm-x86/mm.h            |  4 +-
 xen/include/asm-x86/page.h          |  6 +++
 xen/include/xen/kernel.h            |  8 ++--
 18 files changed, 93 insertions(+), 73 deletions(-)

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

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

[Stefano Stabellini]

Initialize variable target before passing it as a parameter.

Signed-off-by: Stefano Stabellini <stefanos@xilinx.com>
---
 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 2/4] xen/arm: initialize access

[Stefano Stabellini]

Initialize variable *access before returning it back to the caller.

Signed-off-by: Stefano Stabellini <stefanos@xilinx.com>
---
 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..10ab308 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 = XENMEM_access_n;
 
     /* 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 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 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.

Signed-off-by: Stefano Stabellini <stefanos@xilinx.com>
---
 xen/include/asm-arm/mm.h   | 6 ++++++
 xen/include/asm-x86/page.h | 6 ++++++
 2 files changed, 12 insertions(+)

diff --git a/xen/include/asm-arm/mm.h b/xen/include/asm-arm/mm.h
index 940b74b..02ce05a 100644
--- a/xen/include/asm-arm/mm.h
+++ b/xen/include/asm-arm/mm.h
@@ -284,6 +284,12 @@ static inline uint64_t gvirt_to_maddr(vaddr_t va, paddr_t *pa,
 #define __mfn_to_virt(mfn) (maddr_to_virt((paddr_t)(mfn) << PAGE_SHIFT))
 
 /*
+ * Use RELOC_HIDE with symbols such as _stext and _etext to avoid errors
+ * on comparing pointers to different objects
+ */
+#define __symbol(x)         ((char *)RELOC_HIDE((unsigned long)(x), 0))
+
+/*
  * We define non-underscored wrappers for above conversion functions.
  * These are overriden in various source files while underscored version
  * remain intact.
diff --git a/xen/include/asm-x86/page.h b/xen/include/asm-x86/page.h
index c1e9293..3f1592b 100644
--- a/xen/include/asm-x86/page.h
+++ b/xen/include/asm-x86/page.h
@@ -274,6 +274,12 @@ void copy_page_sse2(void *, const void *);
 #define vmap_to_mfn(va)     _mfn(l1e_get_pfn(*virt_to_xen_l1e((unsigned long)(va))))
 #define vmap_to_page(va)    mfn_to_page(vmap_to_mfn(va))
 
+/*
+ * Use RELOC_HIDE with symbols such as _stext and _etext to avoid errors
+ * on comparing pointers to different objects
+ */
+#define __symbol(x)         ((char *)RELOC_HIDE((unsigned long)(x), 0))
+
 #endif /* !defined(__ASSEMBLY__) */
 
 /* Where to find each level of the linear mapping */
-- 
1.9.1


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

[PATCH 4/4] xen: use __symbol everywhere

[Stefano Stabellini]

Use __symbol everywhere _stext, _etext, etc. are used. Technically, it
is only required when comparing pointers, but use it everywhere to avoid
confusion.

Signed-off-by: Stefano Stabellini <stefanos@xilinx.com>
---
 xen/arch/arm/alternative.c          |  6 +--
 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                   |  9 +++--
 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/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            |  8 ++--
 14 files changed, 78 insertions(+), 71 deletions(-)

diff --git a/xen/arch/arm/alternative.c b/xen/arch/arm/alternative.c
index 52ed7ed..305b337 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,7 @@ static int __apply_alternatives_multi_stop(void *unused)
         region.begin = __alt_instructions;
         region.end = __alt_instructions_end;
 
-        ret = __apply_alternatives(&region, xenmap - (void *)_start);
+        ret = __apply_alternatives(&region, xenmap - (void *)__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..e71764c 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..4a31026 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 - (void *)__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..40968d0 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..006dff8 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 - (void *)__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..210b25e 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 - (unsigned long) __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, __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) */
@@ -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];
diff --git a/xen/arch/arm/setup.c b/xen/arch/arm/setup.c
index ea2495a..9d0b915 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..0a7d6c0 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((unsigned long)__symbol(__2M_text_end)) &&
+           !l1_table_offset((unsigned long)__symbol(__2M_rodata_start)) &&
+           !l1_table_offset((unsigned long)__symbol(__2M_rodata_end)) &&
+           !l1_table_offset((unsigned long)__symbol(__2M_init_start)) &&
+           !l1_table_offset((unsigned long)__symbol(__2M_init_end)) &&
+           !l1_table_offset((unsigned long)__symbol(__2M_rwdata_start)) &&
+           !l1_table_offset((unsigned long)__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 = (unsigned long)__symbol(&__2M_init_start),
+        end   = (unsigned long)__symbol(&__2M_init_end);
     }
     else
     {
-        start = (unsigned long)&__init_begin;
-        end   = (unsigned long)&__init_end;
+        start = (unsigned long)__symbol(&__init_begin);
+        end   = (unsigned long)__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((unsigned long)__symbol(_erodata)) ==
+                   l2_table_offset((unsigned long)__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((unsigned long)__symbol(&__2M_rodata_start)) &&
+                          i <  l2_table_offset((unsigned long)__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((unsigned long)__symbol(&__2M_init_start)) &&
+                          i <  l2_table_offset((unsigned long)__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((unsigned long)__symbol(&__2M_rwdata_start)) &&
+                           i <  l2_table_offset((unsigned long)__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 = ((unsigned long)__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,
+                            (unsigned long)__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((unsigned long)__symbol(&__2M_rodata_start),
+                            (unsigned long)__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((unsigned long)__symbol(&__2M_rwdata_start),
+                            (unsigned long)__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((unsigned long)__symbol(&__2M_rwdata_end),
+                             ROUNDUP((unsigned long)__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..032bf8e 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..7840761 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/include/asm-arm/grant_table.h b/xen/include/asm-arm/grant_table.h
index 37415b7..865a073 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 02ce05a..fd8e439 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..b49d00c 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..0d62300 100644
--- a/xen/include/xen/kernel.h
+++ b/xen/include/xen/kernel.h
@@ -68,25 +68,25 @@
 extern char _start[], _end[], start[];
 #define is_kernel(p) ({                         \
     char *__p = (char *)(unsigned long)(p);     \
-    (__p >= _start) && (__p < _end);            \
+    (__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);          \
+    (__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);      \
+    (__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);  \
+    (__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 3/4] xen: introduce __symbol

[Andrew Cooper]

On 15/10/18 10:56, 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 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.
>
> Signed-off-by: Stefano Stabellini <stefanos@xilinx.com>
> ---
>  xen/include/asm-arm/mm.h   | 6 ++++++
>  xen/include/asm-x86/page.h | 6 ++++++
>  2 files changed, 12 insertions(+)
>
> diff --git a/xen/include/asm-arm/mm.h b/xen/include/asm-arm/mm.h
> index 940b74b..02ce05a 100644
> --- a/xen/include/asm-arm/mm.h
> +++ b/xen/include/asm-arm/mm.h
> @@ -284,6 +284,12 @@ static inline uint64_t gvirt_to_maddr(vaddr_t va, paddr_t *pa,
>  #define __mfn_to_virt(mfn) (maddr_to_virt((paddr_t)(mfn) << PAGE_SHIFT))
>  
>  /*
> + * Use RELOC_HIDE with symbols such as _stext and _etext to avoid errors
> + * on comparing pointers to different objects
> + */
> +#define __symbol(x)         ((char *)RELOC_HIDE((unsigned long)(x), 0))

Casting to char * isn't correct.  It breaks pointer arithmetic for some
of the uses you didn't convert in the following patch, and the unsigned
long cast isn't necessary either.

Dropping both of them will keep everything working correctly, as
RELOC_HIDE() already has the correct return type.

~Andrew

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

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

[Julien Grall]

Hi,

On 15/10/2018 10:56, Stefano Stabellini wrote:
> Initialize variable target before passing it as a parameter.

As such the current code is not wrong. So please explain why you need this.

Cheers,

-- 
Julien Grall

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

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

[Julien Grall]

Hi,

Please use scripts/get_maintainers.pl to CC relevant maintainers. In 
this case you need to add Ravzan and Tamas.


On 15/10/2018 10:56, Stefano Stabellini wrote:
> Initialize variable *access before returning it back to the caller.

Same as the previous patch, why do you need this?

Cheers,

> 
> Signed-off-by: Stefano Stabellini <stefanos@xilinx.com>
> ---
>   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..10ab308 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 = XENMEM_access_n;
>   
>       /* If no setting was ever set, just return rwx. */
>       if ( !p2m->mem_access_enabled )
> 

-- 
Julien Grall

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

Re: [PATCH 3/4] xen: introduce __symbol

[Julien Grall]

Hi,

Please use scripts/get_maintainers.pl to CC relevant maintainers.

On 15/10/2018 10:56, 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 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.
> 
> Signed-off-by: Stefano Stabellini <stefanos@xilinx.com>
> ---
>   xen/include/asm-arm/mm.h   | 6 ++++++
>   xen/include/asm-x86/page.h | 6 ++++++
>   2 files changed, 12 insertions(+)
> 
> diff --git a/xen/include/asm-arm/mm.h b/xen/include/asm-arm/mm.h
> index 940b74b..02ce05a 100644
> --- a/xen/include/asm-arm/mm.h
> +++ b/xen/include/asm-arm/mm.h
> @@ -284,6 +284,12 @@ static inline uint64_t gvirt_to_maddr(vaddr_t va, paddr_t *pa,
>   #define __mfn_to_virt(mfn) (maddr_to_virt((paddr_t)(mfn) << PAGE_SHIFT))
>   
>   /*
> + * Use RELOC_HIDE with symbols such as _stext and _etext to avoid errors
> + * on comparing pointers to different objects
> + */
> +#define __symbol(x)         ((char *)RELOC_HIDE((unsigned long)(x), 0))

There are no different between arm and x86. A better place would be 
xen/compiler.h (or something common).

But, after this patch, there are even more chance the compiler will 
consider that 2 _symbol(...) will come from different objects. So how is 
this meant to help here?

Cheers,

-- 
Julien Grall

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

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

[Tamas K Lengyel]

On Mon, Oct 15, 2018 at 3:57 AM Stefano Stabellini
<sstabellini@kernel.org> wrote:
>
> Initialize variable *access before returning it back to the caller.
>
> Signed-off-by: Stefano Stabellini <stefanos@xilinx.com>
> ---
>  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..10ab308 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 = XENMEM_access_n;

 Why XENMEM_access_n and why set this at all here?

Tamas

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

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

[Stefano Stabellini]

On Mon, 15 Oct 2018, Tamas K Lengyel wrote:
> On Mon, Oct 15, 2018 at 3:57 AM Stefano Stabellini
> <sstabellini@kernel.org> wrote:
> >
> > Initialize variable *access before returning it back to the caller.
> >
> > Signed-off-by: Stefano Stabellini <stefanos@xilinx.com>
> > ---
> >  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..10ab308 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 = XENMEM_access_n;
> 
>  Why XENMEM_access_n and why set this at all here?

Hi Tamas,

Yes, I missed an explanation. Initializing variables before passing them
as parameter or as a return value to a function is a safety
certification requirement. Also, it makes the code a bit nicer.

In the specific case of this function, *access is initialized before
returning in all cases but the return -ESRCH case. I thought the nicer
way to make sure *access is always initialized, making the code more
robust, would be to initialize *access at the beginning of the function
with a restrictive value.

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

Re: [PATCH 3/4] xen: introduce __symbol

[Stefano Stabellini]

On Mon, 15 Oct 2018, Julien Grall wrote:
> Hi,
> 
> Please use scripts/get_maintainers.pl to CC relevant maintainers.
> 
> On 15/10/2018 10:56, 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 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.
> > 
> > Signed-off-by: Stefano Stabellini <stefanos@xilinx.com>
> > ---
> >   xen/include/asm-arm/mm.h   | 6 ++++++
> >   xen/include/asm-x86/page.h | 6 ++++++
> >   2 files changed, 12 insertions(+)
> > 
> > diff --git a/xen/include/asm-arm/mm.h b/xen/include/asm-arm/mm.h
> > index 940b74b..02ce05a 100644
> > --- a/xen/include/asm-arm/mm.h
> > +++ b/xen/include/asm-arm/mm.h
> > @@ -284,6 +284,12 @@ static inline uint64_t gvirt_to_maddr(vaddr_t va,
> > paddr_t *pa,
> >   #define __mfn_to_virt(mfn) (maddr_to_virt((paddr_t)(mfn) << PAGE_SHIFT))
> >     /*
> > + * Use RELOC_HIDE with symbols such as _stext and _etext to avoid errors
> > + * on comparing pointers to different objects
> > + */
> > +#define __symbol(x)         ((char *)RELOC_HIDE((unsigned long)(x), 0))
> 
> There are no different between arm and x86. A better place would be
> xen/compiler.h (or something common).

OK


> But, after this patch, there are even more chance the compiler will consider
> that 2 _symbol(...) will come from different objects. So how is this meant to
> help here?

I think the mistake was to cast the return to char*. If I remove the
cast, as Andrew suggested, then any comparison would be a comparison
between unsigned long, that should be accepted and safe.

However, the parameter to RELOC_HIDE has to be casted to unsigned long,
because most often we pass char*:

/local/repos/xen-upstream/xen/include/xen/compiler.h:100:5: error: cast specifies array type
     (typeof(ptr)) (__ptr + (off)); })

So I think the __symbol macro should be:

+#define __symbol(x)         (RELOC_HIDE((unsigned long)(x), 0))


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

Re: [PATCH 3/4] xen: introduce __symbol

[Andrew Cooper]

On 16/10/18 02:34, Stefano Stabellini wrote:
> On Mon, 15 Oct 2018, Julien Grall wrote:
>> Hi,
>>
>> Please use scripts/get_maintainers.pl to CC relevant maintainers.
>>
>> On 15/10/2018 10:56, 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 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.
>>>
>>> Signed-off-by: Stefano Stabellini <stefanos@xilinx.com>
>>> ---
>>>   xen/include/asm-arm/mm.h   | 6 ++++++
>>>   xen/include/asm-x86/page.h | 6 ++++++
>>>   2 files changed, 12 insertions(+)
>>>
>>> diff --git a/xen/include/asm-arm/mm.h b/xen/include/asm-arm/mm.h
>>> index 940b74b..02ce05a 100644
>>> --- a/xen/include/asm-arm/mm.h
>>> +++ b/xen/include/asm-arm/mm.h
>>> @@ -284,6 +284,12 @@ static inline uint64_t gvirt_to_maddr(vaddr_t va,
>>> paddr_t *pa,
>>>   #define __mfn_to_virt(mfn) (maddr_to_virt((paddr_t)(mfn) << PAGE_SHIFT))
>>>     /*
>>> + * Use RELOC_HIDE with symbols such as _stext and _etext to avoid errors
>>> + * on comparing pointers to different objects
>>> + */
>>> +#define __symbol(x)         ((char *)RELOC_HIDE((unsigned long)(x), 0))
>> There are no different between arm and x86. A better place would be
>> xen/compiler.h (or something common).
> OK
>
>
>> But, after this patch, there are even more chance the compiler will consider
>> that 2 _symbol(...) will come from different objects. So how is this meant to
>> help here?
> I think the mistake was to cast the return to char*. If I remove the
> cast, as Andrew suggested, then any comparison would be a comparison
> between unsigned long, that should be accepted and safe.
>
> However, the parameter to RELOC_HIDE has to be casted to unsigned long,
> because most often we pass char*:
>
> /local/repos/xen-upstream/xen/include/xen/compiler.h:100:5: error: cast specifies array type
>      (typeof(ptr)) (__ptr + (off)); })
>
> So I think the __symbol macro should be:
>
> +#define __symbol(x)         (RELOC_HIDE((unsigned long)(x), 0))
>

I don't see why the unsigned long cast is needed.  What is wrong by
having "char *ptr + 0" which is legal pointer arithmetic, and has well
defined behaviour?

If it is necessary, then you need a (typeof(x)) cast again on the
outside, because the unsigned long cast causes RELOC_HIDE's typeof to be
wrong overall.

~Andrew

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

Re: [PATCH 3/4] xen: introduce __symbol

[Stefano Stabellini]

[-- Attachment #1: Type: TEXT/PLAIN, Size: 3988 bytes --]

On Tue, 16 Oct 2018, Andrew Cooper wrote:
> On 16/10/18 02:34, Stefano Stabellini wrote:
> > On Mon, 15 Oct 2018, Julien Grall wrote:
> >> Hi,
> >>
> >> Please use scripts/get_maintainers.pl to CC relevant maintainers.
> >>
> >> On 15/10/2018 10:56, 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 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.
> >>>
> >>> Signed-off-by: Stefano Stabellini <stefanos@xilinx.com>
> >>> ---
> >>>   xen/include/asm-arm/mm.h   | 6 ++++++
> >>>   xen/include/asm-x86/page.h | 6 ++++++
> >>>   2 files changed, 12 insertions(+)
> >>>
> >>> diff --git a/xen/include/asm-arm/mm.h b/xen/include/asm-arm/mm.h
> >>> index 940b74b..02ce05a 100644
> >>> --- a/xen/include/asm-arm/mm.h
> >>> +++ b/xen/include/asm-arm/mm.h
> >>> @@ -284,6 +284,12 @@ static inline uint64_t gvirt_to_maddr(vaddr_t va,
> >>> paddr_t *pa,
> >>>   #define __mfn_to_virt(mfn) (maddr_to_virt((paddr_t)(mfn) << PAGE_SHIFT))
> >>>     /*
> >>> + * Use RELOC_HIDE with symbols such as _stext and _etext to avoid errors
> >>> + * on comparing pointers to different objects
> >>> + */
> >>> +#define __symbol(x)         ((char *)RELOC_HIDE((unsigned long)(x), 0))
> >> There are no different between arm and x86. A better place would be
> >> xen/compiler.h (or something common).
> > OK
> >
> >
> >> But, after this patch, there are even more chance the compiler will consider
> >> that 2 _symbol(...) will come from different objects. So how is this meant to
> >> help here?
> > I think the mistake was to cast the return to char*. If I remove the
> > cast, as Andrew suggested, then any comparison would be a comparison
> > between unsigned long, that should be accepted and safe.
> >
> > However, the parameter to RELOC_HIDE has to be casted to unsigned long,
> > because most often we pass char*:
> >
> > /local/repos/xen-upstream/xen/include/xen/compiler.h:100:5: error: cast specifies array type
> >      (typeof(ptr)) (__ptr + (off)); })
> >
> > So I think the __symbol macro should be:
> >
> > +#define __symbol(x)         (RELOC_HIDE((unsigned long)(x), 0))
> >
> 
> I don't see why the unsigned long cast is needed.  What is wrong by
> having "char *ptr + 0" which is legal pointer arithmetic, and has well
> defined behaviour?

The issue is that _stext is defined as char _stext[], so
__symbol(_stext) wouldn't work because _stext is an array type. As I
pasted above the error is "cast specifies array type". I guess it would
have to be __symbol(&_stext[0]) everywhere.

Also, I don't think it is a good idea for __symbol to return a pointer,
because then we are still left with the original issue of comparing
pointers of different sizes. We just moved the issue from the original
pointers to the ones returned by RELOC_HIDE.


> If it is necessary, then you need a (typeof(x)) cast again on the
> outside, because the unsigned long cast causes RELOC_HIDE's typeof to be
> wrong overall.

I am not sure I follow you here, do you mean:

  ((typeof(x)) RELOC_HIDE((unsigned long)(x), 0)

If so, I think it is best to have RELOC_HIDE return unsigned long (no
outside cast), hence removing all issues of comparing pointers to
different objects. Then, I'll adjust the few call sites that need
changes because of the char* -> unsigned long conversion. I have
already modified patch #4 following this strategy and the modifications
required were very few.

[-- Attachment #2: Type: text/plain, Size: 157 bytes --]

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

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

[Tamas K Lengyel]

On Mon, Oct 15, 2018 at 7:14 PM Stefano Stabellini
<sstabellini@kernel.org> wrote:
>
> On Mon, 15 Oct 2018, Tamas K Lengyel wrote:
> > On Mon, Oct 15, 2018 at 3:57 AM Stefano Stabellini
> > <sstabellini@kernel.org> wrote:
> > >
> > > Initialize variable *access before returning it back to the caller.
> > >
> > > Signed-off-by: Stefano Stabellini <stefanos@xilinx.com>
> > > ---
> > >  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..10ab308 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 = XENMEM_access_n;
> >
> >  Why XENMEM_access_n and why set this at all here?
>
> Hi Tamas,
>
> Yes, I missed an explanation. Initializing variables before passing them
> as parameter or as a return value to a function is a safety
> certification requirement. Also, it makes the code a bit nicer.
>
> In the specific case of this function, *access is initialized before
> returning in all cases but the return -ESRCH case. I thought the nicer
> way to make sure *access is always initialized, making the code more
> robust, would be to initialize *access at the beginning of the function
> with a restrictive value.

Got it, thanks, Please use p2m->default_access for this instead to be
consistent with similar code at other spots.

Tamas

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

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

[Stefano Stabellini]

On Tue, 16 Oct 2018, Tamas K Lengyel wrote:
> On Mon, Oct 15, 2018 at 7:14 PM Stefano Stabellini
> <sstabellini@kernel.org> wrote:
> >
> > On Mon, 15 Oct 2018, Tamas K Lengyel wrote:
> > > On Mon, Oct 15, 2018 at 3:57 AM Stefano Stabellini
> > > <sstabellini@kernel.org> wrote:
> > > >
> > > > Initialize variable *access before returning it back to the caller.
> > > >
> > > > Signed-off-by: Stefano Stabellini <stefanos@xilinx.com>
> > > > ---
> > > >  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..10ab308 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 = XENMEM_access_n;
> > >
> > >  Why XENMEM_access_n and why set this at all here?
> >
> > Hi Tamas,
> >
> > Yes, I missed an explanation. Initializing variables before passing them
> > as parameter or as a return value to a function is a safety
> > certification requirement. Also, it makes the code a bit nicer.
> >
> > In the specific case of this function, *access is initialized before
> > returning in all cases but the return -ESRCH case. I thought the nicer
> > way to make sure *access is always initialized, making the code more
> > robust, would be to initialize *access at the beginning of the function
> > with a restrictive value.
> 
> Got it, thanks, Please use p2m->default_access for this instead to be
> consistent with similar code at other spots.

OK, no prob

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

Re: [PATCH 3/4] xen: introduce __symbol

[Jan Beulich]

>>> On 15.10.18 at 11:56, <sstabellini@kernel.org> wrote:
> Introduce a macro, __symbol,

Irrespective of all other remarks already made, and irrespective
of me not being convinced that we really need to wrap _text,
_stext, and alike - no new name space violations please. I.e.
no leading underscores in header files, and no double leading
underscores in .c ones.

Jan



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

Re: [PATCH 4/4] xen: use __symbol everywhere

[Jan Beulich]

>>> On 15.10.18 at 11:56, <sstabellini@kernel.org> wrote:
> Use __symbol everywhere _stext, _etext, etc. are used. Technically, it
> is only required when comparing pointers, but use it everywhere to avoid
> confusion.

While the description slightly limits the scope, I'm not sure that's what
you mean. What about e.g. __{start,end}_vpci_array? If you say
"everywhere" in the title, then please change everything.

Jan



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

Re: [PATCH 4/4] xen: use __symbol everywhere

[Stefano Stabellini]

On Thu, 25 Oct 2018, Jan Beulich wrote:
> >>> On 15.10.18 at 11:56, <sstabellini@kernel.org> wrote:
> > Use __symbol everywhere _stext, _etext, etc. are used. Technically, it
> > is only required when comparing pointers, but use it everywhere to avoid
> > confusion.
> 
> While the description slightly limits the scope, I'm not sure that's what
> you mean. What about e.g. __{start,end}_vpci_array? If you say
> "everywhere" in the title, then please change everything.

I forgot about that one, I'll fix it

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