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

[Borislav Petkov]

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.

* 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.

* 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!

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.

I hope you catch my drift here.

Thanks.

-- 
Regards/Gruss,
    Boris.

ECO tip #101: Trim your mails when you reply.
--
--
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/