Re: [Qemu-devel] [PATCH] MIPS instruction set configuration

[Dirk Behme <dirk.behme@googlemail.com>]

Thiemo Seufer wrote:
> Dirk Behme wrote:
>>- As I understand it, MIPS III is an extension of MIPS II,
>>MIPS IV is an extension of MIPS III etc. Therefore I used
>>definitions for ISAx which include the smaller ones as well.
> 
> Unfortunately it is not that simple. We have the upward-compatible ISAs:

Mmph ;)  What do you think about this: Looking at the recent 
MIPS code, we have a #define MIPS_USES_R4K_EXT and a #define 
MIPS_USES_FPU, nothing else. No splitting of the different 
ISAs and FPU instructions at all. And, looking at your 
explanation, it seems to be really tricky to do this 
splitting. So I'll remove the  MIPS_FEATURE_ISAx definitions 
from the patch (as mentioned, they were unused and only 
example), this will change nothing compared to the existing 
code.

I'd prefer to make changes and improvements in small 
readable pieces without changing too much. One small 
improvement after the other, not a big 'perfect', but 
unreadable, patch. So the splitting of the ISA levels is 
something for an other patch.

The improvement of this patch is to convert the compile time 
switches MIPS_USES_R4K_EXT, MIPS_USES_FPU to compile *and* 
runtime switches and to make machine runtime selectable (and 
introduce MIPS_FEATURE_NEC_xxx).

>>+enum mips_features {
>>+    MIPS_FEATURE_ISA1 = 0x1, /* MIPS I   instruction set architecture */
>>+    MIPS_FEATURE_ISA2 = 0x3, /* MIPS II  instruction set architecture */
>>+    MIPS_FEATURE_ISA3 = 0x7, /* MIPS III instruction set architecture */
>>+    MIPS_FEATURE_ISA4 = 0xF, /* MIPS IV  instruction set architecture */

I'll remove this.

>>+    MIPS_FEATURE_R4K_EXT = 0x10, /* instruction set extension for MIPS R4K */
> 
> 
> Those "extensions" seem to be unimplemented, maybe this was intended to
> cover partial support for MIPS32R2, IOW, for a 4KE.

Sorry, what do you mean here? I only took the existing 
#define MIPS_USES_R4K_EXT.

>>+    MIPS_FEATURE_NEC_EXT = 0x20, /* instruction set extension for NEC CPUs */
> 
> This should name the specific CPU core (vr5400 or vr5500?). NEC built
> a range of MIPS CPUs (e.g. vr41xx, or R12000) with different capabilities.

Yes, I'll change this.

However, on long term, there is a 'keep code simple' vs 
'split instructions technically correct' tradeoff. Adding 
later switches for e.g. vr41xx and R12000 will be okay from 
clean instruction split point of view, but additional 
switches will mess up the code and may be doublicate some 
stuff. While one MIPS_FEATURE_NEC_EXT which contains all NEC 
extensions will be simple, but wrong from instruction splitting.

But I'll convert this to MIPS_FEATURE_NEC_VR5400.

>>+void cpu_mips_set_model(CPUMIPSState *env, uint32_t id)
>>+{
>>+    env->CP0_PRid = id;
>>+    env->features = 0;
>>+    switch (id) {
>>+    case MIPS_R4Kc:
>>+        set_feature(env, MIPS_FEATURE_ISA3);
>>+        set_feature(env, MIPS_FEATURE_R4K_EXT);
>>+        set_feature(env, MIPS_FEATURE_FPU);
>>+        break;
> 
> 
> What's the meaning of "R4Kc" here?
> - R4000: We don't have 64bit (ISA III) support.
> - 4kc: This one has neither ISA III nor FPU.

That's easy: I don't know ;) If you look at existing 
mips-defs.h it is the name of the only hardcoded machine we 
have at the  moment. It enables R4K extensions and FPU. It 
was my goal to change no functionality, only to make first 
step to runtime selection. lI'll remove the ISA option.

Best regards

Dirk