Re: [Qemu-devel] [PATCH v2 19/35] target-arm: A64: Make cache ID registers visible to AArch64

[Peter Maydell]

On 7 February 2014 07:35, Hu Tao <address@hidden> wrote:
> On Fri, Jan 31, 2014 at 03:45:27PM +0000, Peter Maydell wrote:
>> Make the cache ID system registers (CLIDR, CCSELR, CCSIDR, CTR)
>
> s/CCSELR/CSSELR/
>
>> visible to AArch64. These are mostly simple 64-bit extensions of the
>> existing 32 bit system registers and so can share reginfo definitions.
>
> According to the document(ARM DDI 0487A.a), some AArch64 system
> registers are 32-bit, for example CCSIDR_EL1 is 32-bit. But System_Put()
> and System_Get() writes/reads 64-bit values, which makes me confused.

We've been round this issue before. The documentation
uses "32-bit" as a shorthand for "64 bit but the top
32 bits are RES0". There is no way in AArch64 to do a
32 bit read or write of a system register -- the only
instructions are MSR/MRS, which are always 64 bits.
Similarly, the KVM kernel API exposes all registers as
64 bits wide.

We could in theory have a mechanism that allowed a
64 bit system register access to be backed by a 32 bit
field value, but it's simpler not to.

>> CTR needs to have a split definition, but we can clean up the
>> temporary user-mode implementation in favour of using the CPU-specified
>> reset value, and implement the system-mode-required semantics of
>> restricting its EL0 accessibility if SCTLR.UCT is not set.
>>
>> Signed-off-by: Peter Maydell <address@hidden>
>> ---
>>  target-arm/cpu.c    |  2 ++
>>  target-arm/cpu.h    |  2 +-
>>  target-arm/cpu64.c  |  1 +
>>  target-arm/helper.c | 31 +++++++++++++++++++++----------
>>  4 files changed, 25 insertions(+), 11 deletions(-)
>>
>> diff --git a/target-arm/cpu.c b/target-arm/cpu.c
>> index fe18b65..8fed098 100644
>> --- a/target-arm/cpu.c
>> +++ b/target-arm/cpu.c
>> @@ -91,6 +91,8 @@ static void arm_cpu_reset(CPUState *s)
>>          env->aarch64 = 1;
>>  #if defined(CONFIG_USER_ONLY)
>>          env->pstate = PSTATE_MODE_EL0t;
>> +        /* Userspace expects access to CTL_EL0 */
>> +        env->cp15.c1_sys |= SCTLR_UCT;
>>  #else
>>          env->pstate = PSTATE_D | PSTATE_A | PSTATE_I | PSTATE_F
>>              | PSTATE_MODE_EL1h;
>> diff --git a/target-arm/cpu.h b/target-arm/cpu.h
>> index d1ed423..f5b706e 100644
>> --- a/target-arm/cpu.h
>> +++ b/target-arm/cpu.h
>> @@ -166,7 +166,7 @@ typedef struct CPUARMState {
>>      /* System control coprocessor (cp15) */
>>      struct {
>>          uint32_t c0_cpuid;
>> -        uint32_t c0_cssel; /* Cache size selection.  */
>> +        uint64_t c0_cssel; /* Cache size selection.  */
>
> I see all backing fields for AArch64 system registers are converted to
> uint64_t, why not convert ARMCPU.ccsidr which backs CCSIDR?

It's not directly accessed via a .fieldoffset() entry,
so there's no requirement for it to be 64 bits wide.
I've also generally left the ARMCPU values which are
just constants to be used in ".resetvalue =" specifications
alone. It's only the cases where a struct field is the
backing storage for the register that need widening.

thanks
-- PMM