Re: [Qemu-arm] [PATCH v9 08/14] target/arm: Make PMCEID[01]_EL0 64 bit registers, add PMCEID[23]

[Richard Henderson]

On 12/5/18 9:32 AM, Aaron Lindsay wrote:
> On Dec 05 08:43, Aaron Lindsay wrote:
>> Signed-off-by: Aaron Lindsay <address@hidden>
>> ---
>>  target/arm/cpu.h    |  4 ++--
>>  target/arm/helper.c | 18 ++++++++++++++++--
>>  2 files changed, 18 insertions(+), 4 deletions(-)
>>
>> diff --git a/target/arm/cpu.h b/target/arm/cpu.h
>> index 304e6e47b3..4216fe22db 100644
>> --- a/target/arm/cpu.h
>> +++ b/target/arm/cpu.h
>> @@ -837,8 +837,8 @@ struct ARMCPU {
>>      uint32_t id_pfr0;
>>      uint32_t id_pfr1;
>>      uint32_t id_dfr0;
>> -    uint32_t pmceid0;
>> -    uint32_t pmceid1;
>> +    uint64_t pmceid0;
>> +    uint64_t pmceid1;
>>      uint32_t id_afr0;
>>      uint32_t id_mmfr0;
>>      uint32_t id_mmfr1;
>> diff --git a/target/arm/helper.c b/target/arm/helper.c
>> index 71be6fb578..fb6939e99c 100644
>> --- a/target/arm/helper.c
>> +++ b/target/arm/helper.c
>> @@ -5256,6 +5256,20 @@ void register_cp_regs_for_features(ARMCPU *cpu)
>>      } else {
>>          define_arm_cp_regs(cpu, not_v7_cp_reginfo);
>>      }
>> +    if (FIELD_EX32(cpu->id_dfr0, ID_DFR0, PERFMON) >= 4) {
> 
> After further discussion on my last version, this should be
> 
> if (FIELD_EX32(cpu->id_dfr0, ID_DFR0, PERFMON) >= 4 &&
>       FIELD_EX32(cpu->id_dfr0, ID_DFR0, PERFMON) != 0xf) {
> 
> to guard against defining these registers for implementation-defined
> PMUs.

When id fields define values like 0b1111, that is a hint that the field should
be interpreted as signed, and you should still use a >= comparison.  (See
D12.1.4, Principles of the ID scheme for fields in ID registers.)

A patch adding

#define FIELD_EX32S(storage, reg, field)                                   \
    sextract32((storage), R_ ## reg ## _ ## field ## _SHIFT,               \
               R_ ## reg ## _ ## field ## _LENGTH)
#define FIELD_EX64S(storage, reg, field)                                   \
    sextract64((storage), R_ ## reg ## _ ## field ## _SHIFT,               \
               R_ ## reg ## _ ## field ## _LENGTH)

to include/hw/registerfields.h would be welcome and appropriate, I think.


r~