[PATCH 0/9] Cleanup of qemu_oom_check() and qemu_memalign()

[PATCH 0/9] Cleanup of qemu_oom_check() and qemu_memalign()

[Peter Maydell]

This series does some cleanup of the qemu_oom_check() and
qemu_memalign() functions; I started looking at the first of these and
found myself wanting to tidy some stuff relating to the second in the
process. The TLDR is that this series removes qemu_oom_check() (which
was mostly being misused), unifies the POSIX and Win32 versions of
qemu_memalign() and qemu_try_memalign(), and moves the prototypes out
of osdep.h.

qemu_oom_check() is a function which essentially says "if you pass me
a NULL pointer then print a message and abort()".  On POSIX systems
the message includes strerror(errno); on Windows it includes the
GetLastError() error value printed as an integer.  Other than in the
implementation of qemu_memalign(), we use this function only in
hw/usb/redirect.c, for three checks:
 * on a call to usbredirparser_create()
 * on a call to usberedirparser_serialize()
 * on a call to malloc()

The usbredir library API functions make no guarantees that they will
set errno on errors, let alone that they might set the
Windows-specific GetLastError string. So in 2/3 of the cases using
qemu_oom_check() is definitely wrong, and in the third it's dubious
given that Windows malloc() doesn't set GetLastError (more on that
later). So we start by switching these 3 calls to simple in-line error
checking code, which means we can make qemu_oom_check() not a public
global function.

So now we have a qemu_oom_check() which is used only in
qemu_memalign() and whose major difference between the Windows and
POSIX versions is that the former prints the GetLastError error number
value. This is actually a bug -- in commit dfbd0b873a85021 in 2020 we
changed the implementation of qemu_try_memalign() from using
VirtualAlloc() without noticing that this also should imply changing
what we print in the error case. So we can switch to a single
shared qemu_memalign() which does the error check directly itself
and always prints errno (and the requested size and alignment,
which seem more useful if anybody needs to diagnose the problem).

qemu_try_memalign() also is very similar between POSIX and Windows
versions (it used to be less so, but we have gradually ended up
with the two versions quite similar). The POSIX version already
has an ifdef ladder for different kinds of "allocate aligned
memory" host functions, so it seems neater to just have Windows'
_aligned_malloc() be another ladder in that ifdef rather than
a totally separate function.

Finally, I put the qemu_memalign() and related function prototypes
into a new memalign.h header, because that gets them out of osdep.h
and into somewhere that only the files that care about these functions
need to include.

thanks
--PMM

Peter Maydell (9):
  hw/usb/redirect.c: Stop using qemu_oom_check()
  util: Make qemu_oom_check() a static function
  util: Unify implementations of qemu_memalign()
  util/oslib-win32: Return NULL on qemu_try_memalign() with zero size
  meson.build: Don't misdetect posix_memalign() on Windows
  util: Share qemu_try_memalign() implementation between POSIX and
    Windows
  util: Use meson checks for valloc() and memalign() presence
  util: Put qemu_vfree() in memalign.c
  osdep: Move memalign-related functions to their own header

 meson.build                    |  7 ++-
 include/qemu-common.h          |  2 -
 include/qemu/memalign.h        | 61 +++++++++++++++++++++++
 include/qemu/osdep.h           | 18 -------
 block/blkverify.c              |  1 +
 block/block-copy.c             |  1 +
 block/commit.c                 |  1 +
 block/crypto.c                 |  1 +
 block/dmg.c                    |  1 +
 block/export/fuse.c            |  1 +
 block/file-posix.c             |  1 +
 block/io.c                     |  1 +
 block/mirror.c                 |  1 +
 block/nvme.c                   |  1 +
 block/parallels-ext.c          |  1 +
 block/parallels.c              |  1 +
 block/qcow.c                   |  1 +
 block/qcow2-cache.c            |  1 +
 block/qcow2-cluster.c          |  1 +
 block/qcow2-refcount.c         |  1 +
 block/qcow2-snapshot.c         |  1 +
 block/qcow2.c                  |  1 +
 block/qed-l2-cache.c           |  1 +
 block/qed-table.c              |  1 +
 block/qed.c                    |  1 +
 block/quorum.c                 |  1 +
 block/raw-format.c             |  1 +
 block/vdi.c                    |  1 +
 block/vhdx-log.c               |  1 +
 block/vhdx.c                   |  1 +
 block/vmdk.c                   |  1 +
 block/vpc.c                    |  1 +
 block/win32-aio.c              |  1 +
 hw/block/dataplane/xen-block.c |  1 +
 hw/block/fdc.c                 |  1 +
 hw/ide/core.c                  |  1 +
 hw/ppc/spapr.c                 |  1 +
 hw/ppc/spapr_softmmu.c         |  1 +
 hw/scsi/scsi-disk.c            |  1 +
 hw/tpm/tpm_ppi.c               |  2 +-
 hw/usb/redirect.c              | 17 +++++--
 nbd/server.c                   |  1 +
 net/l2tpv3.c                   |  2 +-
 plugins/loader.c               |  1 +
 qemu-img.c                     |  1 +
 qemu-io-cmds.c                 |  1 +
 qom/object.c                   |  1 +
 softmmu/physmem.c              |  1 +
 target/i386/hvf/hvf.c          |  1 +
 target/i386/kvm/kvm.c          |  1 +
 tcg/region.c                   |  1 +
 tests/bench/atomic_add-bench.c |  1 +
 tests/bench/qht-bench.c        |  1 +
 util/atomic64.c                |  1 +
 util/memalign.c                | 88 ++++++++++++++++++++++++++++++++++
 util/oslib-posix.c             | 46 ------------------
 util/oslib-win32.c             | 35 --------------
 util/qht.c                     |  1 +
 util/meson.build               |  1 +
 59 files changed, 220 insertions(+), 107 deletions(-)
 create mode 100644 include/qemu/memalign.h
 create mode 100644 util/memalign.c

-- 
2.25.1

[PATCH 1/9] hw/usb/redirect.c: Stop using qemu_oom_check()

[Peter Maydell]

qemu_oom_check() is a function which essentially says "if you pass me
a NULL pointer then print a message then abort()".  On POSIX systems
the message includes strerror(errno); on Windows it includes the
GetLastError() error value printed as an integer.

Other than in the implementation of qemu_memalign(), we use this
function only in hw/usb/redirect.c, for three checks:

 * on a call to usbredirparser_create()
 * on a call to usberedirparser_serialize()
 * on a call to malloc()

The usbredir library API functions make no guarantees that they will
set errno on errors, let alone that they might set the
Windows-specific GetLastError string.  malloc() is documented as
setting errno, not GetLastError -- and in any case the only thing it
might set errno to is ENOMEM.  So qemu_oom_check() isn't the right
thing for any of these.  Replace them with straightforward
error-checking code.  This will allow us to get rid of
qemu_oom_check().

Signed-off-by: Peter Maydell <peter.maydell@linaro.org>
---
I have left all of these errors as fatal, since that's what they
were previously. Possibly somebody with a better understanding
of the usbredir code might be able to make them theoretically
non-fatal, but we make malloc failures generally fatal anyway.
---
 hw/usb/redirect.c | 17 ++++++++++++++---
 1 file changed, 14 insertions(+), 3 deletions(-)

diff --git a/hw/usb/redirect.c b/hw/usb/redirect.c
index 5f0ef9cb3b0..8692ea25610 100644
--- a/hw/usb/redirect.c
+++ b/hw/usb/redirect.c
@@ -1239,7 +1239,11 @@ static void usbredir_create_parser(USBRedirDevice *dev)
 
     DPRINTF("creating usbredirparser\n");
 
-    dev->parser = qemu_oom_check(usbredirparser_create());
+    dev->parser = usbredirparser_create();
+    if (!dev->parser) {
+        error_report("usbredirparser_create() failed");
+        exit(1);
+    }
     dev->parser->priv = dev;
     dev->parser->log_func = usbredir_log;
     dev->parser->read_func = usbredir_read;
@@ -2239,7 +2243,10 @@ static int usbredir_put_parser(QEMUFile *f, void *priv, size_t unused,
     }
 
     usbredirparser_serialize(dev->parser, &data, &len);
-    qemu_oom_check(data);
+    if (!data) {
+        error_report("usbredirparser_serialize failed");
+        exit(1);
+    }
 
     qemu_put_be32(f, len);
     qemu_put_buffer(f, data, len);
@@ -2330,7 +2337,11 @@ static int usbredir_get_bufpq(QEMUFile *f, void *priv, size_t unused,
         bufp->len = qemu_get_be32(f);
         bufp->status = qemu_get_be32(f);
         bufp->offset = 0;
-        bufp->data = qemu_oom_check(malloc(bufp->len)); /* regular malloc! */
+        bufp->data = malloc(bufp->len); /* regular malloc! */
+        if (!bufp->data) {
+            error_report("usbredir_get_bufpq: out of memory");
+            exit(1);
+        }
         bufp->free_on_destroy = bufp->data;
         qemu_get_buffer(f, bufp->data, bufp->len);
         QTAILQ_INSERT_TAIL(&endp->bufpq, bufp, next);
-- 
2.25.1

[PATCH 2/9] util: Make qemu_oom_check() a static function

[Peter Maydell]

The qemu_oom_check() function, which we define in both oslib-posix.c
and oslib-win32.c, is now used only locally in that file; make it
static.

Signed-off-by: Peter Maydell <peter.maydell@linaro.org>
---
 include/qemu-common.h | 2 --
 util/oslib-posix.c    | 2 +-
 util/oslib-win32.c    | 2 +-
 3 files changed, 2 insertions(+), 4 deletions(-)

diff --git a/include/qemu-common.h b/include/qemu-common.h
index 68b2e3bc109..8c0d9ab0f77 100644
--- a/include/qemu-common.h
+++ b/include/qemu-common.h
@@ -26,8 +26,6 @@
 int qemu_main(int argc, char **argv, char **envp);
 #endif
 
-void *qemu_oom_check(void *ptr);
-
 ssize_t qemu_write_full(int fd, const void *buf, size_t count)
     QEMU_WARN_UNUSED_RESULT;
 
diff --git a/util/oslib-posix.c b/util/oslib-posix.c
index f2be7321c59..ed5974d3845 100644
--- a/util/oslib-posix.c
+++ b/util/oslib-posix.c
@@ -199,7 +199,7 @@ fail_close:
     return false;
 }
 
-void *qemu_oom_check(void *ptr)
+static void *qemu_oom_check(void *ptr)
 {
     if (ptr == NULL) {
         fprintf(stderr, "Failed to allocate memory: %s\n", strerror(errno));
diff --git a/util/oslib-win32.c b/util/oslib-win32.c
index af559ef3398..c87e6977246 100644
--- a/util/oslib-win32.c
+++ b/util/oslib-win32.c
@@ -44,7 +44,7 @@
 /* this must come after including "trace.h" */
 #include <shlobj.h>
 
-void *qemu_oom_check(void *ptr)
+static void *qemu_oom_check(void *ptr)
 {
     if (ptr == NULL) {
         fprintf(stderr, "Failed to allocate memory: %lu\n", GetLastError());
-- 
2.25.1

[PATCH 3/9] util: Unify implementations of qemu_memalign()

[Peter Maydell]

We implement qemu_memalign() in both oslib-posix.c and oslib-win32.c,
but the two versions are essentially the same: they call
qemu_try_memalign(), and abort() after printing an error message if
it fails.  The only difference is that the win32 version prints the
GetLastError() value whereas the POSIX version prints
strerror(errno).  However, this is a bug in the win32 version: in
commit dfbd0b873a85021 in 2020 we changed the implementation of
qemu_try_memalign() from using VirtualAlloc() (which sets the
GetLastError() value) to using _aligned_malloc() (which sets errno),
but didn't update the error message to match.

Replace the two separate functions with a single version in a
new memalign.c file, which drops the unnecessary extra qemu_oom_check()
function and instead prints a more useful message including the
requested size and alignment as well as the errno string.

Signed-off-by: Peter Maydell <peter.maydell@linaro.org>
---
 util/memalign.c    | 39 +++++++++++++++++++++++++++++++++++++++
 util/oslib-posix.c | 14 --------------
 util/oslib-win32.c | 14 --------------
 util/meson.build   |  1 +
 4 files changed, 40 insertions(+), 28 deletions(-)
 create mode 100644 util/memalign.c

diff --git a/util/memalign.c b/util/memalign.c
new file mode 100644
index 00000000000..6dfc20abbb1
--- /dev/null
+++ b/util/memalign.c
@@ -0,0 +1,39 @@
+/*
+ * memalign.c: Allocate an aligned memory region
+ *
+ * Copyright (c) 2003-2008 Fabrice Bellard
+ * Copyright (c) 2010-2016 Red Hat, Inc.
+ * Copyright (c) 2022 Linaro Ltd
+ *
+ * Permission is hereby granted, free of charge, to any person obtaining a copy
+ * of this software and associated documentation files (the "Software"), to deal
+ * in the Software without restriction, including without limitation the rights
+ * to use, copy, modify, merge, publish, distribute, sublicense, and/or sell
+ * copies of the Software, and to permit persons to whom the Software is
+ * furnished to do so, subject to the following conditions:
+ *
+ * The above copyright notice and this permission notice shall be included in
+ * all copies or substantial portions of the Software.
+ *
+ * THE SOFTWARE IS PROVIDED "AS IS", WITHOUT WARRANTY OF ANY KIND, EXPRESS OR
+ * IMPLIED, INCLUDING BUT NOT LIMITED TO THE WARRANTIES OF MERCHANTABILITY,
+ * FITNESS FOR A PARTICULAR PURPOSE AND NONINFRINGEMENT. IN NO EVENT SHALL
+ * THE AUTHORS OR COPYRIGHT HOLDERS BE LIABLE FOR ANY CLAIM, DAMAGES OR OTHER
+ * LIABILITY, WHETHER IN AN ACTION OF CONTRACT, TORT OR OTHERWISE, ARISING FROM,
+ * OUT OF OR IN CONNECTION WITH THE SOFTWARE OR THE USE OR OTHER DEALINGS IN
+ * THE SOFTWARE.
+ */
+
+#include "qemu/osdep.h"
+
+void *qemu_memalign(size_t alignment, size_t size)
+{
+    void *p = qemu_try_memalign(alignment, size);
+    if (p) {
+        return p;
+    }
+    fprintf(stderr,
+            "qemu_memalign: failed to allocate %zu bytes at alignment %zu: %s\n",
+            size, alignment, strerror(errno));
+    abort();
+}
diff --git a/util/oslib-posix.c b/util/oslib-posix.c
index ed5974d3845..0278902ee79 100644
--- a/util/oslib-posix.c
+++ b/util/oslib-posix.c
@@ -199,15 +199,6 @@ fail_close:
     return false;
 }
 
-static void *qemu_oom_check(void *ptr)
-{
-    if (ptr == NULL) {
-        fprintf(stderr, "Failed to allocate memory: %s\n", strerror(errno));
-        abort();
-    }
-    return ptr;
-}
-
 void *qemu_try_memalign(size_t alignment, size_t size)
 {
     void *ptr;
@@ -234,11 +225,6 @@ void *qemu_try_memalign(size_t alignment, size_t size)
     return ptr;
 }
 
-void *qemu_memalign(size_t alignment, size_t size)
-{
-    return qemu_oom_check(qemu_try_memalign(alignment, size));
-}
-
 /* alloc shared memory pages */
 void *qemu_anon_ram_alloc(size_t size, uint64_t *alignment, bool shared,
                           bool noreserve)
diff --git a/util/oslib-win32.c b/util/oslib-win32.c
index c87e6977246..05857414695 100644
--- a/util/oslib-win32.c
+++ b/util/oslib-win32.c
@@ -44,15 +44,6 @@
 /* this must come after including "trace.h" */
 #include <shlobj.h>
 
-static void *qemu_oom_check(void *ptr)
-{
-    if (ptr == NULL) {
-        fprintf(stderr, "Failed to allocate memory: %lu\n", GetLastError());
-        abort();
-    }
-    return ptr;
-}
-
 void *qemu_try_memalign(size_t alignment, size_t size)
 {
     void *ptr;
@@ -68,11 +59,6 @@ void *qemu_try_memalign(size_t alignment, size_t size)
     return ptr;
 }
 
-void *qemu_memalign(size_t alignment, size_t size)
-{
-    return qemu_oom_check(qemu_try_memalign(alignment, size));
-}
-
 static int get_allocation_granularity(void)
 {
     SYSTEM_INFO system_info;
diff --git a/util/meson.build b/util/meson.build
index 3736988b9f6..f6ee74ad0c8 100644
--- a/util/meson.build
+++ b/util/meson.build
@@ -51,6 +51,7 @@ util_ss.add(when: 'CONFIG_POSIX', if_true: files('drm.c'))
 util_ss.add(files('guest-random.c'))
 util_ss.add(files('yank.c'))
 util_ss.add(files('int128.c'))
+util_ss.add(files('memalign.c'))
 
 if have_user
   util_ss.add(files('selfmap.c'))
-- 
2.25.1

[PATCH 4/9] util/oslib-win32: Return NULL on qemu_try_memalign() with zero size

[Peter Maydell]

Currently if qemu_try_memalign() is asked to allocate 0 bytes, we assert.
Instead return NULL; this is in line with the posix_memalign() API,
and is valid to pass to _aligned_free() (which will do nothing).

This change is a preparation for sharing the qemu_try_memalign()
code between Windows and POSIX -- at the moment only the Windows
version has the assert that size != 0.

Signed-off-by: Peter Maydell <peter.maydell@linaro.org>
---
 util/oslib-win32.c | 7 +++++--
 1 file changed, 5 insertions(+), 2 deletions(-)

diff --git a/util/oslib-win32.c b/util/oslib-win32.c
index 05857414695..8c1c64719d7 100644
--- a/util/oslib-win32.c
+++ b/util/oslib-win32.c
@@ -48,13 +48,16 @@ void *qemu_try_memalign(size_t alignment, size_t size)
 {
     void *ptr;
 
-    g_assert(size != 0);
     if (alignment < sizeof(void *)) {
         alignment = sizeof(void *);
     } else {
         g_assert(is_power_of_2(alignment));
     }
-    ptr = _aligned_malloc(size, alignment);
+    if (size) {
+        ptr = _aligned_malloc(size, alignment);
+    } else {
+        ptr = NULL;
+    }
     trace_qemu_memalign(alignment, size, ptr);
     return ptr;
 }
-- 
2.25.1

[PATCH 5/9] meson.build: Don't misdetect posix_memalign() on Windows

[Peter Maydell]

Currently we incorrectly think that posix_memalign() exists on
Windows.  This is because of a combination of:

 * the msys2/mingw toolchain/libc claim to have a
   __builtin_posix_memalign when there isn't a builtin of that name
 * meson will assume that if you have a __builtin_foo that
   counts for has_function('foo')

Specifying a specific include file via prefix: causes meson to not
treat builtins as sufficient and actually look for the function
itself; see this meson pull request which added that as the official
way to get the right answer:
  https://github.com/mesonbuild/meson/pull/1150

Currently this misdectection doesn't cause problems because we only
use CONFIG_POSIX_MEMALIGN in oslib-posix.c; however that will change
in a following commit.

Signed-off-by: Peter Maydell <peter.maydell@linaro.org>
---
 meson.build | 4 +++-
 1 file changed, 3 insertions(+), 1 deletion(-)

diff --git a/meson.build b/meson.build
index 8df40bfac4d..e08de177c87 100644
--- a/meson.build
+++ b/meson.build
@@ -1606,7 +1606,9 @@ config_host_data.set('CONFIG_CLOCK_ADJTIME', cc.has_function('clock_adjtime'))
 config_host_data.set('CONFIG_DUP3', cc.has_function('dup3'))
 config_host_data.set('CONFIG_FALLOCATE', cc.has_function('fallocate'))
 config_host_data.set('CONFIG_POSIX_FALLOCATE', cc.has_function('posix_fallocate'))
-config_host_data.set('CONFIG_POSIX_MEMALIGN', cc.has_function('posix_memalign'))
+# Note that we need to specify prefix: here to avoid incorrectly
+# thinking that Windows has posix_memalign()
+config_host_data.set('CONFIG_POSIX_MEMALIGN', cc.has_function('posix_memalign', prefix: '#include <stdlib.h>'))
 config_host_data.set('CONFIG_PPOLL', cc.has_function('ppoll'))
 config_host_data.set('CONFIG_PREADV', cc.has_function('preadv', prefix: '#include <sys/uio.h>'))
 config_host_data.set('CONFIG_SEM_TIMEDWAIT', cc.has_function('sem_timedwait', dependencies: threads))
-- 
2.25.1

[PATCH 6/9] util: Share qemu_try_memalign() implementation between POSIX and Windows

[Peter Maydell]

The qemu_try_memalign() functions for POSIX and Windows used to be
significantly different, but these days they are identical except for
the actual allocation function called, and the POSIX version already
has to have ifdeffery for different allocation functions.

Move to a single implementation in memalign.c, which uses the Windows
_aligned_malloc if we detect that function in meson.

Signed-off-by: Peter Maydell <peter.maydell@linaro.org>
---
 meson.build        |  1 +
 util/memalign.c    | 33 +++++++++++++++++++++++++++++++++
 util/oslib-posix.c | 26 --------------------------
 util/oslib-win32.c | 18 ------------------
 4 files changed, 34 insertions(+), 44 deletions(-)

diff --git a/meson.build b/meson.build
index e08de177c87..21511d4fb61 100644
--- a/meson.build
+++ b/meson.build
@@ -1609,6 +1609,7 @@ config_host_data.set('CONFIG_POSIX_FALLOCATE', cc.has_function('posix_fallocate'
 # Note that we need to specify prefix: here to avoid incorrectly
 # thinking that Windows has posix_memalign()
 config_host_data.set('CONFIG_POSIX_MEMALIGN', cc.has_function('posix_memalign', prefix: '#include <stdlib.h>'))
+config_host_data.set('CONFIG_ALIGNED_MALLOC', cc.has_function('_aligned_malloc'))
 config_host_data.set('CONFIG_PPOLL', cc.has_function('ppoll'))
 config_host_data.set('CONFIG_PREADV', cc.has_function('preadv', prefix: '#include <sys/uio.h>'))
 config_host_data.set('CONFIG_SEM_TIMEDWAIT', cc.has_function('sem_timedwait', dependencies: threads))
diff --git a/util/memalign.c b/util/memalign.c
index 6dfc20abbb1..ee3393cc601 100644
--- a/util/memalign.c
+++ b/util/memalign.c
@@ -26,6 +26,39 @@
 
 #include "qemu/osdep.h"
 
+void *qemu_try_memalign(size_t alignment, size_t size)
+{
+    void *ptr;
+
+    if (alignment < sizeof(void*)) {
+        alignment = sizeof(void*);
+    } else {
+        g_assert(is_power_of_2(alignment));
+    }
+
+#if defined(CONFIG_POSIX_MEMALIGN)
+    int ret;
+    ret = posix_memalign(&ptr, alignment, size);
+    if (ret != 0) {
+        errno = ret;
+        ptr = NULL;
+    }
+#elif defined(CONFIG_ALIGNED_MALLOC)
+    /* _aligned_malloc() fails on 0 size */
+    if (size) {
+        ptr = _aligned_malloc(size, alignment);
+    } else {
+        ptr = NULL;
+    }
+#elif defined(CONFIG_BSD)
+    ptr = valloc(size);
+#else
+    ptr = memalign(alignment, size);
+#endif
+    trace_qemu_memalign(alignment, size, ptr);
+    return ptr;
+}
+
 void *qemu_memalign(size_t alignment, size_t size)
 {
     void *p = qemu_try_memalign(alignment, size);
diff --git a/util/oslib-posix.c b/util/oslib-posix.c
index 0278902ee79..91798f7e504 100644
--- a/util/oslib-posix.c
+++ b/util/oslib-posix.c
@@ -199,32 +199,6 @@ fail_close:
     return false;
 }
 
-void *qemu_try_memalign(size_t alignment, size_t size)
-{
-    void *ptr;
-
-    if (alignment < sizeof(void*)) {
-        alignment = sizeof(void*);
-    } else {
-        g_assert(is_power_of_2(alignment));
-    }
-
-#if defined(CONFIG_POSIX_MEMALIGN)
-    int ret;
-    ret = posix_memalign(&ptr, alignment, size);
-    if (ret != 0) {
-        errno = ret;
-        ptr = NULL;
-    }
-#elif defined(CONFIG_BSD)
-    ptr = valloc(size);
-#else
-    ptr = memalign(alignment, size);
-#endif
-    trace_qemu_memalign(alignment, size, ptr);
-    return ptr;
-}
-
 /* alloc shared memory pages */
 void *qemu_anon_ram_alloc(size_t size, uint64_t *alignment, bool shared,
                           bool noreserve)
diff --git a/util/oslib-win32.c b/util/oslib-win32.c
index 8c1c64719d7..d9768532bec 100644
--- a/util/oslib-win32.c
+++ b/util/oslib-win32.c
@@ -44,24 +44,6 @@
 /* this must come after including "trace.h" */
 #include <shlobj.h>
 
-void *qemu_try_memalign(size_t alignment, size_t size)
-{
-    void *ptr;
-
-    if (alignment < sizeof(void *)) {
-        alignment = sizeof(void *);
-    } else {
-        g_assert(is_power_of_2(alignment));
-    }
-    if (size) {
-        ptr = _aligned_malloc(size, alignment);
-    } else {
-        ptr = NULL;
-    }
-    trace_qemu_memalign(alignment, size, ptr);
-    return ptr;
-}
-
 static int get_allocation_granularity(void)
 {
     SYSTEM_INFO system_info;
-- 
2.25.1

[PATCH 7/9] util: Use meson checks for valloc() and memalign() presence

[Peter Maydell]

Instead of assuming that all CONFIG_BSD have valloc() and anything
else is memalign(), explicitly check for those functions in
meson.build and use the "is the function present" define.  Tests for
specific functionality are better than which-OS checks; this also
lets us give a helpful error message if somehow there's no usable
function present.

Signed-off-by: Peter Maydell <peter.maydell@linaro.org>
---
 meson.build     | 2 ++
 util/memalign.c | 8 ++++++--
 2 files changed, 8 insertions(+), 2 deletions(-)

diff --git a/meson.build b/meson.build
index 21511d4fb61..1c44198ebda 100644
--- a/meson.build
+++ b/meson.build
@@ -1610,6 +1610,8 @@ config_host_data.set('CONFIG_POSIX_FALLOCATE', cc.has_function('posix_fallocate'
 # thinking that Windows has posix_memalign()
 config_host_data.set('CONFIG_POSIX_MEMALIGN', cc.has_function('posix_memalign', prefix: '#include <stdlib.h>'))
 config_host_data.set('CONFIG_ALIGNED_MALLOC', cc.has_function('_aligned_malloc'))
+config_host_data.set('CONFIG_VALLOC', cc.has_function('valloc'))
+config_host_data.set('CONFIG_MEMALIGN', cc.has_function('memalign'))
 config_host_data.set('CONFIG_PPOLL', cc.has_function('ppoll'))
 config_host_data.set('CONFIG_PREADV', cc.has_function('preadv', prefix: '#include <sys/uio.h>'))
 config_host_data.set('CONFIG_SEM_TIMEDWAIT', cc.has_function('sem_timedwait', dependencies: threads))
diff --git a/util/memalign.c b/util/memalign.c
index ee3393cc601..fc8228bffb5 100644
--- a/util/memalign.c
+++ b/util/memalign.c
@@ -25,6 +25,8 @@
  */
 
 #include "qemu/osdep.h"
+#include "qemu/host-utils.h"
+#include "trace.h"
 
 void *qemu_try_memalign(size_t alignment, size_t size)
 {
@@ -50,10 +52,12 @@ void *qemu_try_memalign(size_t alignment, size_t size)
     } else {
         ptr = NULL;
     }
-#elif defined(CONFIG_BSD)
+#elif defined(CONFIG_VALLOC)
     ptr = valloc(size);
-#else
+#elif defined(CONFIG_MEMALIGN)
     ptr = memalign(alignment, size);
+#else
+    #error No function to allocate aligned memory available
 #endif
     trace_qemu_memalign(alignment, size, ptr);
     return ptr;
-- 
2.25.1

[PATCH 8/9] util: Put qemu_vfree() in memalign.c

[Peter Maydell]

qemu_vfree() is the companion free function to qemu_memalign(); put
it in memalign.c so the allocation and free functions are together.

Signed-off-by: Peter Maydell <peter.maydell@linaro.org>
---
 util/memalign.c    | 11 +++++++++++
 util/oslib-posix.c |  6 ------
 util/oslib-win32.c |  6 ------
 3 files changed, 11 insertions(+), 12 deletions(-)

diff --git a/util/memalign.c b/util/memalign.c
index fc8228bffb5..db6cdb095f9 100644
--- a/util/memalign.c
+++ b/util/memalign.c
@@ -74,3 +74,14 @@ void *qemu_memalign(size_t alignment, size_t size)
             size, alignment, strerror(errno));
     abort();
 }
+
+void qemu_vfree(void *ptr)
+{
+    trace_qemu_vfree(ptr);
+#if !defined(CONFIG_POSIX_MEMALIGN) && defined(CONFIG_ALIGNED_MALLOC)
+    /* Only Windows _aligned_malloc needs a special free function */
+    _aligned_free(ptr);
+#else
+    free(ptr);
+#endif
+}
diff --git a/util/oslib-posix.c b/util/oslib-posix.c
index 91798f7e504..2ebfb750578 100644
--- a/util/oslib-posix.c
+++ b/util/oslib-posix.c
@@ -220,12 +220,6 @@ void *qemu_anon_ram_alloc(size_t size, uint64_t *alignment, bool shared,
     return ptr;
 }
 
-void qemu_vfree(void *ptr)
-{
-    trace_qemu_vfree(ptr);
-    free(ptr);
-}
-
 void qemu_anon_ram_free(void *ptr, size_t size)
 {
     trace_qemu_anon_ram_free(ptr, size);
diff --git a/util/oslib-win32.c b/util/oslib-win32.c
index d9768532bec..4b1ce0be4b0 100644
--- a/util/oslib-win32.c
+++ b/util/oslib-win32.c
@@ -75,12 +75,6 @@ void *qemu_anon_ram_alloc(size_t size, uint64_t *align, bool shared,
     return ptr;
 }
 
-void qemu_vfree(void *ptr)
-{
-    trace_qemu_vfree(ptr);
-    _aligned_free(ptr);
-}
-
 void qemu_anon_ram_free(void *ptr, size_t size)
 {
     trace_qemu_anon_ram_free(ptr, size);
-- 
2.25.1

[PATCH 9/9] osdep: Move memalign-related functions to their own header

[Peter Maydell]

Move the various memalign-related functions out of osdep.h and into
their own header, which we include only where they are used.
While we're doing this, add some brief documentation comments.

Signed-off-by: Peter Maydell <peter.maydell@linaro.org>
---
 include/qemu/memalign.h        | 61 ++++++++++++++++++++++++++++++++++
 include/qemu/osdep.h           | 18 ----------
 block/blkverify.c              |  1 +
 block/block-copy.c             |  1 +
 block/commit.c                 |  1 +
 block/crypto.c                 |  1 +
 block/dmg.c                    |  1 +
 block/export/fuse.c            |  1 +
 block/file-posix.c             |  1 +
 block/io.c                     |  1 +
 block/mirror.c                 |  1 +
 block/nvme.c                   |  1 +
 block/parallels-ext.c          |  1 +
 block/parallels.c              |  1 +
 block/qcow.c                   |  1 +
 block/qcow2-cache.c            |  1 +
 block/qcow2-cluster.c          |  1 +
 block/qcow2-refcount.c         |  1 +
 block/qcow2-snapshot.c         |  1 +
 block/qcow2.c                  |  1 +
 block/qed-l2-cache.c           |  1 +
 block/qed-table.c              |  1 +
 block/qed.c                    |  1 +
 block/quorum.c                 |  1 +
 block/raw-format.c             |  1 +
 block/vdi.c                    |  1 +
 block/vhdx-log.c               |  1 +
 block/vhdx.c                   |  1 +
 block/vmdk.c                   |  1 +
 block/vpc.c                    |  1 +
 block/win32-aio.c              |  1 +
 hw/block/dataplane/xen-block.c |  1 +
 hw/block/fdc.c                 |  1 +
 hw/ide/core.c                  |  1 +
 hw/ppc/spapr.c                 |  1 +
 hw/ppc/spapr_softmmu.c         |  1 +
 hw/scsi/scsi-disk.c            |  1 +
 hw/tpm/tpm_ppi.c               |  2 +-
 nbd/server.c                   |  1 +
 net/l2tpv3.c                   |  2 +-
 plugins/loader.c               |  1 +
 qemu-img.c                     |  1 +
 qemu-io-cmds.c                 |  1 +
 qom/object.c                   |  1 +
 softmmu/physmem.c              |  1 +
 target/i386/hvf/hvf.c          |  1 +
 target/i386/kvm/kvm.c          |  1 +
 tcg/region.c                   |  1 +
 tests/bench/atomic_add-bench.c |  1 +
 tests/bench/qht-bench.c        |  1 +
 util/atomic64.c                |  1 +
 util/memalign.c                |  1 +
 util/qht.c                     |  1 +
 53 files changed, 112 insertions(+), 20 deletions(-)
 create mode 100644 include/qemu/memalign.h

diff --git a/include/qemu/memalign.h b/include/qemu/memalign.h
new file mode 100644
index 00000000000..fa299f3bf67
--- /dev/null
+++ b/include/qemu/memalign.h
@@ -0,0 +1,61 @@
+/*
+ * Allocation and free functions for aligned memory
+ *
+ * This work is licensed under the terms of the GNU GPL, version 2 or later.
+ * See the COPYING file in the top-level directory.
+ */
+
+#ifndef QEMU_MEMALIGN_H
+#define QEMU_MEMALIGN_H
+
+/**
+ * qemu_try_memalign: Allocate aligned memory
+ * @alignment: required alignment, in bytes
+ * @size: size of allocation, in bytes
+ *
+ * Allocate memory on an aligned boundary (i.e. the returned
+ * address will be an exact multiple of @alignment).
+ * @alignment must be a power of 2, or the function will assert().
+ * On success, returns allocated memory; on failure, returns NULL.
+ *
+ * The memory allocated through this function must be freed via
+ * qemu_vfree() (and not via free()).
+ */
+void *qemu_try_memalign(size_t alignment, size_t size);
+/**
+ * qemu_memalign: Allocate aligned memory, without failing
+ * @alignment: required alignment, in bytes
+ * @size: size of allocation, in bytes
+ *
+ * Allocate memory in the same way as qemu_try_memalign(), but
+ * abort() with an error message if the memory allocation fails.
+ *
+ * The memory allocated through this function must be freed via
+ * qemu_vfree() (and not via free()).
+ */
+void *qemu_memalign(size_t alignment, size_t size);
+/**
+ * qemu_vfree: Free memory allocated through qemu_memalign
+ * @ptr: memory to free
+ *
+ * This function must be used to free memory allocated via qemu_memalign()
+ * or qemu_try_memalign(). (Using the wrong free function will cause
+ * subtle bugs on Windows hosts.)
+ */
+void qemu_vfree(void *ptr);
+/*
+ * It's an analog of GLIB's g_autoptr_cleanup_generic_gfree(), used to define
+ * g_autofree macro.
+ */
+static inline void qemu_cleanup_generic_vfree(void *p)
+{
+  void **pp = (void **)p;
+  qemu_vfree(*pp);
+}
+
+/*
+ * Analog of g_autofree, but qemu_vfree is called on cleanup instead of g_free.
+ */
+#define QEMU_AUTO_VFREE __attribute__((cleanup(qemu_cleanup_generic_vfree)))
+
+#endif
diff --git a/include/qemu/osdep.h b/include/qemu/osdep.h
index 7bcce3bceb0..bc3df26da36 100644
--- a/include/qemu/osdep.h
+++ b/include/qemu/osdep.h
@@ -379,28 +379,10 @@ extern "C" {
 #endif
 
 int qemu_daemon(int nochdir, int noclose);
-void *qemu_try_memalign(size_t alignment, size_t size);
-void *qemu_memalign(size_t alignment, size_t size);
 void *qemu_anon_ram_alloc(size_t size, uint64_t *align, bool shared,
                           bool noreserve);
-void qemu_vfree(void *ptr);
 void qemu_anon_ram_free(void *ptr, size_t size);
 
-/*
- * It's an analog of GLIB's g_autoptr_cleanup_generic_gfree(), used to define
- * g_autofree macro.
- */
-static inline void qemu_cleanup_generic_vfree(void *p)
-{
-  void **pp = (void **)p;
-  qemu_vfree(*pp);
-}
-
-/*
- * Analog of g_autofree, but qemu_vfree is called on cleanup instead of g_free.
- */
-#define QEMU_AUTO_VFREE __attribute__((cleanup(qemu_cleanup_generic_vfree)))
-
 #ifdef _WIN32
 #define HAVE_CHARDEV_SERIAL 1
 #elif defined(__linux__) || defined(__sun__) || defined(__FreeBSD__)    \
diff --git a/block/blkverify.c b/block/blkverify.c
index d1facf5ba90..53ba1c91957 100644
--- a/block/blkverify.c
+++ b/block/blkverify.c
@@ -16,6 +16,7 @@
 #include "qemu/cutils.h"
 #include "qemu/module.h"
 #include "qemu/option.h"
+#include "qemu/memalign.h"
 
 typedef struct {
     BdrvChild *test_file;
diff --git a/block/block-copy.c b/block/block-copy.c
index ce116318b57..619e5580faf 100644
--- a/block/block-copy.c
+++ b/block/block-copy.c
@@ -22,6 +22,7 @@
 #include "qemu/coroutine.h"
 #include "block/aio_task.h"
 #include "qemu/error-report.h"
+#include "qemu/memalign.h"
 
 #define BLOCK_COPY_MAX_COPY_RANGE (16 * MiB)
 #define BLOCK_COPY_MAX_BUFFER (1 * MiB)
diff --git a/block/commit.c b/block/commit.c
index b1fc7b908b2..a1292bb17ed 100644
--- a/block/commit.c
+++ b/block/commit.c
@@ -20,6 +20,7 @@
 #include "qapi/error.h"
 #include "qapi/qmp/qerror.h"
 #include "qemu/ratelimit.h"
+#include "qemu/memalign.h"
 #include "sysemu/block-backend.h"
 
 enum {
diff --git a/block/crypto.c b/block/crypto.c
index c8ba4681e20..a586dd040de 100644
--- a/block/crypto.c
+++ b/block/crypto.c
@@ -31,6 +31,7 @@
 #include "qemu/module.h"
 #include "qemu/option.h"
 #include "qemu/cutils.h"
+#include "qemu/memalign.h"
 #include "crypto.h"
 
 typedef struct BlockCrypto BlockCrypto;
diff --git a/block/dmg.c b/block/dmg.c
index 447901fbb87..c626587f9c5 100644
--- a/block/dmg.c
+++ b/block/dmg.c
@@ -27,6 +27,7 @@
 #include "qemu/bswap.h"
 #include "qemu/error-report.h"
 #include "qemu/module.h"
+#include "qemu/memalign.h"
 #include "dmg.h"
 
 int (*dmg_uncompress_bz2)(char *next_in, unsigned int avail_in,
diff --git a/block/export/fuse.c b/block/export/fuse.c
index fdda8e3c818..4a3bb5a1b8f 100644
--- a/block/export/fuse.c
+++ b/block/export/fuse.c
@@ -19,6 +19,7 @@
 #define FUSE_USE_VERSION 31
 
 #include "qemu/osdep.h"
+#include "qemu/memalign.h"
 #include "block/aio.h"
 #include "block/block.h"
 #include "block/export.h"
diff --git a/block/file-posix.c b/block/file-posix.c
index 1f1756e192a..c000a61db29 100644
--- a/block/file-posix.c
+++ b/block/file-posix.c
@@ -31,6 +31,7 @@
 #include "qemu/module.h"
 #include "qemu/option.h"
 #include "qemu/units.h"
+#include "qemu/memalign.h"
 #include "trace.h"
 #include "block/thread-pool.h"
 #include "qemu/iov.h"
diff --git a/block/io.c b/block/io.c
index 4e4cb556c58..ea031d39601 100644
--- a/block/io.c
+++ b/block/io.c
@@ -32,6 +32,7 @@
 #include "block/coroutines.h"
 #include "block/write-threshold.h"
 #include "qemu/cutils.h"
+#include "qemu/memalign.h"
 #include "qapi/error.h"
 #include "qemu/error-report.h"
 #include "qemu/main-loop.h"
diff --git a/block/mirror.c b/block/mirror.c
index 69b2c1c697e..eda8aab0eb8 100644
--- a/block/mirror.c
+++ b/block/mirror.c
@@ -23,6 +23,7 @@
 #include "qapi/qmp/qerror.h"
 #include "qemu/ratelimit.h"
 #include "qemu/bitmap.h"
+#include "qemu/memalign.h"
 
 #define MAX_IN_FLIGHT 16
 #define MAX_IO_BYTES (1 << 20) /* 1 Mb */
diff --git a/block/nvme.c b/block/nvme.c
index dd20de3865b..552029931d5 100644
--- a/block/nvme.c
+++ b/block/nvme.c
@@ -21,6 +21,7 @@
 #include "qemu/module.h"
 #include "qemu/cutils.h"
 #include "qemu/option.h"
+#include "qemu/memalign.h"
 #include "qemu/vfio-helpers.h"
 #include "block/block_int.h"
 #include "sysemu/replay.h"
diff --git a/block/parallels-ext.c b/block/parallels-ext.c
index e0dd0975c6f..cb22a427d72 100644
--- a/block/parallels-ext.c
+++ b/block/parallels-ext.c
@@ -29,6 +29,7 @@
 #include "parallels.h"
 #include "crypto/hash.h"
 #include "qemu/uuid.h"
+#include "qemu/memalign.h"
 
 #define PARALLELS_FORMAT_EXTENSION_MAGIC 0xAB234CEF23DCEA87ULL
 
diff --git a/block/parallels.c b/block/parallels.c
index 6ebad2a2bbc..5a3487537b4 100644
--- a/block/parallels.c
+++ b/block/parallels.c
@@ -41,6 +41,7 @@
 #include "qapi/qapi-visit-block-core.h"
 #include "qemu/bswap.h"
 #include "qemu/bitmap.h"
+#include "qemu/memalign.h"
 #include "migration/blocker.h"
 #include "parallels.h"
 
diff --git a/block/qcow.c b/block/qcow.c
index c39940f33eb..4fba1b9e364 100644
--- a/block/qcow.c
+++ b/block/qcow.c
@@ -32,6 +32,7 @@
 #include "qemu/option.h"
 #include "qemu/bswap.h"
 #include "qemu/cutils.h"
+#include "qemu/memalign.h"
 #include <zlib.h>
 #include "qapi/qmp/qdict.h"
 #include "qapi/qmp/qstring.h"
diff --git a/block/qcow2-cache.c b/block/qcow2-cache.c
index 7444b9c4ab0..8a0105911f7 100644
--- a/block/qcow2-cache.c
+++ b/block/qcow2-cache.c
@@ -23,6 +23,7 @@
  */
 
 #include "qemu/osdep.h"
+#include "qemu/memalign.h"
 #include "qcow2.h"
 #include "trace.h"
 
diff --git a/block/qcow2-cluster.c b/block/qcow2-cluster.c
index 21884a1ab9a..20a16ba6ee0 100644
--- a/block/qcow2-cluster.c
+++ b/block/qcow2-cluster.c
@@ -28,6 +28,7 @@
 #include "qapi/error.h"
 #include "qcow2.h"
 #include "qemu/bswap.h"
+#include "qemu/memalign.h"
 #include "trace.h"
 
 int qcow2_shrink_l1_table(BlockDriverState *bs, uint64_t exact_size)
diff --git a/block/qcow2-refcount.c b/block/qcow2-refcount.c
index 46145722527..94033972bed 100644
--- a/block/qcow2-refcount.c
+++ b/block/qcow2-refcount.c
@@ -28,6 +28,7 @@
 #include "qemu/range.h"
 #include "qemu/bswap.h"
 #include "qemu/cutils.h"
+#include "qemu/memalign.h"
 #include "trace.h"
 
 static int64_t alloc_clusters_noref(BlockDriverState *bs, uint64_t size,
diff --git a/block/qcow2-snapshot.c b/block/qcow2-snapshot.c
index 71ddb08c212..075269a0237 100644
--- a/block/qcow2-snapshot.c
+++ b/block/qcow2-snapshot.c
@@ -29,6 +29,7 @@
 #include "qemu/bswap.h"
 #include "qemu/error-report.h"
 #include "qemu/cutils.h"
+#include "qemu/memalign.h"
 
 static void qcow2_free_single_snapshot(BlockDriverState *bs, int i)
 {
diff --git a/block/qcow2.c b/block/qcow2.c
index c8115e1cba0..b5c47931ef4 100644
--- a/block/qcow2.c
+++ b/block/qcow2.c
@@ -38,6 +38,7 @@
 #include "qemu/option_int.h"
 #include "qemu/cutils.h"
 #include "qemu/bswap.h"
+#include "qemu/memalign.h"
 #include "qapi/qobject-input-visitor.h"
 #include "qapi/qapi-visit-block-core.h"
 #include "crypto.h"
diff --git a/block/qed-l2-cache.c b/block/qed-l2-cache.c
index b5483623989..caf2c024c2d 100644
--- a/block/qed-l2-cache.c
+++ b/block/qed-l2-cache.c
@@ -51,6 +51,7 @@
  */
 
 #include "qemu/osdep.h"
+#include "qemu/memalign.h"
 #include "trace.h"
 #include "qed.h"
 
diff --git a/block/qed-table.c b/block/qed-table.c
index 405d446cbe7..1cc844b1a5f 100644
--- a/block/qed-table.c
+++ b/block/qed-table.c
@@ -17,6 +17,7 @@
 #include "qemu/sockets.h" /* for EINPROGRESS on Windows */
 #include "qed.h"
 #include "qemu/bswap.h"
+#include "qemu/memalign.h"
 
 /* Called with table_lock held.  */
 static int coroutine_fn qed_read_table(BDRVQEDState *s, uint64_t offset,
diff --git a/block/qed.c b/block/qed.c
index 558d3646c4b..f34d9a3ac1a 100644
--- a/block/qed.c
+++ b/block/qed.c
@@ -20,6 +20,7 @@
 #include "qemu/main-loop.h"
 #include "qemu/module.h"
 #include "qemu/option.h"
+#include "qemu/memalign.h"
 #include "trace.h"
 #include "qed.h"
 #include "sysemu/block-backend.h"
diff --git a/block/quorum.c b/block/quorum.c
index c28dda7baac..f33f30d36b8 100644
--- a/block/quorum.c
+++ b/block/quorum.c
@@ -17,6 +17,7 @@
 #include "qemu/cutils.h"
 #include "qemu/module.h"
 #include "qemu/option.h"
+#include "qemu/memalign.h"
 #include "block/block_int.h"
 #include "block/coroutines.h"
 #include "block/qdict.h"
diff --git a/block/raw-format.c b/block/raw-format.c
index bda757fd195..69fd650eaf7 100644
--- a/block/raw-format.c
+++ b/block/raw-format.c
@@ -31,6 +31,7 @@
 #include "qapi/error.h"
 #include "qemu/module.h"
 #include "qemu/option.h"
+#include "qemu/memalign.h"
 
 typedef struct BDRVRawState {
     uint64_t offset;
diff --git a/block/vdi.c b/block/vdi.c
index bdc58d726ee..cca3a3a3567 100644
--- a/block/vdi.c
+++ b/block/vdi.c
@@ -64,6 +64,7 @@
 #include "qemu/coroutine.h"
 #include "qemu/cutils.h"
 #include "qemu/uuid.h"
+#include "qemu/memalign.h"
 
 /* Code configuration options. */
 
diff --git a/block/vhdx-log.c b/block/vhdx-log.c
index 7672161d955..ff0d4e0da05 100644
--- a/block/vhdx-log.c
+++ b/block/vhdx-log.c
@@ -23,6 +23,7 @@
 #include "block/block_int.h"
 #include "qemu/error-report.h"
 #include "qemu/bswap.h"
+#include "qemu/memalign.h"
 #include "vhdx.h"
 
 
diff --git a/block/vhdx.c b/block/vhdx.c
index 356ec4c455a..410c6f96101 100644
--- a/block/vhdx.c
+++ b/block/vhdx.c
@@ -25,6 +25,7 @@
 #include "qemu/crc32c.h"
 #include "qemu/bswap.h"
 #include "qemu/error-report.h"
+#include "qemu/memalign.h"
 #include "vhdx.h"
 #include "migration/blocker.h"
 #include "qemu/uuid.h"
diff --git a/block/vmdk.c b/block/vmdk.c
index 0dfab6e9413..37c0946066e 100644
--- a/block/vmdk.c
+++ b/block/vmdk.c
@@ -33,6 +33,7 @@
 #include "qemu/module.h"
 #include "qemu/option.h"
 #include "qemu/bswap.h"
+#include "qemu/memalign.h"
 #include "migration/blocker.h"
 #include "qemu/cutils.h"
 #include <zlib.h>
diff --git a/block/vpc.c b/block/vpc.c
index 297a26262ab..4d8f16e1990 100644
--- a/block/vpc.c
+++ b/block/vpc.c
@@ -33,6 +33,7 @@
 #include "migration/blocker.h"
 #include "qemu/bswap.h"
 #include "qemu/uuid.h"
+#include "qemu/memalign.h"
 #include "qapi/qmp/qdict.h"
 #include "qapi/qobject-input-visitor.h"
 #include "qapi/qapi-visit-block-core.h"
diff --git a/block/win32-aio.c b/block/win32-aio.c
index c57e10c9979..aadc7b1bc3c 100644
--- a/block/win32-aio.c
+++ b/block/win32-aio.c
@@ -29,6 +29,7 @@
 #include "block/raw-aio.h"
 #include "qemu/event_notifier.h"
 #include "qemu/iov.h"
+#include "qemu/memalign.h"
 #include <windows.h>
 #include <winioctl.h>
 
diff --git a/hw/block/dataplane/xen-block.c b/hw/block/dataplane/xen-block.c
index 860787580a3..2785b9e8497 100644
--- a/hw/block/dataplane/xen-block.c
+++ b/hw/block/dataplane/xen-block.c
@@ -21,6 +21,7 @@
 #include "qemu/osdep.h"
 #include "qemu/error-report.h"
 #include "qemu/main-loop.h"
+#include "qemu/memalign.h"
 #include "qapi/error.h"
 #include "hw/xen/xen_common.h"
 #include "hw/block/xen_blkif.h"
diff --git a/hw/block/fdc.c b/hw/block/fdc.c
index 21d18ac2e36..347875a0cda 100644
--- a/hw/block/fdc.c
+++ b/hw/block/fdc.c
@@ -32,6 +32,7 @@
 #include "qapi/error.h"
 #include "qemu/error-report.h"
 #include "qemu/timer.h"
+#include "qemu/memalign.h"
 #include "hw/irq.h"
 #include "hw/isa/isa.h"
 #include "hw/qdev-properties.h"
diff --git a/hw/ide/core.c b/hw/ide/core.c
index 33463d9b8f2..a7ac4de18ad 100644
--- a/hw/ide/core.c
+++ b/hw/ide/core.c
@@ -30,6 +30,7 @@
 #include "qemu/main-loop.h"
 #include "qemu/timer.h"
 #include "qemu/hw-version.h"
+#include "qemu/memalign.h"
 #include "sysemu/sysemu.h"
 #include "sysemu/blockdev.h"
 #include "sysemu/dma.h"
diff --git a/hw/ppc/spapr.c b/hw/ppc/spapr.c
index f0b75b22bb6..ae271743ed5 100644
--- a/hw/ppc/spapr.c
+++ b/hw/ppc/spapr.c
@@ -27,6 +27,7 @@
 #include "qemu/osdep.h"
 #include "qemu-common.h"
 #include "qemu/datadir.h"
+#include "qemu/memalign.h"
 #include "qapi/error.h"
 #include "qapi/qapi-events-machine.h"
 #include "qapi/qapi-events-qdev.h"
diff --git a/hw/ppc/spapr_softmmu.c b/hw/ppc/spapr_softmmu.c
index 4ee03c83e48..5170a33369e 100644
--- a/hw/ppc/spapr_softmmu.c
+++ b/hw/ppc/spapr_softmmu.c
@@ -1,5 +1,6 @@
 #include "qemu/osdep.h"
 #include "qemu/cutils.h"
+#include "qemu/memalign.h"
 #include "cpu.h"
 #include "helper_regs.h"
 #include "hw/ppc/spapr.h"
diff --git a/hw/scsi/scsi-disk.c b/hw/scsi/scsi-disk.c
index 3666b8d9468..072686ed58f 100644
--- a/hw/scsi/scsi-disk.c
+++ b/hw/scsi/scsi-disk.c
@@ -26,6 +26,7 @@
 #include "qemu/main-loop.h"
 #include "qemu/module.h"
 #include "qemu/hw-version.h"
+#include "qemu/memalign.h"
 #include "hw/scsi/scsi.h"
 #include "migration/qemu-file-types.h"
 #include "migration/vmstate.h"
diff --git a/hw/tpm/tpm_ppi.c b/hw/tpm/tpm_ppi.c
index 6dbb9f41e40..c89ac53e65e 100644
--- a/hw/tpm/tpm_ppi.c
+++ b/hw/tpm/tpm_ppi.c
@@ -12,7 +12,7 @@
  */
 
 #include "qemu/osdep.h"
-
+#include "qemu/memalign.h"
 #include "qapi/error.h"
 #include "sysemu/memory_mapping.h"
 #include "migration/vmstate.h"
diff --git a/nbd/server.c b/nbd/server.c
index 9fb2f264023..53e68cf027f 100644
--- a/nbd/server.c
+++ b/nbd/server.c
@@ -25,6 +25,7 @@
 #include "trace.h"
 #include "nbd-internal.h"
 #include "qemu/units.h"
+#include "qemu/memalign.h"
 
 #define NBD_META_ID_BASE_ALLOCATION 0
 #define NBD_META_ID_ALLOCATION_DEPTH 1
diff --git a/net/l2tpv3.c b/net/l2tpv3.c
index e4d4218db69..b8faa8796c8 100644
--- a/net/l2tpv3.c
+++ b/net/l2tpv3.c
@@ -34,7 +34,7 @@
 #include "qemu/sockets.h"
 #include "qemu/iov.h"
 #include "qemu/main-loop.h"
-
+#include "qemu/memalign.h"
 
 /* The buffer size needs to be investigated for optimum numbers and
  * optimum means of paging in on different systems. This size is
diff --git a/plugins/loader.c b/plugins/loader.c
index 4883b0a1cbc..88c30bde2d6 100644
--- a/plugins/loader.c
+++ b/plugins/loader.c
@@ -27,6 +27,7 @@
 #include "qemu/cacheinfo.h"
 #include "qemu/xxhash.h"
 #include "qemu/plugin.h"
+#include "qemu/memalign.h"
 #include "hw/core/cpu.h"
 #include "exec/exec-all.h"
 #ifndef CONFIG_USER_ONLY
diff --git a/qemu-img.c b/qemu-img.c
index 6fe2466032f..5dffb3e6160 100644
--- a/qemu-img.c
+++ b/qemu-img.c
@@ -42,6 +42,7 @@
 #include "qemu/module.h"
 #include "qemu/sockets.h"
 #include "qemu/units.h"
+#include "qemu/memalign.h"
 #include "qom/object_interfaces.h"
 #include "sysemu/block-backend.h"
 #include "block/block_int.h"
diff --git a/qemu-io-cmds.c b/qemu-io-cmds.c
index 46593d632d8..633b46cdb25 100644
--- a/qemu-io-cmds.c
+++ b/qemu-io-cmds.c
@@ -21,6 +21,7 @@
 #include "qemu/option.h"
 #include "qemu/timer.h"
 #include "qemu/cutils.h"
+#include "qemu/memalign.h"
 
 #define CMD_NOFILE_OK   0x01
 
diff --git a/qom/object.c b/qom/object.c
index 9f7a33139d4..4048a2fef1e 100644
--- a/qom/object.c
+++ b/qom/object.c
@@ -16,6 +16,7 @@
 #include "qom/object.h"
 #include "qom/object_interfaces.h"
 #include "qemu/cutils.h"
+#include "qemu/memalign.h"
 #include "qapi/visitor.h"
 #include "qapi/string-input-visitor.h"
 #include "qapi/string-output-visitor.h"
diff --git a/softmmu/physmem.c b/softmmu/physmem.c
index a13289a594a..59dcf13faf4 100644
--- a/softmmu/physmem.c
+++ b/softmmu/physmem.c
@@ -42,6 +42,7 @@
 #include "qemu/config-file.h"
 #include "qemu/error-report.h"
 #include "qemu/qemu-print.h"
+#include "qemu/memalign.h"
 #include "exec/memory.h"
 #include "exec/ioport.h"
 #include "sysemu/dma.h"
diff --git a/target/i386/hvf/hvf.c b/target/i386/hvf/hvf.c
index 4ba6e82fab3..fc12c02fb21 100644
--- a/target/i386/hvf/hvf.c
+++ b/target/i386/hvf/hvf.c
@@ -49,6 +49,7 @@
 #include "qemu/osdep.h"
 #include "qemu-common.h"
 #include "qemu/error-report.h"
+#include "qemu/memalign.h"
 
 #include "sysemu/hvf.h"
 #include "sysemu/hvf_int.h"
diff --git a/target/i386/kvm/kvm.c b/target/i386/kvm/kvm.c
index 2c8feb4a6f7..83d09883020 100644
--- a/target/i386/kvm/kvm.c
+++ b/target/i386/kvm/kvm.c
@@ -37,6 +37,7 @@
 #include "qemu/main-loop.h"
 #include "qemu/config-file.h"
 #include "qemu/error-report.h"
+#include "qemu/memalign.h"
 #include "hw/i386/x86.h"
 #include "hw/i386/apic.h"
 #include "hw/i386/apic_internal.h"
diff --git a/tcg/region.c b/tcg/region.c
index 72afb357389..97ca5291d52 100644
--- a/tcg/region.c
+++ b/tcg/region.c
@@ -26,6 +26,7 @@
 #include "qemu/units.h"
 #include "qemu/madvise.h"
 #include "qemu/mprotect.h"
+#include "qemu/memalign.h"
 #include "qemu/cacheinfo.h"
 #include "qapi/error.h"
 #include "exec/exec-all.h"
diff --git a/tests/bench/atomic_add-bench.c b/tests/bench/atomic_add-bench.c
index f05471ab45c..8a6faad6ece 100644
--- a/tests/bench/atomic_add-bench.c
+++ b/tests/bench/atomic_add-bench.c
@@ -2,6 +2,7 @@
 #include "qemu/thread.h"
 #include "qemu/host-utils.h"
 #include "qemu/processor.h"
+#include "qemu/memalign.h"
 
 struct thread_info {
     uint64_t r;
diff --git a/tests/bench/qht-bench.c b/tests/bench/qht-bench.c
index 2e5b70ccd04..8afe161d106 100644
--- a/tests/bench/qht-bench.c
+++ b/tests/bench/qht-bench.c
@@ -10,6 +10,7 @@
 #include "qemu/qht.h"
 #include "qemu/rcu.h"
 #include "qemu/xxhash.h"
+#include "qemu/memalign.h"
 
 struct thread_stats {
     size_t rd;
diff --git a/util/atomic64.c b/util/atomic64.c
index 22983a970f1..c20d071d8e5 100644
--- a/util/atomic64.c
+++ b/util/atomic64.c
@@ -8,6 +8,7 @@
 #include "qemu/atomic.h"
 #include "qemu/thread.h"
 #include "qemu/cacheinfo.h"
+#include "qemu/memalign.h"
 
 #ifdef CONFIG_ATOMIC64
 #error This file must only be compiled if !CONFIG_ATOMIC64
diff --git a/util/memalign.c b/util/memalign.c
index db6cdb095f9..6420f4dc037 100644
--- a/util/memalign.c
+++ b/util/memalign.c
@@ -26,6 +26,7 @@
 
 #include "qemu/osdep.h"
 #include "qemu/host-utils.h"
+#include "qemu/memalign.h"
 #include "trace.h"
 
 void *qemu_try_memalign(size_t alignment, size_t size)
diff --git a/util/qht.c b/util/qht.c
index 079605121b1..065fc501f44 100644
--- a/util/qht.c
+++ b/util/qht.c
@@ -69,6 +69,7 @@
 #include "qemu/qht.h"
 #include "qemu/atomic.h"
 #include "qemu/rcu.h"
+#include "qemu/memalign.h"
 
 //#define QHT_DEBUG
 
-- 
2.25.1

Re: [PATCH 1/9] hw/usb/redirect.c: Stop using qemu_oom_check()

[Peter Maydell]

On Sat, 26 Feb 2022 at 18:07, Peter Maydell <peter.maydell@linaro.org> wrote:

Forgot to cc Gerd on this one as USB maintainer. Sorry..

> qemu_oom_check() is a function which essentially says "if you pass me
> a NULL pointer then print a message then abort()".  On POSIX systems
> the message includes strerror(errno); on Windows it includes the
> GetLastError() error value printed as an integer.
>
> Other than in the implementation of qemu_memalign(), we use this
> function only in hw/usb/redirect.c, for three checks:
>
>  * on a call to usbredirparser_create()
>  * on a call to usberedirparser_serialize()
>  * on a call to malloc()
>
> The usbredir library API functions make no guarantees that they will
> set errno on errors, let alone that they might set the
> Windows-specific GetLastError string.  malloc() is documented as
> setting errno, not GetLastError -- and in any case the only thing it
> might set errno to is ENOMEM.  So qemu_oom_check() isn't the right
> thing for any of these.  Replace them with straightforward
> error-checking code.  This will allow us to get rid of
> qemu_oom_check().
>
> Signed-off-by: Peter Maydell <peter.maydell@linaro.org>
> ---
> I have left all of these errors as fatal, since that's what they
> were previously. Possibly somebody with a better understanding
> of the usbredir code might be able to make them theoretically
> non-fatal, but we make malloc failures generally fatal anyway.
> ---
>  hw/usb/redirect.c | 17 ++++++++++++++---
>  1 file changed, 14 insertions(+), 3 deletions(-)
>
> diff --git a/hw/usb/redirect.c b/hw/usb/redirect.c
> index 5f0ef9cb3b0..8692ea25610 100644
> --- a/hw/usb/redirect.c
> +++ b/hw/usb/redirect.c
> @@ -1239,7 +1239,11 @@ static void usbredir_create_parser(USBRedirDevice *dev)
>
>      DPRINTF("creating usbredirparser\n");
>
> -    dev->parser = qemu_oom_check(usbredirparser_create());
> +    dev->parser = usbredirparser_create();
> +    if (!dev->parser) {
> +        error_report("usbredirparser_create() failed");
> +        exit(1);
> +    }
>      dev->parser->priv = dev;
>      dev->parser->log_func = usbredir_log;
>      dev->parser->read_func = usbredir_read;
> @@ -2239,7 +2243,10 @@ static int usbredir_put_parser(QEMUFile *f, void *priv, size_t unused,
>      }
>
>      usbredirparser_serialize(dev->parser, &data, &len);
> -    qemu_oom_check(data);
> +    if (!data) {
> +        error_report("usbredirparser_serialize failed");
> +        exit(1);
> +    }
>
>      qemu_put_be32(f, len);
>      qemu_put_buffer(f, data, len);
> @@ -2330,7 +2337,11 @@ static int usbredir_get_bufpq(QEMUFile *f, void *priv, size_t unused,
>          bufp->len = qemu_get_be32(f);
>          bufp->status = qemu_get_be32(f);
>          bufp->offset = 0;
> -        bufp->data = qemu_oom_check(malloc(bufp->len)); /* regular malloc! */
> +        bufp->data = malloc(bufp->len); /* regular malloc! */
> +        if (!bufp->data) {
> +            error_report("usbredir_get_bufpq: out of memory");
> +            exit(1);
> +        }
>          bufp->free_on_destroy = bufp->data;
>          qemu_get_buffer(f, bufp->data, bufp->len);
>          QTAILQ_INSERT_TAIL(&endp->bufpq, bufp, next);
> --
> 2.25.1

Re: [PATCH 1/9] hw/usb/redirect.c: Stop using qemu_oom_check()

[Richard Henderson]

On 2/26/22 08:07, Peter Maydell wrote:
> qemu_oom_check() is a function which essentially says "if you pass me
> a NULL pointer then print a message then abort()".  On POSIX systems
> the message includes strerror(errno); on Windows it includes the
> GetLastError() error value printed as an integer.
> 
> Other than in the implementation of qemu_memalign(), we use this
> function only in hw/usb/redirect.c, for three checks:
> 
>   * on a call to usbredirparser_create()
>   * on a call to usberedirparser_serialize()
>   * on a call to malloc()
> 
> The usbredir library API functions make no guarantees that they will
> set errno on errors, let alone that they might set the
> Windows-specific GetLastError string.  malloc() is documented as
> setting errno, not GetLastError -- and in any case the only thing it
> might set errno to is ENOMEM.  So qemu_oom_check() isn't the right
> thing for any of these.  Replace them with straightforward
> error-checking code.  This will allow us to get rid of
> qemu_oom_check().
> 
> Signed-off-by: Peter Maydell<peter.maydell@linaro.org>
> ---
> I have left all of these errors as fatal, since that's what they
> were previously. Possibly somebody with a better understanding
> of the usbredir code might be able to make them theoretically
> non-fatal, but we make malloc failures generally fatal anyway.
> ---
>   hw/usb/redirect.c | 17 ++++++++++++++---
>   1 file changed, 14 insertions(+), 3 deletions(-)

Reviewed-by: Richard Henderson <richard.henderson@linaro.org>

r~

Re: [PATCH 2/9] util: Make qemu_oom_check() a static function

[Richard Henderson]

On 2/26/22 08:07, Peter Maydell wrote:
> The qemu_oom_check() function, which we define in both oslib-posix.c
> and oslib-win32.c, is now used only locally in that file; make it
> static.
> 
> Signed-off-by: Peter Maydell<peter.maydell@linaro.org>
> ---
>   include/qemu-common.h | 2 --
>   util/oslib-posix.c    | 2 +-
>   util/oslib-win32.c    | 2 +-
>   3 files changed, 2 insertions(+), 4 deletions(-)

Reviewed-by: Richard Henderson <richard.henderson@linaro.org>

r~

Re: [PATCH 3/9] util: Unify implementations of qemu_memalign()

[Richard Henderson]

On 2/26/22 08:07, Peter Maydell wrote:
> We implement qemu_memalign() in both oslib-posix.c and oslib-win32.c,
> but the two versions are essentially the same: they call
> qemu_try_memalign(), and abort() after printing an error message if
> it fails.  The only difference is that the win32 version prints the
> GetLastError() value whereas the POSIX version prints
> strerror(errno).  However, this is a bug in the win32 version: in
> commit dfbd0b873a85021 in 2020 we changed the implementation of
> qemu_try_memalign() from using VirtualAlloc() (which sets the
> GetLastError() value) to using _aligned_malloc() (which sets errno),
> but didn't update the error message to match.
> 
> Replace the two separate functions with a single version in a
> new memalign.c file, which drops the unnecessary extra qemu_oom_check()
> function and instead prints a more useful message including the
> requested size and alignment as well as the errno string.
> 
> Signed-off-by: Peter Maydell<peter.maydell@linaro.org>
> ---
>   util/memalign.c    | 39 +++++++++++++++++++++++++++++++++++++++
>   util/oslib-posix.c | 14 --------------
>   util/oslib-win32.c | 14 --------------
>   util/meson.build   |  1 +
>   4 files changed, 40 insertions(+), 28 deletions(-)
>   create mode 100644 util/memalign.c

Reviewed-by: Richard Henderson <richard.henderson@linaro.org>

r~

Re: [PATCH 4/9] util/oslib-win32: Return NULL on qemu_try_memalign() with zero size

[Richard Henderson]

On 2/26/22 08:07, Peter Maydell wrote:
> Currently if qemu_try_memalign() is asked to allocate 0 bytes, we assert.
> Instead return NULL; this is in line with the posix_memalign() API,
> and is valid to pass to _aligned_free() (which will do nothing).
> 
> This change is a preparation for sharing the qemu_try_memalign()
> code between Windows and POSIX -- at the moment only the Windows
> version has the assert that size != 0.
> 
> Signed-off-by: Peter Maydell<peter.maydell@linaro.org>
> ---
>   util/oslib-win32.c | 7 +++++--
>   1 file changed, 5 insertions(+), 2 deletions(-)

That would be my fault, I believe.

Reviewed-by: Richard Henderson <richard.henderson@linaro.org>

r~

Re: [PATCH 5/9] meson.build: Don't misdetect posix_memalign() on Windows

[Richard Henderson]

On 2/26/22 08:07, Peter Maydell wrote:
> Currently we incorrectly think that posix_memalign() exists on
> Windows.  This is because of a combination of:
> 
>   * the msys2/mingw toolchain/libc claim to have a
>     __builtin_posix_memalign when there isn't a builtin of that name
>   * meson will assume that if you have a __builtin_foo that
>     counts for has_function('foo')
> 
> Specifying a specific include file via prefix: causes meson to not
> treat builtins as sufficient and actually look for the function
> itself; see this meson pull request which added that as the official
> way to get the right answer:
>    https://github.com/mesonbuild/meson/pull/1150
> 
> Currently this misdectection doesn't cause problems because we only
> use CONFIG_POSIX_MEMALIGN in oslib-posix.c; however that will change
> in a following commit.
> 
> Signed-off-by: Peter Maydell<peter.maydell@linaro.org>
> ---
>   meson.build | 4 +++-
>   1 file changed, 3 insertions(+), 1 deletion(-)

Reviewed-by: Richard Henderson <richard.henderson@linaro.org>

r~

Re: [PATCH 4/9] util/oslib-win32: Return NULL on qemu_try_memalign() with zero size

[Richard Henderson]

On 2/26/22 08:07, Peter Maydell wrote:
> Currently if qemu_try_memalign() is asked to allocate 0 bytes, we assert.
> Instead return NULL; this is in line with the posix_memalign() API,
> and is valid to pass to _aligned_free() (which will do nothing).
> 
> This change is a preparation for sharing the qemu_try_memalign()
> code between Windows and POSIX -- at the moment only the Windows
> version has the assert that size != 0.
> 
> Signed-off-by: Peter Maydell <peter.maydell@linaro.org>
> ---
>   util/oslib-win32.c | 7 +++++--
>   1 file changed, 5 insertions(+), 2 deletions(-)
> 
> diff --git a/util/oslib-win32.c b/util/oslib-win32.c
> index 05857414695..8c1c64719d7 100644
> --- a/util/oslib-win32.c
> +++ b/util/oslib-win32.c
> @@ -48,13 +48,16 @@ void *qemu_try_memalign(size_t alignment, size_t size)
>   {
>       void *ptr;
>   
> -    g_assert(size != 0);
>       if (alignment < sizeof(void *)) {
>           alignment = sizeof(void *);
>       } else {
>           g_assert(is_power_of_2(alignment));
>       }
> -    ptr = _aligned_malloc(size, alignment);
> +    if (size) {
> +        ptr = _aligned_malloc(size, alignment);
> +    } else {
> +        ptr = NULL;
> +    }

Oh, should we set errno to something here?
Otherwise a random value will be used by qemu_memalign.


r~

>       trace_qemu_memalign(alignment, size, ptr);
>       return ptr;
>   }

Re: [PATCH 6/9] util: Share qemu_try_memalign() implementation between POSIX and Windows

[Richard Henderson]

On 2/26/22 08:07, Peter Maydell wrote:
> The qemu_try_memalign() functions for POSIX and Windows used to be
> significantly different, but these days they are identical except for
> the actual allocation function called, and the POSIX version already
> has to have ifdeffery for different allocation functions.
> 
> Move to a single implementation in memalign.c, which uses the Windows
> _aligned_malloc if we detect that function in meson.
> 
> Signed-off-by: Peter Maydell<peter.maydell@linaro.org>
> ---
>   meson.build        |  1 +
>   util/memalign.c    | 33 +++++++++++++++++++++++++++++++++
>   util/oslib-posix.c | 26 --------------------------
>   util/oslib-win32.c | 18 ------------------
>   4 files changed, 34 insertions(+), 44 deletions(-)

Reviewed-by: Richard Henderson <richard.henderson@linaro.org>

r~

Re: [PATCH 7/9] util: Use meson checks for valloc() and memalign() presence

[Richard Henderson]

On 2/26/22 08:07, Peter Maydell wrote:
> Instead of assuming that all CONFIG_BSD have valloc() and anything
> else is memalign(), explicitly check for those functions in
> meson.build and use the "is the function present" define.  Tests for
> specific functionality are better than which-OS checks; this also
> lets us give a helpful error message if somehow there's no usable
> function present.
> 
> Signed-off-by: Peter Maydell<peter.maydell@linaro.org>
> ---
>   meson.build     | 2 ++
>   util/memalign.c | 8 ++++++--
>   2 files changed, 8 insertions(+), 2 deletions(-)

Reviewed-by: Richard Henderson <richard.henderson@linaro.org>

r~

Re: [PATCH 8/9] util: Put qemu_vfree() in memalign.c

[Richard Henderson]

On 2/26/22 08:07, Peter Maydell wrote:
> qemu_vfree() is the companion free function to qemu_memalign(); put
> it in memalign.c so the allocation and free functions are together.
> 
> Signed-off-by: Peter Maydell<peter.maydell@linaro.org>
> ---
>   util/memalign.c    | 11 +++++++++++
>   util/oslib-posix.c |  6 ------
>   util/oslib-win32.c |  6 ------
>   3 files changed, 11 insertions(+), 12 deletions(-)

Reviewed-by: Richard Henderson <richard.henderson@linaro.org>

r~

Re: [PATCH 9/9] osdep: Move memalign-related functions to their own header

[Richard Henderson]

On 2/26/22 08:07, Peter Maydell wrote:
> Move the various memalign-related functions out of osdep.h and into
> their own header, which we include only where they are used.
> While we're doing this, add some brief documentation comments.
> 
> Signed-off-by: Peter Maydell<peter.maydell@linaro.org>
> ---

Reviewed-by: Richard Henderson <richard.henderson@linaro.org>

r~

Re: [PATCH 4/9] util/oslib-win32: Return NULL on qemu_try_memalign() with zero size

[Peter Maydell]

On Sun, 27 Feb 2022 at 00:56, Richard Henderson
<richard.henderson@linaro.org> wrote:
>
> On 2/26/22 08:07, Peter Maydell wrote:
> > Currently if qemu_try_memalign() is asked to allocate 0 bytes, we assert.
> > Instead return NULL; this is in line with the posix_memalign() API,
> > and is valid to pass to _aligned_free() (which will do nothing).
> >
> > This change is a preparation for sharing the qemu_try_memalign()
> > code between Windows and POSIX -- at the moment only the Windows
> > version has the assert that size != 0.
> >
> > Signed-off-by: Peter Maydell <peter.maydell@linaro.org>
> > ---
> >   util/oslib-win32.c | 7 +++++--
> >   1 file changed, 5 insertions(+), 2 deletions(-)
> >
> > diff --git a/util/oslib-win32.c b/util/oslib-win32.c
> > index 05857414695..8c1c64719d7 100644
> > --- a/util/oslib-win32.c
> > +++ b/util/oslib-win32.c
> > @@ -48,13 +48,16 @@ void *qemu_try_memalign(size_t alignment, size_t size)
> >   {
> >       void *ptr;
> >
> > -    g_assert(size != 0);
> >       if (alignment < sizeof(void *)) {
> >           alignment = sizeof(void *);
> >       } else {
> >           g_assert(is_power_of_2(alignment));
> >       }
> > -    ptr = _aligned_malloc(size, alignment);
> > +    if (size) {
> > +        ptr = _aligned_malloc(size, alignment);
> > +    } else {
> > +        ptr = NULL;
> > +    }
>
> Oh, should we set errno to something here?
> Otherwise a random value will be used by qemu_memalign.

Yeah, I guess so, though the errno to use isn't obvious. Maybe EINVAL?

The alternative would be to try to audit all the callsites to
confirm they don't ever try to allocate 0 bytes and then have
the assert for both Windows and POSIX versions...

-- PMM

Re: [PATCH 4/9] util/oslib-win32: Return NULL on qemu_try_memalign() with zero size

[Richard Henderson]

On 2/27/22 02:54, Peter Maydell wrote:
>>> +    if (size) {
>>> +        ptr = _aligned_malloc(size, alignment);
>>> +    } else {
>>> +        ptr = NULL;
>>> +    }
>>
>> Oh, should we set errno to something here?
>> Otherwise a random value will be used by qemu_memalign.
> 
> Yeah, I guess so, though the errno to use isn't obvious. Maybe EINVAL?
> 
> The alternative would be to try to audit all the callsites to
> confirm they don't ever try to allocate 0 bytes and then have
> the assert for both Windows and POSIX versions...

Alternately, force size == 1, so that we always get a non-NULL value that can be freed. 
That's a change on the POSIX side as well, of course.


r~

Re: [PATCH 7/9] util: Use meson checks for valloc() and memalign() presence

[Philippe Mathieu-Daudé]

On 26/2/22 19:07, Peter Maydell wrote:
> Instead of assuming that all CONFIG_BSD have valloc() and anything
> else is memalign(), explicitly check for those functions in
> meson.build and use the "is the function present" define.  Tests for
> specific functionality are better than which-OS checks; this also
> lets us give a helpful error message if somehow there's no usable
> function present.
> 
> Signed-off-by: Peter Maydell <peter.maydell@linaro.org>
> ---
>   meson.build     | 2 ++
>   util/memalign.c | 8 ++++++--
>   2 files changed, 8 insertions(+), 2 deletions(-)

Reviewed-by: Philippe Mathieu-Daudé <f4bug@amsat.org>

Re: [PATCH 9/9] osdep: Move memalign-related functions to their own header

[Philippe Mathieu-Daudé]

On 26/2/22 19:07, Peter Maydell wrote:
> Move the various memalign-related functions out of osdep.h and into
> their own header, which we include only where they are used.
> While we're doing this, add some brief documentation comments.
> 
> Signed-off-by: Peter Maydell <peter.maydell@linaro.org>
> ---
>   include/qemu/memalign.h        | 61 ++++++++++++++++++++++++++++++++++
>   include/qemu/osdep.h           | 18 ----------
>   block/blkverify.c              |  1 +
>   block/block-copy.c             |  1 +
>   block/commit.c                 |  1 +
>   block/crypto.c                 |  1 +
>   block/dmg.c                    |  1 +
>   block/export/fuse.c            |  1 +
>   block/file-posix.c             |  1 +
>   block/io.c                     |  1 +
>   block/mirror.c                 |  1 +
>   block/nvme.c                   |  1 +
>   block/parallels-ext.c          |  1 +
>   block/parallels.c              |  1 +
>   block/qcow.c                   |  1 +
>   block/qcow2-cache.c            |  1 +
>   block/qcow2-cluster.c          |  1 +
>   block/qcow2-refcount.c         |  1 +
>   block/qcow2-snapshot.c         |  1 +
>   block/qcow2.c                  |  1 +
>   block/qed-l2-cache.c           |  1 +
>   block/qed-table.c              |  1 +
>   block/qed.c                    |  1 +
>   block/quorum.c                 |  1 +
>   block/raw-format.c             |  1 +
>   block/vdi.c                    |  1 +
>   block/vhdx-log.c               |  1 +
>   block/vhdx.c                   |  1 +
>   block/vmdk.c                   |  1 +
>   block/vpc.c                    |  1 +
>   block/win32-aio.c              |  1 +
>   hw/block/dataplane/xen-block.c |  1 +
>   hw/block/fdc.c                 |  1 +
>   hw/ide/core.c                  |  1 +
>   hw/ppc/spapr.c                 |  1 +
>   hw/ppc/spapr_softmmu.c         |  1 +
>   hw/scsi/scsi-disk.c            |  1 +
>   hw/tpm/tpm_ppi.c               |  2 +-
>   nbd/server.c                   |  1 +
>   net/l2tpv3.c                   |  2 +-
>   plugins/loader.c               |  1 +
>   qemu-img.c                     |  1 +
>   qemu-io-cmds.c                 |  1 +
>   qom/object.c                   |  1 +
>   softmmu/physmem.c              |  1 +
>   target/i386/hvf/hvf.c          |  1 +
>   target/i386/kvm/kvm.c          |  1 +
>   tcg/region.c                   |  1 +
>   tests/bench/atomic_add-bench.c |  1 +
>   tests/bench/qht-bench.c        |  1 +
>   util/atomic64.c                |  1 +
>   util/memalign.c                |  1 +
>   util/qht.c                     |  1 +
>   53 files changed, 112 insertions(+), 20 deletions(-)
>   create mode 100644 include/qemu/memalign.h

Reviewed-by: Philippe Mathieu-Daudé <f4bug@amsat.org>

Re: [PATCH 1/9] hw/usb/redirect.c: Stop using qemu_oom_check()

[Philippe Mathieu-Daudé]

On 26/2/22 19:07, Peter Maydell wrote:
> qemu_oom_check() is a function which essentially says "if you pass me
> a NULL pointer then print a message then abort()".  On POSIX systems
> the message includes strerror(errno); on Windows it includes the
> GetLastError() error value printed as an integer.
> 
> Other than in the implementation of qemu_memalign(), we use this
> function only in hw/usb/redirect.c, for three checks:
> 
>   * on a call to usbredirparser_create()
>   * on a call to usberedirparser_serialize()
>   * on a call to malloc()
> 
> The usbredir library API functions make no guarantees that they will
> set errno on errors, let alone that they might set the
> Windows-specific GetLastError string.  malloc() is documented as
> setting errno, not GetLastError -- and in any case the only thing it
> might set errno to is ENOMEM.  So qemu_oom_check() isn't the right
> thing for any of these.  Replace them with straightforward
> error-checking code.  This will allow us to get rid of
> qemu_oom_check().
> 
> Signed-off-by: Peter Maydell <peter.maydell@linaro.org>
> ---
> I have left all of these errors as fatal, since that's what they
> were previously. Possibly somebody with a better understanding
> of the usbredir code might be able to make them theoretically
> non-fatal, but we make malloc failures generally fatal anyway.
> ---
>   hw/usb/redirect.c | 17 ++++++++++++++---
>   1 file changed, 14 insertions(+), 3 deletions(-)

Reviewed-by: Philippe Mathieu-Daudé <f4bug@amsat.org>

Re: [PATCH 2/9] util: Make qemu_oom_check() a static function

[Philippe Mathieu-Daudé]

On 26/2/22 19:07, Peter Maydell wrote:
> The qemu_oom_check() function, which we define in both oslib-posix.c
> and oslib-win32.c, is now used only locally in that file; make it
> static.
> 
> Signed-off-by: Peter Maydell <peter.maydell@linaro.org>
> ---
>   include/qemu-common.h | 2 --
>   util/oslib-posix.c    | 2 +-
>   util/oslib-win32.c    | 2 +-
>   3 files changed, 2 insertions(+), 4 deletions(-)

Reviewed-by: Philippe Mathieu-Daudé <f4bug@amsat.org>

Re: [PATCH 1/9] hw/usb/redirect.c: Stop using qemu_oom_check()

[Eric Blake]

On Sat, Feb 26, 2022 at 06:07:15PM +0000, Peter Maydell wrote:
> qemu_oom_check() is a function which essentially says "if you pass me
> a NULL pointer then print a message then abort()".  On POSIX systems
> the message includes strerror(errno); on Windows it includes the
> GetLastError() error value printed as an integer.
> 
> Other than in the implementation of qemu_memalign(), we use this
> function only in hw/usb/redirect.c, for three checks:
> 
>  * on a call to usbredirparser_create()
>  * on a call to usberedirparser_serialize()
>  * on a call to malloc()
> 
> The usbredir library API functions make no guarantees that they will
> set errno on errors, let alone that they might set the
> Windows-specific GetLastError string.  malloc() is documented as
> setting errno, not GetLastError -- and in any case the only thing it
> might set errno to is ENOMEM.  So qemu_oom_check() isn't the right
> thing for any of these.  Replace them with straightforward
> error-checking code.  This will allow us to get rid of
> qemu_oom_check().
> 
> Signed-off-by: Peter Maydell <peter.maydell@linaro.org>
> ---
> I have left all of these errors as fatal, since that's what they
> were previously. Possibly somebody with a better understanding
> of the usbredir code might be able to make them theoretically
> non-fatal, but we make malloc failures generally fatal anyway.
> ---
>  hw/usb/redirect.c | 17 ++++++++++++++---
>  1 file changed, 14 insertions(+), 3 deletions(-)
> 
> diff --git a/hw/usb/redirect.c b/hw/usb/redirect.c
> index 5f0ef9cb3b0..8692ea25610 100644
> --- a/hw/usb/redirect.c
> +++ b/hw/usb/redirect.c
> @@ -1239,7 +1239,11 @@ static void usbredir_create_parser(USBRedirDevice *dev)
>  
>      DPRINTF("creating usbredirparser\n");
>  
> -    dev->parser = qemu_oom_check(usbredirparser_create());
> +    dev->parser = usbredirparser_create();
> +    if (!dev->parser) {
> +        error_report("usbredirparser_create() failed");
> +        exit(1);

Is exit(EXIT_FAILURE) worth using in this file?  We have an
inconsistent history of a magic number vs. a named constant, so either
way,

Reviewed-by: Eric Blake <eblake@redhat.com>

-- 
Eric Blake, Principal Software Engineer
Red Hat, Inc.           +1-919-301-3266
Virtualization:  qemu.org | libvirt.org

Re: [PATCH 1/9] hw/usb/redirect.c: Stop using qemu_oom_check()

[Peter Maydell]

On Wed, 2 Mar 2022 at 16:31, Eric Blake <eblake@redhat.com> wrote:
> Is exit(EXIT_FAILURE) worth using in this file?  We have an
> inconsistent history of a magic number vs. a named constant, so either
> way,

I'm not a huge fan of EXIT_FAILURE, I think it tends to
obscure more than it helps. We have rather more 'exit(1)'
than 'exit(EXIT_FAILURE)' in the codebase.

-- PMM

Re: [PATCH 4/9] util/oslib-win32: Return NULL on qemu_try_memalign() with zero size

[Peter Maydell]

On Sun, 27 Feb 2022 at 18:36, Richard Henderson
<richard.henderson@linaro.org> wrote:
>
> On 2/27/22 02:54, Peter Maydell wrote:
> >>> +    if (size) {
> >>> +        ptr = _aligned_malloc(size, alignment);
> >>> +    } else {
> >>> +        ptr = NULL;
> >>> +    }
> >>
> >> Oh, should we set errno to something here?
> >> Otherwise a random value will be used by qemu_memalign.
> >
> > Yeah, I guess so, though the errno to use isn't obvious. Maybe EINVAL?
> >
> > The alternative would be to try to audit all the callsites to
> > confirm they don't ever try to allocate 0 bytes and then have
> > the assert for both Windows and POSIX versions...
>
> Alternately, force size == 1, so that we always get a non-NULL value that can be freed.
> That's a change on the POSIX side as well, of course.

Yes, I had a look at what actual malloc() implementations tend
to do, and the answer seems to be that forcing size to 1 gives
less weird behaviour for the application. So here that would be

   if (size == 0) {
       size++;
   }
   ptr = _aligned_malloc(size, alignment);

We don't need to do anything on the POSIX side (unless we want to
enforce consistency of handling the size==0 case).

I'd quite like to get this series in before softfreeze (though mostly
just for my personal convenience so it's not hanging around as a
loose end I have to come back to after we reopen for 7.1). Does anybody
object if I squash in that change and put this in a pullrequest,
or would you prefer to see a v2 series first?

thanks
-- PMM

Re: [PATCH 4/9] util/oslib-win32: Return NULL on qemu_try_memalign() with zero size

[Richard Henderson]

On 3/3/22 06:55, Peter Maydell wrote:
>> Alternately, force size == 1, so that we always get a non-NULL value that can be freed.
>> That's a change on the POSIX side as well, of course.
> 
> Yes, I had a look at what actual malloc() implementations tend
> to do, and the answer seems to be that forcing size to 1 gives
> less weird behaviour for the application. So here that would be
> 
>     if (size == 0) {
>         size++;
>     }
>     ptr = _aligned_malloc(size, alignment);
> 
> We don't need to do anything on the POSIX side (unless we want to
> enforce consistency of handling the size==0 case).

I would do this unconditionally.  The POSIX manpage says that either NULL or a unique 
pointer is a valid return value into *memptr here for size == 0.  What we want in our 
caller is NULL if and only if error.

> I'd quite like to get this series in before softfreeze (though mostly
> just for my personal convenience so it's not hanging around as a
> loose end I have to come back to after we reopen for 7.1). Does anybody
> object if I squash in that change and put this in a pullrequest,
> or would you prefer to see a v2 series first?

I'm happy with a squash and PR.


r~

Re: [PATCH 4/9] util/oslib-win32: Return NULL on qemu_try_memalign() with zero size

[Peter Maydell]

On Thu, 3 Mar 2022 at 23:02, Richard Henderson
<richard.henderson@linaro.org> wrote:
>
> On 3/3/22 06:55, Peter Maydell wrote:
> >> Alternately, force size == 1, so that we always get a non-NULL value that can be freed.
> >> That's a change on the POSIX side as well, of course.
> >
> > Yes, I had a look at what actual malloc() implementations tend
> > to do, and the answer seems to be that forcing size to 1 gives
> > less weird behaviour for the application. So here that would be
> >
> >     if (size == 0) {
> >         size++;
> >     }
> >     ptr = _aligned_malloc(size, alignment);
> >
> > We don't need to do anything on the POSIX side (unless we want to
> > enforce consistency of handling the size==0 case).
>
> I would do this unconditionally.  The POSIX manpage says that either NULL or a unique
> pointer is a valid return value into *memptr here for size == 0.  What we want in our
> caller is NULL if and only if error.

Mm, I guess. I was trying to avoid changing the POSIX-side behaviour,
but this seems safe enough.

-- PMM