[v6] Add ARM Cortex-R52 CPU

[PATCH v6 0/7] Add ARM Cortex-R52 CPU

Posted by tobias.roehmel@rwth-aachen.de 3 years, 2 months ago

From: Tobias Röhmel <tobias.roehmel@rwth-aachen.de>

Sorry for the "Reviewed-by" messup. I missed that on the explanation
page. Thanks again for the review :)

v6:
patch 5:
- I also changed HPRENR from ARM_CP_ALIAS to ARM_CP_NO_RAW.
  Its state is also present in the HPRLAR registers,
  but it doesn't make sense to access it raw. (I think)
- I'm freeing the PRBAR/... strings explicitly now since
  I don't know how to use autofree in this setup correctly.
  Maybe {} around the part were the string is created/used,
  such that it is dropped at }?



Tobias Röhmel (7):
  target/arm: Don't add all MIDR aliases for cores that implement PMSA
  target/arm: Make RVBAR available for all ARMv8 CPUs
  target/arm: Make stage_2_format for cache attributes optional
  target/arm: Enable TTBCR_EAE for ARMv8-R AArch32
  target/arm: Add PMSAv8r registers
  target/arm: Add PMSAv8r functionality
  target/arm: Add ARM Cortex-R52 CPU

 target/arm/cpu.c          |  34 +++-
 target/arm/cpu.h          |   6 +
 target/arm/cpu_tcg.c      |  42 +++++
 target/arm/debug_helper.c |   3 +
 target/arm/helper.c       | 336 ++++++++++++++++++++++++++++++++++++--
 target/arm/internals.h    |   4 +
 target/arm/machine.c      |  28 ++++
 target/arm/ptw.c          | 136 ++++++++++++---
 target/arm/tlb_helper.c   |   4 +
 9 files changed, 553 insertions(+), 40 deletions(-)

-- 
2.34.1

Re: [PATCH v6 0/7] Add ARM Cortex-R52 CPU

Posted by Peter Maydell 3 years, 1 month ago

On Tue, 6 Dec 2022 at 10:25, <tobias.roehmel@rwth-aachen.de> wrote:
>
> From: Tobias Röhmel <tobias.roehmel@rwth-aachen.de>
>
> Sorry for the "Reviewed-by" messup. I missed that on the explanation
> page. Thanks again for the review :)
>
> v6:
> patch 5:
> - I also changed HPRENR from ARM_CP_ALIAS to ARM_CP_NO_RAW.
>   Its state is also present in the HPRLAR registers,
>   but it doesn't make sense to access it raw. (I think)
> - I'm freeing the PRBAR/... strings explicitly now since
>   I don't know how to use autofree in this setup correctly.
>   Maybe {} around the part were the string is created/used,
>   such that it is dropped at }?
>

Applied to target-arm.next; thanks for your efforts in getting
this patchset through the code review process.

-- PMM

Re: [PATCH v6 0/7] Add ARM Cortex-R52 CPU

Posted by Tobias Röhmel 3 years, 1 month ago

Thanks for all the help, I learned a lot!

Best regards,

Tobias

On 19.12.22 18:05, Peter Maydell wrote:
> On Tue, 6 Dec 2022 at 10:25, <tobias.roehmel@rwth-aachen.de> wrote:
>> From: Tobias Röhmel <tobias.roehmel@rwth-aachen.de>
>>
>> Sorry for the "Reviewed-by" messup. I missed that on the explanation
>> page. Thanks again for the review :)
>>
>> v6:
>> patch 5:
>> - I also changed HPRENR from ARM_CP_ALIAS to ARM_CP_NO_RAW.
>>    Its state is also present in the HPRLAR registers,
>>    but it doesn't make sense to access it raw. (I think)
>> - I'm freeing the PRBAR/... strings explicitly now since
>>    I don't know how to use autofree in this setup correctly.
>>    Maybe {} around the part were the string is created/used,
>>    such that it is dropped at }?
>>
> Applied to target-arm.next; thanks for your efforts in getting
> this patchset through the code review process.
>
> -- PMM

Re: [PATCH v6 0/7] Add ARM Cortex-R52 CPU

Posted by Philippe Mathieu-Daudé 3 years, 2 months ago

On 6/12/22 11:24, tobias.roehmel@rwth-aachen.de wrote:
> From: Tobias Röhmel <tobias.roehmel@rwth-aachen.de>

> v6:
> patch 5:
> - I'm freeing the PRBAR/... strings explicitly now since
>    I don't know how to use autofree in this setup correctly.
>    Maybe {} around the part were the string is created/used,
>    such that it is dropped at }?

The pointer is declared outside of a for() statement. Then
inside this statement you alloc/free twice, using the same
pointer. This is correct.  If you really want to use
g_autofree in such case, you'd need to declare within the
same statement, one pointer for each string:

   for (i = 0; i < MIN(cpu->pmsav7_dregion, 32); ++i) {
       uint8_t crm = 0b1000 | extract32(i, 1, 3);
       uint8_t opc1 = extract32(i, 4, 1);
       uint8_t opc2 = extract32(i, 0, 1) << 2;
       g_autofree char *prbarn_str = g_strdup_printf("PRBAR%u", i);
       g_autofree char *prlarn_str = g_strdup_printf("PRLAR%u", i);

       const ARMCPRegInfo tmp_prbarn_reginfo = {
           .name = prbarn_str, .type = ARM_CP_ALIAS | ARM_CP_NO_RAW,
           .cp = 15, .opc1 = opc1, .crn = 6, .crm = crm, .opc2 = opc2,
           .access = PL1_RW, .resetvalue = 0,
           .accessfn = access_tvm_trvm,
           .writefn = pmsav8r_regn_write, .readfn = pmsav8r_regn_read
       };
       define_one_arm_cp_reg(cpu, &tmp_prbarn_reginfo);

       opc2 = extract32(i, 0, 1) << 2 | 0x1;
       const ARMCPRegInfo tmp_prlarn_reginfo = {
           .name = prlarn_str, .type = ARM_CP_ALIAS | ARM_CP_NO_RAW,
           .cp = 15, .opc1 = opc1, .crn = 6, .crm = crm, .opc2 = opc2,
           .access = PL1_RW, .resetvalue = 0,
           .accessfn = access_tvm_trvm,
           .writefn = pmsav8r_regn_write, .readfn = pmsav8r_regn_read
       };
       define_one_arm_cp_reg(cpu, &tmp_prlarn_reginfo);
   }

(Note ARMCPRegInfo can be qualified const).

Regards,

Phil.

Re: [PATCH v6 0/7] Add ARM Cortex-R52 CPU

Posted by Tobias Roehmel 3 years, 2 months ago

On 06.12.22 11:39, Philippe Mathieu-Daudé wrote:
> On 6/12/22 11:24, tobias.roehmel@rwth-aachen.de wrote:
>> From: Tobias Röhmel <tobias.roehmel@rwth-aachen.de>
>
>> v6:
>> patch 5:
>> - I'm freeing the PRBAR/... strings explicitly now since
>>    I don't know how to use autofree in this setup correctly.
>>    Maybe {} around the part were the string is created/used,
>>    such that it is dropped at }?
>
> The pointer is declared outside of a for() statement. Then
> inside this statement you alloc/free twice, using the same
> pointer. This is correct.  If you really want to use
> g_autofree in such case, you'd need to declare within the
> same statement, one pointer for each string:
>
>   for (i = 0; i < MIN(cpu->pmsav7_dregion, 32); ++i) {
>       uint8_t crm = 0b1000 | extract32(i, 1, 3);
>       uint8_t opc1 = extract32(i, 4, 1);
>       uint8_t opc2 = extract32(i, 0, 1) << 2;
>       g_autofree char *prbarn_str = g_strdup_printf("PRBAR%u", i);
>       g_autofree char *prlarn_str = g_strdup_printf("PRLAR%u", i);
>
>       const ARMCPRegInfo tmp_prbarn_reginfo = {
>           .name = prbarn_str, .type = ARM_CP_ALIAS | ARM_CP_NO_RAW,
>           .cp = 15, .opc1 = opc1, .crn = 6, .crm = crm, .opc2 = opc2,
>           .access = PL1_RW, .resetvalue = 0,
>           .accessfn = access_tvm_trvm,
>           .writefn = pmsav8r_regn_write, .readfn = pmsav8r_regn_read
>       };
>       define_one_arm_cp_reg(cpu, &tmp_prbarn_reginfo);
>
>       opc2 = extract32(i, 0, 1) << 2 | 0x1;
>       const ARMCPRegInfo tmp_prlarn_reginfo = {
>           .name = prlarn_str, .type = ARM_CP_ALIAS | ARM_CP_NO_RAW,
>           .cp = 15, .opc1 = opc1, .crn = 6, .crm = crm, .opc2 = opc2,
>           .access = PL1_RW, .resetvalue = 0,
>           .accessfn = access_tvm_trvm,
>           .writefn = pmsav8r_regn_write, .readfn = pmsav8r_regn_read
>       };
>       define_one_arm_cp_reg(cpu, &tmp_prlarn_reginfo);
>   }
>
> (Note ARMCPRegInfo can be qualified const).
>
> Regards,
>
> Phil.

Thanks for the explanation! Would that be the preferred way?

Best regards,

Tobias

Re: [PATCH v6 0/7] Add ARM Cortex-R52 CPU

Posted by Philippe Mathieu-Daudé 3 years, 2 months ago

On 6/12/22 12:43, Tobias Roehmel wrote:
> 
> On 06.12.22 11:39, Philippe Mathieu-Daudé wrote:
>> On 6/12/22 11:24, tobias.roehmel@rwth-aachen.de wrote:
>>> From: Tobias Röhmel <tobias.roehmel@rwth-aachen.de>
>>
>>> v6:
>>> patch 5:
>>> - I'm freeing the PRBAR/... strings explicitly now since
>>>    I don't know how to use autofree in this setup correctly.
>>>    Maybe {} around the part were the string is created/used,
>>>    such that it is dropped at }?
>>
>> The pointer is declared outside of a for() statement. Then
>> inside this statement you alloc/free twice, using the same
>> pointer. This is correct.  If you really want to use
>> g_autofree in such case, you'd need to declare within the
>> same statement, one pointer for each string:
>>
>>   for (i = 0; i < MIN(cpu->pmsav7_dregion, 32); ++i) {
>>       uint8_t crm = 0b1000 | extract32(i, 1, 3);
>>       uint8_t opc1 = extract32(i, 4, 1);
>>       uint8_t opc2 = extract32(i, 0, 1) << 2;
>>       g_autofree char *prbarn_str = g_strdup_printf("PRBAR%u", i);
>>       g_autofree char *prlarn_str = g_strdup_printf("PRLAR%u", i);
>>
>>       const ARMCPRegInfo tmp_prbarn_reginfo = {
>>           .name = prbarn_str, .type = ARM_CP_ALIAS | ARM_CP_NO_RAW,
>>           .cp = 15, .opc1 = opc1, .crn = 6, .crm = crm, .opc2 = opc2,
>>           .access = PL1_RW, .resetvalue = 0,
>>           .accessfn = access_tvm_trvm,
>>           .writefn = pmsav8r_regn_write, .readfn = pmsav8r_regn_read
>>       };
>>       define_one_arm_cp_reg(cpu, &tmp_prbarn_reginfo);
>>
>>       opc2 = extract32(i, 0, 1) << 2 | 0x1;
>>       const ARMCPRegInfo tmp_prlarn_reginfo = {
>>           .name = prlarn_str, .type = ARM_CP_ALIAS | ARM_CP_NO_RAW,
>>           .cp = 15, .opc1 = opc1, .crn = 6, .crm = crm, .opc2 = opc2,
>>           .access = PL1_RW, .resetvalue = 0,
>>           .accessfn = access_tvm_trvm,
>>           .writefn = pmsav8r_regn_write, .readfn = pmsav8r_regn_read
>>       };
>>       define_one_arm_cp_reg(cpu, &tmp_prlarn_reginfo);
>>   }
>>
>> (Note ARMCPRegInfo can be qualified const).
>>
>> Regards,
>>
>> Phil.
> 
> Thanks for the explanation! Would that be the preferred way?

What you posted is good enough IMO :) AFAIK there is no formal style
recommendation on using g_autofree.