| Message ID | 1538579266-8389-11-git-send-email-edgar.iglesias@gmail.com |
|---|---|
| State | New |
| Headers | show |
| Series | arm: Add first models of Xilinx Versal SoC | expand |
On 3 October 2018 at 16:07, Edgar E. Iglesias <edgar.iglesias@gmail.com> wrote: > From: "Edgar E. Iglesias" <edgar.iglesias@xilinx.com> > > Add the ARM Cortex-A72. > > Signed-off-by: Edgar E. Iglesias <edgar.iglesias@xilinx.com> > --- > target/arm/cpu64.c | 59 ++++++++++++++++++++++++++++++++++++++++++++++++++++++ > 1 file changed, 59 insertions(+) > > diff --git a/target/arm/cpu64.c b/target/arm/cpu64.c > index 800bff7..02e500b 100644 > --- a/target/arm/cpu64.c > +++ b/target/arm/cpu64.c > @@ -218,6 +218,64 @@ static void aarch64_a53_initfn(Object *obj) > define_arm_cp_regs(cpu, cortex_a57_a53_cp_reginfo); > } > > +static void aarch64_a72_initfn(Object *obj) > +{ > + ARMCPU *cpu = ARM_CPU(obj); > + > + cpu->dtb_compatible = "arm,cortex-a72"; > + set_feature(&cpu->env, ARM_FEATURE_V8); > + set_feature(&cpu->env, ARM_FEATURE_VFP4); > + 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_CBAR_RO); > + set_feature(&cpu->env, ARM_FEATURE_V8_AES); > + set_feature(&cpu->env, ARM_FEATURE_V8_SHA1); > + set_feature(&cpu->env, ARM_FEATURE_V8_SHA256); > + set_feature(&cpu->env, ARM_FEATURE_V8_PMULL); > + set_feature(&cpu->env, ARM_FEATURE_CRC); > + set_feature(&cpu->env, ARM_FEATURE_EL2); > + set_feature(&cpu->env, ARM_FEATURE_EL3); No ARM_FEATURE_PMU ? > + cpu->midr = 0x410fd083; > + cpu->revidr = 0x00000000; > + cpu->reset_fpsid = 0x41034080; > + cpu->mvfr0 = 0x10110222; > + cpu->mvfr1 = 0x12111111; > + cpu->mvfr2 = 0x00000043; > + cpu->ctr = 0x8444c004; > + cpu->reset_sctlr = 0x00c50838; Do you happen to have the hardware to hand to check what the top 4 bits of the reset value of SCTLR_ELx are? I think they should be 0x3 -- the Arm ARM says that [29:28] are RES1 (as does the A72 TRM, though its top level summary table lists 0x00c50838 as the reset value for some of the SCTLR_ELx.) QEMU may have the wrong value for A53/A57 here too, I suspect. > + cpu->id_pfr0 = 0x00000131; > + cpu->id_pfr1 = 0x00011011; > + cpu->id_dfr0 = 0x03010066; > + cpu->id_afr0 = 0x00000000; > + cpu->id_mmfr0 = 0x10201105; > + cpu->id_mmfr1 = 0x40000000; > + cpu->id_mmfr2 = 0x01260000; > + cpu->id_mmfr3 = 0x02102211; > + cpu->id_isar0 = 0x02101110; > + cpu->id_isar1 = 0x13112111; > + cpu->id_isar2 = 0x21232042; > + cpu->id_isar3 = 0x01112131; > + cpu->id_isar4 = 0x00011142; > + cpu->id_isar5 = 0x00011121; > + cpu->id_aa64pfr0 = 0x00002222; > + cpu->id_aa64dfr0 = 0x10305106; > + cpu->pmceid0 = 0x00000000; > + cpu->pmceid1 = 0x00000000; > + cpu->id_aa64isar0 = 0x00011120; > + cpu->id_aa64mmfr0 = 0x00001124; > + cpu->dbgdidr = 0x3516d000; > + cpu->clidr = 0x0a200023; > + cpu->ccsidr[0] = 0x701fe00a; /* 32KB L1 dcache */ > + cpu->ccsidr[1] = 0x201fe012; /* 48KB L1 icache */ > + cpu->ccsidr[2] = 0x707fe07a; /* 1MB L2 cache */ > + cpu->dcz_blocksize = 4; /* 64 bytes */ > + cpu->gic_num_lrs = 4; > + cpu->gic_vpribits = 5; > + cpu->gic_vprebits = 5; > + define_arm_cp_regs(cpu, cortex_a57_a53_cp_reginfo); The impdef registers do seem to be basically the same as the A53/A57. The function name is now a bit inaccurate, though, but I don't have a good idea for a better name. Otherwise Reviewed-by: Peter Maydell <peter.maydell@linaro.org> thanks -- PMM
On Mon, Oct 08, 2018 at 02:10:29PM +0100, Peter Maydell wrote: > On 3 October 2018 at 16:07, Edgar E. Iglesias <edgar.iglesias@gmail.com> wrote: > > From: "Edgar E. Iglesias" <edgar.iglesias@xilinx.com> > > > > Add the ARM Cortex-A72. > > > > Signed-off-by: Edgar E. Iglesias <edgar.iglesias@xilinx.com> > > --- > > target/arm/cpu64.c | 59 ++++++++++++++++++++++++++++++++++++++++++++++++++++++ > > 1 file changed, 59 insertions(+) > > > > diff --git a/target/arm/cpu64.c b/target/arm/cpu64.c > > index 800bff7..02e500b 100644 > > --- a/target/arm/cpu64.c > > +++ b/target/arm/cpu64.c > > @@ -218,6 +218,64 @@ static void aarch64_a53_initfn(Object *obj) > > define_arm_cp_regs(cpu, cortex_a57_a53_cp_reginfo); > > } > > > > +static void aarch64_a72_initfn(Object *obj) > > +{ > > + ARMCPU *cpu = ARM_CPU(obj); > > + > > + cpu->dtb_compatible = "arm,cortex-a72"; > > + set_feature(&cpu->env, ARM_FEATURE_V8); > > + set_feature(&cpu->env, ARM_FEATURE_VFP4); > > + 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_CBAR_RO); > > + set_feature(&cpu->env, ARM_FEATURE_V8_AES); > > + set_feature(&cpu->env, ARM_FEATURE_V8_SHA1); > > + set_feature(&cpu->env, ARM_FEATURE_V8_SHA256); > > + set_feature(&cpu->env, ARM_FEATURE_V8_PMULL); > > + set_feature(&cpu->env, ARM_FEATURE_CRC); > > + set_feature(&cpu->env, ARM_FEATURE_EL2); > > + set_feature(&cpu->env, ARM_FEATURE_EL3); > > No ARM_FEATURE_PMU ? Will add in v2. > > > + cpu->midr = 0x410fd083; > > + cpu->revidr = 0x00000000; > > + cpu->reset_fpsid = 0x41034080; > > + cpu->mvfr0 = 0x10110222; > > + cpu->mvfr1 = 0x12111111; > > + cpu->mvfr2 = 0x00000043; > > + cpu->ctr = 0x8444c004; > > + cpu->reset_sctlr = 0x00c50838; > > Do you happen to have the hardware to hand to check what the > top 4 bits of the reset value of SCTLR_ELx are? I think they > should be 0x3 -- the Arm ARM says that [29:28] are RES1 (as > does the A72 TRM, though its top level summary table lists > 0x00c50838 as the reset value for some of the SCTLR_ELx.) > > QEMU may have the wrong value for A53/A57 here too, I suspect. I don't have access to the A72s at the moment but looking at logs it seems to be 0x00c50838 for both the A53 and A72. Looking at "Table 4-118 SCTLR bit assignments" in the A72 TRM, bits [30:28] seem to have been allocated. Bit 30 depends on configuration inputs to the core and [29:28] seem to be hard-coded to zero. On ZynqMP and Versal, bit 30 (CFGTE) seems to be runtime configurable prior to releasing the A53s/A72s from reset. I haven't tried it though. I ran a test on the ZynqMP a53s and got the folloing right after reset, for EL1, EL2 and EL3: sctlr c52838 30c50838 c52838 The 0x2000 mask is due to V-hivecs being set. SCLTR_EL2 seems to get the RES1 bits set to 1... Perhaps we should just leave it as is for now? > > > + cpu->id_pfr0 = 0x00000131; > > + cpu->id_pfr1 = 0x00011011; > > + cpu->id_dfr0 = 0x03010066; > > + cpu->id_afr0 = 0x00000000; > > + cpu->id_mmfr0 = 0x10201105; > > + cpu->id_mmfr1 = 0x40000000; > > + cpu->id_mmfr2 = 0x01260000; > > + cpu->id_mmfr3 = 0x02102211; > > + cpu->id_isar0 = 0x02101110; > > + cpu->id_isar1 = 0x13112111; > > + cpu->id_isar2 = 0x21232042; > > + cpu->id_isar3 = 0x01112131; > > + cpu->id_isar4 = 0x00011142; > > + cpu->id_isar5 = 0x00011121; > > + cpu->id_aa64pfr0 = 0x00002222; > > + cpu->id_aa64dfr0 = 0x10305106; > > + cpu->pmceid0 = 0x00000000; > > + cpu->pmceid1 = 0x00000000; > > + cpu->id_aa64isar0 = 0x00011120; > > + cpu->id_aa64mmfr0 = 0x00001124; > > + cpu->dbgdidr = 0x3516d000; > > + cpu->clidr = 0x0a200023; > > + cpu->ccsidr[0] = 0x701fe00a; /* 32KB L1 dcache */ > > + cpu->ccsidr[1] = 0x201fe012; /* 48KB L1 icache */ > > + cpu->ccsidr[2] = 0x707fe07a; /* 1MB L2 cache */ > > + cpu->dcz_blocksize = 4; /* 64 bytes */ > > + cpu->gic_num_lrs = 4; > > + cpu->gic_vpribits = 5; > > + cpu->gic_vprebits = 5; > > + define_arm_cp_regs(cpu, cortex_a57_a53_cp_reginfo); > > The impdef registers do seem to be basically the same as > the A53/A57. The function name is now a bit inaccurate, > though, but I don't have a good idea for a better name. > > Otherwise > Reviewed-by: Peter Maydell <peter.maydell@linaro.org> > > thanks > -- PMM
On 8 October 2018 at 22:34, Edgar E. Iglesias <edgar.iglesias@xilinx.com> wrote: > On Mon, Oct 08, 2018 at 02:10:29PM +0100, Peter Maydell wrote: >> On 3 October 2018 at 16:07, Edgar E. Iglesias <edgar.iglesias@gmail.com> wrote: >> > From: "Edgar E. Iglesias" <edgar.iglesias@xilinx.com> >> > >> > Add the ARM Cortex-A72. >> > >> > Signed-off-by: Edgar E. Iglesias <edgar.iglesias@xilinx.com> >> > + cpu->midr = 0x410fd083; >> > + cpu->revidr = 0x00000000; >> > + cpu->reset_fpsid = 0x41034080; >> > + cpu->mvfr0 = 0x10110222; >> > + cpu->mvfr1 = 0x12111111; >> > + cpu->mvfr2 = 0x00000043; >> > + cpu->ctr = 0x8444c004; >> > + cpu->reset_sctlr = 0x00c50838; >> >> Do you happen to have the hardware to hand to check what the >> top 4 bits of the reset value of SCTLR_ELx are? I think they >> should be 0x3 -- the Arm ARM says that [29:28] are RES1 (as >> does the A72 TRM, though its top level summary table lists >> 0x00c50838 as the reset value for some of the SCTLR_ELx.) >> >> QEMU may have the wrong value for A53/A57 here too, I suspect. > > I don't have access to the A72s at the moment but looking at logs > it seems to be 0x00c50838 for both the A53 and A72. > Looking at "Table 4-118 SCTLR bit assignments" in the A72 TRM, > bits [30:28] seem to have been allocated. Bit 30 depends on > configuration inputs to the core and [29:28] seem to be hard-coded > to zero. Ah, this is a 32-bit view vs 64-bit view thing. In 32-bit, SCTLR[28] is TRE (TEX remap enable), and SCTLR[29] is AFE (access flag enable), and both are resets-to-zero. HSCTLR[28] and [29] are both reserved, RES1. In 64-bit, SCTLR_EL1[29:28] are RES1 in ARMv8.1 and v8.0, and have new meanings assigned in v8.2 and v8.3. SCTLR_EL2[29:28] and SCTLR_EL3[29:28] are reserved, RES1. For QEMU at the moment we don't deal with this, and so we have only the one reset value, cpu->reset_sctlr, which we use for both the SCTLR_EL1 and SCTLR_EL3 resets. Our HSCTLR/SCTLR_EL2 resets to zero, and we don't allow for the 64-bit and 32-bit views not necessarily being the same value. thanks -- PMM
On Tue, Oct 09, 2018 at 10:30:01AM +0100, Peter Maydell wrote: > On 8 October 2018 at 22:34, Edgar E. Iglesias <edgar.iglesias@xilinx.com> wrote: > > On Mon, Oct 08, 2018 at 02:10:29PM +0100, Peter Maydell wrote: > >> On 3 October 2018 at 16:07, Edgar E. Iglesias <edgar.iglesias@gmail.com> wrote: > >> > From: "Edgar E. Iglesias" <edgar.iglesias@xilinx.com> > >> > > >> > Add the ARM Cortex-A72. > >> > > >> > Signed-off-by: Edgar E. Iglesias <edgar.iglesias@xilinx.com> > > >> > + cpu->midr = 0x410fd083; > >> > + cpu->revidr = 0x00000000; > >> > + cpu->reset_fpsid = 0x41034080; > >> > + cpu->mvfr0 = 0x10110222; > >> > + cpu->mvfr1 = 0x12111111; > >> > + cpu->mvfr2 = 0x00000043; > >> > + cpu->ctr = 0x8444c004; > >> > + cpu->reset_sctlr = 0x00c50838; > >> > >> Do you happen to have the hardware to hand to check what the > >> top 4 bits of the reset value of SCTLR_ELx are? I think they > >> should be 0x3 -- the Arm ARM says that [29:28] are RES1 (as > >> does the A72 TRM, though its top level summary table lists > >> 0x00c50838 as the reset value for some of the SCTLR_ELx.) > >> > >> QEMU may have the wrong value for A53/A57 here too, I suspect. > > > > I don't have access to the A72s at the moment but looking at logs > > it seems to be 0x00c50838 for both the A53 and A72. > > Looking at "Table 4-118 SCTLR bit assignments" in the A72 TRM, > > bits [30:28] seem to have been allocated. Bit 30 depends on > > configuration inputs to the core and [29:28] seem to be hard-coded > > to zero. > > Ah, this is a 32-bit view vs 64-bit view thing. In 32-bit, > SCTLR[28] is TRE (TEX remap enable), and SCTLR[29] is AFE > (access flag enable), and both are resets-to-zero. > HSCTLR[28] and [29] are both reserved, RES1. > In 64-bit, SCTLR_EL1[29:28] are RES1 in ARMv8.1 and v8.0, and > have new meanings assigned in v8.2 and v8.3. > SCTLR_EL2[29:28] and SCTLR_EL3[29:28] are reserved, RES1. > > For QEMU at the moment we don't deal with this, and so we > have only the one reset value, cpu->reset_sctlr, which we use > for both the SCTLR_EL1 and SCTLR_EL3 resets. Our HSCTLR/SCTLR_EL2 > resets to zero, and we don't allow for the 64-bit and 32-bit views > not necessarily being the same value. Aha, I see. I'll leave as is then and we can fix the 64 bit stuff later I guess. Another A72 related thing I wanted to check with you. A month or two ago I was looking at an issue with Linux running very slowly on our models. Something that popped up was that Linux was running a couple of spectre related "workarounds" and "hardening" sequences on the QEMU A72s. There are a couple of bits in the ID_AARCH64_PFR0 register that Linux checks before enabling the sequences but I never found any documentation of them in the specs. Bits 56 and 60. In Linux these are refered to as: ID_AA64PFR0_CSV2_SHIFT ID_AA64PFR0_CSV3_SHIFT This is what we have in our tree: cpu->gic_vprebits = 5; define_arm_cp_regs(cpu, cortex_a57_a53_cp_reginfo); /* Xilinx FIXUPs. */ /* These indicate the BP hardening and KPTI aren't needed. */ cpu->id_aa64pfr0 |= (uint64_t)1 << 56; /* BP. */ cpu->id_aa64pfr0 |= (uint64_t)1 << 60; /* KPTI. */ } Do you know what these are? Should we be setting these in QEMU? Cheers, Edgar
Hello, On Tue, Oct 9, 2018 at 3:19 PM Edgar E. Iglesias <edgar.iglesias@xilinx.com> wrote: > > Another A72 related thing I wanted to check with you. A month or two ago I was > looking at an issue with Linux running very slowly on our models. > Something that popped up was that Linux was running a couple of spectre related > "workarounds" and "hardening" sequences on the QEMU A72s. > > There are a couple of bits in the ID_AARCH64_PFR0 register that > Linux checks before enabling the sequences but I never found any > documentation of them in the specs. Bits 56 and 60. > > In Linux these are refered to as: > ID_AA64PFR0_CSV2_SHIFT > ID_AA64PFR0_CSV3_SHIFT > > This is what we have in our tree: > > cpu->gic_vprebits = 5; > define_arm_cp_regs(cpu, cortex_a57_a53_cp_reginfo); > > /* Xilinx FIXUPs. */ > /* These indicate the BP hardening and KPTI aren't needed. */ > cpu->id_aa64pfr0 |= (uint64_t)1 << 56; /* BP. */ > cpu->id_aa64pfr0 |= (uint64_t)1 << 60; /* KPTI. */ > } > > Do you know what these are? > Should we be setting these in QEMU? These fields are publicly documented in the system register specification: https://developer.arm.com/products/architecture/cpu-architecture/a-profile/exploration-tools These are ARMv8.5 fields, I don't think these should be set by default for Cortex-A72. Of course nothing prevents you from defining a specific CPU with these fields set to boot faster :-) Or perhaps add a property to override the default value of these registers? Laurent
On Tue, Oct 09, 2018 at 03:40:13PM +0200, Laurent Desnogues wrote: > Hello, > > On Tue, Oct 9, 2018 at 3:19 PM Edgar E. Iglesias > <edgar.iglesias@xilinx.com> wrote: > > > > Another A72 related thing I wanted to check with you. A month or two ago I was > > looking at an issue with Linux running very slowly on our models. > > Something that popped up was that Linux was running a couple of spectre related > > "workarounds" and "hardening" sequences on the QEMU A72s. > > > > There are a couple of bits in the ID_AARCH64_PFR0 register that > > Linux checks before enabling the sequences but I never found any > > documentation of them in the specs. Bits 56 and 60. > > > > In Linux these are refered to as: > > ID_AA64PFR0_CSV2_SHIFT > > ID_AA64PFR0_CSV3_SHIFT > > > > This is what we have in our tree: > > > > cpu->gic_vprebits = 5; > > define_arm_cp_regs(cpu, cortex_a57_a53_cp_reginfo); > > > > /* Xilinx FIXUPs. */ > > /* These indicate the BP hardening and KPTI aren't needed. */ > > cpu->id_aa64pfr0 |= (uint64_t)1 << 56; /* BP. */ > > cpu->id_aa64pfr0 |= (uint64_t)1 << 60; /* KPTI. */ > > } > > > > Do you know what these are? > > Should we be setting these in QEMU? > > These fields are publicly documented in the system register specification: > > https://developer.arm.com/products/architecture/cpu-architecture/a-profile/exploration-tools > > These are ARMv8.5 fields, I don't think these should be set by default > for Cortex-A72. Of course nothing prevents you from defining a > specific CPU with these fields set to boot faster :-) Or perhaps add > a property to override the default value of these registers? Thanks, I had no idea these documents existed! Cheers, Edgar
diff --git a/target/arm/cpu64.c b/target/arm/cpu64.c index 800bff7..02e500b 100644 --- a/target/arm/cpu64.c +++ b/target/arm/cpu64.c @@ -218,6 +218,64 @@ static void aarch64_a53_initfn(Object *obj) define_arm_cp_regs(cpu, cortex_a57_a53_cp_reginfo); } +static void aarch64_a72_initfn(Object *obj) +{ + ARMCPU *cpu = ARM_CPU(obj); + + cpu->dtb_compatible = "arm,cortex-a72"; + set_feature(&cpu->env, ARM_FEATURE_V8); + set_feature(&cpu->env, ARM_FEATURE_VFP4); + 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_CBAR_RO); + set_feature(&cpu->env, ARM_FEATURE_V8_AES); + set_feature(&cpu->env, ARM_FEATURE_V8_SHA1); + set_feature(&cpu->env, ARM_FEATURE_V8_SHA256); + set_feature(&cpu->env, ARM_FEATURE_V8_PMULL); + set_feature(&cpu->env, ARM_FEATURE_CRC); + set_feature(&cpu->env, ARM_FEATURE_EL2); + set_feature(&cpu->env, ARM_FEATURE_EL3); + cpu->midr = 0x410fd083; + cpu->revidr = 0x00000000; + cpu->reset_fpsid = 0x41034080; + cpu->mvfr0 = 0x10110222; + cpu->mvfr1 = 0x12111111; + cpu->mvfr2 = 0x00000043; + cpu->ctr = 0x8444c004; + cpu->reset_sctlr = 0x00c50838; + cpu->id_pfr0 = 0x00000131; + cpu->id_pfr1 = 0x00011011; + cpu->id_dfr0 = 0x03010066; + cpu->id_afr0 = 0x00000000; + cpu->id_mmfr0 = 0x10201105; + cpu->id_mmfr1 = 0x40000000; + cpu->id_mmfr2 = 0x01260000; + cpu->id_mmfr3 = 0x02102211; + cpu->id_isar0 = 0x02101110; + cpu->id_isar1 = 0x13112111; + cpu->id_isar2 = 0x21232042; + cpu->id_isar3 = 0x01112131; + cpu->id_isar4 = 0x00011142; + cpu->id_isar5 = 0x00011121; + cpu->id_aa64pfr0 = 0x00002222; + cpu->id_aa64dfr0 = 0x10305106; + cpu->pmceid0 = 0x00000000; + cpu->pmceid1 = 0x00000000; + cpu->id_aa64isar0 = 0x00011120; + cpu->id_aa64mmfr0 = 0x00001124; + cpu->dbgdidr = 0x3516d000; + cpu->clidr = 0x0a200023; + cpu->ccsidr[0] = 0x701fe00a; /* 32KB L1 dcache */ + cpu->ccsidr[1] = 0x201fe012; /* 48KB L1 icache */ + cpu->ccsidr[2] = 0x707fe07a; /* 1MB L2 cache */ + cpu->dcz_blocksize = 4; /* 64 bytes */ + cpu->gic_num_lrs = 4; + cpu->gic_vpribits = 5; + cpu->gic_vprebits = 5; + define_arm_cp_regs(cpu, cortex_a57_a53_cp_reginfo); +} + static void cpu_max_get_sve_vq(Object *obj, Visitor *v, const char *name, void *opaque, Error **errp) { @@ -293,6 +351,7 @@ typedef struct ARMCPUInfo { 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 = "max", .initfn = aarch64_max_initfn }, { .name = NULL } };