Re: [PATCH 5/7] hw/virtio/virtio-mem: Convert VIRTIO_MEM_USABLE_EXTENT to runtime

[David Hildenbrand <david@redhat.com>]

On 07.03.25 17:49, Philippe Mathieu-Daudé wrote:
> On 7/3/25 17:38, Alex Bennée wrote:
>> Philippe Mathieu-Daudé <philmd@linaro.org> writes:
>>
>>> Use qemu_arch_available() to check at runtime if a target
>>> architecture is built in.
>>>
>>> Signed-off-by: Philippe Mathieu-Daudé <philmd@linaro.org>
>>> ---
>>>    hw/virtio/virtio-mem.c | 20 ++++++++++++--------
>>>    1 file changed, 12 insertions(+), 8 deletions(-)
>>>
>>> diff --git a/hw/virtio/virtio-mem.c b/hw/virtio/virtio-mem.c
>>> index 5f57eccbb66..8c40042108c 100644
>>> --- a/hw/virtio/virtio-mem.c
>>> +++ b/hw/virtio/virtio-mem.c
>>> @@ -15,6 +15,7 @@
>>>    #include "qemu/cutils.h"
>>>    #include "qemu/error-report.h"
>>>    #include "qemu/units.h"
>>> +#include "system/arch_init.h"
>>>    #include "system/numa.h"
>>>    #include "system/system.h"
>>>    #include "system/reset.h"
>>> @@ -170,13 +171,16 @@ static bool virtio_mem_has_shared_zeropage(RAMBlock *rb)
>>>     * necessary (as the section size can change). But it's more likely that the
>>>     * section size will rather get smaller and not bigger over time.
>>>     */
>>> -#if defined(TARGET_X86_64) || defined(TARGET_I386) || defined(TARGET_S390X)
>>> -#define VIRTIO_MEM_USABLE_EXTENT (2 * (128 * MiB))
>>> -#elif defined(TARGET_ARM)
>>> -#define VIRTIO_MEM_USABLE_EXTENT (2 * (512 * MiB))
>>> -#else
>>> -#error VIRTIO_MEM_USABLE_EXTENT not defined
>>> -#endif
>>> +static uint64_t virtio_mem_usable_extent_size(void)
>>> +{
>>> +    if (qemu_arch_available(QEMU_ARCH_I386 | QEMU_ARCH_S390X)) {
>>> +        return 2 * 128 * MiB;
>>> +    } else if (qemu_arch_available(QEMU_ARCH_ARM)) {
>>> +        return 2 * 512 * MiB;
>>> +    } else {
>>> +        g_assert_not_reached();
>>> +    }
>>> +}
>>
>> What happens if/when we have multiple arches available? Won't we want to
>> know which CPU the virtio-mem device is attached to or do we take the
>> minimal value over the whole system?

We should take the maximum value here, not the minimum.

> 
> "per attached vcpu" is how I was previously considering this problem,
> but IIUC from the discussions with Pierrick, we should consider single
> binary as a first step before heterogeneous emulation.

It would be related to the machine/vcpu, yes.

Taking the maximum over all available arches is easier; it's a pure 
optimization.

-- 
Cheers,

David / dhildenb