On 7 October 2014 02:12, Peter Maydell <peter.mayd...@linaro.org> wrote:
> On 7 October 2014 06:06, Greg Bellows <greg.bell...@linaro.org> wrote:
> >
> >
> > On 6 October 2014 11:19, Peter Maydell <peter.mayd...@linaro.org> wrote:
> >>
> >> On 30 September 2014 22:49, Greg Bellows <greg.bell...@linaro.org>
> wrote:
> >> > From: Fabian Aggeler <aggel...@ethz.ch>
> >> >
> >> > 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 <aggel...@ethz.ch>
> >> > Signed-off-by: Greg Bellows <greg.bell...@linaro.org>
> >> >
> >> > ----------
> >> > 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
>
