Re: [Qemu-devel] [PATCH 1/6] target-ppc: POWER8 supports the MSR_LE bit

[Tom Musta <tommusta@gmail.com>]

On 3/27/2014 9:58 AM, Andreas Färber wrote:
> Am 25.03.2014 08:29, schrieb Alex Bennée:
>>
>> Anton Blanchard <anton@samba.org> writes:
>>
>>> Add MSR_LE to the msr_mask for POWER8.
>>>
>> <snip>
>>> -    pcc->msr_mask = 0x800000000284FF36ULL;
>>> +    pcc->msr_mask = 0x800000000284FF37ULL;
>> <snip>
>>
>> Should we be adding some #define's for the various bit positions on this
>> mask? Looking at the current code it looks like a big ream of magic
>> numbers.
> 
> In general I concur that defines would be nice, however for 2.0 that's
> too risky for me as temporary maintainer and I'm not sure if these
> values are being pieced together by contributors or whether this is
> coming directly from the manual?

There are defines in target-ppc/cpu.h that identify the bit position.  So
something like this could/should be done (in 2.1):

    pcc->msr_mask =
        (1ULL << MSR_SF) |
        (1ULL << MSR_TAG) |
        (1ULL << MSR_ISF) |
        (1ULL << MSR_DR /* MISSING */ ) |
        (1ULL << MSR_IR /* MISSING */ ) |
        (1ULL << MSR_FE1 /* MISSING */ ) |
        (1ULL << MSR_BE /* MISSING */ ) |
        (1ULL << MSR_ME /* MISSING */ ) |
        (1ULL << MSR_SE /* MISSING */ ) |
        (1ULL << MSR_FE0 /* MISSING */ ) |
        (1ULL << MSR_FP /* MISSING */ ) |
        (1ULL << MSR_PR /* MISSING */ ) |
        (1ULL << MSR_EE /* MISSING */ );

The set of defines is incomplete -- the items marked MISSING are not currently there.
They could, of course, easily be added.

Unfortunately, this behavior has been repeated over 50 times in the target-ppc/translate_init.c
file and some MSR bit positions have multiple meanings based on processor family and era.  So
a thorough and accurate cleanup would provide a nice challenge :)

I'm willing to tackle this after I get done with some decimal floating point work (probably 2
weeks).

> The other issue has been that adding a new family, even after the
> initial round of cleanups, still requires a chunk of code to be copied,
> which seems prone to forgetting little bits on the new one, then maybe
> fixing up the original template but not the derived models, etc.