[Qemu-devel] [PATCH] target-mips: fix decoding of microMIPS POOL32Axf instructions

[Qemu-devel] [PATCH] target-mips: fix decoding of microMIPS POOL32Axf instructions

[Leon Alrae]

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.alrae@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
@@ -11113,7 +11113,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

Re: [Qemu-devel] [PATCH] target-mips: fix decoding of microMIPS POOL32Axf instructions

[Aurelien Jarno]

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

Re: [Qemu-devel] [PATCH] target-mips: fix decoding of microMIPS POOL32Axf instructions

[Leon Alrae]

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

[Aurelien Jarno]

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

Re: [Qemu-devel] [PATCH] target-mips: fix decoding of microMIPS POOL32Axf instructions

[Leon Alrae]

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

[Maciej W. Rozycki]

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