Re: [Qemu-devel] [PATCH v5 11/33] target-arm: arrayfying fieldoffset for banking

[Greg Bellows]

On 7 October 2014 02:12, Peter Maydell <address@hidden> wrote:

On 7 October 2014 06:06, Greg Bellows <address@hidden> wrote:
>
>
> On 6 October 2014 11:19, Peter Maydell <address@hidden> wrote:
>>
>> On 30 September 2014 22:49, Greg Bellows <address@hidden> wrote:
>> > From: Fabian Aggeler <address@hidden>
>> >
>> > Prepare ARMCPRegInfo to support specifying two fieldoffsets per
>> > register definition. This will allow us to keep one register
>> > definition for banked registers (different offsets for secure/
>> > non-secure world).
>> >
>> > Signed-off-by: Fabian Aggeler <address@hidden>
>> > Signed-off-by: Greg Bellows <address@hidden>
>> >
>> > ----------
>> > v4 -> v5
>> > - Added ARM CP register secure and non-secure bank flags
>> > - Added setting of secure and non-secure flags furing registration
>> > ---
>> >  target-arm/cpu.h    | 23 +++++++++++++++-----
>> >  target-arm/helper.c | 60
>> > +++++++++++++++++++++++++++++++++++++++++------------
>> >  2 files changed, 65 insertions(+), 18 deletions(-)
>> >
>> > diff --git a/target-arm/cpu.h b/target-arm/cpu.h
>> > index 1700676..9681d45 100644
>> > --- a/target-arm/cpu.h
>> > +++ b/target-arm/cpu.h
>> > @@ -958,10 +958,12 @@ static inline uint64_t cpreg_to_kvm_id(uint32_t
>> > cpregid)
>> >  #define ARM_CP_CURRENTEL (ARM_CP_SPECIAL | (4 << 8))
>> >  #define ARM_CP_DC_ZVA (ARM_CP_SPECIAL | (5 << 8))
>> >  #define ARM_LAST_SPECIAL ARM_CP_DC_ZVA
>> > +#define ARM_CP_BANK_S   (1 << 16)
>> > +#define ARM_CP_BANK_NS  (2 << 16)
>>
>> I thought we were going to put these flags into a reginfo->secure
>> field? Mixing them into the 'type' bits seems unnecessarily
>> confusing to me.
>
>
> Hmmm... that's not how I interpreted our discussion.  We discussed having
> BANK_ flags which I figured we were talking about the existing flags.  So,
> you are thinking that the "secure" field becomes a separate flags, so we
> would have 2 flags fields.  Not sure that is any less confusing, maybe more
> because then you have to worry about the flags being put in the right place.

Sorry for any confusion. My intention was that the previous
'secure' field which just had a 1/0 value should have flags
in it instead. Note that we don't have a generic "flags" field;
we have a "type" field which indicates properties of how the
register itself behaves (unrelated to what encodings and
states it is visible from), we have a "state" field which has
the flags for whether it is visible from AArch32 or AArch64
or both, and we have an "access" field which has flags for
whether it is readable or writable from various exception
levels. I think having a separate "secure" field is easier
to understand and fits into that approach.

Fixed in v6 by separating out the security flags and adding a secure field.

 

>> > -    ptrdiff_t fieldoffset; /* offsetof(CPUARMState, field) */
>> > +    union { /* offsetof(CPUARMState, field) */
>> > +        struct {
>> > +            ptrdiff_t fieldoffset_padding;
>> > +            ptrdiff_t fieldoffset;
>>
>> ...why is the padding field first? Given that we always write
>> fieldoffset when we put the banked versions into the hash table
>> I don't think it should matter, should it?
>
>
> The padding aligns the existing fieldoffset with the non-secure bank.  For
> correctness, I added the padding to truly align the default fieldoffset with
> the non-secure bank.  I don't think it matters otherwise.

But do we ever write to "fieldoffset" and then read from
"bank_fieldoffsets[1]" (or vice versa)? If we don't then it's
not necessary for correctness at all... (If we do do that, where
does it happen?)

In short, No.  The only time we use bank_fieldoffset is during registration and that is to fixup fieldoffset to contain the correct bank offset.  Otherwise, we always use fieldoffset when determining the register offset.

I'm still trying to wrap my head around it, but I believe there are cases where we use a different register set depending on whether a given EL is 32 or 64-bit.  I need to spend a bit more time working through the scenarios. 

 

thanks
-- PMM