Re: [Qemu-devel] [PATCH 19/24] target-arm: Implement AArch64 TTBR*

[Peter Maydell]

On 24 January 2014 23:44, Peter Crosthwaite
<address@hidden> wrote:
> On Wed, Jan 22, 2014 at 6:12 AM, Peter Maydell <address@hidden> wrote:
>> Implement the AArch64 TTBR* registers. For v7 these were already 64 bits
>> to handle LPAE, but implemented as two separate uint32_t fields.
>> Combine them into a single uint64_t which can be used for all purposes.
>> Since this requires touching every use, take the opportunity to rename
>> the field to the architectural name.
>>
>> Signed-off-by: Peter Maydell <address@hidden>
>> ---
>>  hw/arm/pxa2xx.c     |  2 +-
>>  target-arm/cpu.h    |  6 ++--
>>  target-arm/helper.c | 95 
>> +++++++++++++++++------------------------------------
>>  3 files changed, 33 insertions(+), 70 deletions(-)
>>
>> diff --git a/hw/arm/pxa2xx.c b/hw/arm/pxa2xx.c
>> index 02b7016..b6465d6 100644
>> --- a/hw/arm/pxa2xx.c
>> +++ b/hw/arm/pxa2xx.c
>> @@ -279,7 +279,7 @@ static int pxa2xx_pwrmode_write(CPUARMState *env, const 
>> ARMCPRegInfo *ri,
>>              ARM_CPU_MODE_SVC | CPSR_A | CPSR_F | CPSR_I;
>>          s->cpu->env.cp15.c1_sys = 0;
>>          s->cpu->env.cp15.c1_coproc = 0;
>> -        s->cpu->env.cp15.c2_base0 = 0;
>> +        s->cpu->env.cp15.ttbr0_el1 = 0;
>>          s->cpu->env.cp15.c3 = 0;
>>          s->pm_regs[PSSR >> 2] |= 0x8; /* Set STS */
>>          s->pm_regs[RCSR >> 2] |= 0x8; /* Set GPR */
>> diff --git a/target-arm/cpu.h b/target-arm/cpu.h
>> index 6f4d174..7138882 100644
>> --- a/target-arm/cpu.h
>> +++ b/target-arm/cpu.h
>> @@ -173,10 +173,8 @@ typedef struct CPUARMState {
>>          uint32_t c1_coproc; /* Coprocessor access register.  */
>>          uint32_t c1_xscaleauxcr; /* XScale auxiliary control register.  */
>>          uint32_t c1_scr; /* secure config register.  */
>> -        uint32_t c2_base0; /* MMU translation table base 0.  */
>> -        uint32_t c2_base0_hi; /* MMU translation table base 0, high 32 bits 
>> */
>> -        uint32_t c2_base1; /* MMU translation table base 0.  */
>> -        uint32_t c2_base1_hi; /* MMU translation table base 1, high 32 bits 
>> */
>> +        uint64_t ttbr0_el1; /* MMU translation table base 0. */
>> +        uint32_t ttbr1_el1; /* MMU translation table base 1. */
>>          uint64_t c2_control; /* MMU translation table base control.  */
>>          uint32_t c2_mask; /* MMU translation table base selection mask.  */
>>          uint32_t c2_base_mask; /* MMU translation table base 0 mask. */
>> diff --git a/target-arm/helper.c b/target-arm/helper.c
>> index e2ae159..1f1dec1 100644
>> --- a/target-arm/helper.c
>> +++ b/target-arm/helper.c
>> @@ -1288,6 +1288,18 @@ static int vmsa_tcr_el1_write(CPUARMState *env, const 
>> ARMCPRegInfo *ri,
>>      return 0;
>>  }
>>
>> +static int vmsa_ttbr_write(CPUARMState *env, const ARMCPRegInfo *ri,
>> +                             uint64_t value)
>> +{
>> +    /* 64 bit accesses to the TTBRs can change the ASID and so we
>> +     * must flush the TLB.
>> +     */
>> +    if ((ri->state == ARM_CP_STATE_AA64) || (ri->type & ARM_CP_64BIT)) {
>> +        tlb_flush(env, 1);
>> +    }
>
> With the level of complexity this if has reached, is it better to just
> check for this ASID change rather than make this overly conservative
> flush?

Adding an "is the ASID the same" would make the check more complicated
again. Maybe we should do it but I'd rather keep that separate from the
"handle 64 bit formats" patches.

thanks
-- PMM