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

[Quentin Casasnovas <quentin.casasnovas@oracle.com>]

On Tue, Mar 17, 2015 at 01:07:39PM +0100, Borislav Petkov wrote:
> On Tue, Mar 17, 2015 at 12:36:58PM +0100, Quentin Casasnovas wrote:
> > Right, FWIW I think your approach is valid, but not very generic.  Re-using
> > the check_insn() and making it more generic so we can widen its use felt
> > like a better approach to me.
> > 
> > AIUI, you didn't like my earlier draft because it wasn't very readable, but
> > I think this was just due to the (bad) example I took and by reworking it a
> > bit more, we could end up with the code you previously envisionned:
> > 
> >   if (static_cpu_has_safe(X86_FEATURE_XSAVEOPT))
> >           return check_insn(XSAVEOPT, xsave_buf, ...);
> >   else if (static_cpu_has_safe(X86_FEATURE_XSAVES)
> >           return check_insn(XSAVES, xsave_buf, ...);
> >   else
> > 	  return check_insn(XSAVE, xsave_buf, ...)
> > 
> > Or maybe you were saying the actual macros weren't readable?
> 
> Well, TBH, I don't like check_insn() either:
> 
> * naming is generic but it is not really used in a generic way - only in
> FPU code.

We could make it generic enough so it becomes useful elsewhere as well.

> 
> * having variable arguments makes it really really unreadable to me when
> you start looking at how it is called:
> 
> 	...
>         if (config_enabled(CONFIG_X86_32))
>                 return check_insn(fxrstor %[fx], "=m" (*fx), [fx] "m" (*fx));
> 	...
> 
> The only thing that lets me differentiate what is input and what is
> output is the "=" in there and you have to know inline asm to know that.
>

It gets even worse with the xstate_fault macro which silently includes the
output operands..

> 
> * The arguments have the same syntax as inline asm() arguments but you
> don't see "asm volatile" there so it looks like something half-arsed in
> between.
> 
> * the first argument is the instruction string with the operands which
> gets stringified, yuck!
> 

What if we renamed it to check_asm()/check_user_asm() and have the first
argument be a string, like an asm statement?  So basically check_asm()
would be exactly like an asm() statement except that it'll use a comma to
separate the input, output and clobber operands instead of a colon, and
would protect the first instruction of the assembler template.

        if (config_enabled(CONFIG_X86_32))
                return check_user_asm("fxrstor %[fx]", [fx] "=m" (*fx),,);

Then we can move that macro up the headers so it can be used elsewhere.
Looks more reable to me than how how we'd write that manually:

        if (config_enabled(CONFIG_X86_32) {
                volatile asm(ASM_STAC
		             "1: fxrstor %[fx]        \n\t"
		             "2:		      \n\t"
		             ASM_CLAC
		             ".section .fixup,\"ax\"  \n\t"
		             "3: movl $-1, %0	      \n\t"
		             "   jmp 2b		      \n\t"
		             ".previous		      \n\t"
		             _ASM_EXTABLE(1b, 3b)
		            : "=r" (err), [fx] "=m" (*fx)
		            : : )
	        return err;
	}

> Do I need to say more? :-)
> 
> So what I would like is if we killed those half-arsed macros and
> use either generic, clean macros like the alternatives or define
> FPU-specific ones which do what FPU code needs done. If the second,
> they should be self-contained, all in one place so that you don't have
> to grep like crazy to rhyme together what the macro does - nothing like
> xsave_fault. Yuck.
> 
> Or even extend the generic macros to fit the FPU use case, if possible
> and if it makes sense.
> 
> Oh, and we shouldn't leave readability somewhere on the road.

Readability will be a tough one since gcc extended asm isn't readable (IMO)
and we need to deal with the input/output/clobber operands syntax.

> 
> I hope you catch my drift here.
>

I do agree with all your above points, which is why I drafted that proposal
rework of check_insn() in my first e-mail :)  AFAICT, you were giving
arguments against the current macros, not against my previous proposal.

Quentin