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
