Re: [RFC PATCH-for-8.0 09/10] hw/virtio: Extract vhost_user_ram_slots_max() to vhost-user-target.c

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

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é <philmd@linaro.org>
>> ---
>> 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 0000000000..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_NREGIONS    8
>>    #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,7 @@ static void vhost_user_state_destroy(gpointer data)
>>    
>>    bool vhost_user_init(VhostUserState *user, CharBackend *chr, Error **errp)
>>    {
>> +    assert(vhost_user_ram_slots_max() <= VHOST_USER_MAX_RAM_SLOTS);
>>        if (user->chr) {
>>            error_setg(errp, "Cannot initialize vhost-user state");
>>            return false;
>> diff --git a/include/hw/virtio/vhost-user.h b/include/hw/virtio/vhost-user.h
>> index 191216a74f..e13584ade8 100644
>> --- a/include/hw/virtio/vhost-user.h
>> +++ b/include/hw/virtio/vhost-user.h
>> @@ -86,4 +86,11 @@ void vhost_user_async_close(DeviceState *d,
>>                                CharBackend *chardev, struct vhost_dev *vhost,
>>                                vu_async_close_fn cb);
>>    
>> +/**
>> + * vhost_user_ram_slots_max()
>> + *
>> + * Return: maximum number of RAM slots supported by the target hardware plaform.
>> + */
>> +unsigned int vhost_user_ram_slots_max(void);
>> +
>>    #endif
>