A couple of patches which in result make it finally possible to build the
kernel for s390 with llvm's integrated assembler. Several configs build
without errors or warnings, and the kernel also works as expected.
Note that patch 6 ("s390/boot: workaround llvm IAS bug") reveals a
miscompile. This looks like a bug in the instruction definitions of the mvc
and clc instructions(?). I'd like to ask people to look into this, since
this silently generated broken code.
This patch series is based on linux-next, which contains two additional
required s390 specific patches to make llvm's IAS work.
Thanks,
Heiko
Heiko Carstens (8):
s390/alternatives: provide identical sized orginal/alternative sequences
s390/alternatives: remove padding generation code
s390/entry: shorten OUTSIDE macro
s390/entry: workaround llvm's IAS limitations
s390/purgatory: workaround llvm's IAS limitations
s390/boot: workaround llvm IAS bug
s390/boot: do not emit debug info for assembly with llvm's IAS
scripts/min-tool-version.sh: raise minimum clang version to 14.0.0 for s390
arch/s390/Makefile | 2 +
arch/s390/boot/head.S | 34 +++++----
arch/s390/include/asm/alternative-asm.h | 76 +++-----------------
arch/s390/include/asm/alternative.h | 93 ++++++-------------------
arch/s390/include/asm/spinlock.h | 2 +-
arch/s390/kernel/alternative.c | 61 +---------------
arch/s390/kernel/entry.S | 39 +++++++----
arch/s390/lib/spinlock.c | 4 +-
arch/s390/purgatory/head.S | 29 ++++++--
scripts/min-tool-version.sh | 3 +-
10 files changed, 104 insertions(+), 239 deletions(-)
--
2.32.0
For at least the mvc and clc instructions llvm's integrated assembler can
generate incorrect code. In particular this happens with decompressor boot
code. The reason seems to be that relocations for the second displacement
of each instruction are at incorrect locations (-/+: gas vs llvm IAS):
mvc __LC_IO_NEW_PSW(16),.Lnewpsw
results in
4: d2 0f 01 f0 00 00 mvc 496(16,%r0),0
- 8: R_390_12 .head.text+0x10
+ 6: R_390_12 .head.text+0x10
and
clc 0(3,%r4),.L_hdr
results in
258: d5 02 40 00 00 00 clc 0(3,%r4),0
- 25c: R_390_12 .head.text+0x324
+ 25a: R_390_12 .head.text+0x324
Workaround this by writing the code in a different way.
Signed-off-by: Heiko Carstens <[email protected]>
---
arch/s390/boot/head.S | 34 +++++++++++++++++++++-------------
1 file changed, 21 insertions(+), 13 deletions(-)
diff --git a/arch/s390/boot/head.S b/arch/s390/boot/head.S
index 2ced90172680..8402e1cd133b 100644
--- a/arch/s390/boot/head.S
+++ b/arch/s390/boot/head.S
@@ -42,7 +42,8 @@ ipl_start:
# subroutine to wait for end I/O
#
.Lirqwait:
- mvc __LC_IO_NEW_PSW(16),.Lnewpsw # set up IO interrupt psw
+ larl %r13,.Lnewpsw # set up IO interrupt psw
+ mvc __LC_IO_NEW_PSW(16),0(%r13)
lpsw .Lwaitpsw
.Lioint:
br %r14
@@ -155,9 +156,11 @@ ipl_start:
lr %r2,%r3
.Lnotrunc:
l %r4,.Linitrd
- clc 0(3,%r4),.L_hdr # if it is HDRx
+ larl %r13,.L_hdr
+ clc 0(3,%r4),0(%r13) # if it is HDRx
bz .Lagain1 # skip dataset header
- clc 0(3,%r4),.L_eof # if it is EOFx
+ larl %r13,.L_eof
+ clc 0(3,%r4),0(%r13) # if it is EOFx
bz .Lagain1 # skip dateset trailer
lr %r5,%r2
@@ -181,9 +184,11 @@ ipl_start:
.Lrdcont:
l %r2,.Linitrd
- clc 0(3,%r2),.L_hdr # skip HDRx and EOFx
+ larl %r13,.L_hdr # skip HDRx and EOFx
+ clc 0(3,%r2),0(%r13)
bz .Lagain2
- clc 0(3,%r2),.L_eof
+ larl %r13,.L_eof
+ clc 0(3,%r2),0(%r13)
bz .Lagain2
#
@@ -260,20 +265,23 @@ SYM_CODE_START_LOCAL(startup_normal)
.fill 16,4,0x0
0: lmh %r0,%r15,0(%r13) # clear high-order half of gprs
sam64 # switch to 64 bit addressing mode
- basr %r13,0 # get base
-.LPG0:
- mvc __LC_EXT_NEW_PSW(16),.Lext_new_psw-.LPG0(%r13)
- mvc __LC_PGM_NEW_PSW(16),.Lpgm_new_psw-.LPG0(%r13)
- mvc __LC_IO_NEW_PSW(16),.Lio_new_psw-.LPG0(%r13)
+ larl %r13,.Lext_new_psw
+ mvc __LC_EXT_NEW_PSW(16),0(%r13)
+ larl %r13,.Lpgm_new_psw
+ mvc __LC_PGM_NEW_PSW(16),0(%r13)
+ larl %r13,.Lio_new_psw
+ mvc __LC_IO_NEW_PSW(16),0(%r13)
xc 0x200(256),0x200 # partially clear lowcore
xc 0x300(256),0x300
xc 0xe00(256),0xe00
xc 0xf00(256),0xf00
- lctlg %c0,%c15,.Lctl-.LPG0(%r13) # load control registers
+ larl %r13,.Lctl
+ lctlg %c0,%c15,0(%r13) # load control registers
stcke __LC_BOOT_CLOCK
mvc __LC_LAST_UPDATE_CLOCK(8),__LC_BOOT_CLOCK+1
- spt 6f-.LPG0(%r13)
- mvc __LC_LAST_UPDATE_TIMER(8),6f-.LPG0(%r13)
+ larl %r13,6f
+ spt 0(%r13)
+ mvc __LC_LAST_UPDATE_TIMER(8),0(%r13)
larl %r15,_stack_end-STACK_FRAME_OVERHEAD
brasl %r14,sclp_early_setup_buffer
brasl %r14,verify_facilities
--
2.32.0
llvm's integrated assembler cannot handle immediate values which are
calculated with two local labels:
<instantiation>:3:13: error: invalid operand for instruction
clgfi %r14,.Lsie_done - .Lsie_gmap
Workaround this by adding clang specific code which reads the specific
value from memory. Since this code is within the hot paths of the kernel
and adds an additional memory reference, keep the original code, and add
ifdef'ed code.
Signed-off-by: Heiko Carstens <[email protected]>
---
arch/s390/kernel/entry.S | 11 +++++++++++
1 file changed, 11 insertions(+)
diff --git a/arch/s390/kernel/entry.S b/arch/s390/kernel/entry.S
index e1664b45090f..ff7a75078e93 100644
--- a/arch/s390/kernel/entry.S
+++ b/arch/s390/kernel/entry.S
@@ -171,8 +171,19 @@ _LPP_OFFSET = __LC_LPP
.macro OUTSIDE reg,start,end,outside_label
larl %r14,\start
slgrk %r14,\reg,%r14
+#ifdef CONFIG_CC_IS_CLANG
+ clgfrl %r14,.Lrange_size\@
+#else
clgfi %r14,\end - \start
+#endif
jhe \outside_label
+#ifdef CONFIG_CC_IS_CLANG
+ .section .rodata, "a"
+ .align 4
+.Lrange_size\@:
+ .long \end - \start
+ .previous
+#endif
.endm
.macro SIEEXIT
--
2.32.0
clang fails to handle ".if" statements in inline assembly which are heavily
used in the alternatives code.
To work around this remove this code, and enforce that users of
alternatives must specify original and alternative instruction sequences
which have identical sizes. Add a compile time check with two ".org"
statements similar to arm64.
In result not only clang can handle this, but also quite a lot of code can
be removed.
Signed-off-by: Heiko Carstens <[email protected]>
---
arch/s390/include/asm/alternative-asm.h | 76 +++-----------------
arch/s390/include/asm/alternative.h | 93 ++++++-------------------
arch/s390/kernel/alternative.c | 61 +---------------
3 files changed, 31 insertions(+), 199 deletions(-)
diff --git a/arch/s390/include/asm/alternative-asm.h b/arch/s390/include/asm/alternative-asm.h
index bb3837d7387c..7db046596b93 100644
--- a/arch/s390/include/asm/alternative-asm.h
+++ b/arch/s390/include/asm/alternative-asm.h
@@ -4,19 +4,6 @@
#ifdef __ASSEMBLY__
-/*
- * Check the length of an instruction sequence. The length may not be larger
- * than 254 bytes and it has to be divisible by 2.
- */
-.macro alt_len_check start,end
- .if ( \end - \start ) > 254
- .error "cpu alternatives does not support instructions blocks > 254 bytes\n"
- .endif
- .if ( \end - \start ) % 2
- .error "cpu alternatives instructions length is odd\n"
- .endif
-.endm
-
/*
* Issue one struct alt_instr descriptor entry (need to put it into
* the section .altinstructions, see below). This entry contains
@@ -28,66 +15,29 @@
.long \alt_start - .
.word \feature
.byte \orig_end - \orig_start
- .byte \alt_end - \alt_start
-.endm
-
-/*
- * Fill up @bytes with nops. The macro emits 6-byte nop instructions
- * for the bulk of the area, possibly followed by a 4-byte and/or
- * a 2-byte nop if the size of the area is not divisible by 6.
- */
-.macro alt_pad_fill bytes
- .rept ( \bytes ) / 6
- brcl 0,0
- .endr
- .rept ( \bytes ) % 6 / 4
- nop
- .endr
- .rept ( \bytes ) % 6 % 4 / 2
- nopr
- .endr
-.endm
-
-/*
- * Fill up @bytes with nops. If the number of bytes is larger
- * than 6, emit a jg instruction to branch over all nops, then
- * fill an area of size (@bytes - 6) with nop instructions.
- */
-.macro alt_pad bytes
- .if ( \bytes > 0 )
- .if ( \bytes > 6 )
- jg . + \bytes
- alt_pad_fill \bytes - 6
- .else
- alt_pad_fill \bytes
- .endif
- .endif
+ .org . - ( \orig_end - \orig_start ) + ( \alt_end - \alt_start )
+ .org . - ( \alt_end - \alt_start ) + ( \orig_end - \orig_start )
.endm
/*
* Define an alternative between two instructions. If @feature is
* present, early code in apply_alternatives() replaces @oldinstr with
- * @newinstr. ".skip" directive takes care of proper instruction padding
- * in case @newinstr is longer than @oldinstr.
+ * @newinstr.
*/
.macro ALTERNATIVE oldinstr, newinstr, feature
.pushsection .altinstr_replacement,"ax"
770: \newinstr
771: .popsection
772: \oldinstr
-773: alt_len_check 770b, 771b
- alt_len_check 772b, 773b
- alt_pad ( ( 771b - 770b ) - ( 773b - 772b ) )
-774: .pushsection .altinstructions,"a"
- alt_entry 772b, 774b, 770b, 771b, \feature
+773: .pushsection .altinstructions,"a"
+ alt_entry 772b, 773b, 770b, 771b, \feature
.popsection
.endm
/*
* Define an alternative between two instructions. If @feature is
* present, early code in apply_alternatives() replaces @oldinstr with
- * @newinstr. ".skip" directive takes care of proper instruction padding
- * in case @newinstr is longer than @oldinstr.
+ * @newinstr.
*/
.macro ALTERNATIVE_2 oldinstr, newinstr1, feature1, newinstr2, feature2
.pushsection .altinstr_replacement,"ax"
@@ -95,17 +45,9 @@
771: \newinstr2
772: .popsection
773: \oldinstr
-774: alt_len_check 770b, 771b
- alt_len_check 771b, 772b
- alt_len_check 773b, 774b
- .if ( 771b - 770b > 772b - 771b )
- alt_pad ( ( 771b - 770b ) - ( 774b - 773b ) )
- .else
- alt_pad ( ( 772b - 771b ) - ( 774b - 773b ) )
- .endif
-775: .pushsection .altinstructions,"a"
- alt_entry 773b, 775b, 770b, 771b,\feature1
- alt_entry 773b, 775b, 771b, 772b,\feature2
+774: .pushsection .altinstructions,"a"
+ alt_entry 773b, 774b, 770b, 771b,\feature1
+ alt_entry 773b, 774b, 771b, 772b,\feature2
.popsection
.endm
diff --git a/arch/s390/include/asm/alternative.h b/arch/s390/include/asm/alternative.h
index 3f2856ed6808..904dd049f954 100644
--- a/arch/s390/include/asm/alternative.h
+++ b/arch/s390/include/asm/alternative.h
@@ -13,32 +13,25 @@ struct alt_instr {
s32 repl_offset; /* offset to replacement instruction */
u16 facility; /* facility bit set for replacement */
u8 instrlen; /* length of original instruction */
- u8 replacementlen; /* length of new instruction */
} __packed;
void apply_alternative_instructions(void);
void apply_alternatives(struct alt_instr *start, struct alt_instr *end);
/*
- * |661: |662: |6620 |663:
- * +-----------+---------------------+
- * | oldinstr | oldinstr_padding |
- * | +----------+----------+
- * | | | |
- * | | >6 bytes |6/4/2 nops|
- * | |6 bytes jg----------->
- * +-----------+---------------------+
- * ^^ static padding ^^
+ * +---------------------------------+
+ * |661: |662:
+ * | oldinstr |
+ * +---------------------------------+
*
* .altinstr_replacement section
- * +---------------------+-----------+
+ * +---------------------------------+
* |6641: |6651:
* | alternative instr 1 |
- * +-----------+---------+- - - - - -+
- * |6642: |6652: |
- * | alternative instr 2 | padding
- * +---------------------+- - - - - -+
- * ^ runtime ^
+ * +---------------------------------+
+ * |6642: |6652:
+ * | alternative instr 2 |
+ * +---------------------------------+
*
* .altinstructions section
* +---------------------------------+
@@ -47,77 +40,31 @@ void apply_alternatives(struct alt_instr *start, struct alt_instr *end);
* +---------------------------------+
*/
-#define b_altinstr(num) "664"#num
-#define e_altinstr(num) "665"#num
-
-#define e_oldinstr_pad_end "663"
+#define b_altinstr(num) "664"#num
+#define e_altinstr(num) "665"#num
#define oldinstr_len "662b-661b"
-#define oldinstr_total_len e_oldinstr_pad_end"b-661b"
#define altinstr_len(num) e_altinstr(num)"b-"b_altinstr(num)"b"
-#define oldinstr_pad_len(num) \
- "-(((" altinstr_len(num) ")-(" oldinstr_len ")) > 0) * " \
- "((" altinstr_len(num) ")-(" oldinstr_len "))"
-
-#define INSTR_LEN_SANITY_CHECK(len) \
- ".if " len " > 254\n" \
- "\t.error \"cpu alternatives does not support instructions " \
- "blocks > 254 bytes\"\n" \
- ".endif\n" \
- ".if (" len ") %% 2\n" \
- "\t.error \"cpu alternatives instructions length is odd\"\n" \
- ".endif\n"
-
-#define OLDINSTR_PADDING(oldinstr, num) \
- ".if " oldinstr_pad_len(num) " > 6\n" \
- "\tjg " e_oldinstr_pad_end "f\n" \
- "6620:\n" \
- "\t.rept (" oldinstr_pad_len(num) " - (6620b-662b)) / 2\n" \
- "\tnopr\n" \
- ".else\n" \
- "\t.rept " oldinstr_pad_len(num) " / 6\n" \
- "\t.brcl 0,0\n" \
- "\t.endr\n" \
- "\t.rept " oldinstr_pad_len(num) " %% 6 / 4\n" \
- "\tnop\n" \
- "\t.endr\n" \
- "\t.rept " oldinstr_pad_len(num) " %% 6 %% 4 / 2\n" \
- "\tnopr\n" \
- ".endr\n" \
- ".endif\n"
-
-#define OLDINSTR(oldinstr, num) \
- "661:\n\t" oldinstr "\n662:\n" \
- OLDINSTR_PADDING(oldinstr, num) \
- e_oldinstr_pad_end ":\n" \
- INSTR_LEN_SANITY_CHECK(oldinstr_len)
-
-#define OLDINSTR_2(oldinstr, num1, num2) \
- "661:\n\t" oldinstr "\n662:\n" \
- ".if " altinstr_len(num1) " < " altinstr_len(num2) "\n" \
- OLDINSTR_PADDING(oldinstr, num2) \
- ".else\n" \
- OLDINSTR_PADDING(oldinstr, num1) \
- ".endif\n" \
- e_oldinstr_pad_end ":\n" \
- INSTR_LEN_SANITY_CHECK(oldinstr_len)
+
+#define OLDINSTR(oldinstr) \
+ "661:\n\t" oldinstr "\n662:\n"
#define ALTINSTR_ENTRY(facility, num) \
"\t.long 661b - .\n" /* old instruction */ \
"\t.long " b_altinstr(num)"b - .\n" /* alt instruction */ \
"\t.word " __stringify(facility) "\n" /* facility bit */ \
- "\t.byte " oldinstr_total_len "\n" /* source len */ \
- "\t.byte " altinstr_len(num) "\n" /* alt instruction len */
+ "\t.byte " oldinstr_len "\n" /* instruction len */ \
+ "\t.org . - (" oldinstr_len ") + (" altinstr_len(num) ")\n" \
+ "\t.org . - (" altinstr_len(num) ") + (" oldinstr_len ")\n"
#define ALTINSTR_REPLACEMENT(altinstr, num) /* replacement */ \
- b_altinstr(num)":\n\t" altinstr "\n" e_altinstr(num) ":\n" \
- INSTR_LEN_SANITY_CHECK(altinstr_len(num))
+ b_altinstr(num)":\n\t" altinstr "\n" e_altinstr(num) ":\n"
/* alternative assembly primitive: */
#define ALTERNATIVE(oldinstr, altinstr, facility) \
".pushsection .altinstr_replacement, \"ax\"\n" \
ALTINSTR_REPLACEMENT(altinstr, 1) \
".popsection\n" \
- OLDINSTR(oldinstr, 1) \
+ OLDINSTR(oldinstr) \
".pushsection .altinstructions,\"a\"\n" \
ALTINSTR_ENTRY(facility, 1) \
".popsection\n"
@@ -127,7 +74,7 @@ void apply_alternatives(struct alt_instr *start, struct alt_instr *end);
ALTINSTR_REPLACEMENT(altinstr1, 1) \
ALTINSTR_REPLACEMENT(altinstr2, 2) \
".popsection\n" \
- OLDINSTR_2(oldinstr, 1, 2) \
+ OLDINSTR(oldinstr) \
".pushsection .altinstructions,\"a\"\n" \
ALTINSTR_ENTRY(facility1, 1) \
ALTINSTR_ENTRY(facility2, 2) \
diff --git a/arch/s390/kernel/alternative.c b/arch/s390/kernel/alternative.c
index cce0ddee2d02..e7bca29f9c34 100644
--- a/arch/s390/kernel/alternative.c
+++ b/arch/s390/kernel/alternative.c
@@ -7,8 +7,6 @@
#include <asm/facility.h>
#include <asm/nospec-branch.h>
-#define MAX_PATCH_LEN (255 - 1)
-
static int __initdata_or_module alt_instr_disabled;
static int __init disable_alternative_instructions(char *str)
@@ -19,85 +17,30 @@ static int __init disable_alternative_instructions(char *str)
early_param("noaltinstr", disable_alternative_instructions);
-struct brcl_insn {
- u16 opc;
- s32 disp;
-} __packed;
-
-static u16 __initdata_or_module nop16 = 0x0700;
-static u32 __initdata_or_module nop32 = 0x47000000;
-static struct brcl_insn __initdata_or_module nop48 = {
- 0xc004, 0
-};
-
-static const void *nops[] __initdata_or_module = {
- &nop16,
- &nop32,
- &nop48
-};
-
-static void __init_or_module add_jump_padding(void *insns, unsigned int len)
-{
- struct brcl_insn brcl = {
- 0xc0f4,
- len / 2
- };
-
- memcpy(insns, &brcl, sizeof(brcl));
- insns += sizeof(brcl);
- len -= sizeof(brcl);
-
- while (len > 0) {
- memcpy(insns, &nop16, 2);
- insns += 2;
- len -= 2;
- }
-}
-
-static void __init_or_module add_padding(void *insns, unsigned int len)
-{
- if (len > 6)
- add_jump_padding(insns, len);
- else if (len >= 2)
- memcpy(insns, nops[len / 2 - 1], len);
-}
-
static void __init_or_module __apply_alternatives(struct alt_instr *start,
struct alt_instr *end)
{
struct alt_instr *a;
u8 *instr, *replacement;
- u8 insnbuf[MAX_PATCH_LEN];
/*
* The scan order should be from start to end. A later scanned
* alternative code can overwrite previously scanned alternative code.
*/
for (a = start; a < end; a++) {
- int insnbuf_sz = 0;
-
instr = (u8 *)&a->instr_offset + a->instr_offset;
replacement = (u8 *)&a->repl_offset + a->repl_offset;
if (!__test_facility(a->facility, alt_stfle_fac_list))
continue;
- if (unlikely(a->instrlen % 2 || a->replacementlen % 2)) {
+ if (unlikely(a->instrlen % 2)) {
WARN_ONCE(1, "cpu alternatives instructions length is "
"odd, skipping patching\n");
continue;
}
- memcpy(insnbuf, replacement, a->replacementlen);
- insnbuf_sz = a->replacementlen;
-
- if (a->instrlen > a->replacementlen) {
- add_padding(insnbuf + a->replacementlen,
- a->instrlen - a->replacementlen);
- insnbuf_sz += a->instrlen - a->replacementlen;
- }
-
- s390_kernel_write(instr, insnbuf, insnbuf_sz);
+ s390_kernel_write(instr, replacement, a->instrlen);
}
}
--
2.32.0
Commit ee6d777d3e93 ("s390/decompressor: support extra debug flags")
added extra debug flags, in particular debug info is created,
depending on config options.
With llvm's IAS this causes this compile warning:
arch/s390/boot/head.S:38:1: warning: DWARF2 only supports one section per compilation unit
.section ".head.text","ax"
^
This is a known problem and was addressed with a commit b8a9092330da
("Kbuild: do not emit debug info for assembly with LLVM_IAS=1").
Just do the same for s390 to get rid of this warning.
Signed-off-by: Heiko Carstens <[email protected]>
---
arch/s390/Makefile | 2 ++
1 file changed, 2 insertions(+)
diff --git a/arch/s390/Makefile b/arch/s390/Makefile
index c59efc83f020..d73611b35164 100644
--- a/arch/s390/Makefile
+++ b/arch/s390/Makefile
@@ -20,7 +20,9 @@ LDFLAGS_vmlinux := -pie
endif
aflags_dwarf := -Wa,-gdwarf-2
KBUILD_AFLAGS_DECOMPRESSOR := $(CLANG_FLAGS) -m64 -D__ASSEMBLY__
+ifndef CONFIG_AS_IS_LLVM
KBUILD_AFLAGS_DECOMPRESSOR += $(if $(CONFIG_DEBUG_INFO),$(aflags_dwarf))
+endif
KBUILD_CFLAGS_DECOMPRESSOR := $(CLANG_FLAGS) -m64 -O2 -mpacked-stack
KBUILD_CFLAGS_DECOMPRESSOR += -DDISABLE_BRANCH_PROFILING -D__NO_FORTIFY
KBUILD_CFLAGS_DECOMPRESSOR += -fno-delete-null-pointer-checks -msoft-float -mbackchain
--
2.32.0
On Wed, May 11, 2022 at 02:05:26PM +0200, Heiko Carstens wrote:
> clang fails to handle ".if" statements in inline assembly which are heavily
> used in the alternatives code.
FWIW, I missed to add error message(s) to the changelog:
In file included from ./include/linux/spinlock.h:93:
./arch/s390/include/asm/spinlock.h:81:3: error: expected absolute expression
ALTERNATIVE("", ".insn rre,0xb2fa0000,7,0", 49) /* NIAI 7 */
^
./arch/s390/include/asm/alternative.h:118:2: note: expanded from macro 'ALTERNATIVE'
ALTINSTR_REPLACEMENT(altinstr, 1) \
^
./arch/s390/include/asm/alternative.h:113:2: note: expanded from macro 'ALTINSTR_REPLACEMENT'
INSTR_LEN_SANITY_CHECK(altinstr_len(num))
^
./arch/s390/include/asm/alternative.h:62:3: note: expanded from macro 'INSTR_LEN_SANITY_CHECK'
".if " len " > 254\n" \
^
<inline asm>:5:5: note: instantiated into assembly here
.if 6651b-6641b > 254
^
Hi Heiko,
On Wed, May 11, 2022 at 02:05:28PM +0200, Heiko Carstens wrote:
> llvm's integrated assembler cannot handle immediate values which are
> calculated with two local labels:
>
> <instantiation>:3:13: error: invalid operand for instruction
> clgfi %r14,.Lsie_done - .Lsie_gmap
>
> Workaround this by adding clang specific code which reads the specific
> value from memory. Since this code is within the hot paths of the kernel
> and adds an additional memory reference, keep the original code, and add
> ifdef'ed code.
>
> Signed-off-by: Heiko Carstens <[email protected]>
> ---
> arch/s390/kernel/entry.S | 11 +++++++++++
> 1 file changed, 11 insertions(+)
>
> diff --git a/arch/s390/kernel/entry.S b/arch/s390/kernel/entry.S
> index e1664b45090f..ff7a75078e93 100644
> --- a/arch/s390/kernel/entry.S
> +++ b/arch/s390/kernel/entry.S
> @@ -171,8 +171,19 @@ _LPP_OFFSET = __LC_LPP
> .macro OUTSIDE reg,start,end,outside_label
> larl %r14,\start
> slgrk %r14,\reg,%r14
> +#ifdef CONFIG_CC_IS_CLANG
I intend to put this series through my build and boot test matrix later
today but one fly by comment in the meantime. Should this be
CONFIG_AS_IS_LLVM if this is an integrated assembler limitation, rather
than a clang one?
> + clgfrl %r14,.Lrange_size\@
> +#else
> clgfi %r14,\end - \start
> +#endif
> jhe \outside_label
> +#ifdef CONFIG_CC_IS_CLANG
> + .section .rodata, "a"
> + .align 4
> +.Lrange_size\@:
> + .long \end - \start
> + .previous
> +#endif
> .endm
>
> .macro SIEEXIT
> --
> 2.32.0
>
>
Cheers,
Nathan
On Wed, May 11, 2022 at 5:05 AM Heiko Carstens <[email protected]> wrote:
>
> A couple of patches which in result make it finally possible to build the
> kernel for s390 with llvm's integrated assembler. Several configs build
> without errors or warnings, and the kernel also works as expected.
>
> Note that patch 6 ("s390/boot: workaround llvm IAS bug") reveals a
> miscompile. This looks like a bug in the instruction definitions of the mvc
> and clc instructions(?). I'd like to ask people to look into this, since
> this silently generated broken code.
>
> This patch series is based on linux-next, which contains two additional
> required s390 specific patches to make llvm's IAS work.
I did a quick test of just a defconfig via:
$ ARCH=s390 CROSS_COMPILE=s390x-linux-gnu- make CC=clang -j72 defconfig all
and this assembled then booted in qemu for me. Thanks for the work
that went into this!
Tested-by: Nick Desaulniers <[email protected]>
Sounds like Nathan is doing additional testing as well.
>
> Thanks,
> Heiko
>
> Heiko Carstens (8):
> s390/alternatives: provide identical sized orginal/alternative sequences
> s390/alternatives: remove padding generation code
> s390/entry: shorten OUTSIDE macro
> s390/entry: workaround llvm's IAS limitations
> s390/purgatory: workaround llvm's IAS limitations
> s390/boot: workaround llvm IAS bug
> s390/boot: do not emit debug info for assembly with llvm's IAS
> scripts/min-tool-version.sh: raise minimum clang version to 14.0.0 for s390
>
> arch/s390/Makefile | 2 +
> arch/s390/boot/head.S | 34 +++++----
> arch/s390/include/asm/alternative-asm.h | 76 +++-----------------
> arch/s390/include/asm/alternative.h | 93 ++++++-------------------
> arch/s390/include/asm/spinlock.h | 2 +-
> arch/s390/kernel/alternative.c | 61 +---------------
> arch/s390/kernel/entry.S | 39 +++++++----
> arch/s390/lib/spinlock.c | 4 +-
> arch/s390/purgatory/head.S | 29 ++++++--
> scripts/min-tool-version.sh | 3 +-
> 10 files changed, 104 insertions(+), 239 deletions(-)
>
> --
> 2.32.0
>
--
Thanks,
~Nick Desaulniers
On Wed, May 11, 2022 at 12:48:34PM -0700, Nick Desaulniers wrote:
> On Wed, May 11, 2022 at 5:05 AM Heiko Carstens <[email protected]> wrote:
> >
> > A couple of patches which in result make it finally possible to build the
> > kernel for s390 with llvm's integrated assembler. Several configs build
> > without errors or warnings, and the kernel also works as expected.
> >
> > Note that patch 6 ("s390/boot: workaround llvm IAS bug") reveals a
> > miscompile. This looks like a bug in the instruction definitions of the mvc
> > and clc instructions(?). I'd like to ask people to look into this, since
> > this silently generated broken code.
> >
> > This patch series is based on linux-next, which contains two additional
> > required s390 specific patches to make llvm's IAS work.
>
> I did a quick test of just a defconfig via:
> $ ARCH=s390 CROSS_COMPILE=s390x-linux-gnu- make CC=clang -j72 defconfig all
> and this assembled then booted in qemu for me. Thanks for the work
> that went into this!
>
> Tested-by: Nick Desaulniers <[email protected]>
Will add this too. Thank you!
On Thu, May 12, 2022 at 07:24:25PM +0200, Heiko Carstens wrote:
> On Wed, May 11, 2022 at 10:30:05AM -0700, Nathan Chancellor wrote:
> > Hi Heiko,
> >
> > On Wed, May 11, 2022 at 02:05:28PM +0200, Heiko Carstens wrote:
> > > llvm's integrated assembler cannot handle immediate values which are
> > > calculated with two local labels:
> > >
> > > <instantiation>:3:13: error: invalid operand for instruction
> > > clgfi %r14,.Lsie_done - .Lsie_gmap
> > >
> > > Workaround this by adding clang specific code which reads the specific
> > > value from memory. Since this code is within the hot paths of the kernel
> > > and adds an additional memory reference, keep the original code, and add
> > > ifdef'ed code.
> > >
> > > Signed-off-by: Heiko Carstens <[email protected]>
> > > ---
> > > arch/s390/kernel/entry.S | 11 +++++++++++
> > > 1 file changed, 11 insertions(+)
> > >
> > > diff --git a/arch/s390/kernel/entry.S b/arch/s390/kernel/entry.S
> > > index e1664b45090f..ff7a75078e93 100644
> > > --- a/arch/s390/kernel/entry.S
> > > +++ b/arch/s390/kernel/entry.S
> > > @@ -171,8 +171,19 @@ _LPP_OFFSET = __LC_LPP
> > > .macro OUTSIDE reg,start,end,outside_label
> > > larl %r14,\start
> > > slgrk %r14,\reg,%r14
> > > +#ifdef CONFIG_CC_IS_CLANG
> >
> > I intend to put this series through my build and boot test matrix later
> > today but one fly by comment in the meantime. Should this be
> > CONFIG_AS_IS_LLVM if this is an integrated assembler limitation, rather
> > than a clang one?
>
> Yes, that makes a lot of sense. Considering that I will drop the
> previous patch within this series, the new version looks like:
>
> From fe4fb0b014378d84ae517deaea338577b2ea6ae0 Mon Sep 17 00:00:00 2001
> From: Heiko Carstens <[email protected]>
> Date: Sat, 7 May 2022 15:00:40 +0200
> Subject: [PATCH 3/7] s390/entry: workaround llvm's IAS limitations
>
> llvm's integrated assembler cannot handle immediate values which are
> calculated with two local labels:
>
> <instantiation>:3:13: error: invalid operand for instruction
> clgfi %r14,.Lsie_done - .Lsie_gmap
>
> Workaround this by adding clang specific code which reads the specific
> value from memory. Since this code is within the hot paths of the kernel
> and adds an additional memory reference, keep the original code, and add
> ifdef'ed code.
>
> Signed-off-by: Heiko Carstens <[email protected]>
> ---
> arch/s390/kernel/entry.S | 14 ++++++++++++--
> 1 file changed, 12 insertions(+), 2 deletions(-)
>
> diff --git a/arch/s390/kernel/entry.S b/arch/s390/kernel/entry.S
> index a6b45eaa3450..f2f30bfba1e9 100644
> --- a/arch/s390/kernel/entry.S
> +++ b/arch/s390/kernel/entry.S
> @@ -172,9 +172,19 @@ _LPP_OFFSET = __LC_LPP
> lgr %r14,\reg
> larl %r13,\start
> slgr %r14,%r13
> - lghi %r13,\end - \start
> - clgr %r14,%r13
> +#ifdef CONFIG_AS_IS_LLVM
> + clgfrl %r14,.Lrange_size\@
> +#else
> + clgfi %r14,\end - \start
> +#endif
> jhe \outside_label
> +#ifdef CONFIG_CC_IS_CLANG
I think this one also wants to be CONFIG_AS_IS_LLVM, right?
Other than that, seems fine to me, although I have no knowledge of s390
assembly so that statement probably means next to nothing :)
Cheers,
Nathan
> + .section .rodata, "a"
> + .align 4
> +.Lrange_size\@:
> + .long \end - \start
> + .previous
> +#endif
> .endm
>
> .macro SIEEXIT
> --
> 2.32.0
On Thu, May 12, 2022 at 12:06:59PM -0700, Nathan Chancellor wrote:
> > +#ifdef CONFIG_AS_IS_LLVM
> > + clgfrl %r14,.Lrange_size\@
> > +#else
> > + clgfi %r14,\end - \start
> > +#endif
> > jhe \outside_label
> > +#ifdef CONFIG_CC_IS_CLANG
>
> I think this one also wants to be CONFIG_AS_IS_LLVM, right?
Yes, of course.
> Other than that, seems fine to me, although I have no knowledge of s390
> assembly so that statement probably means next to nothing :)
Thanks for having a look! :)
Hi Heiko,
On Wed, May 11, 2022 at 02:05:24PM +0200, Heiko Carstens wrote:
> A couple of patches which in result make it finally possible to build the
> kernel for s390 with llvm's integrated assembler. Several configs build
> without errors or warnings, and the kernel also works as expected.
>
> Note that patch 6 ("s390/boot: workaround llvm IAS bug") reveals a
> miscompile. This looks like a bug in the instruction definitions of the mvc
> and clc instructions(?). I'd like to ask people to look into this, since
> this silently generated broken code.
I think it should be pretty simple to file a bug report for this since
it occurs in a standalone assembly file? I agree with Nick that there
should be a bug report filed and linked to in patch 6 so that we don't
lose track of it.
> This patch series is based on linux-next, which contains two additional
> required s390 specific patches to make llvm's IAS work.
>
> Thanks,
> Heiko
>
> Heiko Carstens (8):
> s390/alternatives: provide identical sized orginal/alternative sequences
> s390/alternatives: remove padding generation code
> s390/entry: shorten OUTSIDE macro
> s390/entry: workaround llvm's IAS limitations
> s390/purgatory: workaround llvm's IAS limitations
> s390/boot: workaround llvm IAS bug
> s390/boot: do not emit debug info for assembly with llvm's IAS
> scripts/min-tool-version.sh: raise minimum clang version to 14.0.0 for s390
>
> arch/s390/Makefile | 2 +
> arch/s390/boot/head.S | 34 +++++----
> arch/s390/include/asm/alternative-asm.h | 76 +++-----------------
> arch/s390/include/asm/alternative.h | 93 ++++++-------------------
> arch/s390/include/asm/spinlock.h | 2 +-
> arch/s390/kernel/alternative.c | 61 +---------------
> arch/s390/kernel/entry.S | 39 +++++++----
> arch/s390/lib/spinlock.c | 4 +-
> arch/s390/purgatory/head.S | 29 ++++++--
> scripts/min-tool-version.sh | 3 +-
> 10 files changed, 104 insertions(+), 239 deletions(-)
I applied this series to the latest s390 for-next branch (c4fb15578802)
and built a few in-tree and distribution configurations with clang-14
and clang-15 then boot tested them in QEMU with a simple buildroot
userspace. I did not see any new warnings or errors. This is awesome, I
am excited to get this wired up in our CI!
In case it is worthwhile:
Tested-by: Nathan Chancellor <[email protected]>
Cheers,
Nathan
On Wed, May 11, 2022 at 10:30:05AM -0700, Nathan Chancellor wrote:
> Hi Heiko,
>
> On Wed, May 11, 2022 at 02:05:28PM +0200, Heiko Carstens wrote:
> > llvm's integrated assembler cannot handle immediate values which are
> > calculated with two local labels:
> >
> > <instantiation>:3:13: error: invalid operand for instruction
> > clgfi %r14,.Lsie_done - .Lsie_gmap
> >
> > Workaround this by adding clang specific code which reads the specific
> > value from memory. Since this code is within the hot paths of the kernel
> > and adds an additional memory reference, keep the original code, and add
> > ifdef'ed code.
> >
> > Signed-off-by: Heiko Carstens <[email protected]>
> > ---
> > arch/s390/kernel/entry.S | 11 +++++++++++
> > 1 file changed, 11 insertions(+)
> >
> > diff --git a/arch/s390/kernel/entry.S b/arch/s390/kernel/entry.S
> > index e1664b45090f..ff7a75078e93 100644
> > --- a/arch/s390/kernel/entry.S
> > +++ b/arch/s390/kernel/entry.S
> > @@ -171,8 +171,19 @@ _LPP_OFFSET = __LC_LPP
> > .macro OUTSIDE reg,start,end,outside_label
> > larl %r14,\start
> > slgrk %r14,\reg,%r14
> > +#ifdef CONFIG_CC_IS_CLANG
>
> I intend to put this series through my build and boot test matrix later
> today but one fly by comment in the meantime. Should this be
> CONFIG_AS_IS_LLVM if this is an integrated assembler limitation, rather
> than a clang one?
Yes, that makes a lot of sense. Considering that I will drop the
previous patch within this series, the new version looks like:
From fe4fb0b014378d84ae517deaea338577b2ea6ae0 Mon Sep 17 00:00:00 2001
From: Heiko Carstens <[email protected]>
Date: Sat, 7 May 2022 15:00:40 +0200
Subject: [PATCH 3/7] s390/entry: workaround llvm's IAS limitations
llvm's integrated assembler cannot handle immediate values which are
calculated with two local labels:
<instantiation>:3:13: error: invalid operand for instruction
clgfi %r14,.Lsie_done - .Lsie_gmap
Workaround this by adding clang specific code which reads the specific
value from memory. Since this code is within the hot paths of the kernel
and adds an additional memory reference, keep the original code, and add
ifdef'ed code.
Signed-off-by: Heiko Carstens <[email protected]>
---
arch/s390/kernel/entry.S | 14 ++++++++++++--
1 file changed, 12 insertions(+), 2 deletions(-)
diff --git a/arch/s390/kernel/entry.S b/arch/s390/kernel/entry.S
index a6b45eaa3450..f2f30bfba1e9 100644
--- a/arch/s390/kernel/entry.S
+++ b/arch/s390/kernel/entry.S
@@ -172,9 +172,19 @@ _LPP_OFFSET = __LC_LPP
lgr %r14,\reg
larl %r13,\start
slgr %r14,%r13
- lghi %r13,\end - \start
- clgr %r14,%r13
+#ifdef CONFIG_AS_IS_LLVM
+ clgfrl %r14,.Lrange_size\@
+#else
+ clgfi %r14,\end - \start
+#endif
jhe \outside_label
+#ifdef CONFIG_CC_IS_CLANG
+ .section .rodata, "a"
+ .align 4
+.Lrange_size\@:
+ .long \end - \start
+ .previous
+#endif
.endm
.macro SIEEXIT
--
2.32.0
On Wed, May 11, 2022 at 01:52:13PM -0700, Nathan Chancellor wrote:
> Hi Heiko,
>
> On Wed, May 11, 2022 at 02:05:24PM +0200, Heiko Carstens wrote:
> > A couple of patches which in result make it finally possible to build the
> > kernel for s390 with llvm's integrated assembler. Several configs build
> > without errors or warnings, and the kernel also works as expected.
> >
> > Note that patch 6 ("s390/boot: workaround llvm IAS bug") reveals a
> > miscompile. This looks like a bug in the instruction definitions of the mvc
> > and clc instructions(?). I'd like to ask people to look into this, since
> > this silently generated broken code.
>
> I think it should be pretty simple to file a bug report for this since
> it occurs in a standalone assembly file? I agree with Nick that there
> should be a bug report filed and linked to in patch 6 so that we don't
> lose track of it.
https://github.com/llvm/llvm-project/issues/55411
> I applied this series to the latest s390 for-next branch (c4fb15578802)
> and built a few in-tree and distribution configurations with clang-14
> and clang-15 then boot tested them in QEMU with a simple buildroot
> userspace. I did not see any new warnings or errors. This is awesome, I
> am excited to get this wired up in our CI!
>
> In case it is worthwhile:
>
> Tested-by: Nathan Chancellor <[email protected]>
Yes, it is. Thanks a lot!
On Wed, May 11, 2022 at 02:05:26PM +0200, Heiko Carstens wrote:
> clang fails to handle ".if" statements in inline assembly which are heavily
> used in the alternatives code.
>
> To work around this remove this code, and enforce that users of
> alternatives must specify original and alternative instruction sequences
> which have identical sizes. Add a compile time check with two ".org"
> statements similar to arm64.
>
> In result not only clang can handle this, but also quite a lot of code can
> be removed.
>
> Signed-off-by: Heiko Carstens <[email protected]>
> ---
> arch/s390/include/asm/alternative-asm.h | 76 +++-----------------
> arch/s390/include/asm/alternative.h | 93 ++++++-------------------
> arch/s390/kernel/alternative.c | 61 +---------------
> 3 files changed, 31 insertions(+), 199 deletions(-)
Acked-by: Vasily Gorbik <[email protected]>
On Wed, May 11, 2022 at 5:05 AM Heiko Carstens <[email protected]> wrote:
>
> For at least the mvc and clc instructions llvm's integrated assembler can
> generate incorrect code. In particular this happens with decompressor boot
> code. The reason seems to be that relocations for the second displacement
> of each instruction are at incorrect locations (-/+: gas vs llvm IAS):
>
> mvc __LC_IO_NEW_PSW(16),.Lnewpsw
>
> results in
>
> 4: d2 0f 01 f0 00 00 mvc 496(16,%r0),0
> - 8: R_390_12 .head.text+0x10
> + 6: R_390_12 .head.text+0x10
>
> and
> clc 0(3,%r4),.L_hdr
> results in
>
> 258: d5 02 40 00 00 00 clc 0(3,%r4),0
> - 25c: R_390_12 .head.text+0x324
> + 25a: R_390_12 .head.text+0x324
>
> Workaround this by writing the code in a different way.
>
> Signed-off-by: Heiko Carstens <[email protected]>
Please link to an LLVM bugreport for this.
> ---
> arch/s390/boot/head.S | 34 +++++++++++++++++++++-------------
> 1 file changed, 21 insertions(+), 13 deletions(-)
>
> diff --git a/arch/s390/boot/head.S b/arch/s390/boot/head.S
> index 2ced90172680..8402e1cd133b 100644
> --- a/arch/s390/boot/head.S
> +++ b/arch/s390/boot/head.S
> @@ -42,7 +42,8 @@ ipl_start:
> # subroutine to wait for end I/O
> #
> .Lirqwait:
> - mvc __LC_IO_NEW_PSW(16),.Lnewpsw # set up IO interrupt psw
> + larl %r13,.Lnewpsw # set up IO interrupt psw
> + mvc __LC_IO_NEW_PSW(16),0(%r13)
> lpsw .Lwaitpsw
> .Lioint:
> br %r14
> @@ -155,9 +156,11 @@ ipl_start:
> lr %r2,%r3
> .Lnotrunc:
> l %r4,.Linitrd
> - clc 0(3,%r4),.L_hdr # if it is HDRx
> + larl %r13,.L_hdr
> + clc 0(3,%r4),0(%r13) # if it is HDRx
> bz .Lagain1 # skip dataset header
> - clc 0(3,%r4),.L_eof # if it is EOFx
> + larl %r13,.L_eof
> + clc 0(3,%r4),0(%r13) # if it is EOFx
> bz .Lagain1 # skip dateset trailer
>
> lr %r5,%r2
> @@ -181,9 +184,11 @@ ipl_start:
> .Lrdcont:
> l %r2,.Linitrd
>
> - clc 0(3,%r2),.L_hdr # skip HDRx and EOFx
> + larl %r13,.L_hdr # skip HDRx and EOFx
> + clc 0(3,%r2),0(%r13)
> bz .Lagain2
> - clc 0(3,%r2),.L_eof
> + larl %r13,.L_eof
> + clc 0(3,%r2),0(%r13)
> bz .Lagain2
>
> #
> @@ -260,20 +265,23 @@ SYM_CODE_START_LOCAL(startup_normal)
> .fill 16,4,0x0
> 0: lmh %r0,%r15,0(%r13) # clear high-order half of gprs
> sam64 # switch to 64 bit addressing mode
> - basr %r13,0 # get base
> -.LPG0:
> - mvc __LC_EXT_NEW_PSW(16),.Lext_new_psw-.LPG0(%r13)
> - mvc __LC_PGM_NEW_PSW(16),.Lpgm_new_psw-.LPG0(%r13)
> - mvc __LC_IO_NEW_PSW(16),.Lio_new_psw-.LPG0(%r13)
> + larl %r13,.Lext_new_psw
> + mvc __LC_EXT_NEW_PSW(16),0(%r13)
> + larl %r13,.Lpgm_new_psw
> + mvc __LC_PGM_NEW_PSW(16),0(%r13)
> + larl %r13,.Lio_new_psw
> + mvc __LC_IO_NEW_PSW(16),0(%r13)
> xc 0x200(256),0x200 # partially clear lowcore
> xc 0x300(256),0x300
> xc 0xe00(256),0xe00
> xc 0xf00(256),0xf00
> - lctlg %c0,%c15,.Lctl-.LPG0(%r13) # load control registers
> + larl %r13,.Lctl
> + lctlg %c0,%c15,0(%r13) # load control registers
> stcke __LC_BOOT_CLOCK
> mvc __LC_LAST_UPDATE_CLOCK(8),__LC_BOOT_CLOCK+1
> - spt 6f-.LPG0(%r13)
> - mvc __LC_LAST_UPDATE_TIMER(8),6f-.LPG0(%r13)
> + larl %r13,6f
> + spt 0(%r13)
> + mvc __LC_LAST_UPDATE_TIMER(8),0(%r13)
> larl %r15,_stack_end-STACK_FRAME_OVERHEAD
> brasl %r14,sclp_early_setup_buffer
> brasl %r14,verify_facilities
> --
> 2.32.0
>
--
Thanks,
~Nick Desaulniers
On Thu, May 12, 2022 at 07:24:25PM +0200, Heiko Carstens wrote:
> From fe4fb0b014378d84ae517deaea338577b2ea6ae0 Mon Sep 17 00:00:00 2001
> From: Heiko Carstens <[email protected]>
> Date: Sat, 7 May 2022 15:00:40 +0200
> Subject: [PATCH 3/7] s390/entry: workaround llvm's IAS limitations
>
> llvm's integrated assembler cannot handle immediate values which are
> calculated with two local labels:
>
> <instantiation>:3:13: error: invalid operand for instruction
> clgfi %r14,.Lsie_done - .Lsie_gmap
>
> Workaround this by adding clang specific code which reads the specific
> value from memory. Since this code is within the hot paths of the kernel
> and adds an additional memory reference, keep the original code, and add
> ifdef'ed code.
>
> Signed-off-by: Heiko Carstens <[email protected]>
> ---
> arch/s390/kernel/entry.S | 14 ++++++++++++--
> 1 file changed, 12 insertions(+), 2 deletions(-)
>
> diff --git a/arch/s390/kernel/entry.S b/arch/s390/kernel/entry.S
> index a6b45eaa3450..f2f30bfba1e9 100644
> --- a/arch/s390/kernel/entry.S
> +++ b/arch/s390/kernel/entry.S
> @@ -172,9 +172,19 @@ _LPP_OFFSET = __LC_LPP
> lgr %r14,\reg
> larl %r13,\start
> slgr %r14,%r13
> - lghi %r13,\end - \start
> - clgr %r14,%r13
> +#ifdef CONFIG_AS_IS_LLVM
> + clgfrl %r14,.Lrange_size\@
> +#else
> + clgfi %r14,\end - \start
> +#endif
> jhe \outside_label
> +#ifdef CONFIG_CC_IS_CLANG
> + .section .rodata, "a"
> + .align 4
> +.Lrange_size\@:
> + .long \end - \start
Isn't the machine check handler refers to this memory before checking
unrecoverable storage errors (with CHKSTG macro) as result of this change?
> + .previous
> +#endif
> .endm
>
> .macro SIEEXIT
> --
> 2.32.0
On Mon, May 16, 2022 at 12:19:45PM +0200, Heiko Carstens wrote:
> On Mon, May 16, 2022 at 11:07:43AM +0200, Alexander Gordeev wrote:
> > Isn't the machine check handler refers to this memory before checking
> > unrecoverable storage errors (with CHKSTG macro) as result of this change?
>
> Yes, indeed. However implementing this without another register will
> be quite of a challenge. So what I would prefer in any case: just
> assume that this minimal set of memory accesses work. Actually I'd
> seriously like to go a bit further, and even move the checks for
> storage errors back to C for two reasons:
>
> - this would make the machine check handler entry code easier again
> - it would also allow to enter the machine check handler with DAT on
>
> After all we rely anyway on the fact that at least the local lowcore +
> the page(s) which contain text are still accessible. Assuming that a
> couple of page tables also work won't make this much worse, but the
> code much easier.
>
> So I'd suggest: leave this code as is, and at some later point move
> "rework" the early machine check handler code.
>
> What do you think?
Sounds very reasonable. Please, find my:
Acked-by: Alexander Gordeev <[email protected]>
Also, how such a follow-up looks to you?
lgr %r14,\reg
#ifdef CONFIG_AS_IS_LLVM
larl %r13,\start
slgr %r14,%r13
clgfrl %r14,.Lrange_size\@
#else
slgfi %r14,\start
clgfi %r14,\end - \start
#endif
I will try to get a patch for clang ready soon... /Jonas
On 2022-05-16 12:19 em, Heiko Carstens wrote:
> On Mon, May 16, 2022 at 11:07:43AM +0200, Alexander Gordeev wrote:
>>> diff --git a/arch/s390/kernel/entry.S b/arch/s390/kernel/entry.S
>>> index a6b45eaa3450..f2f30bfba1e9 100644
>>> --- a/arch/s390/kernel/entry.S
>>> +++ b/arch/s390/kernel/entry.S
>>> @@ -172,9 +172,19 @@ _LPP_OFFSET = __LC_LPP
>>> lgr %r14,\reg
>>> larl %r13,\start
>>> slgr %r14,%r13
>>> - lghi %r13,\end - \start
>>> - clgr %r14,%r13
>>> +#ifdef CONFIG_AS_IS_LLVM
>>> + clgfrl %r14,.Lrange_size\@
>>> +#else
>>> + clgfi %r14,\end - \start
>>> +#endif
>>> jhe \outside_label
>>> +#ifdef CONFIG_CC_IS_CLANG
>>> + .section .rodata, "a"
>>> + .align 4
>>> +.Lrange_size\@:
>>> + .long \end - \start
>> Isn't the machine check handler refers to this memory before checking
>> unrecoverable storage errors (with CHKSTG macro) as result of this change?
> Yes, indeed. However implementing this without another register will
> be quite of a challenge. So what I would prefer in any case: just
> assume that this minimal set of memory accesses work. Actually I'd
> seriously like to go a bit further, and even move the checks for
> storage errors back to C for two reasons:
>
> - this would make the machine check handler entry code easier again
> - it would also allow to enter the machine check handler with DAT on
>
> After all we rely anyway on the fact that at least the local lowcore +
> the page(s) which contain text are still accessible. Assuming that a
> couple of page tables also work won't make this much worse, but the
> code much easier.
>
> So I'd suggest: leave this code as is, and at some later point move
> "rework" the early machine check handler code.
>
> What do you think?
On Mon, May 16, 2022 at 01:11:48PM +0200, Alexander Gordeev wrote:
> > So I'd suggest: leave this code as is, and at some later point move
> > "rework" the early machine check handler code.
> >
> > What do you think?
>
> Sounds very reasonable. Please, find my:
>
> Acked-by: Alexander Gordeev <[email protected]>
Thanks!
> Also, how such a follow-up looks to you?
...
> slgfi %r14,\start
> clgfi %r14,\end - \start
I think using an address as an immediate value is a step in the wrong
direction, since I'd like to have all code pc-relative. And as far as
I can tell this new construct would only work as long as \start has an
absolute address that is low enough so that it would work / fit with
slgfi.
Of course this will likely always be the case, but I still think this
is not the way we should go.
On Mon, May 16, 2022 at 11:07:43AM +0200, Alexander Gordeev wrote:
> > diff --git a/arch/s390/kernel/entry.S b/arch/s390/kernel/entry.S
> > index a6b45eaa3450..f2f30bfba1e9 100644
> > --- a/arch/s390/kernel/entry.S
> > +++ b/arch/s390/kernel/entry.S
> > @@ -172,9 +172,19 @@ _LPP_OFFSET = __LC_LPP
> > lgr %r14,\reg
> > larl %r13,\start
> > slgr %r14,%r13
> > - lghi %r13,\end - \start
> > - clgr %r14,%r13
> > +#ifdef CONFIG_AS_IS_LLVM
> > + clgfrl %r14,.Lrange_size\@
> > +#else
> > + clgfi %r14,\end - \start
> > +#endif
> > jhe \outside_label
> > +#ifdef CONFIG_CC_IS_CLANG
> > + .section .rodata, "a"
> > + .align 4
> > +.Lrange_size\@:
> > + .long \end - \start
>
> Isn't the machine check handler refers to this memory before checking
> unrecoverable storage errors (with CHKSTG macro) as result of this change?
Yes, indeed. However implementing this without another register will
be quite of a challenge. So what I would prefer in any case: just
assume that this minimal set of memory accesses work. Actually I'd
seriously like to go a bit further, and even move the checks for
storage errors back to C for two reasons:
- this would make the machine check handler entry code easier again
- it would also allow to enter the machine check handler with DAT on
After all we rely anyway on the fact that at least the local lowcore +
the page(s) which contain text are still accessible. Assuming that a
couple of page tables also work won't make this much worse, but the
code much easier.
So I'd suggest: leave this code as is, and at some later point move
"rework" the early machine check handler code.
What do you think?