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

[Aurelien Jarno <aurelien@aurel32.net>]

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