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

[Peter Crosthwaite]

On Sat, Feb 15, 2014 at 2:41 AM, Peter Maydell <address@hidden> wrote:
> 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?
>

No I think its follow up.

Regards,
Peter

> thanks
> -- PMM
>