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

[Peter Crosthwaite]

On Sat, Jan 25, 2014 at 10:09 AM, Peter Maydell
<address@hidden> wrote:
> 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.
>

Seems similar compexity to me. But I guess this preserves existing behaviour so:

Reviewed-by: Peter Crosthwaite <address@hidden>

> thanks
> -- PMM
>