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 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é <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;
>> }
> 
> 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.