[PATCH RESEND v2 0/6] target/arm: Add nested virtualization support

[PATCH RESEND v2 0/6] target/arm: Add nested virtualization support

[Haibo Xu]

v2:
  - Move the NV to a CPU feature flag(Andrea&Andrew)
  - Add CPU feature 'el2' test(Andrew)

Many thanks to Andrea and Andrew for their comments!

This series add support for ARMv8.3/8.4 nested virtualization support
in KVM mode. It's based on Marc Zyngier's kernel KVM patches[1], and 
has been tested on a FVP model to run a L2 guest with Qemu. Now the 
feature can be enabled by "-M virt,accel=kvm -cpu host,el2=on" when
starting a VM. 

[1] https://git.kernel.org/pub/scm/linux/kernel/git/maz/arm-platforms.git/log/?h=kvm-arm64/nv-5.12-WIP

Haibo Xu (6):
  Update linux header with new arm64 NV macro
  target/arm/kvm: Add helper to detect el2 when using KVM
  target/arm/kvm: Add an option to turn on/off el2 support
  hw/intc/arm_gicv3: Enable support for setting vGIC maintenance IRQ
  target/arm/cpu: Enable 'el2' to work with host/max cpu
  target/arm: Add vCPU feature 'el2' test.

 hw/arm/virt.c                      | 19 ++++++++---
 hw/intc/arm_gicv3_common.c         |  1 +
 hw/intc/arm_gicv3_kvm.c            | 16 +++++++++
 include/hw/intc/arm_gicv3_common.h |  1 +
 linux-headers/asm-arm64/kvm.h      |  2 ++
 linux-headers/linux/kvm.h          |  1 +
 target/arm/cpu.c                   | 14 +++++++-
 target/arm/cpu.h                   |  4 +++
 target/arm/cpu64.c                 | 53 ++++++++++++++++++++++++++++++
 target/arm/kvm64.c                 | 15 +++++++++
 target/arm/kvm_arm.h               | 13 ++++++++
 target/arm/monitor.c               |  2 +-
 tests/qtest/arm-cpu-features.c     |  9 +++++
 13 files changed, 144 insertions(+), 6 deletions(-)

-- 
2.17.1

[PATCH RESEND v2 1/6] Update linux header with new arm64 NV macro

[Haibo Xu]

Signed-off-by: Haibo Xu <haibo.xu@linaro.org>
---
 linux-headers/asm-arm64/kvm.h | 2 ++
 linux-headers/linux/kvm.h     | 1 +
 2 files changed, 3 insertions(+)

diff --git a/linux-headers/asm-arm64/kvm.h b/linux-headers/asm-arm64/kvm.h
index b6a0eaa32a..77b995a26c 100644
--- a/linux-headers/asm-arm64/kvm.h
+++ b/linux-headers/asm-arm64/kvm.h
@@ -106,6 +106,7 @@ struct kvm_regs {
 #define KVM_ARM_VCPU_SVE		4 /* enable SVE for this CPU */
 #define KVM_ARM_VCPU_PTRAUTH_ADDRESS	5 /* VCPU uses address authentication */
 #define KVM_ARM_VCPU_PTRAUTH_GENERIC	6 /* VCPU uses generic authentication */
+#define KVM_ARM_VCPU_HAS_EL2		7 /* Support nested virtualization */
 
 struct kvm_vcpu_init {
 	__u32 target;
@@ -334,6 +335,7 @@ struct kvm_vcpu_events {
 #define KVM_DEV_ARM_VGIC_GRP_CPU_SYSREGS 6
 #define KVM_DEV_ARM_VGIC_GRP_LEVEL_INFO  7
 #define KVM_DEV_ARM_VGIC_GRP_ITS_REGS 8
+#define KVM_DEV_ARM_VGIC_GRP_MAINT_IRQ 9
 #define KVM_DEV_ARM_VGIC_LINE_LEVEL_INFO_SHIFT	10
 #define KVM_DEV_ARM_VGIC_LINE_LEVEL_INFO_MASK \
 			(0x3fffffULL << KVM_DEV_ARM_VGIC_LINE_LEVEL_INFO_SHIFT)
diff --git a/linux-headers/linux/kvm.h b/linux-headers/linux/kvm.h
index 020b62a619..ce4630c4db 100644
--- a/linux-headers/linux/kvm.h
+++ b/linux-headers/linux/kvm.h
@@ -1056,6 +1056,7 @@ struct kvm_ppc_resize_hpt {
 #define KVM_CAP_ENFORCE_PV_FEATURE_CPUID 190
 #define KVM_CAP_SYS_HYPERV_CPUID 191
 #define KVM_CAP_DIRTY_LOG_RING 192
+#define KVM_CAP_ARM_EL2 193
 
 #ifdef KVM_CAP_IRQ_ROUTING
 
-- 
2.17.1

[PATCH RESEND v2 2/6] target/arm/kvm: Add helper to detect el2 when using KVM

[Haibo Xu]

Signed-off-by: Haibo Xu <haibo.xu@linaro.org>
---
 target/arm/kvm64.c   |  5 +++++
 target/arm/kvm_arm.h | 13 +++++++++++++
 2 files changed, 18 insertions(+)

diff --git a/target/arm/kvm64.c b/target/arm/kvm64.c
index dff85f6db9..9cacaf2eb8 100644
--- a/target/arm/kvm64.c
+++ b/target/arm/kvm64.c
@@ -721,6 +721,11 @@ bool kvm_arm_steal_time_supported(void)
     return kvm_check_extension(kvm_state, KVM_CAP_STEAL_TIME);
 }
 
+bool kvm_arm_el2_supported(void)
+{
+    return kvm_check_extension(kvm_state, KVM_CAP_ARM_EL2);
+}
+
 QEMU_BUILD_BUG_ON(KVM_ARM64_SVE_VQ_MIN != 1);
 
 void kvm_arm_sve_get_vls(CPUState *cs, unsigned long *map)
diff --git a/target/arm/kvm_arm.h b/target/arm/kvm_arm.h
index 34f8daa377..7d7fc7981b 100644
--- a/target/arm/kvm_arm.h
+++ b/target/arm/kvm_arm.h
@@ -285,6 +285,14 @@ void kvm_arm_steal_time_finalize(ARMCPU *cpu, Error **errp);
  */
 bool kvm_arm_steal_time_supported(void);
 
+/**
+ * kvm_arm_el2_supported:
+ *
+ * Returns: true if KVM can enable el2(nested virtualization)
+ * and false otherwise.
+ */
+bool kvm_arm_el2_supported(void);
+
 /**
  * kvm_arm_aarch32_supported:
  *
@@ -398,6 +406,11 @@ static inline bool kvm_arm_steal_time_supported(void)
     return false;
 }
 
+static inline bool kvm_arm_el2_supported(void)
+{
+    return false;
+}
+
 /*
  * These functions should never actually be called without KVM support.
  */
-- 
2.17.1

Re: [PATCH RESEND v2 2/6] target/arm/kvm: Add helper to detect el2 when using KVM

[Andrew Jones]

On Thu, Apr 01, 2021 at 05:55:34AM -0700, Haibo Xu wrote:
> Signed-off-by: Haibo Xu <haibo.xu@linaro.org>
> ---
>  target/arm/kvm64.c   |  5 +++++
>  target/arm/kvm_arm.h | 13 +++++++++++++
>  2 files changed, 18 insertions(+)
> 
> diff --git a/target/arm/kvm64.c b/target/arm/kvm64.c
> index dff85f6db9..9cacaf2eb8 100644
> --- a/target/arm/kvm64.c
> +++ b/target/arm/kvm64.c
> @@ -721,6 +721,11 @@ bool kvm_arm_steal_time_supported(void)
>      return kvm_check_extension(kvm_state, KVM_CAP_STEAL_TIME);
>  }
>  
> +bool kvm_arm_el2_supported(void)
> +{
> +    return kvm_check_extension(kvm_state, KVM_CAP_ARM_EL2);
> +}
> +
>  QEMU_BUILD_BUG_ON(KVM_ARM64_SVE_VQ_MIN != 1);
>  
>  void kvm_arm_sve_get_vls(CPUState *cs, unsigned long *map)
> diff --git a/target/arm/kvm_arm.h b/target/arm/kvm_arm.h
> index 34f8daa377..7d7fc7981b 100644
> --- a/target/arm/kvm_arm.h
> +++ b/target/arm/kvm_arm.h
> @@ -285,6 +285,14 @@ void kvm_arm_steal_time_finalize(ARMCPU *cpu, Error **errp);
>   */
>  bool kvm_arm_steal_time_supported(void);
>  
> +/**
> + * kvm_arm_el2_supported:
> + *
> + * Returns: true if KVM can enable el2(nested virtualization)
                                         ^ please add a space
> + * and false otherwise.
> + */
> +bool kvm_arm_el2_supported(void);
> +
>  /**
>   * kvm_arm_aarch32_supported:
>   *
> @@ -398,6 +406,11 @@ static inline bool kvm_arm_steal_time_supported(void)
>      return false;
>  }
>  
> +static inline bool kvm_arm_el2_supported(void)
> +{
> +    return false;
> +}
> +
>  /*
>   * These functions should never actually be called without KVM support.
>   */
> -- 
> 2.17.1
> 
> 

Otherwise

Reviewed-by: Andrew Jones <drjones@redhat.com>

[PATCH RESEND v2 3/6] target/arm/kvm: Add an option to turn on/off el2 support

[Haibo Xu]

Adds an el2=[on/off] option to enable/disable el2(nested virtualization)
support in KVM guest vCPU.

Signed-off-by: Haibo Xu <haibo.xu@linaro.org>
---
 target/arm/cpu.c   | 11 ++++++++++
 target/arm/cpu.h   |  4 ++++
 target/arm/cpu64.c | 52 ++++++++++++++++++++++++++++++++++++++++++++++
 3 files changed, 67 insertions(+)

diff --git a/target/arm/cpu.c b/target/arm/cpu.c
index ae04884408..30cc330f50 100644
--- a/target/arm/cpu.c
+++ b/target/arm/cpu.c
@@ -1349,6 +1349,17 @@ void arm_cpu_finalize_features(ARMCPU *cpu, Error **errp)
                 return;
             }
         }
+
+        /*
+         * Currently, vCPU feature 'el2' only supported in KVM mode.
+         */
+        if (kvm_enabled()) {
+            arm_cpu_el2_finalize(cpu, &local_err);
+            if (local_err != NULL) {
+                error_propagate(errp, local_err);
+                return;
+            }
+        }
     }
 
     if (kvm_enabled()) {
diff --git a/target/arm/cpu.h b/target/arm/cpu.h
index 193a49ec7f..19fa9cfbfd 100644
--- a/target/arm/cpu.h
+++ b/target/arm/cpu.h
@@ -203,10 +203,12 @@ typedef struct {
 # define ARM_MAX_VQ    16
 void arm_cpu_sve_finalize(ARMCPU *cpu, Error **errp);
 void arm_cpu_pauth_finalize(ARMCPU *cpu, Error **errp);
+void arm_cpu_el2_finalize(ARMCPU *cpu, Error **errp);
 #else
 # define ARM_MAX_VQ    1
 static inline void arm_cpu_sve_finalize(ARMCPU *cpu, Error **errp) { }
 static inline void arm_cpu_pauth_finalize(ARMCPU *cpu, Error **errp) { }
+static inline void arm_cpu_el2_finalize(ARMCPU *cpu, Error **errp) { }
 #endif
 
 typedef struct ARMVectorReg {
@@ -1058,6 +1060,7 @@ void aarch64_sve_narrow_vq(CPUARMState *env, unsigned vq);
 void aarch64_sve_change_el(CPUARMState *env, int old_el,
                            int new_el, bool el0_a64);
 void aarch64_add_sve_properties(Object *obj);
+void aarch64_add_el2_properties(Object *obj);
 
 /*
  * SVE registers are encoded in KVM's memory in an endianness-invariant format.
@@ -1089,6 +1092,7 @@ static inline void aarch64_sve_change_el(CPUARMState *env, int o,
                                          int n, bool a)
 { }
 static inline void aarch64_add_sve_properties(Object *obj) { }
+static inline void aarch64_add_el2_properties(Object *obj) { }
 #endif
 
 void aarch64_sync_32_to_64(CPUARMState *env);
diff --git a/target/arm/cpu64.c b/target/arm/cpu64.c
index f0a9e968c9..3f3f2c5495 100644
--- a/target/arm/cpu64.c
+++ b/target/arm/cpu64.c
@@ -603,6 +603,58 @@ static Property arm_cpu_pauth_property =
 static Property arm_cpu_pauth_impdef_property =
     DEFINE_PROP_BOOL("pauth-impdef", ARMCPU, prop_pauth_impdef, false);
 
+void arm_cpu_el2_finalize(ARMCPU *cpu, Error **errp)
+{
+    if (cpu->has_el2) {
+        if (!kvm_enabled() || !kvm_arm_el2_supported()) {
+            error_setg(errp, "'el2' cannot be enabled on this host");
+            return;
+        }
+    }
+
+    if (cpu->has_el2) {
+        set_feature(&cpu->env, ARM_FEATURE_EL2);
+    } else {
+        unset_feature(&cpu->env, ARM_FEATURE_EL2);
+    }
+}
+
+static bool arm_get_el2(Object *obj, Error **errp)
+{
+    ARMCPU *cpu = ARM_CPU(obj);
+
+    return cpu->has_el2;
+}
+
+static void arm_set_el2(Object *obj, bool value, Error **errp)
+{
+    ARMCPU *cpu = ARM_CPU(obj);
+
+    if (value) {
+        if (!kvm_enabled() || !kvm_arm_el2_supported()) {
+            error_setg(errp, "'el2' cannot be enabled on this host");
+            return;
+        }
+        set_feature(&cpu->env, ARM_FEATURE_EL2);
+    } else {
+        unset_feature(&cpu->env, ARM_FEATURE_EL2);
+    }
+
+    cpu->has_el2 = value;
+}
+
+void aarch64_add_el2_properties(Object *obj)
+{
+    /*
+     * vCPU feature 'el2' is only available in KVM mode, and is
+     * disabled by default to keep in line with that in TCG mode.
+     */
+    ARM_CPU(obj)->has_el2 = false;
+    object_property_add_bool(obj, "el2", arm_get_el2, arm_set_el2);
+    object_property_set_description(obj, "el2", "Set off to disable "
+                                    "nested virtulization.");
+}
+
 /* -cpu max: if KVM is enabled, like -cpu host (best possible with this host);
  * otherwise, a CPU with as many features enabled as our emulation supports.
  * The version of '-cpu max' for qemu-system-arm is defined in cpu.c;
-- 
2.17.1

Re: [PATCH RESEND v2 3/6] target/arm/kvm: Add an option to turn on/off el2 support

[Andrew Jones]

On Thu, Apr 01, 2021 at 05:55:35AM -0700, Haibo Xu wrote:
> Adds an el2=[on/off] option to enable/disable el2(nested virtualization)
                                                   ^ space, please

> support in KVM guest vCPU.
> 
> Signed-off-by: Haibo Xu <haibo.xu@linaro.org>
> ---
>  target/arm/cpu.c   | 11 ++++++++++
>  target/arm/cpu.h   |  4 ++++
>  target/arm/cpu64.c | 52 ++++++++++++++++++++++++++++++++++++++++++++++
>  3 files changed, 67 insertions(+)
> 
> diff --git a/target/arm/cpu.c b/target/arm/cpu.c
> index ae04884408..30cc330f50 100644
> --- a/target/arm/cpu.c
> +++ b/target/arm/cpu.c
> @@ -1349,6 +1349,17 @@ void arm_cpu_finalize_features(ARMCPU *cpu, Error **errp)
>                  return;
>              }
>          }
> +
> +        /*
> +         * Currently, vCPU feature 'el2' only supported in KVM mode.
> +         */
> +        if (kvm_enabled()) {
> +            arm_cpu_el2_finalize(cpu, &local_err);
> +            if (local_err != NULL) {
> +                error_propagate(errp, local_err);
> +                return;
> +            }
> +        }

nit: We could tie this 'if (kvm_enabled())' block to the
'if (!kvm_enabled())' block above it by turning one or the other
into an else clause.

>      }
>  
>      if (kvm_enabled()) {
> diff --git a/target/arm/cpu.h b/target/arm/cpu.h
> index 193a49ec7f..19fa9cfbfd 100644
> --- a/target/arm/cpu.h
> +++ b/target/arm/cpu.h
> @@ -203,10 +203,12 @@ typedef struct {
>  # define ARM_MAX_VQ    16
>  void arm_cpu_sve_finalize(ARMCPU *cpu, Error **errp);
>  void arm_cpu_pauth_finalize(ARMCPU *cpu, Error **errp);
> +void arm_cpu_el2_finalize(ARMCPU *cpu, Error **errp);
>  #else
>  # define ARM_MAX_VQ    1
>  static inline void arm_cpu_sve_finalize(ARMCPU *cpu, Error **errp) { }
>  static inline void arm_cpu_pauth_finalize(ARMCPU *cpu, Error **errp) { }
> +static inline void arm_cpu_el2_finalize(ARMCPU *cpu, Error **errp) { }
>  #endif
>  
>  typedef struct ARMVectorReg {
> @@ -1058,6 +1060,7 @@ void aarch64_sve_narrow_vq(CPUARMState *env, unsigned vq);
>  void aarch64_sve_change_el(CPUARMState *env, int old_el,
>                             int new_el, bool el0_a64);
>  void aarch64_add_sve_properties(Object *obj);
> +void aarch64_add_el2_properties(Object *obj);
>  
>  /*
>   * SVE registers are encoded in KVM's memory in an endianness-invariant format.
> @@ -1089,6 +1092,7 @@ static inline void aarch64_sve_change_el(CPUARMState *env, int o,
>                                           int n, bool a)
>  { }
>  static inline void aarch64_add_sve_properties(Object *obj) { }
> +static inline void aarch64_add_el2_properties(Object *obj) { }
>  #endif
>  
>  void aarch64_sync_32_to_64(CPUARMState *env);
> diff --git a/target/arm/cpu64.c b/target/arm/cpu64.c
> index f0a9e968c9..3f3f2c5495 100644
> --- a/target/arm/cpu64.c
> +++ b/target/arm/cpu64.c
> @@ -603,6 +603,58 @@ static Property arm_cpu_pauth_property =
>  static Property arm_cpu_pauth_impdef_property =
>      DEFINE_PROP_BOOL("pauth-impdef", ARMCPU, prop_pauth_impdef, false);
>  
> +void arm_cpu_el2_finalize(ARMCPU *cpu, Error **errp)
> +{
> +    if (cpu->has_el2) {
> +        if (!kvm_enabled() || !kvm_arm_el2_supported()) {
> +            error_setg(errp, "'el2' cannot be enabled on this host");
> +            return;
> +        }
> +    }
> +
> +    if (cpu->has_el2) {
> +        set_feature(&cpu->env, ARM_FEATURE_EL2);
> +    } else {
> +        unset_feature(&cpu->env, ARM_FEATURE_EL2);
> +    }
> +}
> +
> +static bool arm_get_el2(Object *obj, Error **errp)
> +{
> +    ARMCPU *cpu = ARM_CPU(obj);
> +
> +    return cpu->has_el2;
> +}
> +
> +static void arm_set_el2(Object *obj, bool value, Error **errp)
> +{
> +    ARMCPU *cpu = ARM_CPU(obj);
> +
> +    if (value) {
> +        if (!kvm_enabled() || !kvm_arm_el2_supported()) {
> +            error_setg(errp, "'el2' cannot be enabled on this host");
> +            return;
> +        }
> +        set_feature(&cpu->env, ARM_FEATURE_EL2);
> +    } else {
> +        unset_feature(&cpu->env, ARM_FEATURE_EL2);
> +    }
> +
> +    cpu->has_el2 = value;
> +}
> +
> +void aarch64_add_el2_properties(Object *obj)
> +{
> +    /*
> +     * vCPU feature 'el2' is only available in KVM mode, and is
> +     * disabled by default to keep in line with that in TCG mode.
> +     */
> +    ARM_CPU(obj)->has_el2 = false;
> +    object_property_add_bool(obj, "el2", arm_get_el2, arm_set_el2);
> +    object_property_set_description(obj, "el2", "Set off to disable "
> +                                    "nested virtulization.");

Since the default is 'off', it seems like the description should be
instructing one to set 'on to enable' instead. Or both, like the
description of the 'aarch64' property does.

> +}
> +
>  /* -cpu max: if KVM is enabled, like -cpu host (best possible with this host);
>   * otherwise, a CPU with as many features enabled as our emulation supports.
>   * The version of '-cpu max' for qemu-system-arm is defined in cpu.c;
> -- 
> 2.17.1
> 
>

Thanks,
drew 

Re: [PATCH RESEND v2 3/6] target/arm/kvm: Add an option to turn on/off el2 support

[Peter Maydell]

On Thu, 1 Apr 2021 at 13:55, Haibo Xu <haibo.xu@linaro.org> wrote:
>
> Adds an el2=[on/off] option to enable/disable el2(nested virtualization)
> support in KVM guest vCPU.
>
> Signed-off-by: Haibo Xu <haibo.xu@linaro.org>
> ---
>  target/arm/cpu.c   | 11 ++++++++++
>  target/arm/cpu.h   |  4 ++++
>  target/arm/cpu64.c | 52 ++++++++++++++++++++++++++++++++++++++++++++++
>  3 files changed, 67 insertions(+)
>
> diff --git a/target/arm/cpu.c b/target/arm/cpu.c
> index ae04884408..30cc330f50 100644
> --- a/target/arm/cpu.c
> +++ b/target/arm/cpu.c
> @@ -1349,6 +1349,17 @@ void arm_cpu_finalize_features(ARMCPU *cpu, Error **errp)
>                  return;
>              }
>          }
> +
> +        /*
> +         * Currently, vCPU feature 'el2' only supported in KVM mode.
> +         */
> +        if (kvm_enabled()) {
> +            arm_cpu_el2_finalize(cpu, &local_err);
> +            if (local_err != NULL) {
> +                error_propagate(errp, local_err);
> +                return;
> +            }
> +        }

I don't understand this. EL2 is supported in TCG as well...

thanks
-- PMM

[PATCH RESEND v2 4/6] hw/intc/arm_gicv3: Enable support for setting vGIC maintenance IRQ

[Haibo Xu]

Using the new VGIC KVM device attribute to set the maintenance IRQ.
This is fixed to use IRQ 25(PPI 9), as a platform decision matching
the arm64 SBSA recommendation.

Signed-off-by: Haibo Xu <haibo.xu@linaro.org>
---
 hw/arm/virt.c                      |  5 +++++
 hw/intc/arm_gicv3_common.c         |  1 +
 hw/intc/arm_gicv3_kvm.c            | 16 ++++++++++++++++
 include/hw/intc/arm_gicv3_common.h |  1 +
 4 files changed, 23 insertions(+)

diff --git a/hw/arm/virt.c b/hw/arm/virt.c
index aa2bbd14e0..92d46ebcfe 100644
--- a/hw/arm/virt.c
+++ b/hw/arm/virt.c
@@ -663,6 +663,11 @@ static void create_gic(VirtMachineState *vms)
             qdev_prop_set_uint32(vms->gic, "redist-region-count[1]",
                 MIN(smp_cpus - redist0_count, redist1_capacity));
         }
+
+        if (kvm_irqchip_in_kernel()) {
+            bool el2 = object_property_get_bool(OBJECT(first_cpu), "el2", NULL);
+            qdev_prop_set_bit(vms->gic, "has-virtualization-extensions", el2);
+        }
     } else {
         if (!kvm_irqchip_in_kernel()) {
             qdev_prop_set_bit(vms->gic, "has-virtualization-extensions",
diff --git a/hw/intc/arm_gicv3_common.c b/hw/intc/arm_gicv3_common.c
index 58ef65f589..3ac10c8e61 100644
--- a/hw/intc/arm_gicv3_common.c
+++ b/hw/intc/arm_gicv3_common.c
@@ -495,6 +495,7 @@ static Property arm_gicv3_common_properties[] = {
     DEFINE_PROP_UINT32("num-irq", GICv3State, num_irq, 32),
     DEFINE_PROP_UINT32("revision", GICv3State, revision, 3),
     DEFINE_PROP_BOOL("has-security-extensions", GICv3State, security_extn, 0),
+    DEFINE_PROP_BOOL("has-virtualization-extensions", GICv3State, virt_extn, 0),
     DEFINE_PROP_ARRAY("redist-region-count", GICv3State, nb_redist_regions,
                       redist_region_count, qdev_prop_uint32, uint32_t),
     DEFINE_PROP_END_OF_LIST(),
diff --git a/hw/intc/arm_gicv3_kvm.c b/hw/intc/arm_gicv3_kvm.c
index 65a4c880a3..1e1ca66e2c 100644
--- a/hw/intc/arm_gicv3_kvm.c
+++ b/hw/intc/arm_gicv3_kvm.c
@@ -826,6 +826,22 @@ static void kvm_arm_gicv3_realize(DeviceState *dev, Error **errp)
     kvm_device_access(s->dev_fd, KVM_DEV_ARM_VGIC_GRP_CTRL,
                       KVM_DEV_ARM_VGIC_CTRL_INIT, NULL, true, &error_abort);
 
+    if (s->virt_extn) {
+        bool maint_irq_allowed;
+        uint32_t maint_irq = 25;
+
+        maint_irq_allowed =
+            kvm_device_check_attr(s->dev_fd, KVM_DEV_ARM_VGIC_GRP_MAINT_IRQ, 0);
+        if (!maint_irq_allowed) {
+            error_setg(errp, "VGICv3 setting maintenance IRQ are not "
+                             "supported by this host kernel");
+            return;
+        }
+
+        kvm_device_access(s->dev_fd, KVM_DEV_ARM_VGIC_GRP_MAINT_IRQ,
+                          0, &maint_irq, true, &error_abort);
+    }
+
     kvm_arm_register_device(&s->iomem_dist, -1, KVM_DEV_ARM_VGIC_GRP_ADDR,
                             KVM_VGIC_V3_ADDR_TYPE_DIST, s->dev_fd, 0);
 
diff --git a/include/hw/intc/arm_gicv3_common.h b/include/hw/intc/arm_gicv3_common.h
index 91491a2f66..921ddc2c5f 100644
--- a/include/hw/intc/arm_gicv3_common.h
+++ b/include/hw/intc/arm_gicv3_common.h
@@ -220,6 +220,7 @@ struct GICv3State {
     uint32_t num_irq;
     uint32_t revision;
     bool security_extn;
+    bool virt_extn;
     bool irq_reset_nonsecure;
     bool gicd_no_migration_shift_bug;
 
-- 
2.17.1

Re: [PATCH RESEND v2 4/6] hw/intc/arm_gicv3: Enable support for setting vGIC maintenance IRQ

[Andrew Jones]

On Thu, Apr 01, 2021 at 05:55:36AM -0700, Haibo Xu wrote:
> Using the new VGIC KVM device attribute to set the maintenance IRQ.
> This is fixed to use IRQ 25(PPI 9), as a platform decision matching
> the arm64 SBSA recommendation.
> 
> Signed-off-by: Haibo Xu <haibo.xu@linaro.org>
> ---
>  hw/arm/virt.c                      |  5 +++++
>  hw/intc/arm_gicv3_common.c         |  1 +
>  hw/intc/arm_gicv3_kvm.c            | 16 ++++++++++++++++
>  include/hw/intc/arm_gicv3_common.h |  1 +
>  4 files changed, 23 insertions(+)
> 
> diff --git a/hw/arm/virt.c b/hw/arm/virt.c
> index aa2bbd14e0..92d46ebcfe 100644
> --- a/hw/arm/virt.c
> +++ b/hw/arm/virt.c
> @@ -663,6 +663,11 @@ static void create_gic(VirtMachineState *vms)
>              qdev_prop_set_uint32(vms->gic, "redist-region-count[1]",
>                  MIN(smp_cpus - redist0_count, redist1_capacity));
>          }
> +
> +        if (kvm_irqchip_in_kernel()) {
> +            bool el2 = object_property_get_bool(OBJECT(first_cpu), "el2", NULL);
> +            qdev_prop_set_bit(vms->gic, "has-virtualization-extensions", el2);
> +        }
>      } else {
>          if (!kvm_irqchip_in_kernel()) {
>              qdev_prop_set_bit(vms->gic, "has-virtualization-extensions",
> diff --git a/hw/intc/arm_gicv3_common.c b/hw/intc/arm_gicv3_common.c
> index 58ef65f589..3ac10c8e61 100644
> --- a/hw/intc/arm_gicv3_common.c
> +++ b/hw/intc/arm_gicv3_common.c
> @@ -495,6 +495,7 @@ static Property arm_gicv3_common_properties[] = {
>      DEFINE_PROP_UINT32("num-irq", GICv3State, num_irq, 32),
>      DEFINE_PROP_UINT32("revision", GICv3State, revision, 3),
>      DEFINE_PROP_BOOL("has-security-extensions", GICv3State, security_extn, 0),
> +    DEFINE_PROP_BOOL("has-virtualization-extensions", GICv3State, virt_extn, 0),
>      DEFINE_PROP_ARRAY("redist-region-count", GICv3State, nb_redist_regions,
>                        redist_region_count, qdev_prop_uint32, uint32_t),
>      DEFINE_PROP_END_OF_LIST(),
> diff --git a/hw/intc/arm_gicv3_kvm.c b/hw/intc/arm_gicv3_kvm.c
> index 65a4c880a3..1e1ca66e2c 100644
> --- a/hw/intc/arm_gicv3_kvm.c
> +++ b/hw/intc/arm_gicv3_kvm.c
> @@ -826,6 +826,22 @@ static void kvm_arm_gicv3_realize(DeviceState *dev, Error **errp)
>      kvm_device_access(s->dev_fd, KVM_DEV_ARM_VGIC_GRP_CTRL,
>                        KVM_DEV_ARM_VGIC_CTRL_INIT, NULL, true, &error_abort);
>  
> +    if (s->virt_extn) {
> +        bool maint_irq_allowed;
> +        uint32_t maint_irq = 25;

Please use KVM_DEV_ARM_VGIC_GRP_MAINT_IRQ + 16, or better would be
something like PPI(KVM_DEV_ARM_VGIC_GRP_MAINT_IRQ). We have a PPI() macro
in virt.h. I'm not sure if/where we could move that, though.

> +
> +        maint_irq_allowed =
> +            kvm_device_check_attr(s->dev_fd, KVM_DEV_ARM_VGIC_GRP_MAINT_IRQ, 0);
> +        if (!maint_irq_allowed) {

I'll defer to the maintainers, but I'd rather see

        if (!kvm_device_check_attr(s->dev_fd, KVM_DEV_ARM_VGIC_GRP_MAINT_IRQ, 0)) {

which is slightly longer than 80 chars, then require the use of a local
variable and the broken assignment line.

> +            error_setg(errp, "VGICv3 setting maintenance IRQ are not "
> +                             "supported by this host kernel");

"VGICv3 maintenance IRQ setting is not supported by this host kernel"

Also, I think we're trying not to brake error lines like this. It makes
grepping harder.

> +            return;
> +        }
> +
> +        kvm_device_access(s->dev_fd, KVM_DEV_ARM_VGIC_GRP_MAINT_IRQ,
> +                          0, &maint_irq, true, &error_abort);
> +    }
> +
>      kvm_arm_register_device(&s->iomem_dist, -1, KVM_DEV_ARM_VGIC_GRP_ADDR,
>                              KVM_VGIC_V3_ADDR_TYPE_DIST, s->dev_fd, 0);
>  
> diff --git a/include/hw/intc/arm_gicv3_common.h b/include/hw/intc/arm_gicv3_common.h
> index 91491a2f66..921ddc2c5f 100644
> --- a/include/hw/intc/arm_gicv3_common.h
> +++ b/include/hw/intc/arm_gicv3_common.h
> @@ -220,6 +220,7 @@ struct GICv3State {
>      uint32_t num_irq;
>      uint32_t revision;
>      bool security_extn;
> +    bool virt_extn;
>      bool irq_reset_nonsecure;
>      bool gicd_no_migration_shift_bug;
>  
> -- 
> 2.17.1
> 
>

Thanks,
drew 

[PATCH RESEND v2 5/6] target/arm/cpu: Enable 'el2' to work with host/max cpu

[Haibo Xu]

Turn off the 'el2' cpu property by default to keep in line with
that in TCG mode, i.e. we can now use '-cpu max|host,el2=on' to
enable the nested virtualization.

Signed-off-by: Haibo Xu <haibo.xu@linaro.org>
---
 hw/arm/virt.c      | 14 ++++++++++----
 target/arm/cpu.c   |  3 ++-
 target/arm/cpu64.c |  1 +
 target/arm/kvm64.c | 10 ++++++++++
 4 files changed, 23 insertions(+), 5 deletions(-)

diff --git a/hw/arm/virt.c b/hw/arm/virt.c
index 92d46ebcfe..74340e21bd 100644
--- a/hw/arm/virt.c
+++ b/hw/arm/virt.c
@@ -454,6 +454,7 @@ static void fdt_add_gic_node(VirtMachineState *vms)
 {
     MachineState *ms = MACHINE(vms);
     char *nodename;
+    bool has_el2 = object_property_get_bool(OBJECT(first_cpu), "el2", NULL);
 
     vms->gic_phandle = qemu_fdt_alloc_phandle(ms->fdt);
     qemu_fdt_setprop_cell(ms->fdt, "/", "interrupt-parent", vms->gic_phandle);
@@ -491,7 +492,7 @@ static void fdt_add_gic_node(VirtMachineState *vms)
                                  2, vms->memmap[VIRT_HIGH_GIC_REDIST2].size);
         }
 
-        if (vms->virt) {
+        if (vms->virt || has_el2) {
             qemu_fdt_setprop_cells(ms->fdt, nodename, "interrupts",
                                    GIC_FDT_IRQ_TYPE_PPI, ARCH_GIC_MAINT_IRQ,
                                    GIC_FDT_IRQ_FLAGS_LEVEL_HI);
@@ -1911,8 +1912,8 @@ static void machvirt_init(MachineState *machine)
     }
 
     if (vms->virt && kvm_enabled()) {
-        error_report("mach-virt: KVM does not support providing "
-                     "Virtualization extensions to the guest CPU");
+        error_report("mach-virt: VM 'virtualization' feature is not supported "
+                     "in KVM mode, please use CPU feature 'el2' instead");
         exit(1);
     }
 
@@ -1950,11 +1951,16 @@ static void machvirt_init(MachineState *machine)
             object_property_set_bool(cpuobj, "has_el3", false, NULL);
         }
 
-        if (!vms->virt && object_property_find(cpuobj, "has_el2")) {
+        if (!vms->virt && !kvm_enabled() &&
+            object_property_find(cpuobj, "has_el2")) {
             object_property_set_bool(cpuobj, "has_el2", false, NULL);
         }
 
         if (vms->psci_conduit != QEMU_PSCI_CONDUIT_DISABLED) {
+            if (kvm_enabled() && ARM_CPU(cpuobj)->has_el2) {
+                vms->psci_conduit = QEMU_PSCI_CONDUIT_SMC;
+            }
+
             object_property_set_int(cpuobj, "psci-conduit", vms->psci_conduit,
                                     NULL);
 
diff --git a/target/arm/cpu.c b/target/arm/cpu.c
index 30cc330f50..9530a2c4bf 100644
--- a/target/arm/cpu.c
+++ b/target/arm/cpu.c
@@ -1099,7 +1099,7 @@ static Property arm_cpu_rvbar_property =
 
 #ifndef CONFIG_USER_ONLY
 static Property arm_cpu_has_el2_property =
-            DEFINE_PROP_BOOL("has_el2", ARMCPU, has_el2, true);
+            DEFINE_PROP_BOOL("has_el2", ARMCPU, has_el2, false);
 
 static Property arm_cpu_has_el3_property =
             DEFINE_PROP_BOOL("has_el3", ARMCPU, has_el3, true);
@@ -2018,6 +2018,7 @@ static void arm_host_initfn(Object *obj)
     kvm_arm_set_cpu_features_from_host(cpu);
     if (arm_feature(&cpu->env, ARM_FEATURE_AARCH64)) {
         aarch64_add_sve_properties(obj);
+        aarch64_add_el2_properties(obj);
     }
     arm_cpu_post_init(obj);
 }
diff --git a/target/arm/cpu64.c b/target/arm/cpu64.c
index 3f3f2c5495..ae8811d09e 100644
--- a/target/arm/cpu64.c
+++ b/target/arm/cpu64.c
@@ -666,6 +666,7 @@ static void aarch64_max_initfn(Object *obj)
 
     if (kvm_enabled()) {
         kvm_arm_set_cpu_features_from_host(cpu);
+        aarch64_add_el2_properties(obj);
     } else {
         uint64_t t;
         uint32_t u;
diff --git a/target/arm/kvm64.c b/target/arm/kvm64.c
index 9cacaf2eb8..7bf892404f 100644
--- a/target/arm/kvm64.c
+++ b/target/arm/kvm64.c
@@ -500,6 +500,7 @@ bool kvm_arm_get_host_cpu_features(ARMHostCPUFeatures *ahcf)
      */
     int fdarray[3];
     bool sve_supported;
+    bool el2_supported;
     uint64_t features = 0;
     uint64_t t;
     int err;
@@ -646,6 +647,7 @@ bool kvm_arm_get_host_cpu_features(ARMHostCPUFeatures *ahcf)
     }
 
     sve_supported = ioctl(fdarray[0], KVM_CHECK_EXTENSION, KVM_CAP_ARM_SVE) > 0;
+    el2_supported = ioctl(fdarray[0], KVM_CHECK_EXTENSION, KVM_CAP_ARM_EL2) > 0;
 
     kvm_arm_destroy_scratch_host_vcpu(fdarray);
 
@@ -660,6 +662,11 @@ bool kvm_arm_get_host_cpu_features(ARMHostCPUFeatures *ahcf)
         ahcf->isar.id_aa64pfr0 = t;
     }
 
+    /* Use the ARM_FEATURE_EL2 bit to keep inline with that in TCG mode. */
+    if (el2_supported) {
+        features |= 1ULL << ARM_FEATURE_EL2;
+    }
+
     /*
      * We can assume any KVM supporting CPU is at least a v8
      * with VFPv4+Neon; this in turn implies most of the other
@@ -861,6 +868,9 @@ int kvm_arch_init_vcpu(CPUState *cs)
         assert(kvm_arm_sve_supported());
         cpu->kvm_init_features[0] |= 1 << KVM_ARM_VCPU_SVE;
     }
+    if (arm_feature(&cpu->env, ARM_FEATURE_EL2)) {
+        cpu->kvm_init_features[0] |= 1 << KVM_ARM_VCPU_HAS_EL2;
+    }
 
     /* Do KVM_ARM_VCPU_INIT ioctl */
     ret = kvm_arm_vcpu_init(cs);
-- 
2.17.1

Re: [PATCH RESEND v2 5/6] target/arm/cpu: Enable 'el2' to work with host/max cpu

[Andrew Jones]

On Thu, Apr 01, 2021 at 05:55:37AM -0700, Haibo Xu wrote:
> Turn off the 'el2' cpu property by default to keep in line with
> that in TCG mode, i.e. we can now use '-cpu max|host,el2=on' to
> enable the nested virtualization.
> 
> Signed-off-by: Haibo Xu <haibo.xu@linaro.org>
> ---
>  hw/arm/virt.c      | 14 ++++++++++----
>  target/arm/cpu.c   |  3 ++-
>  target/arm/cpu64.c |  1 +
>  target/arm/kvm64.c | 10 ++++++++++
>  4 files changed, 23 insertions(+), 5 deletions(-)
> 
> diff --git a/hw/arm/virt.c b/hw/arm/virt.c
> index 92d46ebcfe..74340e21bd 100644
> --- a/hw/arm/virt.c
> +++ b/hw/arm/virt.c
> @@ -454,6 +454,7 @@ static void fdt_add_gic_node(VirtMachineState *vms)
>  {
>      MachineState *ms = MACHINE(vms);
>      char *nodename;
> +    bool has_el2 = object_property_get_bool(OBJECT(first_cpu), "el2", NULL);
>  
>      vms->gic_phandle = qemu_fdt_alloc_phandle(ms->fdt);
>      qemu_fdt_setprop_cell(ms->fdt, "/", "interrupt-parent", vms->gic_phandle);
> @@ -491,7 +492,7 @@ static void fdt_add_gic_node(VirtMachineState *vms)
>                                   2, vms->memmap[VIRT_HIGH_GIC_REDIST2].size);
>          }
>  
> -        if (vms->virt) {
> +        if (vms->virt || has_el2) {
>              qemu_fdt_setprop_cells(ms->fdt, nodename, "interrupts",
>                                     GIC_FDT_IRQ_TYPE_PPI, ARCH_GIC_MAINT_IRQ,
>                                     GIC_FDT_IRQ_FLAGS_LEVEL_HI);
> @@ -1911,8 +1912,8 @@ static void machvirt_init(MachineState *machine)
>      }
>  
>      if (vms->virt && kvm_enabled()) {
> -        error_report("mach-virt: KVM does not support providing "
> -                     "Virtualization extensions to the guest CPU");
> +        error_report("mach-virt: VM 'virtualization' feature is not supported "
> +                     "in KVM mode, please use CPU feature 'el2' instead");
>          exit(1);
>      }
>  
> @@ -1950,11 +1951,16 @@ static void machvirt_init(MachineState *machine)
>              object_property_set_bool(cpuobj, "has_el3", false, NULL);
>          }
>  
> -        if (!vms->virt && object_property_find(cpuobj, "has_el2")) {
> +        if (!vms->virt && !kvm_enabled() &&
> +            object_property_find(cpuobj, "has_el2")) {
>              object_property_set_bool(cpuobj, "has_el2", false, NULL);
>          }
>  
>          if (vms->psci_conduit != QEMU_PSCI_CONDUIT_DISABLED) {
> +            if (kvm_enabled() && ARM_CPU(cpuobj)->has_el2) {
> +                vms->psci_conduit = QEMU_PSCI_CONDUIT_SMC;
> +            }
> +
>              object_property_set_int(cpuobj, "psci-conduit", vms->psci_conduit,
>                                      NULL);

Is there any reason not to do

  vms->virt = object_property_get_bool(OBJECT(first_cpu), "el2", NULL);

right after we do the cpu realize loop here in machvirt_init()? If we did
that we wouldn't need to scatter that object_property_get_bool() around.
We'd just use 'vms->virt'. Actually, shouldn't vms->virt be consistent
with cpu->has_el2 anyway?

>  
> diff --git a/target/arm/cpu.c b/target/arm/cpu.c
> index 30cc330f50..9530a2c4bf 100644
> --- a/target/arm/cpu.c
> +++ b/target/arm/cpu.c
> @@ -1099,7 +1099,7 @@ static Property arm_cpu_rvbar_property =
>  
>  #ifndef CONFIG_USER_ONLY
>  static Property arm_cpu_has_el2_property =
> -            DEFINE_PROP_BOOL("has_el2", ARMCPU, has_el2, true);
> +            DEFINE_PROP_BOOL("has_el2", ARMCPU, has_el2, false);

Doesn't this break TCG's enablement of the feature?

>  
>  static Property arm_cpu_has_el3_property =
>              DEFINE_PROP_BOOL("has_el3", ARMCPU, has_el3, true);
> @@ -2018,6 +2018,7 @@ static void arm_host_initfn(Object *obj)
>      kvm_arm_set_cpu_features_from_host(cpu);
>      if (arm_feature(&cpu->env, ARM_FEATURE_AARCH64)) {
>          aarch64_add_sve_properties(obj);
> +        aarch64_add_el2_properties(obj);
>      }
>      arm_cpu_post_init(obj);
>  }
> diff --git a/target/arm/cpu64.c b/target/arm/cpu64.c
> index 3f3f2c5495..ae8811d09e 100644
> --- a/target/arm/cpu64.c
> +++ b/target/arm/cpu64.c
> @@ -666,6 +666,7 @@ static void aarch64_max_initfn(Object *obj)
>  
>      if (kvm_enabled()) {
>          kvm_arm_set_cpu_features_from_host(cpu);
> +        aarch64_add_el2_properties(obj);
>      } else {
>          uint64_t t;
>          uint32_t u;
> diff --git a/target/arm/kvm64.c b/target/arm/kvm64.c
> index 9cacaf2eb8..7bf892404f 100644
> --- a/target/arm/kvm64.c
> +++ b/target/arm/kvm64.c
> @@ -500,6 +500,7 @@ bool kvm_arm_get_host_cpu_features(ARMHostCPUFeatures *ahcf)
>       */
>      int fdarray[3];
>      bool sve_supported;
> +    bool el2_supported;
>      uint64_t features = 0;
>      uint64_t t;
>      int err;
> @@ -646,6 +647,7 @@ bool kvm_arm_get_host_cpu_features(ARMHostCPUFeatures *ahcf)
>      }
>  
>      sve_supported = ioctl(fdarray[0], KVM_CHECK_EXTENSION, KVM_CAP_ARM_SVE) > 0;
> +    el2_supported = ioctl(fdarray[0], KVM_CHECK_EXTENSION, KVM_CAP_ARM_EL2) > 0;
>  
>      kvm_arm_destroy_scratch_host_vcpu(fdarray);
>  
> @@ -660,6 +662,11 @@ bool kvm_arm_get_host_cpu_features(ARMHostCPUFeatures *ahcf)
>          ahcf->isar.id_aa64pfr0 = t;
>      }
>  
> +    /* Use the ARM_FEATURE_EL2 bit to keep inline with that in TCG mode. */
> +    if (el2_supported) {
> +        features |= 1ULL << ARM_FEATURE_EL2;
> +    }

Do we need to do this? Why not just used kvm_arm_el2_supported()? Note, we
add a check for SVE here because we want to update ID_AA64PFR0. Unless you
want to update ID registers, which maybe you should, then I don't think we
need to touch kvm_arm_get_host_cpu_features().

> +
>      /*
>       * We can assume any KVM supporting CPU is at least a v8
>       * with VFPv4+Neon; this in turn implies most of the other
> @@ -861,6 +868,9 @@ int kvm_arch_init_vcpu(CPUState *cs)
>          assert(kvm_arm_sve_supported());
>          cpu->kvm_init_features[0] |= 1 << KVM_ARM_VCPU_SVE;
>      }
> +    if (arm_feature(&cpu->env, ARM_FEATURE_EL2)) {
> +        cpu->kvm_init_features[0] |= 1 << KVM_ARM_VCPU_HAS_EL2;
> +    }

I feel like there are way too many ways to track this feature now. If I
didn't lose count we have

 1) cpu->has_el2
 2) cpu->env & ARM_FEATURE_EL2
 3) (for mach-virt) vms->virt
 4) possibly (and probably should) some ID register bits

I realize the first three are already in use for TCG, but I'm guessing
we'll want to clean those up. What's the plan going forward? I presume
it'll be (4), but maybe something like (1) and/or (3) will stick around
for convenience. I'm pretty sure we want to avoid (2).

I suggest figuring out what's the best way forward (at least for a next
step) and then post a patch that changes TCG's use to that and then use
that for KVM too.

>  
>      /* Do KVM_ARM_VCPU_INIT ioctl */
>      ret = kvm_arm_vcpu_init(cs);
> -- 
> 2.17.1
> 
> 

Thanks,
drew

Re: [PATCH RESEND v2 5/6] target/arm/cpu: Enable 'el2' to work with host/max cpu

[Peter Maydell]

On Tue, 27 Apr 2021 at 10:24, Andrew Jones <drjones@redhat.com> wrote:
> I feel like there are way too many ways to track this feature now. If I
> didn't lose count we have
>
>  1) cpu->has_el2
>  2) cpu->env & ARM_FEATURE_EL2
>  3) (for mach-virt) vms->virt
>  4) possibly (and probably should) some ID register bits
>
> I realize the first three are already in use for TCG, but I'm guessing
> we'll want to clean those up. What's the plan going forward? I presume
> it'll be (4), but maybe something like (1) and/or (3) will stick around
> for convenience. I'm pretty sure we want to avoid (2).

For new features added we want to use ID register bits. However,
a lot of older pre-existing features are keyed off ARM_FEATURE_FOO
flag bits. Trying to convert an ARM_FEATURE flag to use ID registers
isn't necessarily 100% trivial (for instance, the ARM_FEATURE flag
is always checkable regardless of AArch64 vs AArch32, but the ID
register checks often need to be split up into separate ones checking
the AArch32 or the AArch64 ID register value). So we aren't really
doing conversion of existing flags. (I did a few which were easy
because there were only a handful of checks of them.) As a side note,
some features really can't be checked in ID registers, like
ARM_FEATURE_V8_1M, so env->features is not going to go away completely.

The only use of cpu->has_foo should be for the QOM property -- the
arm_cpu_realizefn() should look at it and then either clear the
ARM_FEATURE flag or update the ID register bits depending on
which one the feature is using.

vms->virt is for the board code (and controls more things than
just whether the CPU itself has EL2).

thanks
-- PMM

Re: [PATCH RESEND v2 5/6] target/arm/cpu: Enable 'el2' to work with host/max cpu

[Andrew Jones]

On Tue, Apr 27, 2021 at 10:38:00AM +0100, Peter Maydell wrote:
> On Tue, 27 Apr 2021 at 10:24, Andrew Jones <drjones@redhat.com> wrote:
> > I feel like there are way too many ways to track this feature now. If I
> > didn't lose count we have
> >
> >  1) cpu->has_el2
> >  2) cpu->env & ARM_FEATURE_EL2
> >  3) (for mach-virt) vms->virt
> >  4) possibly (and probably should) some ID register bits
> >
> > I realize the first three are already in use for TCG, but I'm guessing
> > we'll want to clean those up. What's the plan going forward? I presume
> > it'll be (4), but maybe something like (1) and/or (3) will stick around
> > for convenience. I'm pretty sure we want to avoid (2).
> 
> For new features added we want to use ID register bits. However,
> a lot of older pre-existing features are keyed off ARM_FEATURE_FOO
> flag bits. Trying to convert an ARM_FEATURE flag to use ID registers
> isn't necessarily 100% trivial (for instance, the ARM_FEATURE flag
> is always checkable regardless of AArch64 vs AArch32, but the ID
> register checks often need to be split up into separate ones checking
> the AArch32 or the AArch64 ID register value). So we aren't really
> doing conversion of existing flags. (I did a few which were easy
> because there were only a handful of checks of them.) As a side note,
> some features really can't be checked in ID registers, like
> ARM_FEATURE_V8_1M, so env->features is not going to go away completely.
> 
> The only use of cpu->has_foo should be for the QOM property -- the
> arm_cpu_realizefn() should look at it and then either clear the
> ARM_FEATURE flag or update the ID register bits depending on
> which one the feature is using.
> 
> vms->virt is for the board code (and controls more things than
> just whether the CPU itself has EL2).
>

Thanks for the summary, Peter. For this series, do you recommend
attempting to convert from ARM_FEATURE_EL2 to feature bits first? Or keep
the ARM_FEATURE flag? Also, while I agree we can't use vms->virt for the
same purposes as the CPU feature (however that's tracked), do you agree
vms->virt should be true when the CPU feature is enabled?

Thanks,
drew

Re: [PATCH RESEND v2 5/6] target/arm/cpu: Enable 'el2' to work with host/max cpu

[Peter Maydell]

On Tue, 27 Apr 2021 at 10:49, Andrew Jones <drjones@redhat.com> wrote:
>
> On Tue, Apr 27, 2021 at 10:38:00AM +0100, Peter Maydell wrote:
> > For new features added we want to use ID register bits. However,
> > a lot of older pre-existing features are keyed off ARM_FEATURE_FOO
> > flag bits. Trying to convert an ARM_FEATURE flag to use ID registers
> > isn't necessarily 100% trivial (for instance, the ARM_FEATURE flag
> > is always checkable regardless of AArch64 vs AArch32, but the ID
> > register checks often need to be split up into separate ones checking
> > the AArch32 or the AArch64 ID register value). So we aren't really
> > doing conversion of existing flags. (I did a few which were easy
> > because there were only a handful of checks of them.) As a side note,
> > some features really can't be checked in ID registers, like
> > ARM_FEATURE_V8_1M, so env->features is not going to go away completely.
> >
> > The only use of cpu->has_foo should be for the QOM property -- the
> > arm_cpu_realizefn() should look at it and then either clear the
> > ARM_FEATURE flag or update the ID register bits depending on
> > which one the feature is using.
> >
> > vms->virt is for the board code (and controls more things than
> > just whether the CPU itself has EL2).
> >
>
> Thanks for the summary, Peter. For this series, do you recommend
> attempting to convert from ARM_FEATURE_EL2 to feature bits first? Or keep
> the ARM_FEATURE flag?

I think we should stick with the ARM_FEATURE flag -- there are
enough uses of it that a conversion would be complicated and
I don't think we should tie this feature work up with that.

> Also, while I agree we can't use vms->virt for the
> same purposes as the CPU feature (however that's tracked), do you agree
> vms->virt should be true when the CPU feature is enabled?

It's the other way around -- setting vms->virt should cause the
board code to set the CPU feature. (Conceptually, the board
owns the CPU so it gets to decide what its configuration should
be. The CPU doesn't get to reach out and mess with the board config.)

thanks
-- PMM

[PATCH RESEND v2 6/6] target/arm: Add vCPU feature 'el2' test.

[Haibo Xu]

Signed-off-by: Haibo Xu <haibo.xu@linaro.org>
---
 target/arm/monitor.c           | 2 +-
 tests/qtest/arm-cpu-features.c | 9 +++++++++
 2 files changed, 10 insertions(+), 1 deletion(-)

diff --git a/target/arm/monitor.c b/target/arm/monitor.c
index 80c64fa355..6c39238925 100644
--- a/target/arm/monitor.c
+++ b/target/arm/monitor.c
@@ -90,7 +90,7 @@ QEMU_BUILD_BUG_ON(ARM_MAX_VQ > 16);
  * then the order that considers those dependencies must be used.
  */
 static const char *cpu_model_advertised_features[] = {
-    "aarch64", "pmu", "sve",
+    "aarch64", "pmu", "sve", "el2",
     "sve128", "sve256", "sve384", "sve512",
     "sve640", "sve768", "sve896", "sve1024", "sve1152", "sve1280",
     "sve1408", "sve1536", "sve1664", "sve1792", "sve1920", "sve2048",
diff --git a/tests/qtest/arm-cpu-features.c b/tests/qtest/arm-cpu-features.c
index 8252b85bb8..be07bf0c76 100644
--- a/tests/qtest/arm-cpu-features.c
+++ b/tests/qtest/arm-cpu-features.c
@@ -509,6 +509,7 @@ static void test_query_cpu_model_expansion_kvm(const void *data)
     if (g_str_equal(qtest_get_arch(), "aarch64")) {
         bool kvm_supports_steal_time;
         bool kvm_supports_sve;
+        bool kvm_supports_el2;
         char max_name[8], name[8];
         uint32_t max_vq, vq;
         uint64_t vls;
@@ -533,10 +534,12 @@ static void test_query_cpu_model_expansion_kvm(const void *data)
          */
         assert_has_feature(qts, "host", "kvm-steal-time");
         assert_has_feature(qts, "host", "sve");
+        assert_has_feature(qts, "host", "el2");
 
         resp = do_query_no_props(qts, "host");
         kvm_supports_steal_time = resp_get_feature(resp, "kvm-steal-time");
         kvm_supports_sve = resp_get_feature(resp, "sve");
+        kvm_supports_el2 = resp_get_feature(resp, "el2");
         vls = resp_get_sve_vls(resp);
         qobject_unref(resp);
 
@@ -602,11 +605,17 @@ static void test_query_cpu_model_expansion_kvm(const void *data)
         } else {
             g_assert(vls == 0);
         }
+
+        if (kvm_supports_el2) {
+            assert_set_feature(qts, "host", "el2", false);
+            assert_set_feature(qts, "host", "el2", true);
+        }
     } else {
         assert_has_not_feature(qts, "host", "aarch64");
         assert_has_not_feature(qts, "host", "pmu");
         assert_has_not_feature(qts, "host", "sve");
         assert_has_not_feature(qts, "host", "kvm-steal-time");
+        assert_has_not_feature(qts, "host", "el2");
     }
 
     qtest_quit(qts);
-- 
2.17.1

Re: [PATCH RESEND v2 6/6] target/arm: Add vCPU feature 'el2' test.

[Andrew Jones]

On Thu, Apr 01, 2021 at 05:55:38AM -0700, Haibo Xu wrote:
> Signed-off-by: Haibo Xu <haibo.xu@linaro.org>
> ---
>  target/arm/monitor.c           | 2 +-
>  tests/qtest/arm-cpu-features.c | 9 +++++++++
>  2 files changed, 10 insertions(+), 1 deletion(-)
> 
> diff --git a/target/arm/monitor.c b/target/arm/monitor.c
> index 80c64fa355..6c39238925 100644
> --- a/target/arm/monitor.c
> +++ b/target/arm/monitor.c

The patch summary says we're adding a test, but here we're touching
monitor code. Adding 'el2' to this monitor list should happen in the
patch where el2 is introduced.

> @@ -90,7 +90,7 @@ QEMU_BUILD_BUG_ON(ARM_MAX_VQ > 16);
>   * then the order that considers those dependencies must be used.
>   */
>  static const char *cpu_model_advertised_features[] = {
> -    "aarch64", "pmu", "sve",
> +    "aarch64", "pmu", "sve", "el2",

It doesn't really matter, but I'd rather not add a new feature between
'sve' and 'sve128'. Why not just add it to the front or back of the list?

>      "sve128", "sve256", "sve384", "sve512",
>      "sve640", "sve768", "sve896", "sve1024", "sve1152", "sve1280",
>      "sve1408", "sve1536", "sve1664", "sve1792", "sve1920", "sve2048",
> diff --git a/tests/qtest/arm-cpu-features.c b/tests/qtest/arm-cpu-features.c
> index 8252b85bb8..be07bf0c76 100644
> --- a/tests/qtest/arm-cpu-features.c
> +++ b/tests/qtest/arm-cpu-features.c
> @@ -509,6 +509,7 @@ static void test_query_cpu_model_expansion_kvm(const void *data)
>      if (g_str_equal(qtest_get_arch(), "aarch64")) {
>          bool kvm_supports_steal_time;
>          bool kvm_supports_sve;
> +        bool kvm_supports_el2;
>          char max_name[8], name[8];
>          uint32_t max_vq, vq;
>          uint64_t vls;
> @@ -533,10 +534,12 @@ static void test_query_cpu_model_expansion_kvm(const void *data)
>           */
>          assert_has_feature(qts, "host", "kvm-steal-time");
>          assert_has_feature(qts, "host", "sve");
> +        assert_has_feature(qts, "host", "el2");
>  
>          resp = do_query_no_props(qts, "host");
>          kvm_supports_steal_time = resp_get_feature(resp, "kvm-steal-time");
>          kvm_supports_sve = resp_get_feature(resp, "sve");
> +        kvm_supports_el2 = resp_get_feature(resp, "el2");

Isn't this feature disabled by default whether the host supports it or
not? If so, this will always be false. I think the test should

 1) confirm the feature is disabled by default
 2) attempt to enable it without asserting on failure
 3) if it gets enabled, then attempt to disable it, asserting on failure

>          vls = resp_get_sve_vls(resp);
>          qobject_unref(resp);
>  
> @@ -602,11 +605,17 @@ static void test_query_cpu_model_expansion_kvm(const void *data)
>          } else {
>              g_assert(vls == 0);
>          }
> +
> +        if (kvm_supports_el2) {
> +            assert_set_feature(qts, "host", "el2", false);
> +            assert_set_feature(qts, "host", "el2", true);

For the steps outlined above, you can drop this extra re-enabling test.

> +        }
>      } else {
>          assert_has_not_feature(qts, "host", "aarch64");
>          assert_has_not_feature(qts, "host", "pmu");
>          assert_has_not_feature(qts, "host", "sve");
>          assert_has_not_feature(qts, "host", "kvm-steal-time");
> +        assert_has_not_feature(qts, "host", "el2");
>      }
>  
>      qtest_quit(qts);
> -- 
> 2.17.1
> 
>

Thanks,
drew

Re: [PATCH RESEND v2 0/6] target/arm: Add nested virtualization support

[Andrea Bolognani]

On Thu, 2021-04-01 at 12:55 +0000, Haibo Xu wrote:
> v2:
>   - Move the NV to a CPU feature flag(Andrea&Andrew)
>   - Add CPU feature 'el2' test(Andrew)
> 
> Many thanks to Andrea and Andrew for their comments!
> 
> This series add support for ARMv8.3/8.4 nested virtualization support
> in KVM mode. It's based on Marc Zyngier's kernel KVM patches[1], and 
> has been tested on a FVP model to run a L2 guest with Qemu. Now the 
> feature can be enabled by "-M virt,accel=kvm -cpu host,el2=on" when
> starting a VM. 

This looks good from the user interface point of view, thanks for
addressing the concerns that were raised!

I'll leave Drew to review the actual code changes :)

-- 
Andrea Bolognani / Red Hat / Virtualization

Re: [PATCH RESEND v2 0/6] target/arm: Add nested virtualization support

[Andrew Jones]

On Thu, Apr 01, 2021 at 05:55:32AM -0700, Haibo Xu wrote:
> v2:
>   - Move the NV to a CPU feature flag(Andrea&Andrew)
>   - Add CPU feature 'el2' test(Andrew)
> 
> Many thanks to Andrea and Andrew for their comments!
> 
> This series add support for ARMv8.3/8.4 nested virtualization support
> in KVM mode. It's based on Marc Zyngier's kernel KVM patches[1], and 
> has been tested on a FVP model to run a L2 guest with Qemu. Now the 
> feature can be enabled by "-M virt,accel=kvm -cpu host,el2=on" when
> starting a VM. 
> 
> [1] https://git.kernel.org/pub/scm/linux/kernel/git/maz/arm-platforms.git/log/?h=kvm-arm64/nv-5.12-WIP
> 
> Haibo Xu (6):
>   Update linux header with new arm64 NV macro
>   target/arm/kvm: Add helper to detect el2 when using KVM
>   target/arm/kvm: Add an option to turn on/off el2 support
>   hw/intc/arm_gicv3: Enable support for setting vGIC maintenance IRQ
>   target/arm/cpu: Enable 'el2' to work with host/max cpu
>   target/arm: Add vCPU feature 'el2' test.

Hi Haibo,

Thank you for this new feature.

Please also update docs/system/arm/cpu-features.rst in the next version of
this patch series.

Thanks,
drew

Re: [PATCH RESEND v2 0/6] target/arm: Add nested virtualization support

[Peter Maydell]

On Thu, 1 Apr 2021 at 13:55, Haibo Xu <haibo.xu@linaro.org> wrote:
> This series add support for ARMv8.3/8.4 nested virtualization support
> in KVM mode. It's based on Marc Zyngier's kernel KVM patches[1], and
> has been tested on a FVP model to run a L2 guest with Qemu. Now the
> feature can be enabled by "-M virt,accel=kvm -cpu host,el2=on" when
> starting a VM.

Why are we making the UI for "enable EL2 guest with KVM" different
from that for "enable EL2 guest with TCG" ? Currently an EL2
TCG guest is set up with "-M virt,virtualization=on", which then
does everything it needs to enable virtualization on all the
components on the board including the CPU.

Unless there's a strong technical reason why KVM EL2 has to
be different, I think we should use the same switch.

thanks
-- PMM

Re: [PATCH RESEND v2 0/6] target/arm: Add nested virtualization support

[Andrew Jones]

On Tue, Apr 27, 2021 at 10:42:18AM +0100, Peter Maydell wrote:
> On Thu, 1 Apr 2021 at 13:55, Haibo Xu <haibo.xu@linaro.org> wrote:
> > This series add support for ARMv8.3/8.4 nested virtualization support
> > in KVM mode. It's based on Marc Zyngier's kernel KVM patches[1], and
> > has been tested on a FVP model to run a L2 guest with Qemu. Now the
> > feature can be enabled by "-M virt,accel=kvm -cpu host,el2=on" when
> > starting a VM.
> 
> Why are we making the UI for "enable EL2 guest with KVM" different
> from that for "enable EL2 guest with TCG" ? Currently an EL2
> TCG guest is set up with "-M virt,virtualization=on", which then
> does everything it needs to enable virtualization on all the
> components on the board including the CPU.
> 
> Unless there's a strong technical reason why KVM EL2 has to
> be different, I think we should use the same switch.

I agree we should use the same switch, but I think I'd prefer it be the
CPU switch instead of the machine switch, as it's a CPU feature. There are
some board properties too, like the maintenance interrupt, but we tend to
call a feature a CPU feature when it shows up in the CPU manual, e.g. the
PMU is also a CPU feature, even though it has a PPI.

Thanks,
drew

Re: [PATCH RESEND v2 0/6] target/arm: Add nested virtualization support

[Peter Maydell]

On Tue, 27 Apr 2021 at 10:55, Andrew Jones <drjones@redhat.com> wrote:
>
> On Tue, Apr 27, 2021 at 10:42:18AM +0100, Peter Maydell wrote:
> > Why are we making the UI for "enable EL2 guest with KVM" different
> > from that for "enable EL2 guest with TCG" ? Currently an EL2
> > TCG guest is set up with "-M virt,virtualization=on", which then
> > does everything it needs to enable virtualization on all the
> > components on the board including the CPU.
> >
> > Unless there's a strong technical reason why KVM EL2 has to
> > be different, I think we should use the same switch.
>
> I agree we should use the same switch, but I think I'd prefer it be the
> CPU switch instead of the machine switch, as it's a CPU feature. There are
> some board properties too, like the maintenance interrupt, but we tend to
> call a feature a CPU feature when it shows up in the CPU manual, e.g. the
> PMU is also a CPU feature, even though it has a PPI.

This is mostly not how we've generally opted to handle this kind of thing on
the virt board where there is something that is not merely a CPU feature
but also has effects on the wider system: look at 'virtualization',
'secure' and 'mte'. Granted, the effects of 'virtualization' on the wider
system are less significant than those of 'secure' or 'mte' -- but I think
we implemented 'virtualization' on the same pattern as 'secure'.

If you want to propose changing how we handle those things, including
a backward-compatibility setup so we don't break existing command lines,
I guess we can have that discussion. But we should either *first* do that
change-of-course and *then* implement KVM EL2 to fit into that, or we should
just implement KVM EL2 to fit into the design we have today (and then do
the redesign later if we decide to do that). I don't think we should add
KVM EL2 support's command line switches using a new design that we haven't
committed to and which leaves it completely out of line with what the TCG
handling of the exact same feature is. And I don't feel strongly enough
that our current approach is wrong that I want to impose a "first do this
big rework" precondition on landing the KVM EL2 support.

thanks
-- PMM

Re: [PATCH RESEND v2 0/6] target/arm: Add nested virtualization support

[Andrew Jones]

On Tue, Apr 27, 2021 at 01:15:24PM +0100, Peter Maydell wrote:
> On Tue, 27 Apr 2021 at 10:55, Andrew Jones <drjones@redhat.com> wrote:
> >
> > On Tue, Apr 27, 2021 at 10:42:18AM +0100, Peter Maydell wrote:
> > > Why are we making the UI for "enable EL2 guest with KVM" different
> > > from that for "enable EL2 guest with TCG" ? Currently an EL2
> > > TCG guest is set up with "-M virt,virtualization=on", which then
> > > does everything it needs to enable virtualization on all the
> > > components on the board including the CPU.
> > >
> > > Unless there's a strong technical reason why KVM EL2 has to
> > > be different, I think we should use the same switch.
> >
> > I agree we should use the same switch, but I think I'd prefer it be the
> > CPU switch instead of the machine switch, as it's a CPU feature. There are
> > some board properties too, like the maintenance interrupt, but we tend to
> > call a feature a CPU feature when it shows up in the CPU manual, e.g. the
> > PMU is also a CPU feature, even though it has a PPI.
> 
> This is mostly not how we've generally opted to handle this kind of thing on
> the virt board where there is something that is not merely a CPU feature
> but also has effects on the wider system: look at 'virtualization',
> 'secure' and 'mte'. Granted, the effects of 'virtualization' on the wider
> system are less significant than those of 'secure' or 'mte' -- but I think
> we implemented 'virtualization' on the same pattern as 'secure'.
> 
> If you want to propose changing how we handle those things, including
> a backward-compatibility setup so we don't break existing command lines,
> I guess we can have that discussion. But we should either *first* do that
> change-of-course and *then* implement KVM EL2 to fit into that, or we should
> just implement KVM EL2 to fit into the design we have today (and then do
> the redesign later if we decide to do that). I don't think we should add
> KVM EL2 support's command line switches using a new design that we haven't
> committed to and which leaves it completely out of line with what the TCG
> handling of the exact same feature is. And I don't feel strongly enough
> that our current approach is wrong that I want to impose a "first do this
> big rework" precondition on landing the KVM EL2 support.
>

Since these types of features seem to blur the line between being a CPU
and board property, then I think I'd prefer we call them CPU properties,
as they come from the CPU manual.

Also, if we'd rather not rework 'virtualization' to be a CPU property,
then we'll need libvirt to learn how to probe and set it, whereas if
it's a CPU property we just need to add it to
cpu_model_advertised_features[].

Whichever way we go, we should commit to it, at least for the foreseeable
future, otherwise libvirt and users will have to flipflop their approaches
as well (I'm assuming there have been limited users of this feature
without KVM and libvirt support, so less users would flipflop now than
later.)

I suggest we rework 'virtualization' now with this KVM support series and
'mte' with the series that brings its KVM support too. 'secure' doesn't
currently work with KVM, so it can probably wait until its KVM support
series comes along to be reworked.

Thanks,
drew

Re: [PATCH RESEND v2 0/6] target/arm: Add nested virtualization support

[Peter Maydell]

On Tue, 27 Apr 2021 at 15:48, Andrew Jones <drjones@redhat.com> wrote:

> Since these types of features seem to blur the line between being a CPU
> and board property, then I think I'd prefer we call them CPU properties,
> as they come from the CPU manual.

Conversely, I prefer to call them board properties, because that's
the way it works in hardware: the hardware board has the necessary
support for the system-level feature, and part of that is that it
has an SoC or CPU which has been configured to have the properties
that are needed for the board to support the feature. Having a CPU
that nominally supports a feature is useless if the system as a whole
doesn't handle it.

thanks
-- PMM

Re: [PATCH RESEND v2 0/6] target/arm: Add nested virtualization support

[Peter Maydell]

On Tue, 27 Apr 2021 at 15:58, Peter Maydell <peter.maydell@linaro.org> wrote:
>
> On Tue, 27 Apr 2021 at 15:48, Andrew Jones <drjones@redhat.com> wrote:
>
> > Since these types of features seem to blur the line between being a CPU
> > and board property, then I think I'd prefer we call them CPU properties,
> > as they come from the CPU manual.
>
> Conversely, I prefer to call them board properties, because that's
> the way it works in hardware: the hardware board has the necessary
> support for the system-level feature, and part of that is that it
> has an SoC or CPU which has been configured to have the properties
> that are needed for the board to support the feature. Having a CPU
> that nominally supports a feature is useless if the system as a whole
> doesn't handle it.

...this also means that we're consistent between boards: some board
models unconditionally have support for a feature (and always set it
on the CPU, GIC, etc), some don't ever support the feature (and always
disable it), and a few offer the user the choice. Having the user
use CPU properties suggests that they can, for instance, plug a
has-el3 CPU into any board model, which in general won't work.

-- PMM

Re: [PATCH RESEND v2 0/6] target/arm: Add nested virtualization support

[Andrew Jones]

On Tue, Apr 27, 2021 at 04:01:10PM +0100, Peter Maydell wrote:
> On Tue, 27 Apr 2021 at 15:58, Peter Maydell <peter.maydell@linaro.org> wrote:
> >
> > On Tue, 27 Apr 2021 at 15:48, Andrew Jones <drjones@redhat.com> wrote:
> >
> > > Since these types of features seem to blur the line between being a CPU
> > > and board property, then I think I'd prefer we call them CPU properties,
> > > as they come from the CPU manual.
> >
> > Conversely, I prefer to call them board properties, because that's
> > the way it works in hardware: the hardware board has the necessary
> > support for the system-level feature, and part of that is that it
> > has an SoC or CPU which has been configured to have the properties
> > that are needed for the board to support the feature. Having a CPU
> > that nominally supports a feature is useless if the system as a whole
> > doesn't handle it.
> 
> ...this also means that we're consistent between boards: some board
> models unconditionally have support for a feature (and always set it
> on the CPU, GIC, etc), some don't ever support the feature (and always
> disable it), and a few offer the user the choice. Having the user
> use CPU properties suggests that they can, for instance, plug a
> has-el3 CPU into any board model, which in general won't work.
>

I feel like this can be looked at both ways. A board can have a supporting
CPU or a CPU can have a supporting board. While a CPU is physically
mounted in a board, giving it a natural "physical member of" relationship
to the board, a board's design is driven by the features of the CPU, which
leads to the board having a "implements dependencies of" relationship to
the CPU.

I think the way we look at it depends on what we consider our top level
requirements to be. If it's a board specification that we're implementing,
then we clearly look at it from the board perspective. However, for mach-
virt, we don't have much of a board specification. We want it to be a
minimal board that supports VIRTIO and CPU features.

Maybe this is a place where our perspective and interface should diverge
between mach-virt and the emulated board models?

All that said, if we'd rather start thinking about system features, then
should we rework 'pmu' and perhaps other CPU features, which might better
be considered system features? Also, is the '-M virt,\?' type of probing
sufficient? Don't we need some sort of probing that considers both the
board support and the CPU support? 'virt' might support a system feature
that '-M virt -cpu xyz' does not support, right? How do users (libvirt)
know if they need to probe both the board and the CPU for feature support?
How do we probe the CPU's support for the feature if we don't want to
expose the feature as a CPU property? And, if we do expose the feature
as a CPU property, then what should be the response to enabling it without
the board property? Error out or imply its enablement when it's available?

I think we need some sort of system feature document to explain what a
system feature is and how it combines board properties together with CPU
features (which may or may not be exposed as properties). We'll need
to document how to properly do the probing and we'll also need tests to
check all our {board-property, cpu-type, cpu-property} misconfiguration
possibilities for proper error handling.

Thanks,
drew