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

Borislav Petkov
2015-03-17T12:09:16Z

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