Re: [Qemu-devel] [PATCH v2 09/35] target-arm: A64: Implement MSR (immediate) instructions

[Peter Maydell]

On 5 February 2014 06:23, Peter Crosthwaite
<address@hidden> wrote:
> On Sat, Feb 1, 2014 at 1:45 AM, Peter Maydell <address@hidden> wrote:
>> +    switch (op) {
>> +    case 0x05: /* SPSel */
>> +        env->pstate = deposit32(env->pstate, 0, 1, imm);
>
> "0","1" hardcoded constants are a bit unfriendly. I guess the current
> macro set doesnt define _SHIFT and _WIDTH definitions, should they be
> added?
>
> FWIW, I have this macro in my tree which makes short work of defining
> mask, shift and width constants as a one liner:
>
> /* Define SHIFT, LENGTH and MASK constants for a field within a register */
>
> #define FIELD(reg, field, length, shift) \
> enum { reg ## _ ## field ## _SHIFT = (shift)}; \
> enum { reg ## _ ## field ## _LENGTH = (length)}; \
> enum { reg ## _ ## field ## _MASK = (((1ULL << (length)) - 1) \
>                                           << (shift)) };
>
> Usage would be something like FIELD(PSTATE, SPSEL, 1, 0)

So, I kind of like this, but I'm a bit reluctant to tie up
this patchset in "add a new generic facility to bitops.h",
and current handling for pstate/cpsr has a lot of hardcoded
constants for bit positions. Is it OK if we do this as a
separate cleanup, or do you feel strongly we should bite
the bullet and do it as part of this series?

thanks
-- PMM