[Qemu-devel] [PATCH] target/arm: fix CBAR register for AArch64 CPUs

[Qemu-devel] [PATCH] target/arm: fix CBAR register for AArch64 CPUs

[Luc Michel]

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 },
                 { .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 */
-- 
2.23.0

Re: [Qemu-devel] [PATCH] target/arm: fix CBAR register for AArch64 CPUs

[Peter Maydell]

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

Re: [Qemu-devel] [PATCH] target/arm: fix CBAR register for AArch64 CPUs

[Luc Michel]

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.

-- 
Luc

> 
> thanks
> -- PMM
> 

Re: [Qemu-devel] [PATCH] target/arm: fix CBAR register for AArch64 CPUs

[Luc Michel]

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.

-- 
Luc

Re: [Qemu-devel] [PATCH] target/arm: fix CBAR register for AArch64 CPUs

[Peter Maydell]

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

Re: [Qemu-devel] [PATCH] target/arm: fix CBAR register for AArch64 CPUs

[Luc Michel]

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.

-- 
Luc

> 
> thanks
> -- PMM
>