[Qemu-ppc] Incorrect handling of more PPC64 insns (PATCH)

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 <aurelien@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 <tg@gmplib.org>
Signed-off-by: Aurelien Jarno <aurelien@aurel32.net>
---
 target-ppc/translate.c |   48 ++++++++++++++++++++++++++++++++----------------
 1 file changed, 32 insertions(+), 16 deletions(-)

On 08.05.2013, at 08:50, Aurelien Jarno wrote:

> 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 <aurelien@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 <tg@gmplib.org>
> Signed-off-by: Aurelien Jarno <aurelien@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);

Can't we handle this through the reserved bits in the instruction map?


Alex

> +        } 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
> aurelien@aurel32.net                 http://www.aurel32.net

Aurelien Jarno <aurelien@aurel32.net> writes:

  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.
  
No.  See my previous post.

The L bit is to be ignored for 32-bit CPUs, just like the original code
did.

On 08.05.2013, at 11:20, Torbjorn Granlund wrote:

> Aurelien Jarno <aurelien@aurel32.net> writes:
> 
>  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.
> 
> No.  See my previous post.
> 
> The L bit is to be ignored for 32-bit CPUs, just like the original code
> did.

I see. So if the target is 64bit capable, then we distinguish by the instruction bit, but for 32bit targets we always call the 32bit variant regardless of the bit?


Alex

On 08.05.2013, at 11:32, Alexander Graf wrote:

> 
> On 08.05.2013, at 11:20, Torbjorn Granlund wrote:
> 
>> Aurelien Jarno <aurelien@aurel32.net> writes:
>> 
>> 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.
>> 
>> No.  See my previous post.
>> 
>> The L bit is to be ignored for 32-bit CPUs, just like the original code
>> did.
> 
> I see. So if the target is 64bit capable, then we distinguish by the instruction bit, but for 32bit targets we always call the 32bit variant regardless of the bit?

Ok, so the real problem here is that NARROW_MODE is not set, but is used to differentiate whether to use the 32bit cmp only or not.

Richard, there are 2 ways out of this:

  1) get rid of NARROW_MODE and always check ctx->sf
  2) add a new 32bit only insns flag and create separate functions for 32bit cmp calls

I have a patch set ready for 2, but I think 1 would be the better alternative. The only cases I could spot where things could break is in the add/sub corner case. Let me try to cook up something.


Alex

Alexander Graf <agraf@suse.de> writes:

  Ok, so the real problem here is that NARROW_MODE is not set, but is
  used to differentiate whether to use the 32bit cmp only or not.
  
Eh?

  Richard, there are 2 ways out of this:
  
    1) get rid of NARROW_MODE and always check ctx->sf

No!

The cmp insn with L set should NOT be affected by SF.  That's the entire
point of my change.

I reviewed the other uses of NARROW_MODE and didn't spot any errors.
(But I must confess that I would need to red the PPC manuals better inn
order to tell for sure.)

    2) add a new 32bit only insns flag and create separate functions for 32bit cmp calls
  
Aurelien's patch looked promising, if one removes the exception casting.

On 08.05.2013, at 12:07, Torbjorn Granlund wrote:

> Alexander Graf <agraf@suse.de> writes:
> 
>  Ok, so the real problem here is that NARROW_MODE is not set, but is
>  used to differentiate whether to use the 32bit cmp only or not.
> 
> Eh?
> 
>  Richard, there are 2 ways out of this:
> 
>    1) get rid of NARROW_MODE and always check ctx->sf
> 
> No!
> 
> The cmp insn with L set should NOT be affected by SF.  That's the entire
> point of my change.

You're right. I got confused there :).

> 
> I reviewed the other uses of NARROW_MODE and didn't spot any errors.
> (But I must confess that I would need to red the PPC manuals better inn
> order to tell for sure.)
> 
>    2) add a new 32bit only insns flag and create separate functions for 32bit cmp calls
> 
> Aurelien's patch looked promising, if one removes the exception casting.

His exception casting is actually correct. You can use qemu-(system-)ppc64, but run it with a CPU definition that is 32bit only, like a G3. These old CPUs did not know the instruction with L yet, so they do throw an illegal instruction exception, which we have to model.


Alex