linux-kernel - Re: [PATCH 14/20] x86/barrier: Use alternative

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]

Message-ID: <Z9VE4Ls0vTJDeDMR@gmail.com>
Date: Sat, 15 Mar 2025 10:14:08 +0100
From: Ingo Molnar <mingo@...nel.org>
To: Josh Poimboeuf <jpoimboe@...nel.org>
Cc: Linus Torvalds <torvalds@...ux-foundation.org>, x86@...nel.org,
	linux-kernel@...r.kernel.org, Peter Zijlstra <peterz@...radead.org>,
	Borislav Petkov <bp@...en8.de>, "H. Peter Anvin" <hpa@...or.com>,
	Uros Bizjak <ubizjak@...il.com>,
	Andrew Cooper <andrew.cooper3@...rix.com>
Subject: Re: [PATCH 14/20] x86/barrier: Use alternative_io() in 32-bit
 barrier functions


* Josh Poimboeuf <jpoimboe@...nel.org> wrote:

> On Fri, Mar 14, 2025 at 01:49:48PM -1000, Linus Torvalds wrote:
> > So all of these patches look like good cleanups to me, but I do wonder
> > if we should
> > 
> >  (a) not use some naming *quite* as generic as 'ARG()'
> > 
> >  (b) make the asms use ARG_OUT/ARG_IN/ARG_CLOBBER() to clarify
> > 
> > because that ARG(), ARG(), ARGC() pattern looks odd to me.
> > 
> > Maybe it's just me.
> > 
> > Regardless, I do think the series looks like a nice improvement even
> > in the current form, even if that particular repeated pattern feels
> > strange.
> 
> So originally I had ASM_OUTPUT/ASM_INPUT/ASM_CLOBBER, but I ended up
> going with ARG() due to its nice vertical alignment and conciseness:
> 
> 
> 	__asm_call(qual,						\
> 		ALTERNATIVE("call this_cpu_cmpxchg8b_emu",		\
> 			    "cmpxchg8b " __percpu_arg([var]),		\
> 			    X86_FEATURE_CX8),				\
> 		ARG([var] "+m" (__my_cpu_var(_var)), "+a" (old__.low),	\
> 		    "+d" (old__.high)),					\
> 		ARG("b" (new__.low), "c" (new__.high), "S" (&(_var))),	\
> 		ARG("memory"));						\

Two nits:

1)

In justified cases we can align vertically just fine by using spaces:

  ASM_INPUT  ([var] "+m" (__my_cpu_var(_var)), "+a" (old__.low)...
  ASM_OUTPUT ("b" (new__.low), "c" (new__.high), "S" (&(_var))),
  ASM_CLOBBER("memory")

But I don't think the vertical alignment of visually disjoint, 
comma-separated arguments is an improvement in this specific case.

A *truly* advanced typographically aware syntactic construct would be 
something like:

		ALTERNATIVE("call this_cpu_cmpxchg8b_emu",		\
			    "cmpxchg8b " __percpu_arg([var]),		\
			    X86_FEATURE_CX8),				\
									\
		  ASM_INPUT( [var] "+m" (__my_cpu_var(_var)),		\
				   "+a" (old__.low),			\
				   "+d" (old__.high)),			\
									\
		  ASM_OUTPUT(       "b" (new__.low),			\
				    "c" (new__.high),			\
				    "S" (&(_var))),			\
									\
		  ASM_CLOBBER(	    "memory"));

Note how horizontal and vertical grouping improves readability by an 
order of magnitude and properly highlights the only named operand and 
makes it very easy to review this code, should it be a new submission 
(which it isn't).

And as Knuth said, the intersection of the sets of good coders and good 
typographers is necessarily a tiny percentage of humanity 
(paraphrased), but I digress ...

2)

If 'ARGS' is included in the naming then I'd like to insist on the 
plural 'ARGS', not 'ARG', because the common case for more complicated 
asm() statements is multiple asm template constraint arguments 
separated by commas.

But I don't think we need the 'ARGS':

> Though ASM_OUTPUT/ASM_INPUT/ASM_CLOBBER isn't so bad either:
> 
> 	__asm_call(qual,						\
> 		ALTERNATIVE("call this_cpu_cmpxchg8b_emu",		\
> 			    "cmpxchg8b " __percpu_arg([var]),		\
> 			    X86_FEATURE_CX8),				\
> 		ASM_OUTPUT([var] "+m" (__my_cpu_var(_var)),		\
> 			   "+a" (old__.low), "+d" (old__.high)),	\
> 		ASM_INPUT("b" (new__.low), "c" (new__.high),		\
> 			  "S" (&(_var))),				\
> 		ASM_CLOBBER("memory"));					\
> 
> 
> That has the nice benefit of being more self-documenting, albeit more
> verbose and less vertically aligned.
> 
> So I could go either way, really.

I'd vote on:

	ASM_INPUT(),
	ASM_OUTPUT(),
	ASM_CLOBBER()

... because the ASM_ prefix is already unique enough visually to reset 
any cross-pollination from other well-known namespaces, and because in 
coding shorter is better, all other things equal.

Thanks,

	Ingo