| Message ID | 20210805073045.916622-2-ishii.shuuichir@fujitsu.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | Add support for Fujitsu A64FX processor | expand |
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 > >
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
> >
> >
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 > > > > > > >
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 > > > > > > > > > >
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 }, };
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(+)