| Message ID | 20190912110103.1417887-1-luc.michel@greensocs.com |
|---|---|
| State | New |
| Headers | show |
| Series | target/arm: fix CBAR register for AArch64 CPUs | expand |
On Thu, 12 Sep 2019 at 12:01, Luc Michel <luc.michel@greensocs.com> wrote: > > For AArch64 CPUs with a CBAR register, we have two views for it: > - in AArch64 state, the CBAR_EL1 register (S3_1_C15_C3_0), returns the > full 64 bits CBAR value > - in AArch32 state, the CBAR register (cp15, opc1=1, CRn=15, CRm=3, opc2=0) > returns a 32 bits view such that: > CBAR = CBAR_EL1[31:18] 0..0 CBAR_EL1[43:32] > > This commit fixes the current implementation where: > - CBAR_EL1 was returning the 32 bits view instead of the full 64 bits > value, > - CBAR was returning a truncated 32 bits version of the full 64 bits > one, instead of the 32 bits view > - CBAR was declared as cp15, opc1=4, CRn=15, CRm=0, opc2=0, which is > the CBAR register found in the ARMv7 Cortex-Ax CPUs, but not in > ARMv8 CPUs. > > Signed-off-by: Luc Michel <luc.michel@greensocs.com> > --- > target/arm/helper.c | 6 +++--- > 1 file changed, 3 insertions(+), 3 deletions(-) > > diff --git a/target/arm/helper.c b/target/arm/helper.c > index 507026c915..755aa18a2d 100644 > --- a/target/arm/helper.c > +++ b/target/arm/helper.c > @@ -6740,12 +6740,12 @@ void register_cp_regs_for_features(ARMCPU *cpu) > ARMCPRegInfo cbar_reginfo[] = { > { .name = "CBAR", > .type = ARM_CP_CONST, > - .cp = 15, .crn = 15, .crm = 0, .opc1 = 4, .opc2 = 0, > - .access = PL1_R, .resetvalue = cpu->reset_cbar }, > + .cp = 15, .crn = 15, .crm = 3, .opc1 = 1, .opc2 = 0, > + .access = PL1_R, .resetvalue = cbar32 }, This will break the Cortex-A9 &c which use the 15/0/4/0 encoding and the un-rearranged value for this register. I think we need to check through the TRMs to confirm which CPUs use which format for the CBAR, and have a different feature bit for the newer format/sysreg encoding, so we can provide the right sysregs for the right cores. thanks -- PMM
Hi Peter, On 9/12/19 6:03 PM, Peter Maydell wrote: > On Thu, 12 Sep 2019 at 12:01, Luc Michel <luc.michel@greensocs.com> wrote: >> >> For AArch64 CPUs with a CBAR register, we have two views for it: >> - in AArch64 state, the CBAR_EL1 register (S3_1_C15_C3_0), returns the >> full 64 bits CBAR value >> - in AArch32 state, the CBAR register (cp15, opc1=1, CRn=15, CRm=3, opc2=0) >> returns a 32 bits view such that: >> CBAR = CBAR_EL1[31:18] 0..0 CBAR_EL1[43:32] >> >> This commit fixes the current implementation where: >> - CBAR_EL1 was returning the 32 bits view instead of the full 64 bits >> value, >> - CBAR was returning a truncated 32 bits version of the full 64 bits >> one, instead of the 32 bits view >> - CBAR was declared as cp15, opc1=4, CRn=15, CRm=0, opc2=0, which is >> the CBAR register found in the ARMv7 Cortex-Ax CPUs, but not in >> ARMv8 CPUs. >> >> Signed-off-by: Luc Michel <luc.michel@greensocs.com> >> --- >> target/arm/helper.c | 6 +++--- >> 1 file changed, 3 insertions(+), 3 deletions(-) >> >> diff --git a/target/arm/helper.c b/target/arm/helper.c >> index 507026c915..755aa18a2d 100644 >> --- a/target/arm/helper.c >> +++ b/target/arm/helper.c >> @@ -6740,12 +6740,12 @@ void register_cp_regs_for_features(ARMCPU *cpu) >> ARMCPRegInfo cbar_reginfo[] = { >> { .name = "CBAR", >> .type = ARM_CP_CONST, >> - .cp = 15, .crn = 15, .crm = 0, .opc1 = 4, .opc2 = 0, >> - .access = PL1_R, .resetvalue = cpu->reset_cbar }, >> + .cp = 15, .crn = 15, .crm = 3, .opc1 = 1, .opc2 = 0, >> + .access = PL1_R, .resetvalue = cbar32 }, > > This will break the Cortex-A9 &c which use the 15/0/4/0 encoding > and the un-rearranged value for this register. I don't think so because we are in the "if (arm_feature(env, ARM_FEATURE_AARCH64))" branch of the code. The else branch still maps 15/0/4/0 for non-AArch64 CPUs. > > I think we need to check through the TRMs to confirm which CPUs use > which format for the CBAR, and have a different feature bit for the > newer format/sysreg encoding, so we can provide the right sysregs for > the right cores. I checked all the AArch64 Cortex's TRMs, for those having a PERIPHBASE signal and CBAR register (namely Cortex-A53, 57, 72, 73), they all match the mapping I put in this patch, so I think we don't need to split the CBAR feature further. I believe more recent Cortex's address the GIC using coprocessor registers, and CBAR does not exist in those ones.
On 9/13/19 9:26 AM, Luc Michel wrote: > Hi Peter, > > On 9/12/19 6:03 PM, Peter Maydell wrote: >> On Thu, 12 Sep 2019 at 12:01, Luc Michel <luc.michel@greensocs.com> wrote: >>> >>> For AArch64 CPUs with a CBAR register, we have two views for it: >>> - in AArch64 state, the CBAR_EL1 register (S3_1_C15_C3_0), returns the >>> full 64 bits CBAR value >>> - in AArch32 state, the CBAR register (cp15, opc1=1, CRn=15, CRm=3, opc2=0) >>> returns a 32 bits view such that: >>> CBAR = CBAR_EL1[31:18] 0..0 CBAR_EL1[43:32] >>> >>> This commit fixes the current implementation where: >>> - CBAR_EL1 was returning the 32 bits view instead of the full 64 bits >>> value, >>> - CBAR was returning a truncated 32 bits version of the full 64 bits >>> one, instead of the 32 bits view >>> - CBAR was declared as cp15, opc1=4, CRn=15, CRm=0, opc2=0, which is >>> the CBAR register found in the ARMv7 Cortex-Ax CPUs, but not in >>> ARMv8 CPUs. >>> >>> Signed-off-by: Luc Michel <luc.michel@greensocs.com> >>> --- >>> target/arm/helper.c | 6 +++--- >>> 1 file changed, 3 insertions(+), 3 deletions(-) >>> >>> diff --git a/target/arm/helper.c b/target/arm/helper.c >>> index 507026c915..755aa18a2d 100644 >>> --- a/target/arm/helper.c >>> +++ b/target/arm/helper.c >>> @@ -6740,12 +6740,12 @@ void register_cp_regs_for_features(ARMCPU *cpu) >>> ARMCPRegInfo cbar_reginfo[] = { >>> { .name = "CBAR", >>> .type = ARM_CP_CONST, >>> - .cp = 15, .crn = 15, .crm = 0, .opc1 = 4, .opc2 = 0, >>> - .access = PL1_R, .resetvalue = cpu->reset_cbar }, >>> + .cp = 15, .crn = 15, .crm = 3, .opc1 = 1, .opc2 = 0, >>> + .access = PL1_R, .resetvalue = cbar32 }, >> >> This will break the Cortex-A9 &c which use the 15/0/4/0 encoding >> and the un-rearranged value for this register. > I don't think so because we are in the "if (arm_feature(env, > ARM_FEATURE_AARCH64))" branch of the code. The else branch still maps > 15/0/4/0 for non-AArch64 CPUs. > >> >> I think we need to check through the TRMs to confirm which CPUs use >> which format for the CBAR, and have a different feature bit for the >> newer format/sysreg encoding, so we can provide the right sysregs for >> the right cores. > I checked all the AArch64 Cortex's TRMs, for those having a PERIPHBASE > signal and CBAR register (namely Cortex-A53, 57, 72, 73), they all match > the mapping I put in this patch, so I think we don't need to split the > CBAR feature further. I believe more recent Cortex's address the GIC > using coprocessor registers, and CBAR does not exist in those ones. Hi Peter, Do you want me to re-roll this patch with some modifications? Thanks.
On Tue, 17 Sep 2019 at 09:43, Luc Michel <luc.michel@greensocs.com> wrote: > > On 9/13/19 9:26 AM, Luc Michel wrote: > > Hi Peter, > > > > On 9/12/19 6:03 PM, Peter Maydell wrote: > >> I think we need to check through the TRMs to confirm which CPUs use > >> which format for the CBAR, and have a different feature bit for the > >> newer format/sysreg encoding, so we can provide the right sysregs for > >> the right cores. > > I checked all the AArch64 Cortex's TRMs, for those having a PERIPHBASE > > signal and CBAR register (namely Cortex-A53, 57, 72, 73), they all match > > the mapping I put in this patch, so I think we don't need to split the > > CBAR feature further. I believe more recent Cortex's address the GIC > > using coprocessor registers, and CBAR does not exist in those ones. > > Hi Peter, > > Do you want me to re-roll this patch with some modifications? No, that's OK. Thanks for checking the TRMs. I think what I'll do is squash in this comment to the patch: diff --git a/target/arm/helper.c b/target/arm/helper.c index 755aa18a2dc..bc1130d989d 100644 --- a/target/arm/helper.c +++ b/target/arm/helper.c @@ -6733,6 +6733,19 @@ void register_cp_regs_for_features(ARMCPU *cpu) } if (arm_feature(env, ARM_FEATURE_CBAR)) { + /* + * CBAR is IMPDEF, but common on Arm Cortex-A implementations. + * There are two flavours: + * (1) older 32-bit only cores have a simple 32-bit CBAR + * (2) 64-bit cores have a 64-bit CBAR visible to AArch64, plus a + * 32-bit register visible to AArch32 at a different encoding + * to the "flavour 1" register and with the bits rearranged to + * be able to squash a 64-bit address into the 32-bit view. + * We distinguish the two via the ARM_FEATURE_AARCH64 flag, but + * in future if we support AArch32-only configs of some of the + * AArch64 cores we might need to add a specific feature flag + * to indicate cores with "flavour 2" CBAR. + */ if (arm_feature(env, ARM_FEATURE_AARCH64)) { /* 32 bit view is [31:18] 0...0 [43:32]. */ uint32_t cbar32 = (extract64(cpu->reset_cbar, 18, 14) << 18) and apply it to target-arm.next, if that's OK with you. thanks -- PMM
On 9/17/19 12:56 PM, Peter Maydell wrote: > On Tue, 17 Sep 2019 at 09:43, Luc Michel <luc.michel@greensocs.com> wrote: >> >> On 9/13/19 9:26 AM, Luc Michel wrote: >>> Hi Peter, >>> >>> On 9/12/19 6:03 PM, Peter Maydell wrote: >>>> I think we need to check through the TRMs to confirm which CPUs use >>>> which format for the CBAR, and have a different feature bit for the >>>> newer format/sysreg encoding, so we can provide the right sysregs for >>>> the right cores. >>> I checked all the AArch64 Cortex's TRMs, for those having a PERIPHBASE >>> signal and CBAR register (namely Cortex-A53, 57, 72, 73), they all match >>> the mapping I put in this patch, so I think we don't need to split the >>> CBAR feature further. I believe more recent Cortex's address the GIC >>> using coprocessor registers, and CBAR does not exist in those ones. >> >> Hi Peter, >> >> Do you want me to re-roll this patch with some modifications? > > No, that's OK. Thanks for checking the TRMs. I think what I'll > do is squash in this comment to the patch: > > diff --git a/target/arm/helper.c b/target/arm/helper.c > index 755aa18a2dc..bc1130d989d 100644 > --- a/target/arm/helper.c > +++ b/target/arm/helper.c > @@ -6733,6 +6733,19 @@ void register_cp_regs_for_features(ARMCPU *cpu) > } > > if (arm_feature(env, ARM_FEATURE_CBAR)) { > + /* > + * CBAR is IMPDEF, but common on Arm Cortex-A implementations. > + * There are two flavours: > + * (1) older 32-bit only cores have a simple 32-bit CBAR > + * (2) 64-bit cores have a 64-bit CBAR visible to AArch64, plus a > + * 32-bit register visible to AArch32 at a different encoding > + * to the "flavour 1" register and with the bits rearranged to > + * be able to squash a 64-bit address into the 32-bit view. > + * We distinguish the two via the ARM_FEATURE_AARCH64 flag, but > + * in future if we support AArch32-only configs of some of the > + * AArch64 cores we might need to add a specific feature flag > + * to indicate cores with "flavour 2" CBAR. > + */ > if (arm_feature(env, ARM_FEATURE_AARCH64)) { > /* 32 bit view is [31:18] 0...0 [43:32]. */ > uint32_t cbar32 = (extract64(cpu->reset_cbar, 18, 14) << 18) > > > and apply it to target-arm.next, if that's OK with you. Yes sure! Thanks.
diff --git a/target/arm/helper.c b/target/arm/helper.c index 507026c915..755aa18a2d 100644 --- a/target/arm/helper.c +++ b/target/arm/helper.c @@ -6740,12 +6740,12 @@ void register_cp_regs_for_features(ARMCPU *cpu) ARMCPRegInfo cbar_reginfo[] = { { .name = "CBAR", .type = ARM_CP_CONST, - .cp = 15, .crn = 15, .crm = 0, .opc1 = 4, .opc2 = 0, - .access = PL1_R, .resetvalue = cpu->reset_cbar }, + .cp = 15, .crn = 15, .crm = 3, .opc1 = 1, .opc2 = 0, + .access = PL1_R, .resetvalue = cbar32 }, { .name = "CBAR_EL1", .state = ARM_CP_STATE_AA64, .type = ARM_CP_CONST, .opc0 = 3, .opc1 = 1, .crn = 15, .crm = 3, .opc2 = 0, - .access = PL1_R, .resetvalue = cbar32 }, + .access = PL1_R, .resetvalue = cpu->reset_cbar }, REGINFO_SENTINEL }; /* We don't implement a r/w 64 bit CBAR currently */
For AArch64 CPUs with a CBAR register, we have two views for it: - in AArch64 state, the CBAR_EL1 register (S3_1_C15_C3_0), returns the full 64 bits CBAR value - in AArch32 state, the CBAR register (cp15, opc1=1, CRn=15, CRm=3, opc2=0) returns a 32 bits view such that: CBAR = CBAR_EL1[31:18] 0..0 CBAR_EL1[43:32] This commit fixes the current implementation where: - CBAR_EL1 was returning the 32 bits view instead of the full 64 bits value, - CBAR was returning a truncated 32 bits version of the full 64 bits one, instead of the 32 bits view - CBAR was declared as cp15, opc1=4, CRn=15, CRm=0, opc2=0, which is the CBAR register found in the ARMv7 Cortex-Ax CPUs, but not in ARMv8 CPUs. Signed-off-by: Luc Michel <luc.michel@greensocs.com> --- target/arm/helper.c | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-)