Re: [Qemu-devel] [PATCH] target-mips: fix decoding of microMIPS POOL32Axf instructions
On Mon, 5 Aug 2013, Leon Alrae wrote: Just to make sure that we are refering to the same thing :) For MFHI: page no. 303 in MIPS Architecture for Programmers Volume II-B: The microMIPS32 Instruction Set (document MD00764) page no. 140 in MIPS Architecture for Programmers Volume IV-e: The MIPS DSP Module for the microMIPS32 Architecture (document MD00582) Where instruction encoding is concerned please remember to always refer to opcode tables rather than individual instruction description pages in case there is a documentation erratum. If that happens, then opcode tables take precedence as they are generated automatically from actual architecture descriptions. We had such a case already actually that was spotted late enough that it was architecture that had to be modified to match software expectations (QEMU included). So in this case it is: Table 6.5 POOL32Axf Encoding of Minor Opcode Extension Field, p. 498, MIPS Architecture for Programmers, Volume II-B: The microMIPS32 Instruction Set, Document Number: MD00582, Revision 5.01, December 16, 2012 and: Table 5.5 POOL32Axf Encoding of Minor Opcode Extension Field, p. 66, MIPS Architecture for Programmers, VolumeIV-e: The MIPSR DSP Module for the microMIPS32 Architecture, Document Number: MD00764, Revision 2.40, December 16, 2012 that you should be referring to (note that you've got the document numbers reversed too ;) ). I agree it looks weird and wasteful to have two separate encodings for instructions that operate on the HI[0] and LO[0] registers, but I suppose it makes the wiring of the DSP Disabled exception easier. Maciej
Re: [Qemu-devel] [PATCH] target-mips: fix decoding of microMIPS POOL32Axf instructions
On 03/08/13 23:01, Aurelien Jarno wrote: On Thu, Aug 01, 2013 at 11:02:27AM +0100, Leon Alrae wrote: These are not DSP instructions, thus there is no ac field. For more details please refer to instruction encoding of MULT, MULTU, MADD, MADDU, MSUB, MSUBU, MFHI, MFLO, MTHI, MTLO in MIPS Architecture for Programmers Volume II-B: The microMIPS32 Instruction Set These instructions have both a non DSP version without the ac field, and a DSP version with the ac field. The encoding of the ac field is done in bits 14 and 15, so the current QEMU code is correct. Please refer to the MIPS32® Architecture Manual Volume IV-e: The MIPS® DSP Application-Specific Extension to the microMIPS32® Architecture (document MD00764). Please note that the patch modifies non-DSP version of listed instructions, where ac field doesn't exist. In this case reading bits 14 and 15 and passing it as acc argument to gen_muldiv() or gen_HILO() is not correct. If those bits are not 0 (and for most of the listed instructions this is true) then check_dsp() is called which will cause an exception if DSP is not supported / disabled.
Re: [Qemu-devel] [PATCH] target-mips: fix decoding of microMIPS POOL32Axf instructions
On Mon, Aug 05, 2013 at 08:41:52AM +0100, Leon Alrae wrote: On 03/08/13 23:01, Aurelien Jarno wrote: On Thu, Aug 01, 2013 at 11:02:27AM +0100, Leon Alrae wrote: These are not DSP instructions, thus there is no ac field. For more details please refer to instruction encoding of MULT, MULTU, MADD, MADDU, MSUB, MSUBU, MFHI, MFLO, MTHI, MTLO in MIPS Architecture for Programmers Volume II-B: The microMIPS32 Instruction Set These instructions have both a non DSP version without the ac field, and a DSP version with the ac field. The encoding of the ac field is done in bits 14 and 15, so the current QEMU code is correct. Please refer to the MIPS32® Architecture Manual Volume IV-e: The MIPS® DSP Application-Specific Extension to the microMIPS32® Architecture (document MD00764). Please note that the patch modifies non-DSP version of listed instructions, where ac field doesn't exist. In this case reading bits This is correct, except for MFHI, MFLO, MTHI, MTLO which share the same opcode for the DSP and non-DSP version. Theses instructions have bits 14 and 15 set to 0 for the non-DSP version, so they are working as expected and thus your patch should not modify them. 14 and 15 and passing it as acc argument to gen_muldiv() or gen_HILO() is not correct. If those bits are not 0 (and for most of the listed instructions this is true) then check_dsp() is called which will cause an exception if DSP is not supported / disabled. This is correct. The current code assumes that all instructions using gen_muldiv() have the same opcode for non-DSP and DSP version (actually it's weird that they have a different encoding, it's just wasting some opcode space). Therefore what should be done: - The part concerning MFHI, MFLO, MTHI, MTLO should be dropped from your patch as it already works correctly. - The part of your patch concerning instructions calling gen_muldiv() is correct and should be kept to handle the non-DSP version of these instructions. - An entry with for the DSP version of these instructions should be created, calling gen_muldiv() passing the ac field from bits 14 and 15. This is basically the same code than now, just with extension 0x32 instead of 0x2c. -- Aurelien Jarno GPG: 1024D/F1BCDB73 aurel...@aurel32.net http://www.aurel32.net
Re: [Qemu-devel] [PATCH] target-mips: fix decoding of microMIPS POOL32Axf instructions
On 05/08/13 11:50, Aurelien Jarno wrote: On Mon, Aug 05, 2013 at 08:41:52AM +0100, Leon Alrae wrote: On 03/08/13 23:01, Aurelien Jarno wrote: On Thu, Aug 01, 2013 at 11:02:27AM +0100, Leon Alrae wrote: These are not DSP instructions, thus there is no ac field. For more details please refer to instruction encoding of MULT, MULTU, MADD, MADDU, MSUB, MSUBU, MFHI, MFLO, MTHI, MTLO in MIPS Architecture for Programmers Volume II-B: The microMIPS32 Instruction Set These instructions have both a non DSP version without the ac field, and a DSP version with the ac field. The encoding of the ac field is done in bits 14 and 15, so the current QEMU code is correct. Please refer to the MIPS32® Architecture Manual Volume IV-e: The MIPS® DSP Application-Specific Extension to the microMIPS32® Architecture (document MD00764). Please note that the patch modifies non-DSP version of listed instructions, where ac field doesn't exist. In this case reading bits This is correct, except for MFHI, MFLO, MTHI, MTLO which share the same opcode for the DSP and non-DSP version. Theses instructions have bits 14 and 15 set to 0 for the non-DSP version, so they are working as expected and thus your patch should not modify them. As far as microMIPS is concerned - MFHI, MFLO, MTHI, MTLO don't seem to share the same opcode for the DSP and non-DSP version. It is true that non-DSP version has bits 14 and 15 set to zero. However, according to already mentioned documents, bits 11..6 (extension) are different in the DSP (set to 0x35) and non-DSP version (set to 0x1). Therefore, we shouldn't read bits 14 and 15 in case of non-DSP version, and we should have a separate entry for DSP version. Just to make sure that we are refering to the same thing :) For MFHI: page no. 303 in MIPS Architecture for Programmers Volume II-B: The microMIPS32 Instruction Set (document MD00764) page no. 140 in MIPS Architecture for Programmers Volume IV-e: The MIPS DSP Module for the microMIPS32 Architecture (document MD00582) 14 and 15 and passing it as acc argument to gen_muldiv() or gen_HILO() is not correct. If those bits are not 0 (and for most of the listed instructions this is true) then check_dsp() is called which will cause an exception if DSP is not supported / disabled. This is correct. The current code assumes that all instructions using gen_muldiv() have the same opcode for non-DSP and DSP version (actually it's weird that they have a different encoding, it's just wasting some opcode space). Therefore what should be done: - The part concerning MFHI, MFLO, MTHI, MTLO should be dropped from your patch as it already works correctly. - The part of your patch concerning instructions calling gen_muldiv() is correct and should be kept to handle the non-DSP version of these instructions. - An entry with for the DSP version of these instructions should be created, calling gen_muldiv() passing the ac field from bits 14 and 15. This is basically the same code than now, just with extension 0x32 instead of 0x2c. OK - I will update the patch, but I would leave the part concerning MFHI, MFLO, MTHI, MTLO as it seems to be correct, but the new entry will be added for DSP version. Regards, Leon
Re: [Qemu-devel] [PATCH] target-mips: fix decoding of microMIPS POOL32Axf instructions
On Thu, Aug 01, 2013 at 11:02:27AM +0100, Leon Alrae wrote: These are not DSP instructions, thus there is no ac field. For more details please refer to instruction encoding of MULT, MULTU, MADD, MADDU, MSUB, MSUBU, MFHI, MFLO, MTHI, MTLO in MIPS Architecture for Programmers Volume II-B: The microMIPS32 Instruction Set These instructions have both a non DSP version without the ac field, and a DSP version with the ac field. The encoding of the ac field is done in bits 14 and 15, so the current QEMU code is correct. Please refer to the MIPS32® Architecture Manual Volume IV-e: The MIPS® DSP Application-Specific Extension to the microMIPS32® Architecture (document MD00764). -- Aurelien Jarno GPG: 1024D/F1BCDB73 aurel...@aurel32.net http://www.aurel32.net
[Qemu-devel] [PATCH] target-mips: fix decoding of microMIPS POOL32Axf instructions
These are not DSP instructions, thus there is no ac field.
For more details please refer to instruction encoding of
MULT, MULTU, MADD, MADDU, MSUB, MSUBU, MFHI, MFLO, MTHI, MTLO in
MIPS Architecture for Programmers Volume II-B: The microMIPS32 Instruction Set
Signed-off-by: Leon Alrae leon.al...@imgtec.com
---
target-mips/translate.c | 10 +-
1 files changed, 5 insertions(+), 5 deletions(-)
diff --git a/target-mips/translate.c b/target-mips/translate.c
index c1d57a7..7451423 100644
--- a/target-mips/translate.c
+++ b/target-mips/translate.c
@@ -3,7 +3,7 @@ static void gen_pool32axf (CPUMIPSState *env,
DisasContext *ctx, int rt, int rs)
mips32_op = OPC_MSUBU;
do_mul:
check_insn(ctx, ISA_MIPS32);
-gen_muldiv(ctx, mips32_op, (ctx-opcode 14) 3, rs, rt);
+gen_muldiv(ctx, mips32_op, 0, rs, rt);
break;
default:
goto pool32axf_invalid;
@@ -11250,16 +11250,16 @@ static void gen_pool32axf (CPUMIPSState *env,
DisasContext *ctx, int rt, int rs)
case 0x35:
switch (minor 3) {
case MFHI32:
-gen_HILO(ctx, OPC_MFHI, minor 2, rs);
+gen_HILO(ctx, OPC_MFHI, 0, rs);
break;
case MFLO32:
-gen_HILO(ctx, OPC_MFLO, minor 2, rs);
+gen_HILO(ctx, OPC_MFLO, 0, rs);
break;
case MTHI32:
-gen_HILO(ctx, OPC_MTHI, minor 2, rs);
+gen_HILO(ctx, OPC_MTHI, 0, rs);
break;
case MTLO32:
-gen_HILO(ctx, OPC_MTLO, minor 2, rs);
+gen_HILO(ctx, OPC_MTLO, 0, rs);
break;
default:
goto pool32axf_invalid;
--
1.7.5.4
