Re: [Qemu-devel] [PATCH v2 12/25] target-arm: Update generic cpreg code for AArch64

[Peter Maydell]

On 2 January 2014 01:51, Peter Crosthwaite <address@hidden> wrote:
> On Mon, Dec 23, 2013 at 8:49 AM, Peter Maydell <address@hidden> wrote:
>> +/* Valid values for ARMCPRegInfo state field, indicating which of
>> + * the AArch32 and AArch64 execution states this register is visible in.
>> + * If the reginfo doesn't explicitly specify then it is AArch32 only.
>> + * If the reginfo is declared to be visible in both states then a second
>> + * reginfo is synthesised for the AArch32 view of the AArch64 register,
>> + * such that the AArch32 view is the lower 32 bits of the AArch64 one.
>> + */
>> +#define ARM_CP_STATE_AA32 0
>> +#define ARM_CP_STATE_AA64 1
>> +#define ARM_CP_STATE_BOTH 2
>
> You iterator below depends on this specific encoding ordering, so
> maybe this should be enumified.

Could do. I'm a bit old-school about using #defines rather than enum :-)

>> +
>>  /* Return true if cptype is a valid type field. This is used to try to
>>   * catch errors where the sentinel has been accidentally left off the end
>>   * of a list of registers.
>> @@ -655,6 +698,8 @@ static inline bool cptype_valid(int cptype)
>>   * (ie anything visible in PL2 is visible in S-PL1, some things are only
>>   * visible in S-PL1) but "Secure PL1" is a bit of a mouthful, we bend the
>>   * terminology a little and call this PL3.
>> + * In AArch64 things are somewhat simpler as the PLx bits line up exactly
>> + * with the ELx exception levels.
>>   *
>>   * If access permissions for a register are more complex than can be
>>   * described with these bits, then use a laxer set of restrictions, and
>> @@ -676,6 +721,10 @@ static inline bool cptype_valid(int cptype)
>>
>>  static inline int arm_current_pl(CPUARMState *env)
>>  {
>> +    if (env->aarch64) {
>> +        return extract32(env->pstate, 2, 2);
>> +    }
>> +
>
> This does seem a little out of scope of this patch and more valid as a
> patch in its own right (Adding aarch64 support to arm_current_pl
> globally).

arm_current_pl() is only used by the cpreg code, so making it work
with aarch64 seemed to me to fit with the rest of the "make cpreg code
work with aarch64" changes.

>>      if ((env->uncached_cpsr & 0x1f) == ARM_CPU_MODE_USR) {
>>          return 0;
>>      }
>> @@ -713,12 +762,22 @@ struct ARMCPRegInfo {
>>       * then behave differently on read/write if necessary.
>>       * For 64 bit registers, only crm and opc1 are relevant; crn and opc2
>>       * must both be zero.
>> +     * For AArch64-visible registers, opc0 is also used.
>> +     * Since there are no "coprocessors" in AArch64, cp is purely used as a
>> +     * way to distinguish (for KVM's benefit) guest-visible system registers
>> +     * from demuxed ones provided to preserve the "no side effects on
>> +     * KVM register read/write from QEMU" semantics. cp==0x13 is guest
>> +     * visible (to match KVM's encoding); cp==0 will be converted to
>> +     * cp==0x13 when the ARMCPRegInfo is registered, for convenience.
>>       */
>>      uint8_t cp;
>>      uint8_t crn;
>>      uint8_t crm;
>> +    uint8_t opc0;
>>      uint8_t opc1;
>>      uint8_t opc2;
>> +    /* Execution state in which this register is visible: ARM_CP_STATE_* */
>> +    int state;
>>      /* Register type: ARM_CP_* bits/values */
>>      int type;
>>      /* Access rights: PL*_[RW] */
>> @@ -798,6 +857,11 @@ int arm_cp_write_ignore(CPUARMState *env, const 
>> ARMCPRegInfo *ri,
>>  /* CPReadFn that can be used for read-as-zero behaviour */
>>  int arm_cp_read_zero(CPUARMState *env, const ARMCPRegInfo *ri, uint64_t 
>> *value);
>>
>> +/* CPResetFn that does nothing, for use if no reset is required even
>> + * if fieldoffset is non zero.
>> + */
>> +void arm_cp_reset_ignore(CPUARMState *env, const ARMCPRegInfo *opaque);
>> +
>>  static inline bool cp_access_ok(CPUARMState *env,
>>                                  const ARMCPRegInfo *ri, int isread)
>>  {
>> diff --git a/target-arm/helper.c b/target-arm/helper.c
>> index d833163..3dac694 100644
>> --- a/target-arm/helper.c
>> +++ b/target-arm/helper.c
>> @@ -1938,7 +1938,8 @@ CpuDefinitionInfoList 
>> *arch_query_cpu_definitions(Error **errp)
>>  }
>>
>>  static void add_cpreg_to_hashtable(ARMCPU *cpu, const ARMCPRegInfo *r,
>> -                                   void *opaque, int crm, int opc1, int 
>> opc2)
>> +                                   void *opaque, int state,
>> +                                   int crm, int opc1, int opc2)
>>  {
>>      /* Private utility function for define_one_arm_cp_reg_with_opaque():
>>       * add a single reginfo struct to the hash table.
>> @@ -1946,7 +1947,34 @@ 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;
>> -    *key = ENCODE_CP_REG(r->cp, is64, r->crn, crm, opc1, opc2);
>> +    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.
>> +         */
>> +        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);
>> +        }
>> +#endif
>
> So my thinking is that this logic should be iffed by whether the
> register storage itself is 64-bit, not the valid access modes ...

>> +     * The state field defines whether the register is to be
>> +     * visible in the AArch32 or AArch64 execution state. If the
>> +     * state is set to ARM_CP_STATE_BOTH then we synthesise a
>> +     * reginfo structure for the AArch32 view, which sees the lower
>> +     * 32 bits of the 64 bit register.
>> +     *
>> +     * Only registers visible in AArch64 may set r->opc0; opc0 cannot
>> +     * be wildcarded. AArch64 registers are always considered to be 64
>> +     * bits; the ARM_CP_64BIT* flag applies only to the AArch32 view of
>> +     * the register, if any.
>
> ... as I still don't like that assumption that all 64 visible regs are
> 64bit storage given the ARM ARM does explictly defines many as "32
> bit". My thinking is when future ARM ARMs actually extend stuff into
> that RES0 upper 32 bits, they will have to redefine the regs are
> 64-bit which is then our trigger to convert uint32_t to uin64_t. But
> that is out of scope of this patch, so with the enum fixup:

No, I did check, and "32 bit" is just a shorthand for "64 bits with
RES0 upper half". In particular:
 * there is absolutely no form of 32-bit-only access instruction
 * KVM in the kernel is also defining all system registers as 64 bit

We could probably use a way to specify the RES0 bits in the
reginfo though, so we don't have to make the choice between
lots of writefns and not getting RES0 right. (At the moment for
most registers we just let the guest write 32 bits even if there
are RES0 bits.)

> Reviewed-by: Peter Crosthwaite <address@hidden>

Thanks.
-- PMM