Restructure the finalize_gic_version with switch cases and, in
KVM mode, 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.

Signed-off-by: Eric Auger <eric.auger@redhat.com>
---
 hw/arm/virt.c | 76 +++++++++++++++++++++++++++++++++++----------------
 1 file changed, 52 insertions(+), 24 deletions(-)

diff --git a/hw/arm/virt.c b/hw/arm/virt.c
index eb8c57c85e..610bfc9ee9 100644
--- a/hw/arm/virt.c
+++ b/hw/arm/virt.c
@@ -1542,33 +1542,61 @@ 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 = kvm_arm_vgic_probe();
 
-            if (!probe_bitmap) {
-                error_report(
-                    "Unable to determine GIC version supported by host");
-                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;
-                }
-            }
+        if (!probe_bitmap) {
+            error_report("Unable to determine GIC version supported by host");
+            exit(1);
         }
-    } else if (vms->gic_version == VIRT_GIC_VERSION_NOSEL) {
+
+        switch (vms->gic_version) {
+        case VIRT_GIC_VERSION_NOSEL:
+            vms->gic_version = VIRT_GIC_VERSION_2;
+            break;
+        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_2:
+        case VIRT_GIC_VERSION_3:
+            break;
+        }
+
+        if (!kvm_irqchip_in_kernel()) {
+            return;
+        }
+
+        /* 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);
+    default: /* explicit V2/V3 are left untouched */
+        break;
     }
 }
 
-- 
2.20.1

On 3/1/20 2:40 AM, Eric Auger wrote:
> +        /* 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);
> +        }

Indentation is wrong here.

> +    case VIRT_GIC_VERSION_HOST:
> +        error_report("gic-version=host requires KVM");
> +        exit(1);
> +    default: /* explicit V2/V3 are left untouched */
> +        break;
>      }

I'd prefer to just list V2 and V3 here explicitly, instead of the default.
It'll be nicer with gic_version changed to an enum.

With those changes,
Reviewed-by: Richard Henderson <richard.henderson@linaro.org>


r~

Hi Richard,
On 3/1/20 6:56 PM, Richard Henderson wrote:
> On 3/1/20 2:40 AM, Eric Auger wrote:
>> +        /* 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);
>> +        }
> 
> Indentation is wrong here.
OK
> 
>> +    case VIRT_GIC_VERSION_HOST:
>> +        error_report("gic-version=host requires KVM");
>> +        exit(1);
>> +    default: /* explicit V2/V3 are left untouched */
>> +        break;
>>      }
> 
> I'd prefer to just list V2 and V3 here explicitly, instead of the default.
> It'll be nicer with gic_version changed to an enum.
OK I will respin with those changes.

Thank you for the review!

Best Regards

Eric
> 
> With those changes,
> Reviewed-by: Richard Henderson <richard.henderson@linaro.org>
> 
> 
> r~
>