Re: [PATCH 2/4] x86/asm/entry: Untangle 'ia32_sysenter_target' into two entry points: entry_SYSENTER_32 and entry_SYSENTER_compat

[Ingo Molnar <mingo@kernel.org>]

* Andy Lutomirski <luto@amacapital.net> wrote:

> On Mon, Jun 8, 2015 at 1:34 AM, Ingo Molnar <mingo@kernel.org> wrote:
> > So the SYSENTER instruction is pretty quirky and it has different behavior
> > depending on bitness and CPU maker.
> >
> > Yet we create a false sense of coherency by naming it 'ia32_sysenter_target'
> > in both of the cases.
> >
> > Split the name into its two uses:
> >
> >         ia32_sysenter_target (32)    -> entry_SYSENTER_32
> >         ia32_sysenter_target (64)    -> entry_SYSENTER_compat
> >
> 
> Now that I'm rebasing my pile on top of this, I have a minor gripe
> about this one.  There are (in my mind, anyway), two SYSENTER
> instructions: the 32-bit one and the 64-bit one.  (That is, there's
> SYSENTER32, which happens when you do SYSENTER in 32-bit or compat
> mode, and SYSENTER64, which happens when you do SYSENTER in long
> mode.)  SYSENTER32, from user code's perspective, does the same thing
> in either case [1].  That means that it really does make sense that
> we'd have two implementations of the same entry point, one written in
> 32-bit asm and one written in 64-bit asm.
> 
> The patch I'm rebasing merges the two wrmsrs to MSR_IA32_SYSENTER, and
> this change makes it uglier.
> 
> [1] Sort of.  We probably have differently nonsensical calling
> conventions, but that's our fault and has nothing to do with the
> hardware.

Did you intend to merge these two wrmsr()s:

 #ifdef CONFIG_X86_64
 void syscall_init(void)
 {
 ...
         wrmsrl_safe(MSR_IA32_SYSENTER_EIP, (u64)entry_SYSENTER_compat);
 ...
 }
 #endif

 #ifdef CONFIG_X86_32
 void enable_sep_cpu(void)
 {
 ...
         wrmsr(MSR_IA32_SYSENTER_EIP, (unsigned long)entry_SYSENTER_32, 0);
 ...
 }

... and the new bifurcated names preserve the #ifdef, right?

So I mostly agree with you, but still I'm a bit torn about this, for the following 
reason:

 - SYSENTER on a 32-bit kernel behaves a bit differently from SYSENTER on a 64-bit
   kernel: for example on 32-bit kernels we'll return with SYSEXIT, while on
   64-bit kernels we return with SYSRET. The difference is small but user-space
   observable: for example EDX is 0 on SYSRET while it points to ->sysenter_return
   in the SYSEXIT case.

This kind of user-observable assymmetry does not exist for other unified syscall 
ABIs, such as the INT80 method.

So I think that despite having to preserve a small non-unified #ifdef for this 
initialization, we are still better off naming the two entry points differently, 
along the pattern we use, because the behavior is slightly different depending on 
the bitness of the kernel.

Thanks,

	Ingo