This was added as a workaround for really old compilers, and it prevents
building with clang now. I can see no reason for keeping it, as it has
already been removed for most architectures in the pre-git era, so
let's remove it everywhere, rather than only for clang.
Signed-off-by: Arnd Bergmann <[email protected]>
---
arch/s390/Makefile | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/arch/s390/Makefile b/arch/s390/Makefile
index 86c76b149cc2..0087d11e3eaf 100644
--- a/arch/s390/Makefile
+++ b/arch/s390/Makefile
@@ -114,7 +114,7 @@ endif
cfi := $(call as-instr,.cfi_startproc\n.cfi_val_offset 15$(comma)-160\n.cfi_endproc,-DCONFIG_AS_CFI_VAL_OFFSET=1)
KBUILD_CFLAGS += -mbackchain -msoft-float $(cflags-y)
-KBUILD_CFLAGS += -pipe -fno-strength-reduce -Wno-sign-compare
+KBUILD_CFLAGS += -pipe -Wno-sign-compare
KBUILD_CFLAGS += -fno-asynchronous-unwind-tables $(cfi)
KBUILD_AFLAGS += $(aflags-y) $(cfi)
export KBUILD_AFLAGS_DECOMPRESSOR
--
2.20.0
clang produces a harmless warning for each use for the qeth_adp_supported
macro:
drivers/s390/net/qeth_l2_main.c:559:31: warning: implicit conversion from enumeration type 'enum qeth_ipa_setadp_cmd' to
different enumeration type 'enum qeth_ipa_funcs' [-Wenum-conversion]
if (qeth_adp_supported(card, IPA_SETADP_SET_PROMISC_MODE))
~~~~~~~~~~~~~~~~~~~~~~~~~^~~~~~~~~~~~~~~~~~~~~~~~~~~~
drivers/s390/net/qeth_core.h:179:41: note: expanded from macro 'qeth_adp_supported'
qeth_is_ipa_supported(&c->options.adp, f)
~~~~~~~~~~~~~~~~~~~~~ ^
Add a version of this macro that uses the correct types, and
remove the unused qeth_adp_enabled() macro that has the same
problem.
Signed-off-by: Arnd Bergmann <[email protected]>
---
drivers/s390/net/qeth_core.h | 10 +++++++---
1 file changed, 7 insertions(+), 3 deletions(-)
diff --git a/drivers/s390/net/qeth_core.h b/drivers/s390/net/qeth_core.h
index c851cf6e01c4..d603dfea97ab 100644
--- a/drivers/s390/net/qeth_core.h
+++ b/drivers/s390/net/qeth_core.h
@@ -163,6 +163,12 @@ struct qeth_vnicc_info {
bool rx_bcast_enabled;
};
+static inline int qeth_is_adp_supported(struct qeth_ipa_info *ipa,
+ enum qeth_ipa_setadp_cmd func)
+{
+ return (ipa->supported_funcs & func);
+}
+
static inline int qeth_is_ipa_supported(struct qeth_ipa_info *ipa,
enum qeth_ipa_funcs func)
{
@@ -176,9 +182,7 @@ static inline int qeth_is_ipa_enabled(struct qeth_ipa_info *ipa,
}
#define qeth_adp_supported(c, f) \
- qeth_is_ipa_supported(&c->options.adp, f)
-#define qeth_adp_enabled(c, f) \
- qeth_is_ipa_enabled(&c->options.adp, f)
+ qeth_is_adp_supported(&c->options.adp, f)
#define qeth_is_supported(c, f) \
qeth_is_ipa_supported(&c->options.ipa4, f)
#define qeth_is_enabled(c, f) \
--
2.20.0
The 'func_code' variable gets printed in debug statements without
a prior initialization in multiple functions, as reported when building
with clang:
drivers/s390/crypto/zcrypt_api.c:659:6: warning: variable 'func_code' is used uninitialized whenever 'if' condition is true
[-Wsometimes-uninitialized]
if (mex->outputdatalength < mex->inputdatalength) {
^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
drivers/s390/crypto/zcrypt_api.c:725:29: note: uninitialized use occurs here
trace_s390_zcrypt_rep(mex, func_code, rc,
^~~~~~~~~
drivers/s390/crypto/zcrypt_api.c:659:2: note: remove the 'if' if its condition is always false
if (mex->outputdatalength < mex->inputdatalength) {
^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
drivers/s390/crypto/zcrypt_api.c:654:24: note: initialize the variable 'func_code' to silence this warning
unsigned int func_code;
^
Add initializations to all affected code paths to shut up the warning
and make the warning output consistent.
Signed-off-by: Arnd Bergmann <[email protected]>
---
drivers/s390/crypto/zcrypt_api.c | 4 ++++
1 file changed, 4 insertions(+)
diff --git a/drivers/s390/crypto/zcrypt_api.c b/drivers/s390/crypto/zcrypt_api.c
index eb93c2d27d0a..23472063d9a8 100644
--- a/drivers/s390/crypto/zcrypt_api.c
+++ b/drivers/s390/crypto/zcrypt_api.c
@@ -657,6 +657,7 @@ static long zcrypt_rsa_modexpo(struct ap_perms *perms,
trace_s390_zcrypt_req(mex, TP_ICARSAMODEXPO);
if (mex->outputdatalength < mex->inputdatalength) {
+ func_code = -1;
rc = -EINVAL;
goto out;
}
@@ -739,6 +740,7 @@ static long zcrypt_rsa_crt(struct ap_perms *perms,
trace_s390_zcrypt_req(crt, TP_ICARSACRT);
if (crt->outputdatalength < crt->inputdatalength) {
+ func_code = -1;
rc = -EINVAL;
goto out;
}
@@ -946,6 +948,7 @@ static long zcrypt_send_ep11_cprb(struct ap_perms *perms,
targets = kcalloc(target_num, sizeof(*targets), GFP_KERNEL);
if (!targets) {
+ func_code = -1;
rc = -ENOMEM;
goto out;
}
@@ -953,6 +956,7 @@ static long zcrypt_send_ep11_cprb(struct ap_perms *perms,
uptr = (struct ep11_target_dev __force __user *) xcrb->targets;
if (copy_from_user(targets, uptr,
target_num * sizeof(*targets))) {
+ func_code = -1;
rc = -EFAULT;
goto out_free;
}
--
2.20.0
clang points out that the return code from this function is
undefined for one of the error paths:
../drivers/s390/net/ctcm_main.c:1595:7: warning: variable 'result' is used uninitialized whenever 'if' condition is true
[-Wsometimes-uninitialized]
if (priv->channel[direction] == NULL) {
^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
../drivers/s390/net/ctcm_main.c:1638:9: note: uninitialized use occurs here
return result;
^~~~~~
../drivers/s390/net/ctcm_main.c:1595:3: note: remove the 'if' if its condition is always false
if (priv->channel[direction] == NULL) {
^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
../drivers/s390/net/ctcm_main.c:1539:12: note: initialize the variable 'result' to silence this warning
int result;
^
Make it return -ENODEV here, as in the related failure cases.
gcc has a known bug in underreporting some of these warnings
when it has already eliminated the assignment of the return code
based on some earlier optimization step.
Signed-off-by: Arnd Bergmann <[email protected]>
---
drivers/s390/net/ctcm_main.c | 1 +
1 file changed, 1 insertion(+)
diff --git a/drivers/s390/net/ctcm_main.c b/drivers/s390/net/ctcm_main.c
index 7617d21cb296..f63c5c871d3d 100644
--- a/drivers/s390/net/ctcm_main.c
+++ b/drivers/s390/net/ctcm_main.c
@@ -1595,6 +1595,7 @@ static int ctcm_new_device(struct ccwgroup_device *cgdev)
if (priv->channel[direction] == NULL) {
if (direction == CTCM_WRITE)
channel_free(priv->channel[CTCM_READ]);
+ result = -ENODEV;
goto out_dev;
}
priv->channel[direction]->netdev = dev;
--
2.20.0
clang points out that the declaration of cio_irb does not match the
definition exactly, it is missing the alignment attribute:
../drivers/s390/cio/cio.c:50:1: warning: section does not match previous declaration [-Wsection]
DEFINE_PER_CPU_ALIGNED(struct irb, cio_irb);
^
../include/linux/percpu-defs.h:150:2: note: expanded from macro 'DEFINE_PER_CPU_ALIGNED'
DEFINE_PER_CPU_SECTION(type, name, PER_CPU_ALIGNED_SECTION) \
^
../include/linux/percpu-defs.h:93:9: note: expanded from macro 'DEFINE_PER_CPU_SECTION'
extern __PCPU_ATTRS(sec) __typeof__(type) name; \
^
../include/linux/percpu-defs.h:49:26: note: expanded from macro '__PCPU_ATTRS'
__percpu __attribute__((section(PER_CPU_BASE_SECTION sec))) \
^
../drivers/s390/cio/cio.h:118:1: note: previous attribute is here
DECLARE_PER_CPU(struct irb, cio_irb);
^
../include/linux/percpu-defs.h:111:2: note: expanded from macro 'DECLARE_PER_CPU'
DECLARE_PER_CPU_SECTION(type, name, "")
^
../include/linux/percpu-defs.h:87:9: note: expanded from macro 'DECLARE_PER_CPU_SECTION'
extern __PCPU_ATTRS(sec) __typeof__(type) name
^
../include/linux/percpu-defs.h:49:26: note: expanded from macro '__PCPU_ATTRS'
__percpu __attribute__((section(PER_CPU_BASE_SECTION sec))) \
^
Use DECLARE_PER_CPU_ALIGNED() here, to make the two match.
Signed-off-by: Arnd Bergmann <[email protected]>
---
drivers/s390/cio/cio.h | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/drivers/s390/cio/cio.h b/drivers/s390/cio/cio.h
index 9811fd8a0c73..92eabbb5f18d 100644
--- a/drivers/s390/cio/cio.h
+++ b/drivers/s390/cio/cio.h
@@ -115,7 +115,7 @@ struct subchannel {
struct schib_config config;
} __attribute__ ((aligned(8)));
-DECLARE_PER_CPU(struct irb, cio_irb);
+DECLARE_PER_CPU_ALIGNED(struct irb, cio_irb);
#define to_subchannel(n) container_of(n, struct subchannel, dev)
--
2.20.0
Building system calls with clang results in a warning
about an alias from a global function to a static one:
../fs/namei.c:3847:1: warning: unused function '__se_sys_mkdirat' [-Wunused-function]
SYSCALL_DEFINE3(mkdirat, int, dfd, const char __user *, pathname, umode_t, mode)
^
../include/linux/syscalls.h:219:36: note: expanded from macro 'SYSCALL_DEFINE3'
#define SYSCALL_DEFINE3(name, ...) SYSCALL_DEFINEx(3, _##name, __VA_ARGS__)
^
../include/linux/syscalls.h:228:2: note: expanded from macro 'SYSCALL_DEFINEx'
__SYSCALL_DEFINEx(x, sname, __VA_ARGS__)
^
../arch/s390/include/asm/syscall_wrapper.h:126:18: note: expanded from macro '__SYSCALL_DEFINEx'
asmlinkage long __se_sys##name(__MAP(x,__SC_LONG,__VA_ARGS__)) \
^
<scratch space>:31:1: note: expanded from here
__se_sys_mkdirat
^
The only reference to the static __se_sys_mkdirat() here is the alias, but
this only gets evaluated later. Making this function global as well avoids
the warning.
Signed-off-by: Arnd Bergmann <[email protected]>
---
I considered opening a bug report for llvm here, but I suspect the
behavior is intentional, so I'm just fixing it here. Let me know if
you disagree.
---
arch/s390/include/asm/syscall_wrapper.h | 4 ++--
1 file changed, 2 insertions(+), 2 deletions(-)
diff --git a/arch/s390/include/asm/syscall_wrapper.h b/arch/s390/include/asm/syscall_wrapper.h
index 5596c5c625d2..3c3d6fe8e2f0 100644
--- a/arch/s390/include/asm/syscall_wrapper.h
+++ b/arch/s390/include/asm/syscall_wrapper.h
@@ -119,8 +119,8 @@
"Type aliasing is used to sanitize syscall arguments");\
asmlinkage long __s390x_sys##name(__MAP(x,__SC_DECL,__VA_ARGS__)) \
__attribute__((alias(__stringify(__se_sys##name)))); \
- ALLOW_ERROR_INJECTION(__s390x_sys##name, ERRNO); \
- static long __se_sys##name(__MAP(x,__SC_LONG,__VA_ARGS__)); \
+ ALLOW_ERROR_INJECTION(__s390x_sys##name, ERRNO); \
+ long __se_sys##name(__MAP(x,__SC_LONG,__VA_ARGS__)); \
static inline long __do_sys##name(__MAP(x,__SC_DECL,__VA_ARGS__)); \
__S390_SYS_STUBx(x, name, __VA_ARGS__) \
asmlinkage long __se_sys##name(__MAP(x,__SC_LONG,__VA_ARGS__)) \
--
2.20.0
llvm skips an empty .bss section entirely, which makes
the check fail with an unexpected error:
/tmp/binutils-multi-test/bin/s390x-linux-gnu-objdump: section '.bss' mentioned in a -j option, but not found in any input file
error: arch/s390/boot/compressed/decompressor.o .bss section is not empty
../arch/s390/scripts/Makefile.chkbss:20: recipe for target 'arch/s390/boot/compressed/decompressor.o.chkbss' failed
Change the check so we first see if a .bss section exists
before trying to read its size.
Signed-off-by: Arnd Bergmann <[email protected]>
---
arch/s390/scripts/Makefile.chkbss | 3 ++-
1 file changed, 2 insertions(+), 1 deletion(-)
diff --git a/arch/s390/scripts/Makefile.chkbss b/arch/s390/scripts/Makefile.chkbss
index cd7e8f4419f5..884a9caff5fb 100644
--- a/arch/s390/scripts/Makefile.chkbss
+++ b/arch/s390/scripts/Makefile.chkbss
@@ -11,7 +11,8 @@ chkbss: $(addprefix $(obj)/, $(chkbss-files))
quiet_cmd_chkbss = CHKBSS $<
cmd_chkbss = \
- if ! $(OBJDUMP) -j .bss -w -h $< | awk 'END { if ($$3) exit 1 }'; then \
+ if $(OBJDUMP) -h $< | grep -q "\.bss" && \
+ ! $(OBJDUMP) -j .bss -w -h $< | awk 'END { if ($$3) exit 1 }'; then \
echo "error: $< .bss section is not empty" >&2; exit 1; \
fi; \
touch $@;
--
2.20.0
llvm on s390 has problems with __builtin_return_address(n), with n>0,
this results in a somewhat cryptic error message:
fatal error: error in backend: Unsupported stack frame traversal count
To work around it, use the direct return address directly. This
is probably not ideal here, but gets things to compile and should
only lead to inferior reporting, not to misbehavior of the generated
code.
Link: https://bugs.llvm.org/show_bug.cgi?id=41424
Signed-off-by: Arnd Bergmann <[email protected]>
---
arch/s390/include/asm/ftrace.h | 5 +++++
1 file changed, 5 insertions(+)
diff --git a/arch/s390/include/asm/ftrace.h b/arch/s390/include/asm/ftrace.h
index 5a3c95b11952..7923c63946fb 100644
--- a/arch/s390/include/asm/ftrace.h
+++ b/arch/s390/include/asm/ftrace.h
@@ -13,7 +13,12 @@
#ifndef __ASSEMBLY__
+#ifdef CONFIG_CC_IS_CLANG
+/* https://bugs.llvm.org/show_bug.cgi?id=41424 */
+#define ftrace_return_address(n) __builtin_return_address(0)
+#else
#define ftrace_return_address(n) __builtin_return_address(n)
+#endif
void _mcount(void);
void ftrace_caller(void);
--
2.20.0
clang fails to use the %O and %R inline assembly modifiers
the same way as gcc, leading to build failures with every use
of __load_psw_mask():
/tmp/nmi-4a9f80.s: Assembler messages:
/tmp/nmi-4a9f80.s:571: Error: junk at end of line: `+8(160(%r11))'
/tmp/nmi-4a9f80.s:626: Error: junk at end of line: `+8(160(%r11))'
Replace these with a more conventional way of passing the addresses
that should work with both clang and gcc.
Signed-off-by: Arnd Bergmann <[email protected]>
---
arch/s390/include/asm/processor.h | 6 +++---
1 file changed, 3 insertions(+), 3 deletions(-)
diff --git a/arch/s390/include/asm/processor.h b/arch/s390/include/asm/processor.h
index 81038ab357ce..700c650ffd4f 100644
--- a/arch/s390/include/asm/processor.h
+++ b/arch/s390/include/asm/processor.h
@@ -339,10 +339,10 @@ static __no_kasan_or_inline void __load_psw_mask(unsigned long mask)
asm volatile(
" larl %0,1f\n"
- " stg %0,%O1+8(%R1)\n"
- " lpswe %1\n"
+ " stg %0,%1\n"
+ " lpswe %2\n"
"1:"
- : "=&d" (addr), "=Q" (psw) : "Q" (psw) : "memory", "cc");
+ : "=&d" (addr), "=Q" (psw.addr) : "Q" (psw) : "memory", "cc");
}
/*
--
2.20.0
clang does not understand the contraint "0" in the CALL_ON_STACK()
macro:
../arch/s390/mm/maccess.c:117:10: error: invalid input constraint '0' in asm
return CALL_ON_STACK(_memcpy_real, S390_lowcore.nodat_stack,
^
../arch/s390/include/asm/processor.h:292:20: note: expanded from macro 'CALL_ON_STACK'
[_fn] "X" (fn) CALL_FMT_##nr : CALL_CLOBBER_##nr); \
^
<scratch space>:207:1: note: expanded from here
CALL_FMT_3
^
../arch/s390/include/asm/processor.h:267:20: note: expanded from macro 'CALL_FMT_3'
#define CALL_FMT_3 CALL_FMT_2, "d" (r4)
^
../arch/s390/include/asm/processor.h:266:20: note: expanded from macro 'CALL_FMT_2'
#define CALL_FMT_2 CALL_FMT_1, "d" (r3)
^
../arch/s390/include/asm/processor.h:265:32: note: expanded from macro 'CALL_FMT_1'
#define CALL_FMT_1 CALL_FMT_0, "0" (r2)
^
I don't know what the correct fix here would be, changing it to "d" made
it build, since clang does understand this one.
Signed-off-by: Arnd Bergmann <[email protected]>
---
arch/s390/include/asm/processor.h | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/arch/s390/include/asm/processor.h b/arch/s390/include/asm/processor.h
index 700c650ffd4f..84c59c99668a 100644
--- a/arch/s390/include/asm/processor.h
+++ b/arch/s390/include/asm/processor.h
@@ -262,7 +262,7 @@ static __no_kasan_or_inline unsigned short stap(void)
register unsigned long r4 asm("6") = (unsigned long)(arg5)
#define CALL_FMT_0
-#define CALL_FMT_1 CALL_FMT_0, "0" (r2)
+#define CALL_FMT_1 CALL_FMT_0, "d" (r2)
#define CALL_FMT_2 CALL_FMT_1, "d" (r3)
#define CALL_FMT_3 CALL_FMT_2, "d" (r4)
#define CALL_FMT_4 CALL_FMT_3, "d" (r5)
--
2.20.0
clang does not support 31 bit object files on s390, so skip
the 32-bit vdso here, and only build it when using gcc to compile
the kernel.
Signed-off-by: Arnd Bergmann <[email protected]>
---
arch/s390/Kconfig | 3 +++
arch/s390/kernel/Makefile | 2 +-
arch/s390/kernel/vdso.c | 10 +++++-----
3 files changed, 9 insertions(+), 6 deletions(-)
diff --git a/arch/s390/Kconfig b/arch/s390/Kconfig
index b6e3d0653002..8cd860cba4d1 100644
--- a/arch/s390/Kconfig
+++ b/arch/s390/Kconfig
@@ -388,6 +388,9 @@ config COMPAT
(and some other stuff like libraries and such) is needed for
executing 31 bit applications. It is safe to say "Y".
+config COMPAT_VDSO
+ def_bool COMPAT && !CC_IS_CLANG
+
config SYSVIPC_COMPAT
def_bool y if COMPAT && SYSVIPC
diff --git a/arch/s390/kernel/Makefile b/arch/s390/kernel/Makefile
index 8a62c7f72e1b..1222db6d4ee9 100644
--- a/arch/s390/kernel/Makefile
+++ b/arch/s390/kernel/Makefile
@@ -86,7 +86,7 @@ obj-$(CONFIG_TRACEPOINTS) += trace.o
# vdso
obj-y += vdso64/
-obj-$(CONFIG_COMPAT) += vdso32/
+obj-$(CONFIG_COMPAT_VDSO) += vdso32/
chkbss := head64.o early_nobss.o
include $(srctree)/arch/s390/scripts/Makefile.chkbss
diff --git a/arch/s390/kernel/vdso.c b/arch/s390/kernel/vdso.c
index e7920a68a12e..243d8b1185bf 100644
--- a/arch/s390/kernel/vdso.c
+++ b/arch/s390/kernel/vdso.c
@@ -29,7 +29,7 @@
#include <asm/vdso.h>
#include <asm/facility.h>
-#ifdef CONFIG_COMPAT
+#ifdef CONFIG_COMPAT_VDSO
extern char vdso32_start, vdso32_end;
static void *vdso32_kbase = &vdso32_start;
static unsigned int vdso32_pages;
@@ -55,7 +55,7 @@ static vm_fault_t vdso_fault(const struct vm_special_mapping *sm,
vdso_pagelist = vdso64_pagelist;
vdso_pages = vdso64_pages;
-#ifdef CONFIG_COMPAT
+#ifdef CONFIG_COMPAT_VDSO
if (vma->vm_mm->context.compat_mm) {
vdso_pagelist = vdso32_pagelist;
vdso_pages = vdso32_pages;
@@ -76,7 +76,7 @@ static int vdso_mremap(const struct vm_special_mapping *sm,
unsigned long vdso_pages;
vdso_pages = vdso64_pages;
-#ifdef CONFIG_COMPAT
+#ifdef CONFIG_COMPAT_VDSO
if (vma->vm_mm->context.compat_mm)
vdso_pages = vdso32_pages;
#endif
@@ -223,7 +223,7 @@ int arch_setup_additional_pages(struct linux_binprm *bprm, int uses_interp)
return 0;
vdso_pages = vdso64_pages;
-#ifdef CONFIG_COMPAT
+#ifdef CONFIG_COMPAT_VDSO
mm->context.compat_mm = is_compat_task();
if (mm->context.compat_mm)
vdso_pages = vdso32_pages;
@@ -280,7 +280,7 @@ static int __init vdso_init(void)
int i;
vdso_init_data(vdso_data);
-#ifdef CONFIG_COMPAT
+#ifdef CONFIG_COMPAT_VDSO
/* Calculate the size of the 32 bit vDSO */
vdso32_pages = ((&vdso32_end - &vdso32_start
+ PAGE_SIZE - 1) >> PAGE_SHIFT) + 1;
--
2.20.0
The purgatory Makefile does not inherit the original cflags,
so clang falls back to the default target architecture when
building it, typically this would be x86 when cross-compiling.
Pass --target=s390x-linux to all compilers that understand
this option.
Signed-off-by: Arnd Bergmann <[email protected]>
---
arch/s390/purgatory/Makefile | 1 +
1 file changed, 1 insertion(+)
diff --git a/arch/s390/purgatory/Makefile b/arch/s390/purgatory/Makefile
index ce6a3f75065b..3a14b968cec3 100644
--- a/arch/s390/purgatory/Makefile
+++ b/arch/s390/purgatory/Makefile
@@ -22,6 +22,7 @@ KBUILD_CFLAGS := -fno-strict-aliasing -Wall -Wstrict-prototypes
KBUILD_CFLAGS += -Wno-pointer-sign -Wno-sign-compare
KBUILD_CFLAGS += -fno-zero-initialized-in-bss -fno-builtin -ffreestanding
KBUILD_CFLAGS += -c -MD -Os -m64 -msoft-float -fno-common
+KBUILD_CFLAGS += $(call cc-option,--target=s390x-linux)
KBUILD_CFLAGS += $(call cc-option,-fno-PIE)
KBUILD_AFLAGS := $(filter-out -DCC_USING_EXPOLINE,$(KBUILD_AFLAGS))
--
2.20.0
On Mon, Apr 08, 2019 at 11:26:16PM +0200, Arnd Bergmann wrote:
> The purgatory Makefile does not inherit the original cflags,
> so clang falls back to the default target architecture when
> building it, typically this would be x86 when cross-compiling.
>
> Pass --target=s390x-linux to all compilers that understand
> this option.
>
> Signed-off-by: Arnd Bergmann <[email protected]>
> ---
> arch/s390/purgatory/Makefile | 1 +
> 1 file changed, 1 insertion(+)
>
> diff --git a/arch/s390/purgatory/Makefile b/arch/s390/purgatory/Makefile
> index ce6a3f75065b..3a14b968cec3 100644
> --- a/arch/s390/purgatory/Makefile
> +++ b/arch/s390/purgatory/Makefile
> @@ -22,6 +22,7 @@ KBUILD_CFLAGS := -fno-strict-aliasing -Wall -Wstrict-prototypes
> KBUILD_CFLAGS += -Wno-pointer-sign -Wno-sign-compare
> KBUILD_CFLAGS += -fno-zero-initialized-in-bss -fno-builtin -ffreestanding
> KBUILD_CFLAGS += -c -MD -Os -m64 -msoft-float -fno-common
> +KBUILD_CFLAGS += $(call cc-option,--target=s390x-linux)
> KBUILD_CFLAGS += $(call cc-option,-fno-PIE)
> KBUILD_AFLAGS := $(filter-out -DCC_USING_EXPOLINE,$(KBUILD_AFLAGS))
>
> --
> 2.20.0
>
Would
ifdef CONFIG_CC_IS_CLANG
KBUILD_CFLAGS += --target=s390x-linux
endif
be a little clearer (and save a cc-option call)?
Otherwise, makes sense.
Reviewed-by: Nathan Chancellor <[email protected]>
On Mon, Apr 08, 2019 at 11:26:19PM +0200, Arnd Bergmann wrote:
> clang points out that the return code from this function is
> undefined for one of the error paths:
>
> ../drivers/s390/net/ctcm_main.c:1595:7: warning: variable 'result' is used uninitialized whenever 'if' condition is true
> [-Wsometimes-uninitialized]
> if (priv->channel[direction] == NULL) {
> ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
> ../drivers/s390/net/ctcm_main.c:1638:9: note: uninitialized use occurs here
> return result;
> ^~~~~~
> ../drivers/s390/net/ctcm_main.c:1595:3: note: remove the 'if' if its condition is always false
> if (priv->channel[direction] == NULL) {
> ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
> ../drivers/s390/net/ctcm_main.c:1539:12: note: initialize the variable 'result' to silence this warning
> int result;
> ^
>
> Make it return -ENODEV here, as in the related failure cases.
> gcc has a known bug in underreporting some of these warnings
> when it has already eliminated the assignment of the return code
> based on some earlier optimization step.
>
> Signed-off-by: Arnd Bergmann <[email protected]>
Reviewed-by: Nathan Chancellor <[email protected]>
> ---
> drivers/s390/net/ctcm_main.c | 1 +
> 1 file changed, 1 insertion(+)
>
> diff --git a/drivers/s390/net/ctcm_main.c b/drivers/s390/net/ctcm_main.c
> index 7617d21cb296..f63c5c871d3d 100644
> --- a/drivers/s390/net/ctcm_main.c
> +++ b/drivers/s390/net/ctcm_main.c
> @@ -1595,6 +1595,7 @@ static int ctcm_new_device(struct ccwgroup_device *cgdev)
> if (priv->channel[direction] == NULL) {
> if (direction == CTCM_WRITE)
> channel_free(priv->channel[CTCM_READ]);
> + result = -ENODEV;
> goto out_dev;
> }
> priv->channel[direction]->netdev = dev;
> --
> 2.20.0
>
On Mon, Apr 08, 2019 at 11:26:18PM +0200, Arnd Bergmann wrote:
> The 'func_code' variable gets printed in debug statements without
> a prior initialization in multiple functions, as reported when building
> with clang:
>
> drivers/s390/crypto/zcrypt_api.c:659:6: warning: variable 'func_code' is used uninitialized whenever 'if' condition is true
> [-Wsometimes-uninitialized]
> if (mex->outputdatalength < mex->inputdatalength) {
> ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
> drivers/s390/crypto/zcrypt_api.c:725:29: note: uninitialized use occurs here
> trace_s390_zcrypt_rep(mex, func_code, rc,
> ^~~~~~~~~
> drivers/s390/crypto/zcrypt_api.c:659:2: note: remove the 'if' if its condition is always false
> if (mex->outputdatalength < mex->inputdatalength) {
> ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
> drivers/s390/crypto/zcrypt_api.c:654:24: note: initialize the variable 'func_code' to silence this warning
> unsigned int func_code;
> ^
>
> Add initializations to all affected code paths to shut up the warning
> and make the warning output consistent.
>
> Signed-off-by: Arnd Bergmann <[email protected]>
I'll never get used to seeing negative numbers assigned to unsigned
integers...
Reviewed-by: Nathan Chancellor <[email protected]>
> ---
> drivers/s390/crypto/zcrypt_api.c | 4 ++++
> 1 file changed, 4 insertions(+)
>
> diff --git a/drivers/s390/crypto/zcrypt_api.c b/drivers/s390/crypto/zcrypt_api.c
> index eb93c2d27d0a..23472063d9a8 100644
> --- a/drivers/s390/crypto/zcrypt_api.c
> +++ b/drivers/s390/crypto/zcrypt_api.c
> @@ -657,6 +657,7 @@ static long zcrypt_rsa_modexpo(struct ap_perms *perms,
> trace_s390_zcrypt_req(mex, TP_ICARSAMODEXPO);
>
> if (mex->outputdatalength < mex->inputdatalength) {
> + func_code = -1;
> rc = -EINVAL;
> goto out;
> }
> @@ -739,6 +740,7 @@ static long zcrypt_rsa_crt(struct ap_perms *perms,
> trace_s390_zcrypt_req(crt, TP_ICARSACRT);
>
> if (crt->outputdatalength < crt->inputdatalength) {
> + func_code = -1;
> rc = -EINVAL;
> goto out;
> }
> @@ -946,6 +948,7 @@ static long zcrypt_send_ep11_cprb(struct ap_perms *perms,
>
> targets = kcalloc(target_num, sizeof(*targets), GFP_KERNEL);
> if (!targets) {
> + func_code = -1;
> rc = -ENOMEM;
> goto out;
> }
> @@ -953,6 +956,7 @@ static long zcrypt_send_ep11_cprb(struct ap_perms *perms,
> uptr = (struct ep11_target_dev __force __user *) xcrb->targets;
> if (copy_from_user(targets, uptr,
> target_num * sizeof(*targets))) {
> + func_code = -1;
> rc = -EFAULT;
> goto out_free;
> }
> --
> 2.20.0
>
On Mon, Apr 08, 2019 at 11:26:20PM +0200, Arnd Bergmann wrote:
> clang points out that the declaration of cio_irb does not match the
> definition exactly, it is missing the alignment attribute:
>
> ../drivers/s390/cio/cio.c:50:1: warning: section does not match previous declaration [-Wsection]
> DEFINE_PER_CPU_ALIGNED(struct irb, cio_irb);
> ^
> ../include/linux/percpu-defs.h:150:2: note: expanded from macro 'DEFINE_PER_CPU_ALIGNED'
> DEFINE_PER_CPU_SECTION(type, name, PER_CPU_ALIGNED_SECTION) \
> ^
> ../include/linux/percpu-defs.h:93:9: note: expanded from macro 'DEFINE_PER_CPU_SECTION'
> extern __PCPU_ATTRS(sec) __typeof__(type) name; \
> ^
> ../include/linux/percpu-defs.h:49:26: note: expanded from macro '__PCPU_ATTRS'
> __percpu __attribute__((section(PER_CPU_BASE_SECTION sec))) \
> ^
> ../drivers/s390/cio/cio.h:118:1: note: previous attribute is here
> DECLARE_PER_CPU(struct irb, cio_irb);
> ^
> ../include/linux/percpu-defs.h:111:2: note: expanded from macro 'DECLARE_PER_CPU'
> DECLARE_PER_CPU_SECTION(type, name, "")
> ^
> ../include/linux/percpu-defs.h:87:9: note: expanded from macro 'DECLARE_PER_CPU_SECTION'
> extern __PCPU_ATTRS(sec) __typeof__(type) name
> ^
> ../include/linux/percpu-defs.h:49:26: note: expanded from macro '__PCPU_ATTRS'
> __percpu __attribute__((section(PER_CPU_BASE_SECTION sec))) \
> ^
> Use DECLARE_PER_CPU_ALIGNED() here, to make the two match.
>
> Signed-off-by: Arnd Bergmann <[email protected]>
Reviewed-by: Nathan Chancellor <[email protected]>
> ---
> drivers/s390/cio/cio.h | 2 +-
> 1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/drivers/s390/cio/cio.h b/drivers/s390/cio/cio.h
> index 9811fd8a0c73..92eabbb5f18d 100644
> --- a/drivers/s390/cio/cio.h
> +++ b/drivers/s390/cio/cio.h
> @@ -115,7 +115,7 @@ struct subchannel {
> struct schib_config config;
> } __attribute__ ((aligned(8)));
>
> -DECLARE_PER_CPU(struct irb, cio_irb);
> +DECLARE_PER_CPU_ALIGNED(struct irb, cio_irb);
>
> #define to_subchannel(n) container_of(n, struct subchannel, dev)
>
> --
> 2.20.0
>
On Mon, Apr 08, 2019 at 11:26:17PM +0200, Arnd Bergmann wrote:
> clang produces a harmless warning for each use for the qeth_adp_supported
> macro:
>
> drivers/s390/net/qeth_l2_main.c:559:31: warning: implicit conversion from enumeration type 'enum qeth_ipa_setadp_cmd' to
> different enumeration type 'enum qeth_ipa_funcs' [-Wenum-conversion]
> if (qeth_adp_supported(card, IPA_SETADP_SET_PROMISC_MODE))
> ~~~~~~~~~~~~~~~~~~~~~~~~~^~~~~~~~~~~~~~~~~~~~~~~~~~~~
> drivers/s390/net/qeth_core.h:179:41: note: expanded from macro 'qeth_adp_supported'
> qeth_is_ipa_supported(&c->options.adp, f)
> ~~~~~~~~~~~~~~~~~~~~~ ^
>
> Add a version of this macro that uses the correct types, and
> remove the unused qeth_adp_enabled() macro that has the same
> problem.
>
> Signed-off-by: Arnd Bergmann <[email protected]>
I wonder if it is better to just change the func parameter to type long.
I guess it's better to keep the type safety to make sure values aren't
unintentionally mixed but the body of the functions is the same so does
the type actually matter?
Regardless, this change does fix the warning so:
Reviewed-by: Nathan Chancellor <[email protected]>
> ---
> drivers/s390/net/qeth_core.h | 10 +++++++---
> 1 file changed, 7 insertions(+), 3 deletions(-)
>
> diff --git a/drivers/s390/net/qeth_core.h b/drivers/s390/net/qeth_core.h
> index c851cf6e01c4..d603dfea97ab 100644
> --- a/drivers/s390/net/qeth_core.h
> +++ b/drivers/s390/net/qeth_core.h
> @@ -163,6 +163,12 @@ struct qeth_vnicc_info {
> bool rx_bcast_enabled;
> };
>
> +static inline int qeth_is_adp_supported(struct qeth_ipa_info *ipa,
> + enum qeth_ipa_setadp_cmd func)
> +{
> + return (ipa->supported_funcs & func);
> +}
> +
> static inline int qeth_is_ipa_supported(struct qeth_ipa_info *ipa,
> enum qeth_ipa_funcs func)
> {
> @@ -176,9 +182,7 @@ static inline int qeth_is_ipa_enabled(struct qeth_ipa_info *ipa,
> }
>
> #define qeth_adp_supported(c, f) \
> - qeth_is_ipa_supported(&c->options.adp, f)
> -#define qeth_adp_enabled(c, f) \
> - qeth_is_ipa_enabled(&c->options.adp, f)
> + qeth_is_adp_supported(&c->options.adp, f)
> #define qeth_is_supported(c, f) \
> qeth_is_ipa_supported(&c->options.ipa4, f)
> #define qeth_is_enabled(c, f) \
> --
> 2.20.0
>
On Tue, Apr 9, 2019 at 12:03 AM Nathan Chancellor
<[email protected]> wrote:
>
> On Mon, Apr 08, 2019 at 11:26:16PM +0200, Arnd Bergmann wrote:
> > The purgatory Makefile does not inherit the original cflags,
> > so clang falls back to the default target architecture when
> > building it, typically this would be x86 when cross-compiling.
> >
> > Pass --target=s390x-linux to all compilers that understand
> > this option.
> >
> > Signed-off-by: Arnd Bergmann <[email protected]>
> > ---
> > arch/s390/purgatory/Makefile | 1 +
> > 1 file changed, 1 insertion(+)
> >
> > diff --git a/arch/s390/purgatory/Makefile b/arch/s390/purgatory/Makefile
> > index ce6a3f75065b..3a14b968cec3 100644
> > --- a/arch/s390/purgatory/Makefile
> > +++ b/arch/s390/purgatory/Makefile
> > @@ -22,6 +22,7 @@ KBUILD_CFLAGS := -fno-strict-aliasing -Wall -Wstrict-prototypes
> > KBUILD_CFLAGS += -Wno-pointer-sign -Wno-sign-compare
> > KBUILD_CFLAGS += -fno-zero-initialized-in-bss -fno-builtin -ffreestanding
> > KBUILD_CFLAGS += -c -MD -Os -m64 -msoft-float -fno-common
> > +KBUILD_CFLAGS += $(call cc-option,--target=s390x-linux)
> > KBUILD_CFLAGS += $(call cc-option,-fno-PIE)
> > KBUILD_AFLAGS := $(filter-out -DCC_USING_EXPOLINE,$(KBUILD_AFLAGS))
> >
> Would
>
> ifdef CONFIG_CC_IS_CLANG
> KBUILD_CFLAGS += --target=s390x-linux
> endif
>
> be a little clearer (and save a cc-option call)?
Fine with me as well. Actually I noticed later that we need the same
thing for arch/s390/boot/Makefile and arch/s390/boot/compressed/Makefile
in some form, so maybe we should drop this one for now and find
a solution that works for all three of them. The boot stuff is the one
patch I did not send, since I did not think I had a good solution there,
just one that happened to make it build.
> Otherwise, makes sense.
>
> Reviewed-by: Nathan Chancellor <[email protected]>
Thanks,
Arnd
On Tue, Apr 9, 2019 at 12:16 AM Nathan Chancellor
<[email protected]> wrote:
>
> On Mon, Apr 08, 2019 at 11:26:17PM +0200, Arnd Bergmann wrote:
> > clang produces a harmless warning for each use for the qeth_adp_supported
> > macro:
> >
> > drivers/s390/net/qeth_l2_main.c:559:31: warning: implicit conversion from enumeration type 'enum qeth_ipa_setadp_cmd' to
> > different enumeration type 'enum qeth_ipa_funcs' [-Wenum-conversion]
> > if (qeth_adp_supported(card, IPA_SETADP_SET_PROMISC_MODE))
> > ~~~~~~~~~~~~~~~~~~~~~~~~~^~~~~~~~~~~~~~~~~~~~~~~~~~~~
> > drivers/s390/net/qeth_core.h:179:41: note: expanded from macro 'qeth_adp_supported'
> > qeth_is_ipa_supported(&c->options.adp, f)
> > ~~~~~~~~~~~~~~~~~~~~~ ^
> >
> > Add a version of this macro that uses the correct types, and
> > remove the unused qeth_adp_enabled() macro that has the same
> > problem.
> >
> > Signed-off-by: Arnd Bergmann <[email protected]>
>
> I wonder if it is better to just change the func parameter to type long.
> I guess it's better to keep the type safety to make sure values aren't
> unintentionally mixed but the body of the functions is the same so does
> the type actually matter?
I think using the right enum type makes most sense here, as this seems
to be something that is easy to confuse.
Arnd
On 08.04.19 23:26, Arnd Bergmann wrote:
> The 'func_code' variable gets printed in debug statements without
> a prior initialization in multiple functions, as reported when building
> with clang:
>
> drivers/s390/crypto/zcrypt_api.c:659:6: warning: variable 'func_code' is used uninitialized whenever 'if' condition is true
> [-Wsometimes-uninitialized]
> if (mex->outputdatalength < mex->inputdatalength) {
> ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
> drivers/s390/crypto/zcrypt_api.c:725:29: note: uninitialized use occurs here
> trace_s390_zcrypt_rep(mex, func_code, rc,
> ^~~~~~~~~
> drivers/s390/crypto/zcrypt_api.c:659:2: note: remove the 'if' if its condition is always false
> if (mex->outputdatalength < mex->inputdatalength) {
> ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
> drivers/s390/crypto/zcrypt_api.c:654:24: note: initialize the variable 'func_code' to silence this warning
> unsigned int func_code;
> ^
>
> Add initializations to all affected code paths to shut up the warning
> and make the warning output consistent.
>
> Signed-off-by: Arnd Bergmann <[email protected]>
> ---
> drivers/s390/crypto/zcrypt_api.c | 4 ++++
> 1 file changed, 4 insertions(+)
>
> diff --git a/drivers/s390/crypto/zcrypt_api.c b/drivers/s390/crypto/zcrypt_api.c
> index eb93c2d27d0a..23472063d9a8 100644
> --- a/drivers/s390/crypto/zcrypt_api.c
> +++ b/drivers/s390/crypto/zcrypt_api.c
> @@ -657,6 +657,7 @@ static long zcrypt_rsa_modexpo(struct ap_perms *perms,
> trace_s390_zcrypt_req(mex, TP_ICARSAMODEXPO);
>
> if (mex->outputdatalength < mex->inputdatalength) {
> + func_code = -1;
> rc = -EINVAL;
> goto out;
> }
> @@ -739,6 +740,7 @@ static long zcrypt_rsa_crt(struct ap_perms *perms,
> trace_s390_zcrypt_req(crt, TP_ICARSACRT);
>
> if (crt->outputdatalength < crt->inputdatalength) {
> + func_code = -1;
> rc = -EINVAL;
> goto out;
> }
> @@ -946,6 +948,7 @@ static long zcrypt_send_ep11_cprb(struct ap_perms *perms,
>
> targets = kcalloc(target_num, sizeof(*targets), GFP_KERNEL);
> if (!targets) {
> + func_code = -1;
> rc = -ENOMEM;
> goto out;
> }
> @@ -953,6 +956,7 @@ static long zcrypt_send_ep11_cprb(struct ap_perms *perms,
> uptr = (struct ep11_target_dev __force __user *) xcrb->targets;
> if (copy_from_user(targets, uptr,
> target_num * sizeof(*targets))) {
> + func_code = -1;
> rc = -EFAULT;
> goto out_free;
> }
Thanks Arnd, but as Nathan already wrote, I'd prefer to have the
variable initialized with 0 instead of -1.
If you agree with this, I'll rewrite the patch and apply it to our
internal git and it will appear at kernel org with the next s390 code merge then.
regards Harald Freudenberger
On Tue, Apr 9, 2019 at 11:54 AM Harald Freudenberger
<[email protected]> wrote:
> On 08.04.19 23:26, Arnd Bergmann wrote:
> Thanks Arnd, but as Nathan already wrote, I'd prefer to have the
> variable initialized with 0 instead of -1.
> If you agree with this, I'll rewrite the patch and apply it to our
> internal git and it will appear at kernel org with the next s390 code merge then.
That's fine with me, I don't care about the specific value that gets
assigned here.
Thanks,
Arnd
On Mon, 8 Apr 2019, Arnd Bergmann wrote:
> clang points out that the declaration of cio_irb does not match the
> definition exactly, it is missing the alignment attribute:
>
> ../drivers/s390/cio/cio.c:50:1: warning: section does not match previous declaration [-Wsection]
> DEFINE_PER_CPU_ALIGNED(struct irb, cio_irb);
> ^
> ../include/linux/percpu-defs.h:150:2: note: expanded from macro 'DEFINE_PER_CPU_ALIGNED'
> DEFINE_PER_CPU_SECTION(type, name, PER_CPU_ALIGNED_SECTION) \
> ^
> ../include/linux/percpu-defs.h:93:9: note: expanded from macro 'DEFINE_PER_CPU_SECTION'
> extern __PCPU_ATTRS(sec) __typeof__(type) name; \
> ^
> ../include/linux/percpu-defs.h:49:26: note: expanded from macro '__PCPU_ATTRS'
> __percpu __attribute__((section(PER_CPU_BASE_SECTION sec))) \
> ^
> ../drivers/s390/cio/cio.h:118:1: note: previous attribute is here
> DECLARE_PER_CPU(struct irb, cio_irb);
> ^
> ../include/linux/percpu-defs.h:111:2: note: expanded from macro 'DECLARE_PER_CPU'
> DECLARE_PER_CPU_SECTION(type, name, "")
> ^
> ../include/linux/percpu-defs.h:87:9: note: expanded from macro 'DECLARE_PER_CPU_SECTION'
> extern __PCPU_ATTRS(sec) __typeof__(type) name
> ^
> ../include/linux/percpu-defs.h:49:26: note: expanded from macro '__PCPU_ATTRS'
> __percpu __attribute__((section(PER_CPU_BASE_SECTION sec))) \
> ^
> Use DECLARE_PER_CPU_ALIGNED() here, to make the two match.
>
> Signed-off-by: Arnd Bergmann <[email protected]>
Thanks for the patch! Applied.
Sebastian
On Tue, Apr 09, 2019 at 08:43:09AM +0200, Arnd Bergmann wrote:
> On Tue, Apr 9, 2019 at 12:03 AM Nathan Chancellor
> <[email protected]> wrote:
> >
> > On Mon, Apr 08, 2019 at 11:26:16PM +0200, Arnd Bergmann wrote:
> > > The purgatory Makefile does not inherit the original cflags,
> > > so clang falls back to the default target architecture when
> > > building it, typically this would be x86 when cross-compiling.
> > >
> > > Pass --target=s390x-linux to all compilers that understand
> > > this option.
> > >
> > > Signed-off-by: Arnd Bergmann <[email protected]>
> > > ---
> > > arch/s390/purgatory/Makefile | 1 +
> > > 1 file changed, 1 insertion(+)
> > >
> > > diff --git a/arch/s390/purgatory/Makefile b/arch/s390/purgatory/Makefile
> > > index ce6a3f75065b..3a14b968cec3 100644
> > > --- a/arch/s390/purgatory/Makefile
> > > +++ b/arch/s390/purgatory/Makefile
> > > @@ -22,6 +22,7 @@ KBUILD_CFLAGS := -fno-strict-aliasing -Wall -Wstrict-prototypes
> > > KBUILD_CFLAGS += -Wno-pointer-sign -Wno-sign-compare
> > > KBUILD_CFLAGS += -fno-zero-initialized-in-bss -fno-builtin -ffreestanding
> > > KBUILD_CFLAGS += -c -MD -Os -m64 -msoft-float -fno-common
> > > +KBUILD_CFLAGS += $(call cc-option,--target=s390x-linux)
> > > KBUILD_CFLAGS += $(call cc-option,-fno-PIE)
> > > KBUILD_AFLAGS := $(filter-out -DCC_USING_EXPOLINE,$(KBUILD_AFLAGS))
> > >
> > Would
> >
> > ifdef CONFIG_CC_IS_CLANG
> > KBUILD_CFLAGS += --target=s390x-linux
> > endif
> >
> > be a little clearer (and save a cc-option call)?
>
> Fine with me as well. Actually I noticed later that we need the same
> thing for arch/s390/boot/Makefile and arch/s390/boot/compressed/Makefile
> in some form, so maybe we should drop this one for now and find
> a solution that works for all three of them. The boot stuff is the one
> patch I did not send, since I did not think I had a good solution there,
> just one that happened to make it build.
>
It did just occur to me that we added the --target and --gcc-toolchain
flags to a clang specific variable, CLANG_FLAGS, that has already been
used by powerpc in commit 813af51f5d30 ("powerpc/boot: Set target when
cross-compiling for clang"). I assume that could be used here?
Nathan
> > Otherwise, makes sense.
> >
> > Reviewed-by: Nathan Chancellor <[email protected]>
>
> Thanks,
>
> Arnd
On Tue, Apr 9, 2019 at 9:30 AM Nathan Chancellor
<[email protected]> wrote:
> It did just occur to me that we added the --target and --gcc-toolchain
> flags to a clang specific variable, CLANG_FLAGS, that has already been
> used by powerpc in commit 813af51f5d30 ("powerpc/boot: Set target when
> cross-compiling for clang"). I assume that could be used here?
I think reusing CLANG_FLAGS is a better approach.
--
Thanks,
~Nick Desaulniers
On Mon, 8 Apr 2019 23:26:25 +0200
Arnd Bergmann <[email protected]> wrote:
> clang does not understand the contraint "0" in the CALL_ON_STACK()
> macro:
>
> ../arch/s390/mm/maccess.c:117:10: error: invalid input constraint '0' in asm
> return CALL_ON_STACK(_memcpy_real, S390_lowcore.nodat_stack,
> ^
> ../arch/s390/include/asm/processor.h:292:20: note: expanded from macro 'CALL_ON_STACK'
> [_fn] "X" (fn) CALL_FMT_##nr : CALL_CLOBBER_##nr); \
> ^
> <scratch space>:207:1: note: expanded from here
> CALL_FMT_3
> ^
> ../arch/s390/include/asm/processor.h:267:20: note: expanded from macro 'CALL_FMT_3'
> #define CALL_FMT_3 CALL_FMT_2, "d" (r4)
> ^
> ../arch/s390/include/asm/processor.h:266:20: note: expanded from macro 'CALL_FMT_2'
> #define CALL_FMT_2 CALL_FMT_1, "d" (r3)
> ^
> ../arch/s390/include/asm/processor.h:265:32: note: expanded from macro 'CALL_FMT_1'
> #define CALL_FMT_1 CALL_FMT_0, "0" (r2)
> ^
>
> I don't know what the correct fix here would be, changing it to "d" made
> it build, since clang does understand this one.
>
> Signed-off-by: Arnd Bergmann <[email protected]>
> ---
> arch/s390/include/asm/processor.h | 2 +-
> 1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/arch/s390/include/asm/processor.h b/arch/s390/include/asm/processor.h
> index 700c650ffd4f..84c59c99668a 100644
> --- a/arch/s390/include/asm/processor.h
> +++ b/arch/s390/include/asm/processor.h
> @@ -262,7 +262,7 @@ static __no_kasan_or_inline unsigned short stap(void)
> register unsigned long r4 asm("6") = (unsigned long)(arg5)
>
> #define CALL_FMT_0
> -#define CALL_FMT_1 CALL_FMT_0, "0" (r2)
> +#define CALL_FMT_1 CALL_FMT_0, "d" (r2)
> #define CALL_FMT_2 CALL_FMT_1, "d" (r3)
> #define CALL_FMT_3 CALL_FMT_2, "d" (r4)
> #define CALL_FMT_4 CALL_FMT_3, "d" (r5)
This is (slightly) wrong. %r2 is used as the input register for the first argument
and the result value for the call. With your patch you force the compiler to load
the first argument in two registers. One solution would be to CALL_FMT1 as
#define CALL_FMT1 CALL_FMT_0
It still is not optimal though as for CALL_FMT_0 the "+&d" (r2) indicates an
input but CALL_ARGS_0 does not initialize r2.
I am thinking about the following patch to cover all cases:
--
From 91a4abbec91a9f26f84f7386f2c0f96de669b0eb Mon Sep 17 00:00:00 2001
From: Martin Schwidefsky <[email protected]>
Date: Wed, 10 Apr 2019 15:48:43 +0200
Subject: [PATCH] s390: fine-tune stack switch helper
The CALL_ON_STACK helper currently does not work with clang and for
calls without arguments it does not initialize r2 although the contraint
is "+&d". Rework the CALL_FMT_x and the CALL_ON_STACK macros to work
with clang and produce optimal code in all cases.
Reported-by: Arnd Bergmann <[email protected]>
Signed-off-by: Martin Schwidefsky <[email protected]>
---
arch/s390/include/asm/processor.h | 18 +++++++++---------
1 file changed, 9 insertions(+), 9 deletions(-)
diff --git a/arch/s390/include/asm/processor.h b/arch/s390/include/asm/processor.h
index 81038ab357ce..0ee022247580 100644
--- a/arch/s390/include/asm/processor.h
+++ b/arch/s390/include/asm/processor.h
@@ -261,12 +261,12 @@ static __no_kasan_or_inline unsigned short stap(void)
CALL_ARGS_4(arg1, arg2, arg3, arg4); \
register unsigned long r4 asm("6") = (unsigned long)(arg5)
-#define CALL_FMT_0
-#define CALL_FMT_1 CALL_FMT_0, "0" (r2)
-#define CALL_FMT_2 CALL_FMT_1, "d" (r3)
-#define CALL_FMT_3 CALL_FMT_2, "d" (r4)
-#define CALL_FMT_4 CALL_FMT_3, "d" (r5)
-#define CALL_FMT_5 CALL_FMT_4, "d" (r6)
+#define CALL_FMT_0 "=&d" (r2) :
+#define CALL_FMT_1 "+&d" (r2) :
+#define CALL_FMT_2 CALL_FMT_1 "d" (r3),
+#define CALL_FMT_3 CALL_FMT_2 "d" (r4),
+#define CALL_FMT_4 CALL_FMT_3 "d" (r5),
+#define CALL_FMT_5 CALL_FMT_4 "d" (r6),
#define CALL_CLOBBER_5 "0", "1", "14", "cc", "memory"
#define CALL_CLOBBER_4 CALL_CLOBBER_5
@@ -286,10 +286,10 @@ static __no_kasan_or_inline unsigned short stap(void)
" stg %[_prev],%[_bc](15)\n" \
" brasl 14,%[_fn]\n" \
" la 15,0(%[_prev])\n" \
- : "+&d" (r2), [_prev] "=&a" (prev) \
- : [_stack] "a" (stack), \
+ : [_prev] "=&a" (prev), CALL_FMT_##nr \
+ [_stack] "a" (stack), \
[_bc] "i" (offsetof(struct stack_frame, back_chain)), \
- [_fn] "X" (fn) CALL_FMT_##nr : CALL_CLOBBER_##nr); \
+ [_fn] "X" (fn) : CALL_CLOBBER_##nr); \
r2; \
})
--
2.16.4
--
blue skies,
Martin.
"Reality continues to ruin my life." - Calvin.
On Mon, 8 Apr 2019 23:26:14 +0200
Arnd Bergmann <[email protected]> wrote:
> This was added as a workaround for really old compilers, and it prevents
> building with clang now. I can see no reason for keeping it, as it has
> already been removed for most architectures in the pre-git era, so
> let's remove it everywhere, rather than only for clang.
>
> Signed-off-by: Arnd Bergmann <[email protected]>
> ---
> arch/s390/Makefile | 2 +-
> 1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/arch/s390/Makefile b/arch/s390/Makefile
> index 86c76b149cc2..0087d11e3eaf 100644
> --- a/arch/s390/Makefile
> +++ b/arch/s390/Makefile
> @@ -114,7 +114,7 @@ endif
> cfi := $(call as-instr,.cfi_startproc\n.cfi_val_offset 15$(comma)-160\n.cfi_endproc,-DCONFIG_AS_CFI_VAL_OFFSET=1)
>
> KBUILD_CFLAGS += -mbackchain -msoft-float $(cflags-y)
> -KBUILD_CFLAGS += -pipe -fno-strength-reduce -Wno-sign-compare
> +KBUILD_CFLAGS += -pipe -Wno-sign-compare
> KBUILD_CFLAGS += -fno-asynchronous-unwind-tables $(cfi)
> KBUILD_AFLAGS += $(aflags-y) $(cfi)
> export KBUILD_AFLAGS_DECOMPRESSOR
Added to s390/linux:features for the next merge window. Thanks.
--
blue skies,
Martin.
"Reality continues to ruin my life." - Calvin.
On Mon, 8 Apr 2019 23:26:17 +0200
Arnd Bergmann <[email protected]> wrote:
> clang produces a harmless warning for each use for the qeth_adp_supported
> macro:
>
> drivers/s390/net/qeth_l2_main.c:559:31: warning: implicit conversion from enumeration type 'enum qeth_ipa_setadp_cmd' to
> different enumeration type 'enum qeth_ipa_funcs' [-Wenum-conversion]
> if (qeth_adp_supported(card, IPA_SETADP_SET_PROMISC_MODE))
> ~~~~~~~~~~~~~~~~~~~~~~~~~^~~~~~~~~~~~~~~~~~~~~~~~~~~~
> drivers/s390/net/qeth_core.h:179:41: note: expanded from macro 'qeth_adp_supported'
> qeth_is_ipa_supported(&c->options.adp, f)
> ~~~~~~~~~~~~~~~~~~~~~ ^
>
> Add a version of this macro that uses the correct types, and
> remove the unused qeth_adp_enabled() macro that has the same
> problem.
>
> Signed-off-by: Arnd Bergmann <[email protected]>
I have added this to our internal tree for Julian to pick up.
--
blue skies,
Martin.
"Reality continues to ruin my life." - Calvin.
On Tue, 9 Apr 2019 11:54:30 +0200
Harald Freudenberger <[email protected]> wrote:
> On 08.04.19 23:26, Arnd Bergmann wrote:
> > The 'func_code' variable gets printed in debug statements without
> > a prior initialization in multiple functions, as reported when building
> > with clang:
> >
> > drivers/s390/crypto/zcrypt_api.c:659:6: warning: variable 'func_code' is used uninitialized whenever 'if' condition is true
> > [-Wsometimes-uninitialized]
> > if (mex->outputdatalength < mex->inputdatalength) {
> > ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
> > drivers/s390/crypto/zcrypt_api.c:725:29: note: uninitialized use occurs here
> > trace_s390_zcrypt_rep(mex, func_code, rc,
> > ^~~~~~~~~
> > drivers/s390/crypto/zcrypt_api.c:659:2: note: remove the 'if' if its condition is always false
> > if (mex->outputdatalength < mex->inputdatalength) {
> > ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
> > drivers/s390/crypto/zcrypt_api.c:654:24: note: initialize the variable 'func_code' to silence this warning
> > unsigned int func_code;
> > ^
> >
> > Add initializations to all affected code paths to shut up the warning
> > and make the warning output consistent.
> >
> > Signed-off-by: Arnd Bergmann <[email protected]>
> > ---
> > drivers/s390/crypto/zcrypt_api.c | 4 ++++
> > 1 file changed, 4 insertions(+)
> >
> > diff --git a/drivers/s390/crypto/zcrypt_api.c b/drivers/s390/crypto/zcrypt_api.c
> > index eb93c2d27d0a..23472063d9a8 100644
> > --- a/drivers/s390/crypto/zcrypt_api.c
> > +++ b/drivers/s390/crypto/zcrypt_api.c
> > @@ -657,6 +657,7 @@ static long zcrypt_rsa_modexpo(struct ap_perms *perms,
> > trace_s390_zcrypt_req(mex, TP_ICARSAMODEXPO);
> >
> > if (mex->outputdatalength < mex->inputdatalength) {
> > + func_code = -1;
> > rc = -EINVAL;
> > goto out;
> > }
> > @@ -739,6 +740,7 @@ static long zcrypt_rsa_crt(struct ap_perms *perms,
> > trace_s390_zcrypt_req(crt, TP_ICARSACRT);
> >
> > if (crt->outputdatalength < crt->inputdatalength) {
> > + func_code = -1;
> > rc = -EINVAL;
> > goto out;
> > }
> > @@ -946,6 +948,7 @@ static long zcrypt_send_ep11_cprb(struct ap_perms *perms,
> >
> > targets = kcalloc(target_num, sizeof(*targets), GFP_KERNEL);
> > if (!targets) {
> > + func_code = -1;
> > rc = -ENOMEM;
> > goto out;
> > }
> > @@ -953,6 +956,7 @@ static long zcrypt_send_ep11_cprb(struct ap_perms *perms,
> > uptr = (struct ep11_target_dev __force __user *) xcrb->targets;
> > if (copy_from_user(targets, uptr,
> > target_num * sizeof(*targets))) {
> > + func_code = -1;
> > rc = -EFAULT;
> > goto out_free;
> > }
> Thanks Arnd, but as Nathan already wrote, I'd prefer to have the
> variable initialized with 0 instead of -1.
> If you agree with this, I'll rewrite the patch and apply it to our
> internal git and it will appear at kernel org with the next s390 code merge then.
Do we agreement on func_coed=0 for this one ?
--
blue skies,
Martin.
"Reality continues to ruin my life." - Calvin.
On Mon, 8 Apr 2019 23:26:15 +0200
Arnd Bergmann <[email protected]> wrote:
> clang does not support 31 bit object files on s390, so skip
> the 32-bit vdso here, and only build it when using gcc to compile
> the kernel.
>
> Signed-off-by: Arnd Bergmann <[email protected]>
Added to s390/linux:features for the next merge window. Thanks.
--
blue skies,
Martin.
"Reality continues to ruin my life." - Calvin.
On Mon, 8 Apr 2019 23:26:19 +0200
Arnd Bergmann <[email protected]> wrote:
> clang points out that the return code from this function is
> undefined for one of the error paths:
>
> ../drivers/s390/net/ctcm_main.c:1595:7: warning: variable 'result' is used uninitialized whenever 'if' condition is true
> [-Wsometimes-uninitialized]
> if (priv->channel[direction] == NULL) {
> ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
> ../drivers/s390/net/ctcm_main.c:1638:9: note: uninitialized use occurs here
> return result;
> ^~~~~~
> ../drivers/s390/net/ctcm_main.c:1595:3: note: remove the 'if' if its condition is always false
> if (priv->channel[direction] == NULL) {
> ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
> ../drivers/s390/net/ctcm_main.c:1539:12: note: initialize the variable 'result' to silence this warning
> int result;
> ^
>
> Make it return -ENODEV here, as in the related failure cases.
> gcc has a known bug in underreporting some of these warnings
> when it has already eliminated the assignment of the return code
> based on some earlier optimization step.
>
> Signed-off-by: Arnd Bergmann <[email protected]>
Added to our internal tree for Julian to pick up.
--
blue skies,
Martin.
"Reality continues to ruin my life." - Calvin.
On Mon, 8 Apr 2019 23:26:22 +0200
Arnd Bergmann <[email protected]> wrote:
> clang fails to use the %O and %R inline assembly modifiers
> the same way as gcc, leading to build failures with every use
> of __load_psw_mask():
>
> /tmp/nmi-4a9f80.s: Assembler messages:
> /tmp/nmi-4a9f80.s:571: Error: junk at end of line: `+8(160(%r11))'
> /tmp/nmi-4a9f80.s:626: Error: junk at end of line: `+8(160(%r11))'
>
> Replace these with a more conventional way of passing the addresses
> that should work with both clang and gcc.
>
> Signed-off-by: Arnd Bergmann <[email protected]>
Added to s390/linux:features for the next merge window. Thanks.
--
blue skies,
Martin.
"Reality continues to ruin my life." - Calvin.
On Mon, 8 Apr 2019 23:26:24 +0200
Arnd Bergmann <[email protected]> wrote:
> llvm skips an empty .bss section entirely, which makes
> the check fail with an unexpected error:
>
> /tmp/binutils-multi-test/bin/s390x-linux-gnu-objdump: section '.bss' mentioned in a -j option, but not found in any input file
> error: arch/s390/boot/compressed/decompressor.o .bss section is not empty
> ../arch/s390/scripts/Makefile.chkbss:20: recipe for target 'arch/s390/boot/compressed/decompressor.o.chkbss' failed
>
> Change the check so we first see if a .bss section exists
> before trying to read its size.
>
> Signed-off-by: Arnd Bergmann <[email protected]>
Added to s390/linux:features for the next merge window. Thanks.
--
blue skies,
Martin.
"Reality continues to ruin my life." - Calvin.
On Mon, Apr 8, 2019 at 2:27 PM Arnd Bergmann <[email protected]> wrote:
>
> clang does not support 31 bit object files on s390, so skip
> the 32-bit vdso here, and only build it when using gcc to compile
> the kernel.
What's the build failure? Would you mind filing a bug against LLVM's
issue tracker for it, please?
>
> Signed-off-by: Arnd Bergmann <[email protected]>
> ---
> arch/s390/Kconfig | 3 +++
> arch/s390/kernel/Makefile | 2 +-
> arch/s390/kernel/vdso.c | 10 +++++-----
> 3 files changed, 9 insertions(+), 6 deletions(-)
>
> diff --git a/arch/s390/Kconfig b/arch/s390/Kconfig
> index b6e3d0653002..8cd860cba4d1 100644
> --- a/arch/s390/Kconfig
> +++ b/arch/s390/Kconfig
> @@ -388,6 +388,9 @@ config COMPAT
> (and some other stuff like libraries and such) is needed for
> executing 31 bit applications. It is safe to say "Y".
>
> +config COMPAT_VDSO
> + def_bool COMPAT && !CC_IS_CLANG
> +
> config SYSVIPC_COMPAT
> def_bool y if COMPAT && SYSVIPC
>
> diff --git a/arch/s390/kernel/Makefile b/arch/s390/kernel/Makefile
> index 8a62c7f72e1b..1222db6d4ee9 100644
> --- a/arch/s390/kernel/Makefile
> +++ b/arch/s390/kernel/Makefile
> @@ -86,7 +86,7 @@ obj-$(CONFIG_TRACEPOINTS) += trace.o
>
> # vdso
> obj-y += vdso64/
> -obj-$(CONFIG_COMPAT) += vdso32/
> +obj-$(CONFIG_COMPAT_VDSO) += vdso32/
>
> chkbss := head64.o early_nobss.o
> include $(srctree)/arch/s390/scripts/Makefile.chkbss
> diff --git a/arch/s390/kernel/vdso.c b/arch/s390/kernel/vdso.c
> index e7920a68a12e..243d8b1185bf 100644
> --- a/arch/s390/kernel/vdso.c
> +++ b/arch/s390/kernel/vdso.c
> @@ -29,7 +29,7 @@
> #include <asm/vdso.h>
> #include <asm/facility.h>
>
> -#ifdef CONFIG_COMPAT
> +#ifdef CONFIG_COMPAT_VDSO
> extern char vdso32_start, vdso32_end;
> static void *vdso32_kbase = &vdso32_start;
> static unsigned int vdso32_pages;
> @@ -55,7 +55,7 @@ static vm_fault_t vdso_fault(const struct vm_special_mapping *sm,
>
> vdso_pagelist = vdso64_pagelist;
> vdso_pages = vdso64_pages;
> -#ifdef CONFIG_COMPAT
> +#ifdef CONFIG_COMPAT_VDSO
> if (vma->vm_mm->context.compat_mm) {
> vdso_pagelist = vdso32_pagelist;
> vdso_pages = vdso32_pages;
> @@ -76,7 +76,7 @@ static int vdso_mremap(const struct vm_special_mapping *sm,
> unsigned long vdso_pages;
>
> vdso_pages = vdso64_pages;
> -#ifdef CONFIG_COMPAT
> +#ifdef CONFIG_COMPAT_VDSO
> if (vma->vm_mm->context.compat_mm)
> vdso_pages = vdso32_pages;
> #endif
> @@ -223,7 +223,7 @@ int arch_setup_additional_pages(struct linux_binprm *bprm, int uses_interp)
> return 0;
>
> vdso_pages = vdso64_pages;
> -#ifdef CONFIG_COMPAT
> +#ifdef CONFIG_COMPAT_VDSO
> mm->context.compat_mm = is_compat_task();
> if (mm->context.compat_mm)
> vdso_pages = vdso32_pages;
> @@ -280,7 +280,7 @@ static int __init vdso_init(void)
> int i;
>
> vdso_init_data(vdso_data);
> -#ifdef CONFIG_COMPAT
> +#ifdef CONFIG_COMPAT_VDSO
> /* Calculate the size of the 32 bit vDSO */
> vdso32_pages = ((&vdso32_end - &vdso32_start
> + PAGE_SIZE - 1) >> PAGE_SHIFT) + 1;
> --
> 2.20.0
>
--
Thanks,
~Nick Desaulniers
On Mon, 8 Apr 2019 23:26:21 +0200
Arnd Bergmann <[email protected]> wrote:
> Building system calls with clang results in a warning
> about an alias from a global function to a static one:
>
> ../fs/namei.c:3847:1: warning: unused function '__se_sys_mkdirat' [-Wunused-function]
> SYSCALL_DEFINE3(mkdirat, int, dfd, const char __user *, pathname, umode_t, mode)
> ^
> ../include/linux/syscalls.h:219:36: note: expanded from macro 'SYSCALL_DEFINE3'
> #define SYSCALL_DEFINE3(name, ...) SYSCALL_DEFINEx(3, _##name, __VA_ARGS__)
> ^
> ../include/linux/syscalls.h:228:2: note: expanded from macro 'SYSCALL_DEFINEx'
> __SYSCALL_DEFINEx(x, sname, __VA_ARGS__)
> ^
> ../arch/s390/include/asm/syscall_wrapper.h:126:18: note: expanded from macro '__SYSCALL_DEFINEx'
> asmlinkage long __se_sys##name(__MAP(x,__SC_LONG,__VA_ARGS__)) \
> ^
> <scratch space>:31:1: note: expanded from here
> __se_sys_mkdirat
> ^
>
> The only reference to the static __se_sys_mkdirat() here is the alias, but
> this only gets evaluated later. Making this function global as well avoids
> the warning.
>
> Signed-off-by: Arnd Bergmann <[email protected]>
Added to s390/linux:features for the next merge window. Thanks.
--
blue skies,
Martin.
"Reality continues to ruin my life." - Calvin.
On Wed, 10 Apr 2019 18:03:57 +0200
Martin Schwidefsky <[email protected]> wrote:
> > --- a/arch/s390/include/asm/ftrace.h
> > +++ b/arch/s390/include/asm/ftrace.h
> > @@ -13,7 +13,12 @@
> >
> > #ifndef __ASSEMBLY__
> >
> > +#ifdef CONFIG_CC_IS_CLANG
> > +/* https://bugs.llvm.org/show_bug.cgi?id=41424 */
> > +#define ftrace_return_address(n) __builtin_return_address(0)
> > +#else
> > #define ftrace_return_address(n) __builtin_return_address(n)
> > +#endif
> >
> > void _mcount(void);
> > void ftrace_caller(void);
>
> I can say I like this one. If the compiler can not do __builtin_return_address(n)
> it feels wrong to just use __builtin_return_address(0).
I agree. The proper return value is 0UL, see include/linux/ftrace.h
/* Archs may use other ways for ADDR1 and beyond */
#ifndef ftrace_return_address
# ifdef CONFIG_FRAME_POINTER
# define ftrace_return_address(n) __builtin_return_address(n)
# else
# define ftrace_return_address(n) 0UL
# endif
#endif
This is why we treat zero differently:
#define CALLER_ADDR0 ((unsigned long)ftrace_return_address0)
#define CALLER_ADDR1 ((unsigned long)ftrace_return_address(1))
#define CALLER_ADDR2 ((unsigned long)ftrace_return_address(2))
#define CALLER_ADDR3 ((unsigned long)ftrace_return_address(3))
#define CALLER_ADDR4 ((unsigned long)ftrace_return_address(4))
#define CALLER_ADDR5 ((unsigned long)ftrace_return_address(5))
#define CALLER_ADDR6 ((unsigned long)ftrace_return_address(6))
-- Steve
On Mon, 8 Apr 2019 23:26:23 +0200
Arnd Bergmann <[email protected]> wrote:
> llvm on s390 has problems with __builtin_return_address(n), with n>0,
> this results in a somewhat cryptic error message:
>
> fatal error: error in backend: Unsupported stack frame traversal count
>
> To work around it, use the direct return address directly. This
> is probably not ideal here, but gets things to compile and should
> only lead to inferior reporting, not to misbehavior of the generated
> code.
>
> Link: https://bugs.llvm.org/show_bug.cgi?id=41424
> Signed-off-by: Arnd Bergmann <[email protected]>
> ---
> arch/s390/include/asm/ftrace.h | 5 +++++
> 1 file changed, 5 insertions(+)
>
> diff --git a/arch/s390/include/asm/ftrace.h b/arch/s390/include/asm/ftrace.h
> index 5a3c95b11952..7923c63946fb 100644
> --- a/arch/s390/include/asm/ftrace.h
> +++ b/arch/s390/include/asm/ftrace.h
> @@ -13,7 +13,12 @@
>
> #ifndef __ASSEMBLY__
>
> +#ifdef CONFIG_CC_IS_CLANG
> +/* https://bugs.llvm.org/show_bug.cgi?id=41424 */
> +#define ftrace_return_address(n) __builtin_return_address(0)
> +#else
> #define ftrace_return_address(n) __builtin_return_address(n)
> +#endif
>
> void _mcount(void);
> void ftrace_caller(void);
I can say I like this one. If the compiler can not do __builtin_return_address(n)
it feels wrong to just use __builtin_return_address(0).
--
blue skies,
Martin.
"Reality continues to ruin my life." - Calvin.
On Wed, Apr 10, 2019 at 6:55 AM Martin Schwidefsky
<[email protected]> wrote:
>
> On Mon, 8 Apr 2019 23:26:25 +0200
> Arnd Bergmann <[email protected]> wrote:
>
> > clang does not understand the contraint "0" in the CALL_ON_STACK()
> > macro:
> >
> > ../arch/s390/mm/maccess.c:117:10: error: invalid input constraint '0' in asm
> > return CALL_ON_STACK(_memcpy_real, S390_lowcore.nodat_stack,
> > ^
> > ../arch/s390/include/asm/processor.h:292:20: note: expanded from macro 'CALL_ON_STACK'
> > [_fn] "X" (fn) CALL_FMT_##nr : CALL_CLOBBER_##nr); \
> > ^
> > <scratch space>:207:1: note: expanded from here
> > CALL_FMT_3
> > ^
> > ../arch/s390/include/asm/processor.h:267:20: note: expanded from macro 'CALL_FMT_3'
> > #define CALL_FMT_3 CALL_FMT_2, "d" (r4)
> > ^
> > ../arch/s390/include/asm/processor.h:266:20: note: expanded from macro 'CALL_FMT_2'
> > #define CALL_FMT_2 CALL_FMT_1, "d" (r3)
> > ^
> > ../arch/s390/include/asm/processor.h:265:32: note: expanded from macro 'CALL_FMT_1'
> > #define CALL_FMT_1 CALL_FMT_0, "0" (r2)
> > ^
> >
> > I don't know what the correct fix here would be, changing it to "d" made
> > it build, since clang does understand this one.
> >
> > Signed-off-by: Arnd Bergmann <[email protected]>
> > ---
> > arch/s390/include/asm/processor.h | 2 +-
> > 1 file changed, 1 insertion(+), 1 deletion(-)
> >
> > diff --git a/arch/s390/include/asm/processor.h b/arch/s390/include/asm/processor.h
> > index 700c650ffd4f..84c59c99668a 100644
> > --- a/arch/s390/include/asm/processor.h
> > +++ b/arch/s390/include/asm/processor.h
> > @@ -262,7 +262,7 @@ static __no_kasan_or_inline unsigned short stap(void)
> > register unsigned long r4 asm("6") = (unsigned long)(arg5)
> >
> > #define CALL_FMT_0
> > -#define CALL_FMT_1 CALL_FMT_0, "0" (r2)
> > +#define CALL_FMT_1 CALL_FMT_0, "d" (r2)
> > #define CALL_FMT_2 CALL_FMT_1, "d" (r3)
> > #define CALL_FMT_3 CALL_FMT_2, "d" (r4)
> > #define CALL_FMT_4 CALL_FMT_3, "d" (r5)
>
> This is (slightly) wrong. %r2 is used as the input register for the first argument
> and the result value for the call. With your patch you force the compiler to load
> the first argument in two registers. One solution would be to CALL_FMT1 as
>
> #define CALL_FMT1 CALL_FMT_0
>
> It still is not optimal though as for CALL_FMT_0 the "+&d" (r2) indicates an
> input but CALL_ARGS_0 does not initialize r2.
>
> I am thinking about the following patch to cover all cases:
> --
> From 91a4abbec91a9f26f84f7386f2c0f96de669b0eb Mon Sep 17 00:00:00 2001
> From: Martin Schwidefsky <[email protected]>
> Date: Wed, 10 Apr 2019 15:48:43 +0200
> Subject: [PATCH] s390: fine-tune stack switch helper
Thanks for the patch. Just some minor typos in the commit.
>
> The CALL_ON_STACK helper currently does not work with clang and for
> calls without arguments it does not initialize r2 although the contraint
- calls without arguments it ...
+ calls without arguments. It ...
- although the contraint
+ although the constraint
`:set spell` in vim (I wonder if there's a way to set that when
writing commit messages automatically)
> is "+&d". Rework the CALL_FMT_x and the CALL_ON_STACK macros to work
> with clang and produce optimal code in all cases.
>
> Reported-by: Arnd Bergmann <[email protected]>
> Signed-off-by: Martin Schwidefsky <[email protected]>
> ---
> arch/s390/include/asm/processor.h | 18 +++++++++---------
> 1 file changed, 9 insertions(+), 9 deletions(-)
>
> diff --git a/arch/s390/include/asm/processor.h b/arch/s390/include/asm/processor.h
> index 81038ab357ce..0ee022247580 100644
> --- a/arch/s390/include/asm/processor.h
> +++ b/arch/s390/include/asm/processor.h
> @@ -261,12 +261,12 @@ static __no_kasan_or_inline unsigned short stap(void)
> CALL_ARGS_4(arg1, arg2, arg3, arg4); \
> register unsigned long r4 asm("6") = (unsigned long)(arg5)
>
> -#define CALL_FMT_0
> -#define CALL_FMT_1 CALL_FMT_0, "0" (r2)
> -#define CALL_FMT_2 CALL_FMT_1, "d" (r3)
> -#define CALL_FMT_3 CALL_FMT_2, "d" (r4)
> -#define CALL_FMT_4 CALL_FMT_3, "d" (r5)
> -#define CALL_FMT_5 CALL_FMT_4, "d" (r6)
> +#define CALL_FMT_0 "=&d" (r2) :
> +#define CALL_FMT_1 "+&d" (r2) :
> +#define CALL_FMT_2 CALL_FMT_1 "d" (r3),
> +#define CALL_FMT_3 CALL_FMT_2 "d" (r4),
> +#define CALL_FMT_4 CALL_FMT_3 "d" (r5),
> +#define CALL_FMT_5 CALL_FMT_4 "d" (r6),
>
> #define CALL_CLOBBER_5 "0", "1", "14", "cc", "memory"
> #define CALL_CLOBBER_4 CALL_CLOBBER_5
> @@ -286,10 +286,10 @@ static __no_kasan_or_inline unsigned short stap(void)
> " stg %[_prev],%[_bc](15)\n" \
> " brasl 14,%[_fn]\n" \
> " la 15,0(%[_prev])\n" \
> - : "+&d" (r2), [_prev] "=&a" (prev) \
> - : [_stack] "a" (stack), \
> + : [_prev] "=&a" (prev), CALL_FMT_##nr \
> + [_stack] "a" (stack), \
> [_bc] "i" (offsetof(struct stack_frame, back_chain)), \
> - [_fn] "X" (fn) CALL_FMT_##nr : CALL_CLOBBER_##nr); \
> + [_fn] "X" (fn) : CALL_CLOBBER_##nr); \
> r2; \
> })
>
> --
> 2.16.4
> --
> blue skies,
> Martin.
>
> "Reality continues to ruin my life." - Calvin.
>
> --
> You received this message because you are subscribed to the Google Groups "Clang Built Linux" group.
> To unsubscribe from this group and stop receiving emails from it, send an email to [email protected].
> To post to this group, send email to [email protected].
> To view this discussion on the web visit https://groups.google.com/d/msgid/clang-built-linux/20190410155513.1609f1a1%40mschwideX1.
> For more options, visit https://groups.google.com/d/optout.
--
Thanks,
~Nick Desaulniers
On Wed, Apr 10, 2019 at 3:55 PM Martin Schwidefsky
<[email protected]> wrote:
>
> On Mon, 8 Apr 2019 23:26:25 +0200
> Arnd Bergmann <[email protected]> wrote:
>
> > diff --git a/arch/s390/include/asm/processor.h b/arch/s390/include/asm/processor.h
> > index 700c650ffd4f..84c59c99668a 100644
> > --- a/arch/s390/include/asm/processor.h
> > +++ b/arch/s390/include/asm/processor.h
> > @@ -262,7 +262,7 @@ static __no_kasan_or_inline unsigned short stap(void)
> > register unsigned long r4 asm("6") = (unsigned long)(arg5)
> >
> > #define CALL_FMT_0
> > -#define CALL_FMT_1 CALL_FMT_0, "0" (r2)
> > +#define CALL_FMT_1 CALL_FMT_0, "d" (r2)
> > #define CALL_FMT_2 CALL_FMT_1, "d" (r3)
> > #define CALL_FMT_3 CALL_FMT_2, "d" (r4)
> > #define CALL_FMT_4 CALL_FMT_3, "d" (r5)
>
> This is (slightly) wrong. %r2 is used as the input register for the first argument
> and the result value for the call. With your patch you force the compiler to load
> the first argument in two registers. One solution would be to CALL_FMT1 as
>
> #define CALL_FMT1 CALL_FMT_0
>
> It still is not optimal though as for CALL_FMT_0 the "+&d" (r2) indicates an
> input but CALL_ARGS_0 does not initialize r2.
Ok, thanks for taking a closer look!
> I am thinking about the following patch to cover all cases:
> --
> From 91a4abbec91a9f26f84f7386f2c0f96de669b0eb Mon Sep 17 00:00:00 2001
> From: Martin Schwidefsky <[email protected]>
> Date: Wed, 10 Apr 2019 15:48:43 +0200
> Subject: [PATCH] s390: fine-tune stack switch helper
>
> The CALL_ON_STACK helper currently does not work with clang and for
> calls without arguments it does not initialize r2 although the contraint
> is "+&d". Rework the CALL_FMT_x and the CALL_ON_STACK macros to work
> with clang and produce optimal code in all cases.
>
> Reported-by: Arnd Bergmann <[email protected]>
> Signed-off-by: Martin Schwidefsky <[email protected]>
I did another build test to confirm that your patch works fine with clang
as well, looks good to me.
Arnd
On Wed, Apr 10, 2019 at 5:59 PM Martin Schwidefsky
<[email protected]> wrote:
> On Tue, 9 Apr 2019 11:54:30 +0200 Harald Freudenberger <[email protected]> wrote:
> > On 08.04.19 23:26, Arnd Bergmann wrote:
> > > }
> > Thanks Arnd, but as Nathan already wrote, I'd prefer to have the
> > variable initialized with 0 instead of -1.
> > If you agree with this, I'll rewrite the patch and apply it to our
> > internal git and it will appear at kernel org with the next s390 code merge then.
>
> Do we agreement on func_coed=0 for this one ?
Yes, I think that was the consensus.
Arnd
On Wed, Apr 10, 2019 at 6:26 PM 'Nick Desaulniers' via Clang Built
Linux <[email protected]> wrote:
>
> On Mon, Apr 8, 2019 at 2:27 PM Arnd Bergmann <[email protected]> wrote:
> >
> > clang does not support 31 bit object files on s390, so skip
> > the 32-bit vdso here, and only build it when using gcc to compile
> > the kernel.
>
> What's the build failure? Would you mind filing a bug against LLVM's
> issue tracker for it, please?
As far as I can tell, llvm does only supports 64-bit output for s390, so this
is not a bug but rather a missing feature that seems highly unlikely to
ever get added.
32-bit (31-bit) mode on s390 is only used for very old existing binaries,
and the vdso support is optional there.
Arnd
On Wed, Apr 10, 2019 at 6:14 PM Steven Rostedt <[email protected]> wrote:
> On Wed, 10 Apr 2019 18:03:57 +0200 Martin Schwidefsky <[email protected]> wrote:
>
> > > --- a/arch/s390/include/asm/ftrace.h
> > > +++ b/arch/s390/include/asm/ftrace.h
> > > @@ -13,7 +13,12 @@
> > >
> > > #ifndef __ASSEMBLY__
> > >
> > > +#ifdef CONFIG_CC_IS_CLANG
> > > +/* https://bugs.llvm.org/show_bug.cgi?id=41424 */
> > > +#define ftrace_return_address(n) __builtin_return_address(0)
> > > +#else
> > > #define ftrace_return_address(n) __builtin_return_address(n)
> > > +#endif
> > >
> > > void _mcount(void);
> > > void ftrace_caller(void);
> >
> > I can say I like this one. If the compiler can not do __builtin_return_address(n)
> > it feels wrong to just use __builtin_return_address(0).
>
> I agree. The proper return value is 0UL, see include/linux/ftrace.h
>
> /* Archs may use other ways for ADDR1 and beyond */
> #ifndef ftrace_return_address
> # ifdef CONFIG_FRAME_POINTER
> # define ftrace_return_address(n) __builtin_return_address(n)
> # else
> # define ftrace_return_address(n) 0UL
> # endif
> #endif
>
> This is why we treat zero differently:
>
> #define CALLER_ADDR0 ((unsigned long)ftrace_return_address0)
> #define CALLER_ADDR1 ((unsigned long)ftrace_return_address(1))
> #define CALLER_ADDR2 ((unsigned long)ftrace_return_address(2))
> #define CALLER_ADDR3 ((unsigned long)ftrace_return_address(3))
> #define CALLER_ADDR4 ((unsigned long)ftrace_return_address(4))
> #define CALLER_ADDR5 ((unsigned long)ftrace_return_address(5))
> #define CALLER_ADDR6 ((unsigned long)ftrace_return_address(6))
Right, got it.
Martin, do you want me to send a replacement patch, or can you
commit the patch with
#ifdef CONFIG_CC_IS_CLANG
/* https://bugs.llvm.org/show_bug.cgi?id=41424 */
#define ftrace_return_address(n) 0UL
#else
#define ftrace_return_address(n) __builtin_return_address(n)
#endif
instead?
Arnd
On Wed, 10 Apr 2019 21:07:56 +0200
Arnd Bergmann <[email protected]> wrote:
> On Wed, Apr 10, 2019 at 6:14 PM Steven Rostedt <[email protected]> wrote:
> > On Wed, 10 Apr 2019 18:03:57 +0200 Martin Schwidefsky <[email protected]> wrote:
> >
> > > > --- a/arch/s390/include/asm/ftrace.h
> > > > +++ b/arch/s390/include/asm/ftrace.h
> > > > @@ -13,7 +13,12 @@
> > > >
> > > > #ifndef __ASSEMBLY__
> > > >
> > > > +#ifdef CONFIG_CC_IS_CLANG
> > > > +/* https://bugs.llvm.org/show_bug.cgi?id=41424 */
> > > > +#define ftrace_return_address(n) __builtin_return_address(0)
> > > > +#else
> > > > #define ftrace_return_address(n) __builtin_return_address(n)
> > > > +#endif
> > > >
> > > > void _mcount(void);
> > > > void ftrace_caller(void);
> > >
> > > I can say I like this one. If the compiler can not do __builtin_return_address(n)
> > > it feels wrong to just use __builtin_return_address(0).
> >
> > I agree. The proper return value is 0UL, see include/linux/ftrace.h
> >
> > /* Archs may use other ways for ADDR1 and beyond */
> > #ifndef ftrace_return_address
> > # ifdef CONFIG_FRAME_POINTER
> > # define ftrace_return_address(n) __builtin_return_address(n)
> > # else
> > # define ftrace_return_address(n) 0UL
> > # endif
> > #endif
> >
> > This is why we treat zero differently:
> >
> > #define CALLER_ADDR0 ((unsigned long)ftrace_return_address0)
> > #define CALLER_ADDR1 ((unsigned long)ftrace_return_address(1))
> > #define CALLER_ADDR2 ((unsigned long)ftrace_return_address(2))
> > #define CALLER_ADDR3 ((unsigned long)ftrace_return_address(3))
> > #define CALLER_ADDR4 ((unsigned long)ftrace_return_address(4))
> > #define CALLER_ADDR5 ((unsigned long)ftrace_return_address(5))
> > #define CALLER_ADDR6 ((unsigned long)ftrace_return_address(6))
>
> Right, got it.
>
> Martin, do you want me to send a replacement patch, or can you
> commit the patch with
>
> #ifdef CONFIG_CC_IS_CLANG
> /* https://bugs.llvm.org/show_bug.cgi?id=41424 */
> #define ftrace_return_address(n) 0UL
> #else
> #define ftrace_return_address(n) __builtin_return_address(n)
> #endif
>
> instead?
Ok, done.
--
blue skies,
Martin.
"Reality continues to ruin my life." - Calvin.
On Wed, 10 Apr 2019 20:57:21 +0200
Arnd Bergmann <[email protected]> wrote:
> On Wed, Apr 10, 2019 at 5:59 PM Martin Schwidefsky
> <[email protected]> wrote:
> > On Tue, 9 Apr 2019 11:54:30 +0200 Harald Freudenberger <[email protected]> wrote:
> > > On 08.04.19 23:26, Arnd Bergmann wrote:
> > > > }
> > > Thanks Arnd, but as Nathan already wrote, I'd prefer to have the
> > > variable initialized with 0 instead of -1.
> > > If you agree with this, I'll rewrite the patch and apply it to our
> > > internal git and it will appear at kernel org with the next s390 code merge then.
> >
> > Do we agreement on func_coed=0 for this one ?
>
> Yes, I think that was the consensus.
>
> Arnd
>
Ok, committed with func_code=0.
--
blue skies,
Martin.
"Reality continues to ruin my life." - Calvin.