Re: [PATCH v2 2/7] y2038: Introduce __ASSUME_64BIT_TIME define

[Lukasz Majewski <lukma at denx dot de>]

Hi Stepan,

> Hi Stepan,
> 
> > Hi Stepan,
> >   
> > > 02.05.2019 в 10:51:08 +0200 Lukasz Majewski написал:    
> > > > Hi Stepan,
> > > >       
> > > > > 30.04.2019 в 11:05:05 +0200 Lukasz Majewski написал:      
> > > > > > IMHO, the abstraction would be:
> > > > > > 
> > > > > > 1. The __ASSUME_64BIT_TIME is _never_ defined for 64 bit
> > > > > > native systems
> > > > > > 
> > > > > > 2. It is defined by default in:
> > > > > > sysdeps/unix/sysv/linux/kernel-features.h for 32 bit systems
> > > > > > (and the actual presence of the syscall is decided upon
> > > > > > definitions of __NR_xxx* (i.e. # ifdef
> > > > > > __NR_clock_settime64).        
> > > > > 
> > > > > I think that __NR_clock_settime64 should be used
> > > > > unconditionally when __ASSUME_64BIT_TIME is defined.      
> > > > 
> > > > Could you clarify it a bit?      
> > > 
> > > I meant something like this:
> > > 
> > > int
> > > __clock_settime64 (clockid_t clock_id, const struct __timespec64
> > > *tp) {
> > > #ifdef __ASSUME_64BIT_TIME
> > >   return INLINE_SYSCALL_CALL (clock_settime64, clock_id, tp);
> > > #else
> > > …
> > > 
> > > But I see now that most of the existing code would just miscompile
> > > in cases where __ASSUME_* is defined while corresponding __NR_* is
> > > not.   
> > > > In the code as proposed in:
> > > > https://patchwork.ozlabs.org/patch/1092583/
> > > > 
> > > > The call to clock_settime64 is protected with # ifdef
> > > > __NR_clock_settime64 - otherwise we do a fallback to (32 bit)
> > > > clock_settime.
> > > > 
> > > > Moreover, the # ifdef __ASSUME_64BIT_TIME provides a fallback
> > > > path if kernel version is older than 5.1.      
> > > 
> > > The fallback would be wrong in cases where __NR_clock_settime is
> > > not defined or is not 32-bit.
> > >     
> > > > > > As those syscalls are provided on almost every 32 bit system
> > > > > > now (5.1-rc6):
> > > > > > git grep -n "clock_settime64"
> > > > > > 
> > > > > > gives support for: arm, arm64 (compat mode), m68k,
> > > > > > microblaze, mips, parisc, powerpc, s390, sh, sparc, x86,
> > > > > > xtensa
> > > > > > 
> > > > > > So it would be reasonable to just add this __ASSUME
> > > > > > definition code to
> > > > > > sysdeps/unix/sysv/linux/kernel-features.h and #undef it for
> > > > > > architectures not supporting it (i.e. c-sky and
> > > > > > riscv).        
> > > > > 
> > > > > I believe that the only 32-bit architecture without
> > > > > __NR_clock_settime64 is x32.       
> > > > 
> > > > Ok, I see. 
> > > > 
> > > > Please correct me - would it be feasible to just #undef
> > > > __ASSYME_TIME64_SYSCALLS for x32 ?      
> > > 
> > > You'll need to know whether to use __NR_clock_settime64 or
> > > __NR_clock_settime in cases where __TIMESIZE == 64 and
> > > __WORDSIZE == 32.  
> 
> Please correct me, but I do have some doubts here.
> 
> As x32 now uses 64 bit time (and has TIMESIZE==64) - it uses the
> clock_settime call (with in-kernel broken tv_nsec padding clearing -
> but for this the fix is in its way to upstream).
> 
> Why does it need to also support clock_settime64 ? 

I was too eager to send the mail.

It is connected with your proposition to use __WORDSIZE for #ifdef on
__ASSUME_TIME64_SYSCALLS. 

As x32 has __WORDSIZE=32 (but __TIMESIZE=64), it would fall into
"category" of archs using explicit 64 bit calls (i.e. clock_settime64).

However, for it - those shall be replaced with syscalls used up till now
(i.e. clock_settime).

Am I right here ?

> 
> > > 
> > > One way would be by defining __ASSUME_TIME64_SYSCALLS
> > > unconditionally on x32 and then defining __NR_clock_settime64 to
> > > __NR_clock_settime when __ASSUME_TIME64_SYSCALLS is defined while
> > > __NR_clock_settime64 isn't.
> > >     
> > 
> > I see. Thanks for the hint.
> >   
> > > > > While newer 32-bit architectures like
> > > > > riscv do not have __NR_clock_settime:
> > > > > https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/commit/?id=d4c08b9776b392e20efc6198ebe1bc8ec1911d9b      
> > > > 
> > > > Then it shall use clock_settime64 from the outset if support
> > > > added.      
> > > 
> > > It probably should have __TIMESIZE == 64 though.    
> > 
> > 
> > Best regards,
> > 
> > Lukasz Majewski
> > 
> > --
> > 
> > DENX Software Engineering GmbH,      Managing Director: Wolfgang
> > Denk HRB 165235 Munich, Office: Kirchenstr.5, D-82194 Groebenzell,
> > Germany Phone: (+49)-8142-66989-59 Fax: (+49)-8142-66989-80 Email:
> > lukma@denx.de  
> 
> 
> 
> 
> Best regards,
> 
> Lukasz Majewski
> 
> --
> 
> DENX Software Engineering GmbH,      Managing Director: Wolfgang Denk
> HRB 165235 Munich, Office: Kirchenstr.5, D-82194 Groebenzell, Germany
> Phone: (+49)-8142-66989-59 Fax: (+49)-8142-66989-80 Email:
> lukma@denx.de




Best regards,

Lukasz Majewski

--

DENX Software Engineering GmbH,      Managing Director: Wolfgang Denk
HRB 165235 Munich, Office: Kirchenstr.5, D-82194 Groebenzell, Germany
Phone: (+49)-8142-66989-59 Fax: (+49)-8142-66989-80 Email: lukma@denx.de

Attachment: pgpeYKDz5ZL6b.pgp
Description: OpenPGP digital signature