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