Re: [PATCH] sysv: linux: Pass 64-bit version of semctl syscall

[Alistair Francis <alistair23 at gmail dot com>]

On Sat, Feb 1, 2020 at 1:07 PM Arnd Bergmann <arnd@arndb.de> wrote:
>
> On Sat, Feb 1, 2020 at 8:57 PM Alistair Francis <alistair23@gmail.com> wrote:
> > On Fri, Jan 31, 2020 at 11:58 PM Arnd Bergmann <arnd@arndb.de> wrote:
> > > On Sat, Feb 1, 2020 at 12:59 AM Alistair Francis <alistair23@gmail.com> wrote:
> > > > On Fri, Jan 31, 2020 at 1:54 PM Arnd Bergmann <arnd@arndb.de> wrote:
> > > > >
> > > > > On Fri, Jan 31, 2020 at 6:11 PM Alistair Francis
> > > > > <alistair.francis@wdc.com> wrote:
> > > > > >
> > > > > > Adjust the semctl syscall to match what the kernel expects. That is pass
> > > > > > a version with a *_high version of sem_otime and sem_ctime in the order
> > > > > > that the kernel expects.
> > > > >
> > > > > > +# ifdef __SEMID_DS_HIGH
> > > > > > +#  if defined (__SEMID_DS_HIGH_END)
> > > > > ...
> > > > > > +#  elif defined (__SEMID_DS_HIGH_SWAP)
> > > > >
> > > > > The three new macros you check for here directly correspond to the
> > > > > possible combinations of the existing __SEM_PAD_AFTER_TIME
> > > > > and __SEM_PAD_BEFORE_TIME macros, right?
> > > >
> > > > Hey Arnd,
> > > >
> > > > I think you are asking why we don't just use the existing macros
> > > > instead of adding more (and keep the core logic as is). Besides the
> > > > naming being confusing then, they two types don't line up. For
> > > > example:
> > > >
> > > > PowerPC has this:
> > > >
> > > >  #define __SEM_PAD_AFTER_TIME 0
> > > >  #define __SEM_PAD_BEFORE_TIME (__TIMESIZE == 32)
> > > > +#define __SEMID_DS_HIGH (__WORDSIZE == 32)
> > > > +#define __SEMID_DS_HIGH_SWAP (__WORDSIZE == 32)
> > > >
> > > > while HPPA has this:
> > > >
> > > >  #define __SEM_PAD_AFTER_TIME 0
> > > >  #define __SEM_PAD_BEFORE_TIME (__TIMESIZE == 32)
> > > > +#define __SEMID_DS_HIGH (__WORDSIZE == 32)
> > > >
> > > > They have different __SEMID_DS macros, but the same __SEM_PAD macros.
> > >
> > > If they don't line up, that probably means that one of them is wrong ;-)
> >
> > Ha, ok I'll double check and update it.
> >
> > Even still, I would rather not use the existing macros as they use
> > __TIMESIZE == 32 and the new ones are using __WORDSIZE == 32.
>
> Ah, I had not noticed that difference.
>
> > We could of course just have a RV32 override that fixes it for RV32, but
> > then we need another override.
> >
> > The main reason not to is that I think it would be confusing to
> > overload the macro like that and I don't see a disadvantage in having
> > a new macro. It makes what is going on (which is already somewhat
> > confusing) more clear.
>
> Makes sense.
>
> Another idea would be to not define a separate struct at all, except for
> mips (which needs special handling anyway), and then define a
> new macro fro the few targets that do need to swap the one word
> with __TIMESIZE==64: s390-32, arm/be, sh/be, m68k and microblaze/be
> and do nothing on the others.

Ah, that is a good idea. I still prefer this way as it means that MIPS
does the same thing as everyone else (just with a different
structure). It also makes it clearer to follow as the values are
always in the right place instead of being swapped around after the
syscall.

I'll fix up the HPPA bug and send a v2.

Alistair

>
>        Arnd