diff mbox

[1/6] target-ppc: POWER8 supports the MSR_LE bit

Message ID 1395715231-4217-1-git-send-email-anton@samba.org
State New
Headers show

Commit Message

Anton Blanchard March 25, 2014, 2:40 a.m. UTC
Add MSR_LE to the msr_mask for POWER8.

Signed-off-by: Anton Blanchard <anton@samba.org>
Signed-off-by: Cédric Le Goater <clg@fr.ibm.com>
---
 target-ppc/translate_init.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

Comments

Alex Bennée March 25, 2014, 7:29 a.m. UTC | #1
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.

Cheers,
Andreas Färber March 27, 2014, 2:58 p.m. UTC | #2
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?

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.

Cheers,
Andreas
Andreas Färber March 27, 2014, 3:36 p.m. UTC | #3
Hi Anton,

Am 25.03.2014 03:40, schrieb Anton Blanchard:
> Add MSR_LE to the msr_mask for POWER8.
> 
> Signed-off-by: Anton Blanchard <anton@samba.org>
> Signed-off-by: Cédric Le Goater <clg@fr.ibm.com>

Something went wrong here - you're sending patches without placing your
Sob last, which breaks the DCO chain. Cédric indicated to me on IRC to
use Reviewed-by and/or Tested-by for him, fixed.

Please remember to include a cover letter next time.

I've picked up patches 1-4 for 2.0, tweaking two of the commit messages:
https://github.com/afaerber/qemu-cpu/commits/ppc-next

The SPR patches were too big for me to review right now, and it was not
clear to me from commit messages what those patches were fixing (test case).

Thanks,
Andreas
Tom Musta March 27, 2014, 5:10 p.m. UTC | #4
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.
diff mbox

Patch

diff --git a/target-ppc/translate_init.c b/target-ppc/translate_init.c
index 7f53c33..a82c8f9 100644
--- a/target-ppc/translate_init.c
+++ b/target-ppc/translate_init.c
@@ -7175,7 +7175,7 @@  POWERPC_FAMILY(POWER8)(ObjectClass *oc, void *data)
                         PPC2_FP_TST_ISA206 | PPC2_BCTAR_ISA207 |
                         PPC2_LSQ_ISA207 | PPC2_ALTIVEC_207 |
                         PPC2_ISA205 | PPC2_ISA207S;
-    pcc->msr_mask = 0x800000000284FF36ULL;
+    pcc->msr_mask = 0x800000000284FF37ULL;
     pcc->mmu_model = POWERPC_MMU_2_06;
 #if defined(CONFIG_SOFTMMU)
     pcc->handle_mmu_fault = ppc_hash64_handle_mmu_fault;