target/arm: Conditionalize arm_div assert on aarch32 support

[Qemu-devel] [PATCH] target/arm: Conditionalize arm_div assert on aarch32 support

Posted by Richard Henderson 5 years, 5 months ago

When populating id registers from kvm, on a host that doesn't support
aarch32 mode at all, aa32_arm_div will not be supported either.

Signed-off-by: Richard Henderson <richard.henderson@linaro.org>
---

"Tested" on an APM Mustang, which does support AArch32.  I'm not
sure, off hand, which cpu(s) don't have it, and Alex didn't say
in his bug report.  Tsk tsk.  ;-)


r~

---
 target/arm/cpu.h |  5 +++++
 target/arm/cpu.c | 10 +++++++++-
 2 files changed, 14 insertions(+), 1 deletion(-)

diff --git a/target/arm/cpu.h b/target/arm/cpu.h
index 895f9909d8..4521ad5ae8 100644
--- a/target/arm/cpu.h
+++ b/target/arm/cpu.h
@@ -3300,6 +3300,11 @@ static inline bool isar_feature_aa64_fp16(const ARMISARegisters *id)
     return FIELD_EX64(id->id_aa64pfr0, ID_AA64PFR0, FP) == 1;
 }
 
+static inline bool isar_feature_aa64_a32(const ARMISARegisters *id)
+{
+    return FIELD_EX64(id->id_aa64pfr0, ID_AA64PFR0, EL0) == 2;
+}
+
 static inline bool isar_feature_aa64_sve(const ARMISARegisters *id)
 {
     return FIELD_EX64(id->id_aa64pfr0, ID_AA64PFR0, SVE) != 0;
diff --git a/target/arm/cpu.c b/target/arm/cpu.c
index e08a2d2d79..988d97d1f1 100644
--- a/target/arm/cpu.c
+++ b/target/arm/cpu.c
@@ -828,8 +828,16 @@ static void arm_cpu_realizefn(DeviceState *dev, Error **errp)
          * include the various other features that V7VE implies.
          * Presence of EL2 itself is ARM_FEATURE_EL2, and of the
          * Security Extensions is ARM_FEATURE_EL3.
+         *
+         * V7VE requires ARM division.  However, there exist AArch64 cpus
+         * without AArch32 support.  When KVM queries ID_ISAR0_EL1 on such
+         * a host, the value is UNKNOWN.  Similarly, we cannot check
+         * ID_AA64PFR0 without AArch64 support.  Check everything in order.
          */
-        assert(cpu_isar_feature(arm_div, cpu));
+        if (arm_feature(&cpu->env, ARM_FEATURE_AARCH64)
+            && cpu_isar_feature(aa64_a32, cpu)) {
+            assert(cpu_isar_feature(arm_div, cpu));
+        }
         set_feature(env, ARM_FEATURE_LPAE);
         set_feature(env, ARM_FEATURE_V7);
     }
-- 
2.17.2

Re: [Qemu-devel] [PATCH] target/arm: Conditionalize arm_div assert on aarch32 support

Posted by Philippe Mathieu-Daudé 5 years, 5 months ago

On 1/11/18 22:57, Richard Henderson wrote:
> When populating id registers from kvm, on a host that doesn't support
> aarch32 mode at all, aa32_arm_div will not be supported either.
> 
> Signed-off-by: Richard Henderson <richard.henderson@linaro.org>
> ---
> 
> "Tested" on an APM Mustang, which does support AArch32.  I'm not
> sure, off hand, which cpu(s) don't have it, and Alex didn't say
> in his bug report.  Tsk tsk.  ;-)

Packet provides access to cavium,thunder-88xx cpus which lack aa32.

> 
> 
> r~
> 
> ---
>   target/arm/cpu.h |  5 +++++
>   target/arm/cpu.c | 10 +++++++++-
>   2 files changed, 14 insertions(+), 1 deletion(-)
> 
> diff --git a/target/arm/cpu.h b/target/arm/cpu.h
> index 895f9909d8..4521ad5ae8 100644
> --- a/target/arm/cpu.h
> +++ b/target/arm/cpu.h
> @@ -3300,6 +3300,11 @@ static inline bool isar_feature_aa64_fp16(const ARMISARegisters *id)
>       return FIELD_EX64(id->id_aa64pfr0, ID_AA64PFR0, FP) == 1;
>   }
>   
> +static inline bool isar_feature_aa64_a32(const ARMISARegisters *id)
> +{
> +    return FIELD_EX64(id->id_aa64pfr0, ID_AA64PFR0, EL0) == 2;
> +}
> +
>   static inline bool isar_feature_aa64_sve(const ARMISARegisters *id)
>   {
>       return FIELD_EX64(id->id_aa64pfr0, ID_AA64PFR0, SVE) != 0;
> diff --git a/target/arm/cpu.c b/target/arm/cpu.c
> index e08a2d2d79..988d97d1f1 100644
> --- a/target/arm/cpu.c
> +++ b/target/arm/cpu.c
> @@ -828,8 +828,16 @@ static void arm_cpu_realizefn(DeviceState *dev, Error **errp)
>            * include the various other features that V7VE implies.
>            * Presence of EL2 itself is ARM_FEATURE_EL2, and of the
>            * Security Extensions is ARM_FEATURE_EL3.
> +         *
> +         * V7VE requires ARM division.  However, there exist AArch64 cpus
> +         * without AArch32 support.  When KVM queries ID_ISAR0_EL1 on such
> +         * a host, the value is UNKNOWN.  Similarly, we cannot check
> +         * ID_AA64PFR0 without AArch64 support.  Check everything in order.
>            */
> -        assert(cpu_isar_feature(arm_div, cpu));
> +        if (arm_feature(&cpu->env, ARM_FEATURE_AARCH64)
> +            && cpu_isar_feature(aa64_a32, cpu)) {
> +            assert(cpu_isar_feature(arm_div, cpu));
> +        }
>           set_feature(env, ARM_FEATURE_LPAE);
>           set_feature(env, ARM_FEATURE_V7);
>       }
>

Re: [Qemu-devel] [PATCH] target/arm: Conditionalize arm_div assert on aarch32 support

Posted by Alex Bennée 5 years, 5 months ago

Richard Henderson <richard.henderson@linaro.org> writes:

> When populating id registers from kvm, on a host that doesn't support
> aarch32 mode at all, aa32_arm_div will not be supported either.
>
> Signed-off-by: Richard Henderson <richard.henderson@linaro.org>
> ---
>
> "Tested" on an APM Mustang, which does support AArch32.  I'm not
> sure, off hand, which cpu(s) don't have it, and Alex didn't say
> in his bug report.  Tsk tsk.  ;-)

It's qemu-test - which I think is a ThunderX. Unfortunately I think we
need the same treatment for the Jazelle test:

  ./aarch64-softmmu/qemu-system-aarch64 -machine virt,gic-version=3 -accel kvm -cpu host -serial mon:stdio -nic user,model=virtio-net-pci,hostfwd=tcp::2222-:22 -device virtio-scsi-pci -kernel ../linux.git/arch/arm64/boot/Image -append "console=ttyAMA0 panic=-1" -display none -m 4096 --no-reboot
  qemu-system-aarch64: /home/alex/lsrc/qemu.git/target/arm/cpu.c:866: arm_cpu_realizefn: Assertion `cpu_isar_feature(jazelle, cpu)' failed.
  fish: “./aarch64-softmmu/qemu-system-a…” terminated by signal SIGABRT (Abort)


>
>
> r~
>
> ---
>  target/arm/cpu.h |  5 +++++
>  target/arm/cpu.c | 10 +++++++++-
>  2 files changed, 14 insertions(+), 1 deletion(-)
>
> diff --git a/target/arm/cpu.h b/target/arm/cpu.h
> index 895f9909d8..4521ad5ae8 100644
> --- a/target/arm/cpu.h
> +++ b/target/arm/cpu.h
> @@ -3300,6 +3300,11 @@ static inline bool isar_feature_aa64_fp16(const ARMISARegisters *id)
>      return FIELD_EX64(id->id_aa64pfr0, ID_AA64PFR0, FP) == 1;
>  }
>
> +static inline bool isar_feature_aa64_a32(const ARMISARegisters *id)
> +{
> +    return FIELD_EX64(id->id_aa64pfr0, ID_AA64PFR0, EL0) == 2;
> +}
> +
>  static inline bool isar_feature_aa64_sve(const ARMISARegisters *id)
>  {
>      return FIELD_EX64(id->id_aa64pfr0, ID_AA64PFR0, SVE) != 0;
> diff --git a/target/arm/cpu.c b/target/arm/cpu.c
> index e08a2d2d79..988d97d1f1 100644
> --- a/target/arm/cpu.c
> +++ b/target/arm/cpu.c
> @@ -828,8 +828,16 @@ static void arm_cpu_realizefn(DeviceState *dev, Error **errp)
>           * include the various other features that V7VE implies.
>           * Presence of EL2 itself is ARM_FEATURE_EL2, and of the
>           * Security Extensions is ARM_FEATURE_EL3.
> +         *
> +         * V7VE requires ARM division.  However, there exist AArch64 cpus
> +         * without AArch32 support.  When KVM queries ID_ISAR0_EL1 on such
> +         * a host, the value is UNKNOWN.  Similarly, we cannot check
> +         * ID_AA64PFR0 without AArch64 support.  Check everything in order.
>           */
> -        assert(cpu_isar_feature(arm_div, cpu));
> +        if (arm_feature(&cpu->env, ARM_FEATURE_AARCH64)
> +            && cpu_isar_feature(aa64_a32, cpu)) {
> +            assert(cpu_isar_feature(arm_div, cpu));
> +        }
>          set_feature(env, ARM_FEATURE_LPAE);
>          set_feature(env, ARM_FEATURE_V7);
>      }


--
Alex Bennée

Re: [Qemu-devel] [PATCH] target/arm: Conditionalize arm_div assert on aarch32 support

Posted by Peter Maydell 5 years, 5 months ago

On 1 November 2018 at 21:57, Richard Henderson
<richard.henderson@linaro.org> wrote:
> When populating id registers from kvm, on a host that doesn't support
> aarch32 mode at all, aa32_arm_div will not be supported either.
>
> Signed-off-by: Richard Henderson <richard.henderson@linaro.org>
> ---
>
> "Tested" on an APM Mustang, which does support AArch32.  I'm not
> sure, off hand, which cpu(s) don't have it, and Alex didn't say
> in his bug report.  Tsk tsk.  ;-)
>
>
> r~
>
> ---
>  target/arm/cpu.h |  5 +++++
>  target/arm/cpu.c | 10 +++++++++-
>  2 files changed, 14 insertions(+), 1 deletion(-)
>
> diff --git a/target/arm/cpu.h b/target/arm/cpu.h
> index 895f9909d8..4521ad5ae8 100644
> --- a/target/arm/cpu.h
> +++ b/target/arm/cpu.h
> @@ -3300,6 +3300,11 @@ static inline bool isar_feature_aa64_fp16(const ARMISARegisters *id)
>      return FIELD_EX64(id->id_aa64pfr0, ID_AA64PFR0, FP) == 1;
>  }
>
> +static inline bool isar_feature_aa64_a32(const ARMISARegisters *id)
> +{
> +    return FIELD_EX64(id->id_aa64pfr0, ID_AA64PFR0, EL0) == 2;
> +}
> +

Doesn't the stuff in the Arm ARM's "Principles of the ID
scheme for fields in ID registers" about signed and unsigned
values for ID register fields strictly mean you want to be
testing (unsigned) >= 2 here rather than strict equality?

thanks
-- PMM

Re: [Qemu-devel] [PATCH] target/arm: Conditionalize arm_div assert on aarch32 support

Posted by Richard Henderson 5 years, 5 months ago

On 11/2/18 9:48 AM, Peter Maydell wrote:
>> +static inline bool isar_feature_aa64_a32(const ARMISARegisters *id)
>> +{
>> +    return FIELD_EX64(id->id_aa64pfr0, ID_AA64PFR0, EL0) == 2;
>> +}
>> +
> 
> Doesn't the stuff in the Arm ARM's "Principles of the ID
> scheme for fields in ID registers" about signed and unsigned
> values for ID register fields strictly mean you want to be
> testing (unsigned) >= 2 here rather than strict equality?

Yes.  Will fix.


r~