Messages in this thread | | | From | Damian Tometzki <> | Subject | Re: [GIT PULL] x86/asm changes for v5.6 | Date | Thu, 30 Jan 2020 06:47:14 +0100 |
| |
Am 29.01.20 um 19:34 schrieb Borislav Petkov: > On Wed, Jan 29, 2020 at 09:40:58AM -0800, Linus Torvalds wrote: >> On Wed, Jan 29, 2020 at 9:07 AM Luck, Tony <tony.luck@intel.com> wrote: >>> >>> This returns "3" ... not what we want. But swapping the ERMS/FSRM order >>> gets the correct version. >> >> That actually makes sense, and is what I suspected (after I wrote the >> patch) what would happen. It would just be good to have it explicitly >> documented at the macro. > > Like this? > > --- > diff --git a/arch/x86/include/asm/alternative.h b/arch/x86/include/asm/alternative.h > index 13adca37c99a..d94bad03bcb4 100644 > --- a/arch/x86/include/asm/alternative.h > +++ b/arch/x86/include/asm/alternative.h > @@ -164,6 +164,11 @@ static inline int alternatives_text_reserved(void *start, void *end) > ALTINSTR_REPLACEMENT(newinstr, feature, 1) \ > ".popsection\n" > > +/* > + * The patching happens in the natural order of first newinstr1 and then > + * newinstr2, iff the respective feature bits are set. See apply_alternatives() > + * for details. > + */ > #define ALTERNATIVE_2(oldinstr, newinstr1, feature1, newinstr2, feature2)\ > OLDINSTR_2(oldinstr, 1, 2) \ > ".pushsection .altinstructions,\"a\"\n" \ > >> That would be bad indeed, but I don't think it should matter >> particularly for this case - it would have been bad before too. >> >> I suspect there is some other issue going on, like the NOP padding >> logic being confused. > > Or the cmp $0x20 test missing in the default case, see below. > >> In particular, look here, this is the alt_instruction entries: >> >> altinstruction_entry 140b,143f,\feature1,142b-140b,144f-143f,142b-141b >> altinstruction_entry 140b,144f,\feature2,142b-140b,145f-144f,142b-141b >> >> where the arguments are "orig alt feature orig_len alt_len pad_len" in >> that order. >> >> Notice how "pad_len" in both cases is the padding from the _original_ >> instruction (142b-141b). > > <snip this which I'll take a look later so that we can sort out the > issue at hand first> > >> So I'm just hand-waving. Maybe there was some simpler explanation >> (like me just picking the wrong instructions when I did the rough >> conversion and simply breaking things with some stupid bug). > > Looks like it. So I did this: > > --- > diff --git a/arch/x86/lib/memmove_64.S b/arch/x86/lib/memmove_64.S > index 7ff00ea64e4f..a670d01570df 100644 > --- a/arch/x86/lib/memmove_64.S > +++ b/arch/x86/lib/memmove_64.S > @@ -39,23 +39,19 @@ SYM_FUNC_START(__memmove) > cmp %rdi, %r8 > jg 2f > > - /* FSRM implies ERMS => no length checks, do the copy directly */ > -.Lmemmove_begin_forward: > - ALTERNATIVE "cmp $0x20, %rdx; jb 1f", "", X86_FEATURE_FSRM > - ALTERNATIVE "", "movq %rdx, %rcx; rep movsb; retq", X86_FEATURE_ERMS > - > /* > - * movsq instruction have many startup latency > - * so we handle small size by general register. > - */ > - cmp $680, %rdx > - jb 3f > - /* > - * movsq instruction is only good for aligned case. > + * Three rep-string alternatives: > + * - go to "movsq" for large regions where source and dest are > + * mutually aligned (same in low 8 bits). "label 4" > + * - plain rep-movsb for FSRM > + * - rep-movs for > 32 byte for ERMS. > */ > +.Lmemmove_begin_forward: > + ALTERNATIVE_2 \ > + "cmp $0x20, %rdx; jb 1f; cmp $680, %rdx ; jb 3f ; cmpb %dil, %sil; je 4f", \ > + "cmp $0x20, %rdx; jb 1f; movq %rdx, %rcx; rep movsb; retq", X86_FEATURE_ERMS, \ > + "movq %rdx, %rcx ; rep movsb; retq", X86_FEATURE_FSRM > > - cmpb %dil, %sil > - je 4f > 3: > sub $0x20, %rdx > /* > > --- > > Notice how the *first* option of the alternative, which is the default > one, has that gotten that additional "cmp $0x20, %rdx; jb 1f" test which > sends us down to the less than 32 bytes copy length. > > Your original version didn't have it and here's what I saw: > > So I stopped the guest just before that code and the call trace looked > like this: > > #0 memmove () at arch/x86/lib/memmove_64.S:43 > #1 0xffffffff824448c2 in memblock_insert_region (type=0xffffffff824a8298 <memblock+56>, idx=<optimized out>, base=0, > size=4096, nid=2, flags=MEMBLOCK_NONE) at mm/memblock.c:553 > #2 0xffffffff824454f0 in memblock_add_range (type=0xffffffff824a8298 <memblock+56>, base=0, size=<optimized out>, > nid=73400320, flags=<optimized out>) at mm/memblock.c:641 > #3 0xffffffff82445627 in memblock_reserve (base=0, size=4096) at mm/memblock.c:830 > #4 0xffffffff823ff399 in setup_arch (cmdline_p=0xffffffff82003f28) at arch/x86/kernel/setup.c:798 > #5 0xffffffff823f9ae1 in start_kernel () at init/main.c:598 > #6 0xffffffff810000d4 in secondary_startup_64 () at arch/x86/kernel/head_64.S:242 > #7 0x0000000000000000 in ?? () > > and count in rdx was: > > rdx 0x18 24 > > Without that "cmp $0x20" test above, we do the "cmp $680, %rdx; jb 3f;" test > and we run into the following asm at label 3: > > 3: > sub $0x20, %rdx > /* > * We gobble 32 bytes forward in each loop. > */ > > <--- right here %rdx is: > > rdx 0xfffffffffffffff8 -8 > > and yeeehaaw, we're in the weeds and then end up triplefaulting at some > unmapped source address in %rsi or so. > > So now I'm going to play all three variants with pen and paper to make > sure we're still sane. > > Thx. > Hello together,
in my qemu env the system isnt coming up. I tried both with and without the changes from Borislav.
Best regards Damian
0.605193] ------------[ cut here ]------------ [ 0.605933] General protection fault in user access. Non-canonical address? [ 0.605948] WARNING: CPU: 0 PID: 1 at arch/x86/mm/extable.c:77 ex_handler_uaccess+0x48/0x50 [ 0.606931] Modules linked in: [ 0.606931] CPU: 0 PID: 1 Comm: swapper/0 Not tainted 5.5.0 #15 [ 0.606931] Hardware name: QEMU Standard PC (i440FX + PIIX, 1996), BIOS rel-1.12.0-0-ga698c8995f-prebuilt.qemu.org 04/01/2014 [ 0.606931] RIP: 0010:ex_handler_uaccess+0x48/0x50 [ 0.606931] Code: 83 c4 08 b8 01 00 00 00 5b c3 80 3d 78 75 50 01 00 75 dc 48 c7 c7 00 ea 1e 82 48 89 34 24 c6 05 64 75 50 01 01 e8 9e fd 00 00 <0f> 0b 48 8b 34 24 eb bd 80 3d 4f 75 50 01 00 55 48 89 fd 53 49 [ 0.606931] RSP: 0000:ffffc900000dbc30 EFLAGS: 00010286 [ 0.606931] RAX: 000000000000003f RBX: ffffffff82339d6c RCX: 0000000000000000 [ 0.606931] RDX: 0000000000000007 RSI: ffffffff8316dc5f RDI: ffffffff8316e05f [ 0.606931] RBP: 000000000000000d R08: ffffffff8316dc20 R09: 0000000000029840 [ 0.606931] R10: 000000010196fab4 R11: 0000000000000001 R12: 0000000000000000 [ 0.606931] R13: 0000000000000000 R14: 0000000000000000 R15: 0000000000000000 [ 0.606931] FS: 0000000000000000(0000) GS:ffff88800f800000(0000) knlGS:0000000000000000 [ 0.606931] CS: 0010 DS: 0000 ES: 0000 CR0: 0000000080050033 [ 0.606931] CR2: 0000000000000000 CR3: 000000000240a000 CR4: 00000000000006f0 [ 0.606931] Call Trace: [ 0.606931] fixup_exception+0x3e/0x51 [ 0.606931] do_general_protection+0x9c/0x260 [ 0.606931] general_protection+0x28/0x30 [ 0.606931] RIP: 0010:copy_user_generic_string+0x2c/0x40 [ 0.606931] Code: 00 83 fa 08 72 27 89 f9 83 e1 07 74 15 83 e9 08 f7 d9 29 ca 8a 06 88 07 48 ff c6 48 ff c7 ff c9 75 f2 89 d1 c1 e9 03 83 e2 07 <f3> 48 a5 89 d1 f3 a4 31 c0 0f 1f 00 c3 0f 1f 80 00 00 00 00 0f [ 0.606931] RSP: 0000:ffffc900000dbda0 EFLAGS: 00010246 [ 0.606931] RAX: 0000000000000000 RBX: ffff88801e488000 RCX: 0000000000000001 [ 0.606931] RDX: 0000000000000000 RSI: 009896808086a201 RDI: ffffc900000dbdc0 [ 0.606931] RBP: ffffffffffffffff R08: ffff88800f82d280 R09: 0000000000000073 [ 0.606931] R10: 0000000000000000 R11: ffffc900000dbd68 R12: 0000000000000dc0 [ 0.606931] R13: 00000000000000fe R14: ffffffff81252d01 R15: 009896808086a201 [ 0.606931] ? __register_sysctl_table+0x361/0x500 [ 0.606931] __probe_kernel_read+0x2e/0x60 [ 0.606931] __kmalloc+0x10b/0x230 [ 0.606931] __register_sysctl_table+0x361/0x500 [ 0.606931] ? kmem_cache_alloc_trace+0xee/0x1e0 [ 0.606931] __register_sysctl_paths+0x186/0x1b0 [ 0.606931] ? iomap_init+0x1b/0x1b [ 0.606931] ? do_early_param+0x89/0x89 [ 0.606931] dquot_init+0x23/0x117 [ 0.606931] ? iomap_init+0x1b/0x1b [ 0.606931] do_one_initcall+0x31/0x1b0 [ 0.606931] ? do_early_param+0x89/0x89 [ 0.606931] ? do_early_param+0x89/0x89 [ 0.606931] kernel_init_freeable+0x15b/0x1b3 [ 0.606931] ? rest_init+0x9a/0x9a [ 0.606931] kernel_init+0x5/0xf6 [ 0.606931] ret_from_fork+0x35/0x40 [ 0.606931] ---[ end trace 8ee2a58282a5eb54 ]--- [ 0.650539] general protection fault: 0000 [#1] SMP DEBUG_PAGEALLOC PTI
| |