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 <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);
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
> aurel...@aurel32.net http://www.aurel32.net
