Re: [RFC PATCH-for-8.0 09/10] hw/virtio: Extract vhost_user_ram_slots_max() to vhost-user-target.c
On 10/4/25 19:29, Pierrick Bouvier wrote: On 4/10/25 10:21, Philippe Mathieu-Daudé wrote: On 10/4/25 16:36, Pierrick Bouvier wrote: On 4/10/25 05:14, Philippe Mathieu-Daudé wrote: Hi Pierrick, On 13/12/22 00:05, Philippe Mathieu-Daudé wrote: The current definition of VHOST_USER_MAX_RAM_SLOTS is target specific. By converting this definition to a runtime vhost_user_ram_slots_max() helper declared in a target specific unit, we can have the rest of vhost-user.c target independent. To avoid variable length array or using the heap to store arrays of vhost_user_ram_slots_max() elements, we simply declare an array of the biggest VHOST_USER_MAX_RAM_SLOTS, and each target uses up to vhost_user_ram_slots_max() elements of it. Ensure arrays are big enough by adding an assertion in vhost_user_init(). Signed-off-by: Philippe Mathieu-Daudé --- RFC: Should I add VHOST_USER_MAX_RAM_SLOTS to vhost-user.h or create an internal header for it? --- hw/virtio/meson.build | 1 + hw/virtio/vhost-user-target.c | 29 + hw/virtio/vhost-user.c | 26 +- include/hw/virtio/vhost-user.h | 7 +++ 4 files changed, 42 insertions(+), 21 deletions(-) create mode 100644 hw/virtio/vhost-user-target.c diff --git a/hw/virtio/meson.build b/hw/virtio/meson.build index eb7ee8ea92..bf7e35fa8a 100644 --- a/hw/virtio/meson.build +++ b/hw/virtio/meson.build @@ -11,6 +11,7 @@ if have_vhost specific_virtio_ss.add(files('vhost.c', 'vhost-backend.c', 'vhost-iova-tree.c')) if have_vhost_user specific_virtio_ss.add(files('vhost-user.c')) + specific_virtio_ss.add(files('vhost-user-target.c')) endif if have_vhost_vdpa specific_virtio_ss.add(files('vhost-vdpa.c', 'vhost-shadow- virtqueue.c')) diff --git a/hw/virtio/vhost-user-target.c b/hw/virtio/vhost-user- target.c new file mode 100644 index 00..6a0d0f53d0 --- /dev/null +++ b/hw/virtio/vhost-user-target.c @@ -0,0 +1,29 @@ +/* + * vhost-user target-specific helpers + * + * Copyright (c) 2013 Virtual Open Systems Sarl. + * + * SPDX-License-Identifier: GPL-2.0-or-later + */ + +#include "qemu/osdep.h" +#include "hw/virtio/vhost-user.h" + +#if defined(TARGET_X86) || defined(TARGET_X86_64) || \ + defined(TARGET_ARM) || defined(TARGET_ARM_64) +#include "hw/acpi/acpi.h" +#elif defined(TARGET_PPC) || defined(TARGET_PPC64) +#include "hw/ppc/spapr.h" +#endif + +unsigned int vhost_user_ram_slots_max(void) +{ +#if defined(TARGET_X86) || defined(TARGET_X86_64) || \ + defined(TARGET_ARM) || defined(TARGET_ARM_64) + return ACPI_MAX_RAM_SLOTS; +#elif defined(TARGET_PPC) || defined(TARGET_PPC64) + return SPAPR_MAX_RAM_SLOTS; +#else + return 512; Should vhost_user_ram_slots_max be another TargetInfo field? I don't think so, it would be better to transform the existing function in something like: switch (target_current()) { case TARGET_X86: case TARGET_ARM: case TARGET_X86_64: case TARGET_ARM_64: return ACPI_MAX_RAM_SLOTS; case TARGET PPC: case TARGET PPC64: return SPAPR_MAX_RAM_SLOTS; default: return 512; } Clever, I like it, thanks! It's a pattern we can reuse in all places where it'll be needed. It's better if we keep in TargetInfo only global information, that is used through all the codebase, and not specifics about a given subsystem/device/file. By the way, TARGET_ARM_64 is probably TARGET_AARCH64. Correct, it has been fixed by Akihiko: commit 744734ccc9eff28394a453de462b2a155f364118 Author: Akihiko Odaki Date: Mon Jan 9 15:31:30 2023 +0900 vhost-user: Correct a reference of TARGET_AARCH64 Presumably TARGET_ARM_64 should be a mistake of TARGET_AARCH64. Signed-off-by: Akihiko Odaki Message-Id: <20230109063130.81296-1-akihiko.od...@daynix.com> Fixes: 27598393a2 ("Lift max memory slots limit imposed by vhost-user") Reviewed-by: Philippe Mathieu-Daudé Reviewed-by: Alex Bennée Reviewed-by: Michael S. Tsirkin Signed-off-by: Michael S. Tsirkin diff --git a/hw/virtio/vhost-user.c b/hw/virtio/vhost-user.c index d9ce0501b2c..6c79da953b3 100644 --- a/hw/virtio/vhost-user.c +++ b/hw/virtio/vhost-user.c @@ -48,7 +48,7 @@ * hardware plaform. */ #if defined(TARGET_X86) || defined(TARGET_X86_64) || \ -defined(TARGET_ARM) || defined(TARGET_ARM_64) +defined(TARGET_ARM) || defined(TARGET_AARCH64) #include "hw/acpi/acpi.h" #define VHOST_USER_MAX_RAM_SLOTS ACPI_MAX_RAM_SLOTS
Re: [RFC PATCH-for-8.0 09/10] hw/virtio: Extract vhost_user_ram_slots_max() to vhost-user-target.c
On 4/10/25 10:21, Philippe Mathieu-Daudé wrote: On 10/4/25 16:36, Pierrick Bouvier wrote: On 4/10/25 05:14, Philippe Mathieu-Daudé wrote: Hi Pierrick, On 13/12/22 00:05, Philippe Mathieu-Daudé wrote: The current definition of VHOST_USER_MAX_RAM_SLOTS is target specific. By converting this definition to a runtime vhost_user_ram_slots_max() helper declared in a target specific unit, we can have the rest of vhost-user.c target independent. To avoid variable length array or using the heap to store arrays of vhost_user_ram_slots_max() elements, we simply declare an array of the biggest VHOST_USER_MAX_RAM_SLOTS, and each target uses up to vhost_user_ram_slots_max() elements of it. Ensure arrays are big enough by adding an assertion in vhost_user_init(). Signed-off-by: Philippe Mathieu-Daudé --- RFC: Should I add VHOST_USER_MAX_RAM_SLOTS to vhost-user.h or create an internal header for it? --- hw/virtio/meson.build | 1 + hw/virtio/vhost-user-target.c | 29 + hw/virtio/vhost-user.c | 26 +- include/hw/virtio/vhost-user.h | 7 +++ 4 files changed, 42 insertions(+), 21 deletions(-) create mode 100644 hw/virtio/vhost-user-target.c diff --git a/hw/virtio/meson.build b/hw/virtio/meson.build index eb7ee8ea92..bf7e35fa8a 100644 --- a/hw/virtio/meson.build +++ b/hw/virtio/meson.build @@ -11,6 +11,7 @@ if have_vhost specific_virtio_ss.add(files('vhost.c', 'vhost-backend.c', 'vhost-iova-tree.c')) if have_vhost_user specific_virtio_ss.add(files('vhost-user.c')) + specific_virtio_ss.add(files('vhost-user-target.c')) endif if have_vhost_vdpa specific_virtio_ss.add(files('vhost-vdpa.c', 'vhost-shadow- virtqueue.c')) diff --git a/hw/virtio/vhost-user-target.c b/hw/virtio/vhost-user- target.c new file mode 100644 index 00..6a0d0f53d0 --- /dev/null +++ b/hw/virtio/vhost-user-target.c @@ -0,0 +1,29 @@ +/* + * vhost-user target-specific helpers + * + * Copyright (c) 2013 Virtual Open Systems Sarl. + * + * SPDX-License-Identifier: GPL-2.0-or-later + */ + +#include "qemu/osdep.h" +#include "hw/virtio/vhost-user.h" + +#if defined(TARGET_X86) || defined(TARGET_X86_64) || \ + defined(TARGET_ARM) || defined(TARGET_ARM_64) +#include "hw/acpi/acpi.h" +#elif defined(TARGET_PPC) || defined(TARGET_PPC64) +#include "hw/ppc/spapr.h" +#endif + +unsigned int vhost_user_ram_slots_max(void) +{ +#if defined(TARGET_X86) || defined(TARGET_X86_64) || \ + defined(TARGET_ARM) || defined(TARGET_ARM_64) + return ACPI_MAX_RAM_SLOTS; +#elif defined(TARGET_PPC) || defined(TARGET_PPC64) + return SPAPR_MAX_RAM_SLOTS; +#else + return 512; Should vhost_user_ram_slots_max be another TargetInfo field? I don't think so, it would be better to transform the existing function in something like: switch (target_current()) { case TARGET_X86: case TARGET_ARM: case TARGET_X86_64: case TARGET_ARM_64: return ACPI_MAX_RAM_SLOTS; case TARGET PPC: case TARGET PPC64: return SPAPR_MAX_RAM_SLOTS; default: return 512; } Clever, I like it, thanks! It's a pattern we can reuse in all places where it'll be needed. It's better if we keep in TargetInfo only global information, that is used through all the codebase, and not specifics about a given subsystem/device/file. By the way, TARGET_ARM_64 is probably TARGET_AARCH64.
Re: [RFC PATCH-for-8.0 09/10] hw/virtio: Extract vhost_user_ram_slots_max() to vhost-user-target.c
On 10/4/25 16:36, Pierrick Bouvier wrote: On 4/10/25 05:14, Philippe Mathieu-Daudé wrote: Hi Pierrick, On 13/12/22 00:05, Philippe Mathieu-Daudé wrote: The current definition of VHOST_USER_MAX_RAM_SLOTS is target specific. By converting this definition to a runtime vhost_user_ram_slots_max() helper declared in a target specific unit, we can have the rest of vhost-user.c target independent. To avoid variable length array or using the heap to store arrays of vhost_user_ram_slots_max() elements, we simply declare an array of the biggest VHOST_USER_MAX_RAM_SLOTS, and each target uses up to vhost_user_ram_slots_max() elements of it. Ensure arrays are big enough by adding an assertion in vhost_user_init(). Signed-off-by: Philippe Mathieu-Daudé --- RFC: Should I add VHOST_USER_MAX_RAM_SLOTS to vhost-user.h or create an internal header for it? --- hw/virtio/meson.build | 1 + hw/virtio/vhost-user-target.c | 29 + hw/virtio/vhost-user.c | 26 +- include/hw/virtio/vhost-user.h | 7 +++ 4 files changed, 42 insertions(+), 21 deletions(-) create mode 100644 hw/virtio/vhost-user-target.c diff --git a/hw/virtio/meson.build b/hw/virtio/meson.build index eb7ee8ea92..bf7e35fa8a 100644 --- a/hw/virtio/meson.build +++ b/hw/virtio/meson.build @@ -11,6 +11,7 @@ if have_vhost specific_virtio_ss.add(files('vhost.c', 'vhost-backend.c', 'vhost-iova-tree.c')) if have_vhost_user specific_virtio_ss.add(files('vhost-user.c')) + specific_virtio_ss.add(files('vhost-user-target.c')) endif if have_vhost_vdpa specific_virtio_ss.add(files('vhost-vdpa.c', 'vhost-shadow- virtqueue.c')) diff --git a/hw/virtio/vhost-user-target.c b/hw/virtio/vhost-user- target.c new file mode 100644 index 00..6a0d0f53d0 --- /dev/null +++ b/hw/virtio/vhost-user-target.c @@ -0,0 +1,29 @@ +/* + * vhost-user target-specific helpers + * + * Copyright (c) 2013 Virtual Open Systems Sarl. + * + * SPDX-License-Identifier: GPL-2.0-or-later + */ + +#include "qemu/osdep.h" +#include "hw/virtio/vhost-user.h" + +#if defined(TARGET_X86) || defined(TARGET_X86_64) || \ + defined(TARGET_ARM) || defined(TARGET_ARM_64) +#include "hw/acpi/acpi.h" +#elif defined(TARGET_PPC) || defined(TARGET_PPC64) +#include "hw/ppc/spapr.h" +#endif + +unsigned int vhost_user_ram_slots_max(void) +{ +#if defined(TARGET_X86) || defined(TARGET_X86_64) || \ + defined(TARGET_ARM) || defined(TARGET_ARM_64) + return ACPI_MAX_RAM_SLOTS; +#elif defined(TARGET_PPC) || defined(TARGET_PPC64) + return SPAPR_MAX_RAM_SLOTS; +#else + return 512; Should vhost_user_ram_slots_max be another TargetInfo field? I don't think so, it would be better to transform the existing function in something like: switch (target_current()) { case TARGET_X86: case TARGET_ARM: case TARGET_X86_64: case TARGET_ARM_64: return ACPI_MAX_RAM_SLOTS; case TARGET PPC: case TARGET PPC64: return SPAPR_MAX_RAM_SLOTS; default: return 512; } Clever, I like it, thanks!
Re: [RFC PATCH-for-8.0 09/10] hw/virtio: Extract vhost_user_ram_slots_max() to vhost-user-target.c
On 4/10/25 05:14, Philippe Mathieu-Daudé wrote: Hi Pierrick, On 13/12/22 00:05, Philippe Mathieu-Daudé wrote: The current definition of VHOST_USER_MAX_RAM_SLOTS is target specific. By converting this definition to a runtime vhost_user_ram_slots_max() helper declared in a target specific unit, we can have the rest of vhost-user.c target independent. To avoid variable length array or using the heap to store arrays of vhost_user_ram_slots_max() elements, we simply declare an array of the biggest VHOST_USER_MAX_RAM_SLOTS, and each target uses up to vhost_user_ram_slots_max() elements of it. Ensure arrays are big enough by adding an assertion in vhost_user_init(). Signed-off-by: Philippe Mathieu-Daudé --- RFC: Should I add VHOST_USER_MAX_RAM_SLOTS to vhost-user.h or create an internal header for it? --- hw/virtio/meson.build | 1 + hw/virtio/vhost-user-target.c | 29 + hw/virtio/vhost-user.c | 26 +- include/hw/virtio/vhost-user.h | 7 +++ 4 files changed, 42 insertions(+), 21 deletions(-) create mode 100644 hw/virtio/vhost-user-target.c diff --git a/hw/virtio/meson.build b/hw/virtio/meson.build index eb7ee8ea92..bf7e35fa8a 100644 --- a/hw/virtio/meson.build +++ b/hw/virtio/meson.build @@ -11,6 +11,7 @@ if have_vhost specific_virtio_ss.add(files('vhost.c', 'vhost-backend.c', 'vhost-iova-tree.c')) if have_vhost_user specific_virtio_ss.add(files('vhost-user.c')) +specific_virtio_ss.add(files('vhost-user-target.c')) endif if have_vhost_vdpa specific_virtio_ss.add(files('vhost-vdpa.c', 'vhost-shadow-virtqueue.c')) diff --git a/hw/virtio/vhost-user-target.c b/hw/virtio/vhost-user-target.c new file mode 100644 index 00..6a0d0f53d0 --- /dev/null +++ b/hw/virtio/vhost-user-target.c @@ -0,0 +1,29 @@ +/* + * vhost-user target-specific helpers + * + * Copyright (c) 2013 Virtual Open Systems Sarl. + * + * SPDX-License-Identifier: GPL-2.0-or-later + */ + +#include "qemu/osdep.h" +#include "hw/virtio/vhost-user.h" + +#if defined(TARGET_X86) || defined(TARGET_X86_64) || \ +defined(TARGET_ARM) || defined(TARGET_ARM_64) +#include "hw/acpi/acpi.h" +#elif defined(TARGET_PPC) || defined(TARGET_PPC64) +#include "hw/ppc/spapr.h" +#endif + +unsigned int vhost_user_ram_slots_max(void) +{ +#if defined(TARGET_X86) || defined(TARGET_X86_64) || \ +defined(TARGET_ARM) || defined(TARGET_ARM_64) +return ACPI_MAX_RAM_SLOTS; +#elif defined(TARGET_PPC) || defined(TARGET_PPC64) +return SPAPR_MAX_RAM_SLOTS; +#else +return 512; Should vhost_user_ram_slots_max be another TargetInfo field? I don't think so, it would be better to transform the existing function in something like: switch (target_current()) { case TARGET_X86: case TARGET_ARM: case TARGET_X86_64: case TARGET_ARM_64: return ACPI_MAX_RAM_SLOTS; case TARGET PPC: case TARGET PPC64: return SPAPR_MAX_RAM_SLOTS; default: return 512; } We should not add anything possible to TargetInfo, just for the sake of it. Especially becomes it's hard to follow values set per architecture. In a case like this, a switch is much more readable and located in one place. With a generated jump table, it's quite efficient also. In my opinion, it's another proof we need to have TARGET_X, and target_X() available at runtime. +#endif +} diff --git a/hw/virtio/vhost-user.c b/hw/virtio/vhost-user.c index 8f635844af..21fc176725 100644 --- a/hw/virtio/vhost-user.c +++ b/hw/virtio/vhost-user.c @@ -41,24 +41,7 @@ #define VHOST_MEMORY_BASELINE_NREGIONS8 #define VHOST_USER_F_PROTOCOL_FEATURES 30 #define VHOST_USER_SLAVE_MAX_FDS 8 - -/* - * Set maximum number of RAM slots supported to - * the maximum number supported by the target - * hardware plaform. - */ -#if defined(TARGET_X86) || defined(TARGET_X86_64) || \ -defined(TARGET_ARM) || defined(TARGET_ARM_64) -#include "hw/acpi/acpi.h" -#define VHOST_USER_MAX_RAM_SLOTS ACPI_MAX_RAM_SLOTS - -#elif defined(TARGET_PPC) || defined(TARGET_PPC64) -#include "hw/ppc/spapr.h" -#define VHOST_USER_MAX_RAM_SLOTS SPAPR_MAX_RAM_SLOTS - -#else #define VHOST_USER_MAX_RAM_SLOTS 512 -#endif /* * Maximum size of virtio device config space @@ -935,7 +918,7 @@ static int vhost_user_add_remove_regions(struct vhost_dev *dev, if (track_ramblocks) { memcpy(u->postcopy_client_bases, shadow_pcb, - sizeof(uint64_t) * VHOST_USER_MAX_RAM_SLOTS); + sizeof(uint64_t) * vhost_user_ram_slots_max()); /* * Now we've registered this with the postcopy code, we ack to the * client, because now we're in the position to be able to deal with @@ -956,7 +939,7 @@ static int vhost_user_add_remove_regions(struct vhost_dev *dev, err: if (track_ramblocks) { memcpy(u->postcopy_client_bases, shadow_pcb, - sizeof(uint64_t) * VHOST_USER_MAX_RAM_SLOTS); +
Re: [RFC PATCH-for-8.0 09/10] hw/virtio: Extract vhost_user_ram_slots_max() to vhost-user-target.c
Hi Pierrick, On 13/12/22 00:05, Philippe Mathieu-Daudé wrote: The current definition of VHOST_USER_MAX_RAM_SLOTS is target specific. By converting this definition to a runtime vhost_user_ram_slots_max() helper declared in a target specific unit, we can have the rest of vhost-user.c target independent. To avoid variable length array or using the heap to store arrays of vhost_user_ram_slots_max() elements, we simply declare an array of the biggest VHOST_USER_MAX_RAM_SLOTS, and each target uses up to vhost_user_ram_slots_max() elements of it. Ensure arrays are big enough by adding an assertion in vhost_user_init(). Signed-off-by: Philippe Mathieu-Daudé --- RFC: Should I add VHOST_USER_MAX_RAM_SLOTS to vhost-user.h or create an internal header for it? --- hw/virtio/meson.build | 1 + hw/virtio/vhost-user-target.c | 29 + hw/virtio/vhost-user.c | 26 +- include/hw/virtio/vhost-user.h | 7 +++ 4 files changed, 42 insertions(+), 21 deletions(-) create mode 100644 hw/virtio/vhost-user-target.c diff --git a/hw/virtio/meson.build b/hw/virtio/meson.build index eb7ee8ea92..bf7e35fa8a 100644 --- a/hw/virtio/meson.build +++ b/hw/virtio/meson.build @@ -11,6 +11,7 @@ if have_vhost specific_virtio_ss.add(files('vhost.c', 'vhost-backend.c', 'vhost-iova-tree.c')) if have_vhost_user specific_virtio_ss.add(files('vhost-user.c')) +specific_virtio_ss.add(files('vhost-user-target.c')) endif if have_vhost_vdpa specific_virtio_ss.add(files('vhost-vdpa.c', 'vhost-shadow-virtqueue.c')) diff --git a/hw/virtio/vhost-user-target.c b/hw/virtio/vhost-user-target.c new file mode 100644 index 00..6a0d0f53d0 --- /dev/null +++ b/hw/virtio/vhost-user-target.c @@ -0,0 +1,29 @@ +/* + * vhost-user target-specific helpers + * + * Copyright (c) 2013 Virtual Open Systems Sarl. + * + * SPDX-License-Identifier: GPL-2.0-or-later + */ + +#include "qemu/osdep.h" +#include "hw/virtio/vhost-user.h" + +#if defined(TARGET_X86) || defined(TARGET_X86_64) || \ +defined(TARGET_ARM) || defined(TARGET_ARM_64) +#include "hw/acpi/acpi.h" +#elif defined(TARGET_PPC) || defined(TARGET_PPC64) +#include "hw/ppc/spapr.h" +#endif + +unsigned int vhost_user_ram_slots_max(void) +{ +#if defined(TARGET_X86) || defined(TARGET_X86_64) || \ +defined(TARGET_ARM) || defined(TARGET_ARM_64) +return ACPI_MAX_RAM_SLOTS; +#elif defined(TARGET_PPC) || defined(TARGET_PPC64) +return SPAPR_MAX_RAM_SLOTS; +#else +return 512; Should vhost_user_ram_slots_max be another TargetInfo field? +#endif +} diff --git a/hw/virtio/vhost-user.c b/hw/virtio/vhost-user.c index 8f635844af..21fc176725 100644 --- a/hw/virtio/vhost-user.c +++ b/hw/virtio/vhost-user.c @@ -41,24 +41,7 @@ #define VHOST_MEMORY_BASELINE_NREGIONS8 #define VHOST_USER_F_PROTOCOL_FEATURES 30 #define VHOST_USER_SLAVE_MAX_FDS 8 - -/* - * Set maximum number of RAM slots supported to - * the maximum number supported by the target - * hardware plaform. - */ -#if defined(TARGET_X86) || defined(TARGET_X86_64) || \ -defined(TARGET_ARM) || defined(TARGET_ARM_64) -#include "hw/acpi/acpi.h" -#define VHOST_USER_MAX_RAM_SLOTS ACPI_MAX_RAM_SLOTS - -#elif defined(TARGET_PPC) || defined(TARGET_PPC64) -#include "hw/ppc/spapr.h" -#define VHOST_USER_MAX_RAM_SLOTS SPAPR_MAX_RAM_SLOTS - -#else #define VHOST_USER_MAX_RAM_SLOTS 512 -#endif /* * Maximum size of virtio device config space @@ -935,7 +918,7 @@ static int vhost_user_add_remove_regions(struct vhost_dev *dev, if (track_ramblocks) { memcpy(u->postcopy_client_bases, shadow_pcb, - sizeof(uint64_t) * VHOST_USER_MAX_RAM_SLOTS); + sizeof(uint64_t) * vhost_user_ram_slots_max()); /* * Now we've registered this with the postcopy code, we ack to the * client, because now we're in the position to be able to deal with @@ -956,7 +939,7 @@ static int vhost_user_add_remove_regions(struct vhost_dev *dev, err: if (track_ramblocks) { memcpy(u->postcopy_client_bases, shadow_pcb, - sizeof(uint64_t) * VHOST_USER_MAX_RAM_SLOTS); + sizeof(uint64_t) * vhost_user_ram_slots_max()); } return ret; @@ -1030,7 +1013,7 @@ static int vhost_user_set_mem_table_postcopy(struct vhost_dev *dev, } memset(u->postcopy_client_bases, 0, - sizeof(uint64_t) * VHOST_USER_MAX_RAM_SLOTS); + sizeof(uint64_t) * vhost_user_ram_slots_max()); /* * They're in the same order as the regions that were sent @@ -2169,7 +2152,7 @@ static int vhost_user_backend_init(struct vhost_dev *dev, void *opaque, return -EINVAL; } -u->user->memory_slots = MIN(ram_slots, VHOST_USER_MAX_RAM_SLOTS); +u->user->memory_slots = MIN(ram_slots, vhost_user_ram_slots_max()); } } @@ -2649,6 +2632,