Patches for Linux sound ioctl x32 compatibility

[C. Cartwright]

Only the pcm and control interfaces have been tested.

[H.J. Lu]

On Thu, Apr 4, 2013 at 9:54 AM, C. Cartwright <cjc...@virginmedia.com> wrote:
> Only the pcm and control interfaces have been tested.
>

+ SNDRV_CTL_IOCTL_ELEM_READX32 = _IOWR('U', 0xf2, struct snd_ctl_elem_valueX32),
+ SNDRV_CTL_IOCTL_ELEM_WRITEX32 = _IOWR('U', 0xf3, struct
snd_ctl_elem_valueX32),

Can we use

+ SNDRV_CTL_IOCTL_ELEM_READX32 = _IOWR('U', 0x12, struct snd_ctl_elem_valueX32),
+ SNDRV_CTL_IOCTL_ELEM_WRITEX32 = _IOWR('U', 0x13, struct
snd_ctl_elem_valueX32),

Size of snd_ctl_elem_valueX32 is different from snd_ctl_elem_value32.


--
H.J.

[H.J. Lu]

On Thu, Apr 4, 2013 at 9:54 AM, C. Cartwright <cjc...@virginmedia.com> wrote:

> Only the pcm and control interfaces have been tested.
>

Also you should try to use COMPAT_USE_64BIT_TIME instead of
CONFIG_X86_X32_ABI.

--
H.J.

[H. Peter Anvin]

Finally, we need a Signed-off-by: line on a patch before we can accept
it, sorry.

-hpa

[H.J. Lu]

Can you try this modified patch?

--
H.J.

[C. Cartwright]

It works fine. Thanks for cleaning it up.

[H.J. Lu]

One potential change is to make snd_ctl_elem_valueX32 identical
to snd_ctl_elem_value32 so that we don't need to change control_compat.c
It means we need to change the 64-bit integer alignment in
struct snd_ctl_elem_value in user space. Peter, should we do it?

--
H.J.

[H. Peter Anvin]

On 04/08/2013 08:45 AM, H.J. Lu wrote:
>
> One potential change is to make snd_ctl_elem_valueX32 identical
> to snd_ctl_elem_value32 so that we don't need to change control_compat.c
> It means we need to change the 64-bit integer alignment in
> struct snd_ctl_elem_value in user space. Peter, should we do it?
>

Change it on i386 or x32?

-hpa

--
H. Peter Anvin, Intel Open Source Technology Center
I work for Intel. I don't speak on their behalf.

[H.J. Lu]

On Mon, Apr 8, 2013 at 8:51 AM, H. Peter Anvin <h...@zytor.com> wrote:
> On 04/08/2013 08:45 AM, H.J. Lu wrote:
>>
>> One potential change is to make snd_ctl_elem_valueX32 identical
>> to snd_ctl_elem_value32 so that we don't need to change control_compat.c
>> It means we need to change the 64-bit integer alignment in
>> struct snd_ctl_elem_value in user space. Peter, should we do it?
>>
>
> Change it on i386 or x32?
>

User space struct snd_ctl_elem_value has

struct snd_ctl_elem_value {
struct snd_ctl_elem_id id; /* W: element ID */
unsigned int indirect: 1; /* W: indirect access - obsoleted */
union {
union {
long value[128];
long *value_ptr; /* obsoleted */
} integer;
union {
long long value[64];
long long *value_ptr; /* obsoleted */
} integer64;
union {
unsigned int item[128];
unsigned int *item_ptr; /* obsoleted */
} enumerated;
union {
unsigned char data[512];
unsigned char *data_ptr; /* obsoleted */
} bytes;
struct snd_aes_iec958 iec958;
} value; /* RO */
struct timespec tstamp;
unsigned char reserved[128-sizeof(struct timespec)];
};

and the kernel has

struct snd_ctl_elem_value32 {
struct snd_ctl_elem_id id;
unsigned int indirect; /* bit-field causes misalignment */
union {
s32 integer[128];
unsigned char data[512];
#ifndef CONFIG_X86_64
s64 integer64[64];
#endif
} value;
unsigned char reserved[128];
};

The difference between i386 and x32 is:

union {
long long value[64];
long long *value_ptr; /* obsoleted */
} integer64;

which is aligned to 32-bit for i486 and 64-bit for x32. With something
like

--
diff --git a/include/uapi/sound/asound.h b/include/uapi/sound/asound.h
index 1774a5c..c40a1d15 100644
--- a/include/uapi/sound/asound.h
+++ b/include/uapi/sound/asound.h
@@ -866,6 +866,17 @@ struct snd_ctl_elem_info {
unsigned char reserved[64-4*sizeof(unsigned short)];
};

+#if defined(__x86_64__) && defined(__ILP32__)
+typedef long long __kernel_snd_int64_t __attribute__((aligned(4)));
+struct __kernel_snd_timespec
+ __kernel_snd_int64_t tv_sec;
+ __kernel_snd_int64_t tv_nsec;
+};
+#else
+#define __kernel_snd_int64_t long long
+#define __kernel_snd_timespec timespec
+#endif
+
struct snd_ctl_elem_value {
struct snd_ctl_elem_id id; /* W: element ID */
unsigned int indirect: 1; /* W: indirect access - obsoleted */
@@ -875,7 +886,7 @@ struct snd_ctl_elem_value {
long *value_ptr; /* obsoleted */
} integer;
union {
- long long value[64];
+ __kernel_snd_int64_t value[64];
long long *value_ptr; /* obsoleted */
} integer64;
union {
@@ -888,8 +899,8 @@ struct snd_ctl_elem_value {
} bytes;
struct snd_aes_iec958 iec958;
} value; /* RO */
- struct timespec tstamp;
- unsigned char reserved[128-sizeof(struct timespec)];
+ struct __kernel_snd_timespec tstamp;
+ unsigned char reserved[128-sizeof(struct __kernel_snd_timespec)];
};

struct snd_ctl_tlv {
--

we can use snd_ctl_elem_valueX32 for x32 and we don't need to
change sound/core/control_compat.c.

--
H.J.

[H. Peter Anvin]

Could we simply put __attribute__((aligned(4))) on this union?

-hpa

[H.J. Lu]

__attribute__((aligned(N))) can only be used to increase the alignment
of a field in union. To reduce the alignment of a field, we have to
use __attribute__((aligned(4))) to declare a new type and use it on
the field.

--
H.J.

[H.J. Lu]

But we can use __attribute__((packed)) and explicitly layout
snd_ctl_elem_value for x32.

--
H.J.

--
diff --git a/include/uapi/sound/asound.h b/include/uapi/sound/asound.h

index 1774a5c..51ba385 100644
--- a/include/uapi/sound/asound.h
+++ b/include/uapi/sound/asound.h
@@ -869,6 +869,9 @@ struct snd_ctl_elem_info {

struct snd_ctl_elem_value {
struct snd_ctl_elem_id id; /* W: element ID */
unsigned int indirect: 1; /* W: indirect access - obsoleted */

+#if defined(__x86_64__) && defined(__ILP32__)
+ unsigned int pad: 31;
+#endif

union {
union {
long value[128];

@@ -890,7 +893,11 @@ struct snd_ctl_elem_value {

} value; /* RO */
struct timespec tstamp;
unsigned char reserved[128-sizeof(struct timespec)];

-};
+}
+#if defined(__x86_64__) && defined(__ILP32__)
+__attribute__((packed))
+#endif
+;

struct snd_ctl_tlv {
unsigned int numid; /* control element numeric identification */

[H.J. Lu]

Here is the updated patch for 3.9. I added snd_ctl_elem_value_long_long
to align long long to 4 bytes in snd_ctl_elem_value for x32. I also added
SNDRV_PCM_IOCTL_CHANNEL_INFO support for x32.

--
H.J.

[H. Peter Anvin]

On 06/11/2013 11:55 AM, H.J. Lu wrote:
> +#if defined(__x86_64__) && defined(__ILP32__)

> +typedef long long __attribute__((aligned(4))) snd_ctl_elem_value_long_long;
> +#else
> +typedef long long snd_ctl_elem_value_long_long;
> +#endif

I would really be happier making this a specific type, __compat_u64 or
something like that. I thought we had __u64 exported this way already,
for compatibility, but perhaps I'm smoking something?

-hpa

[Mike Frysinger]

nah, __u64 is just unsigned long long ... no alignment

__aligned_u64 is close, but that's aligned(8) instead of aligned(4)
-mike

[H.J. Lu]

On Thu, Jun 13, 2013 at 9:38 AM, H. Peter Anvin <h...@zytor.com> wrote:
> On 06/11/2013 11:55 AM, H.J. Lu wrote:
>> +#if defined(__x86_64__) && defined(__ILP32__)
>> +typedef long long __attribute__((aligned(4))) snd_ctl_elem_value_long_long;
>> +#else
>> +typedef long long snd_ctl_elem_value_long_long;
>> +#endif
>
> I would really be happier making this a specific type, __compat_u64 or

__compat_u64 is used in 64-bit kernel to provide COMPAT 64-bit
type. We need a 64-bit type for user space with natural
alignment in 64-bit and COMPAT alignment otherwise. How about
__compat_aligned_u64?

> something like that. I thought we had __u64 exported this way already,
> for compatibility, but perhaps I'm smoking something?
>

__64 has natural alignment.


--
H.J.

[H.J. Lu]

On Thu, Jun 13, 2013 at 11:04 AM, H. Peter Anvin <h...@zytor.com> wrote:

> On 06/13/2013 09:54 AM, H.J. Lu wrote:
>>
>> __compat_u64 is used in 64-bit kernel to provide COMPAT 64-bit
>> type. We need a 64-bit type for user space with natural
>> alignment in 64-bit and COMPAT alignment otherwise.
>>
>

> OK, we need to invent a type I guess.
>

About __aligned_compat_u64 or __compat_aligned_u64?


--
H.J.

[H. Peter Anvin]

Something like __compat_aligned_u64 or so, yes...

-hpa

[H.J. Lu]

Like this? I added __compat_aligned_s64/ __compat_aligned_u64
to int-ll64.h.


--
H.J.