Re: [PATCH] x86/xsave: Robustify and merge macros

[Borislav Petkov]

On Thu, Apr 02, 2015 at 05:52:10PM +0200, Quentin Casasnovas wrote:
> FWIW I think this looks much nicer!  I have a couple of comments though,
> apologies in advance if they aren't relevant :)

No worries, I very much appreciate the looking at. :)

> I thought the SYSTEM_BOOTING checks were present to make sure we call these
> functions only when the alternative instructions had *not* been applied
> (i.e. when SYSTEM_BOOTING).  We could have added the opposite checks in
> xsave_state()/xrstor_state() to make sure the alternative instructions are
> applied when these are called (i.e. when !SYSTEM_BOOTING).

Well, I think this was a clumsy way to say that we shouldn't be using
the _booting() variants when the system isn't booting anymore:

-       WARN_ON(system_state != SYSTEM_BOOTING);
 
-       if (boot_cpu_has(X86_FEATURE_XSAVES))
-               asm volatile("1:"XSAVES"\n\t"
-                       "2:\n\t"
-                            xstate_fault
-                       : "D" (fx), "m" (*fx), "a" (lmask), "d" (hmask)
-                       :   "memory");
        else
-               asm volatile("1:"XSAVE"\n\t"
-                       "2:\n\t"
-                            xstate_fault
-                       : "D" (fx), "m" (*fx), "a" (lmask), "d" (hmask)
-                       :   "memory");


WRT alternatives, the code didn't have any alternatives invocations
there - it is just a cluttered way of saying:

	if (CPU has XSAVES support)
		XSAVES
	else
		XSAVE

that's it. With exception handling of course.

> Are you not invariably clearing err here?  If the instruction fault, we go
> to label '3' which does 'err = -1; goto 2', which clears err.  Same remark
> for XSTATE_XSAVE()/XSTATE_RESTORE().
> 
> Probably missing something..

No, you're not. The backwards jump label needs to be after the XOR.
Thanks for catching that.

> Also, tiny consistency nit, maybe use "\n\t" everywhere?

Yeah, didn't want to make the macro more unreadable than it is now. The
"\n\t" things are only for when looking at the .s file and almost no one
does that :-)

> I've tried compiling this on top of v4.0-rc5 and I get a compile error
> because alt_end_marker isn't defined.  Which other patches should I take to
> test this?

It needs tip/master.

Let me redo the patch.

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/