[PULL v2 00/14] Mem next patches

[PULL v2 00/14] Mem next patches

[Peter Xu]

The following changes since commit ffaf7f0376f8040ce9068d71ae9ae8722505c42e:

  Merge tag 'pull-10.0-testing-and-gdstub-updates-100225-1' of https://gitlab.com/stsquad/qemu into staging (2025-02-10 13:26:17 -0500)

are available in the Git repository at:

  https://gitlab.com/peterx/qemu.git tags/mem-next-pull-request

for you to fetch changes up to 13057e064a3edae7abf9ca2c207cdf48b82c5aad:

  overcommit: introduce mem-lock=on-fault (2025-02-12 11:36:13 -0500)

----------------------------------------------------------------
Memory pull request for 10.0

v2 changelog:

- Fix Mac (and possibly some other) build issues for two patches
  - os: add an ability to lock memory on_fault
  - memory: pass MemTxAttrs to memory_access_is_direct()

List of features:

- William's fix on ram hole punching when with file offset
- Daniil's patchset to introduce mem-lock=on-fault
- William's hugetlb hwpoison fix for size report & remap
- David's series to allow qemu debug writes to MMIOs

----------------------------------------------------------------

Daniil Tatianin (4):
  os: add an ability to lock memory on_fault
  system/vl: extract overcommit option parsing into a helper
  system: introduce a new MlockState enum
  overcommit: introduce mem-lock=on-fault

David Hildenbrand (7):
  physmem: factor out memory_region_is_ram_device() check in
    memory_access_is_direct()
  physmem: factor out RAM/ROMD check in memory_access_is_direct()
  physmem: factor out direct access check into
    memory_region_supports_direct_access()
  physmem: disallow direct access to RAM DEVICE in
    address_space_write_rom()
  memory: pass MemTxAttrs to memory_access_is_direct()
  hmp: use cpu_get_phys_page_debug() in hmp_gva2gpa()
  physmem: teach cpu_memory_rw_debug() to write to more memory regions

William Roche (3):
  system/physmem: take into account fd_offset for file fallocate
  system/physmem: handle hugetlb correctly in qemu_ram_remap()
  system/physmem: poisoned memory discard on reboot

 meson.build               |   6 ++
 include/exec/cpu-common.h |   2 +-
 include/exec/memattrs.h   |   5 +-
 include/exec/memory.h     |  35 ++++++++---
 include/system/os-posix.h |   2 +-
 include/system/os-win32.h |   2 +-
 include/system/system.h   |  12 +++-
 accel/kvm/kvm-all.c       |   2 +-
 hw/core/cpu-system.c      |  13 ++--
 hw/core/loader.c          |   2 +-
 hw/remote/vfio-user-obj.c |   2 +-
 hw/virtio/virtio-mem.c    |   2 +-
 migration/postcopy-ram.c  |   4 +-
 monitor/hmp-cmds-target.c |   3 +-
 os-posix.c                |  15 ++++-
 system/globals.c          |  12 +++-
 system/physmem.c          | 121 ++++++++++++++++++++++++--------------
 system/vl.c               |  52 ++++++++++++----
 system/memory_ldst.c.inc  |  18 +++---
 hw/display/apple-gfx.m    |   3 +-
 qemu-options.hx           |  14 +++--
 21 files changed, 229 insertions(+), 98 deletions(-)

-- 
2.47.0

[PULL v2 06/14] memory: pass MemTxAttrs to memory_access_is_direct()

[Peter Xu]

From: David Hildenbrand <david@redhat.com>

We want to pass another flag that will be stored in MemTxAttrs. So pass
MemTxAttrs directly.

Reviewed-by: Peter Xu <peterx@redhat.com>
Reviewed-by: Philippe Mathieu-Daudé <philmd@linaro.org>
Signed-off-by: David Hildenbrand <david@redhat.com>
Link: https://lore.kernel.org/r/20250210084648.33798-6-david@redhat.com
[peterx: Fix MacOS builds]
Signed-off-by: Peter Xu <peterx@redhat.com>
---
 include/exec/memory.h     |  5 +++--
 hw/core/loader.c          |  2 +-
 hw/remote/vfio-user-obj.c |  2 +-
 system/physmem.c          | 12 ++++++------
 system/memory_ldst.c.inc  | 18 +++++++++---------
 hw/display/apple-gfx.m    |  3 ++-
 6 files changed, 22 insertions(+), 20 deletions(-)

diff --git a/include/exec/memory.h b/include/exec/memory.h
index 4e2cf95ab6..b18ecf933e 100644
--- a/include/exec/memory.h
+++ b/include/exec/memory.h
@@ -3012,7 +3012,8 @@ static inline bool memory_region_supports_direct_access(MemoryRegion *mr)
     return !memory_region_is_ram_device(mr);
 }
 
-static inline bool memory_access_is_direct(MemoryRegion *mr, bool is_write)
+static inline bool memory_access_is_direct(MemoryRegion *mr, bool is_write,
+                                           MemTxAttrs attrs)
 {
     if (!memory_region_supports_direct_access(mr)) {
         return false;
@@ -3053,7 +3054,7 @@ MemTxResult address_space_read(AddressSpace *as, hwaddr addr,
             fv = address_space_to_flatview(as);
             l = len;
             mr = flatview_translate(fv, addr, &addr1, &l, false, attrs);
-            if (len == l && memory_access_is_direct(mr, false)) {
+            if (len == l && memory_access_is_direct(mr, false, attrs)) {
                 ptr = qemu_map_ram_ptr(mr->ram_block, addr1);
                 memcpy(buf, ptr, len);
             } else {
diff --git a/hw/core/loader.c b/hw/core/loader.c
index fd25c5e01b..332b879a0b 100644
--- a/hw/core/loader.c
+++ b/hw/core/loader.c
@@ -144,7 +144,7 @@ ssize_t load_image_mr(const char *filename, MemoryRegion *mr)
 {
     ssize_t size;
 
-    if (!memory_access_is_direct(mr, false)) {
+    if (!memory_access_is_direct(mr, false, MEMTXATTRS_UNSPECIFIED)) {
         /* Can only load an image into RAM or ROM */
         return -1;
     }
diff --git a/hw/remote/vfio-user-obj.c b/hw/remote/vfio-user-obj.c
index 9e5ff6d87a..6e51a92856 100644
--- a/hw/remote/vfio-user-obj.c
+++ b/hw/remote/vfio-user-obj.c
@@ -358,7 +358,7 @@ static int vfu_object_mr_rw(MemoryRegion *mr, uint8_t *buf, hwaddr offset,
     int access_size;
     uint64_t val;
 
-    if (memory_access_is_direct(mr, is_write)) {
+    if (memory_access_is_direct(mr, is_write, MEMTXATTRS_UNSPECIFIED)) {
         /**
          * Some devices expose a PCI expansion ROM, which could be buffer
          * based as compared to other regions which are primarily based on
diff --git a/system/physmem.c b/system/physmem.c
index cff15ca1df..8745c10c9d 100644
--- a/system/physmem.c
+++ b/system/physmem.c
@@ -573,7 +573,7 @@ MemoryRegion *flatview_translate(FlatView *fv, hwaddr addr, hwaddr *xlat,
                                     is_write, true, &as, attrs);
     mr = section.mr;
 
-    if (xen_enabled() && memory_access_is_direct(mr, is_write)) {
+    if (xen_enabled() && memory_access_is_direct(mr, is_write, attrs)) {
         hwaddr page = ((addr & TARGET_PAGE_MASK) + TARGET_PAGE_SIZE) - addr;
         *plen = MIN(page, *plen);
     }
@@ -2869,7 +2869,7 @@ static MemTxResult flatview_write_continue_step(MemTxAttrs attrs,
         return MEMTX_ACCESS_ERROR;
     }
 
-    if (!memory_access_is_direct(mr, true)) {
+    if (!memory_access_is_direct(mr, true, attrs)) {
         uint64_t val;
         MemTxResult result;
         bool release_lock = prepare_mmio_access(mr);
@@ -2965,7 +2965,7 @@ static MemTxResult flatview_read_continue_step(MemTxAttrs attrs, uint8_t *buf,
         return MEMTX_ACCESS_ERROR;
     }
 
-    if (!memory_access_is_direct(mr, false)) {
+    if (!memory_access_is_direct(mr, false, attrs)) {
         /* I/O case */
         uint64_t val;
         MemTxResult result;
@@ -3274,7 +3274,7 @@ static bool flatview_access_valid(FlatView *fv, hwaddr addr, hwaddr len,
     while (len > 0) {
         l = len;
         mr = flatview_translate(fv, addr, &xlat, &l, is_write, attrs);
-        if (!memory_access_is_direct(mr, is_write)) {
+        if (!memory_access_is_direct(mr, is_write, attrs)) {
             l = memory_access_size(mr, l, addr);
             if (!memory_region_access_valid(mr, xlat, l, is_write, attrs)) {
                 return false;
@@ -3354,7 +3354,7 @@ void *address_space_map(AddressSpace *as,
     fv = address_space_to_flatview(as);
     mr = flatview_translate(fv, addr, &xlat, &l, is_write, attrs);
 
-    if (!memory_access_is_direct(mr, is_write)) {
+    if (!memory_access_is_direct(mr, is_write, attrs)) {
         size_t used = qatomic_read(&as->bounce_buffer_size);
         for (;;) {
             hwaddr alloc = MIN(as->max_bounce_buffer_size - used, l);
@@ -3487,7 +3487,7 @@ int64_t address_space_cache_init(MemoryRegionCache *cache,
 
     mr = cache->mrs.mr;
     memory_region_ref(mr);
-    if (memory_access_is_direct(mr, is_write)) {
+    if (memory_access_is_direct(mr, is_write, MEMTXATTRS_UNSPECIFIED)) {
         /* We don't care about the memory attributes here as we're only
          * doing this if we found actual RAM, which behaves the same
          * regardless of attributes; so UNSPECIFIED is fine.
diff --git a/system/memory_ldst.c.inc b/system/memory_ldst.c.inc
index 0e6f3940a9..7f32d3d9ff 100644
--- a/system/memory_ldst.c.inc
+++ b/system/memory_ldst.c.inc
@@ -34,7 +34,7 @@ static inline uint32_t glue(address_space_ldl_internal, SUFFIX)(ARG1_DECL,
 
     RCU_READ_LOCK();
     mr = TRANSLATE(addr, &addr1, &l, false, attrs);
-    if (l < 4 || !memory_access_is_direct(mr, false)) {
+    if (l < 4 || !memory_access_is_direct(mr, false, attrs)) {
         release_lock |= prepare_mmio_access(mr);
 
         /* I/O case */
@@ -103,7 +103,7 @@ static inline uint64_t glue(address_space_ldq_internal, SUFFIX)(ARG1_DECL,
 
     RCU_READ_LOCK();
     mr = TRANSLATE(addr, &addr1, &l, false, attrs);
-    if (l < 8 || !memory_access_is_direct(mr, false)) {
+    if (l < 8 || !memory_access_is_direct(mr, false, attrs)) {
         release_lock |= prepare_mmio_access(mr);
 
         /* I/O case */
@@ -170,7 +170,7 @@ uint8_t glue(address_space_ldub, SUFFIX)(ARG1_DECL,
 
     RCU_READ_LOCK();
     mr = TRANSLATE(addr, &addr1, &l, false, attrs);
-    if (!memory_access_is_direct(mr, false)) {
+    if (!memory_access_is_direct(mr, false, attrs)) {
         release_lock |= prepare_mmio_access(mr);
 
         /* I/O case */
@@ -207,7 +207,7 @@ static inline uint16_t glue(address_space_lduw_internal, SUFFIX)(ARG1_DECL,
 
     RCU_READ_LOCK();
     mr = TRANSLATE(addr, &addr1, &l, false, attrs);
-    if (l < 2 || !memory_access_is_direct(mr, false)) {
+    if (l < 2 || !memory_access_is_direct(mr, false, attrs)) {
         release_lock |= prepare_mmio_access(mr);
 
         /* I/O case */
@@ -277,7 +277,7 @@ void glue(address_space_stl_notdirty, SUFFIX)(ARG1_DECL,
 
     RCU_READ_LOCK();
     mr = TRANSLATE(addr, &addr1, &l, true, attrs);
-    if (l < 4 || !memory_access_is_direct(mr, true)) {
+    if (l < 4 || !memory_access_is_direct(mr, true, attrs)) {
         release_lock |= prepare_mmio_access(mr);
 
         r = memory_region_dispatch_write(mr, addr1, val, MO_32, attrs);
@@ -314,7 +314,7 @@ static inline void glue(address_space_stl_internal, SUFFIX)(ARG1_DECL,
 
     RCU_READ_LOCK();
     mr = TRANSLATE(addr, &addr1, &l, true, attrs);
-    if (l < 4 || !memory_access_is_direct(mr, true)) {
+    if (l < 4 || !memory_access_is_direct(mr, true, attrs)) {
         release_lock |= prepare_mmio_access(mr);
         r = memory_region_dispatch_write(mr, addr1, val,
                                          MO_32 | devend_memop(endian), attrs);
@@ -377,7 +377,7 @@ void glue(address_space_stb, SUFFIX)(ARG1_DECL,
 
     RCU_READ_LOCK();
     mr = TRANSLATE(addr, &addr1, &l, true, attrs);
-    if (!memory_access_is_direct(mr, true)) {
+    if (!memory_access_is_direct(mr, true, attrs)) {
         release_lock |= prepare_mmio_access(mr);
         r = memory_region_dispatch_write(mr, addr1, val, MO_8, attrs);
     } else {
@@ -410,7 +410,7 @@ static inline void glue(address_space_stw_internal, SUFFIX)(ARG1_DECL,
 
     RCU_READ_LOCK();
     mr = TRANSLATE(addr, &addr1, &l, true, attrs);
-    if (l < 2 || !memory_access_is_direct(mr, true)) {
+    if (l < 2 || !memory_access_is_direct(mr, true, attrs)) {
         release_lock |= prepare_mmio_access(mr);
         r = memory_region_dispatch_write(mr, addr1, val,
                                          MO_16 | devend_memop(endian), attrs);
@@ -474,7 +474,7 @@ static void glue(address_space_stq_internal, SUFFIX)(ARG1_DECL,
 
     RCU_READ_LOCK();
     mr = TRANSLATE(addr, &addr1, &l, true, attrs);
-    if (l < 8 || !memory_access_is_direct(mr, true)) {
+    if (l < 8 || !memory_access_is_direct(mr, true, attrs)) {
         release_lock |= prepare_mmio_access(mr);
         r = memory_region_dispatch_write(mr, addr1, val,
                                          MO_64 | devend_memop(endian), attrs);
diff --git a/hw/display/apple-gfx.m b/hw/display/apple-gfx.m
index aa1455b629..1554f3b801 100644
--- a/hw/display/apple-gfx.m
+++ b/hw/display/apple-gfx.m
@@ -137,7 +137,8 @@ static void apple_gfx_destroy_task(AppleGFXState *s, PGTask_t *task)
                                          MEMTXATTRS_UNSPECIFIED);
 
     if (!ram_region || ram_region_length < length ||
-        !memory_access_is_direct(ram_region, !read_only)) {
+        !memory_access_is_direct(ram_region, !read_only,
+				 MEMTXATTRS_UNSPECIFIED)) {
         return NULL;
     }
 
-- 
2.47.0

[PULL v2 11/14] os: add an ability to lock memory on_fault

[Peter Xu]

From: Daniil Tatianin <d-tatianin@yandex-team.ru>

This will be used in the following commits to make it possible to only
lock memory on fault instead of right away.

Signed-off-by: Daniil Tatianin <d-tatianin@yandex-team.ru>
Reviewed-by: Vladimir Sementsov-Ogievskiy <vsementsov@yandex-team.ru>
Link: https://lore.kernel.org/r/20250212143920.1269754-2-d-tatianin@yandex-team.ru
[peterx: fail os_mlock(on_fault=1) when not supported]
[peterx: use G_GNUC_UNUSED instead of "(void)on_fault", per Dan]
Signed-off-by: Peter Xu <peterx@redhat.com>
---
 meson.build               |  6 ++++++
 include/system/os-posix.h |  2 +-
 include/system/os-win32.h |  2 +-
 migration/postcopy-ram.c  |  2 +-
 os-posix.c                | 15 +++++++++++++--
 system/vl.c               |  2 +-
 6 files changed, 23 insertions(+), 6 deletions(-)

diff --git a/meson.build b/meson.build
index 18cf9e2913..59953cbe6b 100644
--- a/meson.build
+++ b/meson.build
@@ -2885,6 +2885,12 @@ config_host_data.set('HAVE_MLOCKALL', cc.links(gnu_source_prefix + '''
     return mlockall(MCL_FUTURE);
   }'''))
 
+config_host_data.set('HAVE_MLOCK_ONFAULT', cc.links(gnu_source_prefix + '''
+  #include <sys/mman.h>
+  int main(void) {
+      return mlockall(MCL_FUTURE | MCL_ONFAULT);
+  }'''))
+
 have_l2tpv3 = false
 if get_option('l2tpv3').allowed() and have_system
   have_l2tpv3 = cc.has_type('struct mmsghdr',
diff --git a/include/system/os-posix.h b/include/system/os-posix.h
index b881ac6c6f..ce5b3bccf8 100644
--- a/include/system/os-posix.h
+++ b/include/system/os-posix.h
@@ -53,7 +53,7 @@ bool os_set_runas(const char *user_id);
 void os_set_chroot(const char *path);
 void os_setup_limits(void);
 void os_setup_post(void);
-int os_mlock(void);
+int os_mlock(bool on_fault);
 
 /**
  * qemu_alloc_stack:
diff --git a/include/system/os-win32.h b/include/system/os-win32.h
index b82a5d3ad9..bc623061d8 100644
--- a/include/system/os-win32.h
+++ b/include/system/os-win32.h
@@ -123,7 +123,7 @@ static inline bool is_daemonized(void)
     return false;
 }
 
-static inline int os_mlock(void)
+static inline int os_mlock(bool on_fault G_GNUC_UNUSED)
 {
     return -ENOSYS;
 }
diff --git a/migration/postcopy-ram.c b/migration/postcopy-ram.c
index 6a6da6ba7f..fc4d8a10df 100644
--- a/migration/postcopy-ram.c
+++ b/migration/postcopy-ram.c
@@ -652,7 +652,7 @@ int postcopy_ram_incoming_cleanup(MigrationIncomingState *mis)
     }
 
     if (enable_mlock) {
-        if (os_mlock() < 0) {
+        if (os_mlock(false) < 0) {
             error_report("mlock: %s", strerror(errno));
             /*
              * It doesn't feel right to fail at this point, we have a valid
diff --git a/os-posix.c b/os-posix.c
index 9cce55ff2f..52925c23d3 100644
--- a/os-posix.c
+++ b/os-posix.c
@@ -327,18 +327,29 @@ void os_set_line_buffering(void)
     setvbuf(stdout, NULL, _IOLBF, 0);
 }
 
-int os_mlock(void)
+int os_mlock(bool on_fault)
 {
 #ifdef HAVE_MLOCKALL
     int ret = 0;
+    int flags = MCL_CURRENT | MCL_FUTURE;
 
-    ret = mlockall(MCL_CURRENT | MCL_FUTURE);
+    if (on_fault) {
+#ifdef HAVE_MLOCK_ONFAULT
+        flags |= MCL_ONFAULT;
+#else
+        error_report("mlockall: on_fault not supported");
+        return -EINVAL;
+#endif
+    }
+
+    ret = mlockall(flags);
     if (ret < 0) {
         error_report("mlockall: %s", strerror(errno));
     }
 
     return ret;
 #else
+    (void)on_fault;
     return -ENOSYS;
 #endif
 }
diff --git a/system/vl.c b/system/vl.c
index 9c6942c6cf..e94fc7ea35 100644
--- a/system/vl.c
+++ b/system/vl.c
@@ -797,7 +797,7 @@ static QemuOptsList qemu_run_with_opts = {
 static void realtime_init(void)
 {
     if (enable_mlock) {
-        if (os_mlock() < 0) {
+        if (os_mlock(false) < 0) {
             error_report("locking memory failed");
             exit(1);
         }
-- 
2.47.0

Re: [PULL v2 11/14] os: add an ability to lock memory on_fault

[Daniel P. Berrangé]

On Wed, Feb 12, 2025 at 12:38:23PM -0500, Peter Xu wrote:
> From: Daniil Tatianin <d-tatianin@yandex-team.ru>
> 
> This will be used in the following commits to make it possible to only
> lock memory on fault instead of right away.
> 
> Signed-off-by: Daniil Tatianin <d-tatianin@yandex-team.ru>
> Reviewed-by: Vladimir Sementsov-Ogievskiy <vsementsov@yandex-team.ru>
> Link: https://lore.kernel.org/r/20250212143920.1269754-2-d-tatianin@yandex-team.ru
> [peterx: fail os_mlock(on_fault=1) when not supported]
> [peterx: use G_GNUC_UNUSED instead of "(void)on_fault", per Dan]
> Signed-off-by: Peter Xu <peterx@redhat.com>
> ---
>  meson.build               |  6 ++++++
>  include/system/os-posix.h |  2 +-
>  include/system/os-win32.h |  2 +-
>  migration/postcopy-ram.c  |  2 +-
>  os-posix.c                | 15 +++++++++++++--
>  system/vl.c               |  2 +-
>  6 files changed, 23 insertions(+), 6 deletions(-)
> 
> diff --git a/meson.build b/meson.build
> index 18cf9e2913..59953cbe6b 100644
> --- a/meson.build
> +++ b/meson.build
> @@ -2885,6 +2885,12 @@ config_host_data.set('HAVE_MLOCKALL', cc.links(gnu_source_prefix + '''
>      return mlockall(MCL_FUTURE);
>    }'''))
>  
> +config_host_data.set('HAVE_MLOCK_ONFAULT', cc.links(gnu_source_prefix + '''
> +  #include <sys/mman.h>
> +  int main(void) {
> +      return mlockall(MCL_FUTURE | MCL_ONFAULT);
> +  }'''))
> +
>  have_l2tpv3 = false
>  if get_option('l2tpv3').allowed() and have_system
>    have_l2tpv3 = cc.has_type('struct mmsghdr',
> diff --git a/include/system/os-posix.h b/include/system/os-posix.h
> index b881ac6c6f..ce5b3bccf8 100644
> --- a/include/system/os-posix.h
> +++ b/include/system/os-posix.h
> @@ -53,7 +53,7 @@ bool os_set_runas(const char *user_id);
>  void os_set_chroot(const char *path);
>  void os_setup_limits(void);
>  void os_setup_post(void);
> -int os_mlock(void);
> +int os_mlock(bool on_fault);
>  
>  /**
>   * qemu_alloc_stack:
> diff --git a/include/system/os-win32.h b/include/system/os-win32.h
> index b82a5d3ad9..bc623061d8 100644
> --- a/include/system/os-win32.h
> +++ b/include/system/os-win32.h
> @@ -123,7 +123,7 @@ static inline bool is_daemonized(void)
>      return false;
>  }
>  
> -static inline int os_mlock(void)
> +static inline int os_mlock(bool on_fault G_GNUC_UNUSED)

So did this actually generate a warning ? We don' even need
G_GNUC_UNUSED unless we're actually seeing warnings about this.

>  {
>      return -ENOSYS;
>  }
> diff --git a/migration/postcopy-ram.c b/migration/postcopy-ram.c
> index 6a6da6ba7f..fc4d8a10df 100644
> --- a/migration/postcopy-ram.c
> +++ b/migration/postcopy-ram.c
> @@ -652,7 +652,7 @@ int postcopy_ram_incoming_cleanup(MigrationIncomingState *mis)
>      }
>  
>      if (enable_mlock) {
> -        if (os_mlock() < 0) {
> +        if (os_mlock(false) < 0) {
>              error_report("mlock: %s", strerror(errno));
>              /*
>               * It doesn't feel right to fail at this point, we have a valid
> diff --git a/os-posix.c b/os-posix.c
> index 9cce55ff2f..52925c23d3 100644
> --- a/os-posix.c
> +++ b/os-posix.c
> @@ -327,18 +327,29 @@ void os_set_line_buffering(void)
>      setvbuf(stdout, NULL, _IOLBF, 0);
>  }
>  
> -int os_mlock(void)
> +int os_mlock(bool on_fault)
>  {
>  #ifdef HAVE_MLOCKALL
>      int ret = 0;
> +    int flags = MCL_CURRENT | MCL_FUTURE;
>  
> -    ret = mlockall(MCL_CURRENT | MCL_FUTURE);
> +    if (on_fault) {
> +#ifdef HAVE_MLOCK_ONFAULT
> +        flags |= MCL_ONFAULT;
> +#else
> +        error_report("mlockall: on_fault not supported");
> +        return -EINVAL;
> +#endif
> +    }
> +
> +    ret = mlockall(flags);
>      if (ret < 0) {
>          error_report("mlockall: %s", strerror(errno));
>      }
>  
>      return ret;
>  #else
> +    (void)on_fault;

Still has the pointless (void) cast I mentioned on the previous postign.

>      return -ENOSYS;
>  #endif
>  }

With regards,
Daniel
-- 
|: https://berrange.com      -o-    https://www.flickr.com/photos/dberrange :|
|: https://libvirt.org         -o-            https://fstop138.berrange.com :|
|: https://entangle-photo.org    -o-    https://www.instagram.com/dberrange :|

Re: [PULL v2 11/14] os: add an ability to lock memory on_fault

[Peter Xu]

On Wed, Feb 12, 2025 at 05:48:46PM +0000, Daniel P. Berrangé wrote:
> On Wed, Feb 12, 2025 at 12:38:23PM -0500, Peter Xu wrote:
> > From: Daniil Tatianin <d-tatianin@yandex-team.ru>
> > 
> > This will be used in the following commits to make it possible to only
> > lock memory on fault instead of right away.
> > 
> > Signed-off-by: Daniil Tatianin <d-tatianin@yandex-team.ru>
> > Reviewed-by: Vladimir Sementsov-Ogievskiy <vsementsov@yandex-team.ru>
> > Link: https://lore.kernel.org/r/20250212143920.1269754-2-d-tatianin@yandex-team.ru
> > [peterx: fail os_mlock(on_fault=1) when not supported]
> > [peterx: use G_GNUC_UNUSED instead of "(void)on_fault", per Dan]
> > Signed-off-by: Peter Xu <peterx@redhat.com>
> > ---
> >  meson.build               |  6 ++++++
> >  include/system/os-posix.h |  2 +-
> >  include/system/os-win32.h |  2 +-
> >  migration/postcopy-ram.c  |  2 +-
> >  os-posix.c                | 15 +++++++++++++--
> >  system/vl.c               |  2 +-
> >  6 files changed, 23 insertions(+), 6 deletions(-)
> > 
> > diff --git a/meson.build b/meson.build
> > index 18cf9e2913..59953cbe6b 100644
> > --- a/meson.build
> > +++ b/meson.build
> > @@ -2885,6 +2885,12 @@ config_host_data.set('HAVE_MLOCKALL', cc.links(gnu_source_prefix + '''
> >      return mlockall(MCL_FUTURE);
> >    }'''))
> >  
> > +config_host_data.set('HAVE_MLOCK_ONFAULT', cc.links(gnu_source_prefix + '''
> > +  #include <sys/mman.h>
> > +  int main(void) {
> > +      return mlockall(MCL_FUTURE | MCL_ONFAULT);
> > +  }'''))
> > +
> >  have_l2tpv3 = false
> >  if get_option('l2tpv3').allowed() and have_system
> >    have_l2tpv3 = cc.has_type('struct mmsghdr',
> > diff --git a/include/system/os-posix.h b/include/system/os-posix.h
> > index b881ac6c6f..ce5b3bccf8 100644
> > --- a/include/system/os-posix.h
> > +++ b/include/system/os-posix.h
> > @@ -53,7 +53,7 @@ bool os_set_runas(const char *user_id);
> >  void os_set_chroot(const char *path);
> >  void os_setup_limits(void);
> >  void os_setup_post(void);
> > -int os_mlock(void);
> > +int os_mlock(bool on_fault);
> >  
> >  /**
> >   * qemu_alloc_stack:
> > diff --git a/include/system/os-win32.h b/include/system/os-win32.h
> > index b82a5d3ad9..bc623061d8 100644
> > --- a/include/system/os-win32.h
> > +++ b/include/system/os-win32.h
> > @@ -123,7 +123,7 @@ static inline bool is_daemonized(void)
> >      return false;
> >  }
> >  
> > -static inline int os_mlock(void)
> > +static inline int os_mlock(bool on_fault G_GNUC_UNUSED)
> 
> So did this actually generate a warning ? We don' even need
> G_GNUC_UNUSED unless we're actually seeing warnings about this.

I didn't try to hit a warning without it, as we can use different compilers
and I thought the results could be different, even if I try it and it
didn't raise a warning?

I do see though that we have plenty of such uses in the current tree,
though.  Does it mean it's a broader question to ask, rather than this
patch only?

Thanks,

-- 
Peter Xu

Re: [PULL v2 11/14] os: add an ability to lock memory on_fault

[Daniel P. Berrangé]

On Wed, Feb 12, 2025 at 12:56:46PM -0500, Peter Xu wrote:
> On Wed, Feb 12, 2025 at 05:48:46PM +0000, Daniel P. Berrangé wrote:
> > On Wed, Feb 12, 2025 at 12:38:23PM -0500, Peter Xu wrote:
> > > From: Daniil Tatianin <d-tatianin@yandex-team.ru>
> > > 
> > > This will be used in the following commits to make it possible to only
> > > lock memory on fault instead of right away.
> > > 
> > > Signed-off-by: Daniil Tatianin <d-tatianin@yandex-team.ru>
> > > Reviewed-by: Vladimir Sementsov-Ogievskiy <vsementsov@yandex-team.ru>
> > > Link: https://lore.kernel.org/r/20250212143920.1269754-2-d-tatianin@yandex-team.ru
> > > [peterx: fail os_mlock(on_fault=1) when not supported]
> > > [peterx: use G_GNUC_UNUSED instead of "(void)on_fault", per Dan]
> > > Signed-off-by: Peter Xu <peterx@redhat.com>
> > > ---
> > >  meson.build               |  6 ++++++
> > >  include/system/os-posix.h |  2 +-
> > >  include/system/os-win32.h |  2 +-
> > >  migration/postcopy-ram.c  |  2 +-
> > >  os-posix.c                | 15 +++++++++++++--
> > >  system/vl.c               |  2 +-
> > >  6 files changed, 23 insertions(+), 6 deletions(-)
> > > 
> > > diff --git a/meson.build b/meson.build
> > > index 18cf9e2913..59953cbe6b 100644
> > > --- a/meson.build
> > > +++ b/meson.build
> > > @@ -2885,6 +2885,12 @@ config_host_data.set('HAVE_MLOCKALL', cc.links(gnu_source_prefix + '''
> > >      return mlockall(MCL_FUTURE);
> > >    }'''))
> > >  
> > > +config_host_data.set('HAVE_MLOCK_ONFAULT', cc.links(gnu_source_prefix + '''
> > > +  #include <sys/mman.h>
> > > +  int main(void) {
> > > +      return mlockall(MCL_FUTURE | MCL_ONFAULT);
> > > +  }'''))
> > > +
> > >  have_l2tpv3 = false
> > >  if get_option('l2tpv3').allowed() and have_system
> > >    have_l2tpv3 = cc.has_type('struct mmsghdr',
> > > diff --git a/include/system/os-posix.h b/include/system/os-posix.h
> > > index b881ac6c6f..ce5b3bccf8 100644
> > > --- a/include/system/os-posix.h
> > > +++ b/include/system/os-posix.h
> > > @@ -53,7 +53,7 @@ bool os_set_runas(const char *user_id);
> > >  void os_set_chroot(const char *path);
> > >  void os_setup_limits(void);
> > >  void os_setup_post(void);
> > > -int os_mlock(void);
> > > +int os_mlock(bool on_fault);
> > >  
> > >  /**
> > >   * qemu_alloc_stack:
> > > diff --git a/include/system/os-win32.h b/include/system/os-win32.h
> > > index b82a5d3ad9..bc623061d8 100644
> > > --- a/include/system/os-win32.h
> > > +++ b/include/system/os-win32.h
> > > @@ -123,7 +123,7 @@ static inline bool is_daemonized(void)
> > >      return false;
> > >  }
> > >  
> > > -static inline int os_mlock(void)
> > > +static inline int os_mlock(bool on_fault G_GNUC_UNUSED)
> > 
> > So did this actually generate a warning ? We don' even need
> > G_GNUC_UNUSED unless we're actually seeing warnings about this.
> 
> I didn't try to hit a warning without it, as we can use different compilers
> and I thought the results could be different, even if I try it and it
> didn't raise a warning?

We strictly only permit use of clang & gcc.

> I do see though that we have plenty of such uses in the current tree,
> though.  Does it mean it's a broader question to ask, rather than this
> patch only?
> 
> Thanks,
> 
> -- 
> Peter Xu
> 

With regards,
Daniel
-- 
|: https://berrange.com      -o-    https://www.flickr.com/photos/dberrange :|
|: https://libvirt.org         -o-            https://fstop138.berrange.com :|
|: https://entangle-photo.org    -o-    https://www.instagram.com/dberrange :|

Re: [PULL v2 11/14] os: add an ability to lock memory on_fault

[Peter Xu]

On Wed, Feb 12, 2025 at 06:03:30PM +0000, Daniel P. Berrangé wrote:
> On Wed, Feb 12, 2025 at 12:56:46PM -0500, Peter Xu wrote:
> > On Wed, Feb 12, 2025 at 05:48:46PM +0000, Daniel P. Berrangé wrote:
> > > On Wed, Feb 12, 2025 at 12:38:23PM -0500, Peter Xu wrote:
> > > > From: Daniil Tatianin <d-tatianin@yandex-team.ru>
> > > > 
> > > > This will be used in the following commits to make it possible to only
> > > > lock memory on fault instead of right away.
> > > > 
> > > > Signed-off-by: Daniil Tatianin <d-tatianin@yandex-team.ru>
> > > > Reviewed-by: Vladimir Sementsov-Ogievskiy <vsementsov@yandex-team.ru>
> > > > Link: https://lore.kernel.org/r/20250212143920.1269754-2-d-tatianin@yandex-team.ru
> > > > [peterx: fail os_mlock(on_fault=1) when not supported]
> > > > [peterx: use G_GNUC_UNUSED instead of "(void)on_fault", per Dan]
> > > > Signed-off-by: Peter Xu <peterx@redhat.com>
> > > > ---
> > > >  meson.build               |  6 ++++++
> > > >  include/system/os-posix.h |  2 +-
> > > >  include/system/os-win32.h |  2 +-
> > > >  migration/postcopy-ram.c  |  2 +-
> > > >  os-posix.c                | 15 +++++++++++++--
> > > >  system/vl.c               |  2 +-
> > > >  6 files changed, 23 insertions(+), 6 deletions(-)
> > > > 
> > > > diff --git a/meson.build b/meson.build
> > > > index 18cf9e2913..59953cbe6b 100644
> > > > --- a/meson.build
> > > > +++ b/meson.build
> > > > @@ -2885,6 +2885,12 @@ config_host_data.set('HAVE_MLOCKALL', cc.links(gnu_source_prefix + '''
> > > >      return mlockall(MCL_FUTURE);
> > > >    }'''))
> > > >  
> > > > +config_host_data.set('HAVE_MLOCK_ONFAULT', cc.links(gnu_source_prefix + '''
> > > > +  #include <sys/mman.h>
> > > > +  int main(void) {
> > > > +      return mlockall(MCL_FUTURE | MCL_ONFAULT);
> > > > +  }'''))
> > > > +
> > > >  have_l2tpv3 = false
> > > >  if get_option('l2tpv3').allowed() and have_system
> > > >    have_l2tpv3 = cc.has_type('struct mmsghdr',
> > > > diff --git a/include/system/os-posix.h b/include/system/os-posix.h
> > > > index b881ac6c6f..ce5b3bccf8 100644
> > > > --- a/include/system/os-posix.h
> > > > +++ b/include/system/os-posix.h
> > > > @@ -53,7 +53,7 @@ bool os_set_runas(const char *user_id);
> > > >  void os_set_chroot(const char *path);
> > > >  void os_setup_limits(void);
> > > >  void os_setup_post(void);
> > > > -int os_mlock(void);
> > > > +int os_mlock(bool on_fault);
> > > >  
> > > >  /**
> > > >   * qemu_alloc_stack:
> > > > diff --git a/include/system/os-win32.h b/include/system/os-win32.h
> > > > index b82a5d3ad9..bc623061d8 100644
> > > > --- a/include/system/os-win32.h
> > > > +++ b/include/system/os-win32.h
> > > > @@ -123,7 +123,7 @@ static inline bool is_daemonized(void)
> > > >      return false;
> > > >  }
> > > >  
> > > > -static inline int os_mlock(void)
> > > > +static inline int os_mlock(bool on_fault G_GNUC_UNUSED)
> > > 
> > > So did this actually generate a warning ? We don' even need
> > > G_GNUC_UNUSED unless we're actually seeing warnings about this.
> > 
> > I didn't try to hit a warning without it, as we can use different compilers
> > and I thought the results could be different, even if I try it and it
> > didn't raise a warning?
> 
> We strictly only permit use of clang & gcc.

I meant I am also not sure whether the versions could matter.. Totally not
expert on compilers.  Hence I chose to be safe with the attribute applied,
because I know it'll always be safe when with it.

I tried to grep QEMU code base:

$ git grep unused-parameter
$ git grep -w Wall
pc-bios/optionrom/Makefile:override CFLAGS += -march=i486 -Wall $(EXTRA_CFLAGS) -m16
pc-bios/s390-ccw/Makefile:EXTRA_CFLAGS += -Wall
tests/multiboot/Makefile:CCFLAGS=-m32 -Wall -Wextra -Werror -fno-stack-protector -nostdinc -fno-builtin
tests/tcg/Makefile.target:CFLAGS+=-Wall -Werror -O0 -g -fno-strict-aliasing
tests/tcg/mips/user/isa/r5900/Makefile:CFLAGS  = -Wall -mabi=32 -march=r5900 -static

We don't seem to have explicit requirement on that (as I grepped nothing
for "unused-parameter"), meanwhile indeed we also seem to have zero usage
at least in qemu root with enabling -Wall.  I'm not sure whether other
compiler option could matter here.

Can this be justifed as we can safely drop G_GNUC_UNUSED in this patch?

OTOH:

$ git grep G_GNUC_UNUSED | wc -l
169

So we have 169 existing such use cases.  Does it also mean we could
potentially drop all of them?

It'll definitely be easier for me (and hopefully Stefan too..) if this can
be done later, but I'm OK respin v3.

Dan, do you have any preference / suggestion?

Thanks,

-- 
Peter Xu

Re: [PULL v2 11/14] os: add an ability to lock memory on_fault

[Daniel P. Berrangé]

On Wed, Feb 12, 2025 at 04:33:51PM -0500, Peter Xu wrote:
> On Wed, Feb 12, 2025 at 06:03:30PM +0000, Daniel P. Berrangé wrote:
> > On Wed, Feb 12, 2025 at 12:56:46PM -0500, Peter Xu wrote:
> > > On Wed, Feb 12, 2025 at 05:48:46PM +0000, Daniel P. Berrangé wrote:
> > > > On Wed, Feb 12, 2025 at 12:38:23PM -0500, Peter Xu wrote:
> > > > > From: Daniil Tatianin <d-tatianin@yandex-team.ru>
> > > > > 
> > > > > This will be used in the following commits to make it possible to only
> > > > > lock memory on fault instead of right away.
> > > > > 
> > > > > Signed-off-by: Daniil Tatianin <d-tatianin@yandex-team.ru>
> > > > > Reviewed-by: Vladimir Sementsov-Ogievskiy <vsementsov@yandex-team.ru>
> > > > > Link: https://lore.kernel.org/r/20250212143920.1269754-2-d-tatianin@yandex-team.ru
> > > > > [peterx: fail os_mlock(on_fault=1) when not supported]
> > > > > [peterx: use G_GNUC_UNUSED instead of "(void)on_fault", per Dan]
> > > > > Signed-off-by: Peter Xu <peterx@redhat.com>
> > > > > ---
> > > > >  meson.build               |  6 ++++++
> > > > >  include/system/os-posix.h |  2 +-
> > > > >  include/system/os-win32.h |  2 +-
> > > > >  migration/postcopy-ram.c  |  2 +-
> > > > >  os-posix.c                | 15 +++++++++++++--
> > > > >  system/vl.c               |  2 +-
> > > > >  6 files changed, 23 insertions(+), 6 deletions(-)
> > > > > 
> > > > > diff --git a/meson.build b/meson.build
> > > > > index 18cf9e2913..59953cbe6b 100644
> > > > > --- a/meson.build
> > > > > +++ b/meson.build
> > > > > @@ -2885,6 +2885,12 @@ config_host_data.set('HAVE_MLOCKALL', cc.links(gnu_source_prefix + '''
> > > > >      return mlockall(MCL_FUTURE);
> > > > >    }'''))
> > > > >  
> > > > > +config_host_data.set('HAVE_MLOCK_ONFAULT', cc.links(gnu_source_prefix + '''
> > > > > +  #include <sys/mman.h>
> > > > > +  int main(void) {
> > > > > +      return mlockall(MCL_FUTURE | MCL_ONFAULT);
> > > > > +  }'''))
> > > > > +
> > > > >  have_l2tpv3 = false
> > > > >  if get_option('l2tpv3').allowed() and have_system
> > > > >    have_l2tpv3 = cc.has_type('struct mmsghdr',
> > > > > diff --git a/include/system/os-posix.h b/include/system/os-posix.h
> > > > > index b881ac6c6f..ce5b3bccf8 100644
> > > > > --- a/include/system/os-posix.h
> > > > > +++ b/include/system/os-posix.h
> > > > > @@ -53,7 +53,7 @@ bool os_set_runas(const char *user_id);
> > > > >  void os_set_chroot(const char *path);
> > > > >  void os_setup_limits(void);
> > > > >  void os_setup_post(void);
> > > > > -int os_mlock(void);
> > > > > +int os_mlock(bool on_fault);
> > > > >  
> > > > >  /**
> > > > >   * qemu_alloc_stack:
> > > > > diff --git a/include/system/os-win32.h b/include/system/os-win32.h
> > > > > index b82a5d3ad9..bc623061d8 100644
> > > > > --- a/include/system/os-win32.h
> > > > > +++ b/include/system/os-win32.h
> > > > > @@ -123,7 +123,7 @@ static inline bool is_daemonized(void)
> > > > >      return false;
> > > > >  }
> > > > >  
> > > > > -static inline int os_mlock(void)
> > > > > +static inline int os_mlock(bool on_fault G_GNUC_UNUSED)
> > > > 
> > > > So did this actually generate a warning ? We don' even need
> > > > G_GNUC_UNUSED unless we're actually seeing warnings about this.
> > > 
> > > I didn't try to hit a warning without it, as we can use different compilers
> > > and I thought the results could be different, even if I try it and it
> > > didn't raise a warning?
> > 
> > We strictly only permit use of clang & gcc.
> 
> I meant I am also not sure whether the versions could matter.. Totally not
> expert on compilers.  Hence I chose to be safe with the attribute applied,
> because I know it'll always be safe when with it.
> 
> I tried to grep QEMU code base:
> 
> $ git grep unused-parameter
> $ git grep -w Wall
> pc-bios/optionrom/Makefile:override CFLAGS += -march=i486 -Wall $(EXTRA_CFLAGS) -m16
> pc-bios/s390-ccw/Makefile:EXTRA_CFLAGS += -Wall
> tests/multiboot/Makefile:CCFLAGS=-m32 -Wall -Wextra -Werror -fno-stack-protector -nostdinc -fno-builtin
> tests/tcg/Makefile.target:CFLAGS+=-Wall -Werror -O0 -g -fno-strict-aliasing
> tests/tcg/mips/user/isa/r5900/Makefile:CFLAGS  = -Wall -mabi=32 -march=r5900 -static
> 
> We don't seem to have explicit requirement on that (as I grepped nothing
> for "unused-parameter"), meanwhile indeed we also seem to have zero usage
> at least in qemu root with enabling -Wall.  I'm not sure whether other
> compiler option could matter here.
> 
> Can this be justifed as we can safely drop G_GNUC_UNUSED in this patch?
> 
> OTOH:
> 
> $ git grep G_GNUC_UNUSED | wc -l
> 169
> 
> So we have 169 existing such use cases.  Does it also mean we could
> potentially drop all of them?

Yes, or actually turn on the warning about unused params and mark
the rest. It is initially noisey, but IME does end up flagging
real problems periodically. Anyway, given we're inconsistent already
there's no need to respin this series.

> 
> It'll definitely be easier for me (and hopefully Stefan too..) if this can
> be done later, but I'm OK respin v3.
> 
> Dan, do you have any preference / suggestion?

With regards,
Daniel
-- 
|: https://berrange.com      -o-    https://www.flickr.com/photos/dberrange :|
|: https://libvirt.org         -o-            https://fstop138.berrange.com :|
|: https://entangle-photo.org    -o-    https://www.instagram.com/dberrange :|

Re: [PULL v2 11/14] os: add an ability to lock memory on_fault

[Peter Xu]

On Tue, Feb 18, 2025 at 04:36:31PM +0000, Daniel P. Berrangé wrote:
> Yes, or actually turn on the warning about unused params and mark
> the rest. It is initially noisey, but IME does end up flagging
> real problems periodically. Anyway, given we're inconsistent already
> there's no need to respin this series.

Thanks, Dan.

Stefan, I hope this PR can still be picked up.  If I need a repost, please
let me know!

-- 
Peter Xu

Re: [PULL v2 00/14] Mem next patches

[Stefan Hajnoczi]

[-- Attachment #1: Type: text/plain, Size: 116 bytes --]

Applied, thanks.

Please update the changelog at https://wiki.qemu.org/ChangeLog/10.0 for any user-visible changes.

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]