[PATCH 1/5] util/qemu-openpty: fix build with musl libc by include termios.h as fallback

[Natanael Copa]

Include termios.h as POSIX fallback when not glibc, bsd or solaris.
POSIX says that termios.h should define struct termios and TCAFLUSH.
http://pubs.opengroup.org/onlinepubs/9699919799/basedefs/termios.h.html

This fixes the following compile errors with musl libc:

util/qemu-openpty.c: In function 'qemu_openpty_raw':
util/qemu-openpty.c:112:20: error: storage size of 'tty' isn't known
struct termios tty;
^
...
util/qemu-openpty.c:128:24: error: 'TCSAFLUSH' undeclared (first use in this function)
tcsetattr(*aslave, TCSAFLUSH, &tty);
^

Signed-off-by: Natanael Copa <***@alpinelinux.org>
---
util/qemu-openpty.c | 2 ++
1 file changed, 2 insertions(+)

diff --git a/util/qemu-openpty.c b/util/qemu-openpty.c
index 4febfe9..4c53211 100644
--- a/util/qemu-openpty.c
+++ b/util/qemu-openpty.c
@@ -47,6 +47,8 @@
#elif defined CONFIG_SOLARIS
# include <termios.h>
# include <stropts.h>
+#else
+# include <termios.h>
#endif

#ifdef __sun__

[Natanael Copa]

ffsl is a GNU extension and not available in musl libc.

See also commit fbeadf50 (bitops: unify bitops_ffsl with the one in
host-utils.h, call it bitops_ctzl) on why ctzl should be used instead
of ffsl.

Signed-off-by: Natanael Copa <***@alpinelinux.org>
---
xen-all.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/xen-all.c b/xen-all.c
index ba34739..3a0e9e5 100644
--- a/xen-all.c
+++ b/xen-all.c
@@ -511,7 +511,7 @@ static void xen_sync_dirty_bitmap(XenIOState *state,
for (i = 0; i < ARRAY_SIZE(bitmap); i++) {
unsigned long map = bitmap[i];
while (map != 0) {
- j = ffsl(map) - 1;
+ j = ctzl(map);
map &= ~(1ul << j);
memory_region_set_dirty(framebuffer,
(i * width + j) * TARGET_PAGE_SIZE,

[Natanael Copa]

See commit fbeadf50 (bitops: unify bitops_ffsl with the one in
host-utils.h, call it bitops_ctzl) on why ctzl should be used instead
of ffsl.

This is also needed for musl libc which does not implement ffsl.

Signed-off-by: Natanael Copa <***@alpinelinux.org>
---
include/exec/ram_addr.h | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/include/exec/ram_addr.h b/include/exec/ram_addr.h
index 2edfa96..b94de02 100644
--- a/include/exec/ram_addr.h
+++ b/include/exec/ram_addr.h
@@ -117,7 +117,7 @@ static inline void cpu_physical_memory_set_dirty_lebitmap(unsigned long *bitmap,
if (bitmap[i] != 0) {
c = leul_to_cpu(bitmap[i]);
do {
- j = ffsl(c) - 1;
+ j = ctzl(c);
c &= ~(1ul << j);
page_number = (i * HOST_LONG_BITS + j) * hpratio;
addr = page_number * TARGET_PAGE_SIZE;

[Natanael Copa]

Avoid using the GNU extesion ffsl which is not implemented in musl libc.

The atomic_xchg() means we know that vhost_log_chunk_t will never be
larger than the 'long' type, so ctzl() is always sufficient.

See also commit fbeadf50 (bitops: unify bitops_ffsl with the one in
host-utils.h, call it bitops_ctzl) on why ctzl should be used instead
of ffsl.

Signed-off-by: Natanael Copa <***@alpinelinux.org>
---
hw/virtio/vhost.c | 6 ++----
1 file changed, 2 insertions(+), 4 deletions(-)

diff --git a/hw/virtio/vhost.c b/hw/virtio/vhost.c
index 9e336ad..f62cfaf 100644
--- a/hw/virtio/vhost.c
+++ b/hw/virtio/vhost.c
@@ -41,7 +41,6 @@ static void vhost_dev_sync_region(struct vhost_dev *dev,

for (;from < to; ++from) {
vhost_log_chunk_t log;
- int bit;
/* We first check with non-atomic: much cheaper,
* and we expect non-dirty to be the common case. */
if (!*from) {
@@ -51,12 +50,11 @@ static void vhost_dev_sync_region(struct vhost_dev *dev,
/* Data must be read atomically. We don't really need barrier semantics
* but it's easier to use atomic_* than roll our own. */
log = atomic_xchg(from, 0);
- while ((bit = sizeof(log) > sizeof(int) ?
- ffsll(log) : ffs(log))) {
+ while (log) {
+ int bit = ctzl(log);
hwaddr page_addr;
hwaddr section_offset;
hwaddr mr_offset;
- bit -= 1;
page_addr = addr + bit * VHOST_LOG_PAGE;
section_offset = page_addr - section->offset_within_address_space;
mr_offset = section_offset + section->offset_within_region;

[Natanael Copa]

The __SIGRTMIN and __SIGRTMAX are glibc internals and are not available
on all platforms, so we define those if they are missing.

This is needed for musl libc.

Signed-off-by: Natanael Copa <***@alpinelinux.org>
---
linux-user/signal.c | 7 +++++++
1 file changed, 7 insertions(+)

diff --git a/linux-user/signal.c b/linux-user/signal.c
index 7d6246f..6019dbb 100644
--- a/linux-user/signal.c
+++ b/linux-user/signal.c
@@ -32,6 +32,13 @@

//#define DEBUG_SIGNAL

+#ifndef __SIGRTMIN
+#define __SIGRTMIN 32
+#endif
+#ifndef __SIGRTMAX
+#define __SIGRTMAX (NSIG-1)
+#endif
+
static struct target_sigaltstack target_sigaltstack_used = {
.ss_sp = 0,
.ss_size = 0,

[Eric Blake]

Post by Natanael Copa
The __SIGRTMIN and __SIGRTMAX are glibc internals and are not available
on all platforms, so we define those if they are missing.
This is needed for musl libc.
---
linux-user/signal.c | 7 +++++++
1 file changed, 7 insertions(+)
diff --git a/linux-user/signal.c b/linux-user/signal.c
index 7d6246f..6019dbb 100644
--- a/linux-user/signal.c
+++ b/linux-user/signal.c
@@ -32,6 +32,13 @@
//#define DEBUG_SIGNAL
+#ifndef __SIGRTMIN
+#define __SIGRTMIN 32

Rather than defining the implementation-specific __SIGRTMIN to a magic
number that is liable to be wrong, why not instead fix the code to use
the POSIX-mandated SIGRTMIN and SIGRTMAX public defines instead?

[Natanael Copa]

On Tue, 29 Apr 2014 08:28:29 -0600

Post by Eric Blake

Post by Natanael Copa
The __SIGRTMIN and __SIGRTMAX are glibc internals and are not available
on all platforms, so we define those if they are missing.
This is needed for musl libc.
---
linux-user/signal.c | 7 +++++++
1 file changed, 7 insertions(+)
diff --git a/linux-user/signal.c b/linux-user/signal.c
index 7d6246f..6019dbb 100644
--- a/linux-user/signal.c
+++ b/linux-user/signal.c
@@ -32,6 +32,13 @@
//#define DEBUG_SIGNAL
+#ifndef __SIGRTMIN
+#define __SIGRTMIN 32

Rather than defining the implementation-specific __SIGRTMIN to a magic
number that is liable to be wrong, why not instead fix the code to use
the POSIX-mandated SIGRTMIN and SIGRTMAX public defines instead?

Those seems to be runtime values:
/usr/include/signal.h:#define SIGRTMIN (__libc_current_sigrtmin())
/usr/include/signal.h:#define SIGRTMAX (__libc_current_sigrtmax())

so it gives:
/home/ncopa/src/qemu/linux-user/signal.c:93:5: error: nonconstant array index in initializer
[SIGRTMIN] = __SIGRTMAX,

I could have used (NSIG-1) but are not sure if NSIG is a runtime macro
in glibc. The array itself is using _NSIG instead of NSIG for some
reason.

-nc

[Eric Blake]

Post by Natanael Copa
On Tue, 29 Apr 2014 08:28:29 -0600

Post by Eric Blake

Post by Natanael Copa
The __SIGRTMIN and __SIGRTMAX are glibc internals and are not available
on all platforms, so we define those if they are missing.
+#define __SIGRTMIN 32

Rather than defining the implementation-specific __SIGRTMIN to a magic
number that is liable to be wrong, why not instead fix the code to use
the POSIX-mandated SIGRTMIN and SIGRTMAX public defines instead?

/usr/include/signal.h:#define SIGRTMIN (__libc_current_sigrtmin())

Oh right - POSIX allows them to be runtime variable. But we are
interacting with a given kernel, where the values will be fixed. Maybe
you have to define __SIGRTMIN after all, but can we at least have an
assert() that the value you picked matches SIGRTMIN at runtime?

Post by Natanael Copa
/usr/include/signal.h:#define SIGRTMAX (__libc_current_sigrtmax())
/home/ncopa/src/qemu/linux-user/signal.c:93:5: error: nonconstant array index in initializer
[SIGRTMIN] = __SIGRTMAX,
I could have used (NSIG-1) but are not sure if NSIG is a runtime macro
in glibc. The array itself is using _NSIG instead of NSIG for some
reason.

NSIG is not any more portable; nor does POSIX require that the RT
signals occur at the tail end of NSIG (in other words, NSIG-1 need not
be SIGRTMAX). Unless someone knows of a kernel define, it sounds like
we're stuck hard-coding in some knowledge of Linux.

[Natanael Copa]

On Tue, 29 Apr 2014 09:02:13 -0600

Post by Eric Blake

Post by Natanael Copa
On Tue, 29 Apr 2014 08:28:29 -0600

Post by Eric Blake

Post by Natanael Copa
The __SIGRTMIN and __SIGRTMAX are glibc internals and are not available
on all platforms, so we define those if they are missing.
+#define __SIGRTMIN 32

Rather than defining the implementation-specific __SIGRTMIN to a magic
number that is liable to be wrong, why not instead fix the code to use
the POSIX-mandated SIGRTMIN and SIGRTMAX public defines instead?

/usr/include/signal.h:#define SIGRTMIN (__libc_current_sigrtmin())

Oh right - POSIX allows them to be runtime variable. But we are
interacting with a given kernel, where the values will be fixed. Maybe
you have to define __SIGRTMIN after all, but can we at least have an
assert() that the value you picked matches SIGRTMIN at runtime?

Yeah, that might be an idea.

Post by Eric Blake

Post by Natanael Copa
/usr/include/signal.h:#define SIGRTMAX (__libc_current_sigrtmax())
/home/ncopa/src/qemu/linux-user/signal.c:93:5: error: nonconstant array index in initializer
[SIGRTMIN] = __SIGRTMAX,
I could have used (NSIG-1) but are not sure if NSIG is a runtime macro
in glibc. The array itself is using _NSIG instead of NSIG for some
reason.

NSIG is not any more portable; nor does POSIX require that the RT
signals occur at the tail end of NSIG (in other words, NSIG-1 need not
be SIGRTMAX). Unless someone knows of a kernel define, it sounds like
we're stuck hard-coding in some knowledge of Linux.

Since we already use _NSIG to define the size of the array, and we want
to use the last element of the array, maybe we should just use _NSIG-1?

-nc

[Riku Voipio]

Post by Natanael Copa
On Tue, 29 Apr 2014 09:02:13 -0600

Post by Eric Blake

Post by Natanael Copa
On Tue, 29 Apr 2014 08:28:29 -0600

Post by Eric Blake

Post by Natanael Copa
The __SIGRTMIN and __SIGRTMAX are glibc internals and are not available
on all platforms, so we define those if they are missing.
+#define __SIGRTMIN 32

Rather than defining the implementation-specific __SIGRTMIN to a magic
number that is liable to be wrong, why not instead fix the code to use
the POSIX-mandated SIGRTMIN and SIGRTMAX public defines instead?

/usr/include/signal.h:#define SIGRTMIN (__libc_current_sigrtmin())

Oh right - POSIX allows them to be runtime variable. But we are
interacting with a given kernel, where the values will be fixed. Maybe
you have to define __SIGRTMIN after all, but can we at least have an
assert() that the value you picked matches SIGRTMIN at runtime?

Yeah, that might be an idea.

If you can update the patch with asserts, I'd be fine with the patch.
I don't we want to go into changing to use NSIG-1 unless there is some
compelling advantage on it.

Post by Natanael Copa

Post by Eric Blake

Post by Natanael Copa
/usr/include/signal.h:#define SIGRTMAX (__libc_current_sigrtmax())
/home/ncopa/src/qemu/linux-user/signal.c:93:5: error: nonconstant array index in initializer
[SIGRTMIN] = __SIGRTMAX,
I could have used (NSIG-1) but are not sure if NSIG is a runtime macro
in glibc. The array itself is using _NSIG instead of NSIG for some
reason.

NSIG is not any more portable; nor does POSIX require that the RT
signals occur at the tail end of NSIG (in other words, NSIG-1 need not
be SIGRTMAX). Unless someone knows of a kernel define, it sounds like
we're stuck hard-coding in some knowledge of Linux.

Since we already use _NSIG to define the size of the array, and we want
to use the last element of the array, maybe we should just use _NSIG-1?
-nc

[Natanael Copa]

The __SIGRTMIN and __SIGRTMAX are glibc internals and are not available
on all platforms, so we define those if they are missing. We also check
that those corresponds with the posix variables SIGRTMIN/SIGRTMAX which
may only be available during runtime.

This is needed for musl libc.

Signed-off-by: Natanael Copa <***@alpinelinux.org>
---
Changes v1 -> v2:
- replace NSIG with _NSIG since thats use everywhere else in the code.
- add runtime asserts.

linux-user/signal.c | 14 ++++++++++++++
1 file changed, 14 insertions(+)

diff --git a/linux-user/signal.c b/linux-user/signal.c
index 5b8a01f..67771ad 100644
--- a/linux-user/signal.c
+++ b/linux-user/signal.c
@@ -32,6 +32,13 @@

//#define DEBUG_SIGNAL

+#ifndef __SIGRTMIN
+#define __SIGRTMIN 32
+#endif
+#ifndef __SIGRTMAX
+#define __SIGRTMAX (_NSIG-1)
+#endif
+
static struct target_sigaltstack target_sigaltstack_used = {
.ss_sp = 0,
.ss_size = 0,
@@ -379,6 +386,13 @@ void signal_init(void)
int i, j;
int host_sig;

+ /* SIGRTMIN/SIGRTMAX might be runtime variables so we cannot use them
+ to declare the host_to_target_signal table. But we are interacting
+ with a given kernel where the values will be fixed. Check that the
+ runtime values actually corresponds. */
+ assert(__SIGRTMIN == SIGRTMIN);
+ assert(__SIGRTMAX == SIGRTMAX);
+
/* generate signal conversion tables */
for(i = 1; i < _NSIG; i++) {
if (host_to_target_signal_table[i] == 0)

[Riku Voipio]

Hi,

Post by Natanael Copa
The __SIGRTMIN and __SIGRTMAX are glibc internals and are not available
on all platforms, so we define those if they are missing. We also check
that those corresponds with the posix variables SIGRTMIN/SIGRTMAX which
may only be available during runtime.
This is needed for musl libc.

After all, the idea of asserts doesn't work on glibc it seems:

qemu-arm qemu-smoke/armel/busybox ls -ld .
qemu-arm: linux-user/signal.c:393: signal_init: Assertion `32 == (__libc_current_sigrtmin ())' failed.
Aborted

Quick test on my amd64/glibc 2.18 system:

printf("RTMIN: %d RTMAX: %d\n", SIGRTMIN, SIGRTMAX);
RTMIN: 34 RTMAX: 64

While: /usr/include/bits/signum.h
#define __SIGRTMIN 32

Post by Natanael Copa
---
- replace NSIG with _NSIG since thats use everywhere else in the code.
- add runtime asserts.
linux-user/signal.c | 14 ++++++++++++++
1 file changed, 14 insertions(+)
diff --git a/linux-user/signal.c b/linux-user/signal.c
index 5b8a01f..67771ad 100644
--- a/linux-user/signal.c
+++ b/linux-user/signal.c
@@ -32,6 +32,13 @@
//#define DEBUG_SIGNAL
+#ifndef __SIGRTMIN
+#define __SIGRTMIN 32
+#endif
+#ifndef __SIGRTMAX
+#define __SIGRTMAX (_NSIG-1)
+#endif
+
static struct target_sigaltstack target_sigaltstack_used = {
.ss_sp = 0,
.ss_size = 0,
@@ -379,6 +386,13 @@ void signal_init(void)
int i, j;
int host_sig;
+ /* SIGRTMIN/SIGRTMAX might be runtime variables so we cannot use them
+ to declare the host_to_target_signal table. But we are interacting
+ with a given kernel where the values will be fixed. Check that the
+ runtime values actually corresponds. */
+ assert(__SIGRTMIN == SIGRTMIN);
+ assert(__SIGRTMAX == SIGRTMAX);
+
/* generate signal conversion tables */
for(i = 1; i < _NSIG; i++) {
if (host_to_target_signal_table[i] == 0)
--
2.0.0

[Paolo Bonzini]

Post by Riku Voipio
Hi,

Post by Natanael Copa
The __SIGRTMIN and __SIGRTMAX are glibc internals and are not available
on all platforms, so we define those if they are missing. We also check
that those corresponds with the posix variables SIGRTMIN/SIGRTMAX which
may only be available during runtime.
This is needed for musl libc.

qemu-arm qemu-smoke/armel/busybox ls -ld .
qemu-arm: linux-user/signal.c:393: signal_init: Assertion `32 == (__libc_current_sigrtmin ())' failed.
Aborted
printf("RTMIN: %d RTMAX: %d\n", SIGRTMIN, SIGRTMAX);
RTMIN: 34 RTMAX: 64
While: /usr/include/bits/signum.h
#define __SIGRTMIN 32

That's because glibc reserves two signals (one for cancellation, the
other to implement set*id system calls). Basically you'd need to extend
the hack of host_to_target_signal_table to all signals in the
[__SIGRTMIN, SIGRTMIN) range, computing the table at run-time.

Paolo

[Paolo Bonzini]

In addition to the previoiusly sent "linux-user: avoid using glibc
internals in _syscall5 and in definition of target_sigevent struct",
those are needed for making qemu build with musl libc on Alpine Linux.
There is still 2 missing fcntl.h definitions (F_EXLCK and F_SHLCK) but
those should probably be defined in libc so patch for that is not
included here.
util/qemu-openpty: fix build with musl libc by include termios.h as
fallback
xen: replace ffsl with ctzl
vhost: replace ffsl with ctzl
exec: replace ffsl with ctzl
linux-user/signal.c: define __SIGRTMIN/MAX for non-GNU platforms
hw/virtio/vhost.c | 6 ++----
include/exec/ram_addr.h | 2 +-
linux-user/signal.c | 7 +++++++
util/qemu-openpty.c | 2 ++
xen-all.c | 2 +-
5 files changed, 13 insertions(+), 6 deletions(-)

Reviewed-by: Paolo Bonzini <***@redhat.com>

[Peter Maydell]

In addition to the previoiusly sent "linux-user: avoid using glibc
internals in _syscall5 and in definition of target_sigevent struct",
those are needed for making qemu build with musl libc on Alpine Linux.
There is still 2 missing fcntl.h definitions (F_EXLCK and F_SHLCK) but
those should probably be defined in libc so patch for that is not
included here.
util/qemu-openpty: fix build with musl libc by include termios.h as
fallback
xen: replace ffsl with ctzl
vhost: replace ffsl with ctzl
exec: replace ffsl with ctzl
linux-user/signal.c: define __SIGRTMIN/MAX for non-GNU platforms

Hmm. I just noticed these ffsl patches didn't get committed (the BSDs
don't like ffsl either). There were issues with patch 5( the signal.c one)
but 1..4 are good (and get my Reviewed-by: as well as Paolo's).

I'm currently building up a tree with various bsd-user fixes in it,
so I'll take these through that, I think. (Patch 1 is arguably slightly
out of scope for that but better to keep it together with 2..4.)

Natanael: you might want to update the signal.c patch to accommodate
review comments and resubmit it.

thanks
-- PMM

[Natanael Copa]

On Wed, 4 Jun 2014 00:37:44 +0100

Post by Peter Maydell

In addition to the previoiusly sent "linux-user: avoid using glibc
internals in _syscall5 and in definition of target_sigevent struct",
those are needed for making qemu build with musl libc on Alpine Linux.
There is still 2 missing fcntl.h definitions (F_EXLCK and F_SHLCK) but
those should probably be defined in libc so patch for that is not
included here.
util/qemu-openpty: fix build with musl libc by include termios.h as
fallback
xen: replace ffsl with ctzl
vhost: replace ffsl with ctzl
exec: replace ffsl with ctzl
linux-user/signal.c: define __SIGRTMIN/MAX for non-GNU platforms

Hmm. I just noticed these ffsl patches didn't get committed (the BSDs
don't like ffsl either). There were issues with patch 5( the signal.c one)
but 1..4 are good (and get my Reviewed-by: as well as Paolo's).
I'm currently building up a tree with various bsd-user fixes in it,
so I'll take these through that, I think. (Patch 1 is arguably slightly
out of scope for that but better to keep it together with 2..4.)
Natanael: you might want to update the signal.c patch to accommodate
review comments and resubmit it.
thanks

I will try get it done today. Thanks!

-nc

Post by Peter Maydell
-- PMM