| Message ID | 20250306064118.3879213-6-pierrick.bouvier@linaro.org |
|---|---|
| State | New |
| Headers | show |
| Series | hw/hyperv: remove duplication compilation units | expand |
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~
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~
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.
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?
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'))
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(-)