The CONFIG_LINUX check at the top of mmap-alloc.c never worked
because it was done before including osdep.h.
This means MAP_SYNC and MAP_SHARED_VALIDATE would always be set
to 0 at the beginning of the file. Luckily, this didn't break
when using recent glibc versions (2.28+), because those macros
were redefined by glibc headers.
Move the CONFIG_LINUX check after the main include lines, so the
CONFIG_LINUX check works and we actually include <linux/mman.h>.
This will make MAP_SYNC and MAP_SHARED_VALIDATE available even if
the host has an older glibc version.
Reported-by: Jingqi Liu <jingqi.liu@intel.com>
Reviewed-by: Michael S. Tsirkin <mst@redhat.com>
Signed-off-by: Eduardo Habkost <ehabkost@redhat.com>
---
Changes v1 -> v2:
* (none)
---
util/mmap-alloc.c | 7 +++----
1 file changed, 3 insertions(+), 4 deletions(-)
diff --git a/util/mmap-alloc.c b/util/mmap-alloc.c
index 27dcccd8ec..7c2ce98eb0 100644
--- a/util/mmap-alloc.c
+++ b/util/mmap-alloc.c
@@ -9,6 +9,9 @@
* This work is licensed under the terms of the GNU GPL, version 2 or
* later. See the COPYING file in the top-level directory.
*/
+#include "qemu/osdep.h"
+#include "qemu/mmap-alloc.h"
+#include "qemu/host-utils.h"
#ifdef CONFIG_LINUX
#include <linux/mman.h>
@@ -17,10 +20,6 @@
#define MAP_SHARED_VALIDATE 0x0
#endif /* CONFIG_LINUX */
-#include "qemu/osdep.h"
-#include "qemu/mmap-alloc.h"
-#include "qemu/host-utils.h"
-
#define HUGETLBFS_MAGIC 0x958458f6
#ifdef CONFIG_LINUX
--
2.24.1
On Wed, Mar 11, 2020 at 07:23:42PM -0400, Eduardo Habkost wrote: > The CONFIG_LINUX check at the top of mmap-alloc.c never worked > because it was done before including osdep.h. > > This means MAP_SYNC and MAP_SHARED_VALIDATE would always be set > to 0 at the beginning of the file. Luckily, this didn't break > when using recent glibc versions (2.28+), because those macros > were redefined by glibc headers. > > Move the CONFIG_LINUX check after the main include lines, so the > CONFIG_LINUX check works and we actually include <linux/mman.h>. > This will make MAP_SYNC and MAP_SHARED_VALIDATE available even if > the host has an older glibc version. > > Reported-by: Jingqi Liu <jingqi.liu@intel.com> > Reviewed-by: Michael S. Tsirkin <mst@redhat.com> > Signed-off-by: Eduardo Habkost <ehabkost@redhat.com> Reviewed-by: Michael S. Tsirkin <mst@redhat.com> Cc: qemu-stable@nongnu.org > --- > Changes v1 -> v2: > * (none) > --- > util/mmap-alloc.c | 7 +++---- > 1 file changed, 3 insertions(+), 4 deletions(-) > > diff --git a/util/mmap-alloc.c b/util/mmap-alloc.c > index 27dcccd8ec..7c2ce98eb0 100644 > --- a/util/mmap-alloc.c > +++ b/util/mmap-alloc.c > @@ -9,6 +9,9 @@ > * This work is licensed under the terms of the GNU GPL, version 2 or > * later. See the COPYING file in the top-level directory. > */ > +#include "qemu/osdep.h" > +#include "qemu/mmap-alloc.h" > +#include "qemu/host-utils.h" > > #ifdef CONFIG_LINUX > #include <linux/mman.h> > @@ -17,10 +20,6 @@ > #define MAP_SHARED_VALIDATE 0x0 > #endif /* CONFIG_LINUX */ > > -#include "qemu/osdep.h" > -#include "qemu/mmap-alloc.h" > -#include "qemu/host-utils.h" > - > #define HUGETLBFS_MAGIC 0x958458f6 > > #ifdef CONFIG_LINUX > -- > 2.24.1
On Wed, Mar 11, 2020 at 07:23:42PM -0400, Eduardo Habkost wrote: > The CONFIG_LINUX check at the top of mmap-alloc.c never worked > because it was done before including osdep.h. > > This means MAP_SYNC and MAP_SHARED_VALIDATE would always be set > to 0 at the beginning of the file. Luckily, this didn't break > when using recent glibc versions (2.28+), because those macros > were redefined by glibc headers. > > Move the CONFIG_LINUX check after the main include lines, so the > CONFIG_LINUX check works and we actually include <linux/mman.h>. > This will make MAP_SYNC and MAP_SHARED_VALIDATE available even if > the host has an older glibc version. > > Reported-by: Jingqi Liu <jingqi.liu@intel.com> > Reviewed-by: Michael S. Tsirkin <mst@redhat.com> > Signed-off-by: Eduardo Habkost <ehabkost@redhat.com> > --- > Changes v1 -> v2: > * (none) > --- > util/mmap-alloc.c | 7 +++---- > 1 file changed, 3 insertions(+), 4 deletions(-) > > diff --git a/util/mmap-alloc.c b/util/mmap-alloc.c > index 27dcccd8ec..7c2ce98eb0 100644 > --- a/util/mmap-alloc.c > +++ b/util/mmap-alloc.c > @@ -9,6 +9,9 @@ > * This work is licensed under the terms of the GNU GPL, version 2 or > * later. See the COPYING file in the top-level directory. > */ > +#include "qemu/osdep.h" > +#include "qemu/mmap-alloc.h" > +#include "qemu/host-utils.h" > > #ifdef CONFIG_LINUX > #include <linux/mman.h> This breaks the build on mips, because mips doesn't have MAP_SYNC defined at linux/mman.h: https://app.shippable.com/github/ehabkost/qemu-hacks/runs/9/9/console > @@ -17,10 +20,6 @@ > #define MAP_SHARED_VALIDATE 0x0 > #endif /* CONFIG_LINUX */ > > -#include "qemu/osdep.h" > -#include "qemu/mmap-alloc.h" > -#include "qemu/host-utils.h" > - > #define HUGETLBFS_MAGIC 0x958458f6 > > #ifdef CONFIG_LINUX > -- > 2.24.1 > -- Eduardo
On Sun, Mar 15, 2020 at 11:45:59AM -0400, Eduardo Habkost wrote: > On Wed, Mar 11, 2020 at 07:23:42PM -0400, Eduardo Habkost wrote: > > The CONFIG_LINUX check at the top of mmap-alloc.c never worked > > because it was done before including osdep.h. > > > > This means MAP_SYNC and MAP_SHARED_VALIDATE would always be set > > to 0 at the beginning of the file. Luckily, this didn't break > > when using recent glibc versions (2.28+), because those macros > > were redefined by glibc headers. > > > > Move the CONFIG_LINUX check after the main include lines, so the > > CONFIG_LINUX check works and we actually include <linux/mman.h>. > > This will make MAP_SYNC and MAP_SHARED_VALIDATE available even if > > the host has an older glibc version. Wait a second, MAP_SHARED_VALIDATE is from linux-headers/linux/mman.h - it's available on all architectures. > > > > Reported-by: Jingqi Liu <jingqi.liu@intel.com> > > Reviewed-by: Michael S. Tsirkin <mst@redhat.com> > > Signed-off-by: Eduardo Habkost <ehabkost@redhat.com> > > --- > > Changes v1 -> v2: > > * (none) > > --- > > util/mmap-alloc.c | 7 +++---- > > 1 file changed, 3 insertions(+), 4 deletions(-) > > > > diff --git a/util/mmap-alloc.c b/util/mmap-alloc.c > > index 27dcccd8ec..7c2ce98eb0 100644 > > --- a/util/mmap-alloc.c > > +++ b/util/mmap-alloc.c > > @@ -9,6 +9,9 @@ > > * This work is licensed under the terms of the GNU GPL, version 2 or > > * later. See the COPYING file in the top-level directory. > > */ > > +#include "qemu/osdep.h" > > +#include "qemu/mmap-alloc.h" > > +#include "qemu/host-utils.h" > > > > #ifdef CONFIG_LINUX > > #include <linux/mman.h> > > This breaks the build on mips, because mips doesn't have MAP_SYNC > defined at linux/mman.h: > > https://app.shippable.com/github/ehabkost/qemu-hacks/runs/9/9/console Oops. But that in fact means it's currently building on mips but not working correctly there! MAP_SHARED_VALIDATE 0x0 is especially problematic. I'm unsure what's the right thing to do is, I guess as a first step we can go back and device MAP_SYNC to 0, > > > @@ -17,10 +20,6 @@ > > #define MAP_SHARED_VALIDATE 0x0 > > #endif /* CONFIG_LINUX */ > > > > -#include "qemu/osdep.h" > > -#include "qemu/mmap-alloc.h" > > -#include "qemu/host-utils.h" > > - > > #define HUGETLBFS_MAGIC 0x958458f6 > > > > #ifdef CONFIG_LINUX > > -- > > 2.24.1 > > > > -- > Eduardo
On Sun, Mar 15, 2020 at 05:15:46PM -0400, Michael S. Tsirkin wrote: > On Sun, Mar 15, 2020 at 11:45:59AM -0400, Eduardo Habkost wrote: > > On Wed, Mar 11, 2020 at 07:23:42PM -0400, Eduardo Habkost wrote: > > > The CONFIG_LINUX check at the top of mmap-alloc.c never worked > > > because it was done before including osdep.h. > > > > > > This means MAP_SYNC and MAP_SHARED_VALIDATE would always be set > > > to 0 at the beginning of the file. Luckily, this didn't break > > > when using recent glibc versions (2.28+), because those macros > > > were redefined by glibc headers. > > > > > > Move the CONFIG_LINUX check after the main include lines, so the > > > CONFIG_LINUX check works and we actually include <linux/mman.h>. > > > This will make MAP_SYNC and MAP_SHARED_VALIDATE available even if > > > the host has an older glibc version. > > Wait a second, MAP_SHARED_VALIDATE is from > linux-headers/linux/mman.h - it's available on all architectures. Yes, but both MAP_SYNC and MAP_SHARED_VALIDATE aren't available if the host is not Linux. > > > > > > > Reported-by: Jingqi Liu <jingqi.liu@intel.com> > > > Reviewed-by: Michael S. Tsirkin <mst@redhat.com> > > > Signed-off-by: Eduardo Habkost <ehabkost@redhat.com> > > > --- > > > Changes v1 -> v2: > > > * (none) > > > --- > > > util/mmap-alloc.c | 7 +++---- > > > 1 file changed, 3 insertions(+), 4 deletions(-) > > > > > > diff --git a/util/mmap-alloc.c b/util/mmap-alloc.c > > > index 27dcccd8ec..7c2ce98eb0 100644 > > > --- a/util/mmap-alloc.c > > > +++ b/util/mmap-alloc.c > > > @@ -9,6 +9,9 @@ > > > * This work is licensed under the terms of the GNU GPL, version 2 or > > > * later. See the COPYING file in the top-level directory. > > > */ > > > +#include "qemu/osdep.h" > > > +#include "qemu/mmap-alloc.h" > > > +#include "qemu/host-utils.h" > > > > > > #ifdef CONFIG_LINUX > > > #include <linux/mman.h> > > > > This breaks the build on mips, because mips doesn't have MAP_SYNC > > defined at linux/mman.h: > > > > https://app.shippable.com/github/ehabkost/qemu-hacks/runs/9/9/console > > > Oops. But that in fact means it's currently building on mips but not > working correctly there! MAP_SHARED_VALIDATE 0x0 is especially > problematic. I'm unsure what's the right thing to do is, > I guess as a first step we can go back and device MAP_SYNC to 0, Defining MAP_SYNC to 0 on MIPS would restore the existing behavior, so it seems like a reasonable step to fix the build failure. But not even printing a warning when the host doesn't have MAP_SYNC (the existing behavior on MIPS and non-Linux) seems wrong. -- Eduardo
On Mon, 16 Mar 2020 at 17:51, Eduardo Habkost <ehabkost@redhat.com> wrote: > Yes, but both MAP_SYNC and MAP_SHARED_VALIDATE aren't available > if the host is not Linux. > > Defining MAP_SYNC to 0 on MIPS would restore the existing > behavior, so it seems like a reasonable step to fix the build > failure. But not even printing a warning when the host doesn't > have MAP_SYNC (the existing behavior on MIPS and non-Linux) seems > wrong. The usual approach is that if you don't have the Linux-specific feature available you quietly fall back to whatever the sensible behaviour is for when the feature isn't present. We definitely don't want to be printing warnings on non-Linux systems that are effectively just saying "you're not running Linux". Same goes for "host happens not to be running a bleeding-edge Linux kernel and this feature isn't available yet". thanks -- PMM
On Mon, Mar 16, 2020 at 06:08:54PM +0000, Peter Maydell wrote: > On Mon, 16 Mar 2020 at 17:51, Eduardo Habkost <ehabkost@redhat.com> wrote: > > Yes, but both MAP_SYNC and MAP_SHARED_VALIDATE aren't available > > if the host is not Linux. > > > > Defining MAP_SYNC to 0 on MIPS would restore the existing > > behavior, so it seems like a reasonable step to fix the build > > failure. But not even printing a warning when the host doesn't > > have MAP_SYNC (the existing behavior on MIPS and non-Linux) seems > > wrong. > > The usual approach is that if you don't have the Linux-specific > feature available you quietly fall back to whatever the sensible > behaviour is for when the feature isn't present. We definitely > don't want to be printing warnings on non-Linux systems that > are effectively just saying "you're not running Linux". Same goes > for "host happens not to be running a bleeding-edge Linux kernel > and this feature isn't available yet". I don't think using pmem=on without MAP_SYNC is expected to be a supported use case, is it? If a use case is not supported, the sensible behavior is to tell the user it is not supported. -- Eduardo
On Mon, 16 Mar 2020 at 18:40, Eduardo Habkost <ehabkost@redhat.com> wrote: > > On Mon, Mar 16, 2020 at 06:08:54PM +0000, Peter Maydell wrote: > > The usual approach is that if you don't have the Linux-specific > > feature available you quietly fall back to whatever the sensible > > behaviour is for when the feature isn't present. We definitely > > don't want to be printing warnings on non-Linux systems that > > are effectively just saying "you're not running Linux". Same goes > > for "host happens not to be running a bleeding-edge Linux kernel > > and this feature isn't available yet". > > I don't think using pmem=on without MAP_SYNC is expected to be a > supported use case, is it? If a use case is not supported, the > sensible behavior is to tell the user it is not supported. Yeah, that's fair. But the code at the moment does a fallback to "proceed without SHARED_VALIDATE | SYNC", so I assumed it was supposed to work. thanks -- PMM
On Mon, Mar 16, 2020 at 07:20:02PM +0000, Peter Maydell wrote: > On Mon, 16 Mar 2020 at 18:40, Eduardo Habkost <ehabkost@redhat.com> wrote: > > > > On Mon, Mar 16, 2020 at 06:08:54PM +0000, Peter Maydell wrote: > > > The usual approach is that if you don't have the Linux-specific > > > feature available you quietly fall back to whatever the sensible > > > behaviour is for when the feature isn't present. We definitely > > > don't want to be printing warnings on non-Linux systems that > > > are effectively just saying "you're not running Linux". Same goes > > > for "host happens not to be running a bleeding-edge Linux kernel > > > and this feature isn't available yet". > > > > I don't think using pmem=on without MAP_SYNC is expected to be a > > supported use case, is it? If a use case is not supported, the > > sensible behavior is to tell the user it is not supported. > > Yeah, that's fair. But the code at the moment does a fallback > to "proceed without SHARED_VALIDATE | SYNC", so I assumed it > was supposed to work. > > thanks > -- PMM Oh I remember now. pmem=on was introduced without MAP_SYNC first. So yes, it's ok to set it to 0 for mips. -- MST
On Mon, Mar 16, 2020 at 02:40:46PM -0400, Eduardo Habkost wrote: > On Mon, Mar 16, 2020 at 06:08:54PM +0000, Peter Maydell wrote: > > On Mon, 16 Mar 2020 at 17:51, Eduardo Habkost <ehabkost@redhat.com> wrote: > > > Yes, but both MAP_SYNC and MAP_SHARED_VALIDATE aren't available > > > if the host is not Linux. > > > > > > Defining MAP_SYNC to 0 on MIPS would restore the existing > > > behavior, so it seems like a reasonable step to fix the build > > > failure. But not even printing a warning when the host doesn't > > > have MAP_SYNC (the existing behavior on MIPS and non-Linux) seems > > > wrong. > > > > The usual approach is that if you don't have the Linux-specific > > feature available you quietly fall back to whatever the sensible > > behaviour is for when the feature isn't present. We definitely > > don't want to be printing warnings on non-Linux systems that > > are effectively just saying "you're not running Linux". Same goes > > for "host happens not to be running a bleeding-edge Linux kernel > > and this feature isn't available yet". > > I don't think using pmem=on without MAP_SYNC is expected to be a > supported use case, is it? If a use case is not supported, the > sensible behavior is to tell the user it is not supported. Futher when eventually an arch does gain the feature, not failing when it's not available would mean users have no way to detect what's actually going on. > -- > Eduardo
© 2016 - 2025 Red Hat, Inc.