Re: [RFC PATCH 09/10] C-SKY: Linux Syscall Interface

[Mao Han <han_mao at c-sky dot com>]

Hi Adhemerval,
Thanks a lot for the comments!

On Sun, Mar 18, 2018 at 11:19:30AM +0800, Adhemerval Zanella wrote:
> 
> 
> On 16/03/2018 17:58, Mao Han wrote:
> 
> > diff --git a/sysdeps/unix/sysv/linux/csky/abiv2_mmap.S b/sysdeps/unix/sysv/linux/csky/abiv2_mmap.S
> > ...
> > +ENTRY (__mmap)
> > ...
> > +PSEUDO_END (__mmap)
> > +
> > +weak_alias (__mmap, mmap)
> > +libc_hidden_builtin_def (__mmap)
> 
> Is there anything preventing C-SKY to use linux default mmap.c
> implementation? It seems it using a kABI similar to s390 which
> passes all the argument in the stack, so you just need to
> follow the idea of sysdeps/unix/sysv/linux/s390/mmap_internal.h.

OK, I'll try the idea of s390 

> 
> > diff --git a/sysdeps/unix/sysv/linux/csky/abiv2_socket.S b/sysdeps/unix/sysv/linux/csky/abiv2_socket.S
> > ...
> > +ENTRY (__socket)
> > ...
> > +PSEUDO_END(__socket)
> > +
> > +libc_hidden_def (__socket)
> > +#ifndef NO_WEAK_ALIAS
> > +weak_alias (__socket, socket)
> > +#endif
> 
> The socket.S common gate has been removed on all architectures and I
> prefer to continue doing so.  If C-SKY does not follow generic kernel
> ABI which prefer wire-up socket syscalls it is a matter to define
> __ASSUME_SOCKETCALL in kernel-features.h.  Current code should use
> the wire-up call if the syscall is defined, otherwise the socketcall
> is used instead.
>

OK, I'll change to generic kernel ABI.
 
> > diff --git a/sysdeps/unix/sysv/linux/csky/clone.S b/sysdeps/unix/sysv/linux/csky/clone.S
> > ...
> > +        .text
> > +ENTRY(__clone)
> > +	/* Sanity check arguments. */
> > +	cmpnei	r2, 0
> > +	bf	__error_arg	/* no NULL function pointers */
> > +	cmpnei	r3, 0
> > +	bf	__error_arg	/* no NULL function pointers */
> 
> I think you mean no NULL stack pointer here.

Yes, r3 is the child_stack pointer. I'll update the comment.

> 
> > +
> > +	subi	r3, 8
> > +	stw	r2, (r3, 0)	/* insert the args onto the new stack */
> > +	stw	r5, (r3, 4)	/* insert the args onto the new stack */
> > +
> > +	ldw	r5, (sp, 0)	/* arg4 = ctid */
> > +# ifdef RESET_PID
> > +	subi	r3, 8
> > +	stw	r4, (r3, 0x0)	/* save r4(flags) */
> > +# endif
> 
> There is no more RESET_PID in upstream, you can drop this code.

OK.

> > ...
> > +2:
> > +	lrw	r7, PID_OFFSET
> > +	add	r7, r6
> > +	stw	r2, (r7)	/* save pid */
> > +	lrw	r7, TID_OFFSET
> > +	add	r7, r6
> > +	stw	r2, (r7)	/* save tid */
> > +3:
> > +	addi	sp, 8
> > +# endif /* RESET_PID */
> 
> Same as before.

OK

> > ...
> > +int
> > +__ftruncate64 (int fd, off64_t length)
> > +{
> > +  unsigned int low = length & 0xffffffff;
> > +  unsigned int high = length >> 32;
> > +#ifdef __CSKYABIV2__
> > +  int result = INLINE_SYSCALL (ftruncate64, 3, fd,
> > +			       __LONG_LONG_PAIR (high, low));
> > +#else
> > +  int result = INLINE_SYSCALL (ftruncate64, 4, fd, 0,
> > +			       __LONG_LONG_PAIR (high, low));
> > +#endif
> > +  return result;
> > +}
> > +weak_alias (__ftruncate64, ftruncate64)
> 
> With recent INLINE_SYSCALL_CALL, which generic linux ftruncate64 uses, you do not
> need to condicionaly the code for different argument argument number.  You just
> need to define the expected SYSCALL_LL64 macro for the required API and
> INLINE_SYSCALL_CALL will issue the syscall with correct argument.
> 
> So I think this arch-specific implementation is not really required.

OK, I'll try the generic implementation.

> > diff --git a/sysdeps/unix/sysv/linux/csky/mmap.S b/sysdeps/unix/sysv/linux/csky/mmap.S
> > ...
> > +PSEUDO_END (__mmap)
> > +
> > +weak_alias (__mmap, mmap)
> > +libc_hidden_builtin_def (__mmap)
> > +#endif	/* __CSKYABVI2__*/
> 
> Same remark for abiv2_mmap.S applies here.

OK

> > ...
> > +ssize_t
> > +__readahead (int fd, off64_t offset, size_t count)
> > +{
> > +#ifdef __CSKYABVI2__
> > +  return INLINE_SYSCALL (readahead, 4, fd,
> > +			 __LONG_LONG_PAIR ((off_t) (offset >> 32),
> > +					   (off_t) (offset & 0xffffffff)),
> > +			 count);
> > +#else
> > +  return INLINE_SYSCALL (readahead, 5, fd, 0,
> > +			 __LONG_LONG_PAIR ((off_t) (offset >> 32),
> > +					   (off_t) (offset & 0xffffffff)),
> > +			 count);
> > +
> > +#endif
> > +}
> > +
> > +weak_alias (__readahead, readahead)
> 
> I would like to avoid adding another arch-specific Linux implementation while
> we can consolidate Linux implementation.  
> 
> On generic implementation (sysdeps/unix/sysv/linux/readahead.c) we can drop
> the __NR_readahead exist check since for Linux 3.2 the syscall is presented
> in all supported architectures. And for generic implementation we can use
> current macros to handle kABI with a generic call like:
> 
> ssize_t
> __readahead (int fd, off64_t offset, size_t count)
> {
>   return INLINE_SYSCALL_CALL (readahead, fd,
>                               __ALIGNMENT_ARG SYSCALL_LL64 (length));
> }
> 
> So it will be a matter to define both __ALIGNMENT_ARG and SYCALL_LL64 correctly
> for C-SKY, as it is already done for other architectures.
>

OK. I'll check __ALIGNMENT_ARG and SYCALL_LL64 in C-SKY. 
 
> > diff --git a/sysdeps/unix/sysv/linux/csky/socket.S b/sysdeps/unix/sysv/linux/csky/socket.S
> > ...
> > +PSEUDO_END (__socket)
> > +
> > +libc_hidden_def (__socket)
> > +#ifndef NO_WEAK_ALIAS
> > +weak_alias (__socket, socket)
> > +#endif
> > +
> > +#endif	/* __CSKYABIV2__*/
> 
> Same comment for abiv2_socket.S applies here.

OK

Best Regards
 Mao Han