RE: [PATCH v4 6/6] x86/syscall: use int everywhere for system call numbers

[David Laight <David.Laight@ACULAB.COM>]

From: H. Peter Anvin
> Sent: 21 May 2021 22:37
> 
> On 5/20/21 1:53 AM, Thomas Gleixner wrote:
> > On Tue, May 18 2021 at 12:13, H. Peter Anvin wrote:
> >> +static __always_inline bool do_syscall_x64(struct pt_regs *regs, int nr)
> >> +{
> >> +	/*
> >> +	 * Convert negative numbers to very high and thus out of range
> >> +	 * numbers for comparisons. Use unsigned long to slightly
> >> +	 * improve the array_index_nospec() generated code.
> >
> > How is that actually improving the generated code?
> >
> > unsigned long:
> >
> >   104:	48 81 fa bf 01 00 00 	cmp    $0x1bf,%rdx
> >   10b:	48 19 c0             	sbb    %rax,%rax
> >   10e:	48 21 c2             	and    %rax,%rdx
> >   111:	48 89 df             	mov    %rbx,%rdi
> >   114:	48 8b 04 d5 00 00 00 	mov    0x0(,%rdx,8),%rax
> >   11b:	00
> >   11c:	e8 00 00 00 00       	callq  121 <do_syscall_64+0x41>
> >
> > unsigned int:
> >
> >    f1:	48 81 fa bf 01 00 00 	cmp    $0x1bf,%rdx
> >    f8:	48 19 d2             	sbb    %rdx,%rdx
> >    fb:	21 d0                	and    %edx,%eax
> >    fd:	48 89 df             	mov    %rbx,%rdi
> >   100:	48 8b 04 c5 00 00 00 	mov    0x0(,%rax,8),%rax
> >   107:	00
> >   108:	e8 00 00 00 00       	callq  10d <do_syscall_64+0x3d>
> >
> > Text size increases with that unsigned long cast.
> >
> > I must be missing something.
> >
> 
> "unsigned long" gave slightly better code than "int", but as you
> correctly point out here, "unsigned int" is even better.

Indexing arrays with 'int' almost always ends up generating
an extra instruction to sign-extend the 32bit value to 64bits.
This lengthens the register dependency chain as is likely to
add a clock.

OTOH using 'unsigned int' can save a 'reg' prefix (as here)
marginally reducing the cache footprint.
That might speed it up, but may slow it down!
Rather depends on the exact alignment of instructions
relative to (on Intel cpu) the 16-byte fetch/decode blocks.

Looking at the above code, out of range values get masked
to zero to ensure that speculative execution doesn't expose
anything.
If the syscall number is offset by one before masking
a zero will only be generated for invalid values:

https://godbolt.org/z/av839bsxf

bool do_syscall_x64(struct pt_regs *regs, int nr)
{
	unsigned long unr = nr + 1;

	unr = array_index_nospec(unr, NR_syscalls + 1);
	if (!unr)
		return false;
	regs->ax = sys_call_table[unr - 1](regs);
	return true;
}

This speeds up the native system calls with a slight slow down
of the compat ones.

In principle sys_call_table[] could be offset by one.
So that invalid numbers go through sys_call_table[0].
You wouldn't want to do this if a second table follows.

I'm also seeing better code for 'unsigned long'.
Probably because array_index_mask_nospec() is defined for long.

	David

-
Registered Address Lakeside, Bramley Road, Mount Farm, Milton Keynes, MK1 1PT, UK
Registration No: 1397386 (Wales)