Linux Virtualization - Nov 2015 - [PATCH 1/3] x86/xen: Avoid fast syscall path for Xen PV guests

On Wed, Nov 18, 2015 at 12:06 PM, Boris Ostrovsky
<boris.ostrovsky at oracle.com> wrote:
> After 32-bit syscall rewrite, and specifically after commit 5f310f739b4c
> ("x86/entry/32: Re-implement SYSENTER using the new C path"), the
stack
> frame that is passed to xen_sysexit is no longer a "standard" one
(i.e.
> it's not pt_regs).
>
> Since we end up calling xen_iret from xen_sysexit we don't need to fix
> up the stack and instead follow entry_SYSENTER_32's IRET path directly
> to xen_iret.
>
> We can do the same thing for compat mode even though stack does not need
> to be fixed. This will allow us to drop usergs_sysret32 paravirt op (in
> the subsequent patch)
Looks generally quite nice.  Minor comments below:
> --- a/arch/x86/entry/entry_32.S
> +++ b/arch/x86/entry/entry_32.S
> @@ -308,7 +308,8 @@ sysenter_past_esp:
>
>         movl    %esp, %eax
>         call    do_fast_syscall_32
> -       testl   %eax, %eax
> +       /* XEN PV guests always use IRET path */
> +       ALTERNATIVE "testl %eax, %eax", "xor %eax,
%eax", X86_FEATURE_XENPV
>         jz      .Lsyscall_32_done
Could we make this a little less subtle:

ALTERNATIVE "testl %eax, %eax; lz .Lsyscall_32_done", "jmp
.Lsyscasll_32_done", X86_FEATURE_XENPV

Borislav, what do you think?

Ditto for the others.
> diff --git a/arch/x86/include/asm/cpufeature.h
b/arch/x86/include/asm/cpufeature.h
> index e4f8010..0e4fe5b 100644
> --- a/arch/x86/include/asm/cpufeature.h
> +++ b/arch/x86/include/asm/cpufeature.h
> @@ -216,6 +216,7 @@
>  #define X86_FEATURE_PAUSEFILTER ( 8*32+13) /* AMD filtered pause intercept
*/
>  #define X86_FEATURE_PFTHRESHOLD ( 8*32+14) /* AMD pause filter threshold
*/
>  #define X86_FEATURE_VMMCALL     ( 8*32+15) /* Prefer vmmcall to vmcall */
> +#define X86_FEATURE_XENPV       ( 8*32+16) /* Xen paravirtual guest */
>
This bit is highly magical and I think we need Borislav's ack.

--Andy

On Wed, Nov 18, 2015 at 12:21:56PM -0800, Andy Lutomirski
wrote:
> > diff --git a/arch/x86/include/asm/cpufeature.h
b/arch/x86/include/asm/cpufeature.h
> > index e4f8010..0e4fe5b 100644
> > --- a/arch/x86/include/asm/cpufeature.h
> > +++ b/arch/x86/include/asm/cpufeature.h
> > @@ -216,6 +216,7 @@
> >  #define X86_FEATURE_PAUSEFILTER ( 8*32+13) /* AMD filtered pause
intercept */
> >  #define X86_FEATURE_PFTHRESHOLD ( 8*32+14) /* AMD pause filter
threshold */
> >  #define X86_FEATURE_VMMCALL     ( 8*32+15) /* Prefer vmmcall to
vmcall */
> > +#define X86_FEATURE_XENPV       ( 8*32+16) /* Xen paravirtual guest
*/
> >
> 
> This bit is highly magical and I think we need Borislav's ack.
Yeah, that should be

#define X86_FEATURE_XENPV       ( 8*32+16) /* "" Xen paravirtual guest
*/
					      ^^

note the empty "". This prevents it from appearing in /proc/cpuinfo.

-- 
Regards/Gruss,
    Boris.

SUSE Linux GmbH, GF: Felix Imend?rffer, Jane Smithard, Graham Norton, HRB 21284
(AG N?rnberg)
--

On Wed, Nov 18, 2015 at 3:21 PM, Andy Lutomirski <luto at amacapital.net>
wrote:
> On Wed, Nov 18, 2015 at 12:06 PM, Boris Ostrovsky
> <boris.ostrovsky at oracle.com> wrote:
>> After 32-bit syscall rewrite, and specifically after commit
5f310f739b4c
>> ("x86/entry/32: Re-implement SYSENTER using the new C path"),
the stack
>> frame that is passed to xen_sysexit is no longer a "standard"
one (i.e.
>> it's not pt_regs).
>>
>> Since we end up calling xen_iret from xen_sysexit we don't need to
fix
>> up the stack and instead follow entry_SYSENTER_32's IRET path
directly
>> to xen_iret.
>>
>> We can do the same thing for compat mode even though stack does not
need
>> to be fixed. This will allow us to drop usergs_sysret32 paravirt op (in
>> the subsequent patch)
>
> Looks generally quite nice.  Minor comments below:
>
>> --- a/arch/x86/entry/entry_32.S
>> +++ b/arch/x86/entry/entry_32.S
>> @@ -308,7 +308,8 @@ sysenter_past_esp:
>>
>>         movl    %esp, %eax
>>         call    do_fast_syscall_32
>> -       testl   %eax, %eax
>> +       /* XEN PV guests always use IRET path */
>> +       ALTERNATIVE "testl %eax, %eax", "xor %eax,
%eax", X86_FEATURE_XENPV
>>         jz      .Lsyscall_32_done
>
> Could we make this a little less subtle:
>
> ALTERNATIVE "testl %eax, %eax; lz .Lsyscall_32_done", "jmp
> .Lsyscasll_32_done", X86_FEATURE_XENPV
>
> Borislav, what do you think?
>
> Ditto for the others.
Can you just add !xen_pv_domain() to the opportunistic SYSRET check
instead?  Bury the alternatives in that macro, ie.
static_cpu_has(X86_FEATURE_XENPV).  That would likely benefit other
code as well.

--
Brian Gerst

On Wed, Nov 18, 2015 at 12:50 PM, Brian Gerst <brgerst at gmail.com>
wrote:
> On Wed, Nov 18, 2015 at 3:21 PM, Andy Lutomirski <luto at
amacapital.net> wrote:
>> On Wed, Nov 18, 2015 at 12:06 PM, Boris Ostrovsky
>> <boris.ostrovsky at oracle.com> wrote:
>>> After 32-bit syscall rewrite, and specifically after commit
5f310f739b4c
>>> ("x86/entry/32: Re-implement SYSENTER using the new C
path"), the stack
>>> frame that is passed to xen_sysexit is no longer a
"standard" one (i.e.
>>> it's not pt_regs).
>>>
>>> Since we end up calling xen_iret from xen_sysexit we don't need
to fix
>>> up the stack and instead follow entry_SYSENTER_32's IRET path
directly
>>> to xen_iret.
>>>
>>> We can do the same thing for compat mode even though stack does not
need
>>> to be fixed. This will allow us to drop usergs_sysret32 paravirt op
(in
>>> the subsequent patch)
>>
>> Looks generally quite nice.  Minor comments below:
>>
>>> --- a/arch/x86/entry/entry_32.S
>>> +++ b/arch/x86/entry/entry_32.S
>>> @@ -308,7 +308,8 @@ sysenter_past_esp:
>>>
>>>         movl    %esp, %eax
>>>         call    do_fast_syscall_32
>>> -       testl   %eax, %eax
>>> +       /* XEN PV guests always use IRET path */
>>> +       ALTERNATIVE "testl %eax, %eax", "xor %eax,
%eax", X86_FEATURE_XENPV
>>>         jz      .Lsyscall_32_done
>>
>> Could we make this a little less subtle:
>>
>> ALTERNATIVE "testl %eax, %eax; lz .Lsyscall_32_done",
"jmp
>> .Lsyscasll_32_done", X86_FEATURE_XENPV
>>
>> Borislav, what do you think?
>>
>> Ditto for the others.
>
> Can you just add !xen_pv_domain() to the opportunistic SYSRET check
> instead?  Bury the alternatives in that macro, ie.
> static_cpu_has(X86_FEATURE_XENPV).  That would likely benefit other
> code as well.
We could, but that won't help the 64-bit case where we want to keep
the full asm path.

Also, Xen is capable of the equivalent of sysret32 in the compat case.
We might want to enable something like that, and using the existing
opportunistic sysret check may make sense, in which case we wouldn't
want to disable it.

--Andy

On Wed, Nov 18, 2015 at 12:21:56PM -0800, Andy Lutomirski
wrote:
> Could we make this a little less subtle:
> 
> ALTERNATIVE "testl %eax, %eax; lz .Lsyscall_32_done", "jmp
> .Lsyscasll_32_done", X86_FEATURE_XENPV
> 
> Borislav, what do you think?
I don't mind either.

I would've said your version doesn't touch %eax so the result in there
might be useful for callers but all paths do overwrite it, AFAICT.

-- 
Regards/Gruss,
    Boris.

SUSE Linux GmbH, GF: Felix Imend?rffer, Jane Smithard, Graham Norton, HRB 21284
(AG N?rnberg)
--