linux-kernel - Re: [PATCH 7/9] x86/asm/entry/32: tidy up some instructions

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Date:	Wed, 1 Apr 2015 10:29:35 +0200
From:	Ingo Molnar <mingo@...nel.org>
To:	Denys Vlasenko <dvlasenk@...hat.com>
Cc:	Linus Torvalds <torvalds@...ux-foundation.org>,
	Steven Rostedt <rostedt@...dmis.org>,
	Borislav Petkov <bp@...en8.de>,
	"H. Peter Anvin" <hpa@...or.com>,
	Andy Lutomirski <luto@...capital.net>,
	Oleg Nesterov <oleg@...hat.com>,
	Frederic Weisbecker <fweisbec@...il.com>,
	Alexei Starovoitov <ast@...mgrid.com>,
	Will Drewry <wad@...omium.org>,
	Kees Cook <keescook@...omium.org>, x86@...nel.org,
	linux-kernel@...r.kernel.org
Subject: Re: [PATCH 7/9] x86/asm/entry/32: tidy up some instructions

* Denys Vlasenko <dvlasenk@...hat.com> wrote:

> After TESTs, use logically correct JZ mnemonic instead of JE
> (this doesn't change code).
> 
> Tidy up CMPW insns:
> 
> Modern CPUs are not good with 16-bit operations.
> The instructions with 16-bit immediates are especially bad,
> on many CPUs they cause length changing prefix stall
> in the decoders, costing ~6 cycles to recover.
> 
> Replace CMPWs with CMPLs.
> Of these, for form with 8-bit sign-extended immediates
> it is a win because they are smaller now
> (no 0x66 prefix anymore);
> ones with 16-bit immediates are faster.

This patch does JE->JZ transitions, but it also does CMPW instruction 
tweaking - which was buggy as Brian (miraculously!) noticed.

This isn't the first such incident, and I made this point about three 
times already in the past, but it appears I've not made it loud 
enough: which part of 'do not put two unrelated changes into the same 
patch' did you not understand??

We _DO NOT PUT_ multiple, unrelated changes to assembly files into a 
single patch! And we _especially_ don't mix them up under a 
meaningless, repetitive, misleading 'tidy up instructions' title!

Full stop.

The titles of the two patches should have been something like:

 x86/asm/entry/32: Convert JNE to JNZ mnemonics, to improve readability
 x86/asm/entry/32: Optimize CMPW to CMPL instructions, to make use of automatic zero-extend

We were lucky that Brian was alert enough to have read through a 
misleadingly titled, seemingly harmless patch and noticed the bug in 
your patch, but heck you made it hard!!!

And no, it's not a problem if you create a dozen trivial looking 
patches and have to wait a bit more for them to trickle into the 
maintainer tree: asm patches are seldom trivial, and even if they are 
trivial, both reviewability and bisectability will improve from the 
process.

You are doing a nice job improving the x86/asm/entry code, but if you 
cannot create suitably conservative, maximally reviewable and 
maximally bisectable patches to x86/asm then I won't be able to apply 
assembly patches from you!

</rant>

Thanks,

	Ingo
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@...r.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/