Re: [Qemu-devel] [PATCH v8 18/27] target-arm: make c2_mask and c2_base_mask banked

[Greg Bellows]

Ah... I was confused and thinking of ttbcr_raw_write.  Yes, raw_write does not actually touch the masks.  

The reason we used raw_write is that it updates the correct ri offset.  We could be calling with the secure or non-secure ri so it was necessary to make sure the correct offset gets updated. 

Staying with this pattern, I introduced a new utility function raw_ptr() that gives back the pointer calculation based on the ri fieldoffset.  For TCR, this will be the base of the TCR struct which we can then update.

On 4 November 2014 17:27, Peter Maydell <address@hidden> wrote:

On 4 November 2014 22:46, Greg Bellows <address@hidden> wrote:
>
>
> On 31 October 2014 10:26, Peter Maydell <address@hidden> wrote:
>>
>> On 30 October 2014 21:28, Greg Bellows <address@hidden> wrote:
>> >  static void vmsa_ttbcr_reset(CPUARMState *env, const ARMCPRegInfo *ri)
>> >  {
>> > -    env->cp15.c2_base_mask = 0xffffc000u;
>> > +    /* Rest both the TTBCR as well as the masks corresponding to the
>> > bank of
>> > +     * the TTBCR being reset.
>> > +     */
>> > +    A32_BANKED_REG_SET(env, c2_base_mask,
>> > +                       ARM_CP_SECSTATE_TEST(ri, ARM_CP_SECSTATE_S),
>> > +                       0xffffc000u);
>> > +    A32_BANKED_REG_SET(env, c2_mask,
>> > +                       ARM_CP_SECSTATE_TEST(ri, ARM_CP_SECSTATE_S), 0);
>> >      raw_write(env, ri, 0);
>> > -    env->cp15.c2_mask = 0;
>>
>> Similarly this will be much cleaner.
>
>
> Changed in v9.  Question on reset.  We call raw_write() which also sets the
> masks, but we set the masks separately here too, but different values.  It
> seems we should only need to set them in raw_write() is this true?

No, raw_write() won't set the masks -- it just writes 32 or 64 bits
to the field pointed to by fieldoffset. Which makes it a pretty
obfuscated way of saying env->cp15.c2_control = 0; and I don't
know why we do it this way currently. If we go to having a TTBCR
struct we should just set all the fields directly here I think.

-- PMM