On Fri, Dec 23 2022, Alexander Graf <ag...@csgraf.de> wrote:
> Up to now, the finalize_gic_version() code open coded what is essentially
> a support bitmap match between host/emulation environment and desired
> target GIC type.
>
> This open coding leads to undesirable side effects. For example, a VM with
> KVM and -smp 10 will automatically choose GICv3 while the same command
> line with TCG will stay on GICv2 and fail the launch.
>
> This patch combines the TCG and KVM matching code paths by making
> everything a 2 pass process. First, we determine which GIC versions the
> current environment is able to support, then we go through a single
> state machine to determine which target GIC mode that means for us.
>
> After this patch, the only user noticable changes should be consolidated
> error messages as well as TCG -M virt supporting -smp > 8 automatically.
>
> Signed-off-by: Alexander Graf <ag...@csgraf.de>
>
> ---
>
> v1 -> v2:
>
> - Leave VIRT_GIC_VERSION defines intact, we need them for MADT generation
>
> v2 -> v3:
>
> - Fix comment
> - Flip kvm-enabled logic for host around
> ---
> hw/arm/virt.c | 198 ++++++++++++++++++++++--------------------
> include/hw/arm/virt.h | 15 ++--
> 2 files changed, 112 insertions(+), 101 deletions(-)
>
> diff --git a/hw/arm/virt.c b/hw/arm/virt.c
> index ea2413a0ba..6d27f044fe 100644
> --- a/hw/arm/virt.c
> +++ b/hw/arm/virt.c
> @@ -1820,6 +1820,84 @@ static void virt_set_memmap(VirtMachineState *vms, int
> pa_bits)
> }
> }
>
> +static VirtGICType finalize_gic_version_do(const char *accel_name,
> + VirtGICType gic_version,
> + int gics_supported,
> + unsigned int max_cpus)
> +{
> + /* Convert host/max/nosel to GIC version number */
> + switch (gic_version) {
> + case VIRT_GIC_VERSION_HOST:
> + if (!kvm_enabled()) {
> + error_report("gic-version=host requires KVM");
> + exit(1);
> + }
> +
> + /* For KVM, gic-version=host means gic-version=max */
> + return finalize_gic_version_do(accel_name, VIRT_GIC_VERSION_MAX,
> + gics_supported, max_cpus);
I think I'd still rather use /* fallthrough */ here, but let's leave
that decision to the maintainers.
In any case,
Reviewed-by: Cornelia Huck <coh...@redhat.com>
[As an aside, we have a QEMU_FALLTHROUGH #define that maps to
__attribute__((fallthrough)) if available, but unlike the Linux kernel,
we didn't bother to convert everything to use it in QEMU. Should we?
Would using the attribute give us some extra benefits?]
> + case VIRT_GIC_VERSION_MAX:
> + if (gics_supported & VIRT_GIC_VERSION_4_MASK) {
> + gic_version = VIRT_GIC_VERSION_4;
> + } else if (gics_supported & VIRT_GIC_VERSION_3_MASK) {
> + gic_version = VIRT_GIC_VERSION_3;
> + } else {
> + gic_version = VIRT_GIC_VERSION_2;
> + }
> + break;
> + case VIRT_GIC_VERSION_NOSEL:
> + if ((gics_supported & VIRT_GIC_VERSION_2_MASK) &&
> + max_cpus <= GIC_NCPU) {
> + gic_version = VIRT_GIC_VERSION_2;
> + } else if (gics_supported & VIRT_GIC_VERSION_3_MASK) {
> + /*
> + * in case the host does not support v2 emulation or
> + * the end-user requested more than 8 VCPUs we now default
> + * to v3. In any case defaulting to v2 would be broken.
> + */
> + gic_version = VIRT_GIC_VERSION_3;
> + } else if (max_cpus > GIC_NCPU) {
> + error_report("%s only supports GICv2 emulation but more than 8 "
> + "vcpus are requested", accel_name);
> + exit(1);
> + }
> + break;
> + case VIRT_GIC_VERSION_2:
> + case VIRT_GIC_VERSION_3:
> + case VIRT_GIC_VERSION_4:
> + break;
> + }
