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.

For SVE, the A64FX processor supports only 128,256 and 512bit vector lengths.

Signed-off-by: Shuuichirou Ishii <ishii.shuuichir@fujitsu.com>
---
 target/arm/cpu.c   | 27 +++++++++++++++++++++++----
 target/arm/cpu.h   |  1 +
 target/arm/cpu64.c | 42 ++++++++++++++++++++++++++++++++++++++++++
 3 files changed, 66 insertions(+), 4 deletions(-)

diff --git a/target/arm/cpu.c b/target/arm/cpu.c
index 2866dd7658..162e46afc3 100644
--- a/target/arm/cpu.c
+++ b/target/arm/cpu.c
@@ -1345,15 +1345,34 @@ static void arm_cpu_finalizefn(Object *obj)
 #endif
 }
 
+static void a64fx_cpu_set_sve(ARMCPU *cpu)
+{
+    /* Suppport of A64FX's vector length are 128,256 and 512bit 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); /* 128bit */
+    set_bit(0, cpu->sve_vq_init);
+    set_bit(1, cpu->sve_vq_map); /* 256bit */
+    set_bit(1, cpu->sve_vq_init);
+    set_bit(3, cpu->sve_vq_map); /* 512bit */
+    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_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;
+            }
         }
 
         /*
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 c690318a9b..5e7e885f9d 100644
--- a/target/arm/cpu64.c
+++ b/target/arm/cpu64.c
@@ -847,10 +847,52 @@ 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_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 = 0x86668006;
+    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 */
+}
+
 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

On Thu, Aug 12, 2021 at 03:04:38PM +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.
> 
> For SVE, the A64FX processor supports only 128,256 and 512bit vector lengths.
> 
> Signed-off-by: Shuuichirou Ishii <ishii.shuuichir@fujitsu.com>
> ---
>  target/arm/cpu.c   | 27 +++++++++++++++++++++++----
>  target/arm/cpu.h   |  1 +
>  target/arm/cpu64.c | 42 ++++++++++++++++++++++++++++++++++++++++++
>  3 files changed, 66 insertions(+), 4 deletions(-)
> 
> diff --git a/target/arm/cpu.c b/target/arm/cpu.c
> index 2866dd7658..162e46afc3 100644
> --- a/target/arm/cpu.c
> +++ b/target/arm/cpu.c
> @@ -1345,15 +1345,34 @@ static void arm_cpu_finalizefn(Object *obj)
>  #endif
>  }
>  
> +static void a64fx_cpu_set_sve(ARMCPU *cpu)
> +{
> +    /* Suppport of A64FX's vector length are 128,256 and 512bit 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); /* 128bit */
> +    set_bit(0, cpu->sve_vq_init);
> +    set_bit(1, cpu->sve_vq_map); /* 256bit */
> +    set_bit(1, cpu->sve_vq_init);
> +    set_bit(3, cpu->sve_vq_map); /* 512bit */
> +    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_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;
> +            }
>          }
>  
>          /*
> 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 c690318a9b..5e7e885f9d 100644
> --- a/target/arm/cpu64.c
> +++ b/target/arm/cpu64.c
> @@ -847,10 +847,52 @@ 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_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 = 0x86668006;
> +    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 */
> +}
> +
>  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
>

For the SVE related bits

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

On Thu, Aug 12, 2021 at 11:16:50AM +0200, Andrew Jones wrote:
> On Thu, Aug 12, 2021 at 03:04:38PM +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.
> > 
> > For SVE, the A64FX processor supports only 128,256 and 512bit vector lengths.
> > 
> > Signed-off-by: Shuuichirou Ishii <ishii.shuuichir@fujitsu.com>
> > ---
> >  target/arm/cpu.c   | 27 +++++++++++++++++++++++----
> >  target/arm/cpu.h   |  1 +
> >  target/arm/cpu64.c | 42 ++++++++++++++++++++++++++++++++++++++++++
> >  3 files changed, 66 insertions(+), 4 deletions(-)
> > 
> > diff --git a/target/arm/cpu.c b/target/arm/cpu.c
> > index 2866dd7658..162e46afc3 100644
> > --- a/target/arm/cpu.c
> > +++ b/target/arm/cpu.c
> > @@ -1345,15 +1345,34 @@ static void arm_cpu_finalizefn(Object *obj)
> >  #endif
> >  }
> >  
> > +static void a64fx_cpu_set_sve(ARMCPU *cpu)
> > +{
> > +    /* Suppport of A64FX's vector length are 128,256 and 512bit 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); /* 128bit */
> > +    set_bit(0, cpu->sve_vq_init);
> > +    set_bit(1, cpu->sve_vq_map); /* 256bit */
> > +    set_bit(1, cpu->sve_vq_init);
> > +    set_bit(3, cpu->sve_vq_map); /* 512bit */
> > +    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_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;
> > +            }
> >          }
> >  
> >          /*
> > 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 c690318a9b..5e7e885f9d 100644
> > --- a/target/arm/cpu64.c
> > +++ b/target/arm/cpu64.c
> > @@ -847,10 +847,52 @@ 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_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 = 0x86668006;
> > +    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 */
> > +}
> > +
> >  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
> >
> 
> For the SVE related bits
> 
> Reviewed-by: Andrew Jones <drjones@redhat.com>

On second thought, do we want the QMP CPU model expansion query to show
that this CPU type has sve,sve128,sve256,sve512? If so, then our SVE work
isn't complete, because we need those properties, set true by default, but
forbidden from changing.

Thanks,
drew

On Thu, 12 Aug 2021 at 10:25, Andrew Jones <drjones@redhat.com> wrote:
> On second thought, do we want the QMP CPU model expansion query to show
> that this CPU type has sve,sve128,sve256,sve512? If so, then our SVE work
> isn't complete, because we need those properties, set true by default, but
> forbidden from changing.

Do we have precedent elsewhere (arm, x86, wherever) for "this CPU object
exposes these properties as constant unwriteable" ?

-- PMM

> On Thu, 12 Aug 2021 at 10:25, Andrew Jones <drjones@redhat.com> wrote:
> > On second thought, do we want the QMP CPU model expansion query to
> > show that this CPU type has sve,sve128,sve256,sve512? If so, then our
> > SVE work isn't complete, because we need those properties, set true by
> > default, but forbidden from changing.
> 
> Do we have precedent elsewhere (arm, x86, wherever) for "this CPU object
> exposes these properties as constant unwriteable" ?

We have not yet conducted a confirmation of Peter's question, but...

> On second thought, do we want the QMP CPU model expansion query to show that
> this CPU type has sve,sve128,sve256,sve512? If so, then our SVE work isn't
> complete, because we need those properties, set true by default, but forbidden
> from changing.

Based on Andrew's comment, 
We have created a patch based on v4 that works as intended in QMP.

----------
diff --git a/target/arm/cpu.c b/target/arm/cpu.c
index 162e46afc3..2d9f779cb6 100644
--- a/target/arm/cpu.c
+++ b/target/arm/cpu.c
@@ -1345,29 +1345,12 @@ static void arm_cpu_finalizefn(Object *obj)
 #endif
 }

-static void a64fx_cpu_set_sve(ARMCPU *cpu)
-{
-    /* Suppport of A64FX's vector length are 128,256 and 512bit 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); /* 128bit */
-    set_bit(0, cpu->sve_vq_init);
-    set_bit(1, cpu->sve_vq_map); /* 256bit */
-    set_bit(1, cpu->sve_vq_init);
-    set_bit(3, cpu->sve_vq_map); /* 512bit */
-    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_finalize_features(ARMCPU *cpu, Error **errp)
 {
     Error *local_err = NULL;

     if (arm_feature(&cpu->env, ARM_FEATURE_AARCH64)) {
-        if (arm_feature(&cpu->env, ARM_FEATURE_A64FX)) {
-            a64fx_cpu_set_sve(cpu);
-        } else {
+        if (!arm_feature(&cpu->env, ARM_FEATURE_A64FX)) {
             arm_cpu_sve_finalize(cpu, &local_err);
             if (local_err != NULL) {
                 error_propagate(errp, local_err);
diff --git a/target/arm/cpu64.c b/target/arm/cpu64.c
index 5e7e885f9d..1ec2a7c6da 100644
--- a/target/arm/cpu64.c
+++ b/target/arm/cpu64.c
@@ -847,6 +847,23 @@ static void aarch64_max_initfn(Object *obj)
                         cpu_max_set_sve_max_vq, NULL, NULL);
 }

+static void a64fx_cpu_set_sve(Object *obj)
+{
+    int i;
+    Error *errp = NULL;
+    ARMCPU *cpu = ARM_CPU(obj);
+    /* Suppport of A64FX's vector length are 128,256 and 512bit only */
+    const char *vl[] = {"sve128", "sve256", "sve512"};
+
+    for(i = 0; i <sizeof(vl)/sizeof(vl[0]); i++){
+        object_property_add(obj, vl[i], "bool", cpu_arm_get_sve_vq,
+                            cpu_arm_set_sve_vq, NULL, NULL);
+        object_property_set_bool(obj, vl[i], true, &errp);
+    }
+
+    cpu->sve_max_vq = find_last_bit(cpu->sve_vq_map, ARM_MAX_VQ) + 1;
+}
+
 static void aarch64_a64fx_initfn(Object *obj)
 {
     ARMCPU *cpu = ARM_CPU(obj);
@@ -885,6 +902,9 @@ static void aarch64_a64fx_initfn(Object *obj)
     cpu->gic_vpribits = 5;
     cpu->gic_vprebits = 5;

+    /* Set SVE properties */
+    a64fx_cpu_set_sve(obj);
+
     /* TODO:  Add A64FX specific HPC extension registers */
 }
----------

In the case of the patch above,
it is possible to identify only the SVE vector length supported by A64FX from QMP, 
as shown in the following result.

----------
Welcome to the QMP low-level shell!
Connected to QEMU 6.0.93

(QEMU) query-cpu-model-expansion type=full model={"name":"a64fx"}
{"return": {"model": {"name": "a64fx", "props": {"sve128": true, "sve256": true, "sve512": true, "aarch64": true, "pmu": true}}}}
(QEMU)
----------

How about this kind of fix?
However, by allowing the sve128, sve256, and sve512 properties to be specified, 
the user can explicitly change the settings (ex: sve128=off), 
but the only properties that can be set is the vector length supported by A64FX. 
We personally think this is a no problem.

We would appreciate your comments.

Best regards.

> -----Original Message-----
> From: Andrew Jones <drjones@redhat.com>
> Sent: Thursday, August 12, 2021 6: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 v4 1/3] target-arm: Add support for Fujitsu A64FX
> 
> On Thu, Aug 12, 2021 at 11:16:50AM +0200, Andrew Jones wrote:
> > On Thu, Aug 12, 2021 at 03:04:38PM +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.
> > >
> > > For SVE, the A64FX processor supports only 128,256 and 512bit vector
> lengths.
> > >
> > > Signed-off-by: Shuuichirou Ishii <ishii.shuuichir@fujitsu.com>
> > > ---
> > >  target/arm/cpu.c   | 27 +++++++++++++++++++++++----
> > >  target/arm/cpu.h   |  1 +
> > >  target/arm/cpu64.c | 42
> ++++++++++++++++++++++++++++++++++++++++++
> > >  3 files changed, 66 insertions(+), 4 deletions(-)
> > >
> > > diff --git a/target/arm/cpu.c b/target/arm/cpu.c index
> > > 2866dd7658..162e46afc3 100644
> > > --- a/target/arm/cpu.c
> > > +++ b/target/arm/cpu.c
> > > @@ -1345,15 +1345,34 @@ static void arm_cpu_finalizefn(Object *obj)
> > > #endif  }
> > >
> > > +static void a64fx_cpu_set_sve(ARMCPU *cpu) {
> > > +    /* Suppport of A64FX's vector length are 128,256 and 512bit 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); /* 128bit */
> > > +    set_bit(0, cpu->sve_vq_init);
> > > +    set_bit(1, cpu->sve_vq_map); /* 256bit */
> > > +    set_bit(1, cpu->sve_vq_init);
> > > +    set_bit(3, cpu->sve_vq_map); /* 512bit */
> > > +    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_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;
> > > +            }
> > >          }
> > >
> > >          /*
> > > 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
> > > c690318a9b..5e7e885f9d 100644
> > > --- a/target/arm/cpu64.c
> > > +++ b/target/arm/cpu64.c
> > > @@ -847,10 +847,52 @@ 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_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 = 0x86668006;
> > > +    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 */ }
> > > +
> > >  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
> > >
> >
> > For the SVE related bits
> >
> > Reviewed-by: Andrew Jones <drjones@redhat.com>
> 
> On second thought, do we want the QMP CPU model expansion query to show that
> this CPU type has sve,sve128,sve256,sve512? If so, then our SVE work isn't
> complete, because we need those properties, set true by default, but forbidden
> from changing.
> 
> Thanks,
> drew

On Tue, Aug 17, 2021 at 06:43:58AM +0000, ishii.shuuichir@fujitsu.com wrote:
> 
> > On Thu, 12 Aug 2021 at 10:25, Andrew Jones <drjones@redhat.com> wrote:
> > > On second thought, do we want the QMP CPU model expansion query to
> > > show that this CPU type has sve,sve128,sve256,sve512? If so, then our
> > > SVE work isn't complete, because we need those properties, set true by
> > > default, but forbidden from changing.
> > 
> > Do we have precedent elsewhere (arm, x86, wherever) for "this CPU object
> > exposes these properties as constant unwriteable" ?
> 
> We have not yet conducted a confirmation of Peter's question, but...
> 
> > On second thought, do we want the QMP CPU model expansion query to show that
> > this CPU type has sve,sve128,sve256,sve512? If so, then our SVE work isn't
> > complete, because we need those properties, set true by default, but forbidden
> > from changing.
> 
> Based on Andrew's comment, 
> We have created a patch based on v4 that works as intended in QMP.
> 
> ----------
> diff --git a/target/arm/cpu.c b/target/arm/cpu.c
> index 162e46afc3..2d9f779cb6 100644
> --- a/target/arm/cpu.c
> +++ b/target/arm/cpu.c
> @@ -1345,29 +1345,12 @@ static void arm_cpu_finalizefn(Object *obj)
>  #endif
>  }
> 
> -static void a64fx_cpu_set_sve(ARMCPU *cpu)
> -{
> -    /* Suppport of A64FX's vector length are 128,256 and 512bit 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); /* 128bit */
> -    set_bit(0, cpu->sve_vq_init);
> -    set_bit(1, cpu->sve_vq_map); /* 256bit */
> -    set_bit(1, cpu->sve_vq_init);
> -    set_bit(3, cpu->sve_vq_map); /* 512bit */
> -    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_finalize_features(ARMCPU *cpu, Error **errp)
>  {
>      Error *local_err = NULL;
> 
>      if (arm_feature(&cpu->env, ARM_FEATURE_AARCH64)) {
> -        if (arm_feature(&cpu->env, ARM_FEATURE_A64FX)) {
> -            a64fx_cpu_set_sve(cpu);
> -        } else {
> +        if (!arm_feature(&cpu->env, ARM_FEATURE_A64FX)) {
>              arm_cpu_sve_finalize(cpu, &local_err);
>              if (local_err != NULL) {
>                  error_propagate(errp, local_err);
> diff --git a/target/arm/cpu64.c b/target/arm/cpu64.c
> index 5e7e885f9d..1ec2a7c6da 100644
> --- a/target/arm/cpu64.c
> +++ b/target/arm/cpu64.c
> @@ -847,6 +847,23 @@ static void aarch64_max_initfn(Object *obj)
>                          cpu_max_set_sve_max_vq, NULL, NULL);
>  }
> 
> +static void a64fx_cpu_set_sve(Object *obj)
> +{
> +    int i;
> +    Error *errp = NULL;
> +    ARMCPU *cpu = ARM_CPU(obj);
> +    /* Suppport of A64FX's vector length are 128,256 and 512bit only */
> +    const char *vl[] = {"sve128", "sve256", "sve512"};
> +
> +    for(i = 0; i <sizeof(vl)/sizeof(vl[0]); i++){
> +        object_property_add(obj, vl[i], "bool", cpu_arm_get_sve_vq,
> +                            cpu_arm_set_sve_vq, NULL, NULL);
> +        object_property_set_bool(obj, vl[i], true, &errp);
> +    }
> +
> +    cpu->sve_max_vq = find_last_bit(cpu->sve_vq_map, ARM_MAX_VQ) + 1;
> +}
> +
>  static void aarch64_a64fx_initfn(Object *obj)
>  {
>      ARMCPU *cpu = ARM_CPU(obj);
> @@ -885,6 +902,9 @@ static void aarch64_a64fx_initfn(Object *obj)
>      cpu->gic_vpribits = 5;
>      cpu->gic_vprebits = 5;
> 
> +    /* Set SVE properties */
> +    a64fx_cpu_set_sve(obj);
> +
>      /* TODO:  Add A64FX specific HPC extension registers */
>  }
> ----------
> 
> In the case of the patch above,
> it is possible to identify only the SVE vector length supported by A64FX from QMP, 
> as shown in the following result.
> 
> ----------
> Welcome to the QMP low-level shell!
> Connected to QEMU 6.0.93
> 
> (QEMU) query-cpu-model-expansion type=full model={"name":"a64fx"}
> {"return": {"model": {"name": "a64fx", "props": {"sve128": true, "sve256": true, "sve512": true, "aarch64": true, "pmu": true}}}}
> (QEMU)
> ----------
> 
> How about this kind of fix?

This looks reasonable to me, but you also need the 'sve' property that
states sve in supported at all.

> However, by allowing the sve128, sve256, and sve512 properties to be specified, 
> the user can explicitly change the settings (ex: sve128=off), 
> but the only properties that can be set is the vector length supported by A64FX. 
> We personally think this is a no problem.

I guess it's fine. You could easily create a new cpu_arm_set_sve_vq()
which would forbid changing the properties if you wanted to, but then
we need to answer Peter's question in order to see if there's a
precedent for that type of property.

Thanks,
drew

> 
> We would appreciate your comments.
> 
> Best regards.
> 
> > -----Original Message-----
> > From: Andrew Jones <drjones@redhat.com>
> > Sent: Thursday, August 12, 2021 6: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 v4 1/3] target-arm: Add support for Fujitsu A64FX
> > 
> > On Thu, Aug 12, 2021 at 11:16:50AM +0200, Andrew Jones wrote:
> > > On Thu, Aug 12, 2021 at 03:04:38PM +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.
> > > >
> > > > For SVE, the A64FX processor supports only 128,256 and 512bit vector
> > lengths.
> > > >
> > > > Signed-off-by: Shuuichirou Ishii <ishii.shuuichir@fujitsu.com>
> > > > ---
> > > >  target/arm/cpu.c   | 27 +++++++++++++++++++++++----
> > > >  target/arm/cpu.h   |  1 +
> > > >  target/arm/cpu64.c | 42
> > ++++++++++++++++++++++++++++++++++++++++++
> > > >  3 files changed, 66 insertions(+), 4 deletions(-)
> > > >
> > > > diff --git a/target/arm/cpu.c b/target/arm/cpu.c index
> > > > 2866dd7658..162e46afc3 100644
> > > > --- a/target/arm/cpu.c
> > > > +++ b/target/arm/cpu.c
> > > > @@ -1345,15 +1345,34 @@ static void arm_cpu_finalizefn(Object *obj)
> > > > #endif  }
> > > >
> > > > +static void a64fx_cpu_set_sve(ARMCPU *cpu) {
> > > > +    /* Suppport of A64FX's vector length are 128,256 and 512bit 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); /* 128bit */
> > > > +    set_bit(0, cpu->sve_vq_init);
> > > > +    set_bit(1, cpu->sve_vq_map); /* 256bit */
> > > > +    set_bit(1, cpu->sve_vq_init);
> > > > +    set_bit(3, cpu->sve_vq_map); /* 512bit */
> > > > +    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_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;
> > > > +            }
> > > >          }
> > > >
> > > >          /*
> > > > 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
> > > > c690318a9b..5e7e885f9d 100644
> > > > --- a/target/arm/cpu64.c
> > > > +++ b/target/arm/cpu64.c
> > > > @@ -847,10 +847,52 @@ 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_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 = 0x86668006;
> > > > +    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 */ }
> > > > +
> > > >  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
> > > >
> > >
> > > For the SVE related bits
> > >
> > > Reviewed-by: Andrew Jones <drjones@redhat.com>
> > 
> > On second thought, do we want the QMP CPU model expansion query to show that
> > this CPU type has sve,sve128,sve256,sve512? If so, then our SVE work isn't
> > complete, because we need those properties, set true by default, but forbidden
> > from changing.
> > 
> > Thanks,
> > drew
> 

On 8/17/21 1:56 AM, Andrew Jones wrote:
> I guess it's fine. You could easily create a new cpu_arm_set_sve_vq()
> which would forbid changing the properties if you wanted to, but then
> we need to answer Peter's question in order to see if there's a
> precedent for that type of property.

I don't see the point in read-only properties.  If the user wants to set non-standard 
values on the command-line, let them.  What is most important is getting the correct 
default from '-cpu a64fx'.


r~

On Tue, Aug 17, 2021 at 05:23:17AM -1000, Richard Henderson wrote:
> On 8/17/21 1:56 AM, Andrew Jones wrote:
> > I guess it's fine. You could easily create a new cpu_arm_set_sve_vq()
> > which would forbid changing the properties if you wanted to, but then
> > we need to answer Peter's question in order to see if there's a
> > precedent for that type of property.
> 
> I don't see the point in read-only properties.  If the user wants to set
> non-standard values on the command-line, let them.  What is most important
> is getting the correct default from '-cpu a64fx'.
>

So maybe we should just go ahead and add all sve* properties, but then
make sure the default vq map is correct.

Thanks,
drew

On 8/17/21 5:36 AM, Andrew Jones wrote:
> On Tue, Aug 17, 2021 at 05:23:17AM -1000, Richard Henderson wrote:
>> On 8/17/21 1:56 AM, Andrew Jones wrote:
>>> I guess it's fine. You could easily create a new cpu_arm_set_sve_vq()
>>> which would forbid changing the properties if you wanted to, but then
>>> we need to answer Peter's question in order to see if there's a
>>> precedent for that type of property.
>>
>> I don't see the point in read-only properties.  If the user wants to set
>> non-standard values on the command-line, let them.  What is most important
>> is getting the correct default from '-cpu a64fx'.
>>
> 
> So maybe we should just go ahead and add all sve* properties, but then
> make sure the default vq map is correct.

I think that's the right answer.

Presently we have a kvm_supported variable that's initialized by kvm_arm_sve_get_vls().  I 
think we want to rename that variable and provide a version of that function for tcg. 
Probably we should have done that before, with a trivial function for -cpu max to set all 
bits.

Then eliminate most of the other kvm_enabled() checks in arm_cpu_sve_finalize.  I think 
the only one we keep is the last, where we verify that the final sve_vq_map matches 
kvm_enabled exactly, modulo max_vq.

This should minimize the differences in behaviour between tcg and kvm.

r~

On Tue, Aug 17, 2021 at 05:53:34AM -1000, Richard Henderson wrote:
> On 8/17/21 5:36 AM, Andrew Jones wrote:
> > On Tue, Aug 17, 2021 at 05:23:17AM -1000, Richard Henderson wrote:
> > > On 8/17/21 1:56 AM, Andrew Jones wrote:
> > > > I guess it's fine. You could easily create a new cpu_arm_set_sve_vq()
> > > > which would forbid changing the properties if you wanted to, but then
> > > > we need to answer Peter's question in order to see if there's a
> > > > precedent for that type of property.
> > > 
> > > I don't see the point in read-only properties.  If the user wants to set
> > > non-standard values on the command-line, let them.  What is most important
> > > is getting the correct default from '-cpu a64fx'.
> > > 
> > 
> > So maybe we should just go ahead and add all sve* properties, but then
> > make sure the default vq map is correct.
> 
> I think that's the right answer.
> 
> Presently we have a kvm_supported variable that's initialized by
> kvm_arm_sve_get_vls().  I think we want to rename that variable and provide
> a version of that function for tcg. Probably we should have done that
> before, with a trivial function for -cpu max to set all bits.
> 
> Then eliminate most of the other kvm_enabled() checks in
> arm_cpu_sve_finalize.  I think the only one we keep is the last, where we
> verify that the final sve_vq_map matches kvm_enabled exactly, modulo max_vq.
> 
> This should minimize the differences in behaviour between tcg and kvm.

That's a good idea. I'll send a patch with your suggested-by.

Thanks,
drew

We appreciate everyone's comments.
Before making the V5 patch, please let me check the patch contents.

> This looks reasonable to me, but you also need the 'sve' property that states sve in
> supported at all.
> > > So maybe we should just go ahead and add all sve* properties, 

In response to the above comment,
We understood that the sve property will be added to the v4 patch.

i.e. 
(QEMU) query-cpu-model-expansion type=full model={"name":"a64fx"}
{"return": {"model": {"name": "a64fx", "props": {"sve128": false, "sve256": true, "sve": true, "sve512": true, "aarch64": true, "pmu": true}}}}

> > > but
> > > then make sure the default vq map is correct.

Furthermore, We understood that I need to add the above process as well, is that correct?

> That's a good idea. I'll send a patch with your suggested-by.

If that's correct,
In the current v4 patch, in the aarch64_a64fx_initfn function, 
the a64fx_cpu_set_sve function is executed to set the SVE property, 
and the arm_cpu_sve_finalize function is not called. 

In which function is it appropriate to execute the modulo max_vq function 
(or equivalent process)?

If We are not understanding you correctly,
We would appreciate your comments.

Best regards.

> -----Original Message-----
> From: Andrew Jones <drjones@redhat.com>
> Sent: Wednesday, August 18, 2021 1:28 AM
> To: Richard Henderson <richard.henderson@linaro.org>
> Cc: Ishii, Shuuichirou/石井 周一郎 <ishii.shuuichir@fujitsu.com>;
> peter.maydell@linaro.org; qemu-arm@nongnu.org; qemu-devel@nongnu.org
> Subject: Re: [PATCH v4 1/3] target-arm: Add support for Fujitsu A64FX
> 
> On Tue, Aug 17, 2021 at 05:53:34AM -1000, Richard Henderson wrote:
> > On 8/17/21 5:36 AM, Andrew Jones wrote:
> > > On Tue, Aug 17, 2021 at 05:23:17AM -1000, Richard Henderson wrote:
> > > > On 8/17/21 1:56 AM, Andrew Jones wrote:
> > > > > I guess it's fine. You could easily create a new
> > > > > cpu_arm_set_sve_vq() which would forbid changing the properties
> > > > > if you wanted to, but then we need to answer Peter's question in
> > > > > order to see if there's a precedent for that type of property.
> > > >
> > > > I don't see the point in read-only properties.  If the user wants
> > > > to set non-standard values on the command-line, let them.  What is
> > > > most important is getting the correct default from '-cpu a64fx'.
> > > >
> > >
> > > So maybe we should just go ahead and add all sve* properties, but
> > > then make sure the default vq map is correct.
> >
> > I think that's the right answer.
> >
> > Presently we have a kvm_supported variable that's initialized by
> > kvm_arm_sve_get_vls().  I think we want to rename that variable and
> > provide a version of that function for tcg. Probably we should have
> > done that before, with a trivial function for -cpu max to set all bits.
> >
> > Then eliminate most of the other kvm_enabled() checks in
> > arm_cpu_sve_finalize.  I think the only one we keep is the last, where
> > we verify that the final sve_vq_map matches kvm_enabled exactly, modulo
> max_vq.
> >
> > This should minimize the differences in behaviour between tcg and kvm.
> 
> That's a good idea. I'll send a patch with your suggested-by.
> 
> Thanks,
> drew

On Wed, Aug 18, 2021 at 08:29:15AM +0000, ishii.shuuichir@fujitsu.com wrote:
> 
> We appreciate everyone's comments.
> Before making the V5 patch, please let me check the patch contents.
> 
> > This looks reasonable to me, but you also need the 'sve' property that states sve in
> > supported at all.
> > > > So maybe we should just go ahead and add all sve* properties, 
> 
> In response to the above comment,
> We understood that the sve property will be added to the v4 patch.
> 
> i.e. 
> (QEMU) query-cpu-model-expansion type=full model={"name":"a64fx"}
> {"return": {"model": {"name": "a64fx", "props": {"sve128": false, "sve256": true, "sve": true, "sve512": true, "aarch64": true, "pmu": true}}}}
> 
> > > > but
> > > > then make sure the default vq map is correct.
> 
> Furthermore, We understood that I need to add the above process as well, is that correct?
> 
> > That's a good idea. I'll send a patch with your suggested-by.
> 
> If that's correct,
> In the current v4 patch, in the aarch64_a64fx_initfn function, 
> the a64fx_cpu_set_sve function is executed to set the SVE property, 
> and the arm_cpu_sve_finalize function is not called. 
> 
> In which function is it appropriate to execute the modulo max_vq function 
> (or equivalent process)?
> 
> If We are not understanding you correctly,
> We would appreciate your comments.

Richard's suggestion is to generalize the "supported" bitmap concept,
which is currently only used for KVM, in order to also use it for
TCG cpu models. The 'max' cpu type will have the trivial all-set
supported bitmap, but the a64fx will have a specific one. I plan to
do this "supported" bitmap generalization and apply it to the TCG
max cpu type. You'll need to rebase this series on those patches and
provide the a64fx supported bitmap.

I think this will be more clear once I get the patch posted (which I
haven't started writing yet). I'll try to get it posted by tomorrow
evening though, since I have vacation on Friday.

Thanks,
drew


> 
> Best regards.
> 
> > -----Original Message-----
> > From: Andrew Jones <drjones@redhat.com>
> > Sent: Wednesday, August 18, 2021 1:28 AM
> > To: Richard Henderson <richard.henderson@linaro.org>
> > Cc: Ishii, Shuuichirou/石井 周一郎 <ishii.shuuichir@fujitsu.com>;
> > peter.maydell@linaro.org; qemu-arm@nongnu.org; qemu-devel@nongnu.org
> > Subject: Re: [PATCH v4 1/3] target-arm: Add support for Fujitsu A64FX
> > 
> > On Tue, Aug 17, 2021 at 05:53:34AM -1000, Richard Henderson wrote:
> > > On 8/17/21 5:36 AM, Andrew Jones wrote:
> > > > On Tue, Aug 17, 2021 at 05:23:17AM -1000, Richard Henderson wrote:
> > > > > On 8/17/21 1:56 AM, Andrew Jones wrote:
> > > > > > I guess it's fine. You could easily create a new
> > > > > > cpu_arm_set_sve_vq() which would forbid changing the properties
> > > > > > if you wanted to, but then we need to answer Peter's question in
> > > > > > order to see if there's a precedent for that type of property.
> > > > >
> > > > > I don't see the point in read-only properties.  If the user wants
> > > > > to set non-standard values on the command-line, let them.  What is
> > > > > most important is getting the correct default from '-cpu a64fx'.
> > > > >
> > > >
> > > > So maybe we should just go ahead and add all sve* properties, but
> > > > then make sure the default vq map is correct.
> > >
> > > I think that's the right answer.
> > >
> > > Presently we have a kvm_supported variable that's initialized by
> > > kvm_arm_sve_get_vls().  I think we want to rename that variable and
> > > provide a version of that function for tcg. Probably we should have
> > > done that before, with a trivial function for -cpu max to set all bits.
> > >
> > > Then eliminate most of the other kvm_enabled() checks in
> > > arm_cpu_sve_finalize.  I think the only one we keep is the last, where
> > > we verify that the final sve_vq_map matches kvm_enabled exactly, modulo
> > max_vq.
> > >
> > > This should minimize the differences in behaviour between tcg and kvm.
> > 
> > That's a good idea. I'll send a patch with your suggested-by.
> > 
> > Thanks,
> > drew
> 

> I think this will be more clear once I get the patch posted (which I haven't started
> writing yet). I'll try to get it posted by tomorrow evening though, since I have
> vacation on Friday.

While Andrew is working on the patch in a hurry, 
I'm sorry, I'll be on vacation for a while starting Friday too,
so my reply will be delayed.

Best regards.


> -----Original Message-----
> From: Andrew Jones <drjones@redhat.com>
> Sent: Wednesday, August 18, 2021 5:58 PM
> To: Ishii, Shuuichirou/石井 周一郎 <ishii.shuuichir@fujitsu.com>
> Cc: Richard Henderson <richard.henderson@linaro.org>;
> peter.maydell@linaro.org; qemu-arm@nongnu.org; qemu-devel@nongnu.org
> Subject: Re: [PATCH v4 1/3] target-arm: Add support for Fujitsu A64FX
> 
> On Wed, Aug 18, 2021 at 08:29:15AM +0000, ishii.shuuichir@fujitsu.com wrote:
> >
> > We appreciate everyone's comments.
> > Before making the V5 patch, please let me check the patch contents.
> >
> > > This looks reasonable to me, but you also need the 'sve' property
> > > that states sve in supported at all.
> > > > > So maybe we should just go ahead and add all sve* properties,
> >
> > In response to the above comment,
> > We understood that the sve property will be added to the v4 patch.
> >
> > i.e.
> > (QEMU) query-cpu-model-expansion type=full model={"name":"a64fx"}
> > {"return": {"model": {"name": "a64fx", "props": {"sve128": false,
> > "sve256": true, "sve": true, "sve512": true, "aarch64": true, "pmu":
> > true}}}}
> >
> > > > > but
> > > > > then make sure the default vq map is correct.
> >
> > Furthermore, We understood that I need to add the above process as well, is
> that correct?
> >
> > > That's a good idea. I'll send a patch with your suggested-by.
> >
> > If that's correct,
> > In the current v4 patch, in the aarch64_a64fx_initfn function, the
> > a64fx_cpu_set_sve function is executed to set the SVE property, and
> > the arm_cpu_sve_finalize function is not called.
> >
> > In which function is it appropriate to execute the modulo max_vq
> > function (or equivalent process)?
> >
> > If We are not understanding you correctly, We would appreciate your
> > comments.
> 
> Richard's suggestion is to generalize the "supported" bitmap concept, which is
> currently only used for KVM, in order to also use it for TCG cpu models. The 'max'
> cpu type will have the trivial all-set supported bitmap, but the a64fx will have a
> specific one. I plan to do this "supported" bitmap generalization and apply it to the
> TCG max cpu type. You'll need to rebase this series on those patches and provide
> the a64fx supported bitmap.
> 
> I think this will be more clear once I get the patch posted (which I haven't started
> writing yet). I'll try to get it posted by tomorrow evening though, since I have
> vacation on Friday.
> 
> Thanks,
> drew
> 
> 
> >
> > Best regards.
> >
> > > -----Original Message-----
> > > From: Andrew Jones <drjones@redhat.com>
> > > Sent: Wednesday, August 18, 2021 1:28 AM
> > > To: Richard Henderson <richard.henderson@linaro.org>
> > > Cc: Ishii, Shuuichirou/石井 周一郎 <ishii.shuuichir@fujitsu.com>;
> > > peter.maydell@linaro.org; qemu-arm@nongnu.org; qemu-devel@nongnu.org
> > > Subject: Re: [PATCH v4 1/3] target-arm: Add support for Fujitsu
> > > A64FX
> > >
> > > On Tue, Aug 17, 2021 at 05:53:34AM -1000, Richard Henderson wrote:
> > > > On 8/17/21 5:36 AM, Andrew Jones wrote:
> > > > > On Tue, Aug 17, 2021 at 05:23:17AM -1000, Richard Henderson wrote:
> > > > > > On 8/17/21 1:56 AM, Andrew Jones wrote:
> > > > > > > I guess it's fine. You could easily create a new
> > > > > > > cpu_arm_set_sve_vq() which would forbid changing the
> > > > > > > properties if you wanted to, but then we need to answer
> > > > > > > Peter's question in order to see if there's a precedent for that type of
> property.
> > > > > >
> > > > > > I don't see the point in read-only properties.  If the user
> > > > > > wants to set non-standard values on the command-line, let
> > > > > > them.  What is most important is getting the correct default from '-cpu
> a64fx'.
> > > > > >
> > > > >
> > > > > So maybe we should just go ahead and add all sve* properties,
> > > > > but then make sure the default vq map is correct.
> > > >
> > > > I think that's the right answer.
> > > >
> > > > Presently we have a kvm_supported variable that's initialized by
> > > > kvm_arm_sve_get_vls().  I think we want to rename that variable
> > > > and provide a version of that function for tcg. Probably we should
> > > > have done that before, with a trivial function for -cpu max to set all bits.
> > > >
> > > > Then eliminate most of the other kvm_enabled() checks in
> > > > arm_cpu_sve_finalize.  I think the only one we keep is the last,
> > > > where we verify that the final sve_vq_map matches kvm_enabled
> > > > exactly, modulo
> > > max_vq.
> > > >
> > > > This should minimize the differences in behaviour between tcg and kvm.
> > >
> > > That's a good idea. I'll send a patch with your suggested-by.
> > >
> > > Thanks,
> > > drew
> >