Re: [PATCH RFC 0/2] x86/fpu: avoid "xstate_fault" in xsave_user/xrestore_user

[Quentin Casasnovas]

On Sun, Mar 15, 2015 at 05:49:48PM +0100, Oleg Nesterov wrote:
> Hello.
> 
> Another a bit off-topic change, but I'd like to finish the discussion
> with Quentin.
>
> And almost cosmetic. But I added the RFC tag to make it clear that this
> needs a review from someone who understands gcc-asm better. In particular
> I am worried if that dummy "=m" (*buf) is actually correct.

Derp, I might have given the wrong impression but I'm certainly not that
guy who understands gcc-asm better!

> 
> And I agree with Quentin, user_insn/check_insn can be improved to allow
> clobbers, more flexible "output", etc. But imo they already can make this
> code look a bit better, and "xstate_fault" must die eventually.
>

So I really think we should have a clean user_insn() for which we could add
arbitrary outputs, inputs and clobbers and which would follow more closely
the extended assembly syntax (so it's easier to parse for the elite who
actually understands GCC extended asm ;).  Would something like this be
Okay?

/**
 * This is so multiple outputs/inputs/clobbers are interpreted as a
 * single macro argument.
 */
#define SINGLE_ARG(...) __VA_ARGS__

#define PASTE_1(...) , __VA_ARGS__
#define PASTE_0(...)
#define PASTE__(Empty, ...) PASTE_ ## Empty(__VA_ARGS__)
#define PASTE_(Empty, ...) PASTE__(Empty, __VA_ARGS__)

#define IS_EMPTY(...) 1

/**
 * Prints ", __VA_ARGS__" (note the comma as prefix) if there are any
 * argument and does not print anything otherwise.
 */
#define PASTE(...) PASTE_(IS_EMPTY(__VA_ARGS), __VA_ARGS__)

#define __user_insn(stingified_instructions, outputs, inputs, clobbers) \
	({								\
		int err;						\
		asm volatile(ASM_STAC                  "\n\t"		\
	                     "try:                      \n\t"		\
	                     stringified_instructions			\
	                     "finally:                  \n\t"		\
	                     ASM_CLAC                  "\n\t"		\
			     ".section .fixup,\"ax\"    \n\t"		\
			     "catch:  movl $-1,%[err]   \n\t"		\
			     "    jmp  finallyb         \n\t"		\
			     ".previous                 \n\t"		\
			     _ASM_EXTABLE(tryb, catchb)			\
			     : [err] "=r" (err) PASTE(outputs)		\
			     : "0" (0) PASTE(inputs)			\
			     : clobbers)				\
			err;						\
	})

Then the callers can use SINGLE_ARG() to pass multiple outputs, inputs or
clobbers as a single argument to __user_insn like this:

  __user_insn("btl [var2], %0		\n\t",
  	      , /* no outputs, no need for dummy arg */
	      SINGLE_ARG("r" (var1), [var2] "r" (var2)), /* two inputs */
	      "cc");

As added bonus, we would not need any dummy operand when there is no
outputs, which I think is preferable.

The IS_EMPTY() macro could be implemented like on the following article
  https://gustedt.wordpress.com/2010/06/08/detect-empty-macro-arguments/

We could then write safe variants of the alternative_*() macros to call
check_insn() since I don't think it is possible as is to do
check_insn(alternative_2(...), ...) as you suggested.

>
> Quentin, could you review? I can't find your last email about this change,
> and I can't recall if you agree or not.
> 

I can give it a try, but really not the best person to ack/nack this kind
of changes..

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