[Qemu-devel] [PATCH v2 5/5] target/mips: Enable only tested modes for R5900

Aleksandar Markovic posted 5 patches 7 years ago
There is a newer version of this series
[Qemu-devel] [PATCH v2 5/5] target/mips: Enable only tested modes for R5900
Posted by Aleksandar Markovic 7 years ago
From: Aleksandar Markovic <amarkovic@wavecomp.com>

Enable MIPS 032 user mode for R5900.

Signed-off-by: Aleksandar Markovic <amarkovic@wavecomp.com>
---
 target/mips/translate_init.inc.c | 4 ++++
 1 file changed, 4 insertions(+)

diff --git a/target/mips/translate_init.inc.c b/target/mips/translate_init.inc.c
index cab2003..d84c58e 100644
--- a/target/mips/translate_init.inc.c
+++ b/target/mips/translate_init.inc.c
@@ -410,6 +410,8 @@ const mips_def_t mips_defs[] =
         .insn_flags = CPU_MIPS32R5 | ASE_MSA,
         .mmu_type = MMU_TYPE_R4000,
     },
+#if defined(CONFIG_USER_ONLY)
+#if !defined(TARGET_MIPS64)
     {
         .name = "R5900",
         .CP0_PRid = 0x00002E00,
@@ -457,6 +459,8 @@ const mips_def_t mips_defs[] =
         .insn_flags = CPU_R5900 | ASE_MMI,
         .mmu_type = MMU_TYPE_R4000,
     },
+#endif
+#endif
     {
         /* A generic CPU supporting MIPS32 Release 6 ISA.
            FIXME: Support IEEE 754-2008 FP.
-- 
2.7.4


Re: [Qemu-devel] [PATCH v2 5/5] target/mips: Enable only tested modes for R5900
Posted by Philippe Mathieu-Daudé 7 years ago
Hi Aleksandar,

On 30/10/18 12:36, Aleksandar Markovic wrote:
> From: Aleksandar Markovic <amarkovic@wavecomp.com>
> 
> Enable MIPS 032 user mode for R5900.
> 
> Signed-off-by: Aleksandar Markovic <amarkovic@wavecomp.com>
> ---
>   target/mips/translate_init.inc.c | 4 ++++
>   1 file changed, 4 insertions(+)
> 
> diff --git a/target/mips/translate_init.inc.c b/target/mips/translate_init.inc.c
> index cab2003..d84c58e 100644
> --- a/target/mips/translate_init.inc.c
> +++ b/target/mips/translate_init.inc.c
> @@ -410,6 +410,8 @@ const mips_def_t mips_defs[] =
>           .insn_flags = CPU_MIPS32R5 | ASE_MSA,
>           .mmu_type = MMU_TYPE_R4000,
>       },
> +#if defined(CONFIG_USER_ONLY)

Can you explain this change in the commit message? I don't understand 
why you want to disable this CPU.

I'm currently running few asm tests with:

$ qemu-system-mips64 -machine mipssim -cpu R5900 -bios tx79_test.bin

(I use mipssim because I don't care about devices, but I'm trying to 
test priviledged instructions).

> +#if !defined(TARGET_MIPS64)

Ditto... Why? This cpu is 64-bit.

>       {
>           .name = "R5900",
>           .CP0_PRid = 0x00002E00,
> @@ -457,6 +459,8 @@ const mips_def_t mips_defs[] =
>           .insn_flags = CPU_R5900 | ASE_MMI,
>           .mmu_type = MMU_TYPE_R4000,
>       },
> +#endif
> +#endif
>       {
>           /* A generic CPU supporting MIPS32 Release 6 ISA.
>              FIXME: Support IEEE 754-2008 FP.
> 

Re: [Qemu-devel] [PATCH v2 5/5] target/mips: Enable only tested modes for R5900
Posted by Aleksandar Markovic 7 years ago
> From: Philippe Mathieu-Daudé <philmd@redhat.com>
>
> > +#if defined(CONFIG_USER_ONLY)
>
> > +#if !defined(TARGET_MIPS64)
> 
> Can you explain this change in the commit message? I don't understand
why you want to disable this CPU.

These limitations are meant to expose to end-user only features that make sense and are appropriately tested. People like you can easily comment out theses lines and do whatever experimentation they like.

About defined(CONFIG_USER_ONLY), it is just because a reasonable testing was not provided for system mode. It is not enough to add a code, and then enable system mode. A reasonable ("acceptance") testing should be done, and made available to others. I know, we do not have strict acceptance rules. But I would say a system image, kernel, and command line + plus some relatively mild testing of system mode should suffice. It would be nice to formalize that.

About !defined(TARGET_MIPS64), this is because O32 is the only supported user-mode ABI for this CPU.

One missing part is documentation update - unfortunately often forgotten.

Aleksandar