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

Quentin Casasnovas
2015-03-18T09:05:02Z

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