Re: [PATCH v4 5/6] hw/arm/virt: kvm: Restructure finalize_gic_version()

[Andrew Jones]

On Wed, Mar 11, 2020 at 12:16:25PM +0100, Eric Auger wrote:
> Restructure the finalize_gic_version with switch cases and
> clearly separate the following cases:
> 
> - KVM mode / in-kernel irqchip
> - KVM mode / userspace irqchip
> - TCG mode
> 
> In KVM mode / in-kernel irqchip , we explictly check whether
> the chosen version is supported by the host. If the end-user
> explicitly sets v2/v3 and this is not supported by the host,
> then the user gets an explicit error message. Note that for
> old kernels where the CREATE_DEVICE ioctl doesn't exist then
> we will now fail if the user specifically asked for gicv2,
> where previously we (probably) would have succeeded.
> 
> In KVM mode / userspace irqchip we immediatly output an error
> in case the end-user explicitly selected v3. Also we warn the
> end-user about the unexpected usage of gic-version=host in
> that case as only userspace GICv2 is supported.
> 
> Signed-off-by: Eric Auger <address@hidden>
> 
> ---
> 
> v3 -> v4:
> - introduce a separate switch case for KVM mode / userspace
>   irqchip
> - don't probe the host GIC version in case of userspace
>   GIC
> - add error/warning messages in v3/host mode in userspace
>   mode
> - mention old kernel case with explicit v2/v3 regression
> 
> v2 -> v3:
> - explictly list V2 and V3 in the switch/case
> - fix indent
> ---
>  hw/arm/virt.c | 89 +++++++++++++++++++++++++++++++++++++++------------
>  1 file changed, 68 insertions(+), 21 deletions(-)
> 
> diff --git a/hw/arm/virt.c b/hw/arm/virt.c
> index f798b6ec10..f5ff2d9006 100644
> --- a/hw/arm/virt.c
> +++ b/hw/arm/virt.c
> @@ -1543,33 +1543,80 @@ static void virt_set_memmap(VirtMachineState *vms)
>   */
>  static void finalize_gic_version(VirtMachineState *vms)
>  {
> -    if (vms->gic_version == VIRT_GIC_VERSION_HOST ||
> -        vms->gic_version == VIRT_GIC_VERSION_MAX) {
> -        if (!kvm_enabled()) {
> -            if (vms->gic_version == VIRT_GIC_VERSION_HOST) {
> -                error_report("gic-version=host requires KVM");
> -                exit(1);
> -            } else {
> -                /* "max": currently means 3 for TCG */
> -                vms->gic_version = VIRT_GIC_VERSION_3;
> -            }
> -        } else {
> -            int probe_bitmap = kvm_arm_vgic_probe();
> +    if (kvm_enabled()) {
> +        int probe_bitmap;
>  
> -            if (!probe_bitmap) {
> +        if (!kvm_irqchip_in_kernel()) {
> +            switch (vms->gic_version) {
> +            case VIRT_GIC_VERSION_HOST:
> +                warn_report(
> +                    "gic-version=host not relevant with kernel-irqchip=off "
> +                     "as only userspace GICv2 is supported. Using v2 ...");
> +                return;
> +            case VIRT_GIC_VERSION_MAX:
> +            case VIRT_GIC_VERSION_NOSEL:
> +                vms->gic_version = VIRT_GIC_VERSION_2;
> +                return;
> +            case VIRT_GIC_VERSION_2:
> +                return;
> +            case VIRT_GIC_VERSION_3:
>                  error_report(
> -                    "Unable to determine GIC version supported by host");
> +                    "gic-version=3 is not supported with 
> kernel-irqchip=off");
>                  exit(1);
> -            } else {
> -                if (probe_bitmap & KVM_ARM_VGIC_V3) {
> -                    vms->gic_version = VIRT_GIC_VERSION_3;
> -                } else {
> -                    vms->gic_version = VIRT_GIC_VERSION_2;
> -                }
>              }
>          }
> -    } else if (vms->gic_version == VIRT_GIC_VERSION_NOSEL) {
> +
> +        probe_bitmap = kvm_arm_vgic_probe();
> +        if (!probe_bitmap) {
> +            error_report(
> +                "Unable to determine GIC version supported by host");

No need for this line break

> +            exit(1);
> +        }
> +
> +        switch (vms->gic_version) {
> +        case VIRT_GIC_VERSION_HOST:
> +        case VIRT_GIC_VERSION_MAX:
> +            if (probe_bitmap & KVM_ARM_VGIC_V3) {
> +                vms->gic_version = VIRT_GIC_VERSION_3;
> +            } else {
> +                vms->gic_version = VIRT_GIC_VERSION_2;
> +            }
> +            return;
> +        case VIRT_GIC_VERSION_NOSEL:
> +            vms->gic_version = VIRT_GIC_VERSION_2;
> +            break;
> +        case VIRT_GIC_VERSION_2:
> +        case VIRT_GIC_VERSION_3:
> +            break;
> +        }
> +
> +        /* Check chosen version is effectively supported by the host */
> +        if (vms->gic_version == VIRT_GIC_VERSION_2 &&
> +            !(probe_bitmap & KVM_ARM_VGIC_V2)) {
> +            error_report("host does not support in-kernel GICv2 emulation");
> +            exit(1);
> +        } else if (vms->gic_version == VIRT_GIC_VERSION_3 &&
> +                   !(probe_bitmap & KVM_ARM_VGIC_V3)) {
> +            error_report("host does not support in-kernel GICv3 emulation");
> +            exit(1);
> +        }
> +        return;
> +    }
> +
> +    /* TCG mode */
> +    switch (vms->gic_version) {
> +    case VIRT_GIC_VERSION_NOSEL:
>          vms->gic_version = VIRT_GIC_VERSION_2;
> +        break;
> +    case VIRT_GIC_VERSION_MAX:
> +        vms->gic_version = VIRT_GIC_VERSION_3;
> +        break;
> +    case VIRT_GIC_VERSION_HOST:
> +        error_report("gic-version=host requires KVM");
> +        exit(1);
> +    case VIRT_GIC_VERSION_2:
> +    case VIRT_GIC_VERSION_3:
> +        break;
>      }
>  }
>  
> -- 
> 2.20.1
>

Other than the nit,

Reviewed-by: Andrew Jones <address@hidden>