fg-----"Jason Gunthorpe" <jgg@xxxxxxxx> wrote: -----
>To: "Bernard Metzler" <BMT@xxxxxxxxxxxxxx>
>From: "Jason Gunthorpe" <jgg@xxxxxxxx>
>Date: 08/06/2019 07:35PM
>Cc: linux-rdma@xxxxxxxxxxxxxxx
>Subject: Re: Re: [PATCH 1/1] Make user mmapped CQ arming flags field
>32 bit size to remove 64 bit architecture dependency of siw.
>
>On Tue, Aug 06, 2019 at 05:01:49PM +0000, Bernard Metzler wrote:
>>
>> >To: "Bernard Metzler" <BMT@xxxxxxxxxxxxxx>
>> >From: "Jason Gunthorpe" <jgg@xxxxxxxx>
>> >Date: 08/06/2019 06:39PM
>> >Cc: linux-rdma@xxxxxxxxxxxxxxx
>> >Subject: Re: Re: [PATCH 1/1] Make user mmapped CQ arming flags
>field
>> >32 bit size to remove 64 bit architecture dependency of siw.
>> >
>> >On Tue, Aug 06, 2019 at 04:36:26PM +0000, Bernard Metzler wrote:
>> >>
>> >> >To: "Bernard Metzler" <BMT@xxxxxxxxxxxxxx>
>> >> >From: "Jason Gunthorpe" <jgg@xxxxxxxx>
>> >> >Date: 08/06/2019 05:31PM
>> >> >Cc: linux-rdma@xxxxxxxxxxxxxxx
>> >> >Subject: Re: Re: [PATCH 1/1] Make user mmapped CQ arming flags
>> >field
>> >> >32 bit size to remove 64 bit architecture dependency of siw.
>> >> >
>> >> >On Tue, Aug 06, 2019 at 02:53:49PM +0000, Bernard Metzler
>wrote:
>> >> >
>> >> >> >> index 7de68f1dc707..af735f55b291 100644
>> >> >> >> +++ b/include/uapi/rdma/siw-abi.h
>> >> >> >> @@ -180,6 +180,7 @@ struct siw_cqe {
>> >> >> >> * to control CQ arming.
>> >> >> >> */
>> >> >> >> struct siw_cq_ctrl {
>> >> >> >> - __aligned_u64 notify;
>> >> >> >> + __u32 flags;
>> >> >> >> + __u32 pad;
>> >> >> >
>> >> >> >The commit message needs to explain why this is compatible
>with
>> >> >> >existing user space, if it is even is safe..
>> >> >> >
>> >> >> Old libsiw would remain compatible with the new layout, since
>it
>> >> >> simply reads the 32bit 'flags' and zeroed 32bit 'pad' into a
>> >64bit
>> >> >> 'notify', ending with reading the same bits.
>> >> >
>> >> >Even on big endian?
>> >> >
>> >> Well I do not have access to a BE system right now to verify.
>> >> But on a BE system, the lowest 3 bits (which are in use) of the
>> >first
>> >> 32bit variable 'flags' shall be the lowest (leftmost) 3 bits of
>an
>> >> 'overlayed' 64bit variable 'notify' as well...
>> >
>> >One of LE or BE won't work with this scheme, it can't, the flag
>bit
>> >will end up in the pad.
>> >
>> Sitting here on a x86 (LE), and it works. On a 64bits machine,
>> two consecutive 32bits are obviously reordered in memory. Leaves
>> 32bit LE broken, which is currently not supported.
>
>? I still think 64 bit BE will be broken as the low two bits will
>overlap the pad, not the new flags.
>
It hurts, but I did finally setup qemu with a ppc image to check,
and so you are right!
...
#ifdef __BIG_ENDIAN__
seem to be available in both kernel and user land...
But, general question: siw in its current shape isn't out
for long, older versions from github are already broken with
the abi. So, silently changing the abi at this stage of siw
deployment is no option? It's a hassle to see an old mistake
carried along forever with that #ifdef statement...no?
Many thanks,
Bernard.
>> Anyway, what would you suggest as the best path forward? A new ABI
>> version? If we move to test_and_clear_bit(), 'flags' size would
>> become architecture dependent...and we would break the ABI as well,
>> no?
>
>Maybe a #ifdef __big_endian__ and swap the order of the pad/flags ?
>
>Jason
>
>
