diff mbox series

[v2,5/7] hw/virtio/virtio-mem: Convert VIRTIO_MEM_USABLE_EXTENT to runtime

Message ID 20250307223949.54040-6-philmd@linaro.org
State New
Headers show
Series hw/virtio: Build virtio-mem.c once | expand

Commit Message

Philippe Mathieu-Daudé March 7, 2025, 10:39 p.m. UTC 
Use qemu_arch_available() to check at runtime if a target
architecture is built in.

Consider the maximum extent size of any architecture built in.

Signed-off-by: Philippe Mathieu-Daudé <philmd@linaro.org>
---
 hw/virtio/virtio-mem.c | 28 ++++++++++++++++++++--------
 1 file changed, 20 insertions(+), 8 deletions(-)

Comments

Philippe Mathieu-Daudé March 8, 2025, 5:41 a.m. UTC | #1 
On 7/3/25 23:39, Philippe Mathieu-Daudé wrote:
> Use qemu_arch_available() to check at runtime if a target
> architecture is built in.
> 
> Consider the maximum extent size of any architecture built in.
> 
> Signed-off-by: Philippe Mathieu-Daudé <philmd@linaro.org>
> ---
>   hw/virtio/virtio-mem.c | 28 ++++++++++++++++++++--------
>   1 file changed, 20 insertions(+), 8 deletions(-)


> @@ -170,13 +171,24 @@ 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)
> +{
> +    uint64_t size = 0;
> +
> +    assert(qemu_arch_available(QEMU_ARCH_ARM | QEMU_ARCH_I386 | QEMU_ARCH_S390X));

I'm not sure this assertion is doing what I thought it'd do.

For example, building with --target-list=aarch64-softmmu,riscv32-softmmu,
this device is now linked in. However, riscv32 machines won't be able
to plug it until they allow TYPE_VIRTIO_MD_PCI in some of their
HotplugHandlerClass handlers. Still I'd like to catch this case here
to avoid bad surprises.

> +    /*
> +     * FIXME: We should use the maximum of instantiated vCPUs ARCH, but
> +     *        for now it is easier to take the maximum of any ARCH built in.
> +     */
> +    if (qemu_arch_available(QEMU_ARCH_I386 | QEMU_ARCH_S390X)) {
> +        size = MAX(size, 2 * 128 * MiB);
> +    }
> +    if (qemu_arch_available(QEMU_ARCH_ARM)) {
> +        size = MAX(size, 2 * 512 * MiB);
> +    }
> +
> +    return size;
> +}
Pierrick Bouvier March 8, 2025, 6:57 p.m. UTC | #2 
On 3/7/25 21:41, Philippe Mathieu-Daudé wrote:
> On 7/3/25 23:39, Philippe Mathieu-Daudé wrote:
>> Use qemu_arch_available() to check at runtime if a target
>> architecture is built in.
>>
>> Consider the maximum extent size of any architecture built in.
>>
>> Signed-off-by: Philippe Mathieu-Daudé <philmd@linaro.org>
>> ---
>>    hw/virtio/virtio-mem.c | 28 ++++++++++++++++++++--------
>>    1 file changed, 20 insertions(+), 8 deletions(-)
> 
> 
>> @@ -170,13 +171,24 @@ 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)
>> +{
>> +    uint64_t size = 0;
>> +
>> +    assert(qemu_arch_available(QEMU_ARCH_ARM | QEMU_ARCH_I386 | QEMU_ARCH_S390X));
> 
> I'm not sure this assertion is doing what I thought it'd do.
> 
> For example, building with --target-list=aarch64-softmmu,riscv32-softmmu,
> this device is now linked in. However, riscv32 machines won't be able
> to plug it until they allow TYPE_VIRTIO_MD_PCI in some of their
> HotplugHandlerClass handlers. Still I'd like to catch this case here
> to avoid bad surprises.
> 

With the single binary, a lot of devices will be linked in, even though 
they won't be used. There is no specific very specific here, and 
definitely something we can tackle later, once we are able to link a 
single binary.

>> +    /*
>> +     * FIXME: We should use the maximum of instantiated vCPUs ARCH, but
>> +     *        for now it is easier to take the maximum of any ARCH built in.
>> +     */
>> +    if (qemu_arch_available(QEMU_ARCH_I386 | QEMU_ARCH_S390X)) {
>> +        size = MAX(size, 2 * 128 * MiB);
>> +    }
>> +    if (qemu_arch_available(QEMU_ARCH_ARM)) {
>> +        size = MAX(size, 2 * 512 * MiB);
>> +    }
>> +
>> +    return size;
>> +}
>
diff mbox series

Patch

diff --git a/hw/virtio/virtio-mem.c b/hw/virtio/virtio-mem.c
index 5f57eccbb66..6ff9dab0f66 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,24 @@  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)
+{
+    uint64_t size = 0;
+
+    assert(qemu_arch_available(QEMU_ARCH_ARM | QEMU_ARCH_I386 | QEMU_ARCH_S390X));
+    /*
+     * FIXME: We should use the maximum of instantiated vCPUs ARCH, but
+     *        for now it is easier to take the maximum of any ARCH built in.
+     */
+    if (qemu_arch_available(QEMU_ARCH_I386 | QEMU_ARCH_S390X)) {
+        size = MAX(size, 2 * 128 * MiB);
+    }
+    if (qemu_arch_available(QEMU_ARCH_ARM)) {
+        size = MAX(size, 2 * 512 * MiB);
+    }
+
+    return size;
+}
 
 static bool virtio_mem_is_busy(void)
 {
@@ -721,7 +733,7 @@  static void virtio_mem_resize_usable_region(VirtIOMEM *vmem,
                                             bool can_shrink)
 {
     uint64_t newsize = MIN(memory_region_size(&vmem->memdev->mr),
-                           requested_size + VIRTIO_MEM_USABLE_EXTENT);
+                           requested_size + virtio_mem_usable_extent_size());
 
     /* The usable region size always has to be multiples of the block size. */
     newsize = QEMU_ALIGN_UP(newsize, vmem->block_size);