| Message ID | 20250307151543.8156-6-philmd@linaro.org |
|---|---|
| State | New |
| Headers | show |
| Series | hw/virtio: Build virtio-mem.c once | expand |
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);
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 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.
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 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!
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);
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(-)