[PATCH 0/7] hw/hyperv: remove duplication compilation units

[PATCH 0/7] hw/hyperv: remove duplication compilation units

[Pierrick Bouvier]

Work towards having a single binary, by removing duplicated object files.

hw/hyperv/hyperv.c was excluded at this time, because it depends on target
dependent symbols:
- from system/kvm.h
    - kvm_check_extension
    - kvm_vm_ioctl
- from exec/cpu-all.h | memory_ldst_phys.h.inc
    - ldq_phys

Pierrick Bouvier (7):
  hw/hyperv/hv-balloon-stub: common compilation unit
  hw/hyperv/hyperv.h: header cleanup
  hw/hyperv/vmbus: common compilation unit
  hw/hyperv/hyperv-proto: move SYNDBG definition from target/i386
  hw/hyperv/syndbg: common compilation unit
  hw/hyperv/balloon: common balloon compilation units
  hw/hyperv/hyperv_testdev: common compilation unit

 include/hw/hyperv/hyperv-proto.h | 12 ++++++++
 include/hw/hyperv/hyperv.h       |  4 ++-
 target/i386/kvm/hyperv-proto.h   | 12 --------
 hw/hyperv/syndbg.c               |  7 +++--
 hw/hyperv/vmbus.c                | 50 ++++++++++++++++----------------
 hw/hyperv/meson.build            |  9 +++---
 6 files changed, 49 insertions(+), 45 deletions(-)

-- 
2.39.5

[PATCH 1/7] hw/hyperv/hv-balloon-stub: common compilation unit

[Pierrick Bouvier]

Signed-off-by: Pierrick Bouvier <pierrick.bouvier@linaro.org>
---
 hw/hyperv/meson.build | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/hw/hyperv/meson.build b/hw/hyperv/meson.build
index d3d2668c71a..f4aa0a5ada9 100644
--- a/hw/hyperv/meson.build
+++ b/hw/hyperv/meson.build
@@ -2,4 +2,5 @@ specific_ss.add(when: 'CONFIG_HYPERV', if_true: files('hyperv.c'))
 specific_ss.add(when: 'CONFIG_HYPERV_TESTDEV', if_true: files('hyperv_testdev.c'))
 specific_ss.add(when: 'CONFIG_VMBUS', if_true: files('vmbus.c'))
 specific_ss.add(when: 'CONFIG_SYNDBG', if_true: files('syndbg.c'))
-specific_ss.add(when: 'CONFIG_HV_BALLOON', if_true: files('hv-balloon.c', 'hv-balloon-page_range_tree.c', 'hv-balloon-our_range_memslots.c'), if_false: files('hv-balloon-stub.c'))
+specific_ss.add(when: 'CONFIG_HV_BALLOON', if_true: files('hv-balloon.c', 'hv-balloon-page_range_tree.c', 'hv-balloon-our_range_memslots.c'))
+system_ss.add(when: 'CONFIG_HV_BALLOON', if_false: files('hv-balloon-stub.c'))
-- 
2.39.5

[PATCH 2/7] hw/hyperv/hyperv.h: header cleanup

[Pierrick Bouvier]

Signed-off-by: Pierrick Bouvier <pierrick.bouvier@linaro.org>
---
 include/hw/hyperv/hyperv.h | 4 +++-
 1 file changed, 3 insertions(+), 1 deletion(-)

diff --git a/include/hw/hyperv/hyperv.h b/include/hw/hyperv/hyperv.h
index d717b4e13d4..c6f7039447f 100644
--- a/include/hw/hyperv/hyperv.h
+++ b/include/hw/hyperv/hyperv.h
@@ -10,7 +10,9 @@
 #ifndef HW_HYPERV_HYPERV_H
 #define HW_HYPERV_HYPERV_H
 
-#include "cpu-qom.h"
+#include "qemu/osdep.h"
+#include "exec/hwaddr.h"
+#include "hw/core/cpu.h"
 #include "hw/hyperv/hyperv-proto.h"
 
 typedef struct HvSintRoute HvSintRoute;
-- 
2.39.5

[PATCH 3/7] hw/hyperv/vmbus: common compilation unit

[Pierrick Bouvier]

Replace TARGET_PAGE.* by runtime calls.

Signed-off-by: Pierrick Bouvier <pierrick.bouvier@linaro.org>
---
 hw/hyperv/vmbus.c     | 50 +++++++++++++++++++++----------------------
 hw/hyperv/meson.build |  2 +-
 2 files changed, 26 insertions(+), 26 deletions(-)

diff --git a/hw/hyperv/vmbus.c b/hw/hyperv/vmbus.c
index 12a7dc43128..109ac319caf 100644
--- a/hw/hyperv/vmbus.c
+++ b/hw/hyperv/vmbus.c
@@ -18,7 +18,7 @@
 #include "hw/hyperv/vmbus.h"
 #include "hw/hyperv/vmbus-bridge.h"
 #include "hw/sysbus.h"
-#include "cpu.h"
+#include "exec/target_page.h"
 #include "trace.h"
 
 enum {
@@ -309,7 +309,7 @@ void vmbus_put_gpadl(VMBusGpadl *gpadl)
 
 uint32_t vmbus_gpadl_len(VMBusGpadl *gpadl)
 {
-    return gpadl->num_gfns * TARGET_PAGE_SIZE;
+    return gpadl->num_gfns * qemu_target_page_size();
 }
 
 static void gpadl_iter_init(GpadlIter *iter, VMBusGpadl *gpadl,
@@ -323,14 +323,14 @@ static void gpadl_iter_init(GpadlIter *iter, VMBusGpadl *gpadl,
 
 static inline void gpadl_iter_cache_unmap(GpadlIter *iter)
 {
-    uint32_t map_start_in_page = (uintptr_t)iter->map & ~TARGET_PAGE_MASK;
-    uint32_t io_end_in_page = ((iter->last_off - 1) & ~TARGET_PAGE_MASK) + 1;
+    uint32_t map_start_in_page = (uintptr_t)iter->map & ~qemu_target_page_mask();
+    uint32_t io_end_in_page = ((iter->last_off - 1) & ~qemu_target_page_mask()) + 1;
 
     /* mapping is only done to do non-zero amount of i/o */
     assert(iter->last_off > 0);
     assert(map_start_in_page < io_end_in_page);
 
-    dma_memory_unmap(iter->as, iter->map, TARGET_PAGE_SIZE - map_start_in_page,
+    dma_memory_unmap(iter->as, iter->map, qemu_target_page_size() - map_start_in_page,
                      iter->dir, io_end_in_page - map_start_in_page);
 }
 
@@ -348,17 +348,17 @@ static ssize_t gpadl_iter_io(GpadlIter *iter, void *buf, uint32_t len)
     assert(iter->active);
 
     while (len) {
-        uint32_t off_in_page = iter->off & ~TARGET_PAGE_MASK;
-        uint32_t pgleft = TARGET_PAGE_SIZE - off_in_page;
+        uint32_t off_in_page = iter->off & ~qemu_target_page_mask();
+        uint32_t pgleft = qemu_target_page_size() - off_in_page;
         uint32_t cplen = MIN(pgleft, len);
         void *p;
 
         /* try to reuse the cached mapping */
         if (iter->map) {
             uint32_t map_start_in_page =
-                (uintptr_t)iter->map & ~TARGET_PAGE_MASK;
-            uint32_t off_base = iter->off & ~TARGET_PAGE_MASK;
-            uint32_t mapped_base = (iter->last_off - 1) & ~TARGET_PAGE_MASK;
+                (uintptr_t)iter->map & ~qemu_target_page_mask();
+            uint32_t off_base = iter->off & ~qemu_target_page_mask();
+            uint32_t mapped_base = (iter->last_off - 1) & ~qemu_target_page_mask();
             if (off_base != mapped_base || off_in_page < map_start_in_page) {
                 gpadl_iter_cache_unmap(iter);
                 iter->map = NULL;
@@ -368,10 +368,10 @@ static ssize_t gpadl_iter_io(GpadlIter *iter, void *buf, uint32_t len)
         if (!iter->map) {
             dma_addr_t maddr;
             dma_addr_t mlen = pgleft;
-            uint32_t idx = iter->off >> TARGET_PAGE_BITS;
+            uint32_t idx = iter->off >> qemu_target_page_bits();
             assert(idx < iter->gpadl->num_gfns);
 
-            maddr = (iter->gpadl->gfns[idx] << TARGET_PAGE_BITS) | off_in_page;
+            maddr = (iter->gpadl->gfns[idx] << qemu_target_page_bits()) | off_in_page;
 
             iter->map = dma_memory_map(iter->as, maddr, &mlen, iter->dir,
                                        MEMTXATTRS_UNSPECIFIED);
@@ -382,7 +382,7 @@ static ssize_t gpadl_iter_io(GpadlIter *iter, void *buf, uint32_t len)
             }
         }
 
-        p = (void *)(uintptr_t)(((uintptr_t)iter->map & TARGET_PAGE_MASK) |
+        p = (void *)(uintptr_t)(((uintptr_t)iter->map & qemu_target_page_mask()) |
                 off_in_page);
         if (iter->dir == DMA_DIRECTION_FROM_DEVICE) {
             memcpy(p, buf, cplen);
@@ -591,9 +591,9 @@ static void ringbuf_init_common(VMBusRingBufCommon *ringbuf, VMBusGpadl *gpadl,
                                 uint32_t begin, uint32_t end)
 {
     ringbuf->as = as;
-    ringbuf->rb_addr = gpadl->gfns[begin] << TARGET_PAGE_BITS;
-    ringbuf->base = (begin + 1) << TARGET_PAGE_BITS;
-    ringbuf->len = (end - begin - 1) << TARGET_PAGE_BITS;
+    ringbuf->rb_addr = gpadl->gfns[begin] << qemu_target_page_bits();
+    ringbuf->base = (begin + 1) << qemu_target_page_bits();
+    ringbuf->len = (end - begin - 1) << qemu_target_page_bits();
     gpadl_iter_init(&ringbuf->iter, gpadl, as, dir);
 }
 
@@ -734,7 +734,7 @@ static int vmbus_channel_notify_guest(VMBusChannel *chan)
     unsigned long *int_map, mask;
     unsigned idx;
     hwaddr addr = chan->vmbus->int_page_gpa;
-    hwaddr len = TARGET_PAGE_SIZE / 2, dirty = 0;
+    hwaddr len = qemu_target_page_size() / 2, dirty = 0;
 
     trace_vmbus_channel_notify_guest(chan->id);
 
@@ -743,7 +743,7 @@ static int vmbus_channel_notify_guest(VMBusChannel *chan)
     }
 
     int_map = cpu_physical_memory_map(addr, &len, 1);
-    if (len != TARGET_PAGE_SIZE / 2) {
+    if (len != qemu_target_page_size() / 2) {
         res = -ENXIO;
         goto unmap;
     }
@@ -1038,14 +1038,14 @@ static int sgl_from_gpa_ranges(QEMUSGList *sgl, VMBusDevice *dev,
         }
         len -= sizeof(range);
 
-        if (range.byte_offset & TARGET_PAGE_MASK) {
+        if (range.byte_offset & qemu_target_page_mask()) {
             goto eio;
         }
 
         for (; range.byte_count; range.byte_offset = 0) {
             uint64_t paddr;
             uint32_t plen = MIN(range.byte_count,
-                                TARGET_PAGE_SIZE - range.byte_offset);
+                                qemu_target_page_size() - range.byte_offset);
 
             if (len < sizeof(uint64_t)) {
                 goto eio;
@@ -1055,7 +1055,7 @@ static int sgl_from_gpa_ranges(QEMUSGList *sgl, VMBusDevice *dev,
                 goto err;
             }
             len -= sizeof(uint64_t);
-            paddr <<= TARGET_PAGE_BITS;
+            paddr <<= qemu_target_page_bits();
             paddr |= range.byte_offset;
             range.byte_count -= plen;
 
@@ -1804,7 +1804,7 @@ static void handle_gpadl_header(VMBus *vmbus, vmbus_message_gpadl_header *msg,
      * anything else and simplify things greatly.
      */
     if (msg->rangecount != 1 || msg->range[0].byte_offset ||
-        (msg->range[0].byte_count != (num_gfns << TARGET_PAGE_BITS))) {
+        (msg->range[0].byte_count != (num_gfns << qemu_target_page_bits()))) {
         return;
     }
 
@@ -2240,10 +2240,10 @@ static void vmbus_signal_event(EventNotifier *e)
         return;
     }
 
-    addr = vmbus->int_page_gpa + TARGET_PAGE_SIZE / 2;
-    len = TARGET_PAGE_SIZE / 2;
+    addr = vmbus->int_page_gpa + qemu_target_page_size() / 2;
+    len = qemu_target_page_size() / 2;
     int_map = cpu_physical_memory_map(addr, &len, 1);
-    if (len != TARGET_PAGE_SIZE / 2) {
+    if (len != qemu_target_page_size() / 2) {
         goto unmap;
     }
 
diff --git a/hw/hyperv/meson.build b/hw/hyperv/meson.build
index f4aa0a5ada9..c855fdcf04c 100644
--- a/hw/hyperv/meson.build
+++ b/hw/hyperv/meson.build
@@ -1,6 +1,6 @@
 specific_ss.add(when: 'CONFIG_HYPERV', if_true: files('hyperv.c'))
 specific_ss.add(when: 'CONFIG_HYPERV_TESTDEV', if_true: files('hyperv_testdev.c'))
-specific_ss.add(when: 'CONFIG_VMBUS', if_true: files('vmbus.c'))
+system_ss.add(when: 'CONFIG_VMBUS', if_true: files('vmbus.c'))
 specific_ss.add(when: 'CONFIG_SYNDBG', if_true: files('syndbg.c'))
 specific_ss.add(when: 'CONFIG_HV_BALLOON', if_true: files('hv-balloon.c', 'hv-balloon-page_range_tree.c', 'hv-balloon-our_range_memslots.c'))
 system_ss.add(when: 'CONFIG_HV_BALLOON', if_false: files('hv-balloon-stub.c'))
-- 
2.39.5

[PATCH 4/7] hw/hyperv/hyperv-proto: move SYNDBG definition from target/i386

[Pierrick Bouvier]

Allows them to be available for common compilation units.

Signed-off-by: Pierrick Bouvier <pierrick.bouvier@linaro.org>
---
 include/hw/hyperv/hyperv-proto.h | 12 ++++++++++++
 target/i386/kvm/hyperv-proto.h   | 12 ------------
 2 files changed, 12 insertions(+), 12 deletions(-)

diff --git a/include/hw/hyperv/hyperv-proto.h b/include/hw/hyperv/hyperv-proto.h
index 4a2297307b0..fffc5ce342f 100644
--- a/include/hw/hyperv/hyperv-proto.h
+++ b/include/hw/hyperv/hyperv-proto.h
@@ -61,6 +61,18 @@
 #define HV_MESSAGE_X64_APIC_EOI               0x80010004
 #define HV_MESSAGE_X64_LEGACY_FP_ERROR        0x80010005
 
+/*
+ * Hyper-V Synthetic debug options MSR
+ */
+#define HV_X64_MSR_SYNDBG_CONTROL               0x400000F1
+#define HV_X64_MSR_SYNDBG_STATUS                0x400000F2
+#define HV_X64_MSR_SYNDBG_SEND_BUFFER           0x400000F3
+#define HV_X64_MSR_SYNDBG_RECV_BUFFER           0x400000F4
+#define HV_X64_MSR_SYNDBG_PENDING_BUFFER        0x400000F5
+#define HV_X64_MSR_SYNDBG_OPTIONS               0x400000FF
+
+#define HV_X64_SYNDBG_OPTION_USE_HCALLS         BIT(2)
+
 /*
  * Message flags
  */
diff --git a/target/i386/kvm/hyperv-proto.h b/target/i386/kvm/hyperv-proto.h
index 464fbf09e35..a9f056f2f3e 100644
--- a/target/i386/kvm/hyperv-proto.h
+++ b/target/i386/kvm/hyperv-proto.h
@@ -151,18 +151,6 @@
 #define HV_X64_MSR_STIMER3_CONFIG               0x400000B6
 #define HV_X64_MSR_STIMER3_COUNT                0x400000B7
 
-/*
- * Hyper-V Synthetic debug options MSR
- */
-#define HV_X64_MSR_SYNDBG_CONTROL               0x400000F1
-#define HV_X64_MSR_SYNDBG_STATUS                0x400000F2
-#define HV_X64_MSR_SYNDBG_SEND_BUFFER           0x400000F3
-#define HV_X64_MSR_SYNDBG_RECV_BUFFER           0x400000F4
-#define HV_X64_MSR_SYNDBG_PENDING_BUFFER        0x400000F5
-#define HV_X64_MSR_SYNDBG_OPTIONS               0x400000FF
-
-#define HV_X64_SYNDBG_OPTION_USE_HCALLS         BIT(2)
-
 /*
  * Guest crash notification MSRs
  */
-- 
2.39.5

[PATCH 5/7] hw/hyperv/syndbg: common compilation unit

[Pierrick Bouvier]

Replace TARGET_PAGE.* by runtime calls

Signed-off-by: Pierrick Bouvier <pierrick.bouvier@linaro.org>
---
 hw/hyperv/syndbg.c    | 7 ++++---
 hw/hyperv/meson.build | 2 +-
 2 files changed, 5 insertions(+), 4 deletions(-)

diff --git a/hw/hyperv/syndbg.c b/hw/hyperv/syndbg.c
index d3e39170772..f9382202ed3 100644
--- a/hw/hyperv/syndbg.c
+++ b/hw/hyperv/syndbg.c
@@ -14,7 +14,7 @@
 #include "migration/vmstate.h"
 #include "hw/qdev-properties.h"
 #include "hw/loader.h"
-#include "cpu.h"
+#include "exec/target_page.h"
 #include "hw/hyperv/hyperv.h"
 #include "hw/hyperv/vmbus-bridge.h"
 #include "hw/hyperv/hyperv-proto.h"
@@ -188,7 +188,8 @@ static uint16_t handle_recv_msg(HvSynDbg *syndbg, uint64_t outgpa,
                                 uint64_t timeout, uint32_t *retrieved_count)
 {
     uint16_t ret;
-    uint8_t data_buf[TARGET_PAGE_SIZE - UDP_PKT_HEADER_SIZE];
+    const size_t buf_size = qemu_target_page_size() - UDP_PKT_HEADER_SIZE;
+    uint8_t *data_buf = g_alloca(buf_size);
     hwaddr out_len;
     void *out_data;
     ssize_t recv_byte_count;
@@ -201,7 +202,7 @@ static uint16_t handle_recv_msg(HvSynDbg *syndbg, uint64_t outgpa,
         recv_byte_count = 0;
     } else {
         recv_byte_count = recv(syndbg->socket, data_buf,
-                               MIN(sizeof(data_buf), count), MSG_WAITALL);
+                               MIN(buf_size, count), MSG_WAITALL);
         if (recv_byte_count == -1) {
             return HV_STATUS_INVALID_PARAMETER;
         }
diff --git a/hw/hyperv/meson.build b/hw/hyperv/meson.build
index c855fdcf04c..a9f2045a9af 100644
--- a/hw/hyperv/meson.build
+++ b/hw/hyperv/meson.build
@@ -1,6 +1,6 @@
 specific_ss.add(when: 'CONFIG_HYPERV', if_true: files('hyperv.c'))
 specific_ss.add(when: 'CONFIG_HYPERV_TESTDEV', if_true: files('hyperv_testdev.c'))
 system_ss.add(when: 'CONFIG_VMBUS', if_true: files('vmbus.c'))
-specific_ss.add(when: 'CONFIG_SYNDBG', if_true: files('syndbg.c'))
+system_ss.add(when: 'CONFIG_SYNDBG', if_true: files('syndbg.c'))
 specific_ss.add(when: 'CONFIG_HV_BALLOON', if_true: files('hv-balloon.c', 'hv-balloon-page_range_tree.c', 'hv-balloon-our_range_memslots.c'))
 system_ss.add(when: 'CONFIG_HV_BALLOON', if_false: files('hv-balloon-stub.c'))
-- 
2.39.5

[PATCH 6/7] hw/hyperv/balloon: common balloon compilation units

[Pierrick Bouvier]

Signed-off-by: Pierrick Bouvier <pierrick.bouvier@linaro.org>
---
 hw/hyperv/meson.build | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/hw/hyperv/meson.build b/hw/hyperv/meson.build
index a9f2045a9af..5acd709bdd5 100644
--- a/hw/hyperv/meson.build
+++ b/hw/hyperv/meson.build
@@ -2,5 +2,5 @@ specific_ss.add(when: 'CONFIG_HYPERV', if_true: files('hyperv.c'))
 specific_ss.add(when: 'CONFIG_HYPERV_TESTDEV', if_true: files('hyperv_testdev.c'))
 system_ss.add(when: 'CONFIG_VMBUS', if_true: files('vmbus.c'))
 system_ss.add(when: 'CONFIG_SYNDBG', if_true: files('syndbg.c'))
-specific_ss.add(when: 'CONFIG_HV_BALLOON', if_true: files('hv-balloon.c', 'hv-balloon-page_range_tree.c', 'hv-balloon-our_range_memslots.c'))
+system_ss.add(when: 'CONFIG_HV_BALLOON', if_true: files('hv-balloon.c', 'hv-balloon-page_range_tree.c', 'hv-balloon-our_range_memslots.c'))
 system_ss.add(when: 'CONFIG_HV_BALLOON', if_false: files('hv-balloon-stub.c'))
-- 
2.39.5

[PATCH 7/7] hw/hyperv/hyperv_testdev: common compilation unit

[Pierrick Bouvier]

Signed-off-by: Pierrick Bouvier <pierrick.bouvier@linaro.org>
---
 hw/hyperv/meson.build | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/hw/hyperv/meson.build b/hw/hyperv/meson.build
index 5acd709bdd5..ef5a596c8ab 100644
--- a/hw/hyperv/meson.build
+++ b/hw/hyperv/meson.build
@@ -1,5 +1,5 @@
 specific_ss.add(when: 'CONFIG_HYPERV', if_true: files('hyperv.c'))
-specific_ss.add(when: 'CONFIG_HYPERV_TESTDEV', if_true: files('hyperv_testdev.c'))
+system_ss.add(when: 'CONFIG_HYPERV_TESTDEV', if_true: files('hyperv_testdev.c'))
 system_ss.add(when: 'CONFIG_VMBUS', if_true: files('vmbus.c'))
 system_ss.add(when: 'CONFIG_SYNDBG', if_true: files('syndbg.c'))
 system_ss.add(when: 'CONFIG_HV_BALLOON', if_true: files('hv-balloon.c', 'hv-balloon-page_range_tree.c', 'hv-balloon-our_range_memslots.c'))
-- 
2.39.5

Re: [PATCH 4/7] hw/hyperv/hyperv-proto: move SYNDBG definition from target/i386

[Philippe Mathieu-Daudé]

On 6/3/25 07:41, Pierrick Bouvier wrote:
> Allows them to be available for common compilation units.
> 
> Signed-off-by: Pierrick Bouvier <pierrick.bouvier@linaro.org>
> ---
>   include/hw/hyperv/hyperv-proto.h | 12 ++++++++++++
>   target/i386/kvm/hyperv-proto.h   | 12 ------------
>   2 files changed, 12 insertions(+), 12 deletions(-)

Reviewed-by: Philippe Mathieu-Daudé <philmd@linaro.org>

Re: [PATCH 2/7] hw/hyperv/hyperv.h: header cleanup

[Alex Bennée]

Pierrick Bouvier <pierrick.bouvier@linaro.org> writes:

> Signed-off-by: Pierrick Bouvier <pierrick.bouvier@linaro.org>
> ---
>  include/hw/hyperv/hyperv.h | 4 +++-
>  1 file changed, 3 insertions(+), 1 deletion(-)
>
> diff --git a/include/hw/hyperv/hyperv.h b/include/hw/hyperv/hyperv.h
> index d717b4e13d4..c6f7039447f 100644
> --- a/include/hw/hyperv/hyperv.h
> +++ b/include/hw/hyperv/hyperv.h
> @@ -10,7 +10,9 @@
>  #ifndef HW_HYPERV_HYPERV_H
>  #define HW_HYPERV_HYPERV_H
>  
> -#include "cpu-qom.h"
> +#include "qemu/osdep.h"

We shouldn't need to include osdep.h in headers, indeed style says:

  Do not include "qemu/osdep.h" from header files since the .c file will have
  already included it.

> +#include "exec/hwaddr.h"
> +#include "hw/core/cpu.h"
>  #include "hw/hyperv/hyperv-proto.h"
>  
>  typedef struct HvSintRoute HvSintRoute;

-- 
Alex Bennée
Virtualisation Tech Lead @ Linaro

Re: [PATCH 5/7] hw/hyperv/syndbg: common compilation unit

[Richard Henderson]

On 3/5/25 22:41, Pierrick Bouvier wrote:
> Replace TARGET_PAGE.* by runtime calls
> 
> Signed-off-by: Pierrick Bouvier <pierrick.bouvier@linaro.org>
> ---
>   hw/hyperv/syndbg.c    | 7 ++++---
>   hw/hyperv/meson.build | 2 +-
>   2 files changed, 5 insertions(+), 4 deletions(-)
> 
> diff --git a/hw/hyperv/syndbg.c b/hw/hyperv/syndbg.c
> index d3e39170772..f9382202ed3 100644
> --- a/hw/hyperv/syndbg.c
> +++ b/hw/hyperv/syndbg.c
> @@ -14,7 +14,7 @@
>   #include "migration/vmstate.h"
>   #include "hw/qdev-properties.h"
>   #include "hw/loader.h"
> -#include "cpu.h"
> +#include "exec/target_page.h"
>   #include "hw/hyperv/hyperv.h"
>   #include "hw/hyperv/vmbus-bridge.h"
>   #include "hw/hyperv/hyperv-proto.h"
> @@ -188,7 +188,8 @@ static uint16_t handle_recv_msg(HvSynDbg *syndbg, uint64_t outgpa,
>                                   uint64_t timeout, uint32_t *retrieved_count)
>   {
>       uint16_t ret;
> -    uint8_t data_buf[TARGET_PAGE_SIZE - UDP_PKT_HEADER_SIZE];
> +    const size_t buf_size = qemu_target_page_size() - UDP_PKT_HEADER_SIZE;
> +    uint8_t *data_buf = g_alloca(buf_size);
>       hwaddr out_len;
>       void *out_data;
>       ssize_t recv_byte_count;

We've purged the code base of VLAs, and those are preferable to alloca.
Just use g_malloc and g_autofree.


r~

Re: [PATCH 5/7] hw/hyperv/syndbg: common compilation unit

[Pierrick Bouvier]

On 3/6/25 08:19, Richard Henderson wrote:
> On 3/5/25 22:41, Pierrick Bouvier wrote:
>> Replace TARGET_PAGE.* by runtime calls
>>
>> Signed-off-by: Pierrick Bouvier <pierrick.bouvier@linaro.org>
>> ---
>>    hw/hyperv/syndbg.c    | 7 ++++---
>>    hw/hyperv/meson.build | 2 +-
>>    2 files changed, 5 insertions(+), 4 deletions(-)
>>
>> diff --git a/hw/hyperv/syndbg.c b/hw/hyperv/syndbg.c
>> index d3e39170772..f9382202ed3 100644
>> --- a/hw/hyperv/syndbg.c
>> +++ b/hw/hyperv/syndbg.c
>> @@ -14,7 +14,7 @@
>>    #include "migration/vmstate.h"
>>    #include "hw/qdev-properties.h"
>>    #include "hw/loader.h"
>> -#include "cpu.h"
>> +#include "exec/target_page.h"
>>    #include "hw/hyperv/hyperv.h"
>>    #include "hw/hyperv/vmbus-bridge.h"
>>    #include "hw/hyperv/hyperv-proto.h"
>> @@ -188,7 +188,8 @@ static uint16_t handle_recv_msg(HvSynDbg *syndbg, uint64_t outgpa,
>>                                    uint64_t timeout, uint32_t *retrieved_count)
>>    {
>>        uint16_t ret;
>> -    uint8_t data_buf[TARGET_PAGE_SIZE - UDP_PKT_HEADER_SIZE];
>> +    const size_t buf_size = qemu_target_page_size() - UDP_PKT_HEADER_SIZE;
>> +    uint8_t *data_buf = g_alloca(buf_size);
>>        hwaddr out_len;
>>        void *out_data;
>>        ssize_t recv_byte_count;
> 
> We've purged the code base of VLAs, and those are preferable to alloca.
> Just use g_malloc and g_autofree.
>

I hesitated, due to potential performance considerations for people 
reviewing the patch. I'll switch to heap based storage.

> 
> r~

Re: [PATCH 0/7] hw/hyperv: remove duplication compilation units

[Richard Henderson]

On 3/5/25 22:41, Pierrick Bouvier wrote:
> Work towards having a single binary, by removing duplicated object files.
> 
> hw/hyperv/hyperv.c was excluded at this time, because it depends on target
> dependent symbols:
> - from system/kvm.h
>      - kvm_check_extension
>      - kvm_vm_ioctl
> - from exec/cpu-all.h | memory_ldst_phys.h.inc
>      - ldq_phys
> 
> Pierrick Bouvier (7):
>    hw/hyperv/hv-balloon-stub: common compilation unit
>    hw/hyperv/hyperv.h: header cleanup
>    hw/hyperv/vmbus: common compilation unit
>    hw/hyperv/hyperv-proto: move SYNDBG definition from target/i386
>    hw/hyperv/syndbg: common compilation unit
>    hw/hyperv/balloon: common balloon compilation units
>    hw/hyperv/hyperv_testdev: common compilation unit
> 
>   include/hw/hyperv/hyperv-proto.h | 12 ++++++++
>   include/hw/hyperv/hyperv.h       |  4 ++-
>   target/i386/kvm/hyperv-proto.h   | 12 --------
>   hw/hyperv/syndbg.c               |  7 +++--
>   hw/hyperv/vmbus.c                | 50 ++++++++++++++++----------------
>   hw/hyperv/meson.build            |  9 +++---
>   6 files changed, 49 insertions(+), 45 deletions(-)
> 

I'm reasonably certain that hyperv is specific to x86.
Are these only "duplicated" because of qemu-system-{i386,x86_64}?


r~

Re: [PATCH 0/7] hw/hyperv: remove duplication compilation units

[Pierrick Bouvier]

On 3/6/25 08:26, Richard Henderson wrote:
> On 3/5/25 22:41, Pierrick Bouvier wrote:
>> Work towards having a single binary, by removing duplicated object files.
>>
>> hw/hyperv/hyperv.c was excluded at this time, because it depends on target
>> dependent symbols:
>> - from system/kvm.h
>>       - kvm_check_extension
>>       - kvm_vm_ioctl
>> - from exec/cpu-all.h | memory_ldst_phys.h.inc
>>       - ldq_phys
>>
>> Pierrick Bouvier (7):
>>     hw/hyperv/hv-balloon-stub: common compilation unit
>>     hw/hyperv/hyperv.h: header cleanup
>>     hw/hyperv/vmbus: common compilation unit
>>     hw/hyperv/hyperv-proto: move SYNDBG definition from target/i386
>>     hw/hyperv/syndbg: common compilation unit
>>     hw/hyperv/balloon: common balloon compilation units
>>     hw/hyperv/hyperv_testdev: common compilation unit
>>
>>    include/hw/hyperv/hyperv-proto.h | 12 ++++++++
>>    include/hw/hyperv/hyperv.h       |  4 ++-
>>    target/i386/kvm/hyperv-proto.h   | 12 --------
>>    hw/hyperv/syndbg.c               |  7 +++--
>>    hw/hyperv/vmbus.c                | 50 ++++++++++++++++----------------
>>    hw/hyperv/meson.build            |  9 +++---
>>    6 files changed, 49 insertions(+), 45 deletions(-)
>>
> 
> I'm reasonably certain that hyperv is specific to x86.

That's correct.

> Are these only "duplicated" because of qemu-system-{i386,x86_64}?
> 

Yes. A lot of duplications in hw/ is related to 32/64bits variants.

> 
> r~

Re: [PATCH 0/7] hw/hyperv: remove duplication compilation units

[Pierrick Bouvier]

On 3/6/25 08:35, Pierrick Bouvier wrote:
> On 3/6/25 08:26, Richard Henderson wrote:
>> On 3/5/25 22:41, Pierrick Bouvier wrote:
>>> Work towards having a single binary, by removing duplicated object files.
>>>
>>> hw/hyperv/hyperv.c was excluded at this time, because it depends on target
>>> dependent symbols:
>>> - from system/kvm.h
>>>        - kvm_check_extension
>>>        - kvm_vm_ioctl
>>> - from exec/cpu-all.h | memory_ldst_phys.h.inc
>>>        - ldq_phys
>>>
>>> Pierrick Bouvier (7):
>>>      hw/hyperv/hv-balloon-stub: common compilation unit
>>>      hw/hyperv/hyperv.h: header cleanup
>>>      hw/hyperv/vmbus: common compilation unit
>>>      hw/hyperv/hyperv-proto: move SYNDBG definition from target/i386
>>>      hw/hyperv/syndbg: common compilation unit
>>>      hw/hyperv/balloon: common balloon compilation units
>>>      hw/hyperv/hyperv_testdev: common compilation unit
>>>
>>>     include/hw/hyperv/hyperv-proto.h | 12 ++++++++
>>>     include/hw/hyperv/hyperv.h       |  4 ++-
>>>     target/i386/kvm/hyperv-proto.h   | 12 --------
>>>     hw/hyperv/syndbg.c               |  7 +++--
>>>     hw/hyperv/vmbus.c                | 50 ++++++++++++++++----------------
>>>     hw/hyperv/meson.build            |  9 +++---
>>>     6 files changed, 49 insertions(+), 45 deletions(-)
>>>
>>
>> I'm reasonably certain that hyperv is specific to x86.
> 
> That's correct.
> 

But potentially could be extended to arm64 one day as well.

>> Are these only "duplicated" because of qemu-system-{i386,x86_64}?
>>
> 
> Yes. A lot of duplications in hw/ is related to 32/64bits variants.
> 
>>
>> r~
> 

Re: [PATCH 5/7] hw/hyperv/syndbg: common compilation unit

[Philippe Mathieu-Daudé]

On 6/3/25 17:23, Pierrick Bouvier wrote:
> On 3/6/25 08:19, Richard Henderson wrote:
>> On 3/5/25 22:41, Pierrick Bouvier wrote:
>>> Replace TARGET_PAGE.* by runtime calls
>>>
>>> Signed-off-by: Pierrick Bouvier <pierrick.bouvier@linaro.org>
>>> ---
>>>    hw/hyperv/syndbg.c    | 7 ++++---
>>>    hw/hyperv/meson.build | 2 +-
>>>    2 files changed, 5 insertions(+), 4 deletions(-)
>>>
>>> diff --git a/hw/hyperv/syndbg.c b/hw/hyperv/syndbg.c
>>> index d3e39170772..f9382202ed3 100644
>>> --- a/hw/hyperv/syndbg.c
>>> +++ b/hw/hyperv/syndbg.c
>>> @@ -14,7 +14,7 @@
>>>    #include "migration/vmstate.h"
>>>    #include "hw/qdev-properties.h"
>>>    #include "hw/loader.h"
>>> -#include "cpu.h"
>>> +#include "exec/target_page.h"
>>>    #include "hw/hyperv/hyperv.h"
>>>    #include "hw/hyperv/vmbus-bridge.h"
>>>    #include "hw/hyperv/hyperv-proto.h"
>>> @@ -188,7 +188,8 @@ static uint16_t handle_recv_msg(HvSynDbg *syndbg, 
>>> uint64_t outgpa,
>>>                                    uint64_t timeout, uint32_t 
>>> *retrieved_count)
>>>    {
>>>        uint16_t ret;
>>> -    uint8_t data_buf[TARGET_PAGE_SIZE - UDP_PKT_HEADER_SIZE];
>>> +    const size_t buf_size = qemu_target_page_size() - 
>>> UDP_PKT_HEADER_SIZE;
>>> +    uint8_t *data_buf = g_alloca(buf_size);
>>>        hwaddr out_len;
>>>        void *out_data;
>>>        ssize_t recv_byte_count;
>>
>> We've purged the code base of VLAs, and those are preferable to alloca.
>> Just use g_malloc and g_autofree.
>>
> 
> I hesitated, due to potential performance considerations for people 
> reviewing the patch. I'll switch to heap based storage.

OTOH hyperv is x86-only, so we could do:

#define BUFSZ (4 * KiB)

handle_recv_msg()
{
   uint8_t data_buf[BUFSZ - UDP_PKT_HEADER_SIZE];
   ...

hv_syndbg_class_init()
{
   assert(BUFSZ > qemu_target_page_size());
   ...

and call it a day.

Re: [PATCH 3/7] hw/hyperv/vmbus: common compilation unit

[Maciej S. Szmigiero]

On 6.03.2025 07:41, Pierrick Bouvier wrote:
> Replace TARGET_PAGE.* by runtime calls.

Seems like this patch subject/title is not aligned
well with its content, or a least incomplete.

Also, could you provide more detailed information
why TARGET_PAGE_SIZE is getting replaced by
qemu_target_page_size() please?

I don't see such information in the cover letter either.

Thanks,
Maciej
  
> Signed-off-by: Pierrick Bouvier <pierrick.bouvier@linaro.org>
> ---
>   hw/hyperv/vmbus.c     | 50 +++++++++++++++++++++----------------------
>   hw/hyperv/meson.build |  2 +-
>   2 files changed, 26 insertions(+), 26 deletions(-)
> 
> diff --git a/hw/hyperv/vmbus.c b/hw/hyperv/vmbus.c
> index 12a7dc43128..109ac319caf 100644
> --- a/hw/hyperv/vmbus.c
> +++ b/hw/hyperv/vmbus.c
> @@ -18,7 +18,7 @@
>   #include "hw/hyperv/vmbus.h"
>   #include "hw/hyperv/vmbus-bridge.h"
>   #include "hw/sysbus.h"
> -#include "cpu.h"
> +#include "exec/target_page.h"
>   #include "trace.h"
>   
>   enum {
> @@ -309,7 +309,7 @@ void vmbus_put_gpadl(VMBusGpadl *gpadl)
>   
>   uint32_t vmbus_gpadl_len(VMBusGpadl *gpadl)
>   {
> -    return gpadl->num_gfns * TARGET_PAGE_SIZE;
> +    return gpadl->num_gfns * qemu_target_page_size();
>   }
>   
>   static void gpadl_iter_init(GpadlIter *iter, VMBusGpadl *gpadl,
> @@ -323,14 +323,14 @@ static void gpadl_iter_init(GpadlIter *iter, VMBusGpadl *gpadl,
>   
>   static inline void gpadl_iter_cache_unmap(GpadlIter *iter)
>   {
> -    uint32_t map_start_in_page = (uintptr_t)iter->map & ~TARGET_PAGE_MASK;
> -    uint32_t io_end_in_page = ((iter->last_off - 1) & ~TARGET_PAGE_MASK) + 1;
> +    uint32_t map_start_in_page = (uintptr_t)iter->map & ~qemu_target_page_mask();
> +    uint32_t io_end_in_page = ((iter->last_off - 1) & ~qemu_target_page_mask()) + 1;
>   
>       /* mapping is only done to do non-zero amount of i/o */
>       assert(iter->last_off > 0);
>       assert(map_start_in_page < io_end_in_page);
>   
> -    dma_memory_unmap(iter->as, iter->map, TARGET_PAGE_SIZE - map_start_in_page,
> +    dma_memory_unmap(iter->as, iter->map, qemu_target_page_size() - map_start_in_page,
>                        iter->dir, io_end_in_page - map_start_in_page);
>   }
>   
> @@ -348,17 +348,17 @@ static ssize_t gpadl_iter_io(GpadlIter *iter, void *buf, uint32_t len)
>       assert(iter->active);
>   
>       while (len) {
> -        uint32_t off_in_page = iter->off & ~TARGET_PAGE_MASK;
> -        uint32_t pgleft = TARGET_PAGE_SIZE - off_in_page;
> +        uint32_t off_in_page = iter->off & ~qemu_target_page_mask();
> +        uint32_t pgleft = qemu_target_page_size() - off_in_page;
>           uint32_t cplen = MIN(pgleft, len);
>           void *p;
>   
>           /* try to reuse the cached mapping */
>           if (iter->map) {
>               uint32_t map_start_in_page =
> -                (uintptr_t)iter->map & ~TARGET_PAGE_MASK;
> -            uint32_t off_base = iter->off & ~TARGET_PAGE_MASK;
> -            uint32_t mapped_base = (iter->last_off - 1) & ~TARGET_PAGE_MASK;
> +                (uintptr_t)iter->map & ~qemu_target_page_mask();
> +            uint32_t off_base = iter->off & ~qemu_target_page_mask();
> +            uint32_t mapped_base = (iter->last_off - 1) & ~qemu_target_page_mask();
>               if (off_base != mapped_base || off_in_page < map_start_in_page) {
>                   gpadl_iter_cache_unmap(iter);
>                   iter->map = NULL;
> @@ -368,10 +368,10 @@ static ssize_t gpadl_iter_io(GpadlIter *iter, void *buf, uint32_t len)
>           if (!iter->map) {
>               dma_addr_t maddr;
>               dma_addr_t mlen = pgleft;
> -            uint32_t idx = iter->off >> TARGET_PAGE_BITS;
> +            uint32_t idx = iter->off >> qemu_target_page_bits();
>               assert(idx < iter->gpadl->num_gfns);
>   
> -            maddr = (iter->gpadl->gfns[idx] << TARGET_PAGE_BITS) | off_in_page;
> +            maddr = (iter->gpadl->gfns[idx] << qemu_target_page_bits()) | off_in_page;
>   
>               iter->map = dma_memory_map(iter->as, maddr, &mlen, iter->dir,
>                                          MEMTXATTRS_UNSPECIFIED);
> @@ -382,7 +382,7 @@ static ssize_t gpadl_iter_io(GpadlIter *iter, void *buf, uint32_t len)
>               }
>           }
>   
> -        p = (void *)(uintptr_t)(((uintptr_t)iter->map & TARGET_PAGE_MASK) |
> +        p = (void *)(uintptr_t)(((uintptr_t)iter->map & qemu_target_page_mask()) |
>                   off_in_page);
>           if (iter->dir == DMA_DIRECTION_FROM_DEVICE) {
>               memcpy(p, buf, cplen);
> @@ -591,9 +591,9 @@ static void ringbuf_init_common(VMBusRingBufCommon *ringbuf, VMBusGpadl *gpadl,
>                                   uint32_t begin, uint32_t end)
>   {
>       ringbuf->as = as;
> -    ringbuf->rb_addr = gpadl->gfns[begin] << TARGET_PAGE_BITS;
> -    ringbuf->base = (begin + 1) << TARGET_PAGE_BITS;
> -    ringbuf->len = (end - begin - 1) << TARGET_PAGE_BITS;
> +    ringbuf->rb_addr = gpadl->gfns[begin] << qemu_target_page_bits();
> +    ringbuf->base = (begin + 1) << qemu_target_page_bits();
> +    ringbuf->len = (end - begin - 1) << qemu_target_page_bits();
>       gpadl_iter_init(&ringbuf->iter, gpadl, as, dir);
>   }
>   
> @@ -734,7 +734,7 @@ static int vmbus_channel_notify_guest(VMBusChannel *chan)
>       unsigned long *int_map, mask;
>       unsigned idx;
>       hwaddr addr = chan->vmbus->int_page_gpa;
> -    hwaddr len = TARGET_PAGE_SIZE / 2, dirty = 0;
> +    hwaddr len = qemu_target_page_size() / 2, dirty = 0;
>   
>       trace_vmbus_channel_notify_guest(chan->id);
>   
> @@ -743,7 +743,7 @@ static int vmbus_channel_notify_guest(VMBusChannel *chan)
>       }
>   
>       int_map = cpu_physical_memory_map(addr, &len, 1);
> -    if (len != TARGET_PAGE_SIZE / 2) {
> +    if (len != qemu_target_page_size() / 2) {
>           res = -ENXIO;
>           goto unmap;
>       }
> @@ -1038,14 +1038,14 @@ static int sgl_from_gpa_ranges(QEMUSGList *sgl, VMBusDevice *dev,
>           }
>           len -= sizeof(range);
>   
> -        if (range.byte_offset & TARGET_PAGE_MASK) {
> +        if (range.byte_offset & qemu_target_page_mask()) {
>               goto eio;
>           }
>   
>           for (; range.byte_count; range.byte_offset = 0) {
>               uint64_t paddr;
>               uint32_t plen = MIN(range.byte_count,
> -                                TARGET_PAGE_SIZE - range.byte_offset);
> +                                qemu_target_page_size() - range.byte_offset);
>   
>               if (len < sizeof(uint64_t)) {
>                   goto eio;
> @@ -1055,7 +1055,7 @@ static int sgl_from_gpa_ranges(QEMUSGList *sgl, VMBusDevice *dev,
>                   goto err;
>               }
>               len -= sizeof(uint64_t);
> -            paddr <<= TARGET_PAGE_BITS;
> +            paddr <<= qemu_target_page_bits();
>               paddr |= range.byte_offset;
>               range.byte_count -= plen;
>   
> @@ -1804,7 +1804,7 @@ static void handle_gpadl_header(VMBus *vmbus, vmbus_message_gpadl_header *msg,
>        * anything else and simplify things greatly.
>        */
>       if (msg->rangecount != 1 || msg->range[0].byte_offset ||
> -        (msg->range[0].byte_count != (num_gfns << TARGET_PAGE_BITS))) {
> +        (msg->range[0].byte_count != (num_gfns << qemu_target_page_bits()))) {
>           return;
>       }
>   
> @@ -2240,10 +2240,10 @@ static void vmbus_signal_event(EventNotifier *e)
>           return;
>       }
>   
> -    addr = vmbus->int_page_gpa + TARGET_PAGE_SIZE / 2;
> -    len = TARGET_PAGE_SIZE / 2;
> +    addr = vmbus->int_page_gpa + qemu_target_page_size() / 2;
> +    len = qemu_target_page_size() / 2;
>       int_map = cpu_physical_memory_map(addr, &len, 1);
> -    if (len != TARGET_PAGE_SIZE / 2) {
> +    if (len != qemu_target_page_size() / 2) {
>           goto unmap;
>       }
>   
> diff --git a/hw/hyperv/meson.build b/hw/hyperv/meson.build
> index f4aa0a5ada9..c855fdcf04c 100644
> --- a/hw/hyperv/meson.build
> +++ b/hw/hyperv/meson.build
> @@ -1,6 +1,6 @@
>   specific_ss.add(when: 'CONFIG_HYPERV', if_true: files('hyperv.c'))
>   specific_ss.add(when: 'CONFIG_HYPERV_TESTDEV', if_true: files('hyperv_testdev.c'))
> -specific_ss.add(when: 'CONFIG_VMBUS', if_true: files('vmbus.c'))
> +system_ss.add(when: 'CONFIG_VMBUS', if_true: files('vmbus.c'))
>   specific_ss.add(when: 'CONFIG_SYNDBG', if_true: files('syndbg.c'))
>   specific_ss.add(when: 'CONFIG_HV_BALLOON', if_true: files('hv-balloon.c', 'hv-balloon-page_range_tree.c', 'hv-balloon-our_range_memslots.c'))
>   system_ss.add(when: 'CONFIG_HV_BALLOON', if_false: files('hv-balloon-stub.c'))

Re: [PATCH 2/7] hw/hyperv/hyperv.h: header cleanup

[Pierrick Bouvier]

On 3/6/25 04:27, Alex Bennée wrote:
> Pierrick Bouvier <pierrick.bouvier@linaro.org> writes:
> 
>> Signed-off-by: Pierrick Bouvier <pierrick.bouvier@linaro.org>
>> ---
>>   include/hw/hyperv/hyperv.h | 4 +++-
>>   1 file changed, 3 insertions(+), 1 deletion(-)
>>
>> diff --git a/include/hw/hyperv/hyperv.h b/include/hw/hyperv/hyperv.h
>> index d717b4e13d4..c6f7039447f 100644
>> --- a/include/hw/hyperv/hyperv.h
>> +++ b/include/hw/hyperv/hyperv.h
>> @@ -10,7 +10,9 @@
>>   #ifndef HW_HYPERV_HYPERV_H
>>   #define HW_HYPERV_HYPERV_H
>>   
>> -#include "cpu-qom.h"
>> +#include "qemu/osdep.h"
> 
> We shouldn't need to include osdep.h in headers, indeed style says:
> 
>    Do not include "qemu/osdep.h" from header files since the .c file will have
>    already included it.
>

Sure, I'll remove it.

>> +#include "exec/hwaddr.h"
>> +#include "hw/core/cpu.h"
>>   #include "hw/hyperv/hyperv-proto.h"
>>   
>>   typedef struct HvSintRoute HvSintRoute;
> 

Re: [PATCH 5/7] hw/hyperv/syndbg: common compilation unit

[Pierrick Bouvier]

On 3/6/25 09:58, Philippe Mathieu-Daudé wrote:
> On 6/3/25 17:23, Pierrick Bouvier wrote:
>> On 3/6/25 08:19, Richard Henderson wrote:
>>> On 3/5/25 22:41, Pierrick Bouvier wrote:
>>>> Replace TARGET_PAGE.* by runtime calls
>>>>
>>>> Signed-off-by: Pierrick Bouvier <pierrick.bouvier@linaro.org>
>>>> ---
>>>>     hw/hyperv/syndbg.c    | 7 ++++---
>>>>     hw/hyperv/meson.build | 2 +-
>>>>     2 files changed, 5 insertions(+), 4 deletions(-)
>>>>
>>>> diff --git a/hw/hyperv/syndbg.c b/hw/hyperv/syndbg.c
>>>> index d3e39170772..f9382202ed3 100644
>>>> --- a/hw/hyperv/syndbg.c
>>>> +++ b/hw/hyperv/syndbg.c
>>>> @@ -14,7 +14,7 @@
>>>>     #include "migration/vmstate.h"
>>>>     #include "hw/qdev-properties.h"
>>>>     #include "hw/loader.h"
>>>> -#include "cpu.h"
>>>> +#include "exec/target_page.h"
>>>>     #include "hw/hyperv/hyperv.h"
>>>>     #include "hw/hyperv/vmbus-bridge.h"
>>>>     #include "hw/hyperv/hyperv-proto.h"
>>>> @@ -188,7 +188,8 @@ static uint16_t handle_recv_msg(HvSynDbg *syndbg,
>>>> uint64_t outgpa,
>>>>                                     uint64_t timeout, uint32_t
>>>> *retrieved_count)
>>>>     {
>>>>         uint16_t ret;
>>>> -    uint8_t data_buf[TARGET_PAGE_SIZE - UDP_PKT_HEADER_SIZE];
>>>> +    const size_t buf_size = qemu_target_page_size() -
>>>> UDP_PKT_HEADER_SIZE;
>>>> +    uint8_t *data_buf = g_alloca(buf_size);
>>>>         hwaddr out_len;
>>>>         void *out_data;
>>>>         ssize_t recv_byte_count;
>>>
>>> We've purged the code base of VLAs, and those are preferable to alloca.
>>> Just use g_malloc and g_autofree.
>>>
>>
>> I hesitated, due to potential performance considerations for people
>> reviewing the patch. I'll switch to heap based storage.
> 
> OTOH hyperv is x86-only, so we could do:
> 
> #define BUFSZ (4 * KiB)
> 
> handle_recv_msg()
> {
>     uint8_t data_buf[BUFSZ - UDP_PKT_HEADER_SIZE];
>     ...
> 
> hv_syndbg_class_init()
> {
>     assert(BUFSZ > qemu_target_page_size());
>     ...
> 
> and call it a day.

Could be possible for now yes.

Any opinion from concerned maintainers?

Re: [PATCH 3/7] hw/hyperv/vmbus: common compilation unit

[Pierrick Bouvier]

Hi Maciej,

we are currently working toward building a single QEMU binary able to 
emulate all architectures, and one prerequisite is to remove duplication 
of compilation units (some are duplicated per target now, because of 
compile time defines).

So the work here is to replace those compile time defines with runtime 
functions instead, so the same code can be used for various architectures.

Is it more clear for you?

On 3/6/25 12:29, Maciej S. Szmigiero wrote:
> On 6.03.2025 07:41, Pierrick Bouvier wrote:
>> Replace TARGET_PAGE.* by runtime calls.
> 
> Seems like this patch subject/title is not aligned
> well with its content, or a least incomplete.
> 
> Also, could you provide more detailed information
> why TARGET_PAGE_SIZE is getting replaced by
> qemu_target_page_size() please?
> 
> I don't see such information in the cover letter either.
> 
> Thanks,
> Maciej
>    
>> Signed-off-by: Pierrick Bouvier <pierrick.bouvier@linaro.org>
>> ---
>>    hw/hyperv/vmbus.c     | 50 +++++++++++++++++++++----------------------
>>    hw/hyperv/meson.build |  2 +-
>>    2 files changed, 26 insertions(+), 26 deletions(-)
>>
>> diff --git a/hw/hyperv/vmbus.c b/hw/hyperv/vmbus.c
>> index 12a7dc43128..109ac319caf 100644
>> --- a/hw/hyperv/vmbus.c
>> +++ b/hw/hyperv/vmbus.c
>> @@ -18,7 +18,7 @@
>>    #include "hw/hyperv/vmbus.h"
>>    #include "hw/hyperv/vmbus-bridge.h"
>>    #include "hw/sysbus.h"
>> -#include "cpu.h"
>> +#include "exec/target_page.h"
>>    #include "trace.h"
>>    
>>    enum {
>> @@ -309,7 +309,7 @@ void vmbus_put_gpadl(VMBusGpadl *gpadl)
>>    
>>    uint32_t vmbus_gpadl_len(VMBusGpadl *gpadl)
>>    {
>> -    return gpadl->num_gfns * TARGET_PAGE_SIZE;
>> +    return gpadl->num_gfns * qemu_target_page_size();
>>    }
>>    
>>    static void gpadl_iter_init(GpadlIter *iter, VMBusGpadl *gpadl,
>> @@ -323,14 +323,14 @@ static void gpadl_iter_init(GpadlIter *iter, VMBusGpadl *gpadl,
>>    
>>    static inline void gpadl_iter_cache_unmap(GpadlIter *iter)
>>    {
>> -    uint32_t map_start_in_page = (uintptr_t)iter->map & ~TARGET_PAGE_MASK;
>> -    uint32_t io_end_in_page = ((iter->last_off - 1) & ~TARGET_PAGE_MASK) + 1;
>> +    uint32_t map_start_in_page = (uintptr_t)iter->map & ~qemu_target_page_mask();
>> +    uint32_t io_end_in_page = ((iter->last_off - 1) & ~qemu_target_page_mask()) + 1;
>>    
>>        /* mapping is only done to do non-zero amount of i/o */
>>        assert(iter->last_off > 0);
>>        assert(map_start_in_page < io_end_in_page);
>>    
>> -    dma_memory_unmap(iter->as, iter->map, TARGET_PAGE_SIZE - map_start_in_page,
>> +    dma_memory_unmap(iter->as, iter->map, qemu_target_page_size() - map_start_in_page,
>>                         iter->dir, io_end_in_page - map_start_in_page);
>>    }
>>    
>> @@ -348,17 +348,17 @@ static ssize_t gpadl_iter_io(GpadlIter *iter, void *buf, uint32_t len)
>>        assert(iter->active);
>>    
>>        while (len) {
>> -        uint32_t off_in_page = iter->off & ~TARGET_PAGE_MASK;
>> -        uint32_t pgleft = TARGET_PAGE_SIZE - off_in_page;
>> +        uint32_t off_in_page = iter->off & ~qemu_target_page_mask();
>> +        uint32_t pgleft = qemu_target_page_size() - off_in_page;
>>            uint32_t cplen = MIN(pgleft, len);
>>            void *p;
>>    
>>            /* try to reuse the cached mapping */
>>            if (iter->map) {
>>                uint32_t map_start_in_page =
>> -                (uintptr_t)iter->map & ~TARGET_PAGE_MASK;
>> -            uint32_t off_base = iter->off & ~TARGET_PAGE_MASK;
>> -            uint32_t mapped_base = (iter->last_off - 1) & ~TARGET_PAGE_MASK;
>> +                (uintptr_t)iter->map & ~qemu_target_page_mask();
>> +            uint32_t off_base = iter->off & ~qemu_target_page_mask();
>> +            uint32_t mapped_base = (iter->last_off - 1) & ~qemu_target_page_mask();
>>                if (off_base != mapped_base || off_in_page < map_start_in_page) {
>>                    gpadl_iter_cache_unmap(iter);
>>                    iter->map = NULL;
>> @@ -368,10 +368,10 @@ static ssize_t gpadl_iter_io(GpadlIter *iter, void *buf, uint32_t len)
>>            if (!iter->map) {
>>                dma_addr_t maddr;
>>                dma_addr_t mlen = pgleft;
>> -            uint32_t idx = iter->off >> TARGET_PAGE_BITS;
>> +            uint32_t idx = iter->off >> qemu_target_page_bits();
>>                assert(idx < iter->gpadl->num_gfns);
>>    
>> -            maddr = (iter->gpadl->gfns[idx] << TARGET_PAGE_BITS) | off_in_page;
>> +            maddr = (iter->gpadl->gfns[idx] << qemu_target_page_bits()) | off_in_page;
>>    
>>                iter->map = dma_memory_map(iter->as, maddr, &mlen, iter->dir,
>>                                           MEMTXATTRS_UNSPECIFIED);
>> @@ -382,7 +382,7 @@ static ssize_t gpadl_iter_io(GpadlIter *iter, void *buf, uint32_t len)
>>                }
>>            }
>>    
>> -        p = (void *)(uintptr_t)(((uintptr_t)iter->map & TARGET_PAGE_MASK) |
>> +        p = (void *)(uintptr_t)(((uintptr_t)iter->map & qemu_target_page_mask()) |
>>                    off_in_page);
>>            if (iter->dir == DMA_DIRECTION_FROM_DEVICE) {
>>                memcpy(p, buf, cplen);
>> @@ -591,9 +591,9 @@ static void ringbuf_init_common(VMBusRingBufCommon *ringbuf, VMBusGpadl *gpadl,
>>                                    uint32_t begin, uint32_t end)
>>    {
>>        ringbuf->as = as;
>> -    ringbuf->rb_addr = gpadl->gfns[begin] << TARGET_PAGE_BITS;
>> -    ringbuf->base = (begin + 1) << TARGET_PAGE_BITS;
>> -    ringbuf->len = (end - begin - 1) << TARGET_PAGE_BITS;
>> +    ringbuf->rb_addr = gpadl->gfns[begin] << qemu_target_page_bits();
>> +    ringbuf->base = (begin + 1) << qemu_target_page_bits();
>> +    ringbuf->len = (end - begin - 1) << qemu_target_page_bits();
>>        gpadl_iter_init(&ringbuf->iter, gpadl, as, dir);
>>    }
>>    
>> @@ -734,7 +734,7 @@ static int vmbus_channel_notify_guest(VMBusChannel *chan)
>>        unsigned long *int_map, mask;
>>        unsigned idx;
>>        hwaddr addr = chan->vmbus->int_page_gpa;
>> -    hwaddr len = TARGET_PAGE_SIZE / 2, dirty = 0;
>> +    hwaddr len = qemu_target_page_size() / 2, dirty = 0;
>>    
>>        trace_vmbus_channel_notify_guest(chan->id);
>>    
>> @@ -743,7 +743,7 @@ static int vmbus_channel_notify_guest(VMBusChannel *chan)
>>        }
>>    
>>        int_map = cpu_physical_memory_map(addr, &len, 1);
>> -    if (len != TARGET_PAGE_SIZE / 2) {
>> +    if (len != qemu_target_page_size() / 2) {
>>            res = -ENXIO;
>>            goto unmap;
>>        }
>> @@ -1038,14 +1038,14 @@ static int sgl_from_gpa_ranges(QEMUSGList *sgl, VMBusDevice *dev,
>>            }
>>            len -= sizeof(range);
>>    
>> -        if (range.byte_offset & TARGET_PAGE_MASK) {
>> +        if (range.byte_offset & qemu_target_page_mask()) {
>>                goto eio;
>>            }
>>    
>>            for (; range.byte_count; range.byte_offset = 0) {
>>                uint64_t paddr;
>>                uint32_t plen = MIN(range.byte_count,
>> -                                TARGET_PAGE_SIZE - range.byte_offset);
>> +                                qemu_target_page_size() - range.byte_offset);
>>    
>>                if (len < sizeof(uint64_t)) {
>>                    goto eio;
>> @@ -1055,7 +1055,7 @@ static int sgl_from_gpa_ranges(QEMUSGList *sgl, VMBusDevice *dev,
>>                    goto err;
>>                }
>>                len -= sizeof(uint64_t);
>> -            paddr <<= TARGET_PAGE_BITS;
>> +            paddr <<= qemu_target_page_bits();
>>                paddr |= range.byte_offset;
>>                range.byte_count -= plen;
>>    
>> @@ -1804,7 +1804,7 @@ static void handle_gpadl_header(VMBus *vmbus, vmbus_message_gpadl_header *msg,
>>         * anything else and simplify things greatly.
>>         */
>>        if (msg->rangecount != 1 || msg->range[0].byte_offset ||
>> -        (msg->range[0].byte_count != (num_gfns << TARGET_PAGE_BITS))) {
>> +        (msg->range[0].byte_count != (num_gfns << qemu_target_page_bits()))) {
>>            return;
>>        }
>>    
>> @@ -2240,10 +2240,10 @@ static void vmbus_signal_event(EventNotifier *e)
>>            return;
>>        }
>>    
>> -    addr = vmbus->int_page_gpa + TARGET_PAGE_SIZE / 2;
>> -    len = TARGET_PAGE_SIZE / 2;
>> +    addr = vmbus->int_page_gpa + qemu_target_page_size() / 2;
>> +    len = qemu_target_page_size() / 2;
>>        int_map = cpu_physical_memory_map(addr, &len, 1);
>> -    if (len != TARGET_PAGE_SIZE / 2) {
>> +    if (len != qemu_target_page_size() / 2) {
>>            goto unmap;
>>        }
>>    
>> diff --git a/hw/hyperv/meson.build b/hw/hyperv/meson.build
>> index f4aa0a5ada9..c855fdcf04c 100644
>> --- a/hw/hyperv/meson.build
>> +++ b/hw/hyperv/meson.build
>> @@ -1,6 +1,6 @@
>>    specific_ss.add(when: 'CONFIG_HYPERV', if_true: files('hyperv.c'))
>>    specific_ss.add(when: 'CONFIG_HYPERV_TESTDEV', if_true: files('hyperv_testdev.c'))
>> -specific_ss.add(when: 'CONFIG_VMBUS', if_true: files('vmbus.c'))
>> +system_ss.add(when: 'CONFIG_VMBUS', if_true: files('vmbus.c'))
>>    specific_ss.add(when: 'CONFIG_SYNDBG', if_true: files('syndbg.c'))
>>    specific_ss.add(when: 'CONFIG_HV_BALLOON', if_true: files('hv-balloon.c', 'hv-balloon-page_range_tree.c', 'hv-balloon-our_range_memslots.c'))
>>    system_ss.add(when: 'CONFIG_HV_BALLOON', if_false: files('hv-balloon-stub.c'))
> 

Re: [PATCH 3/7] hw/hyperv/vmbus: common compilation unit

[Maciej S. Szmigiero]

Hi Pierrick,

On 6.03.2025 23:59, Pierrick Bouvier wrote:
> Hi Maciej,
> 
> we are currently working toward building a single QEMU binary able to emulate all architectures, and one prerequisite is to remove duplication of compilation units (some are duplicated per target now, because of compile time defines).
> 
> So the work here is to replace those compile time defines with runtime functions instead, so the same code can be used for various architectures.

But this is x86-only where AFAIK page size is always 4k
so is TARGET_PAGE_SIZE going away eventually or is the
QEMU policy to get rid of it at the first opportunity?
  
> Is it more clear for you?

Thanks,
Maciej

> On 3/6/25 12:29, Maciej S. Szmigiero wrote:
>> On 6.03.2025 07:41, Pierrick Bouvier wrote:
>>> Replace TARGET_PAGE.* by runtime calls.
>>
>> Seems like this patch subject/title is not aligned
>> well with its content, or a least incomplete.
>>
>> Also, could you provide more detailed information
>> why TARGET_PAGE_SIZE is getting replaced by
>> qemu_target_page_size() please?
>>
>> I don't see such information in the cover letter either.
>>
>> Thanks,
>> Maciej
>>> Signed-off-by: Pierrick Bouvier <pierrick.bouvier@linaro.org>
>>> ---
>>>    hw/hyperv/vmbus.c     | 50 +++++++++++++++++++++----------------------
>>>    hw/hyperv/meson.build |  2 +-
>>>    2 files changed, 26 insertions(+), 26 deletions(-)
>>>
>>> diff --git a/hw/hyperv/vmbus.c b/hw/hyperv/vmbus.c
>>> index 12a7dc43128..109ac319caf 100644
>>> --- a/hw/hyperv/vmbus.c
>>> +++ b/hw/hyperv/vmbus.c
>>> @@ -18,7 +18,7 @@
>>>    #include "hw/hyperv/vmbus.h"
>>>    #include "hw/hyperv/vmbus-bridge.h"
>>>    #include "hw/sysbus.h"
>>> -#include "cpu.h"
>>> +#include "exec/target_page.h"
>>>    #include "trace.h"
>>>    enum {
>>> @@ -309,7 +309,7 @@ void vmbus_put_gpadl(VMBusGpadl *gpadl)
>>>    uint32_t vmbus_gpadl_len(VMBusGpadl *gpadl)
>>>    {
>>> -    return gpadl->num_gfns * TARGET_PAGE_SIZE;
>>> +    return gpadl->num_gfns * qemu_target_page_size();
>>>    }
>>>    static void gpadl_iter_init(GpadlIter *iter, VMBusGpadl *gpadl,
>>> @@ -323,14 +323,14 @@ static void gpadl_iter_init(GpadlIter *iter, VMBusGpadl *gpadl,
>>>    static inline void gpadl_iter_cache_unmap(GpadlIter *iter)
>>>    {
>>> -    uint32_t map_start_in_page = (uintptr_t)iter->map & ~TARGET_PAGE_MASK;
>>> -    uint32_t io_end_in_page = ((iter->last_off - 1) & ~TARGET_PAGE_MASK) + 1;
>>> +    uint32_t map_start_in_page = (uintptr_t)iter->map & ~qemu_target_page_mask();
>>> +    uint32_t io_end_in_page = ((iter->last_off - 1) & ~qemu_target_page_mask()) + 1;
>>>        /* mapping is only done to do non-zero amount of i/o */
>>>        assert(iter->last_off > 0);
>>>        assert(map_start_in_page < io_end_in_page);
>>> -    dma_memory_unmap(iter->as, iter->map, TARGET_PAGE_SIZE - map_start_in_page,
>>> +    dma_memory_unmap(iter->as, iter->map, qemu_target_page_size() - map_start_in_page,
>>>                         iter->dir, io_end_in_page - map_start_in_page);
>>>    }
>>> @@ -348,17 +348,17 @@ static ssize_t gpadl_iter_io(GpadlIter *iter, void *buf, uint32_t len)
>>>        assert(iter->active);
>>>        while (len) {
>>> -        uint32_t off_in_page = iter->off & ~TARGET_PAGE_MASK;
>>> -        uint32_t pgleft = TARGET_PAGE_SIZE - off_in_page;
>>> +        uint32_t off_in_page = iter->off & ~qemu_target_page_mask();
>>> +        uint32_t pgleft = qemu_target_page_size() - off_in_page;
>>>            uint32_t cplen = MIN(pgleft, len);
>>>            void *p;
>>>            /* try to reuse the cached mapping */
>>>            if (iter->map) {
>>>                uint32_t map_start_in_page =
>>> -                (uintptr_t)iter->map & ~TARGET_PAGE_MASK;
>>> -            uint32_t off_base = iter->off & ~TARGET_PAGE_MASK;
>>> -            uint32_t mapped_base = (iter->last_off - 1) & ~TARGET_PAGE_MASK;
>>> +                (uintptr_t)iter->map & ~qemu_target_page_mask();
>>> +            uint32_t off_base = iter->off & ~qemu_target_page_mask();
>>> +            uint32_t mapped_base = (iter->last_off - 1) & ~qemu_target_page_mask();
>>>                if (off_base != mapped_base || off_in_page < map_start_in_page) {
>>>                    gpadl_iter_cache_unmap(iter);
>>>                    iter->map = NULL;
>>> @@ -368,10 +368,10 @@ static ssize_t gpadl_iter_io(GpadlIter *iter, void *buf, uint32_t len)
>>>            if (!iter->map) {
>>>                dma_addr_t maddr;
>>>                dma_addr_t mlen = pgleft;
>>> -            uint32_t idx = iter->off >> TARGET_PAGE_BITS;
>>> +            uint32_t idx = iter->off >> qemu_target_page_bits();
>>>                assert(idx < iter->gpadl->num_gfns);
>>> -            maddr = (iter->gpadl->gfns[idx] << TARGET_PAGE_BITS) | off_in_page;
>>> +            maddr = (iter->gpadl->gfns[idx] << qemu_target_page_bits()) | off_in_page;
>>>                iter->map = dma_memory_map(iter->as, maddr, &mlen, iter->dir,
>>>                                           MEMTXATTRS_UNSPECIFIED);
>>> @@ -382,7 +382,7 @@ static ssize_t gpadl_iter_io(GpadlIter *iter, void *buf, uint32_t len)
>>>                }
>>>            }
>>> -        p = (void *)(uintptr_t)(((uintptr_t)iter->map & TARGET_PAGE_MASK) |
>>> +        p = (void *)(uintptr_t)(((uintptr_t)iter->map & qemu_target_page_mask()) |
>>>                    off_in_page);
>>>            if (iter->dir == DMA_DIRECTION_FROM_DEVICE) {
>>>                memcpy(p, buf, cplen);
>>> @@ -591,9 +591,9 @@ static void ringbuf_init_common(VMBusRingBufCommon *ringbuf, VMBusGpadl *gpadl,
>>>                                    uint32_t begin, uint32_t end)
>>>    {
>>>        ringbuf->as = as;
>>> -    ringbuf->rb_addr = gpadl->gfns[begin] << TARGET_PAGE_BITS;
>>> -    ringbuf->base = (begin + 1) << TARGET_PAGE_BITS;
>>> -    ringbuf->len = (end - begin - 1) << TARGET_PAGE_BITS;
>>> +    ringbuf->rb_addr = gpadl->gfns[begin] << qemu_target_page_bits();
>>> +    ringbuf->base = (begin + 1) << qemu_target_page_bits();
>>> +    ringbuf->len = (end - begin - 1) << qemu_target_page_bits();
>>>        gpadl_iter_init(&ringbuf->iter, gpadl, as, dir);
>>>    }
>>> @@ -734,7 +734,7 @@ static int vmbus_channel_notify_guest(VMBusChannel *chan)
>>>        unsigned long *int_map, mask;
>>>        unsigned idx;
>>>        hwaddr addr = chan->vmbus->int_page_gpa;
>>> -    hwaddr len = TARGET_PAGE_SIZE / 2, dirty = 0;
>>> +    hwaddr len = qemu_target_page_size() / 2, dirty = 0;
>>>        trace_vmbus_channel_notify_guest(chan->id);
>>> @@ -743,7 +743,7 @@ static int vmbus_channel_notify_guest(VMBusChannel *chan)
>>>        }
>>>        int_map = cpu_physical_memory_map(addr, &len, 1);
>>> -    if (len != TARGET_PAGE_SIZE / 2) {
>>> +    if (len != qemu_target_page_size() / 2) {
>>>            res = -ENXIO;
>>>            goto unmap;
>>>        }
>>> @@ -1038,14 +1038,14 @@ static int sgl_from_gpa_ranges(QEMUSGList *sgl, VMBusDevice *dev,
>>>            }
>>>            len -= sizeof(range);
>>> -        if (range.byte_offset & TARGET_PAGE_MASK) {
>>> +        if (range.byte_offset & qemu_target_page_mask()) {
>>>                goto eio;
>>>            }
>>>            for (; range.byte_count; range.byte_offset = 0) {
>>>                uint64_t paddr;
>>>                uint32_t plen = MIN(range.byte_count,
>>> -                                TARGET_PAGE_SIZE - range.byte_offset);
>>> +                                qemu_target_page_size() - range.byte_offset);
>>>                if (len < sizeof(uint64_t)) {
>>>                    goto eio;
>>> @@ -1055,7 +1055,7 @@ static int sgl_from_gpa_ranges(QEMUSGList *sgl, VMBusDevice *dev,
>>>                    goto err;
>>>                }
>>>                len -= sizeof(uint64_t);
>>> -            paddr <<= TARGET_PAGE_BITS;
>>> +            paddr <<= qemu_target_page_bits();
>>>                paddr |= range.byte_offset;
>>>                range.byte_count -= plen;
>>> @@ -1804,7 +1804,7 @@ static void handle_gpadl_header(VMBus *vmbus, vmbus_message_gpadl_header *msg,
>>>         * anything else and simplify things greatly.
>>>         */
>>>        if (msg->rangecount != 1 || msg->range[0].byte_offset ||
>>> -        (msg->range[0].byte_count != (num_gfns << TARGET_PAGE_BITS))) {
>>> +        (msg->range[0].byte_count != (num_gfns << qemu_target_page_bits()))) {
>>>            return;
>>>        }
>>> @@ -2240,10 +2240,10 @@ static void vmbus_signal_event(EventNotifier *e)
>>>            return;
>>>        }
>>> -    addr = vmbus->int_page_gpa + TARGET_PAGE_SIZE / 2;
>>> -    len = TARGET_PAGE_SIZE / 2;
>>> +    addr = vmbus->int_page_gpa + qemu_target_page_size() / 2;
>>> +    len = qemu_target_page_size() / 2;
>>>        int_map = cpu_physical_memory_map(addr, &len, 1);
>>> -    if (len != TARGET_PAGE_SIZE / 2) {
>>> +    if (len != qemu_target_page_size() / 2) {
>>>            goto unmap;
>>>        }
>>> diff --git a/hw/hyperv/meson.build b/hw/hyperv/meson.build
>>> index f4aa0a5ada9..c855fdcf04c 100644
>>> --- a/hw/hyperv/meson.build
>>> +++ b/hw/hyperv/meson.build
>>> @@ -1,6 +1,6 @@
>>>    specific_ss.add(when: 'CONFIG_HYPERV', if_true: files('hyperv.c'))
>>>    specific_ss.add(when: 'CONFIG_HYPERV_TESTDEV', if_true: files('hyperv_testdev.c'))
>>> -specific_ss.add(when: 'CONFIG_VMBUS', if_true: files('vmbus.c'))
>>> +system_ss.add(when: 'CONFIG_VMBUS', if_true: files('vmbus.c'))
>>>    specific_ss.add(when: 'CONFIG_SYNDBG', if_true: files('syndbg.c'))
>>>    specific_ss.add(when: 'CONFIG_HV_BALLOON', if_true: files('hv-balloon.c', 'hv-balloon-page_range_tree.c', 'hv-balloon-our_range_memslots.c'))
>>>    system_ss.add(when: 'CONFIG_HV_BALLOON', if_false: files('hv-balloon-stub.c'))
>>
> 

Re: [PATCH 5/7] hw/hyperv/syndbg: common compilation unit

[Maciej S. Szmigiero]

On 6.03.2025 23:56, Pierrick Bouvier wrote:
> On 3/6/25 09:58, Philippe Mathieu-Daudé wrote:
>> On 6/3/25 17:23, Pierrick Bouvier wrote:
>>> On 3/6/25 08:19, Richard Henderson wrote:
>>>> On 3/5/25 22:41, Pierrick Bouvier wrote:
>>>>> Replace TARGET_PAGE.* by runtime calls
>>>>>
>>>>> Signed-off-by: Pierrick Bouvier <pierrick.bouvier@linaro.org>
>>>>> ---
>>>>>     hw/hyperv/syndbg.c    | 7 ++++---
>>>>>     hw/hyperv/meson.build | 2 +-
>>>>>     2 files changed, 5 insertions(+), 4 deletions(-)
>>>>>
>>>>> diff --git a/hw/hyperv/syndbg.c b/hw/hyperv/syndbg.c
>>>>> index d3e39170772..f9382202ed3 100644
>>>>> --- a/hw/hyperv/syndbg.c
>>>>> +++ b/hw/hyperv/syndbg.c
>>>>> @@ -14,7 +14,7 @@
>>>>>     #include "migration/vmstate.h"
>>>>>     #include "hw/qdev-properties.h"
>>>>>     #include "hw/loader.h"
>>>>> -#include "cpu.h"
>>>>> +#include "exec/target_page.h"
>>>>>     #include "hw/hyperv/hyperv.h"
>>>>>     #include "hw/hyperv/vmbus-bridge.h"
>>>>>     #include "hw/hyperv/hyperv-proto.h"
>>>>> @@ -188,7 +188,8 @@ static uint16_t handle_recv_msg(HvSynDbg *syndbg,
>>>>> uint64_t outgpa,
>>>>>                                     uint64_t timeout, uint32_t
>>>>> *retrieved_count)
>>>>>     {
>>>>>         uint16_t ret;
>>>>> -    uint8_t data_buf[TARGET_PAGE_SIZE - UDP_PKT_HEADER_SIZE];
>>>>> +    const size_t buf_size = qemu_target_page_size() -
>>>>> UDP_PKT_HEADER_SIZE;
>>>>> +    uint8_t *data_buf = g_alloca(buf_size);
>>>>>         hwaddr out_len;
>>>>>         void *out_data;
>>>>>         ssize_t recv_byte_count;
>>>>
>>>> We've purged the code base of VLAs, and those are preferable to alloca.
>>>> Just use g_malloc and g_autofree.
>>>>
>>>
>>> I hesitated, due to potential performance considerations for people
>>> reviewing the patch. I'll switch to heap based storage.
>>
>> OTOH hyperv is x86-only, so we could do:
>>
>> #define BUFSZ (4 * KiB)
>>
>> handle_recv_msg()
>> {
>>     uint8_t data_buf[BUFSZ - UDP_PKT_HEADER_SIZE];
>>     ...
>>
>> hv_syndbg_class_init()
>> {
>>     assert(BUFSZ > qemu_target_page_size());
>>     ...
>>
>> and call it a day.
> 
> Could be possible for now yes.
> 
> Any opinion from concerned maintainers?

I think essentially hardcoding 4k pages in hyperv is okay
(with an appropriate checking/enforcement asserts() of course),
since even if this gets ported to ARM64 at some point
it is going to need *a lot* of changes anyway.

Thanks,
Maciej

Re: [PATCH 5/7] hw/hyperv/syndbg: common compilation unit

[Alex Bennée]

"Maciej S. Szmigiero" <mail@maciej.szmigiero.name> writes:

> On 6.03.2025 23:56, Pierrick Bouvier wrote:
>> On 3/6/25 09:58, Philippe Mathieu-Daudé wrote:
>>> On 6/3/25 17:23, Pierrick Bouvier wrote:
>>>> On 3/6/25 08:19, Richard Henderson wrote:
>>>>> On 3/5/25 22:41, Pierrick Bouvier wrote:
>>>>>> Replace TARGET_PAGE.* by runtime calls
>>>>>>
>>>>>> Signed-off-by: Pierrick Bouvier <pierrick.bouvier@linaro.org>
>>>>>> ---
>>>>>>     hw/hyperv/syndbg.c    | 7 ++++---
>>>>>>     hw/hyperv/meson.build | 2 +-
>>>>>>     2 files changed, 5 insertions(+), 4 deletions(-)
>>>>>>
>>>>>> diff --git a/hw/hyperv/syndbg.c b/hw/hyperv/syndbg.c
>>>>>> index d3e39170772..f9382202ed3 100644
>>>>>> --- a/hw/hyperv/syndbg.c
>>>>>> +++ b/hw/hyperv/syndbg.c
>>>>>> @@ -14,7 +14,7 @@
>>>>>>     #include "migration/vmstate.h"
>>>>>>     #include "hw/qdev-properties.h"
>>>>>>     #include "hw/loader.h"
>>>>>> -#include "cpu.h"
>>>>>> +#include "exec/target_page.h"
>>>>>>     #include "hw/hyperv/hyperv.h"
>>>>>>     #include "hw/hyperv/vmbus-bridge.h"
>>>>>>     #include "hw/hyperv/hyperv-proto.h"
>>>>>> @@ -188,7 +188,8 @@ static uint16_t handle_recv_msg(HvSynDbg *syndbg,
>>>>>> uint64_t outgpa,
>>>>>>                                     uint64_t timeout, uint32_t
>>>>>> *retrieved_count)
>>>>>>     {
>>>>>>         uint16_t ret;
>>>>>> -    uint8_t data_buf[TARGET_PAGE_SIZE - UDP_PKT_HEADER_SIZE];
>>>>>> +    const size_t buf_size = qemu_target_page_size() -
>>>>>> UDP_PKT_HEADER_SIZE;
>>>>>> +    uint8_t *data_buf = g_alloca(buf_size);
>>>>>>         hwaddr out_len;
>>>>>>         void *out_data;
>>>>>>         ssize_t recv_byte_count;
>>>>>
>>>>> We've purged the code base of VLAs, and those are preferable to alloca.
>>>>> Just use g_malloc and g_autofree.
>>>>>
>>>>
>>>> I hesitated, due to potential performance considerations for people
>>>> reviewing the patch. I'll switch to heap based storage.
>>>
>>> OTOH hyperv is x86-only, so we could do:
>>>
>>> #define BUFSZ (4 * KiB)
>>>
>>> handle_recv_msg()
>>> {
>>>     uint8_t data_buf[BUFSZ - UDP_PKT_HEADER_SIZE];
>>>     ...
>>>
>>> hv_syndbg_class_init()
>>> {
>>>     assert(BUFSZ > qemu_target_page_size());
>>>     ...
>>>
>>> and call it a day.
>> Could be possible for now yes.
>> Any opinion from concerned maintainers?
>
> I think essentially hardcoding 4k pages in hyperv is okay
> (with an appropriate checking/enforcement asserts() of course),
> since even if this gets ported to ARM64 at some point
> it is going to need *a lot* of changes anyway.

There was a talk at last years KVM Forum about porting WHPX for Windows
on Arm:

  https://kvm-forum.qemu.org/2024/Qemu_support_for_Windows_on_Arm_GgKlLjf.pdf

but am I right in thinking all the hyperv code in QEMU is about
providing guest facing enlightenments for Windows guests under KVM? I
guess no one is working on that at the moment.


>
> Thanks,
> Maciej

-- 
Alex Bennée
Virtualisation Tech Lead @ Linaro

Re: [PATCH 3/7] hw/hyperv/vmbus: common compilation unit

[Pierrick Bouvier]

On 3/7/25 03:03, Maciej S. Szmigiero wrote:
> Hi Pierrick,
> 
> On 6.03.2025 23:59, Pierrick Bouvier wrote:
>> Hi Maciej,
>>
>> we are currently working toward building a single QEMU binary able to emulate all architectures, and one prerequisite is to remove duplication of compilation units (some are duplicated per target now, because of compile time defines).
>>
>> So the work here is to replace those compile time defines with runtime functions instead, so the same code can be used for various architectures.
> 
> But this is x86-only where AFAIK page size is always 4k
> so is TARGET_PAGE_SIZE going away eventually or is the
> QEMU policy to get rid of it at the first opportunity?
>

Policy is a strong word, but this is the work we are trying to do at the 
moment. Please note that Richard is trying to convert those globally to 
runtime checks:
https://patchew.org/QEMU/20250306234108.378881-1-richard.henderson@linaro.org/

While this is not upstream, we try to move forward with what is 
available, and we can do the replacement again later.

>> Is it more clear for you?
> 
> Thanks,
> Maciej
> 
>> On 3/6/25 12:29, Maciej S. Szmigiero wrote:
>>> On 6.03.2025 07:41, Pierrick Bouvier wrote:
>>>> Replace TARGET_PAGE.* by runtime calls.
>>>
>>> Seems like this patch subject/title is not aligned
>>> well with its content, or a least incomplete.
>>>
>>> Also, could you provide more detailed information
>>> why TARGET_PAGE_SIZE is getting replaced by
>>> qemu_target_page_size() please?
>>>
>>> I don't see such information in the cover letter either.
>>>
>>> Thanks,
>>> Maciej
>>>> Signed-off-by: Pierrick Bouvier <pierrick.bouvier@linaro.org>
>>>> ---
>>>>     hw/hyperv/vmbus.c     | 50 +++++++++++++++++++++----------------------
>>>>     hw/hyperv/meson.build |  2 +-
>>>>     2 files changed, 26 insertions(+), 26 deletions(-)
>>>>
>>>> diff --git a/hw/hyperv/vmbus.c b/hw/hyperv/vmbus.c
>>>> index 12a7dc43128..109ac319caf 100644
>>>> --- a/hw/hyperv/vmbus.c
>>>> +++ b/hw/hyperv/vmbus.c
>>>> @@ -18,7 +18,7 @@
>>>>     #include "hw/hyperv/vmbus.h"
>>>>     #include "hw/hyperv/vmbus-bridge.h"
>>>>     #include "hw/sysbus.h"
>>>> -#include "cpu.h"
>>>> +#include "exec/target_page.h"
>>>>     #include "trace.h"
>>>>     enum {
>>>> @@ -309,7 +309,7 @@ void vmbus_put_gpadl(VMBusGpadl *gpadl)
>>>>     uint32_t vmbus_gpadl_len(VMBusGpadl *gpadl)
>>>>     {
>>>> -    return gpadl->num_gfns * TARGET_PAGE_SIZE;
>>>> +    return gpadl->num_gfns * qemu_target_page_size();
>>>>     }
>>>>     static void gpadl_iter_init(GpadlIter *iter, VMBusGpadl *gpadl,
>>>> @@ -323,14 +323,14 @@ static void gpadl_iter_init(GpadlIter *iter, VMBusGpadl *gpadl,
>>>>     static inline void gpadl_iter_cache_unmap(GpadlIter *iter)
>>>>     {
>>>> -    uint32_t map_start_in_page = (uintptr_t)iter->map & ~TARGET_PAGE_MASK;
>>>> -    uint32_t io_end_in_page = ((iter->last_off - 1) & ~TARGET_PAGE_MASK) + 1;
>>>> +    uint32_t map_start_in_page = (uintptr_t)iter->map & ~qemu_target_page_mask();
>>>> +    uint32_t io_end_in_page = ((iter->last_off - 1) & ~qemu_target_page_mask()) + 1;
>>>>         /* mapping is only done to do non-zero amount of i/o */
>>>>         assert(iter->last_off > 0);
>>>>         assert(map_start_in_page < io_end_in_page);
>>>> -    dma_memory_unmap(iter->as, iter->map, TARGET_PAGE_SIZE - map_start_in_page,
>>>> +    dma_memory_unmap(iter->as, iter->map, qemu_target_page_size() - map_start_in_page,
>>>>                          iter->dir, io_end_in_page - map_start_in_page);
>>>>     }
>>>> @@ -348,17 +348,17 @@ static ssize_t gpadl_iter_io(GpadlIter *iter, void *buf, uint32_t len)
>>>>         assert(iter->active);
>>>>         while (len) {
>>>> -        uint32_t off_in_page = iter->off & ~TARGET_PAGE_MASK;
>>>> -        uint32_t pgleft = TARGET_PAGE_SIZE - off_in_page;
>>>> +        uint32_t off_in_page = iter->off & ~qemu_target_page_mask();
>>>> +        uint32_t pgleft = qemu_target_page_size() - off_in_page;
>>>>             uint32_t cplen = MIN(pgleft, len);
>>>>             void *p;
>>>>             /* try to reuse the cached mapping */
>>>>             if (iter->map) {
>>>>                 uint32_t map_start_in_page =
>>>> -                (uintptr_t)iter->map & ~TARGET_PAGE_MASK;
>>>> -            uint32_t off_base = iter->off & ~TARGET_PAGE_MASK;
>>>> -            uint32_t mapped_base = (iter->last_off - 1) & ~TARGET_PAGE_MASK;
>>>> +                (uintptr_t)iter->map & ~qemu_target_page_mask();
>>>> +            uint32_t off_base = iter->off & ~qemu_target_page_mask();
>>>> +            uint32_t mapped_base = (iter->last_off - 1) & ~qemu_target_page_mask();
>>>>                 if (off_base != mapped_base || off_in_page < map_start_in_page) {
>>>>                     gpadl_iter_cache_unmap(iter);
>>>>                     iter->map = NULL;
>>>> @@ -368,10 +368,10 @@ static ssize_t gpadl_iter_io(GpadlIter *iter, void *buf, uint32_t len)
>>>>             if (!iter->map) {
>>>>                 dma_addr_t maddr;
>>>>                 dma_addr_t mlen = pgleft;
>>>> -            uint32_t idx = iter->off >> TARGET_PAGE_BITS;
>>>> +            uint32_t idx = iter->off >> qemu_target_page_bits();
>>>>                 assert(idx < iter->gpadl->num_gfns);
>>>> -            maddr = (iter->gpadl->gfns[idx] << TARGET_PAGE_BITS) | off_in_page;
>>>> +            maddr = (iter->gpadl->gfns[idx] << qemu_target_page_bits()) | off_in_page;
>>>>                 iter->map = dma_memory_map(iter->as, maddr, &mlen, iter->dir,
>>>>                                            MEMTXATTRS_UNSPECIFIED);
>>>> @@ -382,7 +382,7 @@ static ssize_t gpadl_iter_io(GpadlIter *iter, void *buf, uint32_t len)
>>>>                 }
>>>>             }
>>>> -        p = (void *)(uintptr_t)(((uintptr_t)iter->map & TARGET_PAGE_MASK) |
>>>> +        p = (void *)(uintptr_t)(((uintptr_t)iter->map & qemu_target_page_mask()) |
>>>>                     off_in_page);
>>>>             if (iter->dir == DMA_DIRECTION_FROM_DEVICE) {
>>>>                 memcpy(p, buf, cplen);
>>>> @@ -591,9 +591,9 @@ static void ringbuf_init_common(VMBusRingBufCommon *ringbuf, VMBusGpadl *gpadl,
>>>>                                     uint32_t begin, uint32_t end)
>>>>     {
>>>>         ringbuf->as = as;
>>>> -    ringbuf->rb_addr = gpadl->gfns[begin] << TARGET_PAGE_BITS;
>>>> -    ringbuf->base = (begin + 1) << TARGET_PAGE_BITS;
>>>> -    ringbuf->len = (end - begin - 1) << TARGET_PAGE_BITS;
>>>> +    ringbuf->rb_addr = gpadl->gfns[begin] << qemu_target_page_bits();
>>>> +    ringbuf->base = (begin + 1) << qemu_target_page_bits();
>>>> +    ringbuf->len = (end - begin - 1) << qemu_target_page_bits();
>>>>         gpadl_iter_init(&ringbuf->iter, gpadl, as, dir);
>>>>     }
>>>> @@ -734,7 +734,7 @@ static int vmbus_channel_notify_guest(VMBusChannel *chan)
>>>>         unsigned long *int_map, mask;
>>>>         unsigned idx;
>>>>         hwaddr addr = chan->vmbus->int_page_gpa;
>>>> -    hwaddr len = TARGET_PAGE_SIZE / 2, dirty = 0;
>>>> +    hwaddr len = qemu_target_page_size() / 2, dirty = 0;
>>>>         trace_vmbus_channel_notify_guest(chan->id);
>>>> @@ -743,7 +743,7 @@ static int vmbus_channel_notify_guest(VMBusChannel *chan)
>>>>         }
>>>>         int_map = cpu_physical_memory_map(addr, &len, 1);
>>>> -    if (len != TARGET_PAGE_SIZE / 2) {
>>>> +    if (len != qemu_target_page_size() / 2) {
>>>>             res = -ENXIO;
>>>>             goto unmap;
>>>>         }
>>>> @@ -1038,14 +1038,14 @@ static int sgl_from_gpa_ranges(QEMUSGList *sgl, VMBusDevice *dev,
>>>>             }
>>>>             len -= sizeof(range);
>>>> -        if (range.byte_offset & TARGET_PAGE_MASK) {
>>>> +        if (range.byte_offset & qemu_target_page_mask()) {
>>>>                 goto eio;
>>>>             }
>>>>             for (; range.byte_count; range.byte_offset = 0) {
>>>>                 uint64_t paddr;
>>>>                 uint32_t plen = MIN(range.byte_count,
>>>> -                                TARGET_PAGE_SIZE - range.byte_offset);
>>>> +                                qemu_target_page_size() - range.byte_offset);
>>>>                 if (len < sizeof(uint64_t)) {
>>>>                     goto eio;
>>>> @@ -1055,7 +1055,7 @@ static int sgl_from_gpa_ranges(QEMUSGList *sgl, VMBusDevice *dev,
>>>>                     goto err;
>>>>                 }
>>>>                 len -= sizeof(uint64_t);
>>>> -            paddr <<= TARGET_PAGE_BITS;
>>>> +            paddr <<= qemu_target_page_bits();
>>>>                 paddr |= range.byte_offset;
>>>>                 range.byte_count -= plen;
>>>> @@ -1804,7 +1804,7 @@ static void handle_gpadl_header(VMBus *vmbus, vmbus_message_gpadl_header *msg,
>>>>          * anything else and simplify things greatly.
>>>>          */
>>>>         if (msg->rangecount != 1 || msg->range[0].byte_offset ||
>>>> -        (msg->range[0].byte_count != (num_gfns << TARGET_PAGE_BITS))) {
>>>> +        (msg->range[0].byte_count != (num_gfns << qemu_target_page_bits()))) {
>>>>             return;
>>>>         }
>>>> @@ -2240,10 +2240,10 @@ static void vmbus_signal_event(EventNotifier *e)
>>>>             return;
>>>>         }
>>>> -    addr = vmbus->int_page_gpa + TARGET_PAGE_SIZE / 2;
>>>> -    len = TARGET_PAGE_SIZE / 2;
>>>> +    addr = vmbus->int_page_gpa + qemu_target_page_size() / 2;
>>>> +    len = qemu_target_page_size() / 2;
>>>>         int_map = cpu_physical_memory_map(addr, &len, 1);
>>>> -    if (len != TARGET_PAGE_SIZE / 2) {
>>>> +    if (len != qemu_target_page_size() / 2) {
>>>>             goto unmap;
>>>>         }
>>>> diff --git a/hw/hyperv/meson.build b/hw/hyperv/meson.build
>>>> index f4aa0a5ada9..c855fdcf04c 100644
>>>> --- a/hw/hyperv/meson.build
>>>> +++ b/hw/hyperv/meson.build
>>>> @@ -1,6 +1,6 @@
>>>>     specific_ss.add(when: 'CONFIG_HYPERV', if_true: files('hyperv.c'))
>>>>     specific_ss.add(when: 'CONFIG_HYPERV_TESTDEV', if_true: files('hyperv_testdev.c'))
>>>> -specific_ss.add(when: 'CONFIG_VMBUS', if_true: files('vmbus.c'))
>>>> +system_ss.add(when: 'CONFIG_VMBUS', if_true: files('vmbus.c'))
>>>>     specific_ss.add(when: 'CONFIG_SYNDBG', if_true: files('syndbg.c'))
>>>>     specific_ss.add(when: 'CONFIG_HV_BALLOON', if_true: files('hv-balloon.c', 'hv-balloon-page_range_tree.c', 'hv-balloon-our_range_memslots.c'))
>>>>     system_ss.add(when: 'CONFIG_HV_BALLOON', if_false: files('hv-balloon-stub.c'))
>>>
>>
> 

Re: [PATCH 5/7] hw/hyperv/syndbg: common compilation unit

[Pierrick Bouvier]

On 3/7/25 03:07, Maciej S. Szmigiero wrote:
> On 6.03.2025 23:56, Pierrick Bouvier wrote:
>> On 3/6/25 09:58, Philippe Mathieu-Daudé wrote:
>>> On 6/3/25 17:23, Pierrick Bouvier wrote:
>>>> On 3/6/25 08:19, Richard Henderson wrote:
>>>>> On 3/5/25 22:41, Pierrick Bouvier wrote:
>>>>>> Replace TARGET_PAGE.* by runtime calls
>>>>>>
>>>>>> Signed-off-by: Pierrick Bouvier <pierrick.bouvier@linaro.org>
>>>>>> ---
>>>>>>      hw/hyperv/syndbg.c    | 7 ++++---
>>>>>>      hw/hyperv/meson.build | 2 +-
>>>>>>      2 files changed, 5 insertions(+), 4 deletions(-)
>>>>>>
>>>>>> diff --git a/hw/hyperv/syndbg.c b/hw/hyperv/syndbg.c
>>>>>> index d3e39170772..f9382202ed3 100644
>>>>>> --- a/hw/hyperv/syndbg.c
>>>>>> +++ b/hw/hyperv/syndbg.c
>>>>>> @@ -14,7 +14,7 @@
>>>>>>      #include "migration/vmstate.h"
>>>>>>      #include "hw/qdev-properties.h"
>>>>>>      #include "hw/loader.h"
>>>>>> -#include "cpu.h"
>>>>>> +#include "exec/target_page.h"
>>>>>>      #include "hw/hyperv/hyperv.h"
>>>>>>      #include "hw/hyperv/vmbus-bridge.h"
>>>>>>      #include "hw/hyperv/hyperv-proto.h"
>>>>>> @@ -188,7 +188,8 @@ static uint16_t handle_recv_msg(HvSynDbg *syndbg,
>>>>>> uint64_t outgpa,
>>>>>>                                      uint64_t timeout, uint32_t
>>>>>> *retrieved_count)
>>>>>>      {
>>>>>>          uint16_t ret;
>>>>>> -    uint8_t data_buf[TARGET_PAGE_SIZE - UDP_PKT_HEADER_SIZE];
>>>>>> +    const size_t buf_size = qemu_target_page_size() -
>>>>>> UDP_PKT_HEADER_SIZE;
>>>>>> +    uint8_t *data_buf = g_alloca(buf_size);
>>>>>>          hwaddr out_len;
>>>>>>          void *out_data;
>>>>>>          ssize_t recv_byte_count;
>>>>>
>>>>> We've purged the code base of VLAs, and those are preferable to alloca.
>>>>> Just use g_malloc and g_autofree.
>>>>>
>>>>
>>>> I hesitated, due to potential performance considerations for people
>>>> reviewing the patch. I'll switch to heap based storage.
>>>
>>> OTOH hyperv is x86-only, so we could do:
>>>
>>> #define BUFSZ (4 * KiB)
>>>
>>> handle_recv_msg()
>>> {
>>>      uint8_t data_buf[BUFSZ - UDP_PKT_HEADER_SIZE];
>>>      ...
>>>
>>> hv_syndbg_class_init()
>>> {
>>>      assert(BUFSZ > qemu_target_page_size());
>>>      ...
>>>
>>> and call it a day.
>>
>> Could be possible for now yes.
>>
>> Any opinion from concerned maintainers?
> 
> I think essentially hardcoding 4k pages in hyperv is okay
> (with an appropriate checking/enforcement asserts() of course),
> since even if this gets ported to ARM64 at some point
> it is going to need *a lot* of changes anyway.
> 

Ok, I will hardcode this following Philippe suggestion then.

> Thanks,
> Maciej
> 

Re: [PATCH 5/7] hw/hyperv/syndbg: common compilation unit

[Maciej S. Szmigiero]

On 7.03.2025 15:50, Alex Bennée wrote:
> "Maciej S. Szmigiero" <mail@maciej.szmigiero.name> writes:
> 
>> On 6.03.2025 23:56, Pierrick Bouvier wrote:
>>> On 3/6/25 09:58, Philippe Mathieu-Daudé wrote:
>>>> On 6/3/25 17:23, Pierrick Bouvier wrote:
>>>>> On 3/6/25 08:19, Richard Henderson wrote:
>>>>>> On 3/5/25 22:41, Pierrick Bouvier wrote:
>>>>>>> Replace TARGET_PAGE.* by runtime calls
>>>>>>>
>>>>>>> Signed-off-by: Pierrick Bouvier <pierrick.bouvier@linaro.org>
>>>>>>> ---
>>>>>>>      hw/hyperv/syndbg.c    | 7 ++++---
>>>>>>>      hw/hyperv/meson.build | 2 +-
>>>>>>>      2 files changed, 5 insertions(+), 4 deletions(-)
>>>>>>>
>>>>>>> diff --git a/hw/hyperv/syndbg.c b/hw/hyperv/syndbg.c
>>>>>>> index d3e39170772..f9382202ed3 100644
>>>>>>> --- a/hw/hyperv/syndbg.c
>>>>>>> +++ b/hw/hyperv/syndbg.c
>>>>>>> @@ -14,7 +14,7 @@
>>>>>>>      #include "migration/vmstate.h"
>>>>>>>      #include "hw/qdev-properties.h"
>>>>>>>      #include "hw/loader.h"
>>>>>>> -#include "cpu.h"
>>>>>>> +#include "exec/target_page.h"
>>>>>>>      #include "hw/hyperv/hyperv.h"
>>>>>>>      #include "hw/hyperv/vmbus-bridge.h"
>>>>>>>      #include "hw/hyperv/hyperv-proto.h"
>>>>>>> @@ -188,7 +188,8 @@ static uint16_t handle_recv_msg(HvSynDbg *syndbg,
>>>>>>> uint64_t outgpa,
>>>>>>>                                      uint64_t timeout, uint32_t
>>>>>>> *retrieved_count)
>>>>>>>      {
>>>>>>>          uint16_t ret;
>>>>>>> -    uint8_t data_buf[TARGET_PAGE_SIZE - UDP_PKT_HEADER_SIZE];
>>>>>>> +    const size_t buf_size = qemu_target_page_size() -
>>>>>>> UDP_PKT_HEADER_SIZE;
>>>>>>> +    uint8_t *data_buf = g_alloca(buf_size);
>>>>>>>          hwaddr out_len;
>>>>>>>          void *out_data;
>>>>>>>          ssize_t recv_byte_count;
>>>>>>
>>>>>> We've purged the code base of VLAs, and those are preferable to alloca.
>>>>>> Just use g_malloc and g_autofree.
>>>>>>
>>>>>
>>>>> I hesitated, due to potential performance considerations for people
>>>>> reviewing the patch. I'll switch to heap based storage.
>>>>
>>>> OTOH hyperv is x86-only, so we could do:
>>>>
>>>> #define BUFSZ (4 * KiB)
>>>>
>>>> handle_recv_msg()
>>>> {
>>>>      uint8_t data_buf[BUFSZ - UDP_PKT_HEADER_SIZE];
>>>>      ...
>>>>
>>>> hv_syndbg_class_init()
>>>> {
>>>>      assert(BUFSZ > qemu_target_page_size());
>>>>      ...
>>>>
>>>> and call it a day.
>>> Could be possible for now yes.
>>> Any opinion from concerned maintainers?
>>
>> I think essentially hardcoding 4k pages in hyperv is okay
>> (with an appropriate checking/enforcement asserts() of course),
>> since even if this gets ported to ARM64 at some point
>> it is going to need *a lot* of changes anyway.
> 
> There was a talk at last years KVM Forum about porting WHPX for Windows
> on Arm:
> 
>    https://kvm-forum.qemu.org/2024/Qemu_support_for_Windows_on_Arm_GgKlLjf.pdf
> 
> but am I right in thinking all the hyperv code in QEMU is about
> providing guest facing enlightenments for Windows guests under KVM? I
> guess no one is working on that at the moment.

Yes, I think that talk was about running QEMU on actual Hyper-V hypervisor,
while the code in hw/hyperv is about QEMU emulating Hyper-V interfaces to
QEMU's VMs.

CONFIG_HYPERV in hw/hyperv/Kconfig is even marked as "depends on KVM".

Thanks,
Maciej