On Tue, May 07, 2013 at 09:30:24PM +0200, Torbjorn Granlund wrote:
> I realised a possible problem with my suggested patch.
>
> What about a 32-bit processor? Then NARROW_MODE macro is identical 0.
>
> The pre-patch behaviour was then to ignore the L bit and decode both
> 32-bit and 64-bit instruction in the same way.
>
> Apparently that is correct behaviour. (The manual is slightly vague,
> but I let hardware decide.)
>
> With my patch, the bit is not ignored, and invalid code will be
> generated for 32-bit targets, if they'd set the L bit.
>
> Here is an uglier but hopefully completely correct patch.
>
> diff --git a/target-ppc/translate.c b/target-ppc/translate.c
> index 1a84653..69d684c 100644
> --- a/target-ppc/translate.c
> +++ b/target-ppc/translate.c
> @@ -675,49 +675,65 @@ static inline void gen_set_Rc0(DisasContext *ctx, TCGv
> reg)
> /* cmp */
> static void gen_cmp(DisasContext *ctx)
> {
> - if (NARROW_MODE(ctx) || !(ctx->opcode & 0x00200000)) {
> +#if defined(TARGET_PPC64)
> + if (!(ctx->opcode & 0x00200000)) {
> +#endif
> gen_op_cmp32(cpu_gpr[rA(ctx->opcode)], cpu_gpr[rB(ctx->opcode)],
> 1, crfD(ctx->opcode));
> +#if defined(TARGET_PPC64)
> } else {
> gen_op_cmp(cpu_gpr[rA(ctx->opcode)], cpu_gpr[rB(ctx->opcode)],
> 1, crfD(ctx->opcode));
> }
> +#endif
> }
I agree that there is a bug there, and that cmp32 should be used with
when L=0. That said your code is not going to generate and invalid code
on a 32-bit CPU with L=1, but instead just skip the instruction.
Moreover as Alexander pointed, TARGET_PPC64 doesn't mean it's a 64-bit
CPU, but that the resulting qemu binaries support 64-bit CPU.
What about the following patch (only lightly tested).
From: Aurelien Jarno <aurel...@aurel32.net>
target-ppc: fix cmp instructions on 64-bit CPUs
64-bit CPUs check for the L bit of comparison instruction to determine
if the instruction is 32-bit wide, and not to the MSR SF bit.
L=1 on a 32-bit CPU should generate an invalid instruction exception.
Reported-by: Torbjorn Granlund <t...@gmplib.org>
Signed-off-by: Aurelien Jarno <aurel...@aurel32.net>
---
target-ppc/translate.c | 48 ++++++++++++++++++++++++++++++++----------------
1 file changed, 32 insertions(+), 16 deletions(-)
diff --git a/target-ppc/translate.c b/target-ppc/translate.c
index 0886f4d..ab41dc1 100644
--- a/target-ppc/translate.c
+++ b/target-ppc/translate.c
@@ -675,48 +675,64 @@ static inline void gen_set_Rc0(DisasContext *ctx, TCGv
reg)
/* cmp */
static void gen_cmp(DisasContext *ctx)
{
- if (NARROW_MODE(ctx) || !(ctx->opcode & 0x00200000)) {
+ if (ctx->opcode & 0x00200000) {
+ if (unlikely(!(ctx->insns_flags & PPC_64B))) {
+ gen_inval_exception(ctx, POWERPC_EXCP_INVAL_INVAL);
+ } else {
+ gen_op_cmp(cpu_gpr[rA(ctx->opcode)], cpu_gpr[rB(ctx->opcode)],
+ 1, crfD(ctx->opcode));
+ }
+ } else {
gen_op_cmp32(cpu_gpr[rA(ctx->opcode)], cpu_gpr[rB(ctx->opcode)],
1, crfD(ctx->opcode));
- } else {
- gen_op_cmp(cpu_gpr[rA(ctx->opcode)], cpu_gpr[rB(ctx->opcode)],
- 1, crfD(ctx->opcode));
}
}
/* cmpi */
static void gen_cmpi(DisasContext *ctx)
{
- if (NARROW_MODE(ctx) || !(ctx->opcode & 0x00200000)) {
+ if (ctx->opcode & 0x00200000) {
+ if (unlikely(!(ctx->insns_flags & PPC_64B))) {
+ gen_inval_exception(ctx, POWERPC_EXCP_INVAL_INVAL);
+ } else {
+ gen_op_cmpi(cpu_gpr[rA(ctx->opcode)], SIMM(ctx->opcode),
+ 1, crfD(ctx->opcode));
+ }
+ } else {
gen_op_cmpi32(cpu_gpr[rA(ctx->opcode)], SIMM(ctx->opcode),
1, crfD(ctx->opcode));
- } else {
- gen_op_cmpi(cpu_gpr[rA(ctx->opcode)], SIMM(ctx->opcode),
- 1, crfD(ctx->opcode));
}
}
/* cmpl */
static void gen_cmpl(DisasContext *ctx)
{
- if (NARROW_MODE(ctx) || !(ctx->opcode & 0x00200000)) {
+ if (ctx->opcode & 0x00200000) {
+ if (unlikely(!(ctx->insns_flags & PPC_64B))) {
+ gen_inval_exception(ctx, POWERPC_EXCP_INVAL_INVAL);
+ } else {
+ gen_op_cmp(cpu_gpr[rA(ctx->opcode)], cpu_gpr[rB(ctx->opcode)],
+ 0, crfD(ctx->opcode));
+ }
+ } else {
gen_op_cmp32(cpu_gpr[rA(ctx->opcode)], cpu_gpr[rB(ctx->opcode)],
0, crfD(ctx->opcode));
- } else {
- gen_op_cmp(cpu_gpr[rA(ctx->opcode)], cpu_gpr[rB(ctx->opcode)],
- 0, crfD(ctx->opcode));
}
}
/* cmpli */
static void gen_cmpli(DisasContext *ctx)
{
- if (NARROW_MODE(ctx) || !(ctx->opcode & 0x00200000)) {
+ if (ctx->opcode & 0x00200000) {
+ if (unlikely(!(ctx->insns_flags & PPC_64B))) {
+ gen_inval_exception(ctx, POWERPC_EXCP_INVAL_INVAL);
+ } else {
+ gen_op_cmpi(cpu_gpr[rA(ctx->opcode)], UIMM(ctx->opcode),
+ 0, crfD(ctx->opcode));
+ }
+ } else {
gen_op_cmpi32(cpu_gpr[rA(ctx->opcode)], UIMM(ctx->opcode),
0, crfD(ctx->opcode));
- } else {
- gen_op_cmpi(cpu_gpr[rA(ctx->opcode)], UIMM(ctx->opcode),
- 0, crfD(ctx->opcode));
}
}
--
1.7.10.4
--
Aurelien Jarno GPG: 1024D/F1BCDB73
aurel...@aurel32.net http://www.aurel32.net
