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();
+ }
+}
static bool virtio_mem_is_busy(void)
{
@@ -721,7 +725,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);
--
2.47.1
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?
>
> static bool virtio_mem_is_busy(void)
> {
> @@ -721,7 +725,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);
--
Alex Bennée
Virtualisation Tech Lead @ Linaro
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?
"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.
If you think the minimal value is good enough, then that'd be my
preferred choice, as the simplest to implement.
>
>>
>> static bool virtio_mem_is_busy(void)
>> {
>> @@ -721,7 +725,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);
>
On 3/7/25 08: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?
>
> "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.
>
I think it's safe to assume only a single arch is enable for now, in the
context of the single binary.
A thing we could do is introduce qemu_arch_heterogenenous_emulation(),
that returns false for now. And assert this in places that will need to
be changed. So spots that will need refactoring will already be flagged
in the codebase.
Reviewed-by: Pierrick Bouvier <pierrick.bouvier@linaro.org>
> If you think the minimal value is good enough, then that'd be my
> preferred choice, as the simplest to implement.
>
>>
>>>
>>> static bool virtio_mem_is_busy(void)
>>> {
>>> @@ -721,7 +725,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);
>>
>
On 7/3/25 20:28, Pierrick Bouvier wrote:
> On 3/7/25 08: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?
>>
>> "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.
>>
>
> I think it's safe to assume only a single arch is enable for now, in the
> context of the single binary.
> A thing we could do is introduce qemu_arch_heterogenenous_emulation(),
> that returns false for now. And assert this in places that will need to
> be changed. So spots that will need refactoring will already be flagged
> in the codebase.
Yes, after some discussion with Markus I started a branch adding such
macro. I didn't posted it so far waiting to show more realistic changes
w.r.t. heterogeneous emulation, to not add code that could bitrot.
Since we might be at a pivot position, I'll consider rebase and post it.
> Reviewed-by: Pierrick Bouvier <pierrick.bouvier@linaro.org>
Thanks!
On 3/7/25 08:49, Philippe Mathieu-Daudé wrote:
>>> @@ -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?
>
> "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.
>
> If you think the minimal value is good enough, then that'd be my
> preferred choice, as the simplest to implement.
Indeed, you have already done so; see above.
Reviewed-by: Richard Henderson <richard.henderson@linaro.org>
r~
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
© 2016 - 2025 Red Hat, Inc.