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

[Pierrick Bouvier <pierrick.bouvier@linaro.org>]

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'))
>>>
>>
>