Groups | Search | Server Info | Keyboard shortcuts | Login | Register [http] [https] [nntp] [nntps]

Groups > linux.kernel > #1457948 > unrolled thread

[PATCH] x86/hweight: Don't clobber %rdi

Back to article view | Back to linux.kernel

Contents

  [PATCH] x86/hweight: Don't clobber %rdi ville.syrjala@linux.intel.com - 2016-08-08 19:40 +0200
    Re: [PATCH] x86/hweight: Don't clobber %rdi Linus Torvalds <torvalds@linux-foundation.org> - 2016-08-08 20:00 +0200
      Re: [PATCH] x86/hweight: Don't clobber %rdi Peter Zijlstra <peterz@infradead.org> - 2016-08-08 20:40 +0200
        Re: [PATCH] x86/hweight: Don't clobber %rdi Jiri Kosina <jikos@kernel.org> - 2016-08-08 21:00 +0200
          Re: [PATCH] x86/hweight: Don't clobber %rdi Borislav Petkov <bp@alien8.de> - 2016-08-08 21:40 +0200
    Re: [PATCH] x86/hweight: Don't clobber %rdi Borislav Petkov <bp@alien8.de> - 2016-08-08 20:10 +0200
      Re: [PATCH] x86/hweight: Don't clobber %rdi Linus Torvalds <torvalds@linux-foundation.org> - 2016-08-08 20:30 +0200
        Re: [PATCH] x86/hweight: Don't clobber %rdi Borislav Petkov <bp@alien8.de> - 2016-08-08 20:40 +0200
          Re: [PATCH] x86/hweight: Don't clobber %rdi Linus Torvalds <torvalds@linux-foundation.org> - 2016-08-08 20:50 +0200
            Re: [PATCH] x86/hweight: Don't clobber %rdi Borislav Petkov <bp@alien8.de> - 2016-08-08 21:00 +0200

#1457948 — [PATCH] x86/hweight: Don't clobber %rdi

From: Ville Syrjälä <ville.syrjala@linux.intel.com>

The caller expects %rdi to remain intact, push+pop it make that happen.

Fixes the following kind of explosions on my core2duo machine when
trying to reboot or shut down:

general protection fault: 0000 [#1] PREEMPT SMP
Modules linked in: i915 i2c_algo_bit drm_kms_helper cfbfillrect syscopyarea cfbimgblt sysfillrect sysimgblt fb_sys_fops cfbcopyarea drm netconsole configfs binfmt_misc iTCO_wdt psmouse pcspkr snd_hda_codec_idt e100 coretemp hwmon snd_hda_codec_generic i2c_i801 mii i2c_smbus lpc_ich mfd_core snd_hda_intel uhci_hcd snd_hda_codec snd_hwdep snd_hda_core ehci_pci 8250 ehci_hcd snd_pcm 8250_base usbcore evdev serial_core usb_common parport_pc parport snd_timer snd soundcore
CPU: 0 PID: 3070 Comm: reboot Not tainted 4.8.0-rc1-perf-dirty #69
Hardware name:                  /D946GZIS, BIOS TS94610J.86A.0087.2007.1107.1049 11/07/2007
task: ffff88012a0b4080 task.stack: ffff880123850000
RIP: 0010:[<ffffffff81003c92>]  [<ffffffff81003c92>] x86_perf_event_update+0x52/0xc0
RSP: 0018:ffff880123853b60  EFLAGS: 00010087
RAX: 0000000000000001 RBX: ffff88012fc0a3c0 RCX: 000000000000001e
RDX: 0000000000000000 RSI: 0000000040000000 RDI: ffff88012b014800
RBP: ffff880123853b88 R08: ffffffffffffffff R09: 0000000000000000
R10: ffffea0004a012c0 R11: ffffea0004acedc0 R12: ffffffff80000001
R13: ffff88012b0149c0 R14: ffff88012b014800 R15: 0000000000000018
FS:  00007f8b155cd700(0000) GS:ffff88012fc00000(0000) knlGS:0000000000000000
CS:  0010 DS: 0000 ES: 0000 CR0: 0000000080050033
CR2: 00007f8b155f5000 CR3: 000000012a2d7000 CR4: 00000000000006f0
Stack:
 ffff88012fc0a3c0 ffff88012b014800 0000000000000004 0000000000000001
 ffff88012fc1b750 ffff880123853bb0 ffffffff81003d59 ffff88012b014800
 ffff88012fc0a3c0 ffff88012b014800 ffff880123853bd8 ffffffff81003e13
Call Trace:
 [<ffffffff81003d59>] x86_pmu_stop+0x59/0xd0
 [<ffffffff81003e13>] x86_pmu_del+0x43/0x140
 [<ffffffff8111705d>] event_sched_out.isra.105+0xbd/0x260
 [<ffffffff8111738d>] __perf_remove_from_context+0x2d/0xb0
 [<ffffffff8111745d>] __perf_event_exit_context+0x4d/0x70
 [<ffffffff810c8826>] generic_exec_single+0xb6/0x140
 [<ffffffff81117410>] ? __perf_remove_from_context+0xb0/0xb0
 [<ffffffff81117410>] ? __perf_remove_from_context+0xb0/0xb0
 [<ffffffff810c898f>] smp_call_function_single+0xdf/0x140
 [<ffffffff81113d27>] perf_event_exit_cpu_context+0x87/0xc0
 [<ffffffff81113d73>] perf_reboot+0x13/0x40
 [<ffffffff8107578a>] notifier_call_chain+0x4a/0x70
 [<ffffffff81075ad7>] __blocking_notifier_call_chain+0x47/0x60
 [<ffffffff81075b06>] blocking_notifier_call_chain+0x16/0x20
 [<ffffffff81076a1d>] kernel_restart_prepare+0x1d/0x40
 [<ffffffff81076ae2>] kernel_restart+0x12/0x60
 [<ffffffff81076d56>] SYSC_reboot+0xf6/0x1b0
 [<ffffffff811a823c>] ? mntput_no_expire+0x2c/0x1b0
 [<ffffffff811a83e4>] ? mntput+0x24/0x40
 [<ffffffff811894fc>] ? __fput+0x16c/0x1e0
 [<ffffffff811895ae>] ? ____fput+0xe/0x10
 [<ffffffff81072fc3>] ? task_work_run+0x83/0xa0
 [<ffffffff81001623>] ? exit_to_usermode_loop+0x53/0xc0
 [<ffffffff8100105a>] ? trace_hardirqs_on_thunk+0x1a/0x1c
 [<ffffffff81076e6e>] SyS_reboot+0xe/0x10
 [<ffffffff814c4ba5>] entry_SYSCALL_64_fastpath+0x18/0xa3
Code: 7c 4c 8d af c0 01 00 00 49 89 fe eb 10 48 09 c2 4c 89 e0 49 0f b1 55 00 4c 39 e0 74 35 4d 8b a6 c0 01 00 00 41 8b 8e 60 01 00 00 <0f> 33 8b 35 6e 02 8c 00 48 c1 e2 20 85 f6 7e d2 48 89 d3 89 cf
RIP  [<ffffffff81003c92>] x86_perf_event_update+0x52/0xc0
 RSP <ffff880123853b60>
---[ end trace 7ec95181faf211be ]---
note: reboot[3070] exited with preempt_count 2

Cc: Borislav Petkov <bp@suse.de>
Cc: H. Peter Anvin <hpa@zytor.com>
Cc: Andy Lutomirski <luto@amacapital.net>
Cc: Brian Gerst <brgerst@gmail.com>
Cc: Denys Vlasenko <dvlasenk@redhat.com>
Cc: Linus Torvalds <torvalds@linux-foundation.org>
Cc: Peter Zijlstra <peterz@infradead.org>
Cc: Thomas Gleixner <tglx@linutronix.de>
Cc: Ingo Molnar <mingo@kernel.org>
Fixes: f5967101e9de ("x86/hweight: Get rid of the special calling convention")
Signed-off-by: Ville Syrjälä <ville.syrjala@linux.intel.com>
---
 arch/x86/lib/hweight.S | 2 ++
 1 file changed, 2 insertions(+)

diff --git a/arch/x86/lib/hweight.S b/arch/x86/lib/hweight.S
index 02de3d74d2c5..8a602a1e404a 100644
--- a/arch/x86/lib/hweight.S
+++ b/arch/x86/lib/hweight.S
@@ -35,6 +35,7 @@ ENDPROC(__sw_hweight32)
 
 ENTRY(__sw_hweight64)
 #ifdef CONFIG_X86_64
+	pushq   %rdi
 	pushq   %rdx
 
 	movq    %rdi, %rdx                      # w -> t
@@ -60,6 +61,7 @@ ENTRY(__sw_hweight64)
 	shrq    $56, %rax                       # w = w_tmp >> 56
 
 	popq    %rdx
+	popq    %rdi
 	ret
 #else /* CONFIG_X86_32 */
 	/* We're getting an u64 arg in (%eax,%edx): unsigned long hweight64(__u64 w) */
-- 
2.7.4

[toc] | [next] | [standalone]

#1457953

On Mon, Aug 8, 2016 at 10:35 AM,  <ville.syrjala@linux.intel.com> wrote:
> From: Ville Syrjälä <ville.syrjala@linux.intel.com>
>
> The caller expects %rdi to remain intact, push+pop it make that happen.

Indeed. Applied,

           Linus

[toc] | [prev] | [next] | [standalone]

#1457981

On Mon, Aug 08, 2016 at 10:58:51AM -0700, Linus Torvalds wrote:
> On Mon, Aug 8, 2016 at 10:35 AM,  <ville.syrjala@linux.intel.com> wrote:
> > From: Ville Syrj??l?? <ville.syrjala@linux.intel.com>
> >
> > The caller expects %rdi to remain intact, push+pop it make that happen.
> 
> Indeed. Applied,

See also:

  lkml.kernel.org/r/alpine.LNX.2.00.1608081634530.22028@cbobk.fhfr.pm

[toc] | [prev] | [next] | [standalone]

#1457995

On Mon, 8 Aug 2016, Peter Zijlstra wrote:

> > > The caller expects %rdi to remain intact, push+pop it make that happen.
> > 
> > Indeed. Applied,
> 
> See also:
> 
>   lkml.kernel.org/r/alpine.LNX.2.00.1608081634530.22028@cbobk.fhfr.pm

Ville's patch is the proper thing to do; I still think the comments 
probably should be removed as they might provide more confusion than help, 
but that's a separate discussion.

Thanks,

-- 
Jiri Kosina
SUSE Labs

[toc] | [prev] | [next] | [standalone]

#1458125

On Mon, Aug 08, 2016 at 08:53:16PM +0200, Jiri Kosina wrote:
> Ville's patch is the proper thing to do; I still think the comments 
> probably should be removed as they might provide more confusion than help, 
> but that's a separate discussion.

Right, so while trying to debug this with a pen and paper, I found that
adding a subsript to the w variable makes it more understandable. I.e.,
something like this:

	w_1 -= (w >> 1) & 0x55555555;
	w_2 =  (w_1 & 0x33333333) + ((w_1 >> 2) & 0x33333333);

and then you'd have

	movl %eax, %edx				# w
	shrl %edx				# w >>= 1
	andl $0x55555555, %edx			# (w >> 1) & 0x55555555
	subl %edx, %eax				# w_1 = w - ...

	movl %eax, %edx				# w_1
	shrl $2, %eax				# w_1 >>= 2
	andl $0x33333333, %edx			# (w_1 >> 2) & 0x33333333
	andl $0x33333333, %eax			# w_1 &= 0x33333333
	addl %edx, %eax				# w_2 = ... + ...

...

Anyway, something like that. I find that more readable TBH.

Thanks and thanks for confirming Ville's patch fixes it on your box.

-- 
Regards/Gruss,
    Boris.

ECO tip #101: Trim your mails when you reply.
--

[toc] | [prev] | [next] | [standalone]

#1457965

+ jikos.

On Mon, Aug 08, 2016 at 08:35:29PM +0300, ville.syrjala@linux.intel.com wrote:
> From: Ville Syrjälä <ville.syrjala@linux.intel.com>
> 
> The caller expects %rdi to remain intact, push+pop it make that happen.

Hmm, I've been staring at asm for the last hours and my head is spinning
now, so can you please point me at the exact where this happens. I can't
find it in x86_perf_event_update().

I'm guessing even though %rdi is callee-clobbered but since we're
spelling the asm for gcc, it can't know what happens to %rdi in order to
stash it away for the caller.

In any case, it is not a good idea to do the compiler's work. :-\

Thanks.

-- 
Regards/Gruss,
    Boris.

ECO tip #101: Trim your mails when you reply.
--

[toc] | [prev] | [next] | [standalone]

#1457977

On Mon, Aug 8, 2016 at 11:04 AM, Borislav Petkov <bp@alien8.de> wrote:
>
> Hmm, I've been staring at asm for the last hours and my head is spinning
> now, so can you please point me at the exact where this happens. I can't
> find it in x86_perf_event_update().

The corruption could easily have happened long long before.

With a random register clobbered (by a function that is *not* a C
function - it's a replacement for the "popcnt %rdi, %rax"
instruction), you might end up with memory corruption somewhere, and
then an oops much later.

That said, in this case it's likely something like the the
intel_pmu_init() doing

    c->weight = hweight64(c->idxmsk64);

in arch/x86/events/intel/core.c, which then corrupts something related
to the event constraints, and then you get the oops in
x86_perf_event_update() later.

> In any case, it is not a good idea to do the compiler's work. :-\

The compiler has absolutely nothing to do with this. It's all assembly
language and an inline asm.

We *used* to try to have the compiler generate the code. That's what
caused problems.

               Linus

[toc] | [prev] | [next] | [standalone]

#1457982

On Mon, Aug 08, 2016 at 11:21:20AM -0700, Linus Torvalds wrote:
> ...
> in arch/x86/events/intel/core.c, which then corrupts something related
> to the event constraints, and then you get the oops in
> x86_perf_event_update() later.

Damn.

And I thought that when I hold on to the C ABI and since %rdi is
callee-clobbered, I can simply do "call __sw_hweight64" from within an
asm() statement and it'll all be fine.

Ok, so do you think it would work too if I stated that the input
register gets clobbered:

	asm (ALTERNATIVE("call __sw_hweight64", POPCNT64, X86_FEATURE_POPCNT)
			 : "="REG_OUT (res)
			 : REG_IN (w)
			 : REG_IN);

(untested of course).

Because my primitive way of thinking would go like this: well, the input
register is in the list of clobbers and gcc should take care of stashing
it away if it is live across the hweight call. IOW, let gcc do the
push/pop instead of us doing it explicitly.

Or am I missing some aspect?

> The compiler has absolutely nothing to do with this. It's all assembly
> language and an inline asm.

I meant I shouldn't do the compiler's job by coding __sw_hweightXX in
asm. Even though arch/x86/lib/hweight.S is basically copied gcc asm
output, more or less.

But that got us rid of the special calling convention which was a win in
itself.

Thanks.

-- 
Regards/Gruss,
    Boris.

ECO tip #101: Trim your mails when you reply.
--

[toc] | [prev] | [next] | [standalone]

#1457987

On Mon, Aug 8, 2016 at 11:37 AM, Borislav Petkov <bp@alien8.de> wrote:
>
> Ok, so do you think it would work too if I stated that the input
> register gets clobbered:
>
>         asm (ALTERNATIVE("call __sw_hweight64", POPCNT64, X86_FEATURE_POPCNT)
>                          : "="REG_OUT (res)
>                          : REG_IN (w)
>                          : REG_IN);

That would work (although the clobbered registers have a different
syntax than the in/out registers), but it would be wrong, in my
opinion.

We want the actual POPCNT instruction to be the common case, and that
instruction does *not* clobber any other registers than the output.

So I think it's much better to just say: "the __sw_hweight functions
should have the same semantics as popcnt" (although without the eflags
rules that we don't care about).

There's nothing wrong with keeping it as assembly language - it's not
like it's a maintenance headache once it is written.

               Linus

[toc] | [prev] | [next] | [standalone]

#1457994

On Mon, Aug 08, 2016 at 11:45:58AM -0700, Linus Torvalds wrote:
> That would work (although the clobbered registers have a different
> syntax than the in/out registers), but it would be wrong, in my
> opinion.
> 
> We want the actual POPCNT instruction to be the common case, and that
> instruction does *not* clobber any other registers than the output.

Ok, that's a good point. We would be punishing the common case.

> So I think it's much better to just say: "the __sw_hweight functions
> should have the same semantics as popcnt" (although without the eflags
> rules that we don't care about).
> 
> There's nothing wrong with keeping it as assembly language - it's not
> like it's a maintenance headache once it is written.

Yap, makes a whole lotta sense to me.

Thanks!

-- 
Regards/Gruss,
    Boris.

ECO tip #101: Trim your mails when you reply.
--

[toc] | [prev] | [standalone]

Back to top | Article view | linux.kernel

csiph-web