[PATCH v3 0/2] hw/arm/virt: Handle HVF in finalize_gic_version()

[PATCH v3 0/2] hw/arm/virt: Handle HVF in finalize_gic_version()

[Alexander Graf]

The finalize_gic_version() function tries to determine which GIC version
the current accelerator / host combination supports. During the initial
HVF porting efforts, I didn't realize that I also had to touch this
function. Then Zenghui brought up this function as reply to my HVF GICv3
enablement patch - and boy it is a mess.

This patch set cleans up all of the GIC finalization so that we can
easily plug HVF in and also hopefully will have a better time extending
it in the future. As second step, it explicitly adds HVF support and
fails loudly for any unsupported accelerators.

Alex

v1 -> v2:

  - Leave VIRT_GIC_VERSION defines intact, we need them for MADT generation
  - Include TCG header for tcg_enabled()

v2 -> v3:

  - Fix comment
  - Flip kvm-enabled logic for host around

Alexander Graf (2):
  hw/arm/virt: Consolidate GIC finalize logic
  hw/arm/virt: Make accels in GIC finalize logic explicit

 hw/arm/virt.c         | 200 ++++++++++++++++++++++--------------------
 include/hw/arm/virt.h |  15 ++--
 2 files changed, 115 insertions(+), 100 deletions(-)

-- 
2.37.1 (Apple Git-137.1)

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

[Alexander Graf]

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

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);
+    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;
+    }
+
+    /* Check chosen version is effectively supported */
+    switch (gic_version) {
+    case VIRT_GIC_VERSION_2:
+        if (!(gics_supported & VIRT_GIC_VERSION_2_MASK)) {
+            error_report("%s does not support GICv2 emulation", accel_name);
+            exit(1);
+        }
+        break;
+    case VIRT_GIC_VERSION_3:
+        if (!(gics_supported & VIRT_GIC_VERSION_3_MASK)) {
+            error_report("%s does not support GICv3 emulation", accel_name);
+            exit(1);
+        }
+        break;
+    case VIRT_GIC_VERSION_4:
+        if (!(gics_supported & VIRT_GIC_VERSION_4_MASK)) {
+            error_report("%s does not support GICv4 emulation, is virtualization=on?",
+                         accel_name);
+            exit(1);
+        }
+        break;
+    default:
+        error_report("logic error in finalize_gic_version");
+        exit(1);
+        break;
+    }
+
+    return gic_version;
+}
+
 /*
  * finalize_gic_version - Determines the final gic_version
  * according to the gic-version property
@@ -1828,118 +1906,46 @@ static void virt_set_memmap(VirtMachineState *vms, int pa_bits)
  */
 static void finalize_gic_version(VirtMachineState *vms)
 {
+    const char *accel_name = current_accel_name();
     unsigned int max_cpus = MACHINE(vms)->smp.max_cpus;
+    int gics_supported = 0;
 
-    if (kvm_enabled()) {
-        int probe_bitmap;
+    /* Determine which GIC versions the current environment supports */
+    if (kvm_enabled() && kvm_irqchip_in_kernel()) {
+        int probe_bitmap = kvm_arm_vgic_probe();
 
-        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(
-                    "gic-version=3 is not supported with kernel-irqchip=off");
-                exit(1);
-            case VIRT_GIC_VERSION_4:
-                error_report(
-                    "gic-version=4 is not supported with kernel-irqchip=off");
-                exit(1);
-            }
-        }
-
-        probe_bitmap = kvm_arm_vgic_probe();
         if (!probe_bitmap) {
             error_report("Unable to determine GIC version supported by host");
             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:
-            if ((probe_bitmap & KVM_ARM_VGIC_V2) && max_cpus <= GIC_NCPU) {
-                vms->gic_version = VIRT_GIC_VERSION_2;
-            } else if (probe_bitmap & KVM_ARM_VGIC_V3) {
-                /*
-                 * in case the host does not support v2 in-kernel emulation or
-                 * the end-user requested more than 8 VCPUs we now default
-                 * to v3. In any case defaulting to v2 would be broken.
-                 */
-                vms->gic_version = VIRT_GIC_VERSION_3;
-            } else if (max_cpus > GIC_NCPU) {
-                error_report("host only supports in-kernel GICv2 emulation "
-                             "but more than 8 vcpus are requested");
-                exit(1);
-            }
-            break;
-        case VIRT_GIC_VERSION_2:
-        case VIRT_GIC_VERSION_3:
-            break;
-        case VIRT_GIC_VERSION_4:
-            error_report("gic-version=4 is not supported with KVM");
-            exit(1);
+        if (probe_bitmap & KVM_ARM_VGIC_V2) {
+            gics_supported |= VIRT_GIC_VERSION_2_MASK;
         }
-
-        /* 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);
+        if (probe_bitmap & KVM_ARM_VGIC_V3) {
+            gics_supported |= VIRT_GIC_VERSION_3_MASK;
         }
-        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:
+    } else if (kvm_enabled() && !kvm_irqchip_in_kernel()) {
+        /* KVM w/o kernel irqchip can only deal with GICv2 */
+        gics_supported |= VIRT_GIC_VERSION_2_MASK;
+        accel_name = "KVM with kernel-irqchip=off";
+    } else {
+        gics_supported |= VIRT_GIC_VERSION_2_MASK;
         if (module_object_class_by_name("arm-gicv3")) {
-            /* CONFIG_ARM_GICV3_TCG was set */
+            gics_supported |= VIRT_GIC_VERSION_3_MASK;
             if (vms->virt) {
                 /* GICv4 only makes sense if CPU has EL2 */
-                vms->gic_version = VIRT_GIC_VERSION_4;
-            } else {
-                vms->gic_version = VIRT_GIC_VERSION_3;
+                gics_supported |= VIRT_GIC_VERSION_4_MASK;
             }
-        } else {
-            vms->gic_version = VIRT_GIC_VERSION_2;
-        }
-        break;
-    case VIRT_GIC_VERSION_HOST:
-        error_report("gic-version=host requires KVM");
-        exit(1);
-    case VIRT_GIC_VERSION_4:
-        if (!vms->virt) {
-            error_report("gic-version=4 requires virtualization enabled");
-            exit(1);
         }
-        break;
-    case VIRT_GIC_VERSION_2:
-    case VIRT_GIC_VERSION_3:
-        break;
     }
+
+    /*
+     * Then convert helpers like host/max to concrete GIC versions and ensure
+     * the desired version is supported
+     */
+    vms->gic_version = finalize_gic_version_do(accel_name, vms->gic_version,
+                                               gics_supported, max_cpus);
 }
 
 /*
diff --git a/include/hw/arm/virt.h b/include/hw/arm/virt.h
index c7dd59d7f1..e1ddbea96b 100644
--- a/include/hw/arm/virt.h
+++ b/include/hw/arm/virt.h
@@ -109,14 +109,19 @@ typedef enum VirtMSIControllerType {
 } VirtMSIControllerType;
 
 typedef enum VirtGICType {
-    VIRT_GIC_VERSION_MAX,
-    VIRT_GIC_VERSION_HOST,
-    VIRT_GIC_VERSION_2,
-    VIRT_GIC_VERSION_3,
-    VIRT_GIC_VERSION_4,
+    VIRT_GIC_VERSION_MAX = 0,
+    VIRT_GIC_VERSION_HOST = 1,
+    /* The concrete GIC values have to match the GIC version number */
+    VIRT_GIC_VERSION_2 = 2,
+    VIRT_GIC_VERSION_3 = 3,
+    VIRT_GIC_VERSION_4 = 4,
     VIRT_GIC_VERSION_NOSEL,
 } VirtGICType;
 
+#define VIRT_GIC_VERSION_2_MASK BIT(VIRT_GIC_VERSION_2)
+#define VIRT_GIC_VERSION_3_MASK BIT(VIRT_GIC_VERSION_3)
+#define VIRT_GIC_VERSION_4_MASK BIT(VIRT_GIC_VERSION_4)
+
 struct VirtMachineClass {
     MachineClass parent;
     bool disallow_affinity_adjustment;
-- 
2.37.1 (Apple Git-137.1)

[PATCH v3 2/2] hw/arm/virt: Make accels in GIC finalize logic explicit

[Alexander Graf]

Let's explicitly list out all accelerators that we support when trying to
determine the supported set of GIC versions. KVM was already separate, so
the only missing one is HVF which simply reuses all of TCG's emulation
code and thus has the same compatibility matrix.

Signed-off-by: Alexander Graf <agraf@csgraf.de>
Reviewed-by: Philippe Mathieu-Daudé <philmd@linaro.org>
Reviewed-by: Cornelia Huck <cohuck@redhat.com>

---

v1 -> v2:

  - Include TCG header for tcg_enabled()
---
 hw/arm/virt.c | 6 +++++-
 1 file changed, 5 insertions(+), 1 deletion(-)

diff --git a/hw/arm/virt.c b/hw/arm/virt.c
index 6d27f044fe..611f40c1da 100644
--- a/hw/arm/virt.c
+++ b/hw/arm/virt.c
@@ -47,6 +47,7 @@
 #include "sysemu/numa.h"
 #include "sysemu/runstate.h"
 #include "sysemu/tpm.h"
+#include "sysemu/tcg.h"
 #include "sysemu/kvm.h"
 #include "sysemu/hvf.h"
 #include "hw/loader.h"
@@ -1929,7 +1930,7 @@ static void finalize_gic_version(VirtMachineState *vms)
         /* KVM w/o kernel irqchip can only deal with GICv2 */
         gics_supported |= VIRT_GIC_VERSION_2_MASK;
         accel_name = "KVM with kernel-irqchip=off";
-    } else {
+    } else if (tcg_enabled() || hvf_enabled())  {
         gics_supported |= VIRT_GIC_VERSION_2_MASK;
         if (module_object_class_by_name("arm-gicv3")) {
             gics_supported |= VIRT_GIC_VERSION_3_MASK;
@@ -1938,6 +1939,9 @@ static void finalize_gic_version(VirtMachineState *vms)
                 gics_supported |= VIRT_GIC_VERSION_4_MASK;
             }
         }
+    } else {
+        error_report("Unsupported accelerator, can not determine GIC support");
+        exit(1);
     }
 
     /*
-- 
2.37.1 (Apple Git-137.1)

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

[Cornelia Huck]

On Fri, Dec 23 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
>
> 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 <cohuck@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;
> +    }

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

[Alexander Graf]

Hey Cornelia,

On 23.12.22 13:30, Cornelia Huck wrote:
> On Fri, Dec 23 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
>>
>> 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.


I originally had a fallthrough here, then looked at the code and 
concluded for myself that I dislike fallthroughs :). They make more 
complicated code flows insanely complicated and are super error prone.

> In any case,
>
> Reviewed-by: Cornelia Huck <cohuck@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?]


IMHO we're be better off just refactoring code in ways that don't 
require fall-throughs. Modern compilers inline functions pretty well, so 
I think there's very little reason for them anymore.

Thanks a lot for the reviews!


Alex

Re: [PATCH v3 0/2] hw/arm/virt: Handle HVF in finalize_gic_version()

[Richard Henderson]

On 12/23/22 01:01, Alexander Graf wrote:
> The finalize_gic_version() function tries to determine which GIC version
> the current accelerator / host combination supports. During the initial
> HVF porting efforts, I didn't realize that I also had to touch this
> function. Then Zenghui brought up this function as reply to my HVF GICv3
> enablement patch - and boy it is a mess.
> 
> This patch set cleans up all of the GIC finalization so that we can
> easily plug HVF in and also hopefully will have a better time extending
> it in the future. As second step, it explicitly adds HVF support and
> fails loudly for any unsupported accelerators.
> 
> Alex
> 
> v1 -> v2:
> 
>    - Leave VIRT_GIC_VERSION defines intact, we need them for MADT generation
>    - Include TCG header for tcg_enabled()
> 
> v2 -> v3:
> 
>    - Fix comment
>    - Flip kvm-enabled logic for host around
> 
> Alexander Graf (2):
>    hw/arm/virt: Consolidate GIC finalize logic
>    hw/arm/virt: Make accels in GIC finalize logic explicit

Series:
Reviewed-by: Richard Henderson <richard.henderson@linaro.org>

r~

Re: [PATCH v3 2/2] hw/arm/virt: Make accels in GIC finalize logic explicit

[Zenghui Yu via]

Hi Alexander,

On 2022/12/23 17:01, Alexander Graf wrote:
> Let's explicitly list out all accelerators that we support when trying to
> determine the supported set of GIC versions. KVM was already separate, so
> the only missing one is HVF which simply reuses all of TCG's emulation
> code and thus has the same compatibility matrix.
> 
> Signed-off-by: Alexander Graf <agraf@csgraf.de>
> Reviewed-by: Philippe Mathieu-Daudé <philmd@linaro.org>
> Reviewed-by: Cornelia Huck <cohuck@redhat.com>


> @@ -1938,6 +1939,9 @@ static void finalize_gic_version(VirtMachineState *vms)
>                  gics_supported |= VIRT_GIC_VERSION_4_MASK;
>              }
>          }
> +    } else {
> +        error_report("Unsupported accelerator, can not determine GIC support");
> +        exit(1);

Looks like qtest will use arguments like '-accel qtest' and a 'make
check-qtest' triggers this error_report() on my box. It'd be good if you
can have a look (as I really don't have much knowledge about qtest..).

Thanks,
Zenghui

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

[Zenghui Yu via]

On 2022/12/23 17:01, Alexander Graf 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>

Looks good,

Reviewed-by: Zenghui Yu <yuzenghui@huawei.com>

Re: [PATCH v3 2/2] hw/arm/virt: Make accels in GIC finalize logic explicit

[Philippe Mathieu-Daudé]

On 11/1/23 14:35, Zenghui Yu wrote:
> Hi Alexander,
> 
> On 2022/12/23 17:01, Alexander Graf wrote:
>> Let's explicitly list out all accelerators that we support when trying to
>> determine the supported set of GIC versions. KVM was already separate, so
>> the only missing one is HVF which simply reuses all of TCG's emulation
>> code and thus has the same compatibility matrix.
>>
>> Signed-off-by: Alexander Graf <agraf@csgraf.de>
>> Reviewed-by: Philippe Mathieu-Daudé <philmd@linaro.org>
>> Reviewed-by: Cornelia Huck <cohuck@redhat.com>
> 
> 
>> @@ -1938,6 +1939,9 @@ static void 
>> finalize_gic_version(VirtMachineState *vms)
>>                  gics_supported |= VIRT_GIC_VERSION_4_MASK;
>>              }
>>          }
>> +    } else {
>> +        error_report("Unsupported accelerator, can not determine GIC 
>> support");
>> +        exit(1);
> 
> Looks like qtest will use arguments like '-accel qtest' and a 'make
> check-qtest' triggers this error_report() on my box. It'd be good if you
> can have a look (as I really don't have much knowledge about qtest..).

Indeed, I had to squash:

-- >8 --
diff --git a/hw/arm/virt.c b/hw/arm/virt.c
index 611f40c1da..b17e3dafa8 100644
--- a/hw/arm/virt.c
+++ b/hw/arm/virt.c
@@ -50,6 +50,7 @@
  #include "sysemu/tcg.h"
  #include "sysemu/kvm.h"
  #include "sysemu/hvf.h"
+#include "sysemu/qtest.h"
  #include "hw/loader.h"
  #include "qapi/error.h"
  #include "qemu/bitops.h"
@@ -1930,7 +1931,7 @@ static void finalize_gic_version(VirtMachineState 
*vms)
          /* KVM w/o kernel irqchip can only deal with GICv2 */
          gics_supported |= VIRT_GIC_VERSION_2_MASK;
          accel_name = "KVM with kernel-irqchip=off";
-    } else if (tcg_enabled() || hvf_enabled())  {
+    } else if (tcg_enabled() || hvf_enabled() || qtest_enabled())  {
          gics_supported |= VIRT_GIC_VERSION_2_MASK;
          if (module_object_class_by_name("arm-gicv3")) {
              gics_supported |= VIRT_GIC_VERSION_3_MASK;
---

Tested-by: Philippe Mathieu-Daudé <philmd@linaro.org>

Re: [PATCH v3 0/2] hw/arm/virt: Handle HVF in finalize_gic_version()

[Peter Maydell]

On Fri, 23 Dec 2022 at 09:01, Alexander Graf <agraf@csgraf.de> wrote:
>
> The finalize_gic_version() function tries to determine which GIC version
> the current accelerator / host combination supports. During the initial
> HVF porting efforts, I didn't realize that I also had to touch this
> function. Then Zenghui brought up this function as reply to my HVF GICv3
> enablement patch - and boy it is a mess.
>
> This patch set cleans up all of the GIC finalization so that we can
> easily plug HVF in and also hopefully will have a better time extending
> it in the future. As second step, it explicitly adds HVF support and
> fails loudly for any unsupported accelerators.
>
> Alex
>
> v1 -> v2:
>
>   - Leave VIRT_GIC_VERSION defines intact, we need them for MADT generation
>   - Include TCG header for tcg_enabled()
>
> v2 -> v3:
>
>   - Fix comment
>   - Flip kvm-enabled logic for host around
>
> Alexander Graf (2):
>   hw/arm/virt: Consolidate GIC finalize logic
>   hw/arm/virt: Make accels in GIC finalize logic explicit

Since AIUI these patches depend on "hvf: arm: Add support for GICv3",
would you mind including these when you respin that one (ie make
a series-of-3-patches)? That way I don't need to keep this series
on my to-review queue just because it's blocked on another patch :-)

thanks
-- PMM

Re: [PATCH v3 0/2] hw/arm/virt: Handle HVF in finalize_gic_version()

[Peter Maydell]

On Fri, 23 Dec 2022 at 09:01, Alexander Graf <agraf@csgraf.de> wrote:
>
> The finalize_gic_version() function tries to determine which GIC version
> the current accelerator / host combination supports. During the initial
> HVF porting efforts, I didn't realize that I also had to touch this
> function. Then Zenghui brought up this function as reply to my HVF GICv3
> enablement patch - and boy it is a mess.
>
> This patch set cleans up all of the GIC finalization so that we can
> easily plug HVF in and also hopefully will have a better time extending
> it in the future. As second step, it explicitly adds HVF support and
> fails loudly for any unsupported accelerators.
>
> Alex



Applied to target-arm.next, thanks.

-- PMM

Re: [PATCH v3 0/2] hw/arm/virt: Handle HVF in finalize_gic_version()

[Philippe Mathieu-Daudé]

Hi Peter,

On 2/2/23 18:57, Peter Maydell wrote:
> On Fri, 23 Dec 2022 at 09:01, Alexander Graf <agraf@csgraf.de> wrote:
>>
>> The finalize_gic_version() function tries to determine which GIC version
>> the current accelerator / host combination supports. During the initial
>> HVF porting efforts, I didn't realize that I also had to touch this
>> function. Then Zenghui brought up this function as reply to my HVF GICv3
>> enablement patch - and boy it is a mess.
>>
>> This patch set cleans up all of the GIC finalization so that we can
>> easily plug HVF in and also hopefully will have a better time extending
>> it in the future. As second step, it explicitly adds HVF support and
>> fails loudly for any unsupported accelerators.
>>
>> Alex
> 
> 
> 
> Applied to target-arm.next, thanks.

Did you squash the changes mentioned here?
https://lore.kernel.org/qemu-devel/3278ab81-ccdc-9ccc-e504-dca757db5658@linaro.org/

Re: [PATCH v3 0/2] hw/arm/virt: Handle HVF in finalize_gic_version()

[Peter Maydell]

On Fri, 3 Feb 2023 at 07:07, Philippe Mathieu-Daudé <philmd@linaro.org> wrote:
>
> Hi Peter,
>
> On 2/2/23 18:57, Peter Maydell wrote:
> > On Fri, 23 Dec 2022 at 09:01, Alexander Graf <agraf@csgraf.de> wrote:
> >>
> >> The finalize_gic_version() function tries to determine which GIC version
> >> the current accelerator / host combination supports. During the initial
> >> HVF porting efforts, I didn't realize that I also had to touch this
> >> function. Then Zenghui brought up this function as reply to my HVF GICv3
> >> enablement patch - and boy it is a mess.
> >>
> >> This patch set cleans up all of the GIC finalization so that we can
> >> easily plug HVF in and also hopefully will have a better time extending
> >> it in the future. As second step, it explicitly adds HVF support and
> >> fails loudly for any unsupported accelerators.

> > Applied to target-arm.next, thanks.
>
> Did you squash the changes mentioned here?
> https://lore.kernel.org/qemu-devel/3278ab81-ccdc-9ccc-e504-dca757db5658@linaro.org/

Yes, I found those when I noticed the patch didn't pass 'make check' :-)

-- PMM