* WARNING: can't access registers at asm_common_interrupt
@ 2020-11-06 6:04 Shinichiro Kawasaki
2020-11-06 18:06 ` Josh Poimboeuf
2020-11-11 17:05 ` Josh Poimboeuf
0 siblings, 2 replies; 32+ messages in thread
From: Shinichiro Kawasaki @ 2020-11-06 6:04 UTC (permalink / raw)
To: linux-kernel; +Cc: Josh Poimboeuf, Nicholas Piggin, Damien Le Moal
Greetings,
I observe "WARNING: can't access registers at asm_common_interrupt+0x1e/0x40"
in my kernel test system repeatedly, which is printed by unwind_next_frame() in
"arch/x86/kernel/unwind_orc.c". Syzbot already reported that [1]. Similar
warning was reported and discussed [2], but I suppose the cause is not yet
clarified.
The warning was observed with v5.10-rc2 and older tags. I bisected and found
that the commit 044d0d6de9f5 ("lockdep: Only trace IRQ edges") in v5.9-rc3
triggered the warning. Reverting that from 5.10-rc2, the warning disappeared.
May I ask comment by expertise on CC how this commit can relate to the warning?
The test condition to reproduce the warning is rather unique (blktests,
dm-linear and ZNS device emulation by QEMU). If any action is suggested for
further analysis, I'm willing to take it with my test system.
Wish this report helps.
[1] https://lkml.org/lkml/2020/9/6/231
[2] https://lkml.org/lkml/2020/9/8/1538
--
Best Regards,
Shin'ichiro Kawasaki
^ permalink raw reply [flat|nested] 32+ messages in thread
* Re: WARNING: can't access registers at asm_common_interrupt
2020-11-06 6:04 WARNING: can't access registers at asm_common_interrupt Shinichiro Kawasaki
@ 2020-11-06 18:06 ` Josh Poimboeuf
2020-11-09 9:10 ` Shinichiro Kawasaki
2020-11-11 17:05 ` Josh Poimboeuf
1 sibling, 1 reply; 32+ messages in thread
From: Josh Poimboeuf @ 2020-11-06 18:06 UTC (permalink / raw)
To: Shinichiro Kawasaki; +Cc: linux-kernel, Nicholas Piggin, Damien Le Moal
On Fri, Nov 06, 2020 at 06:04:15AM +0000, Shinichiro Kawasaki wrote:
> Greetings,
>
> I observe "WARNING: can't access registers at asm_common_interrupt+0x1e/0x40"
> in my kernel test system repeatedly, which is printed by unwind_next_frame() in
> "arch/x86/kernel/unwind_orc.c". Syzbot already reported that [1]. Similar
> warning was reported and discussed [2], but I suppose the cause is not yet
> clarified.
>
> The warning was observed with v5.10-rc2 and older tags. I bisected and found
> that the commit 044d0d6de9f5 ("lockdep: Only trace IRQ edges") in v5.9-rc3
> triggered the warning. Reverting that from 5.10-rc2, the warning disappeared.
> May I ask comment by expertise on CC how this commit can relate to the warning?
>
> The test condition to reproduce the warning is rather unique (blktests,
> dm-linear and ZNS device emulation by QEMU). If any action is suggested for
> further analysis, I'm willing to take it with my test system.
Hi,
Thanks for reporting this issue. This might be a different issue from
[2].
Can you send me the arch/x86/entry/entry_64.o file from your build?
--
Josh
^ permalink raw reply [flat|nested] 32+ messages in thread
* Re: WARNING: can't access registers at asm_common_interrupt
2020-11-06 18:06 ` Josh Poimboeuf
@ 2020-11-09 9:10 ` Shinichiro Kawasaki
2020-11-10 3:19 ` Josh Poimboeuf
0 siblings, 1 reply; 32+ messages in thread
From: Shinichiro Kawasaki @ 2020-11-09 9:10 UTC (permalink / raw)
To: Josh Poimboeuf; +Cc: linux-kernel, Nicholas Piggin, Damien Le Moal
On Nov 06, 2020 / 12:06, Josh Poimboeuf wrote:
> On Fri, Nov 06, 2020 at 06:04:15AM +0000, Shinichiro Kawasaki wrote:
> > Greetings,
> >
> > I observe "WARNING: can't access registers at asm_common_interrupt+0x1e/0x40"
> > in my kernel test system repeatedly, which is printed by unwind_next_frame() in
> > "arch/x86/kernel/unwind_orc.c". Syzbot already reported that [1]. Similar
> > warning was reported and discussed [2], but I suppose the cause is not yet
> > clarified.
> >
> > The warning was observed with v5.10-rc2 and older tags. I bisected and found
> > that the commit 044d0d6de9f5 ("lockdep: Only trace IRQ edges") in v5.9-rc3
> > triggered the warning. Reverting that from 5.10-rc2, the warning disappeared.
> > May I ask comment by expertise on CC how this commit can relate to the warning?
> >
> > The test condition to reproduce the warning is rather unique (blktests,
> > dm-linear and ZNS device emulation by QEMU). If any action is suggested for
> > further analysis, I'm willing to take it with my test system.
>
> Hi,
>
> Thanks for reporting this issue. This might be a different issue from
> [2].
>
> Can you send me the arch/x86/entry/entry_64.o file from your build?
Hi Josh, thank you for your response. As a separated e-mail, I have sent the
entry_64.o only to your address, since I hesitate to send around the 76kb
attachment file to LKML. In case it does not reach to you, please let me know.
--
Best Regards,
Shin'ichiro Kawasaki
^ permalink raw reply [flat|nested] 32+ messages in thread
* Re: WARNING: can't access registers at asm_common_interrupt
2020-11-09 9:10 ` Shinichiro Kawasaki
@ 2020-11-10 3:19 ` Josh Poimboeuf
2020-11-10 9:19 ` Shinichiro Kawasaki
0 siblings, 1 reply; 32+ messages in thread
From: Josh Poimboeuf @ 2020-11-10 3:19 UTC (permalink / raw)
To: Shinichiro Kawasaki; +Cc: linux-kernel, Nicholas Piggin, Damien Le Moal
On Mon, Nov 09, 2020 at 09:10:38AM +0000, Shinichiro Kawasaki wrote:
> On Nov 06, 2020 / 12:06, Josh Poimboeuf wrote:
> > On Fri, Nov 06, 2020 at 06:04:15AM +0000, Shinichiro Kawasaki wrote:
> > > Greetings,
> > >
> > > I observe "WARNING: can't access registers at asm_common_interrupt+0x1e/0x40"
> > > in my kernel test system repeatedly, which is printed by unwind_next_frame() in
> > > "arch/x86/kernel/unwind_orc.c". Syzbot already reported that [1]. Similar
> > > warning was reported and discussed [2], but I suppose the cause is not yet
> > > clarified.
> > >
> > > The warning was observed with v5.10-rc2 and older tags. I bisected and found
> > > that the commit 044d0d6de9f5 ("lockdep: Only trace IRQ edges") in v5.9-rc3
> > > triggered the warning. Reverting that from 5.10-rc2, the warning disappeared.
> > > May I ask comment by expertise on CC how this commit can relate to the warning?
> > >
> > > The test condition to reproduce the warning is rather unique (blktests,
> > > dm-linear and ZNS device emulation by QEMU). If any action is suggested for
> > > further analysis, I'm willing to take it with my test system.
> >
> > Hi,
> >
> > Thanks for reporting this issue. This might be a different issue from
> > [2].
> >
> > Can you send me the arch/x86/entry/entry_64.o file from your build?
>
> Hi Josh, thank you for your response. As a separated e-mail, I have sent the
> entry_64.o only to your address, since I hesitate to send around the 76kb
> attachment file to LKML. In case it does not reach to you, please let me know.
Got it, thanks. Unfortunately I'm still confused.
Can you test with the following patch, and boot with 'unwind_debug'?
That should hopefully dump a lot of useful data along with the warning.
From: Josh Poimboeuf <jpoimboe@redhat.com>
Subject: [PATCH] x86/unwind/orc: Add 'unwind_debug' cmdline option
Sometimes the one-line ORC unwinder warnings aren't very helpful. Add a
new 'unwind_debug' cmdline option which will dump the full stack
contents of the current task when an error condition is encountered.
Signed-off-by: Josh Poimboeuf <jpoimboe@redhat.com>
Reviewed-by: Miroslav Benes <mbenes@suse.cz>
---
.../admin-guide/kernel-parameters.txt | 6 +++
arch/x86/kernel/unwind_orc.c | 48 ++++++++++++++++++-
2 files changed, 53 insertions(+), 1 deletion(-)
diff --git a/Documentation/admin-guide/kernel-parameters.txt b/Documentation/admin-guide/kernel-parameters.txt
index 526d65d8573a..4bed92c51723 100644
--- a/Documentation/admin-guide/kernel-parameters.txt
+++ b/Documentation/admin-guide/kernel-parameters.txt
@@ -5512,6 +5512,12 @@
unknown_nmi_panic
[X86] Cause panic on unknown NMI.
+ unwind_debug [X86-64]
+ Enable unwinder debug output. This can be
+ useful for debugging certain unwinder error
+ conditions, including corrupt stacks and
+ bad/missing unwinder metadata.
+
usbcore.authorized_default=
[USB] Default USB device authorization:
(default -1 = authorized except for wireless USB,
diff --git a/arch/x86/kernel/unwind_orc.c b/arch/x86/kernel/unwind_orc.c
index 73f800100066..44bae03f9bfc 100644
--- a/arch/x86/kernel/unwind_orc.c
+++ b/arch/x86/kernel/unwind_orc.c
@@ -13,8 +13,13 @@
#define orc_warn_current(args...) \
({ \
- if (state->task == current) \
+ static bool dumped_before; \
+ if (state->task == current) { \
orc_warn(args); \
+ if (unwind_debug && !dumped_before) \
+ unwind_dump(state); \
+ dumped_before = true; \
+ } \
})
extern int __start_orc_unwind_ip[];
@@ -23,8 +28,49 @@ extern struct orc_entry __start_orc_unwind[];
extern struct orc_entry __stop_orc_unwind[];
static bool orc_init __ro_after_init;
+static bool unwind_debug __ro_after_init;
static unsigned int lookup_num_blocks __ro_after_init;
+static int __init unwind_debug_cmdline(char *str)
+{
+ unwind_debug = true;
+
+ return 0;
+}
+early_param("unwind_debug", unwind_debug_cmdline);
+
+static void unwind_dump(struct unwind_state *state)
+{
+ static bool dumped_before;
+ unsigned long word, *sp;
+ struct stack_info stack_info = {0};
+ unsigned long visit_mask = 0;
+
+ if (dumped_before)
+ return;
+
+ dumped_before = true;
+
+ printk_deferred("unwind stack type:%d next_sp:%p mask:0x%lx graph_idx:%d\n",
+ state->stack_info.type, state->stack_info.next_sp,
+ state->stack_mask, state->graph_idx);
+
+ for (sp = __builtin_frame_address(0); sp;
+ sp = PTR_ALIGN(stack_info.next_sp, sizeof(long))) {
+ if (get_stack_info(sp, state->task, &stack_info, &visit_mask))
+ break;
+
+ for (; sp < stack_info.end; sp++) {
+
+ word = READ_ONCE_NOCHECK(*sp);
+
+ printk_deferred("%0*lx: %0*lx (%pB)\n", BITS_PER_LONG/4,
+ (unsigned long)sp, BITS_PER_LONG/4,
+ word, (void *)word);
+ }
+ }
+}
+
static inline unsigned long orc_ip(const int *ip)
{
return (unsigned long)ip + *ip;
--
2.25.4
^ permalink raw reply related [flat|nested] 32+ messages in thread
* Re: WARNING: can't access registers at asm_common_interrupt
2020-11-10 3:19 ` Josh Poimboeuf
@ 2020-11-10 9:19 ` Shinichiro Kawasaki
0 siblings, 0 replies; 32+ messages in thread
From: Shinichiro Kawasaki @ 2020-11-10 9:19 UTC (permalink / raw)
To: Josh Poimboeuf; +Cc: linux-kernel, Nicholas Piggin, Damien Le Moal
On Nov 09, 2020 / 21:19, Josh Poimboeuf wrote:
> On Mon, Nov 09, 2020 at 09:10:38AM +0000, Shinichiro Kawasaki wrote:
> > On Nov 06, 2020 / 12:06, Josh Poimboeuf wrote:
> > > On Fri, Nov 06, 2020 at 06:04:15AM +0000, Shinichiro Kawasaki wrote:
> > > > Greetings,
> > > >
> > > > I observe "WARNING: can't access registers at asm_common_interrupt+0x1e/0x40"
> > > > in my kernel test system repeatedly, which is printed by unwind_next_frame() in
> > > > "arch/x86/kernel/unwind_orc.c". Syzbot already reported that [1]. Similar
> > > > warning was reported and discussed [2], but I suppose the cause is not yet
> > > > clarified.
> > > >
> > > > The warning was observed with v5.10-rc2 and older tags. I bisected and found
> > > > that the commit 044d0d6de9f5 ("lockdep: Only trace IRQ edges") in v5.9-rc3
> > > > triggered the warning. Reverting that from 5.10-rc2, the warning disappeared.
> > > > May I ask comment by expertise on CC how this commit can relate to the warning?
> > > >
> > > > The test condition to reproduce the warning is rather unique (blktests,
> > > > dm-linear and ZNS device emulation by QEMU). If any action is suggested for
> > > > further analysis, I'm willing to take it with my test system.
> > >
> > > Hi,
> > >
> > > Thanks for reporting this issue. This might be a different issue from
> > > [2].
> > >
> > > Can you send me the arch/x86/entry/entry_64.o file from your build?
> >
> > Hi Josh, thank you for your response. As a separated e-mail, I have sent the
> > entry_64.o only to your address, since I hesitate to send around the 76kb
> > attachment file to LKML. In case it does not reach to you, please let me know.
>
> Got it, thanks. Unfortunately I'm still confused.
>
> Can you test with the following patch, and boot with 'unwind_debug'?
> That should hopefully dump a lot of useful data along with the warning.
Thank you for the patch. With the patch and 'unwind_debug' kernel command line,
I recreated the warning. Here I quote the kernel messages printed (smpboot
related messages are usual message printed for blktests test case block/008).
...
[ 113.022680] smpboot: CPU 1 is now offline
[ 113.030546] WARNING: can't access registers at asm_common_interrupt+0x1e/0x40
[ 113.030549] unwind stack type:0 next_sp:0000000000000000 mask:0x6 graph_idx:0
[ 113.030554] ffff8881e87097b0: 1ffff1103d0e1302 (0x1ffff1103d0e1302)
[ 113.030558] ffff8881e87097b8: ffffffff9712a434 (unwind_next_frame+0x15e4/0x1fc0)
[ 113.030560] ffff8881e87097c0: ffffffff99200c5e (asm_common_interrupt+0x1e/0x40)
[ 113.030562] ffff8881e87097c8: 0000000000000000 (0x0)
[ 113.030563] ffff8881e87097d0: ffff8881e87098bd (0xffff8881e87098bd)
[ 113.030564] ffff8881e87097d8: ffff8881e87098d8 (0xffff8881e87098d8)
[ 113.030565] ffff8881e87097e0: ffff8881e87098c0 (0xffff8881e87098c0)
[ 113.030570] ffff8881e87097e8: ffffffff9ac85575 (__start_orc_unwind+0x65f81/0x37a91c)
[ 113.030572] ffff8881e87097f0: ffffffff9ac85575 (__start_orc_unwind+0x65f81/0x37a91c)
[ 113.030573] ffff8881e87097f8: ffff8881e8709938 (0xffff8881e8709938)
[ 113.030574] ffff8881e8709800: 0000000000000000 (0x0)
[ 113.030597] ffff8881e8709808: ffff88810ebd31c0 (0xffff88810ebd31c0)
[ 113.030598] ffff8881e8709810: 0000000041b58ab3 (0x41b58ab3)
[ 113.030602] ffff8881e8709818: ffffffff99d7a850 (.LC2+0x3be/0x44d)
[ 113.030604] ffff8881e8709820: ffffffff97128e50 (deref_stack_reg+0x160/0x160)
[ 113.030605] ffff8881e8709828: 0000000000000000 (0x0)
[ 113.030606] ffff8881e8709830: 0000000000000000 (0x0)
[ 113.030610] ffff8881e8709838: ffffffff972343bf (kernel_text_address.part.0+0x1f/0xc0)
[ 113.030624] ffff8881e8709840: ffffffffc046fd90 (nvme_irq+0x10/0x20 [nvme])
[ 113.030625] ffff8881e8709848: ffff8881e87098d0 (0xffff8881e87098d0)
[ 113.030628] ffff8881e8709850: ffffffff973994c0 (create_prof_cpu_mask+0x20/0x20)
[ 113.030629] ffff8881e8709858: ffff8881e8709908 (0xffff8881e8709908)
[ 113.030630] ffff8881e8709860: ffff8881e8709938 (0xffff8881e8709938)
[ 113.030631] ffff8881e8709868: 0000000000000000 (0x0)
[ 113.030632] ffff8881e8709870: ffff88810ebd31c0 (0xffff88810ebd31c0)
[ 113.030633] ffff8881e8709878: 0000000000000006 (0x6)
[ 113.030636] ffff8881e8709880: ffffffff9709925c (arch_stack_walk+0x6c/0xb0)
[ 113.030637] ffff8881e8709888: 0000000000000000 (0x0)
[ 113.030638] ffff8881e8709890: ffff88814c428000 (0xffff88814c428000)
[ 113.030640] ffff8881e8709898: ffff88814c430000 (0xffff88814c430000)
[ 113.030641] ffff8881e87098a0: 0000000000000000 (0x0)
[ 113.030642] ffff8881e87098a8: 0000000000000006 (0x6)
[ 113.030643] ffff8881e87098b0: ffff88810ebd31c0 (0xffff88810ebd31c0)
[ 113.030644] ffff8881e87098b8: 0000010000000000 (0x10000000000)
[ 113.030645] ffff8881e87098c0: 0000000000000000 (0x0)
[ 113.030646] ffff8881e87098c8: 0000000000000000 (0x0)
[ 113.030648] ffff8881e87098d0: ffffffff99200c5e (asm_common_interrupt+0x1e/0x40)
[ 113.030649] ffff8881e87098d8: ffff88814c42fed0 (0xffff88814c42fed0)
[ 113.030650] ffff8881e87098e0: 0000000000000000 (0x0)
[ 113.030651] ffff8881e87098e8: ffffed103d0e1323 (0xffffed103d0e1323)
[ 113.030652] ffff8881e87098f0: 0000000000000800 (0x800)
[ 113.030653] ffff8881e87098f8: ffff88810b48a780 (0xffff88810b48a780)
[ 113.030654] ffff8881e8709900: ffff8881e8709c48 (0xffff8881e8709c48)
[ 113.030658] ffff8881e8709908: ffffffff97862595 (kmem_cache_free+0xf5/0x590)
[ 113.030660] ffff8881e8709910: ffffffff973996a1 (stack_trace_save+0x81/0xa0)
[ 113.030661] ffff8881e8709918: 0000000041b58ab3 (0x41b58ab3)
[ 113.030663] ffff8881e8709920: ffffffff99d89ef4 (.LC4+0x20f/0x14aa1)
[ 113.030665] ffff8881e8709928: ffffffff97399620 (stack_trace_consume_entry+0x160/0x160)
[ 113.030666] ffff8881e8709930: ffff8881098e9800 (0xffff8881098e9800)
[ 113.030667] ffff8881e8709938: ffff8881e8709988 (0xffff8881e8709988)
[ 113.030668] ffff8881e8709940: 0000000000000040 (0x40)
[ 113.030669] ffff8881e8709948: 0000000000000015 (0x15)
[ 113.030670] ffff8881e8709950: ffff88811858d348 (0xffff88811858d348)
[ 113.030672] ffff8881e8709958: ffffffff97862595 (kmem_cache_free+0xf5/0x590)
[ 113.030673] ffff8881e8709960: 0000000000000800 (0x800)
[ 113.030674] ffff8881e8709968: ffff88810b48a780 (0xffff88810b48a780)
[ 113.030675] ffff8881e8709970: ffff8881e8709c48 (0xffff8881e8709c48)
[ 113.030676] ffff8881e8709978: ffff88811858c008 (0xffff88811858c008)
[ 113.030678] ffff8881e8709980: ffffffff978649db (kasan_save_stack+0x1b/0x40)
[ 113.030680] ffff8881e8709988: ffffffff978649db (kasan_save_stack+0x1b/0x40)
[ 113.030681] ffff8881e8709990: ffffffff97864a1c (kasan_set_track+0x1c/0x30)
[ 113.030684] ffff8881e8709998: ffffffff97866f5b (kasan_set_free_info+0x1b/0x30)
[ 113.030685] ffff8881e87099a0: ffffffff97864980 (__kasan_slab_free+0x110/0x150)
[ 113.030687] ffff8881e87099a8: ffffffff9785b4da (slab_free_freelist_hook+0x5a/0x170)
[ 113.030689] ffff8881e87099b0: ffffffff97862595 (kmem_cache_free+0xf5/0x590)
[ 113.030692] ffff8881e87099b8: ffffffff988f4da0 (dec_pending+0x1f0/0x900)
[ 113.030694] ffff8881e87099c0: ffffffff988fad15 (clone_endio+0x1e5/0x940)
[ 113.030697] ffff8881e87099c8: ffffffff97eac8f6 (blk_update_request+0x716/0xe20)
[ 113.030699] ffff8881e87099d0: ffffffff97ed523b (blk_mq_end_request+0x4b/0x480)
[ 113.030701] ffff8881e87099d8: ffffffffc046e863 (nvme_process_cq+0x563/0xa40 [nvme])
[ 113.030703] ffff8881e87099e0: ffffffffc046fd90 (nvme_irq+0x10/0x20 [nvme])
[ 113.030706] ffff8881e87099e8: ffffffff9733ef52 (__handle_irq_event_percpu+0x252/0x620)
[ 113.030707] ffff8881e87099f0: ffffffff9733f51f (handle_irq_event+0xef/0x240)
[ 113.030709] ffff8881e87099f8: ffffffff9734bff6 (handle_edge_irq+0x1f6/0xb60)
[ 113.030712] ffff8881e8709a00: ffffffff992010b2 (asm_call_irq_on_stack+0x12/0x20)
[ 113.030714] ffff8881e8709a08: ffffffff9918f05b (common_interrupt+0xeb/0x190)
[ 113.030716] ffff8881e8709a10: ffffffff99200c5e (asm_common_interrupt+0x1e/0x40)
[ 113.030717] ffff8881e8709a18: ffffffff97395f70 (exit_to_user_mode_prepare+0xc0/0x1d0)
[ 113.030719] ffff8881e8709a20: ffffffff99200c48 (asm_common_interrupt+0x8/0x40)
[ 113.030721] ffff8881e8709a28: ffffffff99200c5e (asm_common_interrupt+0x1e/0x40)
[ 113.030723] ffff8881e8709a30: ffffffff991a91e0 (schedule+0xd0/0x270)
[ 113.030724] ffff8881e8709a38: ffffffff97395fb7 (exit_to_user_mode_prepare+0x107/0x1d0)
[ 113.030725] ffff8881e8709a40: 1ffff1103d0e134e (0x1ffff1103d0e134e)
[ 113.030727] ffff8881e8709a48: ffff8881e8733dc8 (0xffff8881e8733dc8)
[ 113.030728] ffff8881e8709a50: ffff8881e8733d40 (0xffff8881e8733d40)
[ 113.030729] ffff8881e8709a58: 00000019d79785f8 (0x19d79785f8)
[ 113.030730] ffff8881e8709a60: 0000000000000005 (0x5)
[ 113.030731] ffff8881e8709a68: 0000000000000000 (0x0)
[ 113.030732] ffff8881e8709a70: ffff88810b579980 (0xffff88810b579980)
[ 113.030733] ffff8881e8709a78: ffff88810b579800 (0xffff88810b579800)
[ 113.030736] ffff8881e8709a80: ffffffff97286cda (update_load_avg+0x1fa/0x1ad0)
[ 113.030737] ffff8881e8709a88: ffff88810b57a000 (0xffff88810b57a000)
[ 113.030738] ffff8881e8709a90: ffff88810b579800 (0xffff88810b579800)
[ 113.030739] ffff8881e8709a98: 0000000000000400 (0x400)
[ 113.030741] ffff8881e8709aa0: ffffffff9729be38 (update_cfs_group+0x118/0x290)
[ 113.030743] ffff8881e8709aa8: ffffffff973146a0 (lock_downgrade+0x6a0/0x6a0)
[ 113.030744] ffff8881e8709ab0: 0000000000000000 (0x0)
[ 113.030745] ffff8881e8709ab8: ffff88810b579850 (0xffff88810b579850)
[ 113.030746] ffff8881e8709ac0: ffff8881e8733d68 (0xffff8881e8733d68)
[ 113.030747] ffff8881e8709ac8: ffff8881e8733d40 (0xffff8881e8733d40)
[ 113.030748] ffff8881e8709ad0: 0000000000000001 (0x1)
[ 113.030749] ffff8881e8709ad8: ffff88810b579810 (0xffff88810b579810)
[ 113.030751] ffff8881e8709ae0: ffffffff972a14b2 (enqueue_entity+0x402/0x2ba0)
[ 113.030752] ffff8881e8709ae8: ffff8881e8733d50 (0xffff8881e8733d50)
[ 113.030753] ffff8881e8709af0: ffff88810b579800 (0xffff88810b579800)
[ 113.030754] ffff8881e8709af8: ffff8881e8733d80 (0xffff8881e8733d80)
[ 113.030755] ffff8881e8709b00: ffff888116873268 (0xffff888116873268)
[ 113.030757] ffff8881e8709b08: ffff8881e8734790 (0xffff8881e8734790)
[ 113.030758] ffff8881e8709b10: ffff88810ebd3268 (0xffff88810ebd3268)
[ 113.030759] ffff8881e8709b18: ffff8881e8720e80 (0xffff8881e8720e80)
[ 113.030760] ffff8881e8709b20: ffff88810b579800 (0xffff88810b579800)
[ 113.030761] ffff8881e8709b28: ffff88810b579950 (0xffff88810b579950)
[ 113.030762] ffff8881e8709b30: 0000000000000009 (0x9)
[ 113.030763] ffff8881e8709b38: ffff8881e8733c80 (0xffff8881e8733c80)
[ 113.030764] ffff8881e8709b40: ffff88810ebd31c0 (0xffff88810ebd31c0)
[ 113.030765] ffff8881e8709b48: 0000000000000002 (0x2)
[ 113.030768] ffff8881e8709b50: ffffffff97263aef (resched_curr+0x17f/0x1e0)
[ 113.030769] ffff8881e8709b58: ffff8881098e9000 (0xffff8881098e9000)
[ 113.030770] ffff8881e8709b60: ffff8881098e9038 (0xffff8881098e9038)
[ 113.030771] ffff8881e8709b68: 000000000056e125 (0x56e125)
[ 113.030772] ffff8881e8709b70: 0000000000000002 (0x2)
[ 113.030773] ffff8881e8709b78: 1ffff1103d0e1374 (0x1ffff1103d0e1374)
[ 113.030774] ffff8881e8709b80: ffff888116873e20 (0xffff888116873e20)
[ 113.030775] ffff8881e8709b88: ffff88811858c000 (0xffff88811858c000)
[ 113.030777] ffff8881e8709b90: ffffffff97864a1c (kasan_set_track+0x1c/0x30)
[ 113.030778] ffff8881e8709b98: 1ffff110230b1800 (0x1ffff110230b1800)
[ 113.030780] ffff8881e8709ba0: ffffffff97866f5b (kasan_set_free_info+0x1b/0x30)
[ 113.030781] ffff8881e8709ba8: 0000000000000001 (0x1)
[ 113.030782] ffff8881e8709bb0: ffffffff97864980 (__kasan_slab_free+0x110/0x150)
[ 113.030783] ffff8881e8709bb8: ffff88810b48a780 (0xffff88810b48a780)
[ 113.030785] ffff8881e8709bc0: ffff88811858c000 (0xffff88811858c000)
[ 113.030786] ffff8881e8709bc8: 3b9c1e2c25148a56 (0x3b9c1e2c25148a56)
[ 113.030787] ffff8881e8709bd0: ffff88811858c000 (0xffff88811858c000)
[ 113.030788] ffff8881e8709bd8: ffffffff9785b4da (slab_free_freelist_hook+0x5a/0x170)
[ 113.030789] ffff8881e8709be0: ffff8881e8709c50 (0xffff8881e8709c50)
[ 113.030790] ffff8881e8709be8: ffff88819858c000 (0xffff88819858c000)
[ 113.030792] ffff8881e8709bf0: ffff88810b48a780 (0xffff88810b48a780)
[ 113.030793] ffff8881e8709bf8: ffffea0004616300 (0xffffea0004616300)
[ 113.030794] ffff8881e8709c00: ffff88811858c000 (0xffff88811858c000)
[ 113.030795] ffff8881e8709c08: 0000000000000000 (0x0)
[ 113.030796] ffff8881e8709c10: ffff8881059402a0 (0xffff8881059402a0)
[ 113.030797] ffff8881e8709c18: ffffffff97862595 (kmem_cache_free+0xf5/0x590)
[ 113.030799] ffff8881e8709c20: ffff888114d87038 (0xffff888114d87038)
[ 113.030799] ffff8881e8709c28: 0000000000000000 (0x0)
[ 113.030801] ffff8881e8709c30: ffffffff988f4da0 (dec_pending+0x1f0/0x900)
[ 113.030802] ffff8881e8709c38: ffff8881059403b8 (0xffff8881059403b8)
[ 113.030803] ffff8881e8709c40: ffff88811858c0a8 (0xffff88811858c0a8)
[ 113.030804] ffff8881e8709c48: 0000000000000000 (0x0)
[ 113.030805] ffff8881e8709c50: 0000000000000000 (0x0)
[ 113.030806] ffff8881e8709c58: ffff88811858c000 (0xffff88811858c000)
[ 113.030807] ffff8881e8709c60: ffff88814c427ab8 (0xffff88814c427ab8)
[ 113.030808] ffff8881e8709c68: ffff888105940000 (0xffff888105940000)
[ 113.030809] ffff8881e8709c70: 0000000000000000 (0x0)
[ 113.030810] ffff8881e8709c78: ffff88814c427ac8 (0xffff88814c427ac8)
[ 113.030811] ffff8881e8709c80: ffff8881059402a0 (0xffff8881059402a0)
[ 113.030813] ffff8881e8709c88: ffffffff988f4da0 (dec_pending+0x1f0/0x900)
[ 113.030814] ffff8881e8709c90: ffff888105940000 (0xffff888105940000)
[ 113.030815] ffff8881e8709c98: 00000000fffd21a8 (0xfffd21a8)
[ 113.030816] ffff8881e8709ca0: 0000000400000000 (0x400000000)
[ 113.030817] ffff8881e8709ca8: ffff88814c427ac0 (0xffff88814c427ac0)
[ 113.030818] ffff8881e8709cb0: 1ffff1103d0e139c (0x1ffff1103d0e139c)
[ 113.030819] ffff8881e8709cb8: 1ffff1103d0e13a0 (0x1ffff1103d0e13a0)
[ 113.030820] ffff8881e8709cc0: ffff88811858c0a8 (0xffff88811858c0a8)
[ 113.030822] ffff8881e8709cc8: ffff88814c427ab8 (0xffff88814c427ab8)
[ 113.030823] ffff8881e8709cd0: ffff88811858c000 (0xffff88811858c000)
[ 113.030824] ffff8881e8709cd8: 0000000000000000 (0x0)
[ 113.030825] ffff8881e8709ce0: ffff88811858c088 (0xffff88811858c088)
[ 113.030826] ffff8881e8709ce8: ffffffff988fad15 (clone_endio+0x1e5/0x940)
[ 113.030827] ffff8881e8709cf0: ffff888102a26c38 (0xffff888102a26c38)
[ 113.030828] ffff8881e8709cf8: ffff88811858c0b0 (0xffff88811858c0b0)
[ 113.030829] ffff8881e8709d00: 0000000041b58ab3 (0x41b58ab3)
[ 113.030831] ffff8881e8709d08: ffffffff99e07e8f (.LC0+0x31f92/0x36703)
[ 113.030833] ffff8881e8709d10: ffffffff988fab30 (disable_discard+0xd0/0xd0)
[ 113.030834] ffff8881e8709d18: ffff88811858c0a8 (0xffff88811858c0a8)
[ 113.030835] ffff8881e8709d20: ffff88811858c000 (0xffff88811858c000)
[ 113.030836] ffff8881e8709d28: ffff88811858c0e0 (0xffff88811858c0e0)
[ 113.030837] ffff8881e8709d30: ffff88811858c0b0 (0xffff88811858c0b0)
[ 113.030838] ffff8881e8709d38: ffff88811a753300 (0xffff88811a753300)
[ 113.030840] ffff8881e8709d40: dffffc0000000000 (0xdffffc0000000000)
[ 113.030841] ffff8881e8709d48: ffff88811858c0a8 (0xffff88811858c0a8)
[ 113.030842] ffff8881e8709d50: ffff88811a75331c (0xffff88811a75331c)
[ 113.030843] ffff8881e8709d58: 0000000000001000 (0x1000)
[ 113.030844] ffff8881e8709d60: 0000000000001000 (0x1000)
[ 113.030845] ffff8881e8709d68: ffff88811a753300 (0xffff88811a753300)
[ 113.030846] ffff8881e8709d70: ffffffff97eac8f6 (blk_update_request+0x716/0xe20)
[ 113.030847] ffff8881e8709d78: ffffffff00000000 (0xffffffff00000000)
[ 113.030848] ffff8881e8709d80: 0000000000000000 (0x0)
[ 113.030849] ffff8881e8709d88: ffffed10234ea667 (0xffffed10234ea667)
[ 113.030851] ffff8881e8709d90: ffff88811858c0d0 (0xffff88811858c0d0)
[ 113.030852] ffff8881e8709d98: ffffed10234ea663 (0xffffed10234ea663)
[ 113.030853] ffff8881e8709da0: ffff88811a753318 (0xffff88811a753318)
[ 113.030854] ffff8881e8709da8: ffff88811a753338 (0xffff88811a753338)
[ 113.030855] ffff8881e8709db0: ffff888111788800 (0xffff888111788800)
[ 113.030856] ffff8881e8709db8: dffffc0000000000 (0xdffffc0000000000)
[ 113.030857] ffff8881e8709dc0: ffff88811a753300 (0xffff88811a753300)
[ 113.030858] ffff8881e8709dc8: 0000000000000000 (0x0)
[ 113.030859] ffff8881e8709dd0: ffff88811a753300 (0xffff88811a753300)
[ 113.030860] ffff8881e8709dd8: ffff888116c828c0 (0xffff888116c828c0)
[ 113.030861] ffff8881e8709de0: ffffed1020316ea8 (0xffffed1020316ea8)
[ 113.030863] ffff8881e8709de8: ffffffff97ed523b (blk_mq_end_request+0x4b/0x480)
[ 113.030864] ffff8881e8709df0: dffffc0000000000 (0xdffffc0000000000)
[ 113.030865] ffff8881e8709df8: dffffc0000000000 (0xdffffc0000000000)
[ 113.030866] ffff8881e8709e00: ffff8881018b7480 (0xffff8881018b7480)
[ 113.030867] ffff8881e8709e08: 0000000000000000 (0x0)
[ 113.030868] ffff8881e8709e10: ffff88811a753300 (0xffff88811a753300)
[ 113.030869] ffff8881e8709e18: ffff888116c828c0 (0xffff888116c828c0)
[ 113.030871] ffff8881e8709e20: ffffffffc046e863 (nvme_process_cq+0x563/0xa40 [nvme])
[ 113.030872] ffff8881e8709e28: ffffed1020316ead (0xffffed1020316ead)
[ 113.030873] ffff8881e8709e30: ffff888116c828cc (0xffff888116c828cc)
[ 113.030874] ffff8881e8709e38: 0000000197314fd1 (0x197314fd1)
[ 113.030875] ffff8881e8709e40: ffff8881018b756c (0xffff8881018b756c)
[ 113.030876] ffff8881e8709e48: ffff8881018b7560 (0xffff8881018b7560)
[ 113.030878] ffff8881e8709e50: ffff8881018b756a (0xffff8881018b756a)
[ 113.030879] ffff8881e8709e58: ffff88810e3c98c0 (0xffff88810e3c98c0)
[ 113.030880] ffff8881e8709e60: ffff8881018b7540 (0xffff8881018b7540)
[ 113.030881] ffff8881e8709e68: ffff8881018b7568 (0xffff8881018b7568)
[ 113.030883] ffff8881e8709e70: ffffffff9733f50a (handle_irq_event+0xda/0x240)
[ 113.030884] ffff8881e8709e78: dffffc0000000000 (0xdffffc0000000000)
[ 113.030886] ffff8881e8709e80: ffffffffc046fd80 (nvme_suspend+0x330/0x330 [nvme])
[ 113.030887] ffff8881e8709e88: ffff8881e8709f48 (0xffff8881e8709f48)
[ 113.030888] ffff8881e8709e90: ffff88810e3c9800 (0xffff88810e3c9800)
[ 113.030889] ffff8881e8709e98: ffff8881152e7f00 (0xffff8881152e7f00)
[ 113.030890] ffff8881e8709ea0: 000000000000001c (0x1c)
[ 113.030892] ffff8881e8709ea8: ffffffffc046fd90 (nvme_irq+0x10/0x20 [nvme])
[ 113.030894] ffff8881e8709eb0: ffffffff9733ef52 (__handle_irq_event_percpu+0x252/0x620)
[ 113.030895] ffff8881e8709eb8: 0000000000000002 (0x2)
[ 113.030896] ffff8881e8709ec0: 0000000000000000 (0x0)
[ 113.030897] ffff8881e8709ec8: ffffed1021c7930f (0xffffed1021c7930f)
[ 113.030898] ffff8881e8709ed0: ffff88810ebd3eb8 (0xffff88810ebd3eb8)
[ 113.030899] ffff8881e8709ed8: ffff88810ebd31c0 (0xffff88810ebd31c0)
[ 113.030900] ffff8881e8709ee0: ffff88810e3c9878 (0xffff88810e3c9878)
[ 113.030901] ffff8881e8709ee8: ffff88810e3c9800 (0xffff88810e3c9800)
[ 113.030902] ffff8881e8709ef0: 1ffff1103d0e13e5 (0x1ffff1103d0e13e5)
[ 113.030903] ffff8881e8709ef8: ffff88810e3c9800 (0xffff88810e3c9800)
[ 113.030904] ffff8881e8709f00: ffff88810e3c9838 (0xffff88810e3c9838)
[ 113.030906] ffff8881e8709f08: ffff88810e3c98a8 (0xffff88810e3c98a8)
[ 113.030907] ffff8881e8709f10: ffff88810e3c9800 (0xffff88810e3c9800)
[ 113.030908] ffff8881e8709f18: ffffffff9733f51f (handle_irq_event+0xef/0x240)
[ 113.030909] ffff8881e8709f20: 0000000000000002 (0x2)
[ 113.030910] ffff8881e8709f28: 0000000041b58ab3 (0x41b58ab3)
[ 113.030912] ffff8881e8709f30: ffffffff99d88063 (.LC0+0x10d5/0x2d57)
[ 113.030914] ffff8881e8709f38: ffffffff9733f430 (handle_irq_event_percpu+0x110/0x110)
[ 113.030915] ffff8881e8709f40: 0000000000000000 (0x0)
[ 113.030916] ffff8881e8709f48: 0000000000000000 (0x0)
[ 113.030917] ffff8881e8709f50: ffff88810e3c98a8 (0xffff88810e3c98a8)
[ 113.030918] ffff8881e8709f58: ffff88810e3c987c (0xffff88810e3c987c)
[ 113.030919] ffff8881e8709f60: 0000000000000000 (0x0)
[ 113.030920] ffff8881e8709f68: ffffed1021c7930e (0xffffed1021c7930e)
[ 113.030921] ffff8881e8709f70: ffff88810e3c9838 (0xffff88810e3c9838)
[ 113.030922] ffff8881e8709f78: ffff88810e3c987c (0xffff88810e3c987c)
[ 113.030923] ffff8881e8709f80: ffffed1021c7930f (0xffffed1021c7930f)
[ 113.030925] ffff8881e8709f88: dffffc0000000000 (0xdffffc0000000000)
[ 113.030926] ffff8881e8709f90: ffffffff9734bff6 (handle_edge_irq+0x1f6/0xb60)
[ 113.030927] ffff8881e8709f98: ffff88810e3c98a8 (0xffff88810e3c98a8)
[ 113.030928] ffff8881e8709fa0: ffff88810e3c9870 (0xffff88810e3c9870)
[ 113.030929] ffff8881e8709fa8: ffff88810e3c9840 (0xffff88810e3c9840)
[ 113.030930] ffff8881e8709fb0: ffff88810e3c9828 (0xffff88810e3c9828)
[ 113.030931] ffff8881e8709fb8: 0000000000000000 (0x0)
[ 113.030932] ffff8881e8709fc0: 0000000000000023 (0x23)
[ 113.030933] ffff8881e8709fc8: ffff88814c42fe30 (0xffff88814c42fe30)
[ 113.030934] ffff8881e8709fd0: ffff88810e3c9800 (0xffff88810e3c9800)
[ 113.030935] ffff8881e8709fd8: 0000000000000000 (0x0)
[ 113.030936] ffff8881e8709fe0: 0000000000000000 (0x0)
[ 113.030937] ffff8881e8709fe8: 0000000000000023 (0x23)
[ 113.030939] ffff8881e8709ff0: ffffffff992010b2 (asm_call_irq_on_stack+0x12/0x20)
[ 113.030940] ffff8881e8709ff8: ffff88814c42fe30 (0xffff88814c42fe30)
[ 113.030942] ffff88814c42fe30: ffff88814c42fe78 (0xffff88814c42fe78)
[ 113.030943] ffff88814c42fe38: ffffffff9918f05b (common_interrupt+0xeb/0x190)
[ 113.030944] ffff88814c42fe40: 0000000000000000 (0x0)
[ 113.030945] ffff88814c42fe48: 0000000000000000 (0x0)
[ 113.030946] ffff88814c42fe50: 0000000000000000 (0x0)
[ 113.030947] ffff88814c42fe58: 0000000000000000 (0x0)
[ 113.030948] ffff88814c42fe60: 0000000000000000 (0x0)
[ 113.030949] ffff88814c42fe68: 0000000000000000 (0x0)
[ 113.030950] ffff88814c42fe70: ffffffff99200c5e (asm_common_interrupt+0x1e/0x40)
[ 113.030951] ffff88814c42fe78: 0000000000000000 (0x0)
[ 113.030952] ffff88814c42fe80: 0000000000000000 (0x0)
[ 113.030953] ffff88814c42fe88: 0000000000000000 (0x0)
[ 113.030954] ffff88814c42fe90: ffff88810ebd31c0 (0xffff88810ebd31c0)
[ 113.030955] ffff88814c42fe98: ffff88814c42ff58 (0xffff88814c42ff58)
[ 113.030956] ffff88814c42fea0: 0000000000000000 (0x0)
[ 113.030957] ffff88814c42fea8: 0000000000000001 (0x1)
[ 113.030958] ffff88814c42feb0: ffffed1021d7a638 (0xffffed1021d7a638)
[ 113.030959] ffff88814c42feb8: ffff88810ebd31c7 (0xffff88810ebd31c7)
[ 113.030960] ffff88814c42fec0: 0000000000000000 (0x0)
[ 113.030961] ffff88814c42fec8: 0000000000400140 (0x400140)
[ 113.030963] ffff88814c42fed0: ffffffff991a91f4 (schedule+0xe4/0x270)
[ 113.030963] ffff88814c42fed8: 0000000000000000 (0x0)
[ 113.030964] ffff88814c42fee0: 0000000000000008 (0x8)
[ 113.030965] ffff88814c42fee8: ffff88810ebd31c0 (0xffff88810ebd31c0)
[ 113.030967] ffff88814c42fef0: ffffffffffffffff (0xffffffffffffffff)
[ 113.030968] ffff88814c42fef8: ffffffff97395f70 (exit_to_user_mode_prepare+0xc0/0x1d0)
[ 113.030969] ffff88814c42ff00: 0000000000000010 (0x10)
[ 113.030970] ffff88814c42ff08: 0000000000000246 (0x246)
[ 113.030971] ffff88814c42ff10: ffff88814c42ff28 (0xffff88814c42ff28)
[ 113.030972] ffff88814c42ff18: 0000000000000018 (0x18)
[ 113.030973] ffff88814c42ff20: 0000000000000000 (0x0)
[ 113.030974] ffff88814c42ff28: 0000000000000246 (0x246)
[ 113.030975] ffff88814c42ff30: 0000000000000000 (0x0)
[ 113.030976] ffff88814c42ff38: 0000000000000000 (0x0)
[ 113.030978] ffff88814c42ff40: ffffffff99200c48 (asm_common_interrupt+0x8/0x40)
[ 113.030980] ffff88814c42ff48: ffffffff99191c95 (irqentry_exit_to_user_mode+0x5/0x40)
[ 113.030981] ffff88814c42ff50: ffffffff99200c5e (asm_common_interrupt+0x1e/0x40)
[ 113.030982] ffff88814c42ff58: 0000000000aaa828 (0xaaa828)
[ 113.030983] ffff88814c42ff60: 0000000000001000 (0x1000)
[ 113.030984] ffff88814c42ff68: 0000000000000000 (0x0)
[ 113.030985] ffff88814c42ff70: 00007f02266d1460 (0x7f02266d1460)
[ 113.030986] ffff88814c42ff78: 0000000000aaa800 (0xaaa800)
[ 113.030987] ffff88814c42ff80: 0000000000aaa800 (0xaaa800)
[ 113.030988] ffff88814c42ff88: 0000000000000293 (0x293)
[ 113.030989] ffff88814c42ff90: 0000000000b59000 (0xb59000)
[ 113.030990] ffff88814c42ff98: 00007ffdf02c4090 (0x7ffdf02c4090)
[ 113.030991] ffff88814c42ffa0: 0000000000000000 (0x0)
[ 113.030992] ffff88814c42ffa8: 0000000000001000 (0x1000)
[ 113.030994] ffff88814c42ffb0: 00007f028a34124f (0x7f028a34124f)
[ 113.030995] ffff88814c42ffb8: 0000000000001000 (0x1000)
[ 113.030995] ffff88814c42ffc0: 0000000000abd000 (0xabd000)
[ 113.030996] ffff88814c42ffc8: 0000000000000007 (0x7)
[ 113.030998] ffff88814c42ffd0: ffffffffffffffff (0xffffffffffffffff)
[ 113.030999] ffff88814c42ffd8: 00007f028a34124f (0x7f028a34124f)
[ 113.031000] ffff88814c42ffe0: 0000000000000033 (0x33)
[ 113.031000] ffff88814c42ffe8: 0000000000000293 (0x293)
[ 113.031002] ffff88814c42fff0: 00007ffdf01f96d0 (0x7ffdf01f96d0)
[ 113.031002] ffff88814c42fff8: 000000000000002b (0x2b)
[ 115.105359] smpboot: CPU 3 is now offline
[ 115.359319] smpboot: CPU 2 is now offline
...
--
Best Regards,
Shin'ichiro Kawasaki
^ permalink raw reply [flat|nested] 32+ messages in thread
* Re: WARNING: can't access registers at asm_common_interrupt
2020-11-06 6:04 WARNING: can't access registers at asm_common_interrupt Shinichiro Kawasaki
2020-11-06 18:06 ` Josh Poimboeuf
@ 2020-11-11 17:05 ` Josh Poimboeuf
2020-11-11 17:47 ` Peter Zijlstra
1 sibling, 1 reply; 32+ messages in thread
From: Josh Poimboeuf @ 2020-11-11 17:05 UTC (permalink / raw)
To: Shinichiro Kawasaki
Cc: linux-kernel, Nicholas Piggin, Damien Le Moal, Peter Zijlstra
On Fri, Nov 06, 2020 at 06:04:15AM +0000, Shinichiro Kawasaki wrote:
> Greetings,
>
> I observe "WARNING: can't access registers at asm_common_interrupt+0x1e/0x40"
> in my kernel test system repeatedly, which is printed by unwind_next_frame() in
> "arch/x86/kernel/unwind_orc.c". Syzbot already reported that [1]. Similar
> warning was reported and discussed [2], but I suppose the cause is not yet
> clarified.
>
> The warning was observed with v5.10-rc2 and older tags. I bisected and found
> that the commit 044d0d6de9f5 ("lockdep: Only trace IRQ edges") in v5.9-rc3
> triggered the warning. Reverting that from 5.10-rc2, the warning disappeared.
> May I ask comment by expertise on CC how this commit can relate to the warning?
>
> The test condition to reproduce the warning is rather unique (blktests,
> dm-linear and ZNS device emulation by QEMU). If any action is suggested for
> further analysis, I'm willing to take it with my test system.
>
> Wish this report helps.
>
> [1] https://lkml.org/lkml/2020/9/6/231
> [2] https://lkml.org/lkml/2020/9/8/1538
Shin'ichiro,
Thanks for all the data. It looks like the ORC unwinder is getting
confused by paravirt patching (with runtime-patched pushf/pop changing
the stack layout).
<user interrupt>
exit_to_user_mode_prepare()
exit_to_user_mode_loop()
local_irq_disable_exit_to_user()
local_irq_disable()
raw_irqs_disabled()
arch_irqs_disabled()
arch_local_save_flags()
pushfq
<another interrupt>
Objtool doesn't know about the pushf/pop paravirt patch, so ORC gets
confused by the changed stack layout.
I'm thinking we either need to teach objtool how to deal with
save_fl/restore_fl patches, or we need to just get rid of those nasty
patches somehow. Peter, any thoughts?
It looks like 044d0d6de9f5 ("lockdep: Only trace IRQ edges") is making
the problem more likely, by adding the irqs_disabled() check for every
local_irq_disable().
Also - Peter, Nicholas - is that irqs_disabled() check really necessary
in local_irq_disable()? Presumably irqs would typically be be enabled
before calling it?
--
Josh
^ permalink raw reply [flat|nested] 32+ messages in thread
* Re: WARNING: can't access registers at asm_common_interrupt
2020-11-11 17:05 ` Josh Poimboeuf
@ 2020-11-11 17:47 ` Peter Zijlstra
2020-11-11 18:13 ` Josh Poimboeuf
0 siblings, 1 reply; 32+ messages in thread
From: Peter Zijlstra @ 2020-11-11 17:47 UTC (permalink / raw)
To: Josh Poimboeuf
Cc: Shinichiro Kawasaki, linux-kernel, Nicholas Piggin,
Damien Le Moal, andrew.cooper3, jgross
On Wed, Nov 11, 2020 at 11:05:36AM -0600, Josh Poimboeuf wrote:
> On Fri, Nov 06, 2020 at 06:04:15AM +0000, Shinichiro Kawasaki wrote:
> > Greetings,
> >
> > I observe "WARNING: can't access registers at asm_common_interrupt+0x1e/0x40"
> > in my kernel test system repeatedly, which is printed by unwind_next_frame() in
> > "arch/x86/kernel/unwind_orc.c". Syzbot already reported that [1]. Similar
> > warning was reported and discussed [2], but I suppose the cause is not yet
> > clarified.
> >
> > The warning was observed with v5.10-rc2 and older tags. I bisected and found
> > that the commit 044d0d6de9f5 ("lockdep: Only trace IRQ edges") in v5.9-rc3
> > triggered the warning. Reverting that from 5.10-rc2, the warning disappeared.
> > May I ask comment by expertise on CC how this commit can relate to the warning?
> >
> > The test condition to reproduce the warning is rather unique (blktests,
> > dm-linear and ZNS device emulation by QEMU). If any action is suggested for
> > further analysis, I'm willing to take it with my test system.
> >
> > Wish this report helps.
> >
> > [1] https://lkml.org/lkml/2020/9/6/231
> > [2] https://lkml.org/lkml/2020/9/8/1538
>
> Shin'ichiro,
>
> Thanks for all the data. It looks like the ORC unwinder is getting
> confused by paravirt patching (with runtime-patched pushf/pop changing
> the stack layout).
>
> <user interrupt>
> exit_to_user_mode_prepare()
> exit_to_user_mode_loop()
> local_irq_disable_exit_to_user()
> local_irq_disable()
> raw_irqs_disabled()
> arch_irqs_disabled()
> arch_local_save_flags()
> pushfq
> <another interrupt>
This is PARAVIRT_XXL only, which is a Xen special. My preference, as
always, is to kill it... Sadly the Xen people have a different opinion.
> Objtool doesn't know about the pushf/pop paravirt patch, so ORC gets
> confused by the changed stack layout.
>
> I'm thinking we either need to teach objtool how to deal with
> save_fl/restore_fl patches, or we need to just get rid of those nasty
> patches somehow. Peter, any thoughts?
Don't use Xen? ;-)
So with PARAVIRT_XXL the compiler will emit something like:
"CALL *pvops.save_fl"
Which we then overwrite at runtime with "pushf; pop %[re]ax" and a few
NOPs.
Now, objtool understands alternatives, and ensures they have the same
stack layout, it has no chance in hell of understanding this, simply
because paravirt_patch.c is magic.
I don't have any immediate clever ideas, but let me ponder it a wee bit.
....
Something really disguisting we could do is recognise the indirect call
offset and emit an extra ORC entry for RIP+1. So the cases are:
CALL *pv_ops.save_fl -- 7 bytes IIRC
CALL $imm; -- 5 bytes
PUSHF; POP %[RE]AX -- 2 bytes
so the RIP+1 (the POP insn) will only ever exist in this case. The
indirect and direct call cases would never land on that IP.
....
> It looks like 044d0d6de9f5 ("lockdep: Only trace IRQ edges") is making
> the problem more likely, by adding the irqs_disabled() check for every
> local_irq_disable().
>
> Also - Peter, Nicholas - is that irqs_disabled() check really necessary
> in local_irq_disable()? Presumably irqs would typically be be enabled
> before calling it?
Yeah, so it's all a giant can of worms that; also see:
https://lkml.kernel.org/r/20200821084738.508092956@infradead.org
The basic idea is to only trace edges, ie. when the hardware state
actually changes. Sadly this means doing a pushf/pop before the cli.
Ideally CLI would store the old IF in CF or something like that, but
alas.
^ permalink raw reply [flat|nested] 32+ messages in thread
* Re: WARNING: can't access registers at asm_common_interrupt
2020-11-11 17:47 ` Peter Zijlstra
@ 2020-11-11 18:13 ` Josh Poimboeuf
2020-11-11 18:46 ` Andrew Cooper
2020-11-11 20:42 ` Peter Zijlstra
0 siblings, 2 replies; 32+ messages in thread
From: Josh Poimboeuf @ 2020-11-11 18:13 UTC (permalink / raw)
To: Peter Zijlstra
Cc: Shinichiro Kawasaki, linux-kernel, Nicholas Piggin,
Damien Le Moal, andrew.cooper3, jgross
On Wed, Nov 11, 2020 at 06:47:36PM +0100, Peter Zijlstra wrote:
> This is PARAVIRT_XXL only, which is a Xen special. My preference, as
> always, is to kill it... Sadly the Xen people have a different opinion.
That would be soooo nice... then we could get rid of paravirt patching
altogether and replace it with static calls.
> > Objtool doesn't know about the pushf/pop paravirt patch, so ORC gets
> > confused by the changed stack layout.
> >
> > I'm thinking we either need to teach objtool how to deal with
> > save_fl/restore_fl patches, or we need to just get rid of those nasty
> > patches somehow. Peter, any thoughts?
>
> Don't use Xen? ;-)
>
> So with PARAVIRT_XXL the compiler will emit something like:
>
> "CALL *pvops.save_fl"
>
> Which we then overwrite at runtime with "pushf; pop %[re]ax" and a few
> NOPs.
>
> Now, objtool understands alternatives, and ensures they have the same
> stack layout, it has no chance in hell of understanding this, simply
> because paravirt_patch.c is magic.
>
> I don't have any immediate clever ideas, but let me ponder it a wee bit.
>
> ....
>
> Something really disguisting we could do is recognise the indirect call
> offset and emit an extra ORC entry for RIP+1. So the cases are:
>
> CALL *pv_ops.save_fl -- 7 bytes IIRC
> CALL $imm; -- 5 bytes
> PUSHF; POP %[RE]AX -- 2 bytes
>
> so the RIP+1 (the POP insn) will only ever exist in this case. The
> indirect and direct call cases would never land on that IP.
I had a similar idea, and a bit of deja vu - we may have talked about
this before. At least I know we talked about doing something similar
for alternatives which muck with the stack.
> > It looks like 044d0d6de9f5 ("lockdep: Only trace IRQ edges") is making
> > the problem more likely, by adding the irqs_disabled() check for every
> > local_irq_disable().
> >
> > Also - Peter, Nicholas - is that irqs_disabled() check really necessary
> > in local_irq_disable()? Presumably irqs would typically be be enabled
> > before calling it?
>
> Yeah, so it's all a giant can of worms that; also see:
>
> https://lkml.kernel.org/r/20200821084738.508092956@infradead.org
>
> The basic idea is to only trace edges, ie. when the hardware state
> actually changes. Sadly this means doing a pushf/pop before the cli.
> Ideally CLI would store the old IF in CF or something like that, but
> alas.
Right, that makes sense for save/restore, but is the disabled check
really needed for local_irq_disable()? Wouldn't that always be an edge?
And anyway I don't see a similar check for local_irq_enable().
--
Josh
^ permalink raw reply [flat|nested] 32+ messages in thread
* Re: WARNING: can't access registers at asm_common_interrupt
2020-11-11 18:13 ` Josh Poimboeuf
@ 2020-11-11 18:46 ` Andrew Cooper
2020-11-11 19:42 ` Peter Zijlstra
2020-11-11 20:42 ` Peter Zijlstra
1 sibling, 1 reply; 32+ messages in thread
From: Andrew Cooper @ 2020-11-11 18:46 UTC (permalink / raw)
To: Josh Poimboeuf, Peter Zijlstra
Cc: Shinichiro Kawasaki, linux-kernel, Nicholas Piggin,
Damien Le Moal, jgross
On 11/11/2020 18:13, Josh Poimboeuf wrote:
> On Wed, Nov 11, 2020 at 06:47:36PM +0100, Peter Zijlstra wrote:
>> This is PARAVIRT_XXL only, which is a Xen special. My preference, as
>> always, is to kill it... Sadly the Xen people have a different opinion.
> That would be soooo nice... then we could get rid of paravirt patching
> altogether and replace it with static calls.
>
>>> Objtool doesn't know about the pushf/pop paravirt patch, so ORC gets
>>> confused by the changed stack layout.
>>>
>>> I'm thinking we either need to teach objtool how to deal with
>>> save_fl/restore_fl patches, or we need to just get rid of those nasty
>>> patches somehow. Peter, any thoughts?
>> Don't use Xen? ;-)
>>
>> So with PARAVIRT_XXL the compiler will emit something like:
>>
>> "CALL *pvops.save_fl"
>>
>> Which we then overwrite at runtime with "pushf; pop %[re]ax" and a few
>> NOPs.
>>
>> Now, objtool understands alternatives, and ensures they have the same
>> stack layout, it has no chance in hell of understanding this, simply
>> because paravirt_patch.c is magic.
>>
>> I don't have any immediate clever ideas, but let me ponder it a wee bit.
Well...
static_calls are a newer, and more generic, form of pvops. Most of the
magic is to do with inlining small fragments, but static calls can do
that now too, IIRC?
>> ....
>>
>> Something really disguisting we could do is recognise the indirect call
>> offset and emit an extra ORC entry for RIP+1. So the cases are:
>>
>> CALL *pv_ops.save_fl -- 7 bytes IIRC
>> CALL $imm; -- 5 bytes
>> PUSHF; POP %[RE]AX -- 2 bytes
>>
>> so the RIP+1 (the POP insn) will only ever exist in this case. The
>> indirect and direct call cases would never land on that IP.
> I had a similar idea, and a bit of deja vu - we may have talked about
> this before. At least I know we talked about doing something similar
> for alternatives which muck with the stack.
The main complexity with pvops is that the
CALL *pv_ops.save_fl
form needs to be usable from extremely early in the day (pre general
patching), hence the use of function pointers and some non-standard ABIs.
For performance reasons, the end result of this pvop wants to be `pushf;
pop %[re]ax` in then native case, and `call xen_pv_save_fl` in the Xen
case, but this doesn't mean that the compiled instruction needs to be a
function pointer to begin with.
Would objtool have an easier time coping if this were implemented in
terms of a static call?
~Andrew
^ permalink raw reply [flat|nested] 32+ messages in thread
* Re: WARNING: can't access registers at asm_common_interrupt
2020-11-11 18:46 ` Andrew Cooper
@ 2020-11-11 19:42 ` Peter Zijlstra
2020-11-11 19:59 ` Josh Poimboeuf
0 siblings, 1 reply; 32+ messages in thread
From: Peter Zijlstra @ 2020-11-11 19:42 UTC (permalink / raw)
To: Andrew Cooper
Cc: Josh Poimboeuf, Shinichiro Kawasaki, linux-kernel,
Nicholas Piggin, Damien Le Moal, jgross, x86
On Wed, Nov 11, 2020 at 06:46:37PM +0000, Andrew Cooper wrote:
> Well...
>
> static_calls are a newer, and more generic, form of pvops. Most of the
> magic is to do with inlining small fragments, but static calls can do
> that now too, IIRC?
If you're referring to this glorious hack:
https://lkml.kernel.org/r/20201110101307.GO2651@hirez.programming.kicks-ass.net
that only 'works' because it's a single instruction. That is,
static_call can only poke single instructions. They cannot replace a
call with "PUSHF; POP" / "PUSH; POPF" for example. They also cannot do
NOP padding for 'short' sequences.
Paravirt, like alternatives, are special in that they only happen once,
before SMP bringup.
> >> Something really disguisting we could do is recognise the indirect call
> >> offset and emit an extra ORC entry for RIP+1. So the cases are:
> >>
> >> CALL *pv_ops.save_fl -- 7 bytes IIRC
> >> CALL $imm; -- 5 bytes
> >> PUSHF; POP %[RE]AX -- 2 bytes
> >>
> >> so the RIP+1 (the POP insn) will only ever exist in this case. The
> >> indirect and direct call cases would never land on that IP.
> > I had a similar idea, and a bit of deja vu - we may have talked about
> > this before. At least I know we talked about doing something similar
> > for alternatives which muck with the stack.
Vague memories... luckily we managed to get alternatives to a state
where they match, which is much saner.
> The main complexity with pvops is that the
>
> CALL *pv_ops.save_fl
>
> form needs to be usable from extremely early in the day (pre general
> patching), hence the use of function pointers and some non-standard ABIs.
The performance rasins mentioned below are a large part of the
non-standard ABI (eg CALLEE_SAVE)
> For performance reasons, the end result of this pvop wants to be `pushf;
> pop %[re]ax` in then native case, and `call xen_pv_save_fl` in the Xen
> case, but this doesn't mean that the compiled instruction needs to be a
> function pointer to begin with.
Not sure emitting the native code would be feasible.. also
cpu_usergs_sysret64 is 6 bytes.
> Would objtool have an easier time coping if this were implemented in
> terms of a static call?
I doubt it, the big problem is that there is no visibility into the
actual alternative text. Runtime patching fragments into static call
would have the exact same problem.
Something that _might_ maybe work is trying to morph the immediate
fragments into an alternative. That is, instead of this:
static inline notrace unsigned long arch_local_save_flags(void)
{
return PVOP_CALLEE0(unsigned long, irq.save_fl);
}
Write it something like:
static inline notrace unsigned long arch_local_save_flags(void)
{
PVOP_CALL_ARGS;
PVOP_TEST_NULL(irq.save_fl);
asm_inline volatile(ALTERNATIVE(paravirt_alt(PARAVIRT_CALL),
"PUSHF; POP _ASM_AX",
X86_FEATURE_NATIVE)
: CLBR_RET_REG, ASM_CALL_CONSTRAINT
: paravirt_type(irq.save_fl.func),
paravirt_clobber(PVOP_CALLEE_CLOBBERS)
: "memory", "cc");
return __eax;
}
And then we have to teach objtool how to deal with conflicting
alternatives...
That would remove most (all, if we can figure out a form that deals with
the spinlock fragments) of paravirt_patch.c
Hmm?
^ permalink raw reply [flat|nested] 32+ messages in thread
* Re: WARNING: can't access registers at asm_common_interrupt
2020-11-11 19:42 ` Peter Zijlstra
@ 2020-11-11 19:59 ` Josh Poimboeuf
2020-11-11 20:07 ` Peter Zijlstra
0 siblings, 1 reply; 32+ messages in thread
From: Josh Poimboeuf @ 2020-11-11 19:59 UTC (permalink / raw)
To: Peter Zijlstra
Cc: Andrew Cooper, Shinichiro Kawasaki, linux-kernel,
Nicholas Piggin, Damien Le Moal, jgross, x86
On Wed, Nov 11, 2020 at 08:42:06PM +0100, Peter Zijlstra wrote:
> > Would objtool have an easier time coping if this were implemented in
> > terms of a static call?
>
> I doubt it, the big problem is that there is no visibility into the
> actual alternative text. Runtime patching fragments into static call
> would have the exact same problem.
>
> Something that _might_ maybe work is trying to morph the immediate
> fragments into an alternative. That is, instead of this:
>
> static inline notrace unsigned long arch_local_save_flags(void)
> {
> return PVOP_CALLEE0(unsigned long, irq.save_fl);
> }
>
> Write it something like:
>
> static inline notrace unsigned long arch_local_save_flags(void)
> {
> PVOP_CALL_ARGS;
> PVOP_TEST_NULL(irq.save_fl);
> asm_inline volatile(ALTERNATIVE(paravirt_alt(PARAVIRT_CALL),
> "PUSHF; POP _ASM_AX",
> X86_FEATURE_NATIVE)
> : CLBR_RET_REG, ASM_CALL_CONSTRAINT
> : paravirt_type(irq.save_fl.func),
> paravirt_clobber(PVOP_CALLEE_CLOBBERS)
> : "memory", "cc");
> return __eax;
> }
>
> And then we have to teach objtool how to deal with conflicting
> alternatives...
>
> That would remove most (all, if we can figure out a form that deals with
> the spinlock fragments) of paravirt_patch.c
>
> Hmm?
I was going to suggest something similar. Though I would try to take it
further and replace paravirt_patch_default() with static calls.
Either way it doesn't make objtool's job much easier. But it would be
nice to consolidate runtime patching mechanisms and get rid of
.parainstructions.
--
Josh
^ permalink raw reply [flat|nested] 32+ messages in thread
* Re: WARNING: can't access registers at asm_common_interrupt
2020-11-11 19:59 ` Josh Poimboeuf
@ 2020-11-11 20:07 ` Peter Zijlstra
2020-11-11 20:15 ` Josh Poimboeuf
0 siblings, 1 reply; 32+ messages in thread
From: Peter Zijlstra @ 2020-11-11 20:07 UTC (permalink / raw)
To: Josh Poimboeuf
Cc: Andrew Cooper, Shinichiro Kawasaki, linux-kernel,
Nicholas Piggin, Damien Le Moal, jgross, x86
On Wed, Nov 11, 2020 at 01:59:00PM -0600, Josh Poimboeuf wrote:
> On Wed, Nov 11, 2020 at 08:42:06PM +0100, Peter Zijlstra wrote:
> > > Would objtool have an easier time coping if this were implemented in
> > > terms of a static call?
> >
> > I doubt it, the big problem is that there is no visibility into the
> > actual alternative text. Runtime patching fragments into static call
> > would have the exact same problem.
> >
> > Something that _might_ maybe work is trying to morph the immediate
> > fragments into an alternative. That is, instead of this:
> >
> > static inline notrace unsigned long arch_local_save_flags(void)
> > {
> > return PVOP_CALLEE0(unsigned long, irq.save_fl);
> > }
> >
> > Write it something like:
> >
> > static inline notrace unsigned long arch_local_save_flags(void)
> > {
> > PVOP_CALL_ARGS;
> > PVOP_TEST_NULL(irq.save_fl);
> > asm_inline volatile(ALTERNATIVE(paravirt_alt(PARAVIRT_CALL),
> > "PUSHF; POP _ASM_AX",
> > X86_FEATURE_NATIVE)
> > : CLBR_RET_REG, ASM_CALL_CONSTRAINT
> > : paravirt_type(irq.save_fl.func),
> > paravirt_clobber(PVOP_CALLEE_CLOBBERS)
> > : "memory", "cc");
> > return __eax;
> > }
> >
> > And then we have to teach objtool how to deal with conflicting
> > alternatives...
> >
> > That would remove most (all, if we can figure out a form that deals with
> > the spinlock fragments) of paravirt_patch.c
> >
> > Hmm?
>
> I was going to suggest something similar. Though I would try to take it
> further and replace paravirt_patch_default() with static calls.
Possible, we just need to be _really_ careful to not allow changing
those static_call()s. So maybe we need DEFINE_STATIC_CALL_RO() which
does a __ro_after_init on the whole thing.
> Either way it doesn't make objtool's job much easier. But it would be
> nice to consolidate runtime patching mechanisms and get rid of
> .parainstructions.
I think the above (combining alternative and paravirt/static_call) does
make objtool's job easier, since then we at least have the actual
alternative instructions available to inspect, or am I mis-understanding
things?
^ permalink raw reply [flat|nested] 32+ messages in thread
* Re: WARNING: can't access registers at asm_common_interrupt
2020-11-11 20:07 ` Peter Zijlstra
@ 2020-11-11 20:15 ` Josh Poimboeuf
2020-11-11 20:25 ` Andrew Cooper
2020-11-11 20:35 ` Peter Zijlstra
0 siblings, 2 replies; 32+ messages in thread
From: Josh Poimboeuf @ 2020-11-11 20:15 UTC (permalink / raw)
To: Peter Zijlstra
Cc: Andrew Cooper, Shinichiro Kawasaki, linux-kernel,
Nicholas Piggin, Damien Le Moal, jgross, x86
On Wed, Nov 11, 2020 at 09:07:30PM +0100, Peter Zijlstra wrote:
> On Wed, Nov 11, 2020 at 01:59:00PM -0600, Josh Poimboeuf wrote:
> > On Wed, Nov 11, 2020 at 08:42:06PM +0100, Peter Zijlstra wrote:
> > > > Would objtool have an easier time coping if this were implemented in
> > > > terms of a static call?
> > >
> > > I doubt it, the big problem is that there is no visibility into the
> > > actual alternative text. Runtime patching fragments into static call
> > > would have the exact same problem.
> > >
> > > Something that _might_ maybe work is trying to morph the immediate
> > > fragments into an alternative. That is, instead of this:
> > >
> > > static inline notrace unsigned long arch_local_save_flags(void)
> > > {
> > > return PVOP_CALLEE0(unsigned long, irq.save_fl);
> > > }
> > >
> > > Write it something like:
> > >
> > > static inline notrace unsigned long arch_local_save_flags(void)
> > > {
> > > PVOP_CALL_ARGS;
> > > PVOP_TEST_NULL(irq.save_fl);
> > > asm_inline volatile(ALTERNATIVE(paravirt_alt(PARAVIRT_CALL),
> > > "PUSHF; POP _ASM_AX",
> > > X86_FEATURE_NATIVE)
> > > : CLBR_RET_REG, ASM_CALL_CONSTRAINT
> > > : paravirt_type(irq.save_fl.func),
> > > paravirt_clobber(PVOP_CALLEE_CLOBBERS)
> > > : "memory", "cc");
> > > return __eax;
> > > }
> > >
> > > And then we have to teach objtool how to deal with conflicting
> > > alternatives...
> > >
> > > That would remove most (all, if we can figure out a form that deals with
> > > the spinlock fragments) of paravirt_patch.c
> > >
> > > Hmm?
> >
> > I was going to suggest something similar. Though I would try to take it
> > further and replace paravirt_patch_default() with static calls.
>
> Possible, we just need to be _really_ careful to not allow changing
> those static_call()s. So maybe we need DEFINE_STATIC_CALL_RO() which
> does a __ro_after_init on the whole thing.
But what if you want to live migrate to another hypervisor ;-)
> > Either way it doesn't make objtool's job much easier. But it would be
> > nice to consolidate runtime patching mechanisms and get rid of
> > .parainstructions.
>
> I think the above (combining alternative and paravirt/static_call) does
> make objtool's job easier, since then we at least have the actual
> alternative instructions available to inspect, or am I mis-understanding
> things?
Right, it makes objtool's job a _little_ easier, since it already knows
how to read alternatives. But it still has to learn to deal with the
conflicting stack layouts.
--
Josh
^ permalink raw reply [flat|nested] 32+ messages in thread
* Re: WARNING: can't access registers at asm_common_interrupt
2020-11-11 20:15 ` Josh Poimboeuf
@ 2020-11-11 20:25 ` Andrew Cooper
2020-11-11 20:39 ` Peter Zijlstra
2020-11-13 17:34 ` Andy Lutomirski
2020-11-11 20:35 ` Peter Zijlstra
1 sibling, 2 replies; 32+ messages in thread
From: Andrew Cooper @ 2020-11-11 20:25 UTC (permalink / raw)
To: Josh Poimboeuf, Peter Zijlstra
Cc: Shinichiro Kawasaki, linux-kernel, Nicholas Piggin,
Damien Le Moal, jgross, x86
On 11/11/2020 20:15, Josh Poimboeuf wrote:
> On Wed, Nov 11, 2020 at 09:07:30PM +0100, Peter Zijlstra wrote:
>> On Wed, Nov 11, 2020 at 01:59:00PM -0600, Josh Poimboeuf wrote:
>>> On Wed, Nov 11, 2020 at 08:42:06PM +0100, Peter Zijlstra wrote:
>>>>> Would objtool have an easier time coping if this were implemented in
>>>>> terms of a static call?
>>>> I doubt it, the big problem is that there is no visibility into the
>>>> actual alternative text. Runtime patching fragments into static call
>>>> would have the exact same problem.
>>>>
>>>> Something that _might_ maybe work is trying to morph the immediate
>>>> fragments into an alternative. That is, instead of this:
>>>>
>>>> static inline notrace unsigned long arch_local_save_flags(void)
>>>> {
>>>> return PVOP_CALLEE0(unsigned long, irq.save_fl);
>>>> }
>>>>
>>>> Write it something like:
>>>>
>>>> static inline notrace unsigned long arch_local_save_flags(void)
>>>> {
>>>> PVOP_CALL_ARGS;
>>>> PVOP_TEST_NULL(irq.save_fl);
>>>> asm_inline volatile(ALTERNATIVE(paravirt_alt(PARAVIRT_CALL),
>>>> "PUSHF; POP _ASM_AX",
>>>> X86_FEATURE_NATIVE)
>>>> : CLBR_RET_REG, ASM_CALL_CONSTRAINT
>>>> : paravirt_type(irq.save_fl.func),
>>>> paravirt_clobber(PVOP_CALLEE_CLOBBERS)
>>>> : "memory", "cc");
>>>> return __eax;
>>>> }
>>>>
>>>> And then we have to teach objtool how to deal with conflicting
>>>> alternatives...
>>>>
>>>> That would remove most (all, if we can figure out a form that deals with
>>>> the spinlock fragments) of paravirt_patch.c
>>>>
>>>> Hmm?
>>> I was going to suggest something similar. Though I would try to take it
>>> further and replace paravirt_patch_default() with static calls.
>> Possible, we just need to be _really_ careful to not allow changing
>> those static_call()s. So maybe we need DEFINE_STATIC_CALL_RO() which
>> does a __ro_after_init on the whole thing.
> But what if you want to live migrate to another hypervisor ;-)
The same as what happens currently. The user gets to keep all the
resulting pieces ;)
>>> Either way it doesn't make objtool's job much easier. But it would be
>>> nice to consolidate runtime patching mechanisms and get rid of
>>> .parainstructions.
>> I think the above (combining alternative and paravirt/static_call) does
>> make objtool's job easier, since then we at least have the actual
>> alternative instructions available to inspect, or am I mis-understanding
>> things?
> Right, it makes objtool's job a _little_ easier, since it already knows
> how to read alternatives. But it still has to learn to deal with the
> conflicting stack layouts.
I suppose the needed abstraction is "these blocks will start and end
with the same stack layout", while allowing the internals to diverge.
~Andrew
^ permalink raw reply [flat|nested] 32+ messages in thread
* Re: WARNING: can't access registers at asm_common_interrupt
2020-11-11 20:15 ` Josh Poimboeuf
2020-11-11 20:25 ` Andrew Cooper
@ 2020-11-11 20:35 ` Peter Zijlstra
1 sibling, 0 replies; 32+ messages in thread
From: Peter Zijlstra @ 2020-11-11 20:35 UTC (permalink / raw)
To: Josh Poimboeuf
Cc: Andrew Cooper, Shinichiro Kawasaki, linux-kernel,
Nicholas Piggin, Damien Le Moal, jgross, x86
On Wed, Nov 11, 2020 at 02:15:06PM -0600, Josh Poimboeuf wrote:
> On Wed, Nov 11, 2020 at 09:07:30PM +0100, Peter Zijlstra wrote:
> > Possible, we just need to be _really_ careful to not allow changing
> > those static_call()s. So maybe we need DEFINE_STATIC_CALL_RO() which
> > does a __ro_after_init on the whole thing.
>
> But what if you want to live migrate to another hypervisor ;-)
Then you get to keep the pieces ;-)
> > > Either way it doesn't make objtool's job much easier. But it would be
> > > nice to consolidate runtime patching mechanisms and get rid of
> > > .parainstructions.
> >
> > I think the above (combining alternative and paravirt/static_call) does
> > make objtool's job easier, since then we at least have the actual
> > alternative instructions available to inspect, or am I mis-understanding
> > things?
>
> Right, it makes objtool's job a _little_ easier, since it already knows
> how to read alternatives. But it still has to learn to deal with the
> conflicting stack layouts.
Right, but at least now it can see the instructions. Which is a lot
better than: `add a magic +1 ORC entry when you see an indirect call to
$magic`.
Anyway, __orc_find(addr) looks for the ORC entry with the highest IP <=
@addr. So if we have:
alt0 alt1
0x00 CALL *foo 0x00 PUSHF
0x07 insn 0x01 POP %rax
0x02 .nops 5
0x07 insn
we have ORC entries for alt1.0x00 and alt1.0x01. Then if we hit insn,
it'll find the alt1.0x01 entry, but that had better be the same as the
state at 0x00.
This means that for every alt, we have to unwind using the CFI of every
other alt and match for every instruction. Which should be doable I
think.
Certainly more complicated that outright disallowing CFI changes inside
alt groups though :/
^ permalink raw reply [flat|nested] 32+ messages in thread
* Re: WARNING: can't access registers at asm_common_interrupt
2020-11-11 20:25 ` Andrew Cooper
@ 2020-11-11 20:39 ` Peter Zijlstra
2020-11-13 17:34 ` Andy Lutomirski
1 sibling, 0 replies; 32+ messages in thread
From: Peter Zijlstra @ 2020-11-11 20:39 UTC (permalink / raw)
To: Andrew Cooper
Cc: Josh Poimboeuf, Shinichiro Kawasaki, linux-kernel,
Nicholas Piggin, Damien Le Moal, jgross, x86
On Wed, Nov 11, 2020 at 08:25:36PM +0000, Andrew Cooper wrote:
> > Right, it makes objtool's job a _little_ easier, since it already knows
> > how to read alternatives. But it still has to learn to deal with the
> > conflicting stack layouts.
>
> I suppose the needed abstraction is "these blocks will start and end
> with the same stack layout", while allowing the internals to diverge.
It's a little more complicated than that due to the fact that there is
only a single ORC table.
So something like:
alt0 alt1
0x00 push 0x00 nop
0x01 nop 0x01 push
is impossible to correctly unwind.
^ permalink raw reply [flat|nested] 32+ messages in thread
* Re: WARNING: can't access registers at asm_common_interrupt
2020-11-11 18:13 ` Josh Poimboeuf
2020-11-11 18:46 ` Andrew Cooper
@ 2020-11-11 20:42 ` Peter Zijlstra
1 sibling, 0 replies; 32+ messages in thread
From: Peter Zijlstra @ 2020-11-11 20:42 UTC (permalink / raw)
To: Josh Poimboeuf
Cc: Shinichiro Kawasaki, linux-kernel, Nicholas Piggin,
Damien Le Moal, andrew.cooper3, jgross
On Wed, Nov 11, 2020 at 12:13:28PM -0600, Josh Poimboeuf wrote:
> On Wed, Nov 11, 2020 at 06:47:36PM +0100, Peter Zijlstra wrote:
> > Yeah, so it's all a giant can of worms that; also see:
> >
> > https://lkml.kernel.org/r/20200821084738.508092956@infradead.org
> >
> > The basic idea is to only trace edges, ie. when the hardware state
> > actually changes. Sadly this means doing a pushf/pop before the cli.
> > Ideally CLI would store the old IF in CF or something like that, but
> > alas.
>
> Right, that makes sense for save/restore, but is the disabled check
> really needed for local_irq_disable()? Wouldn't that always be an edge?
IIRC there is code that does local_irq_disable() even though IRQs are
already disabled. This is 'harmless'.
> And anyway I don't see a similar check for local_irq_enable().
I know there is code that does local_irq_enable() with IRQs already
enabled, I'm not exactly sure why this is different. I'll have to put it
on the todo list :/
^ permalink raw reply [flat|nested] 32+ messages in thread
* Re: WARNING: can't access registers at asm_common_interrupt
2020-11-11 20:25 ` Andrew Cooper
2020-11-11 20:39 ` Peter Zijlstra
@ 2020-11-13 17:34 ` Andy Lutomirski
2020-11-14 9:16 ` Jürgen Groß
2020-11-16 11:56 ` Jürgen Groß
1 sibling, 2 replies; 32+ messages in thread
From: Andy Lutomirski @ 2020-11-13 17:34 UTC (permalink / raw)
To: Andrew Cooper
Cc: Josh Poimboeuf, Peter Zijlstra, Shinichiro Kawasaki,
linux-kernel, Nicholas Piggin, Damien Le Moal, Juergen Gross,
X86 ML
On Wed, Nov 11, 2020 at 12:25 PM Andrew Cooper
<andrew.cooper3@citrix.com> wrote:
>
> On 11/11/2020 20:15, Josh Poimboeuf wrote:
> > On Wed, Nov 11, 2020 at 09:07:30PM +0100, Peter Zijlstra wrote:
> >> On Wed, Nov 11, 2020 at 01:59:00PM -0600, Josh Poimboeuf wrote:
> >>> On Wed, Nov 11, 2020 at 08:42:06PM +0100, Peter Zijlstra wrote:
> >>>>> Would objtool have an easier time coping if this were implemented in
> >>>>> terms of a static call?
> >>>> I doubt it, the big problem is that there is no visibility into the
> >>>> actual alternative text. Runtime patching fragments into static call
> >>>> would have the exact same problem.
> >>>>
> >>>> Something that _might_ maybe work is trying to morph the immediate
> >>>> fragments into an alternative. That is, instead of this:
> >>>>
> >>>> static inline notrace unsigned long arch_local_save_flags(void)
> >>>> {
> >>>> return PVOP_CALLEE0(unsigned long, irq.save_fl);
> >>>> }
> >>>>
> >>>> Write it something like:
> >>>>
> >>>> static inline notrace unsigned long arch_local_save_flags(void)
> >>>> {
> >>>> PVOP_CALL_ARGS;
> >>>> PVOP_TEST_NULL(irq.save_fl);
> >>>> asm_inline volatile(ALTERNATIVE(paravirt_alt(PARAVIRT_CALL),
> >>>> "PUSHF; POP _ASM_AX",
> >>>> X86_FEATURE_NATIVE)
> >>>> : CLBR_RET_REG, ASM_CALL_CONSTRAINT
> >>>> : paravirt_type(irq.save_fl.func),
> >>>> paravirt_clobber(PVOP_CALLEE_CLOBBERS)
> >>>> : "memory", "cc");
> >>>> return __eax;
> >>>> }
> >>>>
> >>>> And then we have to teach objtool how to deal with conflicting
> >>>> alternatives...
> >>>>
> >>>> That would remove most (all, if we can figure out a form that deals with
> >>>> the spinlock fragments) of paravirt_patch.c
> >>>>
> >>>> Hmm?
> >>> I was going to suggest something similar. Though I would try to take it
> >>> further and replace paravirt_patch_default() with static calls.
> >> Possible, we just need to be _really_ careful to not allow changing
> >> those static_call()s. So maybe we need DEFINE_STATIC_CALL_RO() which
> >> does a __ro_after_init on the whole thing.
> > But what if you want to live migrate to another hypervisor ;-)
>
> The same as what happens currently. The user gets to keep all the
> resulting pieces ;)
>
> >>> Either way it doesn't make objtool's job much easier. But it would be
> >>> nice to consolidate runtime patching mechanisms and get rid of
> >>> .parainstructions.
> >> I think the above (combining alternative and paravirt/static_call) does
> >> make objtool's job easier, since then we at least have the actual
> >> alternative instructions available to inspect, or am I mis-understanding
> >> things?
> > Right, it makes objtool's job a _little_ easier, since it already knows
> > how to read alternatives. But it still has to learn to deal with the
> > conflicting stack layouts.
>
> I suppose the needed abstraction is "these blocks will start and end
> with the same stack layout", while allowing the internals to diverge.
>
How much of this stuff is actually useful anymore? I'm wondering if
we can move most or all of this crud to
cpu_feature_enabled(X86_FEATURE_XENPV) and its asm equivalent. The
full list, annotated, appears to be:
const unsigned char irq_irq_disable[1];
This is CLI or CALL, right?
const unsigned char irq_irq_enable[1];
STI or CALL.
const unsigned char irq_save_fl[2];
PUSHF; POP %r/eax. I *think* I read the paravirt mess correctly and
this also turns into CALL.
const unsigned char mmu_read_cr2[3];
const unsigned char mmu_read_cr3[3];
const unsigned char mmu_write_cr3[3];
The write CR3 is so slow that I can't imagine us caring. Reading CR3
should already be fairly optimized because it's slow on old non-PV
hypervisors, too. Reading CR2 is rare and lives in asm. These also
appear to just switch between MOV and CALL, anyway.
const unsigned char irq_restore_fl[2];
Ugh, this one sucks. IMO it should be, for native and PV:
if (flags & X86_EFLAGS_IF) {
local_irq_enable(); /* or raw? */
} else {
if (some debugging option) {
WARN_ON_ONCE(save_fl() & X86_EFLAGS_IF);
}
}
POPF is slooooow.
const unsigned char cpu_wbinvd[2];
This is hilariously slow no matter what. static_call() or even just a
plain old indirect call should be fine.
const unsigned char cpu_usergs_sysret64[6];
This is in the asm and we shouldn't be doing it at all for Xen PV.
IOW we should just drop this patch site entirely. I can possibly find
some time to get rid of it, and maybe someone from Xen land can help.
I bet that we can gain a lot of perf on Xen PV by cleaning this up,
and I bet it will simplify everything.
const unsigned char cpu_swapgs[3];
This is SWAPGS or nop, unless I've missed some subtlety.
const unsigned char mov64[3];
This is some PTE magic, and I haven't deciphered it yet.
So I think there is at most one of these that wants anything more
complicated than a plain ALTERNATIVE. Any volunteers to make it so?
Juergen, if you do all of them except USERGS_SYSRET64, I hereby
volunteer to do that one.
BTW, if y'all want to live migrate between Xen PV and anything else,
you are nuts.
^ permalink raw reply [flat|nested] 32+ messages in thread
* Re: WARNING: can't access registers at asm_common_interrupt
2020-11-13 17:34 ` Andy Lutomirski
@ 2020-11-14 9:16 ` Jürgen Groß
2020-11-14 18:10 ` Andy Lutomirski
2020-11-16 11:56 ` Jürgen Groß
1 sibling, 1 reply; 32+ messages in thread
From: Jürgen Groß @ 2020-11-14 9:16 UTC (permalink / raw)
To: Andy Lutomirski, Andrew Cooper
Cc: Josh Poimboeuf, Peter Zijlstra, Shinichiro Kawasaki,
linux-kernel, Nicholas Piggin, Damien Le Moal, X86 ML
[-- Attachment #1.1.1: Type: text/plain, Size: 6143 bytes --]
On 13.11.20 18:34, Andy Lutomirski wrote:
> On Wed, Nov 11, 2020 at 12:25 PM Andrew Cooper
> <andrew.cooper3@citrix.com> wrote:
>>
>> On 11/11/2020 20:15, Josh Poimboeuf wrote:
>>> On Wed, Nov 11, 2020 at 09:07:30PM +0100, Peter Zijlstra wrote:
>>>> On Wed, Nov 11, 2020 at 01:59:00PM -0600, Josh Poimboeuf wrote:
>>>>> On Wed, Nov 11, 2020 at 08:42:06PM +0100, Peter Zijlstra wrote:
>>>>>>> Would objtool have an easier time coping if this were implemented in
>>>>>>> terms of a static call?
>>>>>> I doubt it, the big problem is that there is no visibility into the
>>>>>> actual alternative text. Runtime patching fragments into static call
>>>>>> would have the exact same problem.
>>>>>>
>>>>>> Something that _might_ maybe work is trying to morph the immediate
>>>>>> fragments into an alternative. That is, instead of this:
>>>>>>
>>>>>> static inline notrace unsigned long arch_local_save_flags(void)
>>>>>> {
>>>>>> return PVOP_CALLEE0(unsigned long, irq.save_fl);
>>>>>> }
>>>>>>
>>>>>> Write it something like:
>>>>>>
>>>>>> static inline notrace unsigned long arch_local_save_flags(void)
>>>>>> {
>>>>>> PVOP_CALL_ARGS;
>>>>>> PVOP_TEST_NULL(irq.save_fl);
>>>>>> asm_inline volatile(ALTERNATIVE(paravirt_alt(PARAVIRT_CALL),
>>>>>> "PUSHF; POP _ASM_AX",
>>>>>> X86_FEATURE_NATIVE)
>>>>>> : CLBR_RET_REG, ASM_CALL_CONSTRAINT
>>>>>> : paravirt_type(irq.save_fl.func),
>>>>>> paravirt_clobber(PVOP_CALLEE_CLOBBERS)
>>>>>> : "memory", "cc");
>>>>>> return __eax;
>>>>>> }
>>>>>>
>>>>>> And then we have to teach objtool how to deal with conflicting
>>>>>> alternatives...
>>>>>>
>>>>>> That would remove most (all, if we can figure out a form that deals with
>>>>>> the spinlock fragments) of paravirt_patch.c
>>>>>>
>>>>>> Hmm?
>>>>> I was going to suggest something similar. Though I would try to take it
>>>>> further and replace paravirt_patch_default() with static calls.
>>>> Possible, we just need to be _really_ careful to not allow changing
>>>> those static_call()s. So maybe we need DEFINE_STATIC_CALL_RO() which
>>>> does a __ro_after_init on the whole thing.
>>> But what if you want to live migrate to another hypervisor ;-)
>>
>> The same as what happens currently. The user gets to keep all the
>> resulting pieces ;)
>>
>>>>> Either way it doesn't make objtool's job much easier. But it would be
>>>>> nice to consolidate runtime patching mechanisms and get rid of
>>>>> .parainstructions.
>>>> I think the above (combining alternative and paravirt/static_call) does
>>>> make objtool's job easier, since then we at least have the actual
>>>> alternative instructions available to inspect, or am I mis-understanding
>>>> things?
>>> Right, it makes objtool's job a _little_ easier, since it already knows
>>> how to read alternatives. But it still has to learn to deal with the
>>> conflicting stack layouts.
>>
>> I suppose the needed abstraction is "these blocks will start and end
>> with the same stack layout", while allowing the internals to diverge.
>>
>
> How much of this stuff is actually useful anymore? I'm wondering if
> we can move most or all of this crud to
> cpu_feature_enabled(X86_FEATURE_XENPV) and its asm equivalent. The
> full list, annotated, appears to be:
>
> const unsigned char irq_irq_disable[1];
>
> This is CLI or CALL, right?
Yes.
>
> const unsigned char irq_irq_enable[1];
>
> STI or CALL.
Yes.
>
> const unsigned char irq_save_fl[2];
>
> PUSHF; POP %r/eax. I *think* I read the paravirt mess correctly and
> this also turns into CALL.
It does.
>
> const unsigned char mmu_read_cr2[3];
> const unsigned char mmu_read_cr3[3];
> const unsigned char mmu_write_cr3[3];
>
> The write CR3 is so slow that I can't imagine us caring. Reading CR3
> should already be fairly optimized because it's slow on old non-PV
> hypervisors, too. Reading CR2 is rare and lives in asm. These also
> appear to just switch between MOV and CALL, anyway.
Correct.
>
> const unsigned char irq_restore_fl[2];
>
> Ugh, this one sucks. IMO it should be, for native and PV:
>
> if (flags & X86_EFLAGS_IF) {
> local_irq_enable(); /* or raw? */
> } else {
> if (some debugging option) {
> WARN_ON_ONCE(save_fl() & X86_EFLAGS_IF);
> }
> }
Seems sensible.
>
> POPF is slooooow.
>
> const unsigned char cpu_wbinvd[2];
>
> This is hilariously slow no matter what. static_call() or even just a
> plain old indirect call should be fine.
I'd go with the static_call().
>
> const unsigned char cpu_usergs_sysret64[6];
>
> This is in the asm and we shouldn't be doing it at all for Xen PV.
> IOW we should just drop this patch site entirely. I can possibly find
> some time to get rid of it, and maybe someone from Xen land can help.
> I bet that we can gain a lot of perf on Xen PV by cleaning this up,
> and I bet it will simplify everything.
>
> const unsigned char cpu_swapgs[3];
>
> This is SWAPGS or nop, unless I've missed some subtlety.
>
> const unsigned char mov64[3];
>
> This is some PTE magic, and I haven't deciphered it yet.
Either a mov or a call.
>
> So I think there is at most one of these that wants anything more
> complicated than a plain ALTERNATIVE. Any volunteers to make it so?
> Juergen, if you do all of them except USERGS_SYSRET64, I hereby
> volunteer to do that one.
Why is a plain alternative (either swapgs; sysretq or a jmp xen_sysret64
depending on X86_FEATURE_XENPV) no option?
Its not as if this code would run before alternative patching.
>
> BTW, if y'all want to live migrate between Xen PV and anything else,
> you are nuts.
>
That's no option. Xen PV is a guest property, not one of the hypervisor.
Juergen
[-- Attachment #1.1.2: OpenPGP_0xB0DE9DD628BF132F.asc --]
[-- Type: application/pgp-keys, Size: 3135 bytes --]
[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 495 bytes --]
^ permalink raw reply [flat|nested] 32+ messages in thread
* Re: WARNING: can't access registers at asm_common_interrupt
2020-11-14 9:16 ` Jürgen Groß
@ 2020-11-14 18:10 ` Andy Lutomirski
2020-11-15 6:33 ` Jürgen Groß
0 siblings, 1 reply; 32+ messages in thread
From: Andy Lutomirski @ 2020-11-14 18:10 UTC (permalink / raw)
To: Jürgen Groß
Cc: Andy Lutomirski, Andrew Cooper, Josh Poimboeuf, Peter Zijlstra,
Shinichiro Kawasaki, linux-kernel, Nicholas Piggin,
Damien Le Moal, X86 ML
On Sat, Nov 14, 2020 at 1:16 AM Jürgen Groß <jgross@suse.com> wrote:
>
> On 13.11.20 18:34, Andy Lutomirski wrote:
> > On Wed, Nov 11, 2020 at 12:25 PM Andrew Cooper
> > <andrew.cooper3@citrix.com> wrote:
> >
> > So I think there is at most one of these that wants anything more
> > complicated than a plain ALTERNATIVE. Any volunteers to make it so?
> > Juergen, if you do all of them except USERGS_SYSRET64, I hereby
> > volunteer to do that one.
>
> Why is a plain alternative (either swapgs; sysretq or a jmp xen_sysret64
> depending on X86_FEATURE_XENPV) no option?
>
> Its not as if this code would run before alternative patching.
ALTERNATIVE would "work" in the sense that it would function and be
just about as nonsensical as the current code. Fundamentally, Xen
PV's sysret feature is not a drop-in replacement for SYSRET64, and
pretending that it is seems unlikely to work well. I suspect that the
current code is some combination of exceedingly slow, non-functional,
and incorrect in subtle ways.
We should just have a separate Xen PV exit path the same way we have a
separate entry path in recent kernels. *This* is what I'm
volunteering to do.
--Andy
^ permalink raw reply [flat|nested] 32+ messages in thread
* Re: WARNING: can't access registers at asm_common_interrupt
2020-11-14 18:10 ` Andy Lutomirski
@ 2020-11-15 6:33 ` Jürgen Groß
2020-11-15 16:05 ` Andy Lutomirski
0 siblings, 1 reply; 32+ messages in thread
From: Jürgen Groß @ 2020-11-15 6:33 UTC (permalink / raw)
To: Andy Lutomirski
Cc: Andrew Cooper, Josh Poimboeuf, Peter Zijlstra,
Shinichiro Kawasaki, linux-kernel, Nicholas Piggin,
Damien Le Moal, X86 ML
[-- Attachment #1.1.1: Type: text/plain, Size: 1486 bytes --]
On 14.11.20 19:10, Andy Lutomirski wrote:
> On Sat, Nov 14, 2020 at 1:16 AM Jürgen Groß <jgross@suse.com> wrote:
>>
>> On 13.11.20 18:34, Andy Lutomirski wrote:
>>> On Wed, Nov 11, 2020 at 12:25 PM Andrew Cooper
>>> <andrew.cooper3@citrix.com> wrote:
>>>
>>> So I think there is at most one of these that wants anything more
>>> complicated than a plain ALTERNATIVE. Any volunteers to make it so?
>>> Juergen, if you do all of them except USERGS_SYSRET64, I hereby
>>> volunteer to do that one.
>>
>> Why is a plain alternative (either swapgs; sysretq or a jmp xen_sysret64
>> depending on X86_FEATURE_XENPV) no option?
>>
>> Its not as if this code would run before alternative patching.
>
> ALTERNATIVE would "work" in the sense that it would function and be
> just about as nonsensical as the current code. Fundamentally, Xen
> PV's sysret feature is not a drop-in replacement for SYSRET64, and
> pretending that it is seems unlikely to work well. I suspect that the
> current code is some combination of exceedingly slow, non-functional,
> and incorrect in subtle ways.
>
> We should just have a separate Xen PV exit path the same way we have a
> separate entry path in recent kernels. *This* is what I'm
> volunteering to do.
I don't think there is much work needed. Xen PV does basically a return
to user mode via IRET. I think it might work just to use
swapgs_restore_regs_and_return_to_usermode() unconditionally for Xen PV.
Juergen
[-- Attachment #1.1.2: OpenPGP_0xB0DE9DD628BF132F.asc --]
[-- Type: application/pgp-keys, Size: 3135 bytes --]
[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 495 bytes --]
^ permalink raw reply [flat|nested] 32+ messages in thread
* Re: WARNING: can't access registers at asm_common_interrupt
2020-11-15 6:33 ` Jürgen Groß
@ 2020-11-15 16:05 ` Andy Lutomirski
2020-11-15 16:13 ` Jürgen Groß
0 siblings, 1 reply; 32+ messages in thread
From: Andy Lutomirski @ 2020-11-15 16:05 UTC (permalink / raw)
To: Jürgen Groß
Cc: Andy Lutomirski, Andrew Cooper, Josh Poimboeuf, Peter Zijlstra,
Shinichiro Kawasaki, linux-kernel, Nicholas Piggin,
Damien Le Moal, X86 ML
> On Nov 14, 2020, at 10:33 PM, Jürgen Groß <jgross@suse.com> wrote:
>
> On 14.11.20 19:10, Andy Lutomirski wrote:
>>> On Sat, Nov 14, 2020 at 1:16 AM Jürgen Groß <jgross@suse.com> wrote:
>>>
>>> On 13.11.20 18:34, Andy Lutomirski wrote:
>>>> On Wed, Nov 11, 2020 at 12:25 PM Andrew Cooper
>>>> <andrew.cooper3@citrix.com> wrote:
>>>>
>>>> So I think there is at most one of these that wants anything more
>>>> complicated than a plain ALTERNATIVE. Any volunteers to make it so?
>>>> Juergen, if you do all of them except USERGS_SYSRET64, I hereby
>>>> volunteer to do that one.
>>>
>>> Why is a plain alternative (either swapgs; sysretq or a jmp xen_sysret64
>>> depending on X86_FEATURE_XENPV) no option?
>>>
>>> Its not as if this code would run before alternative patching.
>> ALTERNATIVE would "work" in the sense that it would function and be
>> just about as nonsensical as the current code. Fundamentally, Xen
>> PV's sysret feature is not a drop-in replacement for SYSRET64, and
>> pretending that it is seems unlikely to work well. I suspect that the
>> current code is some combination of exceedingly slow, non-functional,
>> and incorrect in subtle ways.
>> We should just have a separate Xen PV exit path the same way we have a
>> separate entry path in recent kernels. *This* is what I'm
>> volunteering to do.
>
> I don't think there is much work needed. Xen PV does basically a return
> to user mode via IRET. I think it might work just to use
> swapgs_restore_regs_and_return_to_usermode() unconditionally for Xen PV.
>
I’m quite confident that will work, but I was hoping to get it to work quickly too :)
>
> Juergen
> <OpenPGP_0xB0DE9DD628BF132F.asc>
^ permalink raw reply [flat|nested] 32+ messages in thread
* Re: WARNING: can't access registers at asm_common_interrupt
2020-11-15 16:05 ` Andy Lutomirski
@ 2020-11-15 16:13 ` Jürgen Groß
0 siblings, 0 replies; 32+ messages in thread
From: Jürgen Groß @ 2020-11-15 16:13 UTC (permalink / raw)
To: Andy Lutomirski
Cc: Andy Lutomirski, Andrew Cooper, Josh Poimboeuf, Peter Zijlstra,
Shinichiro Kawasaki, linux-kernel, Nicholas Piggin,
Damien Le Moal, X86 ML
[-- Attachment #1.1.1: Type: text/plain, Size: 1863 bytes --]
On 15.11.20 17:05, Andy Lutomirski wrote:
>
>> On Nov 14, 2020, at 10:33 PM, Jürgen Groß <jgross@suse.com> wrote:
>>
>> On 14.11.20 19:10, Andy Lutomirski wrote:
>>>> On Sat, Nov 14, 2020 at 1:16 AM Jürgen Groß <jgross@suse.com> wrote:
>>>>
>>>> On 13.11.20 18:34, Andy Lutomirski wrote:
>>>>> On Wed, Nov 11, 2020 at 12:25 PM Andrew Cooper
>>>>> <andrew.cooper3@citrix.com> wrote:
>>>>>
>>>>> So I think there is at most one of these that wants anything more
>>>>> complicated than a plain ALTERNATIVE. Any volunteers to make it so?
>>>>> Juergen, if you do all of them except USERGS_SYSRET64, I hereby
>>>>> volunteer to do that one.
>>>>
>>>> Why is a plain alternative (either swapgs; sysretq or a jmp xen_sysret64
>>>> depending on X86_FEATURE_XENPV) no option?
>>>>
>>>> Its not as if this code would run before alternative patching.
>>> ALTERNATIVE would "work" in the sense that it would function and be
>>> just about as nonsensical as the current code. Fundamentally, Xen
>>> PV's sysret feature is not a drop-in replacement for SYSRET64, and
>>> pretending that it is seems unlikely to work well. I suspect that the
>>> current code is some combination of exceedingly slow, non-functional,
>>> and incorrect in subtle ways.
>>> We should just have a separate Xen PV exit path the same way we have a
>>> separate entry path in recent kernels. *This* is what I'm
>>> volunteering to do.
>>
>> I don't think there is much work needed. Xen PV does basically a return
>> to user mode via IRET. I think it might work just to use
>> swapgs_restore_regs_and_return_to_usermode() unconditionally for Xen PV.
>>
>
> I’m quite confident that will work, but I was hoping to get it to work quickly too :)
The PV interface requires to use the iret hypercall, because there
is no sysret equivalent.
Juergen
[-- Attachment #1.1.2: OpenPGP_0xB0DE9DD628BF132F.asc --]
[-- Type: application/pgp-keys, Size: 3135 bytes --]
[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 495 bytes --]
^ permalink raw reply [flat|nested] 32+ messages in thread
* Re: WARNING: can't access registers at asm_common_interrupt
2020-11-13 17:34 ` Andy Lutomirski
2020-11-14 9:16 ` Jürgen Groß
@ 2020-11-16 11:56 ` Jürgen Groß
2020-11-16 13:04 ` Peter Zijlstra
1 sibling, 1 reply; 32+ messages in thread
From: Jürgen Groß @ 2020-11-16 11:56 UTC (permalink / raw)
To: Andy Lutomirski, Andrew Cooper
Cc: Josh Poimboeuf, Peter Zijlstra, Shinichiro Kawasaki,
linux-kernel, Nicholas Piggin, Damien Le Moal, X86 ML
[-- Attachment #1.1.1: Type: text/plain, Size: 1727 bytes --]
On 13.11.20 18:34, Andy Lutomirski wrote:
> On Wed, Nov 11, 2020 at 12:25 PM Andrew Cooper
> <andrew.cooper3@citrix.com> wrote:
>>
>> On 11/11/2020 20:15, Josh Poimboeuf wrote:
>>> On Wed, Nov 11, 2020 at 09:07:30PM +0100, Peter Zijlstra wrote:
>>>> On Wed, Nov 11, 2020 at 01:59:00PM -0600, Josh Poimboeuf wrote:
>>>>> On Wed, Nov 11, 2020 at 08:42:06PM +0100, Peter Zijlstra wrote:
>>>>>>> Would objtool have an easier time coping if this were implemented in
>>>>>>> terms of a static call?
>>>>>> I doubt it, the big problem is that there is no visibility into the
>>>>>> actual alternative text. Runtime patching fragments into static call
>>>>>> would have the exact same problem.
>>>>>>
>>>>>> Something that _might_ maybe work is trying to morph the immediate
>>>>>> fragments into an alternative. That is, instead of this:
>>>>>>
>>>>>> static inline notrace unsigned long arch_local_save_flags(void)
>>>>>> {
>>>>>> return PVOP_CALLEE0(unsigned long, irq.save_fl);
>>>>>> }
>>>>>>
>>>>>> Write it something like:
>>>>>>
>>>>>> static inline notrace unsigned long arch_local_save_flags(void)
>>>>>> {
>>>>>> PVOP_CALL_ARGS;
>>>>>> PVOP_TEST_NULL(irq.save_fl);
>>>>>> asm_inline volatile(ALTERNATIVE(paravirt_alt(PARAVIRT_CALL),
>>>>>> "PUSHF; POP _ASM_AX",
>>>>>> X86_FEATURE_NATIVE)
I am wondering whether we really want a new feature (basically "not
XENPV). We could use ~X86_FEATURE_XENPV and teach apply_alternatives()
to understand negated features (yes, this limits the number of features
to 32767, but I don't think this is a real problem for quite some time).
Thoughts?
Juergen
[-- Attachment #1.1.2: OpenPGP_0xB0DE9DD628BF132F.asc --]
[-- Type: application/pgp-keys, Size: 3135 bytes --]
[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 495 bytes --]
^ permalink raw reply [flat|nested] 32+ messages in thread
* Re: WARNING: can't access registers at asm_common_interrupt
2020-11-16 11:56 ` Jürgen Groß
@ 2020-11-16 13:04 ` Peter Zijlstra
2020-11-18 6:47 ` Jürgen Groß
0 siblings, 1 reply; 32+ messages in thread
From: Peter Zijlstra @ 2020-11-16 13:04 UTC (permalink / raw)
To: Jürgen Groß
Cc: Andy Lutomirski, Andrew Cooper, Josh Poimboeuf,
Shinichiro Kawasaki, linux-kernel, Nicholas Piggin,
Damien Le Moal, X86 ML
[-- Attachment #1: Type: text/plain, Size: 6351 bytes --]
On Mon, Nov 16, 2020 at 12:56:32PM +0100, Jürgen Groß wrote:
> > > > > > > static inline notrace unsigned long arch_local_save_flags(void)
> > > > > > > {
> > > > > > > PVOP_CALL_ARGS;
> > > > > > > PVOP_TEST_NULL(irq.save_fl);
> > > > > > > asm_inline volatile(ALTERNATIVE(paravirt_alt(PARAVIRT_CALL),
> > > > > > > "PUSHF; POP _ASM_AX",
> > > > > > > X86_FEATURE_NATIVE)
>
> I am wondering whether we really want a new feature (basically "not
> XENPV). We could use ~X86_FEATURE_XENPV and teach apply_alternatives()
> to understand negated features (yes, this limits the number of features
> to 32767, but I don't think this is a real problem for quite some time).
>
> Thoughts?
I went with the simple thing for now... If we ever want/need another
negative alternative I suppose we can do as you suggest...
I was still poking at objtool to actually dtrt though..
---
diff --git a/arch/x86/include/asm/cpufeatures.h b/arch/x86/include/asm/cpufeatures.h
index dad350d42ecf..cc88f358bac5 100644
--- a/arch/x86/include/asm/cpufeatures.h
+++ b/arch/x86/include/asm/cpufeatures.h
@@ -237,6 +237,7 @@
#define X86_FEATURE_VMCALL ( 8*32+18) /* "" Hypervisor supports the VMCALL instruction */
#define X86_FEATURE_VMW_VMMCALL ( 8*32+19) /* "" VMware prefers VMMCALL hypercall instruction */
#define X86_FEATURE_SEV_ES ( 8*32+20) /* AMD Secure Encrypted Virtualization - Encrypted State */
+#define X86_FEATURE_NOT_XENPV ( 8*32+21) /* "" inverse of X86_FEATURE_XENPV */
/* Intel-defined CPU features, CPUID level 0x00000007:0 (EBX), word 9 */
#define X86_FEATURE_FSGSBASE ( 9*32+ 0) /* RDFSBASE, WRFSBASE, RDGSBASE, WRGSBASE instructions*/
diff --git a/arch/x86/include/asm/paravirt.h b/arch/x86/include/asm/paravirt.h
index d25cc6830e89..e2a9d3e6b7ad 100644
--- a/arch/x86/include/asm/paravirt.h
+++ b/arch/x86/include/asm/paravirt.h
@@ -645,22 +645,56 @@ bool __raw_callee_save___native_vcpu_is_preempted(long cpu);
#ifdef CONFIG_PARAVIRT_XXL
static inline notrace unsigned long arch_local_save_flags(void)
{
- return PVOP_CALLEE0(unsigned long, irq.save_fl);
+ PVOP_CALL_ARGS;
+ PVOP_TEST_NULL(irq.save_fl);
+ asm_inline volatile(ALTERNATIVE(paravirt_alt(PARAVIRT_CALL),
+ "pushf; pop %%" _ASM_AX ";",
+ X86_FEATURE_NOT_XENPV)
+ : PVOP_CALLEE_CLOBBERS, ASM_CALL_CONSTRAINT
+ : paravirt_type(irq.save_fl.func),
+ paravirt_clobber(CLBR_RET_REG)
+ : "memory", "cc");
+ return __eax;
}
static inline notrace void arch_local_irq_restore(unsigned long f)
{
- PVOP_VCALLEE1(irq.restore_fl, f);
+ PVOP_CALL_ARGS;
+ PVOP_TEST_NULL(irq.restore_fl);
+ asm_inline volatile(ALTERNATIVE(paravirt_alt(PARAVIRT_CALL),
+ "push %%" _ASM_ARG1 "; popf;",
+ X86_FEATURE_NOT_XENPV)
+ : PVOP_VCALLEE_CLOBBERS, ASM_CALL_CONSTRAINT
+ : paravirt_type(irq.restore_fl.func),
+ paravirt_clobber(CLBR_RET_REG),
+ PVOP_CALL_ARG1(f)
+ : "memory", "cc");
}
static inline notrace void arch_local_irq_disable(void)
{
- PVOP_VCALLEE0(irq.irq_disable);
+ PVOP_CALL_ARGS;
+ PVOP_TEST_NULL(irq.irq_disable);
+ asm_inline volatile(ALTERNATIVE(paravirt_alt(PARAVIRT_CALL),
+ "cli;",
+ X86_FEATURE_NOT_XENPV)
+ : PVOP_VCALLEE_CLOBBERS, ASM_CALL_CONSTRAINT
+ : paravirt_type(irq.irq_disable.func),
+ paravirt_clobber(CLBR_RET_REG)
+ : "memory", "cc");
}
static inline notrace void arch_local_irq_enable(void)
{
- PVOP_VCALLEE0(irq.irq_enable);
+ PVOP_CALL_ARGS;
+ PVOP_TEST_NULL(irq.irq_enable);
+ asm_inline volatile(ALTERNATIVE(paravirt_alt(PARAVIRT_CALL),
+ "sti;",
+ X86_FEATURE_NOT_XENPV)
+ : PVOP_VCALLEE_CLOBBERS, ASM_CALL_CONSTRAINT
+ : paravirt_type(irq.irq_enable.func),
+ paravirt_clobber(CLBR_RET_REG)
+ : "memory", "cc");
}
static inline notrace unsigned long arch_local_irq_save(void)
diff --git a/arch/x86/kernel/alternative.c b/arch/x86/kernel/alternative.c
index 2400ad62f330..5bd8f5503652 100644
--- a/arch/x86/kernel/alternative.c
+++ b/arch/x86/kernel/alternative.c
@@ -723,6 +723,19 @@ void __init alternative_instructions(void)
* patching.
*/
+ if (!boot_cpu_has(X86_FEATURE_XENPV))
+ setup_force_cpu_cap(X86_FEATURE_NOT_XENPV);
+
+ /*
+ * First patch paravirt functions, such that we overwrite the indirect
+ * call with the direct call.
+ */
+ apply_paravirt(__parainstructions, __parainstructions_end);
+
+ /*
+ * Then patch alternatives, such that those paravirt calls that are in
+ * alternatives can be overwritten by their immediate fragments.
+ */
apply_alternatives(__alt_instructions, __alt_instructions_end);
#ifdef CONFIG_SMP
@@ -741,8 +754,6 @@ void __init alternative_instructions(void)
}
#endif
- apply_paravirt(__parainstructions, __parainstructions_end);
-
restart_nmi();
alternatives_patched = 1;
}
diff --git a/arch/x86/kernel/paravirt_patch.c b/arch/x86/kernel/paravirt_patch.c
index ace6e334cb39..867498db53ad 100644
--- a/arch/x86/kernel/paravirt_patch.c
+++ b/arch/x86/kernel/paravirt_patch.c
@@ -33,13 +33,9 @@ struct patch_xxl {
};
static const struct patch_xxl patch_data_xxl = {
- .irq_irq_disable = { 0xfa }, // cli
- .irq_irq_enable = { 0xfb }, // sti
- .irq_save_fl = { 0x9c, 0x58 }, // pushf; pop %[re]ax
.mmu_read_cr2 = { 0x0f, 0x20, 0xd0 }, // mov %cr2, %[re]ax
.mmu_read_cr3 = { 0x0f, 0x20, 0xd8 }, // mov %cr3, %[re]ax
.mmu_write_cr3 = { 0x0f, 0x22, 0xdf }, // mov %rdi, %cr3
- .irq_restore_fl = { 0x57, 0x9d }, // push %rdi; popfq
.cpu_wbinvd = { 0x0f, 0x09 }, // wbinvd
.cpu_usergs_sysret64 = { 0x0f, 0x01, 0xf8,
0x48, 0x0f, 0x07 }, // swapgs; sysretq
@@ -76,11 +72,6 @@ unsigned int native_patch(u8 type, void *insn_buff, unsigned long addr,
switch (type) {
#ifdef CONFIG_PARAVIRT_XXL
- PATCH_CASE(irq, restore_fl, xxl, insn_buff, len);
- PATCH_CASE(irq, save_fl, xxl, insn_buff, len);
- PATCH_CASE(irq, irq_enable, xxl, insn_buff, len);
- PATCH_CASE(irq, irq_disable, xxl, insn_buff, len);
-
PATCH_CASE(mmu, read_cr2, xxl, insn_buff, len);
PATCH_CASE(mmu, read_cr3, xxl, insn_buff, len);
PATCH_CASE(mmu, write_cr3, xxl, insn_buff, len);
[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]
^ permalink raw reply related [flat|nested] 32+ messages in thread
* Re: WARNING: can't access registers at asm_common_interrupt
2020-11-16 13:04 ` Peter Zijlstra
@ 2020-11-18 6:47 ` Jürgen Groß
2020-11-18 8:22 ` Peter Zijlstra
0 siblings, 1 reply; 32+ messages in thread
From: Jürgen Groß @ 2020-11-18 6:47 UTC (permalink / raw)
To: Peter Zijlstra
Cc: Andy Lutomirski, Andrew Cooper, Josh Poimboeuf,
Shinichiro Kawasaki, linux-kernel, Nicholas Piggin,
Damien Le Moal, X86 ML
[-- Attachment #1.1.1: Type: text/plain, Size: 6935 bytes --]
On 16.11.20 14:04, Peter Zijlstra wrote:
> On Mon, Nov 16, 2020 at 12:56:32PM +0100, Jürgen Groß wrote:
>>>>>>>> static inline notrace unsigned long arch_local_save_flags(void)
>>>>>>>> {
>>>>>>>> PVOP_CALL_ARGS;
>>>>>>>> PVOP_TEST_NULL(irq.save_fl);
>>>>>>>> asm_inline volatile(ALTERNATIVE(paravirt_alt(PARAVIRT_CALL),
>>>>>>>> "PUSHF; POP _ASM_AX",
>>>>>>>> X86_FEATURE_NATIVE)
>>
>> I am wondering whether we really want a new feature (basically "not
>> XENPV). We could use ~X86_FEATURE_XENPV and teach apply_alternatives()
>> to understand negated features (yes, this limits the number of features
>> to 32767, but I don't think this is a real problem for quite some time).
>>
>> Thoughts?
>
> I went with the simple thing for now... If we ever want/need another
> negative alternative I suppose we can do as you suggest...
>
> I was still poking at objtool to actually dtrt though..
I'd like to include this part in my series (with a different solution
for the restore_fl part, as suggested by Andy before).
Peter, are you fine with me taking your patch and adding your SoB?
Juergen
>
> ---
> diff --git a/arch/x86/include/asm/cpufeatures.h b/arch/x86/include/asm/cpufeatures.h
> index dad350d42ecf..cc88f358bac5 100644
> --- a/arch/x86/include/asm/cpufeatures.h
> +++ b/arch/x86/include/asm/cpufeatures.h
> @@ -237,6 +237,7 @@
> #define X86_FEATURE_VMCALL ( 8*32+18) /* "" Hypervisor supports the VMCALL instruction */
> #define X86_FEATURE_VMW_VMMCALL ( 8*32+19) /* "" VMware prefers VMMCALL hypercall instruction */
> #define X86_FEATURE_SEV_ES ( 8*32+20) /* AMD Secure Encrypted Virtualization - Encrypted State */
> +#define X86_FEATURE_NOT_XENPV ( 8*32+21) /* "" inverse of X86_FEATURE_XENPV */
>
> /* Intel-defined CPU features, CPUID level 0x00000007:0 (EBX), word 9 */
> #define X86_FEATURE_FSGSBASE ( 9*32+ 0) /* RDFSBASE, WRFSBASE, RDGSBASE, WRGSBASE instructions*/
> diff --git a/arch/x86/include/asm/paravirt.h b/arch/x86/include/asm/paravirt.h
> index d25cc6830e89..e2a9d3e6b7ad 100644
> --- a/arch/x86/include/asm/paravirt.h
> +++ b/arch/x86/include/asm/paravirt.h
> @@ -645,22 +645,56 @@ bool __raw_callee_save___native_vcpu_is_preempted(long cpu);
> #ifdef CONFIG_PARAVIRT_XXL
> static inline notrace unsigned long arch_local_save_flags(void)
> {
> - return PVOP_CALLEE0(unsigned long, irq.save_fl);
> + PVOP_CALL_ARGS;
> + PVOP_TEST_NULL(irq.save_fl);
> + asm_inline volatile(ALTERNATIVE(paravirt_alt(PARAVIRT_CALL),
> + "pushf; pop %%" _ASM_AX ";",
> + X86_FEATURE_NOT_XENPV)
> + : PVOP_CALLEE_CLOBBERS, ASM_CALL_CONSTRAINT
> + : paravirt_type(irq.save_fl.func),
> + paravirt_clobber(CLBR_RET_REG)
> + : "memory", "cc");
> + return __eax;
> }
>
> static inline notrace void arch_local_irq_restore(unsigned long f)
> {
> - PVOP_VCALLEE1(irq.restore_fl, f);
> + PVOP_CALL_ARGS;
> + PVOP_TEST_NULL(irq.restore_fl);
> + asm_inline volatile(ALTERNATIVE(paravirt_alt(PARAVIRT_CALL),
> + "push %%" _ASM_ARG1 "; popf;",
> + X86_FEATURE_NOT_XENPV)
> + : PVOP_VCALLEE_CLOBBERS, ASM_CALL_CONSTRAINT
> + : paravirt_type(irq.restore_fl.func),
> + paravirt_clobber(CLBR_RET_REG),
> + PVOP_CALL_ARG1(f)
> + : "memory", "cc");
> }
>
> static inline notrace void arch_local_irq_disable(void)
> {
> - PVOP_VCALLEE0(irq.irq_disable);
> + PVOP_CALL_ARGS;
> + PVOP_TEST_NULL(irq.irq_disable);
> + asm_inline volatile(ALTERNATIVE(paravirt_alt(PARAVIRT_CALL),
> + "cli;",
> + X86_FEATURE_NOT_XENPV)
> + : PVOP_VCALLEE_CLOBBERS, ASM_CALL_CONSTRAINT
> + : paravirt_type(irq.irq_disable.func),
> + paravirt_clobber(CLBR_RET_REG)
> + : "memory", "cc");
> }
>
> static inline notrace void arch_local_irq_enable(void)
> {
> - PVOP_VCALLEE0(irq.irq_enable);
> + PVOP_CALL_ARGS;
> + PVOP_TEST_NULL(irq.irq_enable);
> + asm_inline volatile(ALTERNATIVE(paravirt_alt(PARAVIRT_CALL),
> + "sti;",
> + X86_FEATURE_NOT_XENPV)
> + : PVOP_VCALLEE_CLOBBERS, ASM_CALL_CONSTRAINT
> + : paravirt_type(irq.irq_enable.func),
> + paravirt_clobber(CLBR_RET_REG)
> + : "memory", "cc");
> }
>
> static inline notrace unsigned long arch_local_irq_save(void)
> diff --git a/arch/x86/kernel/alternative.c b/arch/x86/kernel/alternative.c
> index 2400ad62f330..5bd8f5503652 100644
> --- a/arch/x86/kernel/alternative.c
> +++ b/arch/x86/kernel/alternative.c
> @@ -723,6 +723,19 @@ void __init alternative_instructions(void)
> * patching.
> */
>
> + if (!boot_cpu_has(X86_FEATURE_XENPV))
> + setup_force_cpu_cap(X86_FEATURE_NOT_XENPV);
> +
> + /*
> + * First patch paravirt functions, such that we overwrite the indirect
> + * call with the direct call.
> + */
> + apply_paravirt(__parainstructions, __parainstructions_end);
> +
> + /*
> + * Then patch alternatives, such that those paravirt calls that are in
> + * alternatives can be overwritten by their immediate fragments.
> + */
> apply_alternatives(__alt_instructions, __alt_instructions_end);
>
> #ifdef CONFIG_SMP
> @@ -741,8 +754,6 @@ void __init alternative_instructions(void)
> }
> #endif
>
> - apply_paravirt(__parainstructions, __parainstructions_end);
> -
> restart_nmi();
> alternatives_patched = 1;
> }
> diff --git a/arch/x86/kernel/paravirt_patch.c b/arch/x86/kernel/paravirt_patch.c
> index ace6e334cb39..867498db53ad 100644
> --- a/arch/x86/kernel/paravirt_patch.c
> +++ b/arch/x86/kernel/paravirt_patch.c
> @@ -33,13 +33,9 @@ struct patch_xxl {
> };
>
> static const struct patch_xxl patch_data_xxl = {
> - .irq_irq_disable = { 0xfa }, // cli
> - .irq_irq_enable = { 0xfb }, // sti
> - .irq_save_fl = { 0x9c, 0x58 }, // pushf; pop %[re]ax
> .mmu_read_cr2 = { 0x0f, 0x20, 0xd0 }, // mov %cr2, %[re]ax
> .mmu_read_cr3 = { 0x0f, 0x20, 0xd8 }, // mov %cr3, %[re]ax
> .mmu_write_cr3 = { 0x0f, 0x22, 0xdf }, // mov %rdi, %cr3
> - .irq_restore_fl = { 0x57, 0x9d }, // push %rdi; popfq
> .cpu_wbinvd = { 0x0f, 0x09 }, // wbinvd
> .cpu_usergs_sysret64 = { 0x0f, 0x01, 0xf8,
> 0x48, 0x0f, 0x07 }, // swapgs; sysretq
> @@ -76,11 +72,6 @@ unsigned int native_patch(u8 type, void *insn_buff, unsigned long addr,
> switch (type) {
>
> #ifdef CONFIG_PARAVIRT_XXL
> - PATCH_CASE(irq, restore_fl, xxl, insn_buff, len);
> - PATCH_CASE(irq, save_fl, xxl, insn_buff, len);
> - PATCH_CASE(irq, irq_enable, xxl, insn_buff, len);
> - PATCH_CASE(irq, irq_disable, xxl, insn_buff, len);
> -
> PATCH_CASE(mmu, read_cr2, xxl, insn_buff, len);
> PATCH_CASE(mmu, read_cr3, xxl, insn_buff, len);
> PATCH_CASE(mmu, write_cr3, xxl, insn_buff, len);
>
[-- Attachment #1.1.2: OpenPGP_0xB0DE9DD628BF132F.asc --]
[-- Type: application/pgp-keys, Size: 3135 bytes --]
[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 495 bytes --]
^ permalink raw reply [flat|nested] 32+ messages in thread
* Re: WARNING: can't access registers at asm_common_interrupt
2020-11-18 6:47 ` Jürgen Groß
@ 2020-11-18 8:22 ` Peter Zijlstra
2020-11-19 11:51 ` Shinichiro Kawasaki
0 siblings, 1 reply; 32+ messages in thread
From: Peter Zijlstra @ 2020-11-18 8:22 UTC (permalink / raw)
To: Jürgen Groß
Cc: Andy Lutomirski, Andrew Cooper, Josh Poimboeuf,
Shinichiro Kawasaki, linux-kernel, Nicholas Piggin,
Damien Le Moal, X86 ML
On Wed, Nov 18, 2020 at 07:47:36AM +0100, Jürgen Groß wrote:
> On 16.11.20 14:04, Peter Zijlstra wrote:
> > On Mon, Nov 16, 2020 at 12:56:32PM +0100, Jürgen Groß wrote:
> > > > > > > > > static inline notrace unsigned long arch_local_save_flags(void)
> > > > > > > > > {
> > > > > > > > > PVOP_CALL_ARGS;
> > > > > > > > > PVOP_TEST_NULL(irq.save_fl);
> > > > > > > > > asm_inline volatile(ALTERNATIVE(paravirt_alt(PARAVIRT_CALL),
> > > > > > > > > "PUSHF; POP _ASM_AX",
> > > > > > > > > X86_FEATURE_NATIVE)
> > >
> > > I am wondering whether we really want a new feature (basically "not
> > > XENPV). We could use ~X86_FEATURE_XENPV and teach apply_alternatives()
> > > to understand negated features (yes, this limits the number of features
> > > to 32767, but I don't think this is a real problem for quite some time).
> > >
> > > Thoughts?
> >
> > I went with the simple thing for now... If we ever want/need another
> > negative alternative I suppose we can do as you suggest...
> >
> > I was still poking at objtool to actually dtrt though..
>
> I'd like to include this part in my series (with a different solution
> for the restore_fl part, as suggested by Andy before).
>
> Peter, are you fine with me taking your patch and adding your SoB?
Sure, note that I only compile tested it, as it was my test-bed for
playing with objtool (which I still haven't managed to get back to and
finish :/).
So if it actually _works_ feel free to take it, otherwise you can have
whatever bits and pieces remain after you've butchered it to do as you
want.
^ permalink raw reply [flat|nested] 32+ messages in thread
* Re: WARNING: can't access registers at asm_common_interrupt
2020-11-18 8:22 ` Peter Zijlstra
@ 2020-11-19 11:51 ` Shinichiro Kawasaki
2020-11-19 12:01 ` Peter Zijlstra
0 siblings, 1 reply; 32+ messages in thread
From: Shinichiro Kawasaki @ 2020-11-19 11:51 UTC (permalink / raw)
To: Peter Zijlstra
Cc: Jürgen Groß,
Andy Lutomirski, Andrew Cooper, Josh Poimboeuf, linux-kernel,
Nicholas Piggin, Damien Le Moal, X86 ML
On Nov 18, 2020 / 09:22, Peter Zijlstra wrote:
> On Wed, Nov 18, 2020 at 07:47:36AM +0100, Jürgen Groß wrote:
> > On 16.11.20 14:04, Peter Zijlstra wrote:
> > > On Mon, Nov 16, 2020 at 12:56:32PM +0100, Jürgen Groß wrote:
> > > > > > > > > > static inline notrace unsigned long arch_local_save_flags(void)
> > > > > > > > > > {
> > > > > > > > > > PVOP_CALL_ARGS;
> > > > > > > > > > PVOP_TEST_NULL(irq.save_fl);
> > > > > > > > > > asm_inline volatile(ALTERNATIVE(paravirt_alt(PARAVIRT_CALL),
> > > > > > > > > > "PUSHF; POP _ASM_AX",
> > > > > > > > > > X86_FEATURE_NATIVE)
> > > >
> > > > I am wondering whether we really want a new feature (basically "not
> > > > XENPV). We could use ~X86_FEATURE_XENPV and teach apply_alternatives()
> > > > to understand negated features (yes, this limits the number of features
> > > > to 32767, but I don't think this is a real problem for quite some time).
> > > >
> > > > Thoughts?
> > >
> > > I went with the simple thing for now... If we ever want/need another
> > > negative alternative I suppose we can do as you suggest...
> > >
> > > I was still poking at objtool to actually dtrt though..
> >
> > I'd like to include this part in my series (with a different solution
> > for the restore_fl part, as suggested by Andy before).
> >
> > Peter, are you fine with me taking your patch and adding your SoB?
>
> Sure, note that I only compile tested it, as it was my test-bed for
> playing with objtool (which I still haven't managed to get back to and
> finish :/).
>
> So if it actually _works_ feel free to take it, otherwise you can have
> whatever bits and pieces remain after you've butchered it to do as you
> want.
All, thank you for the actions.
I tried Peter's patch in my test system and result looks good. The warning is
still observed with 5.10-rc4. With the patch on top of 5.10-rc4, the warning
disappeared.
Of note is that when I built kernel with the patch, objtool reported many
warnings, as follows:
arch/x86/events/intel/core.o: warning: objtool: .altinstr_replacement+0x0: alternative modifies stack
arch/x86/events/core.o: warning: objtool: .altinstr_replacement+0x0: alternative modifies stack
--
Best Regards,
Shin'ichiro Kawasaki
^ permalink raw reply [flat|nested] 32+ messages in thread
* Re: WARNING: can't access registers at asm_common_interrupt
2020-11-19 11:51 ` Shinichiro Kawasaki
@ 2020-11-19 12:01 ` Peter Zijlstra
2020-11-19 12:28 ` Jürgen Groß
2020-11-19 12:48 ` Shinichiro Kawasaki
0 siblings, 2 replies; 32+ messages in thread
From: Peter Zijlstra @ 2020-11-19 12:01 UTC (permalink / raw)
To: Shinichiro Kawasaki
Cc: Jürgen Groß,
Andy Lutomirski, Andrew Cooper, Josh Poimboeuf, linux-kernel,
Nicholas Piggin, Damien Le Moal, X86 ML
On Thu, Nov 19, 2020 at 11:51:00AM +0000, Shinichiro Kawasaki wrote:
> I tried Peter's patch in my test system and result looks good. The warning is
> still observed with 5.10-rc4. With the patch on top of 5.10-rc4, the warning
> disappeared.
The patch on its own is not sufficient to fix things; there needs to be
an accompanying objtool patch to generate the correct unwind
information.
This patch only ensures objtool has enough information to be able to
dtrt, but as it stands it shouldn't even compile, it should complain
about an alternative trying to modify the stack and bail.
^ permalink raw reply [flat|nested] 32+ messages in thread
* Re: WARNING: can't access registers at asm_common_interrupt
2020-11-19 12:01 ` Peter Zijlstra
@ 2020-11-19 12:28 ` Jürgen Groß
2020-11-19 12:48 ` Shinichiro Kawasaki
1 sibling, 0 replies; 32+ messages in thread
From: Jürgen Groß @ 2020-11-19 12:28 UTC (permalink / raw)
To: Peter Zijlstra, Shinichiro Kawasaki
Cc: Andy Lutomirski, Andrew Cooper, Josh Poimboeuf, linux-kernel,
Nicholas Piggin, Damien Le Moal, X86 ML
[-- Attachment #1.1.1: Type: text/plain, Size: 927 bytes --]
On 19.11.20 13:01, Peter Zijlstra wrote:
> On Thu, Nov 19, 2020 at 11:51:00AM +0000, Shinichiro Kawasaki wrote:
>> I tried Peter's patch in my test system and result looks good. The warning is
>> still observed with 5.10-rc4. With the patch on top of 5.10-rc4, the warning
>> disappeared.
>
> The patch on its own is not sufficient to fix things; there needs to be
> an accompanying objtool patch to generate the correct unwind
> information.
>
> This patch only ensures objtool has enough information to be able to
> dtrt, but as it stands it shouldn't even compile, it should complain
> about an alternative trying to modify the stack and bail.
>
It complains, but doesn't bail.
I'm nearly ready with my series replacing all the custom code paravirt
patches by ALTERNATIVEs. Quite some paravirt macro cruft turned out to
be unused, so there is a rather large cleanup patch included. :-)
Juergen
[-- Attachment #1.1.2: OpenPGP_0xB0DE9DD628BF132F.asc --]
[-- Type: application/pgp-keys, Size: 3135 bytes --]
[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 495 bytes --]
^ permalink raw reply [flat|nested] 32+ messages in thread
* Re: WARNING: can't access registers at asm_common_interrupt
2020-11-19 12:01 ` Peter Zijlstra
2020-11-19 12:28 ` Jürgen Groß
@ 2020-11-19 12:48 ` Shinichiro Kawasaki
1 sibling, 0 replies; 32+ messages in thread
From: Shinichiro Kawasaki @ 2020-11-19 12:48 UTC (permalink / raw)
To: Peter Zijlstra
Cc: Jürgen Groß,
Andy Lutomirski, Andrew Cooper, Josh Poimboeuf, linux-kernel,
Nicholas Piggin, Damien Le Moal, X86 ML
On Nov 19, 2020 / 13:01, Peter Zijlstra wrote:
> On Thu, Nov 19, 2020 at 11:51:00AM +0000, Shinichiro Kawasaki wrote:
> > I tried Peter's patch in my test system and result looks good. The warning is
> > still observed with 5.10-rc4. With the patch on top of 5.10-rc4, the warning
> > disappeared.
>
> The patch on its own is not sufficient to fix things; there needs to be
> an accompanying objtool patch to generate the correct unwind
> information.
>
> This patch only ensures objtool has enough information to be able to
> dtrt, but as it stands it shouldn't even compile, it should complain
> about an alternative trying to modify the stack and bail.
Thanks for the clarification. It was too early to try out... When it gets ready
to try and test, please let me know.
--
Best Regards,
Shin'ichiro Kawasaki
^ permalink raw reply [flat|nested] 32+ messages in thread
* WARNING: can't access registers at asm_common_interrupt
@ 2020-09-06 20:46 syzbot
0 siblings, 0 replies; 32+ messages in thread
From: syzbot @ 2020-09-06 20:46 UTC (permalink / raw)
To: alexandre.chartre, bp, hpa, linux-kernel, luto, mingo, peterz,
syzkaller-bugs, tglx, x86
Hello,
syzbot found the following issue on:
HEAD commit: 59126901 Merge tag 'perf-tools-fixes-for-v5.9-2020-09-03' ..
git tree: upstream
console output: https://syzkaller.appspot.com/x/log.txt?x=11221b5d900000
kernel config: https://syzkaller.appspot.com/x/.config?x=3c5f6ce8d5b68299
dashboard link: https://syzkaller.appspot.com/bug?extid=424f7b15245b9615f293
compiler: gcc (GCC) 10.1.0-syz 20200507
Unfortunately, I don't have any reproducer for this issue yet.
IMPORTANT: if you fix the issue, please add the following tag to the commit:
Reported-by: syzbot+424f7b15245b9615f293@syzkaller.appspotmail.com
WARNING: can't access registers at asm_common_interrupt+0x1e/0x40 arch/x86/include/asm/idtentry.h:572
---
This report is generated by a bot. It may contain errors.
See https://goo.gl/tpsmEJ for more information about syzbot.
syzbot engineers can be reached at syzkaller@googlegroups.com.
syzbot will keep track of this issue. See:
https://goo.gl/tpsmEJ#status for how to communicate with syzbot.
^ permalink raw reply [flat|nested] 32+ messages in thread
end of thread, other threads:[~2020-11-19 12:49 UTC | newest]
Thread overview: 32+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2020-11-06 6:04 WARNING: can't access registers at asm_common_interrupt Shinichiro Kawasaki
2020-11-06 18:06 ` Josh Poimboeuf
2020-11-09 9:10 ` Shinichiro Kawasaki
2020-11-10 3:19 ` Josh Poimboeuf
2020-11-10 9:19 ` Shinichiro Kawasaki
2020-11-11 17:05 ` Josh Poimboeuf
2020-11-11 17:47 ` Peter Zijlstra
2020-11-11 18:13 ` Josh Poimboeuf
2020-11-11 18:46 ` Andrew Cooper
2020-11-11 19:42 ` Peter Zijlstra
2020-11-11 19:59 ` Josh Poimboeuf
2020-11-11 20:07 ` Peter Zijlstra
2020-11-11 20:15 ` Josh Poimboeuf
2020-11-11 20:25 ` Andrew Cooper
2020-11-11 20:39 ` Peter Zijlstra
2020-11-13 17:34 ` Andy Lutomirski
2020-11-14 9:16 ` Jürgen Groß
2020-11-14 18:10 ` Andy Lutomirski
2020-11-15 6:33 ` Jürgen Groß
2020-11-15 16:05 ` Andy Lutomirski
2020-11-15 16:13 ` Jürgen Groß
2020-11-16 11:56 ` Jürgen Groß
2020-11-16 13:04 ` Peter Zijlstra
2020-11-18 6:47 ` Jürgen Groß
2020-11-18 8:22 ` Peter Zijlstra
2020-11-19 11:51 ` Shinichiro Kawasaki
2020-11-19 12:01 ` Peter Zijlstra
2020-11-19 12:28 ` Jürgen Groß
2020-11-19 12:48 ` Shinichiro Kawasaki
2020-11-11 20:35 ` Peter Zijlstra
2020-11-11 20:42 ` Peter Zijlstra
-- strict thread matches above, loose matches on Subject: below --
2020-09-06 20:46 syzbot
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.