[PATCH 0/8] xen/x86: Fix build with Clang 3.5

[PATCH 0/8] xen/x86: Fix build with Clang 3.5

[Andrew Cooper]

This is a misc assortment of patches to fix the build with Clang 3.5.  The
result also appears to function.

Andrew Cooper (8):
  xen/lib: Fix ASSERT() to build with clang
  xen/misc: Remove or annotate possibly-unused functions
  xen/x86: Remove %z modifier from inline assembly
  xen/x86: Fix section type mismatch in mm.c
  xen/x86: Improve annotation of autogen_entrypoints[]
  xen/x86: Avoid overriding initialisers in arrays
  xen/x86: Fix get_cpu_info() when built with clang
  x86/efi: Generate uefi_call_wrapper() when compiling with clang

 xen/arch/x86/cpu/mtrr/generic.c      |  3 +--
 xen/arch/x86/mm.c                    |  2 +-
 xen/arch/x86/mm/p2m-ept.c            | 27 ++++++++++++++++-----------
 xen/arch/x86/mm/shadow/multi.c       |  2 ++
 xen/arch/x86/traps.c                 |  2 +-
 xen/arch/x86/xen.lds.S               |  2 +-
 xen/arch/x86/xstate.c                |  3 ++-
 xen/common/rcupdate.c                |  6 ------
 xen/include/asm-x86/current.h        |  6 ++++++
 xen/include/asm-x86/spinlock.h       |  3 ++-
 xen/include/asm-x86/x86_64/efibind.h |  2 +-
 xen/include/xen/compat.h             | 14 ++++++++------
 xen/include/xen/lib.h                |  2 +-
 13 files changed, 42 insertions(+), 32 deletions(-)

-- 
2.1.4

[PATCH 1/8] xen/lib: Fix ASSERT() to build with clang

[Andrew Cooper]

Clang warns about a semicolon immediately following an if() clause as a
possible mistake, and recommends putting the semicolon on a new line if it was
intentional.  A newline is not an option here, so use a set of empty braces
instead.

Signed-off-by: Andrew Cooper <andrew.cooper3@citrix.com>
---
CC: Jan Beulich <JBeulich@suse.com>
CC: Tim Deegan <tim@xen.org>
CC: Ian Campbell <Ian.Campbell@citrix.com>
---
 xen/include/xen/lib.h | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/xen/include/xen/lib.h b/xen/include/xen/lib.h
index 4258912..76232b2 100644
--- a/xen/include/xen/lib.h
+++ b/xen/include/xen/lib.h
@@ -32,7 +32,7 @@
 #define ASSERT_UNREACHABLE() assert_failed("unreachable")
 #define debug_build() 1
 #else
-#define ASSERT(p) do { if ( 0 && (p) ); } while (0)
+#define ASSERT(p) do { if ( 0 && (p) ){} } while (0)
 #define ASSERT_UNREACHABLE() do { } while (0)
 #define debug_build() 0
 #endif
-- 
2.1.4

[PATCH 2/8] xen/misc: Remove or annotate possibly-unused functions

[Andrew Cooper]

Clang notices more unused functions than GCC.

 * sh_next_page() is only used at GUEST_PAGING_LEVELS=2, so remove it from the
   other guest level translation units
 * rcu_batch_after() is completely unused.
 * Various of the COMPAT() generated functions are used only for their
   BUILD_BUG_ON() properties.  Annotate them all as __maybe_used.

Signed-off-by: Andrew Cooper <andrew.cooper3@citrix.com>
---
CC: Jan Beulich <JBeulich@suse.com>
CC: Tim Deegan <tim@xen.org>
CC: Ian Campbell <Ian.Campbell@citrix.com>
---
 xen/arch/x86/mm/shadow/multi.c |  2 ++
 xen/common/rcupdate.c          |  6 ------
 xen/include/xen/compat.h       | 14 ++++++++------
 3 files changed, 10 insertions(+), 12 deletions(-)

diff --git a/xen/arch/x86/mm/shadow/multi.c b/xen/arch/x86/mm/shadow/multi.c
index 4d0b317..aaf8db7 100644
--- a/xen/arch/x86/mm/shadow/multi.c
+++ b/xen/arch/x86/mm/shadow/multi.c
@@ -436,6 +436,7 @@ sh_cmpxchg_guest_entry(struct vcpu *v, intpte_t *p, intpte_t *old,
  * space.)
  */
 
+#if GUEST_PAGING_LEVELS == 2
 /* From one page of a multi-page shadow, find the next one */
 static inline mfn_t sh_next_page(mfn_t smfn)
 {
@@ -454,6 +455,7 @@ static inline mfn_t sh_next_page(mfn_t smfn)
     ASSERT(!next->u.sh.head);
     return page_to_mfn(next);
 }
+#endif
 
 static inline u32
 guest_index(void *ptr)
diff --git a/xen/common/rcupdate.c b/xen/common/rcupdate.c
index f13b87b..8cc5a82 100644
--- a/xen/common/rcupdate.c
+++ b/xen/common/rcupdate.c
@@ -141,12 +141,6 @@ static inline int rcu_batch_before(long a, long b)
     return (a - b) < 0;
 }
 
-/* Is batch a after batch b ? */
-static inline int rcu_batch_after(long a, long b)
-{
-    return (a - b) > 0;
-}
-
 static void force_quiescent_state(struct rcu_data *rdp,
                                   struct rcu_ctrlblk *rcp)
 {
diff --git a/xen/include/xen/compat.h b/xen/include/xen/compat.h
index e5c23e2..3f4cef6 100644
--- a/xen/include/xen/compat.h
+++ b/xen/include/xen/compat.h
@@ -134,14 +134,16 @@
 #define CHECK_NAME_(k, n, tag) __check ## tag ## k ## _ ## n
 
 #define CHECK_TYPE(name) \
-static inline int CHECK_NAME(name, T)(xen_ ## name ## _t *x, \
-                                      compat_ ## name ## _t *c) \
+static inline int __maybe_unused \
+CHECK_NAME(name, T)(xen_ ## name ## _t *x, \
+                    compat_ ## name ## _t *c) \
 { \
     return x == c; \
 }
 #define CHECK_TYPE_(k, n) \
-static inline int CHECK_NAME_(k, n, T)(k xen_ ## n *x, \
-                                       k compat_ ## n *c) \
+static inline int __maybe_unused \
+CHECK_NAME_(k, n, T)(k xen_ ## n *x, \
+                     k compat_ ## n *c) \
 { \
     return x == c; \
 }
@@ -154,14 +156,14 @@ static inline int CHECK_NAME_(k, n, T)(k xen_ ## n *x, \
                                           sizeof(k compat_ ## n)) * 2]
 
 #define CHECK_FIELD_COMMON(name, t, f) \
-static inline int name(xen_ ## t ## _t *x, compat_ ## t ## _t *c) \
+static inline int __maybe_unused name(xen_ ## t ## _t *x, compat_ ## t ## _t *c) \
 { \
     BUILD_BUG_ON(offsetof(xen_ ## t ## _t, f) != \
                  offsetof(compat_ ## t ## _t, f)); \
     return &x->f == &c->f; \
 }
 #define CHECK_FIELD_COMMON_(k, name, n, f) \
-static inline int name(k xen_ ## n *x, k compat_ ## n *c) \
+static inline int __maybe_unused name(k xen_ ## n *x, k compat_ ## n *c) \
 { \
     BUILD_BUG_ON(offsetof(k xen_ ## n, f) != \
                  offsetof(k compat_ ## n, f)); \
-- 
2.1.4

[PATCH 3/8] xen/x86: Remove %z modifier from inline assembly

[Andrew Cooper]

Clang doesn't support the %z modifier.  Replace both uses with an explicit l
suffix, and cover the changes with BUILD_BUG_ON()s

Signed-off-by: Andrew Cooper <andrew.cooper3@citrix.com>
---
CC: Jan Beulich <JBeulich@suse.com>
---
 xen/arch/x86/xstate.c          | 3 ++-
 xen/include/asm-x86/spinlock.h | 3 ++-
 2 files changed, 4 insertions(+), 2 deletions(-)

diff --git a/xen/arch/x86/xstate.c b/xen/arch/x86/xstate.c
index c5d17ff..4f2fb8e 100644
--- a/xen/arch/x86/xstate.c
+++ b/xen/arch/x86/xstate.c
@@ -353,11 +353,12 @@ void xrstor(struct vcpu *v, uint64_t mask)
     {
         switch ( __builtin_expect(ptr->fpu_sse.x[FPU_WORD_SIZE_OFFSET], 8) )
         {
+            BUILD_BUG_ON(sizeof(faults) != 4); /* Clang doesn't support %z in asm. */
 #define XRSTOR(pfx) \
         alternative_io("1: .byte " pfx "0x0f,0xae,0x2f\n" \
                        "3:\n" \
                        "   .section .fixup,\"ax\"\n" \
-                       "2: inc%z[faults] %[faults]\n" \
+                       "2: incl %[faults]\n" \
                        "   jmp 3b\n" \
                        "   .previous\n" \
                        _ASM_EXTABLE(1b, 2b), \
diff --git a/xen/include/asm-x86/spinlock.h b/xen/include/asm-x86/spinlock.h
index 70a85af..be72c0f 100644
--- a/xen/include/asm-x86/spinlock.h
+++ b/xen/include/asm-x86/spinlock.h
@@ -2,7 +2,8 @@
 #define __ASM_SPINLOCK_H
 
 #define _raw_read_unlock(l) \
-    asm volatile ( "lock; dec%z0 %0" : "+m" ((l)->lock) :: "memory" )
+    BUILD_BUG_ON(sizeof((l)->lock) != 4); /* Clang doesn't support %z in asm. */ \
+    asm volatile ( "lock; decl %0" : "+m" ((l)->lock) :: "memory" )
 
 /*
  * On x86 the only reordering is of reads with older writes.  In the
-- 
2.1.4

[PATCH 4/8] xen/x86: Fix section type mismatch in mm.c

[Andrew Cooper]

Clang doesn't like mixing const and non-const data in the same section.  Move
zero_page into .bss.page_aligned.const and wildcard .bss.page_aligned when
linking.

Signed-off-by: Andrew Cooper <andrew.cooper3@citrix.com>
---
CC: Jan Beulich <JBeulich@suse.com>
CC: George Dunlap <george.dunlap@eu.citrix.com>
---
 xen/arch/x86/mm.c      | 2 +-
 xen/arch/x86/xen.lds.S | 2 +-
 2 files changed, 2 insertions(+), 2 deletions(-)

diff --git a/xen/arch/x86/mm.c b/xen/arch/x86/mm.c
index d62d9ec..c214fc5 100644
--- a/xen/arch/x86/mm.c
+++ b/xen/arch/x86/mm.c
@@ -577,7 +577,7 @@ static inline void guest_get_eff_kern_l1e(struct vcpu *v, unsigned long addr,
     TOGGLE_MODE();
 }
 
-static const char __section(".bss.page_aligned") zero_page[PAGE_SIZE];
+static const char __section(".bss.page_aligned.const") zero_page[PAGE_SIZE];
 
 static void invalidate_shadow_ldt(struct vcpu *v, int flush)
 {
diff --git a/xen/arch/x86/xen.lds.S b/xen/arch/x86/xen.lds.S
index c1ce027..3b199ca 100644
--- a/xen/arch/x86/xen.lds.S
+++ b/xen/arch/x86/xen.lds.S
@@ -170,7 +170,7 @@ SECTIONS
        __bss_start = .;
        *(.bss.stack_aligned)
        . = ALIGN(PAGE_SIZE);
-       *(.bss.page_aligned)
+       *(.bss.page_aligned*)
        *(.bss)
        . = ALIGN(SMP_CACHE_BYTES);
        __per_cpu_start = .;
-- 
2.1.4

[PATCH 5/8] xen/x86: Improve annotation of autogen_entrypoints[]

[Andrew Cooper]

Clang complains that the __used attribute is not applicable to an extern.  In
this case, the only relevent attribute is that the data is constant.

Signed-off-by: Andrew Cooper <andrew.cooper3@citrix.com>
---
CC: Jan Beulich <JBeulich@suse.com>
---
 xen/arch/x86/traps.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/xen/arch/x86/traps.c b/xen/arch/x86/traps.c
index e105b95..d19250a 100644
--- a/xen/arch/x86/traps.c
+++ b/xen/arch/x86/traps.c
@@ -3689,7 +3689,7 @@ void __init init_idt_traps(void)
     this_cpu(compat_gdt_table) = boot_cpu_compat_gdt_table;
 }
 
-extern void (*__initconst autogen_entrypoints[NR_VECTORS])(void);
+extern void (*const autogen_entrypoints[NR_VECTORS])(void);
 void __init trap_init(void)
 {
     unsigned int vector;
-- 
2.1.4

[PATCH 6/8] xen/x86: Avoid overriding initialisers in arrays

[Andrew Cooper]

Clang objects to having multiple initialisers when creating an array.

As this warning is useful for spotting obscure bugs, disabling it is
unhelpful.  Instead, fix our two deliberate usecases.

In the p2m-ept case, pull the array out into a helper function, so the helper
can guarentee to cover the NULL pointer case.

No functional change.

Signed-off-by: Andrew Cooper <andrew.cooper3@citrix.com>
---
CC: Jan Beulich <JBeulich@suse.com>
CC: George Dunlap <george.dunlap@eu.citrix.com>
CC: Jun Nakajima <jun.nakajima@intel.com>
CC: Kevin Tian <kevin.tian@intel.com>
---
 xen/arch/x86/cpu/mtrr/generic.c |  3 +--
 xen/arch/x86/mm/p2m-ept.c       | 27 ++++++++++++++++-----------
 2 files changed, 17 insertions(+), 13 deletions(-)

diff --git a/xen/arch/x86/cpu/mtrr/generic.c b/xen/arch/x86/cpu/mtrr/generic.c
index ea0efe2..8839f8d 100644
--- a/xen/arch/x86/cpu/mtrr/generic.c
+++ b/xen/arch/x86/cpu/mtrr/generic.c
@@ -91,7 +91,6 @@ static const char *__init mtrr_attrib_to_str(mtrr_type x)
 {
 	static const char __initconst strings[MTRR_NUM_TYPES][16] =
 	{
-		[0 ... MTRR_NUM_TYPES - 1] = "?",
 		[MTRR_TYPE_UNCACHABLE]     = "uncachable",
 		[MTRR_TYPE_WRCOMB]         = "write-combining",
 		[MTRR_TYPE_WRTHROUGH]      = "write-through",
@@ -99,7 +98,7 @@ static const char *__init mtrr_attrib_to_str(mtrr_type x)
 		[MTRR_TYPE_WRBACK]         = "write-back",
 	};
 
-	return x < MTRR_NUM_TYPES ? strings[x] : "?";
+	return x < MTRR_NUM_TYPES ? (strings[x] ?: "?") : "?";
 }
 
 static unsigned int __initdata last_fixed_start;
diff --git a/xen/arch/x86/mm/p2m-ept.c b/xen/arch/x86/mm/p2m-ept.c
index c094320..c5fe906 100644
--- a/xen/arch/x86/mm/p2m-ept.c
+++ b/xen/arch/x86/mm/p2m-ept.c
@@ -1201,6 +1201,20 @@ void ept_p2m_uninit(struct p2m_domain *p2m)
     free_cpumask_var(ept->invalidate);
 }
 
+static const char *memory_type_to_str(unsigned int x)
+{
+    static const char memory_types[8][2] = {
+        [MTRR_TYPE_UNCACHABLE]     = "UC",
+        [MTRR_TYPE_WRCOMB]         = "WC",
+        [MTRR_TYPE_WRTHROUGH]      = "WT",
+        [MTRR_TYPE_WRPROT]         = "WP",
+        [MTRR_TYPE_WRBACK]         = "WB",
+        [MTRR_NUM_TYPES]           = "??"
+    };
+
+    return x < ARRAY_SIZE(memory_types) ? (memory_types[x] ?: "?") : "?";
+}
+
 static void ept_dump_p2m_table(unsigned char key)
 {
     struct domain *d;
@@ -1212,15 +1226,6 @@ static void ept_dump_p2m_table(unsigned char key)
     unsigned long record_counter = 0;
     struct p2m_domain *p2m;
     struct ept_data *ept;
-    static const char memory_types[8][2] = {
-        [0 ... 7] = "?",
-        [MTRR_TYPE_UNCACHABLE]     = "UC",
-        [MTRR_TYPE_WRCOMB]         = "WC",
-        [MTRR_TYPE_WRTHROUGH]      = "WT",
-        [MTRR_TYPE_WRPROT]         = "WP",
-        [MTRR_TYPE_WRBACK]         = "WB",
-        [MTRR_NUM_TYPES]           = "??"
-    };
 
     for_each_domain(d)
     {
@@ -1260,8 +1265,8 @@ static void ept_dump_p2m_table(unsigned char key)
                            ept_entry->r ? 'r' : ' ',
                            ept_entry->w ? 'w' : ' ',
                            ept_entry->x ? 'x' : ' ',
-                           memory_types[ept_entry->emt][0],
-                           memory_types[ept_entry->emt][1]
+                           memory_type_to_str(ept_entry->emt)[0],
+                           memory_type_to_str(ept_entry->emt)[1]
                            ?: ept_entry->emt + '0',
                            c ?: ept_entry->ipat ? '!' : ' ');
 
-- 
2.1.4

[PATCH 7/8] xen/x86: Fix get_cpu_info() when built with clang

[Andrew Cooper]

Clang understands the GCCism in use here, but still complains that sp is
unintialised.  In such cases, resort to the older version of this code, which
directly reads %rsp into the temporary variable.

Note that we still keep the GCCism in the default case, as it causes GCC to
create rather better assembly.

Signed-off-by: Andrew Cooper <andrew.cooper3@citrix.com>
---
CC: Jan Beulich <JBeulich@suse.com>
---
 xen/include/asm-x86/current.h | 6 ++++++
 1 file changed, 6 insertions(+)

diff --git a/xen/include/asm-x86/current.h b/xen/include/asm-x86/current.h
index f011d2d..15b99ff 100644
--- a/xen/include/asm-x86/current.h
+++ b/xen/include/asm-x86/current.h
@@ -47,7 +47,13 @@ struct cpu_info {
 
 static inline struct cpu_info *get_cpu_info(void)
 {
+#ifdef __clang__
+    /* Clang complains that sp in the else case is not initialised. */
+    unsigned long sp;
+    asm ( "mov %%rsp, %0" : "=r" (sp) );
+#else
     register unsigned long sp asm("rsp");
+#endif
 
     return (struct cpu_info *)((sp & ~(STACK_SIZE-1)) + STACK_SIZE) - 1;
 }
-- 
2.1.4

[PATCH 8/8] x86/efi: Generate uefi_call_wrapper() when compiling with clang

[Andrew Cooper]

Signed-off-by: Andrew Cooper <andrew.cooper3@citrix.com>
---
CC: Jan Beulich <JBeulich@suse.com>

What is the GCC version check supposed to be achieving here?  GCC has
supported this syntax since 3.0
---
 xen/include/asm-x86/x86_64/efibind.h | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/xen/include/asm-x86/x86_64/efibind.h b/xen/include/asm-x86/x86_64/efibind.h
index 2db3568..af5f424 100644
--- a/xen/include/asm-x86/x86_64/efibind.h
+++ b/xen/include/asm-x86/x86_64/efibind.h
@@ -274,7 +274,7 @@ typedef uint64_t   UINTN;
 #endif
 #endif
 
-#if __GNUC__ > 4 || (__GNUC__ == 4 && __GNUC_MINOR__ >= 4)
+#if __GNUC__ > 4 || (__GNUC__ == 4 && __GNUC_MINOR__ >= 4) || __clang__
 #define uefi_call_wrapper(func, va_num, ...)	func(__VA_ARGS__)
 #else
 /* for x86_64, EFI_FUNCTION_WRAPPER must be defined */
-- 
2.1.4

Re: [PATCH 0/8] xen/x86: Fix build with Clang 3.5

[Doug Goldstein]

[-- Attachment #1.1: Type: text/plain, Size: 269 bytes --]

On 2/9/16 2:01 PM, Andrew Cooper wrote:
> This is a misc assortment of patches to fix the build with Clang 3.5.  The
> result also appears to function.
> 

Manually ran the build here:

https://travis-ci.org/cardoe/xen/builds/108116078

-- 
Doug Goldstein


[-- Attachment #1.2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 959 bytes --]

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

_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
http://lists.xen.org/xen-devel

Re: [PATCH 0/8] xen/x86: Fix build with Clang 3.5

[Ian Campbell]

On Tue, 2016-02-09 at 20:01 +0000, Andrew Cooper wrote:
> This is a misc assortment of patches to fix the build with Clang 3.5.

FWIW clang 3.5 appears to be in Debian Jessie, which means the path to
having a build job in osstest is far shorter than it once was if anyone is
interested in adding such a thing.

Ian.

Re: [PATCH 4/8] xen/x86: Fix section type mismatch in mm.c

[George Dunlap]

On 09/02/16 20:01, Andrew Cooper wrote:
> Clang doesn't like mixing const and non-const data in the same section.  Move
> zero_page into .bss.page_aligned.const and wildcard .bss.page_aligned when
> linking.
> 
> Signed-off-by: Andrew Cooper <andrew.cooper3@citrix.com>

Acked-by: George Dunlap <george.dunlap@citrix.com>

> ---
> CC: Jan Beulich <JBeulich@suse.com>
> CC: George Dunlap <george.dunlap@eu.citrix.com>
> ---
>  xen/arch/x86/mm.c      | 2 +-
>  xen/arch/x86/xen.lds.S | 2 +-
>  2 files changed, 2 insertions(+), 2 deletions(-)
> 
> diff --git a/xen/arch/x86/mm.c b/xen/arch/x86/mm.c
> index d62d9ec..c214fc5 100644
> --- a/xen/arch/x86/mm.c
> +++ b/xen/arch/x86/mm.c
> @@ -577,7 +577,7 @@ static inline void guest_get_eff_kern_l1e(struct vcpu *v, unsigned long addr,
>      TOGGLE_MODE();
>  }
>  
> -static const char __section(".bss.page_aligned") zero_page[PAGE_SIZE];
> +static const char __section(".bss.page_aligned.const") zero_page[PAGE_SIZE];
>  
>  static void invalidate_shadow_ldt(struct vcpu *v, int flush)
>  {
> diff --git a/xen/arch/x86/xen.lds.S b/xen/arch/x86/xen.lds.S
> index c1ce027..3b199ca 100644
> --- a/xen/arch/x86/xen.lds.S
> +++ b/xen/arch/x86/xen.lds.S
> @@ -170,7 +170,7 @@ SECTIONS
>         __bss_start = .;
>         *(.bss.stack_aligned)
>         . = ALIGN(PAGE_SIZE);
> -       *(.bss.page_aligned)
> +       *(.bss.page_aligned*)
>         *(.bss)
>         . = ALIGN(SMP_CACHE_BYTES);
>         __per_cpu_start = .;
> 

Re: [PATCH 6/8] xen/x86: Avoid overriding initialisers in arrays

[George Dunlap]

On 09/02/16 20:01, Andrew Cooper wrote:
> Clang objects to having multiple initialisers when creating an array.
> 
> As this warning is useful for spotting obscure bugs, disabling it is
> unhelpful.  Instead, fix our two deliberate usecases.
> 
> In the p2m-ept case, pull the array out into a helper function, so the helper
> can guarentee to cover the NULL pointer case.
> 
> No functional change.
> 
> Signed-off-by: Andrew Cooper <andrew.cooper3@citrix.com>

Acked-by: George Dunlap <george.dunlap@citrix.com>

> ---
> CC: Jan Beulich <JBeulich@suse.com>
> CC: George Dunlap <george.dunlap@eu.citrix.com>
> CC: Jun Nakajima <jun.nakajima@intel.com>
> CC: Kevin Tian <kevin.tian@intel.com>
> ---
>  xen/arch/x86/cpu/mtrr/generic.c |  3 +--
>  xen/arch/x86/mm/p2m-ept.c       | 27 ++++++++++++++++-----------
>  2 files changed, 17 insertions(+), 13 deletions(-)
> 
> diff --git a/xen/arch/x86/cpu/mtrr/generic.c b/xen/arch/x86/cpu/mtrr/generic.c
> index ea0efe2..8839f8d 100644
> --- a/xen/arch/x86/cpu/mtrr/generic.c
> +++ b/xen/arch/x86/cpu/mtrr/generic.c
> @@ -91,7 +91,6 @@ static const char *__init mtrr_attrib_to_str(mtrr_type x)
>  {
>  	static const char __initconst strings[MTRR_NUM_TYPES][16] =
>  	{
> -		[0 ... MTRR_NUM_TYPES - 1] = "?",
>  		[MTRR_TYPE_UNCACHABLE]     = "uncachable",
>  		[MTRR_TYPE_WRCOMB]         = "write-combining",
>  		[MTRR_TYPE_WRTHROUGH]      = "write-through",
> @@ -99,7 +98,7 @@ static const char *__init mtrr_attrib_to_str(mtrr_type x)
>  		[MTRR_TYPE_WRBACK]         = "write-back",
>  	};
>  
> -	return x < MTRR_NUM_TYPES ? strings[x] : "?";
> +	return x < MTRR_NUM_TYPES ? (strings[x] ?: "?") : "?";
>  }
>  
>  static unsigned int __initdata last_fixed_start;
> diff --git a/xen/arch/x86/mm/p2m-ept.c b/xen/arch/x86/mm/p2m-ept.c
> index c094320..c5fe906 100644
> --- a/xen/arch/x86/mm/p2m-ept.c
> +++ b/xen/arch/x86/mm/p2m-ept.c
> @@ -1201,6 +1201,20 @@ void ept_p2m_uninit(struct p2m_domain *p2m)
>      free_cpumask_var(ept->invalidate);
>  }
>  
> +static const char *memory_type_to_str(unsigned int x)
> +{
> +    static const char memory_types[8][2] = {
> +        [MTRR_TYPE_UNCACHABLE]     = "UC",
> +        [MTRR_TYPE_WRCOMB]         = "WC",
> +        [MTRR_TYPE_WRTHROUGH]      = "WT",
> +        [MTRR_TYPE_WRPROT]         = "WP",
> +        [MTRR_TYPE_WRBACK]         = "WB",
> +        [MTRR_NUM_TYPES]           = "??"
> +    };
> +
> +    return x < ARRAY_SIZE(memory_types) ? (memory_types[x] ?: "?") : "?";
> +}
> +
>  static void ept_dump_p2m_table(unsigned char key)
>  {
>      struct domain *d;
> @@ -1212,15 +1226,6 @@ static void ept_dump_p2m_table(unsigned char key)
>      unsigned long record_counter = 0;
>      struct p2m_domain *p2m;
>      struct ept_data *ept;
> -    static const char memory_types[8][2] = {
> -        [0 ... 7] = "?",
> -        [MTRR_TYPE_UNCACHABLE]     = "UC",
> -        [MTRR_TYPE_WRCOMB]         = "WC",
> -        [MTRR_TYPE_WRTHROUGH]      = "WT",
> -        [MTRR_TYPE_WRPROT]         = "WP",
> -        [MTRR_TYPE_WRBACK]         = "WB",
> -        [MTRR_NUM_TYPES]           = "??"
> -    };
>  
>      for_each_domain(d)
>      {
> @@ -1260,8 +1265,8 @@ static void ept_dump_p2m_table(unsigned char key)
>                             ept_entry->r ? 'r' : ' ',
>                             ept_entry->w ? 'w' : ' ',
>                             ept_entry->x ? 'x' : ' ',
> -                           memory_types[ept_entry->emt][0],
> -                           memory_types[ept_entry->emt][1]
> +                           memory_type_to_str(ept_entry->emt)[0],
> +                           memory_type_to_str(ept_entry->emt)[1]
>                             ?: ept_entry->emt + '0',
>                             c ?: ept_entry->ipat ? '!' : ' ');
>  
> 

Re: [PATCH 2/8] xen/misc: Remove or annotate possibly-unused functions

[Tim Deegan]

At 20:01 +0000 on 09 Feb (1455048102), Andrew Cooper wrote:
> Clang notices more unused functions than GCC.
> 
>  * sh_next_page() is only used at GUEST_PAGING_LEVELS=2, so remove it from the
>    other guest level translation units
>  * rcu_batch_after() is completely unused.
>  * Various of the COMPAT() generated functions are used only for their
>    BUILD_BUG_ON() properties.  Annotate them all as __maybe_used.
> 
> Signed-off-by: Andrew Cooper <andrew.cooper3@citrix.com>

Shadow stuff Acked-by: Tim Deegan <tim@xen.org>

Re: [PATCH 2/8] xen/misc: Remove or annotate possibly-unused functions

[Jan Beulich]

>>> On 09.02.16 at 21:01, <andrew.cooper3@citrix.com> wrote:
> --- a/xen/common/rcupdate.c
> +++ b/xen/common/rcupdate.c
> @@ -141,12 +141,6 @@ static inline int rcu_batch_before(long a, long b)
>      return (a - b) < 0;
>  }
>  
> -/* Is batch a after batch b ? */
> -static inline int rcu_batch_after(long a, long b)
> -{
> -    return (a - b) > 0;
> -}

To me it is the nature of inline functions that they may or may not be
used, regardless of whether they live in a header file (where I would
have supposed Clang won't warn about, but the change below makes
me assume I'm wrong) or in a source file.

> --- a/xen/include/xen/compat.h
> +++ b/xen/include/xen/compat.h
> @@ -134,14 +134,16 @@
>  #define CHECK_NAME_(k, n, tag) __check ## tag ## k ## _ ## n
>  
>  #define CHECK_TYPE(name) \
> -static inline int CHECK_NAME(name, T)(xen_ ## name ## _t *x, \
> -                                      compat_ ## name ## _t *c) \
> +static inline int __maybe_unused \
> +CHECK_NAME(name, T)(xen_ ## name ## _t *x, \
> +                    compat_ ## name ## _t *c) \
>  { \
>      return x == c; \
>  }
>  #define CHECK_TYPE_(k, n) \
> -static inline int CHECK_NAME_(k, n, T)(k xen_ ## n *x, \
> -                                       k compat_ ## n *c) \
> +static inline int __maybe_unused \
> +CHECK_NAME_(k, n, T)(k xen_ ## n *x, \
> +                     k compat_ ## n *c) \
>  { \
>      return x == c; \
>  }
> @@ -154,14 +156,14 @@ static inline int CHECK_NAME_(k, n, T)(k xen_ ## n *x, \
>                                            sizeof(k compat_ ## n)) * 2]
>  
>  #define CHECK_FIELD_COMMON(name, t, f) \
> -static inline int name(xen_ ## t ## _t *x, compat_ ## t ## _t *c) \
> +static inline int __maybe_unused name(xen_ ## t ## _t *x, compat_ ## t ## _t *c) \
>  { \
>      BUILD_BUG_ON(offsetof(xen_ ## t ## _t, f) != \
>                   offsetof(compat_ ## t ## _t, f)); \
>      return &x->f == &c->f; \
>  }
>  #define CHECK_FIELD_COMMON_(k, name, n, f) \
> -static inline int name(k xen_ ## n *x, k compat_ ## n *c) \
> +static inline int __maybe_unused name(k xen_ ## n *x, k compat_ ## n *c) \
>  { \
>      BUILD_BUG_ON(offsetof(k xen_ ## n, f) != \
>                   offsetof(k compat_ ## n, f)); \

So if these are all noticed to be unused, why would others in other
header files not be? I think there's at the very least some aspect
missing in the description, explaining what makes these stand out
from the others.

Jan

Re: [PATCH 3/8] xen/x86: Remove %z modifier from inline assembly

[Jan Beulich]

>>> On 09.02.16 at 21:01, <andrew.cooper3@citrix.com> wrote:
> Clang doesn't support the %z modifier.  Replace both uses with an explicit l
> suffix, and cover the changes with BUILD_BUG_ON()s

Ugly. Really ugly. But well - what do you do.

> Signed-off-by: Andrew Cooper <andrew.cooper3@citrix.com>

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

Re: [PATCH 2/8] xen/misc: Remove or annotate possibly-unused functions

[Andrew Cooper]

On 10/02/16 13:06, Jan Beulich wrote:
>>>> On 09.02.16 at 21:01, <andrew.cooper3@citrix.com> wrote:
>> --- a/xen/common/rcupdate.c
>> +++ b/xen/common/rcupdate.c
>> @@ -141,12 +141,6 @@ static inline int rcu_batch_before(long a, long b)
>>      return (a - b) < 0;
>>  }
>>  
>> -/* Is batch a after batch b ? */
>> -static inline int rcu_batch_after(long a, long b)
>> -{
>> -    return (a - b) > 0;
>> -}
> To me it is the nature of inline functions that they may or may not be
> used, regardless of whether they live in a header file (where I would
> have supposed Clang won't warn about, but the change below makes
> me assume I'm wrong) or in a source file.
>
>> --- a/xen/include/xen/compat.h
>> +++ b/xen/include/xen/compat.h
>> @@ -134,14 +134,16 @@
>>  #define CHECK_NAME_(k, n, tag) __check ## tag ## k ## _ ## n
>>  
>>  #define CHECK_TYPE(name) \
>> -static inline int CHECK_NAME(name, T)(xen_ ## name ## _t *x, \
>> -                                      compat_ ## name ## _t *c) \
>> +static inline int __maybe_unused \
>> +CHECK_NAME(name, T)(xen_ ## name ## _t *x, \
>> +                    compat_ ## name ## _t *c) \
>>  { \
>>      return x == c; \
>>  }
>>  #define CHECK_TYPE_(k, n) \
>> -static inline int CHECK_NAME_(k, n, T)(k xen_ ## n *x, \
>> -                                       k compat_ ## n *c) \
>> +static inline int __maybe_unused \
>> +CHECK_NAME_(k, n, T)(k xen_ ## n *x, \
>> +                     k compat_ ## n *c) \
>>  { \
>>      return x == c; \
>>  }
>> @@ -154,14 +156,14 @@ static inline int CHECK_NAME_(k, n, T)(k xen_ ## n *x, \
>>                                            sizeof(k compat_ ## n)) * 2]
>>  
>>  #define CHECK_FIELD_COMMON(name, t, f) \
>> -static inline int name(xen_ ## t ## _t *x, compat_ ## t ## _t *c) \
>> +static inline int __maybe_unused name(xen_ ## t ## _t *x, compat_ ## t ## _t *c) \
>>  { \
>>      BUILD_BUG_ON(offsetof(xen_ ## t ## _t, f) != \
>>                   offsetof(compat_ ## t ## _t, f)); \
>>      return &x->f == &c->f; \
>>  }
>>  #define CHECK_FIELD_COMMON_(k, name, n, f) \
>> -static inline int name(k xen_ ## n *x, k compat_ ## n *c) \
>> +static inline int __maybe_unused name(k xen_ ## n *x, k compat_ ## n *c) \
>>  { \
>>      BUILD_BUG_ON(offsetof(k xen_ ## n, f) != \
>>                   offsetof(k compat_ ## n, f)); \
> So if these are all noticed to be unused, why would others in other
> header files not be?

Because they are instantiated in translation units, with no callers, by
code like common/trace.c:

#ifdef CONFIG_COMPAT
#include <compat/trace.h>
#define xen_t_buf t_buf
CHECK_t_buf;
#undef xen_t_buf
#else
#define compat_t_rec t_rec
#endif

This was the first example which blew up.

~Andrew

Re: [PATCH 6/8] xen/x86: Avoid overriding initialisers in arrays

[Jan Beulich]

>>> On 09.02.16 at 21:01, <andrew.cooper3@citrix.com> wrote:
> Clang objects to having multiple initialisers when creating an array.
> 
> As this warning is useful for spotting obscure bugs, disabling it is
> unhelpful.  Instead, fix our two deliberate usecases.

Ugly again, but - well ...

> --- a/xen/arch/x86/mm/p2m-ept.c
> +++ b/xen/arch/x86/mm/p2m-ept.c
> @@ -1201,6 +1201,20 @@ void ept_p2m_uninit(struct p2m_domain *p2m)
>      free_cpumask_var(ept->invalidate);
>  }
>  
> +static const char *memory_type_to_str(unsigned int x)
> +{
> +    static const char memory_types[8][2] = {
> +        [MTRR_TYPE_UNCACHABLE]     = "UC",
> +        [MTRR_TYPE_WRCOMB]         = "WC",
> +        [MTRR_TYPE_WRTHROUGH]      = "WT",
> +        [MTRR_TYPE_WRPROT]         = "WP",
> +        [MTRR_TYPE_WRBACK]         = "WB",
> +        [MTRR_NUM_TYPES]           = "??"
> +    };
> +
> +    return x < ARRAY_SIZE(memory_types) ? (memory_types[x] ?: "?") : "?";

I think this should really ASSERT() the first condition.

> @@ -1212,15 +1226,6 @@ static void ept_dump_p2m_table(unsigned char key)
>      unsigned long record_counter = 0;
>      struct p2m_domain *p2m;
>      struct ept_data *ept;
> -    static const char memory_types[8][2] = {
> -        [0 ... 7] = "?",
> -        [MTRR_TYPE_UNCACHABLE]     = "UC",
> -        [MTRR_TYPE_WRCOMB]         = "WC",
> -        [MTRR_TYPE_WRTHROUGH]      = "WT",
> -        [MTRR_TYPE_WRPROT]         = "WP",
> -        [MTRR_TYPE_WRBACK]         = "WB",
> -        [MTRR_NUM_TYPES]           = "??"
> -    };
>  
>      for_each_domain(d)
>      {
> @@ -1260,8 +1265,8 @@ static void ept_dump_p2m_table(unsigned char key)
>                             ept_entry->r ? 'r' : ' ',
>                             ept_entry->w ? 'w' : ' ',
>                             ept_entry->x ? 'x' : ' ',
> -                           memory_types[ept_entry->emt][0],
> -                           memory_types[ept_entry->emt][1]
> +                           memory_type_to_str(ept_entry->emt)[0],
> +                           memory_type_to_str(ept_entry->emt)[1]
>                             ?: ept_entry->emt + '0',
>                             c ?: ept_entry->ipat ? '!' : ' ');

There's actually a bug here, which I think is worth fixing at once:
The default initializer was a string of length 1, resulting in a
premature NUL character to get placed into the fully expanded
string, causing - afaict - truncation of the intended message. I
therefore think the default string should be e.g. "? ".

Jan

Re: [PATCH 8/8] x86/efi: Generate uefi_call_wrapper() when compiling with clang

[Jan Beulich]

>>> On 09.02.16 at 21:01, <andrew.cooper3@citrix.com> wrote:
> Signed-off-by: Andrew Cooper <andrew.cooper3@citrix.com>
> ---
> CC: Jan Beulich <JBeulich@suse.com>
> 
> What is the GCC version check supposed to be achieving here?  GCC has
> supported this syntax since 3.0

This is best answered by looking at where we've got these headers
from - the gnu-efi package. It has ...

> --- a/xen/include/asm-x86/x86_64/efibind.h
> +++ b/xen/include/asm-x86/x86_64/efibind.h
> @@ -274,7 +274,7 @@ typedef uint64_t   UINTN;
>  #endif
>  #endif
>  
> -#if __GNUC__ > 4 || (__GNUC__ == 4 && __GNUC_MINOR__ >= 4)
> +#if __GNUC__ > 4 || (__GNUC__ == 4 && __GNUC_MINOR__ >= 4) || __clang__
>  #define uefi_call_wrapper(func, va_num, ...)	func(__VA_ARGS__)
>  #else
>  /* for x86_64, EFI_FUNCTION_WRAPPER must be defined */

/* for x86_64, EFI_FUNCTION_WRAPPER must be defined */
#if defined(HAVE_USE_MS_ABI)
#define uefi_call_wrapper(func, va_num, ...) func(__VA_ARGS__)
#else
UINTN uefi_call_wrapper(void *func, unsigned long va_num, ...);
#endif
#define EFI_FUNCTION __attribute__((ms_abi))

I think this makes clear that the needed feature here is the
attribute, which certainly wasn't available in older gcc.

With that the question is whether the Clang case won't also need
a version check, since I can't imagine them having supported this
prior to gcc introducing it.

Jan

Re: [PATCH 8/8] x86/efi: Generate uefi_call_wrapper() when compiling with clang

[Andrew Cooper]

On 10/02/16 13:31, Jan Beulich wrote:
>>>> On 09.02.16 at 21:01, <andrew.cooper3@citrix.com> wrote:
>> Signed-off-by: Andrew Cooper <andrew.cooper3@citrix.com>
>> ---
>> CC: Jan Beulich <JBeulich@suse.com>
>>
>> What is the GCC version check supposed to be achieving here?  GCC has
>> supported this syntax since 3.0
> This is best answered by looking at where we've got these headers
> from - the gnu-efi package. It has ...
>
>> --- a/xen/include/asm-x86/x86_64/efibind.h
>> +++ b/xen/include/asm-x86/x86_64/efibind.h
>> @@ -274,7 +274,7 @@ typedef uint64_t   UINTN;
>>  #endif
>>  #endif
>>  
>> -#if __GNUC__ > 4 || (__GNUC__ == 4 && __GNUC_MINOR__ >= 4)
>> +#if __GNUC__ > 4 || (__GNUC__ == 4 && __GNUC_MINOR__ >= 4) || __clang__
>>  #define uefi_call_wrapper(func, va_num, ...)	func(__VA_ARGS__)
>>  #else
>>  /* for x86_64, EFI_FUNCTION_WRAPPER must be defined */
> /* for x86_64, EFI_FUNCTION_WRAPPER must be defined */
> #if defined(HAVE_USE_MS_ABI)
> #define uefi_call_wrapper(func, va_num, ...) func(__VA_ARGS__)
> #else
> UINTN uefi_call_wrapper(void *func, unsigned long va_num, ...);
> #endif
> #define EFI_FUNCTION __attribute__((ms_abi))
>
> I think this makes clear that the needed feature here is the
> attribute, which certainly wasn't available in older gcc.
>
> With that the question is whether the Clang case won't also need
> a version check, since I can't imagine them having supported this
> prior to gcc introducing it.

Clang has an substantially more sane way than GCC of checking for
individual features.  I will introduce and use the
__has_{attribute,feature}() infrastructure to tests like this.

Experimentally, Clang 3.5 does have ms_abi support

~Andrew

Re: [PATCH 6/8] xen/x86: Avoid overriding initialisers in arrays

[Andrew Cooper]

On 10/02/16 13:22, Jan Beulich wrote:
>>>> On 09.02.16 at 21:01, <andrew.cooper3@citrix.com> wrote:
>> Clang objects to having multiple initialisers when creating an array.
>>
>> As this warning is useful for spotting obscure bugs, disabling it is
>> unhelpful.  Instead, fix our two deliberate usecases.
> Ugly again, but - well ...
>
>> --- a/xen/arch/x86/mm/p2m-ept.c
>> +++ b/xen/arch/x86/mm/p2m-ept.c
>> @@ -1201,6 +1201,20 @@ void ept_p2m_uninit(struct p2m_domain *p2m)
>>      free_cpumask_var(ept->invalidate);
>>  }
>>  
>> +static const char *memory_type_to_str(unsigned int x)
>> +{
>> +    static const char memory_types[8][2] = {
>> +        [MTRR_TYPE_UNCACHABLE]     = "UC",
>> +        [MTRR_TYPE_WRCOMB]         = "WC",
>> +        [MTRR_TYPE_WRTHROUGH]      = "WT",
>> +        [MTRR_TYPE_WRPROT]         = "WP",
>> +        [MTRR_TYPE_WRBACK]         = "WB",
>> +        [MTRR_NUM_TYPES]           = "??"
>> +    };
>> +
>> +    return x < ARRAY_SIZE(memory_types) ? (memory_types[x] ?: "?") : "?";
> I think this should really ASSERT() the first condition.
>
>> @@ -1212,15 +1226,6 @@ static void ept_dump_p2m_table(unsigned char key)
>>      unsigned long record_counter = 0;
>>      struct p2m_domain *p2m;
>>      struct ept_data *ept;
>> -    static const char memory_types[8][2] = {
>> -        [0 ... 7] = "?",
>> -        [MTRR_TYPE_UNCACHABLE]     = "UC",
>> -        [MTRR_TYPE_WRCOMB]         = "WC",
>> -        [MTRR_TYPE_WRTHROUGH]      = "WT",
>> -        [MTRR_TYPE_WRPROT]         = "WP",
>> -        [MTRR_TYPE_WRBACK]         = "WB",
>> -        [MTRR_NUM_TYPES]           = "??"
>> -    };
>>  
>>      for_each_domain(d)
>>      {
>> @@ -1260,8 +1265,8 @@ static void ept_dump_p2m_table(unsigned char key)
>>                             ept_entry->r ? 'r' : ' ',
>>                             ept_entry->w ? 'w' : ' ',
>>                             ept_entry->x ? 'x' : ' ',
>> -                           memory_types[ept_entry->emt][0],
>> -                           memory_types[ept_entry->emt][1]
>> +                           memory_type_to_str(ept_entry->emt)[0],
>> +                           memory_type_to_str(ept_entry->emt)[1]
>>                             ?: ept_entry->emt + '0',
>>                             c ?: ept_entry->ipat ? '!' : ' ');
> There's actually a bug here, which I think is worth fixing at once:
> The default initializer was a string of length 1, resulting in a
> premature NUL character to get placed into the fully expanded
> string, causing - afaict - truncation of the intended message. I
> therefore think the default string should be e.g. "? ".

The code is very opaque.  However, that appears to be precisely how it
is intended to work.  (Having said that - it is your code from c/s
90e9c95f).

The following line will only format the raw emt value as a number if
there is a NUL character returned from memory_type_to_str().  Putting a
space in instead would break this.

~Andrew

Re: [PATCH 6/8] xen/x86: Avoid overriding initialisers in arrays

[Jan Beulich]

>>> On 10.02.16 at 14:50, <andrew.cooper3@citrix.com> wrote:
> On 10/02/16 13:22, Jan Beulich wrote:
>>>>> On 09.02.16 at 21:01, <andrew.cooper3@citrix.com> wrote:
>>> Clang objects to having multiple initialisers when creating an array.
>>>
>>> As this warning is useful for spotting obscure bugs, disabling it is
>>> unhelpful.  Instead, fix our two deliberate usecases.
>> Ugly again, but - well ...
>>
>>> --- a/xen/arch/x86/mm/p2m-ept.c
>>> +++ b/xen/arch/x86/mm/p2m-ept.c
>>> @@ -1201,6 +1201,20 @@ void ept_p2m_uninit(struct p2m_domain *p2m)
>>>      free_cpumask_var(ept->invalidate);
>>>  }
>>>  
>>> +static const char *memory_type_to_str(unsigned int x)
>>> +{
>>> +    static const char memory_types[8][2] = {
>>> +        [MTRR_TYPE_UNCACHABLE]     = "UC",
>>> +        [MTRR_TYPE_WRCOMB]         = "WC",
>>> +        [MTRR_TYPE_WRTHROUGH]      = "WT",
>>> +        [MTRR_TYPE_WRPROT]         = "WP",
>>> +        [MTRR_TYPE_WRBACK]         = "WB",
>>> +        [MTRR_NUM_TYPES]           = "??"
>>> +    };
>>> +
>>> +    return x < ARRAY_SIZE(memory_types) ? (memory_types[x] ?: "?") : "?";
>> I think this should really ASSERT() the first condition.
>>
>>> @@ -1212,15 +1226,6 @@ static void ept_dump_p2m_table(unsigned char key)
>>>      unsigned long record_counter = 0;
>>>      struct p2m_domain *p2m;
>>>      struct ept_data *ept;
>>> -    static const char memory_types[8][2] = {
>>> -        [0 ... 7] = "?",
>>> -        [MTRR_TYPE_UNCACHABLE]     = "UC",
>>> -        [MTRR_TYPE_WRCOMB]         = "WC",
>>> -        [MTRR_TYPE_WRTHROUGH]      = "WT",
>>> -        [MTRR_TYPE_WRPROT]         = "WP",
>>> -        [MTRR_TYPE_WRBACK]         = "WB",
>>> -        [MTRR_NUM_TYPES]           = "??"
>>> -    };
>>>  
>>>      for_each_domain(d)
>>>      {
>>> @@ -1260,8 +1265,8 @@ static void ept_dump_p2m_table(unsigned char key)
>>>                             ept_entry->r ? 'r' : ' ',
>>>                             ept_entry->w ? 'w' : ' ',
>>>                             ept_entry->x ? 'x' : ' ',
>>> -                           memory_types[ept_entry->emt][0],
>>> -                           memory_types[ept_entry->emt][1]
>>> +                           memory_type_to_str(ept_entry->emt)[0],
>>> +                           memory_type_to_str(ept_entry->emt)[1]
>>>                             ?: ept_entry->emt + '0',
>>>                             c ?: ept_entry->ipat ? '!' : ' ');
>> There's actually a bug here, which I think is worth fixing at once:
>> The default initializer was a string of length 1, resulting in a
>> premature NUL character to get placed into the fully expanded
>> string, causing - afaict - truncation of the intended message. I
>> therefore think the default string should be e.g. "? ".
> 
> The code is very opaque.  However, that appears to be precisely how it
> is intended to work.  (Having said that - it is your code from c/s
> 90e9c95f).

I know.

> The following line will only format the raw emt value as a number if
> there is a NUL character returned from memory_type_to_str().  Putting a
> space in instead would break this.

Oh, right - this is the operand to a ?:, not by itself passed to
printk(). Line breaks like this (to aid people with old editors) are
really undesirable in places like this...

Sorry for the noise,

Jan

Re: [PATCH 6/8] xen/x86: Avoid overriding initialisers in arrays

[George Dunlap]

On 10/02/16 14:03, Jan Beulich wrote:
>>>> On 10.02.16 at 14:50, <andrew.cooper3@citrix.com> wrote:
>> On 10/02/16 13:22, Jan Beulich wrote:
>>>>>> On 09.02.16 at 21:01, <andrew.cooper3@citrix.com> wrote:
>>>> Clang objects to having multiple initialisers when creating an array.
>>>>
>>>> As this warning is useful for spotting obscure bugs, disabling it is
>>>> unhelpful.  Instead, fix our two deliberate usecases.
>>> Ugly again, but - well ...
>>>
>>>> --- a/xen/arch/x86/mm/p2m-ept.c
>>>> +++ b/xen/arch/x86/mm/p2m-ept.c
>>>> @@ -1201,6 +1201,20 @@ void ept_p2m_uninit(struct p2m_domain *p2m)
>>>>      free_cpumask_var(ept->invalidate);
>>>>  }
>>>>  
>>>> +static const char *memory_type_to_str(unsigned int x)
>>>> +{
>>>> +    static const char memory_types[8][2] = {
>>>> +        [MTRR_TYPE_UNCACHABLE]     = "UC",
>>>> +        [MTRR_TYPE_WRCOMB]         = "WC",
>>>> +        [MTRR_TYPE_WRTHROUGH]      = "WT",
>>>> +        [MTRR_TYPE_WRPROT]         = "WP",
>>>> +        [MTRR_TYPE_WRBACK]         = "WB",
>>>> +        [MTRR_NUM_TYPES]           = "??"
>>>> +    };
>>>> +
>>>> +    return x < ARRAY_SIZE(memory_types) ? (memory_types[x] ?: "?") : "?";
>>> I think this should really ASSERT() the first condition.
>>>
>>>> @@ -1212,15 +1226,6 @@ static void ept_dump_p2m_table(unsigned char key)
>>>>      unsigned long record_counter = 0;
>>>>      struct p2m_domain *p2m;
>>>>      struct ept_data *ept;
>>>> -    static const char memory_types[8][2] = {
>>>> -        [0 ... 7] = "?",
>>>> -        [MTRR_TYPE_UNCACHABLE]     = "UC",
>>>> -        [MTRR_TYPE_WRCOMB]         = "WC",
>>>> -        [MTRR_TYPE_WRTHROUGH]      = "WT",
>>>> -        [MTRR_TYPE_WRPROT]         = "WP",
>>>> -        [MTRR_TYPE_WRBACK]         = "WB",
>>>> -        [MTRR_NUM_TYPES]           = "??"
>>>> -    };
>>>>  
>>>>      for_each_domain(d)
>>>>      {
>>>> @@ -1260,8 +1265,8 @@ static void ept_dump_p2m_table(unsigned char key)
>>>>                             ept_entry->r ? 'r' : ' ',
>>>>                             ept_entry->w ? 'w' : ' ',
>>>>                             ept_entry->x ? 'x' : ' ',
>>>> -                           memory_types[ept_entry->emt][0],
>>>> -                           memory_types[ept_entry->emt][1]
>>>> +                           memory_type_to_str(ept_entry->emt)[0],
>>>> +                           memory_type_to_str(ept_entry->emt)[1]
>>>>                             ?: ept_entry->emt + '0',
>>>>                             c ?: ept_entry->ipat ? '!' : ' ');
>>> There's actually a bug here, which I think is worth fixing at once:
>>> The default initializer was a string of length 1, resulting in a
>>> premature NUL character to get placed into the fully expanded
>>> string, causing - afaict - truncation of the intended message. I
>>> therefore think the default string should be e.g. "? ".
>>
>> The code is very opaque.  However, that appears to be precisely how it
>> is intended to work.  (Having said that - it is your code from c/s
>> 90e9c95f).
> 
> I know.
> 
>> The following line will only format the raw emt value as a number if
>> there is a NUL character returned from memory_type_to_str().  Putting a
>> space in instead would break this.
> 
> Oh, right - this is the operand to a ?:, not by itself passed to
> printk(). Line breaks like this (to aid people with old editors) are
> really undesirable in places like this...

Even more so over-clever undocumented code.  If you're going to do
things like this, you need to leave a comment near the string definition
saying that the second byte being NULL is a flag for the printing
routine to print the number, so that people who come along later (maybe
even yourself, as in this case) know there's a dependency there.

 -George

Re: [PATCH 8/8] x86/efi: Generate uefi_call_wrapper() when compiling with clang

[Andrew Cooper]

On 10/02/16 13:41, Andrew Cooper wrote:
> On 10/02/16 13:31, Jan Beulich wrote:
>>>>> On 09.02.16 at 21:01, <andrew.cooper3@citrix.com> wrote:
>>> Signed-off-by: Andrew Cooper <andrew.cooper3@citrix.com>
>>> ---
>>> CC: Jan Beulich <JBeulich@suse.com>
>>>
>>> What is the GCC version check supposed to be achieving here?  GCC has
>>> supported this syntax since 3.0
>> This is best answered by looking at where we've got these headers
>> from - the gnu-efi package. It has ...
>>
>>> --- a/xen/include/asm-x86/x86_64/efibind.h
>>> +++ b/xen/include/asm-x86/x86_64/efibind.h
>>> @@ -274,7 +274,7 @@ typedef uint64_t   UINTN;
>>>  #endif
>>>  #endif
>>>  
>>> -#if __GNUC__ > 4 || (__GNUC__ == 4 && __GNUC_MINOR__ >= 4)
>>> +#if __GNUC__ > 4 || (__GNUC__ == 4 && __GNUC_MINOR__ >= 4) || __clang__
>>>  #define uefi_call_wrapper(func, va_num, ...)	func(__VA_ARGS__)
>>>  #else
>>>  /* for x86_64, EFI_FUNCTION_WRAPPER must be defined */
>> /* for x86_64, EFI_FUNCTION_WRAPPER must be defined */
>> #if defined(HAVE_USE_MS_ABI)
>> #define uefi_call_wrapper(func, va_num, ...) func(__VA_ARGS__)
>> #else
>> UINTN uefi_call_wrapper(void *func, unsigned long va_num, ...);
>> #endif
>> #define EFI_FUNCTION __attribute__((ms_abi))
>>
>> I think this makes clear that the needed feature here is the
>> attribute, which certainly wasn't available in older gcc.
>>
>> With that the question is whether the Clang case won't also need
>> a version check, since I can't imagine them having supported this
>> prior to gcc introducing it.
> Clang has an substantially more sane way than GCC of checking for
> individual features.  I will introduce and use the
> __has_{attribute,feature}() infrastructure to tests like this.
>
> Experimentally, Clang 3.5 does have ms_abi support

Looking at it further, this entire block is unsed.  Nothing in tree
refers to uefi_call_wrapper() or EFI_FUNCTION_WRAPPER other than this
small bit; all declarations use EFIABI directly.

i.e. this:

diff --git a/xen/include/asm-x86/x86_64/efibind.h
b/xen/include/asm-x86/x86_64/efibind.h
index af5f424..b013db1 100644
--- a/xen/include/asm-x86/x86_64/efibind.h
+++ b/xen/include/asm-x86/x86_64/efibind.h
@@ -274,17 +274,6 @@ typedef uint64_t   UINTN;
 #endif
 #endif
 
-#if __GNUC__ > 4 || (__GNUC__ == 4 && __GNUC_MINOR__ >= 4)
-#define uefi_call_wrapper(func, va_num, ...)   func(__VA_ARGS__)
-#else
-/* for x86_64, EFI_FUNCTION_WRAPPER must be defined */
-#ifdef  EFI_FUNCTION_WRAPPER
-UINTN uefi_call_wrapper(void *func, unsigned long va_num, ...);
-#else
-#error "EFI_FUNCTION_WRAPPER must be defined for x86_64 architecture"
-#endif
-#endif
-
 #ifdef _MSC_EXTENSIONS
 #pragma warning ( disable : 4731 )  // Suppress warnings about
modification of EBP
 #endif

works correctly for GCC and clang.  Would dropping this completely be
acceptable?

~Andrew

Re: [PATCH 8/8] x86/efi: Generate uefi_call_wrapper() when compiling with clang

[Jan Beulich]

>>> On 10.02.16 at 20:11, <andrew.cooper3@citrix.com> wrote:
> On 10/02/16 13:41, Andrew Cooper wrote:
>> On 10/02/16 13:31, Jan Beulich wrote:
>>>>>> On 09.02.16 at 21:01, <andrew.cooper3@citrix.com> wrote:
>>>> Signed-off-by: Andrew Cooper <andrew.cooper3@citrix.com>
>>>> ---
>>>> CC: Jan Beulich <JBeulich@suse.com>
>>>>
>>>> What is the GCC version check supposed to be achieving here?  GCC has
>>>> supported this syntax since 3.0
>>> This is best answered by looking at where we've got these headers
>>> from - the gnu-efi package. It has ...
>>>
>>>> --- a/xen/include/asm-x86/x86_64/efibind.h
>>>> +++ b/xen/include/asm-x86/x86_64/efibind.h
>>>> @@ -274,7 +274,7 @@ typedef uint64_t   UINTN;
>>>>  #endif
>>>>  #endif
>>>>  
>>>> -#if __GNUC__ > 4 || (__GNUC__ == 4 && __GNUC_MINOR__ >= 4)
>>>> +#if __GNUC__ > 4 || (__GNUC__ == 4 && __GNUC_MINOR__ >= 4) || __clang__
>>>>  #define uefi_call_wrapper(func, va_num, ...)	func(__VA_ARGS__)
>>>>  #else
>>>>  /* for x86_64, EFI_FUNCTION_WRAPPER must be defined */
>>> /* for x86_64, EFI_FUNCTION_WRAPPER must be defined */
>>> #if defined(HAVE_USE_MS_ABI)
>>> #define uefi_call_wrapper(func, va_num, ...) func(__VA_ARGS__)
>>> #else
>>> UINTN uefi_call_wrapper(void *func, unsigned long va_num, ...);
>>> #endif
>>> #define EFI_FUNCTION __attribute__((ms_abi))
>>>
>>> I think this makes clear that the needed feature here is the
>>> attribute, which certainly wasn't available in older gcc.
>>>
>>> With that the question is whether the Clang case won't also need
>>> a version check, since I can't imagine them having supported this
>>> prior to gcc introducing it.
>> Clang has an substantially more sane way than GCC of checking for
>> individual features.  I will introduce and use the
>> __has_{attribute,feature}() infrastructure to tests like this.
>>
>> Experimentally, Clang 3.5 does have ms_abi support
> 
> Looking at it further, this entire block is unsed.  Nothing in tree
> refers to uefi_call_wrapper() or EFI_FUNCTION_WRAPPER other than this
> small bit; all declarations use EFIABI directly.
> 
> i.e. this:
> 
> diff --git a/xen/include/asm-x86/x86_64/efibind.h
> b/xen/include/asm-x86/x86_64/efibind.h
> index af5f424..b013db1 100644
> --- a/xen/include/asm-x86/x86_64/efibind.h
> +++ b/xen/include/asm-x86/x86_64/efibind.h
> @@ -274,17 +274,6 @@ typedef uint64_t   UINTN;
>  #endif
>  #endif
>  
> -#if __GNUC__ > 4 || (__GNUC__ == 4 && __GNUC_MINOR__ >= 4)
> -#define uefi_call_wrapper(func, va_num, ...)   func(__VA_ARGS__)
> -#else
> -/* for x86_64, EFI_FUNCTION_WRAPPER must be defined */
> -#ifdef  EFI_FUNCTION_WRAPPER
> -UINTN uefi_call_wrapper(void *func, unsigned long va_num, ...);
> -#else
> -#error "EFI_FUNCTION_WRAPPER must be defined for x86_64 architecture"
> -#endif
> -#endif
> -
>  #ifdef _MSC_EXTENSIONS
>  #pragma warning ( disable : 4731 )  // Suppress warnings about
> modification of EBP
>  #endif
> 
> works correctly for GCC and clang.  Would dropping this completely be
> acceptable?

We perhaps might as well; I had mainly kept it to stay as close to
the original header as possible (without leaving around a latent
build breakage).

Jan

Re: [PATCH 6/8] xen/x86: Avoid overriding initialisers in arrays

[Tian, Kevin]

> From: Andrew Cooper [mailto:andrew.cooper3@citrix.com]
> Sent: Wednesday, February 10, 2016 4:02 AM
> 
> Clang objects to having multiple initialisers when creating an array.
> 
> As this warning is useful for spotting obscure bugs, disabling it is
> unhelpful.  Instead, fix our two deliberate usecases.
> 
> In the p2m-ept case, pull the array out into a helper function, so the helper
> can guarentee to cover the NULL pointer case.
> 
> No functional change.
> 
> Signed-off-by: Andrew Cooper <andrew.cooper3@citrix.com>

Acked-by: Kevin Tian <kevin.tian@intel.com>