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

Aleksandar Markovic posted 5 patches 7 years ago
[Qemu-devel] [PATCH v3 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.

Expose to end-user only features that make sense and are appropriately
tested. In this case, enable only MIPS 032 user mode for R5900.

About defined(CONFIG_USER_ONLY), it is just because a reasonable
testing was not provided for system mode. Some reasonable
("acceptance") testing should be done, and made available
to others. A system image, kernel, and command line + plus
some relatively mild testing of system mode should suffice.

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

Reviewed-by: Stefan Markovic <smarkovic@wavecomp.com>
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 v3 5/5] target/mips: Enable only tested modes for R5900
Posted by Philippe Mathieu-Daudé 7 years ago
Hi Aleksandar,

On 30/10/18 16:44, Aleksandar Markovic wrote:
> From: Aleksandar Markovic <amarkovic@wavecomp.com>
> 
> Enable MIPS 032 user mode for R5900.
> 
> Expose to end-user only features that make sense and are appropriately
> tested. In this case, enable only MIPS 032 user mode for R5900.
> 
> About defined(CONFIG_USER_ONLY), it is just because a reasonable
> testing was not provided for system mode. Some reasonable
> ("acceptance") testing should be done, and made available
> to others. A system image, kernel, and command line + plus
> some relatively mild testing of system mode should suffice.
> 
> About !defined(TARGET_MIPS64), this is because O32 is the only
> supported user-mode ABI for this CPU.
> 
> Reviewed-by: Stefan Markovic <smarkovic@wavecomp.com>
> 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,
>       },

Do you mind adding those one line comment when applying this series?
(no need to respin IMHO):

> +#if defined(CONFIG_USER_ONLY)

   #if defined(CONFIG_USER_ONLY) /* System mode not implemented */

> +#if !defined(TARGET_MIPS64)

   #if !defined(TARGET_MIPS64) /* Only O32 user-mode ABI is supported */

>       {
>           .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

I'm not super happy with this patch but as you said it is reasonable, so 
I accept it (Thanks for the comments btw).

Reviewed-by: Philippe Mathieu-Daudé <philmd@redhat.com>

>       {
>           /* A generic CPU supporting MIPS32 Release 6 ISA.
>              FIXME: Support IEEE 754-2008 FP.
> 

Re: [Qemu-devel] [PATCH v3 5/5] target/mips: Enable only tested modes for R5900
Posted by Aleksandar Markovic 7 years ago
> Do you mind adding those one line comment when applying this series?
> 
> > +#if defined(CONFIG_USER_ONLY)
> 
>    #if defined(CONFIG_USER_ONLY) /* System mode not implemented */
> 
> > +#if !defined(TARGET_MIPS64)
> 
>    #if !defined(TARGET_MIPS64) /* Only O32 user-mode ABI is supported */

I do think this is a very good idea, but it was too late - I was already well into preparing pull request when I read this.

Thanks,
Aleksandar