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.
>
> > /* Used only as a terminator for ARMCPRegInfo lists */
> > -#define ARM_CP_SENTINEL 0xffff
> > +#define ARM_CP_SENTINEL 0xffffff
> > /* Mask of only the flag bits in a type field */
> > -#define ARM_CP_FLAG_MASK 0x7f
> > +#define ARM_CP_FLAG_MASK 0x3007f
> >
> > /* Valid values for ARMCPRegInfo state field, indicating which of
> > * the AArch32 and AArch64 execution states this register is visible in.
> > @@ -1096,6 +1098,7 @@ struct ARMCPRegInfo {
> > uint8_t opc0;
> > uint8_t opc1;
> > uint8_t opc2;
> > +
>
> Stray whitespace change.
>
Fixed in v6
>
> > /* Execution state in which this register is visible:
> ARM_CP_STATE_* */
> > int state;
> > /* Register type: ARM_CP_* bits/values */
> > @@ -1111,12 +1114,22 @@ struct ARMCPRegInfo {
> > * fieldoffset is non-zero, the reset value of the register.
> > */
> > uint64_t resetvalue;
> > - /* Offset of the field in CPUARMState for this register. This is not
> > - * needed if either:
> > + /* Offsets of the fields (secure/non-secure) in CPUARMState for this
> > + * register. The array will be accessed by the ns bit which means
> the
> > + * secure instance has to be at [0] while the non-secure instance
> must be
> > + * at [1]. If a register is not banked .fieldoffset can be used,
> which maps
> > + * to the non-secure bank.
> > + * This is not needed if either:
> > * 1. type is ARM_CP_CONST or one of the ARM_CP_SPECIALs
> > * 2. both readfn and writefn are specified
> > */
> > - 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.
>
> > + };
> > + ptrdiff_t bank_fieldoffsets[2];
> > + };
> > /* Function for making any access checks for this register in
> addition to
> > * those specified by the 'access' permissions bits. If NULL, no
> extra
> > * checks required. The access check is performed at runtime, not at
> > diff --git a/target-arm/helper.c b/target-arm/helper.c
> > index a10f459..ab38b68 100644
> > --- a/target-arm/helper.c
> > +++ b/target-arm/helper.c
> > @@ -3296,22 +3296,56 @@ static void add_cpreg_to_hashtable(ARMCPU *cpu,
> const ARMCPRegInfo *r,
> > uint32_t *key = g_new(uint32_t, 1);
> > ARMCPRegInfo *r2 = g_memdup(r, sizeof(ARMCPRegInfo));
> > int is64 = (r->type & ARM_CP_64BIT) ? 1 : 0;
> > - if (r->state == ARM_CP_STATE_BOTH && state == ARM_CP_STATE_AA32) {
> > - /* The AArch32 view of a shared register sees the lower 32 bits
> > - * of a 64 bit backing field. It is not migratable as the
> AArch64
> > - * view handles that. AArch64 also handles reset.
> > - * We assume it is a cp15 register if the .cp field is left
> unset.
> > - */
> > - if (r2->cp == 0) {
> > - r2->cp = 15;
> > +
> > + if (state == ARM_CP_STATE_AA32) {
> > + /* Clear the secure state flags and set based on incoming nsbit
> */
> > + r2->type &= ~(ARM_CP_BANK_S | ARM_CP_BANK_NS);
> > + r2->type |= ARM_CP_BANK_S << nsbit;
> > +
> > + if (r->bank_fieldoffsets[0] && r->bank_fieldoffsets[1]) {
> > + /* Register is banked (using both entries in array).
> > + * Overwriting fieldoffset as the array was only used to
> define
> > + * banked registers but later only fieldoffset is used.
> > + */
> > + r2->fieldoffset = r->bank_fieldoffsets[nsbit];
> > +
> > + /* If V8 is enabled then we don't need to migrate or reset
> the
> > + * AArch32 version of the banked registers as this will be
> handled
> > + * through the AArch64 view.
> > + * The exception to the above is cpregs with a crn of 13
> > + * (specifically FCSEIDR and CONTEXTIDR) in which case
> there may
> > + * not be an AArch64 equivalent for one or either bank so
> migration
> > + * and reset must be preserved.
> > + */
> > + if (arm_feature(&cpu->env, ARM_FEATURE_V8) && r->crn != 13)
> {
> > + r2->type |= ARM_CP_NO_MIGRATE;
> > + r2->resetfn = arm_cp_reset_ignore;
> > + }
> > + } else if (!nsbit) {
> > + /* The register is not banked so we only want to allow
> migration of
> > + * the non-secure instance.
> > + */
> > + r2->type |= ARM_CP_NO_MIGRATE;
> > + r2->resetfn = arm_cp_reset_ignore;
> > }
> > - r2->type |= ARM_CP_NO_MIGRATE;
> > - r2->resetfn = arm_cp_reset_ignore;
> > +
> > + if (r->state == ARM_CP_STATE_BOTH) {
> > + /* The AArch32 view of a shared register sees the lower 32
> bits
> > + * of a 64 bit backing field. It is not migratable as the
> AArch64
> > + * view handles that. AArch64 also handles reset.
> > + * We assume it is a cp15 register if the .cp field is left
> unset.
> > + */
> > + if (r2->cp == 0) {
> > + r2->cp = 15;
> > + }
> > + r2->type |= ARM_CP_NO_MIGRATE;
> > + r2->resetfn = arm_cp_reset_ignore;
> > #ifdef HOST_WORDS_BIGENDIAN
> > - if (r2->fieldoffset) {
> > - r2->fieldoffset += sizeof(uint32_t);
> > - }
> > + if (r2->fieldoffset) {
> > + r2->fieldoffset += sizeof(uint32_t);
> > + }
> > #endif
> > + }
> > }
> > if (state == ARM_CP_STATE_AA64) {
> > /* To allow abbreviation of ARMCPRegInfo
> > --
> > 1.8.3.2
> >
>
>
> thanks
> -- PMM
>
