[PATCH v3 0/3] Add support for Fujitsu A64FX processor

[PATCH v3 0/3] Add support for Fujitsu A64FX processor

[Shuuichirou Ishii]

This is the v3 patch series.

v3:
When we created the v2 patch series, we based it on the v1 patch series
that had not been merged into the upstream, so we created the v3 patch
series as a patch series that can be applied independently.

v2:
No features have been added or removed from the v1 patch series. Removal
of unused definitions that were added in excess, and consolidation of
patches for the purpose of functional consistency.

For patch 1, Implemented Identification registers for A64FX processor.
HPC extension registers will be implemented in the future.

For patch 2, A64FX processor can now be used by specifying the -cpu
a64fx option when the -macine virt option is specified.

For patch 3, added A64FX processor related tests.

Shuuichirou Ishii (3):
  target-arm: cpu64: Add support for Fujitsu A64FX
  hw/arm/virt: target-arm: Add A64FX processor support to virt machine
  tests/arm-cpu-features: Add A64FX processor related tests

 docs/system/arm/virt.rst       |  1 +
 hw/arm/virt.c                  |  1 +
 target/arm/cpu64.c             | 44 ++++++++++++++++++++++++++++++++++
 tests/qtest/arm-cpu-features.c |  3 +++
 4 files changed, 49 insertions(+)

-- 
2.27.0

[PATCH v3 1/3] target-arm: cpu64: Add support for Fujitsu A64FX

[Shuuichirou Ishii]

Add a definition for the Fujitsu A64FX processor.

The A64FX processor does not implement the AArch32 Execution state,
so there are no associated AArch32 Identification registers.

Signed-off-by: Shuuichirou Ishii <ishii.shuuichir@fujitsu.com>
---
 target/arm/cpu64.c | 44 ++++++++++++++++++++++++++++++++++++++++++++
 1 file changed, 44 insertions(+)

diff --git a/target/arm/cpu64.c b/target/arm/cpu64.c
index c690318a9b..612644941b 100644
--- a/target/arm/cpu64.c
+++ b/target/arm/cpu64.c
@@ -847,10 +847,54 @@ static void aarch64_max_initfn(Object *obj)
                         cpu_max_set_sve_max_vq, NULL, NULL);
 }
 
+static void aarch64_a64fx_initfn(Object *obj)
+{
+    ARMCPU *cpu = ARM_CPU(obj);
+
+    cpu->dtb_compatible = "arm,a64fx";
+    set_feature(&cpu->env, ARM_FEATURE_V8);
+    set_feature(&cpu->env, ARM_FEATURE_NEON);
+    set_feature(&cpu->env, ARM_FEATURE_GENERIC_TIMER);
+    set_feature(&cpu->env, ARM_FEATURE_AARCH64);
+    set_feature(&cpu->env, ARM_FEATURE_EL2);
+    set_feature(&cpu->env, ARM_FEATURE_EL3);
+    set_feature(&cpu->env, ARM_FEATURE_PMU);
+    cpu->midr = 0x461f0010;
+    cpu->revidr = 0x00000000;
+    cpu->ctr = 86668006;
+    cpu->reset_sctlr = 0x30000180;
+    cpu->isar.id_aa64pfr0 =   0x0000000101111111; /* No RAS Extensions */
+    cpu->isar.id_aa64pfr1 = 0x0000000000000000;
+    cpu->isar.id_aa64dfr0 = 0x0000000010305408;
+    cpu->isar.id_aa64dfr1 = 0x0000000000000000;
+    cpu->id_aa64afr0 = 0x0000000000000000;
+    cpu->id_aa64afr1 = 0x0000000000000000;
+    cpu->isar.id_aa64mmfr0 = 0x0000000000001122;
+    cpu->isar.id_aa64mmfr1 = 0x0000000011212100;
+    cpu->isar.id_aa64mmfr2 = 0x0000000000001011;
+    cpu->isar.id_aa64isar0 = 0x0000000010211120;
+    cpu->isar.id_aa64isar1 = 0x0000000000010001;
+    cpu->isar.id_aa64zfr0 = 0x0000000000000000;
+    cpu->clidr = 0x0000000080000023;
+    cpu->ccsidr[0] = 0x7007e01c; /* 64KB L1 dcache */
+    cpu->ccsidr[1] = 0x2007e01c; /* 64KB L1 icache */
+    cpu->ccsidr[2] = 0x70ffe07c; /* 8MB L2 cache */
+    cpu->dcz_blocksize = 6; /* 256 bytes */
+    cpu->gic_num_lrs = 4;
+    cpu->gic_vpribits = 5;
+    cpu->gic_vprebits = 5;
+    /* TODO:  Add A64FX specific HPC extension registers */
+
+    aarch64_add_sve_properties(obj);
+    object_property_add(obj, "sve-max-vq", "uint32", cpu_max_get_sve_max_vq,
+                        cpu_max_set_sve_max_vq, NULL, NULL);
+}
+
 static const ARMCPUInfo aarch64_cpus[] = {
     { .name = "cortex-a57",         .initfn = aarch64_a57_initfn },
     { .name = "cortex-a53",         .initfn = aarch64_a53_initfn },
     { .name = "cortex-a72",         .initfn = aarch64_a72_initfn },
+    { .name = "a64fx",              .initfn = aarch64_a64fx_initfn },
     { .name = "max",                .initfn = aarch64_max_initfn },
 };
 
-- 
2.27.0

[PATCH v3 2/3] hw/arm/virt: target-arm: Add A64FX processor support to virt machine

[Shuuichirou Ishii]

Add -cpu a64fx to use A64FX processor when -machine virt option is specified.
In addition, add a64fx to the Supported guest CPU types in the virt.rst document.

Signed-off-by: Shuuichirou Ishii <ishii.shuuichir@fujitsu.com>
---
 docs/system/arm/virt.rst | 1 +
 hw/arm/virt.c            | 1 +
 2 files changed, 2 insertions(+)

diff --git a/docs/system/arm/virt.rst b/docs/system/arm/virt.rst
index 59acf0eeaf..850787495b 100644
--- a/docs/system/arm/virt.rst
+++ b/docs/system/arm/virt.rst
@@ -55,6 +55,7 @@ Supported guest CPU types:
 - ``cortex-a53`` (64-bit)
 - ``cortex-a57`` (64-bit)
 - ``cortex-a72`` (64-bit)
+- ``a64fx`` (64-bit)
 - ``host`` (with KVM only)
 - ``max`` (same as ``host`` for KVM; best possible emulation with TCG)
 
diff --git a/hw/arm/virt.c b/hw/arm/virt.c
index 81eda46b0b..10286d3fd6 100644
--- a/hw/arm/virt.c
+++ b/hw/arm/virt.c
@@ -200,6 +200,7 @@ static const char *valid_cpus[] = {
     ARM_CPU_TYPE_NAME("cortex-a53"),
     ARM_CPU_TYPE_NAME("cortex-a57"),
     ARM_CPU_TYPE_NAME("cortex-a72"),
+    ARM_CPU_TYPE_NAME("a64fx"),
     ARM_CPU_TYPE_NAME("host"),
     ARM_CPU_TYPE_NAME("max"),
 };
-- 
2.27.0

[PATCH v3 3/3] tests/arm-cpu-features: Add A64FX processor related tests

[Shuuichirou Ishii]

Signed-off-by: Shuuichirou Ishii <ishii.shuuichir@fujitsu.com>
---
 tests/qtest/arm-cpu-features.c | 3 +++
 1 file changed, 3 insertions(+)

diff --git a/tests/qtest/arm-cpu-features.c b/tests/qtest/arm-cpu-features.c
index 8252b85bb8..979c6f82f8 100644
--- a/tests/qtest/arm-cpu-features.c
+++ b/tests/qtest/arm-cpu-features.c
@@ -472,6 +472,9 @@ static void test_query_cpu_model_expansion(const void *data)
         assert_has_feature_enabled(qts, "max", "sve128");
         assert_has_feature_enabled(qts, "cortex-a57", "pmu");
         assert_has_feature_enabled(qts, "cortex-a57", "aarch64");
+        assert_has_feature_enabled(qts, "a64fx", "pmu");
+        assert_has_feature_enabled(qts, "a64fx", "aarch64");
+        assert_has_feature_enabled(qts, "a64fx", "sve");
 
         sve_tests_default(qts, "max");
         pauth_tests_default(qts, "max");
-- 
2.27.0

Re: [PATCH v3 1/3] target-arm: cpu64: Add support for Fujitsu A64FX

[Andrew Jones]

On Thu, Aug 05, 2021 at 04:30:43PM +0900, Shuuichirou Ishii wrote:
> Add a definition for the Fujitsu A64FX processor.
> 
> The A64FX processor does not implement the AArch32 Execution state,
> so there are no associated AArch32 Identification registers.
> 
> Signed-off-by: Shuuichirou Ishii <ishii.shuuichir@fujitsu.com>
> ---
>  target/arm/cpu64.c | 44 ++++++++++++++++++++++++++++++++++++++++++++
>  1 file changed, 44 insertions(+)
> 
> diff --git a/target/arm/cpu64.c b/target/arm/cpu64.c
> index c690318a9b..612644941b 100644
> --- a/target/arm/cpu64.c
> +++ b/target/arm/cpu64.c
> @@ -847,10 +847,54 @@ static void aarch64_max_initfn(Object *obj)
>                          cpu_max_set_sve_max_vq, NULL, NULL);
>  }
>  
> +static void aarch64_a64fx_initfn(Object *obj)
> +{
> +    ARMCPU *cpu = ARM_CPU(obj);
> +
> +    cpu->dtb_compatible = "arm,a64fx";
> +    set_feature(&cpu->env, ARM_FEATURE_V8);
> +    set_feature(&cpu->env, ARM_FEATURE_NEON);
> +    set_feature(&cpu->env, ARM_FEATURE_GENERIC_TIMER);
> +    set_feature(&cpu->env, ARM_FEATURE_AARCH64);
> +    set_feature(&cpu->env, ARM_FEATURE_EL2);
> +    set_feature(&cpu->env, ARM_FEATURE_EL3);
> +    set_feature(&cpu->env, ARM_FEATURE_PMU);
> +    cpu->midr = 0x461f0010;
> +    cpu->revidr = 0x00000000;
> +    cpu->ctr = 86668006;
> +    cpu->reset_sctlr = 0x30000180;
> +    cpu->isar.id_aa64pfr0 =   0x0000000101111111; /* No RAS Extensions */
> +    cpu->isar.id_aa64pfr1 = 0x0000000000000000;
> +    cpu->isar.id_aa64dfr0 = 0x0000000010305408;
> +    cpu->isar.id_aa64dfr1 = 0x0000000000000000;
> +    cpu->id_aa64afr0 = 0x0000000000000000;
> +    cpu->id_aa64afr1 = 0x0000000000000000;
> +    cpu->isar.id_aa64mmfr0 = 0x0000000000001122;
> +    cpu->isar.id_aa64mmfr1 = 0x0000000011212100;
> +    cpu->isar.id_aa64mmfr2 = 0x0000000000001011;
> +    cpu->isar.id_aa64isar0 = 0x0000000010211120;
> +    cpu->isar.id_aa64isar1 = 0x0000000000010001;
> +    cpu->isar.id_aa64zfr0 = 0x0000000000000000;
> +    cpu->clidr = 0x0000000080000023;
> +    cpu->ccsidr[0] = 0x7007e01c; /* 64KB L1 dcache */
> +    cpu->ccsidr[1] = 0x2007e01c; /* 64KB L1 icache */
> +    cpu->ccsidr[2] = 0x70ffe07c; /* 8MB L2 cache */
> +    cpu->dcz_blocksize = 6; /* 256 bytes */
> +    cpu->gic_num_lrs = 4;
> +    cpu->gic_vpribits = 5;
> +    cpu->gic_vprebits = 5;
> +    /* TODO:  Add A64FX specific HPC extension registers */
> +
> +    aarch64_add_sve_properties(obj);
> +    object_property_add(obj, "sve-max-vq", "uint32", cpu_max_get_sve_max_vq,
> +                        cpu_max_set_sve_max_vq, NULL, NULL);

I'm not a huge fan of the sve-max-vq property since it's not a good idea
to use it with KVM, because it's not explicit enough for migration[1]. I
realize the a64fx cpu type will likely never be used with KVM, but since
sve-max-vq isn't necessary[2], than I would avoid propagating it to
another cpu type. Finally, if you want the a64fx cpu model to represent
the current a64fx cpu, then don't you want to explicitly set the supported
vector lengths[3] and deny the user the option to change them? You could
do that by directly setting the vq map and not adding the sve properties.

[1] With KVM, sve-max-vq only tells you the maximum vq, but it won't tell
you that the host doesn't support non-power-of-2 vector lengths. So you
don't get an explicit vector length list on the command line. Being
explicit is the only safe way to migrate (see
docs/system/arm/cpu-features.rst:"SVE CPU Property Recommendations").

[2] If a shorthand is desired for specifying vector lengths, then just
use a single sve<N> property. For example, sve-max-vq=4 and sve512=on
are identical (but keep [1] in mind).

[3] a64fx only support 128, 256, and 512 bit vector lengths, afaik.

Thanks,
drew

> +}
> +
>  static const ARMCPUInfo aarch64_cpus[] = {
>      { .name = "cortex-a57",         .initfn = aarch64_a57_initfn },
>      { .name = "cortex-a53",         .initfn = aarch64_a53_initfn },
>      { .name = "cortex-a72",         .initfn = aarch64_a72_initfn },
> +    { .name = "a64fx",              .initfn = aarch64_a64fx_initfn },
>      { .name = "max",                .initfn = aarch64_max_initfn },
>  };
>  
> -- 
> 2.27.0
> 
> 

RE: [PATCH v3 1/3] target-arm: cpu64: Add support for Fujitsu A64FX

[ishii.shuuichir]

Thanks for your comments.

Before reposting the fix patch series,
based on your comments and the v3 1/3 patch,
we have considered the following fixes.

If you have any comments on the fixes, please let us know.

---

diff --git a/target/arm/cpu.h b/target/arm/cpu.h
index 9f0a5f84d5..84ebca731a 100644
--- a/target/arm/cpu.h
+++ b/target/arm/cpu.h
@@ -2145,6 +2145,7 @@ enum arm_features {
     ARM_FEATURE_M_SECURITY, /* M profile Security Extension */
     ARM_FEATURE_M_MAIN, /* M profile Main Extension */
     ARM_FEATURE_V8_1M, /* M profile extras only in v8.1M and later */
+    ARM_FEATURE_A64FX, /* Fujitsu A64FX processor */
 };

 static inline int arm_feature(CPUARMState *env, int feature)
diff --git a/target/arm/cpu64.c b/target/arm/cpu64.c
index 612644941b..62dcb6a919 100644
--- a/target/arm/cpu64.c
+++ b/target/arm/cpu64.c
@@ -248,6 +248,21 @@ static void aarch64_a72_initfn(Object *obj)
     define_arm_cp_regs(cpu, cortex_a72_a57_a53_cp_reginfo);
 }

+static void a64fx_cpu_set_sve(ARMCPU *cpu)
+{
+    /* Suppport of A64FX's vector length are 128,256 and 512byte only */
+    bitmap_zero(cpu->sve_vq_map, ARM_MAX_VQ);
+    bitmap_zero(cpu->sve_vq_init, ARM_MAX_VQ);
+    set_bit(0, cpu->sve_vq_map); /* 128byte */
+    set_bit(0, cpu->sve_vq_init);
+    set_bit(1, cpu->sve_vq_map); /* 256byte */
+    set_bit(1, cpu->sve_vq_init);
+    set_bit(3, cpu->sve_vq_map); /* 512byte */
+    set_bit(3, cpu->sve_vq_init);
+    cpu->sve_max_vq = find_last_bit(cpu->sve_vq_map, ARM_MAX_VQ) + 1;
+
+}
 void arm_cpu_sve_finalize(ARMCPU *cpu, Error **errp)
 {
     /*
@@ -448,6 +463,10 @@ void arm_cpu_sve_finalize(ARMCPU *cpu, Error **errp)

     /* From now on sve_max_vq is the actual maximum supported length. */
     cpu->sve_max_vq = max_vq;
+
+       if(arm_feature(&cpu->env, ARM_FEATURE_A64FX)) {
+        a64fx_cpu_set_sve(cpu);
+    }
 }

 static void cpu_max_get_sve_max_vq(Object *obj, Visitor *v, const char *name,
@@ -852,6 +871,7 @@ static void aarch64_a64fx_initfn(Object *obj)
     ARMCPU *cpu = ARM_CPU(obj);

     cpu->dtb_compatible = "arm,a64fx";
+    set_feature(&cpu->env, ARM_FEATURE_A64FX);
     set_feature(&cpu->env, ARM_FEATURE_V8);
     set_feature(&cpu->env, ARM_FEATURE_NEON);
     set_feature(&cpu->env, ARM_FEATURE_GENERIC_TIMER);
@@ -884,10 +904,6 @@ static void aarch64_a64fx_initfn(Object *obj)
     cpu->gic_vpribits = 5;
     cpu->gic_vprebits = 5;
     /* TODO:  Add A64FX specific HPC extension registers */
-
-    aarch64_add_sve_properties(obj);
-    object_property_add(obj, "sve-max-vq", "uint32", cpu_max_get_sve_max_vq,
-                        cpu_max_set_sve_max_vq, NULL, NULL);
 }

 static const ARMCPUInfo aarch64_cpus[] = {

---

Best regards.


> -----Original Message-----
> From: Andrew Jones <drjones@redhat.com>
> Sent: Thursday, August 5, 2021 8:25 PM
> To: Ishii, Shuuichirou <ishii.shuuichir@fujitsu.com>
> Cc: peter.maydell@linaro.org; qemu-arm@nongnu.org; qemu-devel@nongnu.org
> Subject: Re: [PATCH v3 1/3] target-arm: cpu64: Add support for Fujitsu A64FX
> 
> On Thu, Aug 05, 2021 at 04:30:43PM +0900, Shuuichirou Ishii wrote:
> > Add a definition for the Fujitsu A64FX processor.
> >
> > The A64FX processor does not implement the AArch32 Execution state, so
> > there are no associated AArch32 Identification registers.
> >
> > Signed-off-by: Shuuichirou Ishii <ishii.shuuichir@fujitsu.com>
> > ---
> >  target/arm/cpu64.c | 44
> ++++++++++++++++++++++++++++++++++++++++++++
> >  1 file changed, 44 insertions(+)
> >
> > diff --git a/target/arm/cpu64.c b/target/arm/cpu64.c index
> > c690318a9b..612644941b 100644
> > --- a/target/arm/cpu64.c
> > +++ b/target/arm/cpu64.c
> > @@ -847,10 +847,54 @@ static void aarch64_max_initfn(Object *obj)
> >                          cpu_max_set_sve_max_vq, NULL, NULL);  }
> >
> > +static void aarch64_a64fx_initfn(Object *obj) {
> > +    ARMCPU *cpu = ARM_CPU(obj);
> > +
> > +    cpu->dtb_compatible = "arm,a64fx";
> > +    set_feature(&cpu->env, ARM_FEATURE_V8);
> > +    set_feature(&cpu->env, ARM_FEATURE_NEON);
> > +    set_feature(&cpu->env, ARM_FEATURE_GENERIC_TIMER);
> > +    set_feature(&cpu->env, ARM_FEATURE_AARCH64);
> > +    set_feature(&cpu->env, ARM_FEATURE_EL2);
> > +    set_feature(&cpu->env, ARM_FEATURE_EL3);
> > +    set_feature(&cpu->env, ARM_FEATURE_PMU);
> > +    cpu->midr = 0x461f0010;
> > +    cpu->revidr = 0x00000000;
> > +    cpu->ctr = 86668006;
> > +    cpu->reset_sctlr = 0x30000180;
> > +    cpu->isar.id_aa64pfr0 =   0x0000000101111111; /* No RAS Extensions
> */
> > +    cpu->isar.id_aa64pfr1 = 0x0000000000000000;
> > +    cpu->isar.id_aa64dfr0 = 0x0000000010305408;
> > +    cpu->isar.id_aa64dfr1 = 0x0000000000000000;
> > +    cpu->id_aa64afr0 = 0x0000000000000000;
> > +    cpu->id_aa64afr1 = 0x0000000000000000;
> > +    cpu->isar.id_aa64mmfr0 = 0x0000000000001122;
> > +    cpu->isar.id_aa64mmfr1 = 0x0000000011212100;
> > +    cpu->isar.id_aa64mmfr2 = 0x0000000000001011;
> > +    cpu->isar.id_aa64isar0 = 0x0000000010211120;
> > +    cpu->isar.id_aa64isar1 = 0x0000000000010001;
> > +    cpu->isar.id_aa64zfr0 = 0x0000000000000000;
> > +    cpu->clidr = 0x0000000080000023;
> > +    cpu->ccsidr[0] = 0x7007e01c; /* 64KB L1 dcache */
> > +    cpu->ccsidr[1] = 0x2007e01c; /* 64KB L1 icache */
> > +    cpu->ccsidr[2] = 0x70ffe07c; /* 8MB L2 cache */
> > +    cpu->dcz_blocksize = 6; /* 256 bytes */
> > +    cpu->gic_num_lrs = 4;
> > +    cpu->gic_vpribits = 5;
> > +    cpu->gic_vprebits = 5;
> > +    /* TODO:  Add A64FX specific HPC extension registers */
> > +
> > +    aarch64_add_sve_properties(obj);
> > +    object_property_add(obj, "sve-max-vq", "uint32",
> cpu_max_get_sve_max_vq,
> > +                        cpu_max_set_sve_max_vq, NULL, NULL);
> 
> I'm not a huge fan of the sve-max-vq property since it's not a good idea to use it
> with KVM, because it's not explicit enough for migration[1]. I realize the a64fx cpu
> type will likely never be used with KVM, but since sve-max-vq isn't necessary[2],
> than I would avoid propagating it to another cpu type. Finally, if you want the a64fx
> cpu model to represent the current a64fx cpu, then don't you want to explicitly set
> the supported vector lengths[3] and deny the user the option to change them? You
> could do that by directly setting the vq map and not adding the sve properties.
> 
> [1] With KVM, sve-max-vq only tells you the maximum vq, but it won't tell you that
> the host doesn't support non-power-of-2 vector lengths. So you don't get an
> explicit vector length list on the command line. Being explicit is the only safe way
> to migrate (see docs/system/arm/cpu-features.rst:"SVE CPU Property
> Recommendations").
> 
> [2] If a shorthand is desired for specifying vector lengths, then just use a single
> sve<N> property. For example, sve-max-vq=4 and sve512=on are identical (but
> keep [1] in mind).
> 
> [3] a64fx only support 128, 256, and 512 bit vector lengths, afaik.
> 
> Thanks,
> drew
> 
> > +}
> > +
> >  static const ARMCPUInfo aarch64_cpus[] = {
> >      { .name = "cortex-a57",         .initfn = aarch64_a57_initfn },
> >      { .name = "cortex-a53",         .initfn = aarch64_a53_initfn },
> >      { .name = "cortex-a72",         .initfn = aarch64_a72_initfn },
> > +    { .name = "a64fx",              .initfn = aarch64_a64fx_initfn },
> >      { .name = "max",                .initfn = aarch64_max_initfn },
> >  };
> >
> > --
> > 2.27.0
> >
> >

Re: [PATCH v3 1/3] target-arm: cpu64: Add support for Fujitsu A64FX

[Andrew Jones]

On Tue, Aug 10, 2021 at 08:23:39AM +0000, ishii.shuuichir@fujitsu.com wrote:
> 
> Thanks for your comments.
> 
> Before reposting the fix patch series,
> based on your comments and the v3 1/3 patch,
> we have considered the following fixes.
> 
> If you have any comments on the fixes, please let us know.
> 
> ---
> 
> diff --git a/target/arm/cpu.h b/target/arm/cpu.h
> index 9f0a5f84d5..84ebca731a 100644
> --- a/target/arm/cpu.h
> +++ b/target/arm/cpu.h
> @@ -2145,6 +2145,7 @@ enum arm_features {
>      ARM_FEATURE_M_SECURITY, /* M profile Security Extension */
>      ARM_FEATURE_M_MAIN, /* M profile Main Extension */
>      ARM_FEATURE_V8_1M, /* M profile extras only in v8.1M and later */
> +    ARM_FEATURE_A64FX, /* Fujitsu A64FX processor */
>  };
> 
>  static inline int arm_feature(CPUARMState *env, int feature)
> diff --git a/target/arm/cpu64.c b/target/arm/cpu64.c
> index 612644941b..62dcb6a919 100644
> --- a/target/arm/cpu64.c
> +++ b/target/arm/cpu64.c
> @@ -248,6 +248,21 @@ static void aarch64_a72_initfn(Object *obj)
>      define_arm_cp_regs(cpu, cortex_a72_a57_a53_cp_reginfo);
>  }
> 
> +static void a64fx_cpu_set_sve(ARMCPU *cpu)
> +{
> +    /* Suppport of A64FX's vector length are 128,256 and 512byte only */

Missing spaces in text and s/byte/bit/

> +    bitmap_zero(cpu->sve_vq_map, ARM_MAX_VQ);
> +    bitmap_zero(cpu->sve_vq_init, ARM_MAX_VQ);
> +    set_bit(0, cpu->sve_vq_map); /* 128byte */
> +    set_bit(0, cpu->sve_vq_init);
> +    set_bit(1, cpu->sve_vq_map); /* 256byte */
> +    set_bit(1, cpu->sve_vq_init);
> +    set_bit(3, cpu->sve_vq_map); /* 512byte */
> +    set_bit(3, cpu->sve_vq_init);

For all the comments in the above function s/byte/bit/

> +    cpu->sve_max_vq = find_last_bit(cpu->sve_vq_map, ARM_MAX_VQ) + 1;
> +

Extra blank line

> +}
>  void arm_cpu_sve_finalize(ARMCPU *cpu, Error **errp)
>  {
>      /*
> @@ -448,6 +463,10 @@ void arm_cpu_sve_finalize(ARMCPU *cpu, Error **errp)
> 
>      /* From now on sve_max_vq is the actual maximum supported length. */
>      cpu->sve_max_vq = max_vq;
> +
> +       if(arm_feature(&cpu->env, ARM_FEATURE_A64FX)) {
> +        a64fx_cpu_set_sve(cpu);
> +    }

Bad indentation and spacing, but I don't think this is the right place
for this. I wouldn't even let ARM_FEATURE_A64FX enter
arm_cpu_sve_finalize, since we know it doesn't support sve cpu properties.
While it's ugly wherever we put it, since we have to special case it, I
think it's less ugly at the callsite

diff --git a/target/arm/cpu.c b/target/arm/cpu.c
index 2866dd765882..225800ec361c 100644
--- a/target/arm/cpu.c
+++ b/target/arm/cpu.c
@@ -1350,10 +1350,14 @@ void arm_cpu_finalize_features(ARMCPU *cpu, Error **errp)
     Error *local_err = NULL;
 
     if (arm_feature(&cpu->env, ARM_FEATURE_AARCH64)) {
-        arm_cpu_sve_finalize(cpu, &local_err);
-        if (local_err != NULL) {
-            error_propagate(errp, local_err);
-            return;
+        if (arm_feature(&cpu->env, ARM_FEATURE_A64FX)) {
+            a64fx_cpu_set_sve(cpu);
+        } else {
+            arm_cpu_sve_finalize(cpu, &local_err);
+            if (local_err != NULL) {
+                error_propagate(errp, local_err);
+                return;
+            }
         }
 
         /*

>  }
> 
>  static void cpu_max_get_sve_max_vq(Object *obj, Visitor *v, const char *name,
> @@ -852,6 +871,7 @@ static void aarch64_a64fx_initfn(Object *obj)
>      ARMCPU *cpu = ARM_CPU(obj);
> 
>      cpu->dtb_compatible = "arm,a64fx";
> +    set_feature(&cpu->env, ARM_FEATURE_A64FX);
>      set_feature(&cpu->env, ARM_FEATURE_V8);
>      set_feature(&cpu->env, ARM_FEATURE_NEON);
>      set_feature(&cpu->env, ARM_FEATURE_GENERIC_TIMER);
> @@ -884,10 +904,6 @@ static void aarch64_a64fx_initfn(Object *obj)
>      cpu->gic_vpribits = 5;
>      cpu->gic_vprebits = 5;
>      /* TODO:  Add A64FX specific HPC extension registers */
> -
> -    aarch64_add_sve_properties(obj);
> -    object_property_add(obj, "sve-max-vq", "uint32", cpu_max_get_sve_max_vq,
> -                        cpu_max_set_sve_max_vq, NULL, NULL);
>  }
> 
>  static const ARMCPUInfo aarch64_cpus[] = {

Otherwise looks OK to me.

Thanks,
drew

> 
> ---
> 
> Best regards.
> 
> 
> > -----Original Message-----
> > From: Andrew Jones <drjones@redhat.com>
> > Sent: Thursday, August 5, 2021 8:25 PM
> > To: Ishii, Shuuichirou <ishii.shuuichir@fujitsu.com>
> > Cc: peter.maydell@linaro.org; qemu-arm@nongnu.org; qemu-devel@nongnu.org
> > Subject: Re: [PATCH v3 1/3] target-arm: cpu64: Add support for Fujitsu A64FX
> > 
> > On Thu, Aug 05, 2021 at 04:30:43PM +0900, Shuuichirou Ishii wrote:
> > > Add a definition for the Fujitsu A64FX processor.
> > >
> > > The A64FX processor does not implement the AArch32 Execution state, so
> > > there are no associated AArch32 Identification registers.
> > >
> > > Signed-off-by: Shuuichirou Ishii <ishii.shuuichir@fujitsu.com>
> > > ---
> > >  target/arm/cpu64.c | 44
> > ++++++++++++++++++++++++++++++++++++++++++++
> > >  1 file changed, 44 insertions(+)
> > >
> > > diff --git a/target/arm/cpu64.c b/target/arm/cpu64.c index
> > > c690318a9b..612644941b 100644
> > > --- a/target/arm/cpu64.c
> > > +++ b/target/arm/cpu64.c
> > > @@ -847,10 +847,54 @@ static void aarch64_max_initfn(Object *obj)
> > >                          cpu_max_set_sve_max_vq, NULL, NULL);  }
> > >
> > > +static void aarch64_a64fx_initfn(Object *obj) {
> > > +    ARMCPU *cpu = ARM_CPU(obj);
> > > +
> > > +    cpu->dtb_compatible = "arm,a64fx";
> > > +    set_feature(&cpu->env, ARM_FEATURE_V8);
> > > +    set_feature(&cpu->env, ARM_FEATURE_NEON);
> > > +    set_feature(&cpu->env, ARM_FEATURE_GENERIC_TIMER);
> > > +    set_feature(&cpu->env, ARM_FEATURE_AARCH64);
> > > +    set_feature(&cpu->env, ARM_FEATURE_EL2);
> > > +    set_feature(&cpu->env, ARM_FEATURE_EL3);
> > > +    set_feature(&cpu->env, ARM_FEATURE_PMU);
> > > +    cpu->midr = 0x461f0010;
> > > +    cpu->revidr = 0x00000000;
> > > +    cpu->ctr = 86668006;
> > > +    cpu->reset_sctlr = 0x30000180;
> > > +    cpu->isar.id_aa64pfr0 =   0x0000000101111111; /* No RAS Extensions
> > */
> > > +    cpu->isar.id_aa64pfr1 = 0x0000000000000000;
> > > +    cpu->isar.id_aa64dfr0 = 0x0000000010305408;
> > > +    cpu->isar.id_aa64dfr1 = 0x0000000000000000;
> > > +    cpu->id_aa64afr0 = 0x0000000000000000;
> > > +    cpu->id_aa64afr1 = 0x0000000000000000;
> > > +    cpu->isar.id_aa64mmfr0 = 0x0000000000001122;
> > > +    cpu->isar.id_aa64mmfr1 = 0x0000000011212100;
> > > +    cpu->isar.id_aa64mmfr2 = 0x0000000000001011;
> > > +    cpu->isar.id_aa64isar0 = 0x0000000010211120;
> > > +    cpu->isar.id_aa64isar1 = 0x0000000000010001;
> > > +    cpu->isar.id_aa64zfr0 = 0x0000000000000000;
> > > +    cpu->clidr = 0x0000000080000023;
> > > +    cpu->ccsidr[0] = 0x7007e01c; /* 64KB L1 dcache */
> > > +    cpu->ccsidr[1] = 0x2007e01c; /* 64KB L1 icache */
> > > +    cpu->ccsidr[2] = 0x70ffe07c; /* 8MB L2 cache */
> > > +    cpu->dcz_blocksize = 6; /* 256 bytes */
> > > +    cpu->gic_num_lrs = 4;
> > > +    cpu->gic_vpribits = 5;
> > > +    cpu->gic_vprebits = 5;
> > > +    /* TODO:  Add A64FX specific HPC extension registers */
> > > +
> > > +    aarch64_add_sve_properties(obj);
> > > +    object_property_add(obj, "sve-max-vq", "uint32",
> > cpu_max_get_sve_max_vq,
> > > +                        cpu_max_set_sve_max_vq, NULL, NULL);
> > 
> > I'm not a huge fan of the sve-max-vq property since it's not a good idea to use it
> > with KVM, because it's not explicit enough for migration[1]. I realize the a64fx cpu
> > type will likely never be used with KVM, but since sve-max-vq isn't necessary[2],
> > than I would avoid propagating it to another cpu type. Finally, if you want the a64fx
> > cpu model to represent the current a64fx cpu, then don't you want to explicitly set
> > the supported vector lengths[3] and deny the user the option to change them? You
> > could do that by directly setting the vq map and not adding the sve properties.
> > 
> > [1] With KVM, sve-max-vq only tells you the maximum vq, but it won't tell you that
> > the host doesn't support non-power-of-2 vector lengths. So you don't get an
> > explicit vector length list on the command line. Being explicit is the only safe way
> > to migrate (see docs/system/arm/cpu-features.rst:"SVE CPU Property
> > Recommendations").
> > 
> > [2] If a shorthand is desired for specifying vector lengths, then just use a single
> > sve<N> property. For example, sve-max-vq=4 and sve512=on are identical (but
> > keep [1] in mind).
> > 
> > [3] a64fx only support 128, 256, and 512 bit vector lengths, afaik.
> > 
> > Thanks,
> > drew
> > 
> > > +}
> > > +
> > >  static const ARMCPUInfo aarch64_cpus[] = {
> > >      { .name = "cortex-a57",         .initfn = aarch64_a57_initfn },
> > >      { .name = "cortex-a53",         .initfn = aarch64_a53_initfn },
> > >      { .name = "cortex-a72",         .initfn = aarch64_a72_initfn },
> > > +    { .name = "a64fx",              .initfn = aarch64_a64fx_initfn },
> > >      { .name = "max",                .initfn = aarch64_max_initfn },
> > >  };
> > >
> > > --
> > > 2.27.0
> > >
> > >
> 

RE: [PATCH v3 1/3] target-arm: cpu64: Add support for Fujitsu A64FX

[ishii.shuuichir]

Thank you very much for the source review and the patch.
We will create a series of V4 patches based on your comments.

Best regards.

> -----Original Message-----
> From: Andrew Jones <drjones@redhat.com>
> Sent: Tuesday, August 10, 2021 8:41 PM
> To: Ishii, Shuuichirou/ <ishii.shuuichir@fujitsu.com>
> Cc: peter.maydell@linaro.org; qemu-arm@nongnu.org; qemu-devel@nongnu.org
> Subject: Re: [PATCH v3 1/3] target-arm: cpu64: Add support for Fujitsu A64FX
> 
> On Tue, Aug 10, 2021 at 08:23:39AM +0000, ishii.shuuichir@fujitsu.com wrote:
> >
> > Thanks for your comments.
> >
> > Before reposting the fix patch series, based on your comments and the
> > v3 1/3 patch, we have considered the following fixes.
> >
> > If you have any comments on the fixes, please let us know.
> >
> > ---
> >
> > diff --git a/target/arm/cpu.h b/target/arm/cpu.h index
> > 9f0a5f84d5..84ebca731a 100644
> > --- a/target/arm/cpu.h
> > +++ b/target/arm/cpu.h
> > @@ -2145,6 +2145,7 @@ enum arm_features {
> >      ARM_FEATURE_M_SECURITY, /* M profile Security Extension */
> >      ARM_FEATURE_M_MAIN, /* M profile Main Extension */
> >      ARM_FEATURE_V8_1M, /* M profile extras only in v8.1M and later */
> > +    ARM_FEATURE_A64FX, /* Fujitsu A64FX processor */
> >  };
> >
> >  static inline int arm_feature(CPUARMState *env, int feature) diff
> > --git a/target/arm/cpu64.c b/target/arm/cpu64.c index
> > 612644941b..62dcb6a919 100644
> > --- a/target/arm/cpu64.c
> > +++ b/target/arm/cpu64.c
> > @@ -248,6 +248,21 @@ static void aarch64_a72_initfn(Object *obj)
> >      define_arm_cp_regs(cpu, cortex_a72_a57_a53_cp_reginfo);  }
> >
> > +static void a64fx_cpu_set_sve(ARMCPU *cpu) {
> > +    /* Suppport of A64FX's vector length are 128,256 and 512byte only
> > +*/
> 
> Missing spaces in text and s/byte/bit/
> 
> > +    bitmap_zero(cpu->sve_vq_map, ARM_MAX_VQ);
> > +    bitmap_zero(cpu->sve_vq_init, ARM_MAX_VQ);
> > +    set_bit(0, cpu->sve_vq_map); /* 128byte */
> > +    set_bit(0, cpu->sve_vq_init);
> > +    set_bit(1, cpu->sve_vq_map); /* 256byte */
> > +    set_bit(1, cpu->sve_vq_init);
> > +    set_bit(3, cpu->sve_vq_map); /* 512byte */
> > +    set_bit(3, cpu->sve_vq_init);
> 
> For all the comments in the above function s/byte/bit/
> 
> > +    cpu->sve_max_vq = find_last_bit(cpu->sve_vq_map, ARM_MAX_VQ) +
> 1;
> > +
> 
> Extra blank line
> 
> > +}
> >  void arm_cpu_sve_finalize(ARMCPU *cpu, Error **errp)  {
> >      /*
> > @@ -448,6 +463,10 @@ void arm_cpu_sve_finalize(ARMCPU *cpu, Error
> > **errp)
> >
> >      /* From now on sve_max_vq is the actual maximum supported length. */
> >      cpu->sve_max_vq = max_vq;
> > +
> > +       if(arm_feature(&cpu->env, ARM_FEATURE_A64FX)) {
> > +        a64fx_cpu_set_sve(cpu);
> > +    }
> 
> Bad indentation and spacing, but I don't think this is the right place for this. I
> wouldn't even let ARM_FEATURE_A64FX enter arm_cpu_sve_finalize, since we
> know it doesn't support sve cpu properties.
> While it's ugly wherever we put it, since we have to special case it, I think it's less
> ugly at the callsite
> 
> diff --git a/target/arm/cpu.c b/target/arm/cpu.c index
> 2866dd765882..225800ec361c 100644
> --- a/target/arm/cpu.c
> +++ b/target/arm/cpu.c
> @@ -1350,10 +1350,14 @@ void arm_cpu_finalize_features(ARMCPU *cpu, Error
> **errp)
>      Error *local_err = NULL;
> 
>      if (arm_feature(&cpu->env, ARM_FEATURE_AARCH64)) {
> -        arm_cpu_sve_finalize(cpu, &local_err);
> -        if (local_err != NULL) {
> -            error_propagate(errp, local_err);
> -            return;
> +        if (arm_feature(&cpu->env, ARM_FEATURE_A64FX)) {
> +            a64fx_cpu_set_sve(cpu);
> +        } else {
> +            arm_cpu_sve_finalize(cpu, &local_err);
> +            if (local_err != NULL) {
> +                error_propagate(errp, local_err);
> +                return;
> +            }
>          }
> 
>          /*
> 
> >  }
> >
> >  static void cpu_max_get_sve_max_vq(Object *obj, Visitor *v, const
> > char *name, @@ -852,6 +871,7 @@ static void aarch64_a64fx_initfn(Object
> *obj)
> >      ARMCPU *cpu = ARM_CPU(obj);
> >
> >      cpu->dtb_compatible = "arm,a64fx";
> > +    set_feature(&cpu->env, ARM_FEATURE_A64FX);
> >      set_feature(&cpu->env, ARM_FEATURE_V8);
> >      set_feature(&cpu->env, ARM_FEATURE_NEON);
> >      set_feature(&cpu->env, ARM_FEATURE_GENERIC_TIMER); @@ -884,10
> > +904,6 @@ static void aarch64_a64fx_initfn(Object *obj)
> >      cpu->gic_vpribits = 5;
> >      cpu->gic_vprebits = 5;
> >      /* TODO:  Add A64FX specific HPC extension registers */
> > -
> > -    aarch64_add_sve_properties(obj);
> > -    object_property_add(obj, "sve-max-vq", "uint32",
> cpu_max_get_sve_max_vq,
> > -                        cpu_max_set_sve_max_vq, NULL, NULL);
> >  }
> >
> >  static const ARMCPUInfo aarch64_cpus[] = {
> 
> Otherwise looks OK to me.
> 
> Thanks,
> drew
> 
> >
> > ---
> >
> > Best regards.
> >
> >
> > > -----Original Message-----
> > > From: Andrew Jones <drjones@redhat.com>
> > > Sent: Thursday, August 5, 2021 8:25 PM
> > > To: Ishii, Shuuichirou <ishii.shuuichir@fujitsu.com>
> > > Cc: peter.maydell@linaro.org; qemu-arm@nongnu.org;
> > > qemu-devel@nongnu.org
> > > Subject: Re: [PATCH v3 1/3] target-arm: cpu64: Add support for
> > > Fujitsu A64FX
> > >
> > > On Thu, Aug 05, 2021 at 04:30:43PM +0900, Shuuichirou Ishii wrote:
> > > > Add a definition for the Fujitsu A64FX processor.
> > > >
> > > > The A64FX processor does not implement the AArch32 Execution
> > > > state, so there are no associated AArch32 Identification registers.
> > > >
> > > > Signed-off-by: Shuuichirou Ishii <ishii.shuuichir@fujitsu.com>
> > > > ---
> > > >  target/arm/cpu64.c | 44
> > > ++++++++++++++++++++++++++++++++++++++++++++
> > > >  1 file changed, 44 insertions(+)
> > > >
> > > > diff --git a/target/arm/cpu64.c b/target/arm/cpu64.c index
> > > > c690318a9b..612644941b 100644
> > > > --- a/target/arm/cpu64.c
> > > > +++ b/target/arm/cpu64.c
> > > > @@ -847,10 +847,54 @@ static void aarch64_max_initfn(Object *obj)
> > > >                          cpu_max_set_sve_max_vq, NULL, NULL);  }
> > > >
> > > > +static void aarch64_a64fx_initfn(Object *obj) {
> > > > +    ARMCPU *cpu = ARM_CPU(obj);
> > > > +
> > > > +    cpu->dtb_compatible = "arm,a64fx";
> > > > +    set_feature(&cpu->env, ARM_FEATURE_V8);
> > > > +    set_feature(&cpu->env, ARM_FEATURE_NEON);
> > > > +    set_feature(&cpu->env, ARM_FEATURE_GENERIC_TIMER);
> > > > +    set_feature(&cpu->env, ARM_FEATURE_AARCH64);
> > > > +    set_feature(&cpu->env, ARM_FEATURE_EL2);
> > > > +    set_feature(&cpu->env, ARM_FEATURE_EL3);
> > > > +    set_feature(&cpu->env, ARM_FEATURE_PMU);
> > > > +    cpu->midr = 0x461f0010;
> > > > +    cpu->revidr = 0x00000000;
> > > > +    cpu->ctr = 86668006;
> > > > +    cpu->reset_sctlr = 0x30000180;
> > > > +    cpu->isar.id_aa64pfr0 =   0x0000000101111111; /* No RAS
> Extensions
> > > */
> > > > +    cpu->isar.id_aa64pfr1 = 0x0000000000000000;
> > > > +    cpu->isar.id_aa64dfr0 = 0x0000000010305408;
> > > > +    cpu->isar.id_aa64dfr1 = 0x0000000000000000;
> > > > +    cpu->id_aa64afr0 = 0x0000000000000000;
> > > > +    cpu->id_aa64afr1 = 0x0000000000000000;
> > > > +    cpu->isar.id_aa64mmfr0 = 0x0000000000001122;
> > > > +    cpu->isar.id_aa64mmfr1 = 0x0000000011212100;
> > > > +    cpu->isar.id_aa64mmfr2 = 0x0000000000001011;
> > > > +    cpu->isar.id_aa64isar0 = 0x0000000010211120;
> > > > +    cpu->isar.id_aa64isar1 = 0x0000000000010001;
> > > > +    cpu->isar.id_aa64zfr0 = 0x0000000000000000;
> > > > +    cpu->clidr = 0x0000000080000023;
> > > > +    cpu->ccsidr[0] = 0x7007e01c; /* 64KB L1 dcache */
> > > > +    cpu->ccsidr[1] = 0x2007e01c; /* 64KB L1 icache */
> > > > +    cpu->ccsidr[2] = 0x70ffe07c; /* 8MB L2 cache */
> > > > +    cpu->dcz_blocksize = 6; /* 256 bytes */
> > > > +    cpu->gic_num_lrs = 4;
> > > > +    cpu->gic_vpribits = 5;
> > > > +    cpu->gic_vprebits = 5;
> > > > +    /* TODO:  Add A64FX specific HPC extension registers */
> > > > +
> > > > +    aarch64_add_sve_properties(obj);
> > > > +    object_property_add(obj, "sve-max-vq", "uint32",
> > > cpu_max_get_sve_max_vq,
> > > > +                        cpu_max_set_sve_max_vq, NULL, NULL);
> > >
> > > I'm not a huge fan of the sve-max-vq property since it's not a good
> > > idea to use it with KVM, because it's not explicit enough for
> > > migration[1]. I realize the a64fx cpu type will likely never be used
> > > with KVM, but since sve-max-vq isn't necessary[2], than I would
> > > avoid propagating it to another cpu type. Finally, if you want the
> > > a64fx cpu model to represent the current a64fx cpu, then don't you
> > > want to explicitly set the supported vector lengths[3] and deny the user the
> option to change them? You could do that by directly setting the vq map and not
> adding the sve properties.
> > >
> > > [1] With KVM, sve-max-vq only tells you the maximum vq, but it won't
> > > tell you that the host doesn't support non-power-of-2 vector
> > > lengths. So you don't get an explicit vector length list on the
> > > command line. Being explicit is the only safe way to migrate (see
> > > docs/system/arm/cpu-features.rst:"SVE CPU Property Recommendations").
> > >
> > > [2] If a shorthand is desired for specifying vector lengths, then
> > > just use a single sve<N> property. For example, sve-max-vq=4 and
> > > sve512=on are identical (but keep [1] in mind).
> > >
> > > [3] a64fx only support 128, 256, and 512 bit vector lengths, afaik.
> > >
> > > Thanks,
> > > drew
> > >
> > > > +}
> > > > +
> > > >  static const ARMCPUInfo aarch64_cpus[] = {
> > > >      { .name = "cortex-a57",         .initfn = aarch64_a57_initfn },
> > > >      { .name = "cortex-a53",         .initfn = aarch64_a53_initfn },
> > > >      { .name = "cortex-a72",         .initfn = aarch64_a72_initfn },
> > > > +    { .name = "a64fx",              .initfn = aarch64_a64fx_initfn },
> > > >      { .name = "max",                .initfn = aarch64_max_initfn },
> > > >  };
> > > >
> > > > --
> > > > 2.27.0
> > > >
> > > >
> >