Wrong unpacked structure for epoll_event on qemu-or1k (openrisc user-space)

Posted by Luca Bonissi 9 months, 2 weeks ago

The or1k epoll_event structure - unlike other architectures - is packed, 
so we need to define it as packed in qemu-user, otherwise it leads to 
infinite loop due to missing file descriptor in the returned data:

--- qemu-20230327/linux-user/syscall_defs.h.orig	2023-03-27 
15:41:42.000000000 +0200
+++ qemu-20230327/linux-user/syscall_defs.h	2023-06-30 
17:29:39.034322213 +0200
@@ -2714,7 +2709,7 @@
  #define FUTEX_CMD_MASK          ~(FUTEX_PRIVATE_FLAG | 
FUTEX_CLOCK_REALTIME)

  #ifdef CONFIG_EPOLL
-#if defined(TARGET_X86_64)
+#if defined(TARGET_X86_64) || defined(TARGET_OPENRISC)
  #define TARGET_EPOLL_PACKED QEMU_PACKED
  #else
  #define TARGET_EPOLL_PACKED

Re: Wrong unpacked structure for epoll_event on qemu-or1k (openrisc user-space)

Posted by Peter Maydell 9 months, 2 weeks ago

On Tue, 18 Jul 2023 at 14:03, Luca Bonissi <qemu@bonslack.org> wrote:
>
> The or1k epoll_event structure - unlike other architectures - is packed,
> so we need to define it as packed in qemu-user, otherwise it leads to
> infinite loop due to missing file descriptor in the returned data:

Hi; thanks for this patch. Unfortunately we need patches
to include a Signed-off-by: line that says you're legally
OK with it being contributed to QEMU, or we can't take them.
More info here:

https://www.qemu.org/docs/master/devel/submitting-a-patch.html

thanks
-- PMM

Re: [PATCH] Wrong unpacked structure for epoll_event on qemu-or1k (openrisc user-space)

Posted by Luca Bonissi 9 months, 2 weeks ago

On 18/07/23 16:40, Peter Maydell wrote:
> Hi; thanks for this patch. Unfortunately we need patches
> to include a Signed-off-by: line that says you're legally
> OK with it being contributed to QEMU, or we can't take them.

Sorry for the missing "signed-off-by" line, adding it just now:

==============
The or1k epoll_event structure - unlike other architectures - is packed, 
so we need to define it as packed in qemu-user, otherwise it leads to 
infinite loop due to missing file descriptor in the returned data:


Signed-off-by: Luca Bonissi <qemu@bonslack.org>
---

diff -up a/linux-user/syscall_defs.h b/linux-user/syscall_defs.h
--- a/linux-user/syscall_defs.h    2023-03-27 15:41:42.000000000 +0200
+++ b/linux-user/syscall_defs.h    2023-06-30 17:29:39.034322213 +0200
@@ -2714,7 +2709,7 @@
  #define FUTEX_CMD_MASK          ~(FUTEX_PRIVATE_FLAG | 
FUTEX_CLOCK_REALTIME)

  #ifdef CONFIG_EPOLL
-#if defined(TARGET_X86_64)
+#if defined(TARGET_X86_64) || defined(TARGET_OPENRISC)
  #define TARGET_EPOLL_PACKED QEMU_PACKED
  #else
  #define TARGET_EPOLL_PACKED

Re: [PATCH] Wrong unpacked structure for epoll_event on qemu-or1k (openrisc user-space)

Posted by Laurent Vivier 9 months, 2 weeks ago

Le 18/07/2023 à 17:06, Luca Bonissi a écrit :
> On 18/07/23 16:40, Peter Maydell wrote:
>> Hi; thanks for this patch. Unfortunately we need patches
>> to include a Signed-off-by: line that says you're legally
>> OK with it being contributed to QEMU, or we can't take them.
> 
> Sorry for the missing "signed-off-by" line, adding it just now:
> 
> ==============
> The or1k epoll_event structure - unlike other architectures - is packed, so we need to define it as 
> packed in qemu-user, otherwise it leads to infinite loop due to missing file descriptor in the 
> returned data:
> 
> 
> Signed-off-by: Luca Bonissi <qemu@bonslack.org>
> ---
> 
> diff -up a/linux-user/syscall_defs.h b/linux-user/syscall_defs.h
> --- a/linux-user/syscall_defs.h    2023-03-27 15:41:42.000000000 +0200
> +++ b/linux-user/syscall_defs.h    2023-06-30 17:29:39.034322213 +0200
> @@ -2714,7 +2709,7 @@
>   #define FUTEX_CMD_MASK          ~(FUTEX_PRIVATE_FLAG | FUTEX_CLOCK_REALTIME)
> 
>   #ifdef CONFIG_EPOLL
> -#if defined(TARGET_X86_64)
> +#if defined(TARGET_X86_64) || defined(TARGET_OPENRISC)
>   #define TARGET_EPOLL_PACKED QEMU_PACKED
>   #else
>   #define TARGET_EPOLL_PACKED

According to linux/glibc sourced, epoll is only packed for x86_64.

Did you try to check the alignment of the structure with gdb of a C program using offsetof() in an 
openrisc VM or linux-user container?

Perhaps the default alignment of long is not correctly defined in qemu for openrisc?

You can check with:

int main(void)
{
         printf("alignof(short) %ld\n", __alignof__(short));
         printf("alignof(int) %ld\n", __alignof__(int));
         printf("alignof(long) %ld\n", __alignof__(long));
         printf("alignof(long long) %ld\n", __alignof__(long long));
}

See include/exec/user/abitypes.h to update the value.

Thanks,
Laurent

Re: [PATCH] Wrong unpacked structure for epoll_event on qemu-or1k (openrisc user-space)

Posted by Luca Bonissi 9 months, 2 weeks ago

On 19/07/23 10:49, Laurent Vivier wrote:
> 
> According to linux/glibc sourced, epoll is only packed for x86_64.

And, in recent glibc, also for i386, even it seems not necessary: even 
if the __alignof__(long long) is 8, structures like epoll_event are only 
12 bytes, maybe "packed" for historical reasons. Ancient i386 gcc[s] 
(<3.0.0) have 4 bytes for __alignof__(long long).

> Perhaps the default alignment of long is not correctly defined in qemu 
> for openrisc?

__alignof__(long long):
- 8 bytes: all 64 bit targets + arm, hppa, mips, ppc, sparc, xtensa, x86
- 4 bytes: microblaze, nios2, or1k, sh4
- 2 bytes: m68k
- 1 byte : cris

offsetof(struct epoll_event,data):
- 8: all 64 bit targets + arm, hppa, mips, ppc, sparc, xtensa
- 4: cris, m68k, microblaze, nios2, or1k, sh4, x86

So, epoll_event is "naturally" packed on the following targets (checked 
in linux-user container and/or with cross-compiler):
- cris, m68k, microblaze, nios2, or1k, sh4, x86 (32bit)

> See include/exec/user/abitypes.h to update the value.

OK, abitypes.h should be updated with the following patch (discard the 
previous patch on syscall_defs.h):

Signed-off-by: Luca Bonissi <qemu@bonslack.org>
---

diff -up a/include/exec/user/abitypes.h b/include/exec/user/abitypes.h
--- a/include/exec/user/abitypes.h	2023-03-27 15:41:42.511916232 +0200
+++ b/include/exec/user/abitypes.h	2023-07-19 12:09:03.001687788 +0200
@@ -15,7 +15,15 @@
  #define ABI_LLONG_ALIGNMENT 2
  #endif

+#ifdef TARGET_CRIS
+#define ABI_SHORT_ALIGNMENT 1
+#define ABI_INT_ALIGNMENT 1
+#define ABI_LONG_ALIGNMENT 1
+#define ABI_LLONG_ALIGNMENT 1
+#endif
+
-#if (defined(TARGET_I386) && !defined(TARGET_X86_64)) || 
defined(TARGET_SH4)
+#if (defined(TARGET_I386) && !defined(TARGET_X86_64)) || 
defined(TARGET_SH4) || \
+    defined(TARGET_OPENRISC) || defined(TARGET_NIOS2) || 
defined(TARGET_MICROBLAZE)
  #define ABI_LLONG_ALIGNMENT 4
  #endif

Re: [PATCH] Wrong unpacked structure for epoll_event on qemu-or1k (openrisc user-space)

Posted by Thomas Huth 9 months ago

On 19/07/2023 14.38, Luca Bonissi wrote:
> On 19/07/23 10:49, Laurent Vivier wrote:
>>
>> According to linux/glibc sourced, epoll is only packed for x86_64.
> 
> And, in recent glibc, also for i386, even it seems not necessary: even if 
> the __alignof__(long long) is 8, structures like epoll_event are only 12 
> bytes, maybe "packed" for historical reasons. Ancient i386 gcc[s] (<3.0.0) 
> have 4 bytes for __alignof__(long long).
> 
>> Perhaps the default alignment of long is not correctly defined in qemu for 
>> openrisc?
> 
> __alignof__(long long):
> - 8 bytes: all 64 bit targets + arm, hppa, mips, ppc, sparc, xtensa, x86
> - 4 bytes: microblaze, nios2, or1k, sh4
> - 2 bytes: m68k
> - 1 byte : cris
> 
> offsetof(struct epoll_event,data):
> - 8: all 64 bit targets + arm, hppa, mips, ppc, sparc, xtensa
> - 4: cris, m68k, microblaze, nios2, or1k, sh4, x86
> 
> So, epoll_event is "naturally" packed on the following targets (checked in 
> linux-user container and/or with cross-compiler):
> - cris, m68k, microblaze, nios2, or1k, sh4, x86 (32bit)
> 
>> See include/exec/user/abitypes.h to update the value.
> 
> OK, abitypes.h should be updated with the following patch (discard the 
> previous patch on syscall_defs.h):
> 
> Signed-off-by: Luca Bonissi <qemu@bonslack.org>
> ---
> 
> diff -up a/include/exec/user/abitypes.h b/include/exec/user/abitypes.h
> --- a/include/exec/user/abitypes.h    2023-03-27 15:41:42.511916232 +0200
> +++ b/include/exec/user/abitypes.h    2023-07-19 12:09:03.001687788 +0200
> @@ -15,7 +15,15 @@
>   #define ABI_LLONG_ALIGNMENT 2
>   #endif
> 
> +#ifdef TARGET_CRIS
> +#define ABI_SHORT_ALIGNMENT 1
> +#define ABI_INT_ALIGNMENT 1
> +#define ABI_LONG_ALIGNMENT 1
> +#define ABI_LLONG_ALIGNMENT 1
> +#endif
> +
> -#if (defined(TARGET_I386) && !defined(TARGET_X86_64)) || defined(TARGET_SH4)
> +#if (defined(TARGET_I386) && !defined(TARGET_X86_64)) || 
> defined(TARGET_SH4) || \
> +    defined(TARGET_OPENRISC) || defined(TARGET_NIOS2) || 
> defined(TARGET_MICROBLAZE)
>   #define ABI_LLONG_ALIGNMENT 4
>   #endif

Hi! Thanks for the patch - but could you please send this as a new patch 
mail with a proper subject and patch description, so that it could be 
applied with "git am" ? Thanks!

  Thomas

Re: [PATCH] Wrong unpacked structure for epoll_event on qemu-or1k (openrisc user-space)

Posted by Richard Henderson 9 months ago

On 8/2/23 12:55, Thomas Huth wrote:
> On 19/07/2023 14.38, Luca Bonissi wrote:
>> On 19/07/23 10:49, Laurent Vivier wrote:
>>>
>>> According to linux/glibc sourced, epoll is only packed for x86_64.
>>
>> And, in recent glibc, also for i386, even it seems not necessary: even if the 
>> __alignof__(long long) is 8, structures like epoll_event are only 12 bytes, maybe 
>> "packed" for historical reasons. Ancient i386 gcc[s] (<3.0.0) have 4 bytes for 
>> __alignof__(long long).
>>
>>> Perhaps the default alignment of long is not correctly defined in qemu for openrisc?
>>
>> __alignof__(long long):
>> - 8 bytes: all 64 bit targets + arm, hppa, mips, ppc, sparc, xtensa, x86
>> - 4 bytes: microblaze, nios2, or1k, sh4
>> - 2 bytes: m68k
>> - 1 byte : cris
>>
>> offsetof(struct epoll_event,data):
>> - 8: all 64 bit targets + arm, hppa, mips, ppc, sparc, xtensa
>> - 4: cris, m68k, microblaze, nios2, or1k, sh4, x86
>>
>> So, epoll_event is "naturally" packed on the following targets (checked in linux-user 
>> container and/or with cross-compiler):
>> - cris, m68k, microblaze, nios2, or1k, sh4, x86 (32bit)
>>
>>> See include/exec/user/abitypes.h to update the value.
>>
>> OK, abitypes.h should be updated with the following patch (discard the previous patch on 
>> syscall_defs.h):
>>
>> Signed-off-by: Luca Bonissi <qemu@bonslack.org>
>> ---
>>
>> diff -up a/include/exec/user/abitypes.h b/include/exec/user/abitypes.h
>> --- a/include/exec/user/abitypes.h    2023-03-27 15:41:42.511916232 +0200
>> +++ b/include/exec/user/abitypes.h    2023-07-19 12:09:03.001687788 +0200
>> @@ -15,7 +15,15 @@
>>   #define ABI_LLONG_ALIGNMENT 2
>>   #endif
>>
>> +#ifdef TARGET_CRIS
>> +#define ABI_SHORT_ALIGNMENT 1
>> +#define ABI_INT_ALIGNMENT 1
>> +#define ABI_LONG_ALIGNMENT 1
>> +#define ABI_LLONG_ALIGNMENT 1
>> +#endif
>> +
>> -#if (defined(TARGET_I386) && !defined(TARGET_X86_64)) || defined(TARGET_SH4)
>> +#if (defined(TARGET_I386) && !defined(TARGET_X86_64)) || defined(TARGET_SH4) || \
>> +    defined(TARGET_OPENRISC) || defined(TARGET_NIOS2) || defined(TARGET_MICROBLAZE)
>>   #define ABI_LLONG_ALIGNMENT 4
>>   #endif
> 
> Hi! Thanks for the patch - but could you please send this as a new patch mail with a 
> proper subject and patch description, so that it could be applied with "git am" ? Thanks!

The patch should be against master, because microblaze and nios2 are already fixed.


r~