Re: [PATCH v2 1/2] hw/arm/virt: Consolidate GIC finalize logic

[Cornelia Huck]

On Wed, Dec 21 2022, Alexander Graf <agraf@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 <agraf@csgraf.de>
>
> ---
>
> v1 -> v2:
>
>   - leave VIRT_GIC_VERSION defines intact, we need them for MADT generation
> ---
>  hw/arm/virt.c         | 199 ++++++++++++++++++++++--------------------
>  include/hw/arm/virt.h |  15 ++--
>  2 files changed, 113 insertions(+), 101 deletions(-)
>
> diff --git a/hw/arm/virt.c b/hw/arm/virt.c
> index 04eb6c201d..7b54387958 100644
> --- a/hw/arm/virt.c
> +++ b/hw/arm/virt.c
> @@ -1820,6 +1820,85 @@ 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()) {
> +            /* For KVM, -cpu host means -cpu max */

That should rather be "gic-version" instead of "cpu", I guess?

> +            return finalize_gic_version_do(accel_name, VIRT_GIC_VERSION_MAX,
> +                                           gics_supported, max_cpus);
> +        }
> +
> +        error_report("gic-version=host requires KVM");
> +        exit(1);
> +        break;

I wonder whether reporting an error for !kvm_enabled() and falling
through would be more readable.

...although all of this is already more readable than before :)

> +    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;
> +    }