[Qemu-devel] [PATCH] target/mips: Disable R5900 support

[Qemu-devel] [PATCH] target/mips: Disable R5900 support

[Aleksandar Markovic]

From: Aleksandar Markovic <amarkovic@wavecomp.com>

Disable R5900 support. There are some outstanding issues related
to ABI support and emulation accuracy, that were not understood
well during review process. Disable to avoid backward compatibility
issues.

Signed-off-by: Aleksandar Markovic <aleksandar.markovic@imgtec.com>
---
 target/mips/translate_init.inc.c | 59 ----------------------------------------
 1 file changed, 59 deletions(-)

diff --git a/target/mips/translate_init.inc.c b/target/mips/translate_init.inc.c
index 85da4a2..acab097 100644
--- a/target/mips/translate_init.inc.c
+++ b/target/mips/translate_init.inc.c
@@ -411,65 +411,6 @@ const mips_def_t mips_defs[] =
         .mmu_type = MMU_TYPE_R4000,
     },
     {
-        /*
-         * The Toshiba TX System RISC TX79 Core Architecture manual
-         *
-         * https://wiki.qemu.org/File:C790.pdf
-         *
-         * describes the C790 processor that is a follow-up to the R5900.
-         * There are a few notable differences in that the R5900 FPU
-         *
-         * - is not IEEE 754-1985 compliant,
-         * - does not implement double format, and
-         * - its machine code is nonstandard.
-         */
-        .name = "R5900",
-        .CP0_PRid = 0x00002E00,
-        /* No L2 cache, icache size 32k, dcache size 32k, uncached coherency. */
-        .CP0_Config0 = (0x3 << 9) | (0x3 << 6) | (0x2 << CP0C0_K0),
-        .CP0_Status_rw_bitmask = 0xF4C79C1F,
-#ifdef CONFIG_USER_ONLY
-        /*
-         * R5900 hardware traps to the Linux kernel for IEEE 754-1985 and LL/SC
-         * emulation. For user only, QEMU is the kernel, so we emulate the traps
-         * by simply emulating the instructions directly.
-         *
-         * Note: Config1 is only used internally, the R5900 has only Config0.
-         */
-        .CP0_Config1 = (1 << CP0C1_FP) | (47 << CP0C1_MMU),
-        .CP0_LLAddr_rw_bitmask = 0xFFFFFFFF,
-        .CP0_LLAddr_shift = 4,
-        .CP1_fcr0 = (0x38 << FCR0_PRID) | (0x0 << FCR0_REV),
-        .CP1_fcr31 = 0,
-        .CP1_fcr31_rw_bitmask = 0x0183FFFF,
-#else
-        /*
-         * The R5900 COP1 FPU implements single-precision floating-point
-         * operations but is not entirely IEEE 754-1985 compatible. In
-         * particular,
-         *
-         * - NaN (not a number) and +/- infinities are not supported;
-         * - exception mechanisms are not fully supported;
-         * - denormalized numbers are not supported;
-         * - rounding towards nearest and +/- infinities are not supported;
-         * - computed results usually differs in the least significant bit;
-         * - saturations can differ more than the least significant bit.
-         *
-         * Since only rounding towards zero is supported, the two least
-         * significant bits of FCR31 are hardwired to 01.
-         *
-         * FPU emulation is disabled here until it is implemented.
-         *
-         * Note: Config1 is only used internally, the R5900 has only Config0.
-         */
-        .CP0_Config1 = (47 << CP0C1_MMU),
-#endif /* !CONFIG_USER_ONLY */
-        .SEGBITS = 32,
-        .PABITS = 32,
-        .insn_flags = CPU_R5900 | ASE_MMI,
-        .mmu_type = MMU_TYPE_R4000,
-    },
-    {
         /* A generic CPU supporting MIPS32 Release 6 ISA.
            FIXME: Support IEEE 754-2008 FP.
                   Eventually this should be replaced by a real CPU model. */
-- 
2.7.4

Re: [Qemu-devel] [PATCH] target/mips: Disable R5900 support

[Philippe Mathieu-Daudé]

Hi Aleksandar,

On Tue, Nov 13, 2018 at 8:08 PM Aleksandar Markovic
<aleksandar.markovic@rt-rk.com> wrote:
>
> From: Aleksandar Markovic <amarkovic@wavecomp.com>
>
> Disable R5900 support. There are some outstanding issues related
> to ABI support and emulation accuracy, that were not understood
> well during review process. Disable to avoid backward compatibility
> issues.

Can you add:

This reverts commit ed4f49ba9bb56ebca6987b1083255daf6c89b5de.

Or

Fixes: ed4f49ba9

when applying?

>
> Signed-off-by: Aleksandar Markovic <aleksandar.markovic@imgtec.com>
> ---
>  target/mips/translate_init.inc.c | 59 ----------------------------------------
>  1 file changed, 59 deletions(-)
>
> diff --git a/target/mips/translate_init.inc.c b/target/mips/translate_init.inc.c
> index 85da4a2..acab097 100644
> --- a/target/mips/translate_init.inc.c
> +++ b/target/mips/translate_init.inc.c
> @@ -411,65 +411,6 @@ const mips_def_t mips_defs[] =
>          .mmu_type = MMU_TYPE_R4000,
>      },
>      {
> -        /*
> -         * The Toshiba TX System RISC TX79 Core Architecture manual
> -         *
> -         * https://wiki.qemu.org/File:C790.pdf
> -         *
> -         * describes the C790 processor that is a follow-up to the R5900.
> -         * There are a few notable differences in that the R5900 FPU
> -         *
> -         * - is not IEEE 754-1985 compliant,
> -         * - does not implement double format, and
> -         * - its machine code is nonstandard.
> -         */
> -        .name = "R5900",
> -        .CP0_PRid = 0x00002E00,
> -        /* No L2 cache, icache size 32k, dcache size 32k, uncached coherency. */
> -        .CP0_Config0 = (0x3 << 9) | (0x3 << 6) | (0x2 << CP0C0_K0),
> -        .CP0_Status_rw_bitmask = 0xF4C79C1F,
> -#ifdef CONFIG_USER_ONLY
> -        /*
> -         * R5900 hardware traps to the Linux kernel for IEEE 754-1985 and LL/SC
> -         * emulation. For user only, QEMU is the kernel, so we emulate the traps
> -         * by simply emulating the instructions directly.
> -         *
> -         * Note: Config1 is only used internally, the R5900 has only Config0.
> -         */
> -        .CP0_Config1 = (1 << CP0C1_FP) | (47 << CP0C1_MMU),
> -        .CP0_LLAddr_rw_bitmask = 0xFFFFFFFF,
> -        .CP0_LLAddr_shift = 4,
> -        .CP1_fcr0 = (0x38 << FCR0_PRID) | (0x0 << FCR0_REV),
> -        .CP1_fcr31 = 0,
> -        .CP1_fcr31_rw_bitmask = 0x0183FFFF,
> -#else
> -        /*
> -         * The R5900 COP1 FPU implements single-precision floating-point
> -         * operations but is not entirely IEEE 754-1985 compatible. In
> -         * particular,
> -         *
> -         * - NaN (not a number) and +/- infinities are not supported;
> -         * - exception mechanisms are not fully supported;
> -         * - denormalized numbers are not supported;
> -         * - rounding towards nearest and +/- infinities are not supported;
> -         * - computed results usually differs in the least significant bit;
> -         * - saturations can differ more than the least significant bit.
> -         *
> -         * Since only rounding towards zero is supported, the two least
> -         * significant bits of FCR31 are hardwired to 01.
> -         *
> -         * FPU emulation is disabled here until it is implemented.
> -         *
> -         * Note: Config1 is only used internally, the R5900 has only Config0.
> -         */
> -        .CP0_Config1 = (47 << CP0C1_MMU),
> -#endif /* !CONFIG_USER_ONLY */
> -        .SEGBITS = 32,
> -        .PABITS = 32,
> -        .insn_flags = CPU_R5900 | ASE_MMI,
> -        .mmu_type = MMU_TYPE_R4000,
> -    },
> -    {
>          /* A generic CPU supporting MIPS32 Release 6 ISA.
>             FIXME: Support IEEE 754-2008 FP.
>                    Eventually this should be replaced by a real CPU model. */
> --
> 2.7.4
>
>

Re: [Qemu-devel] [PATCH] target/mips: Disable R5900 support

[Philippe Mathieu-Daudé]

On Tue, Nov 13, 2018 at 8:29 PM Philippe Mathieu-Daudé
<philippe@mathieu-daude.net> wrote:
> On Tue, Nov 13, 2018 at 8:08 PM Aleksandar Markovic
> <aleksandar.markovic@rt-rk.com> wrote:
> >
> > From: Aleksandar Markovic <amarkovic@wavecomp.com>
> >
> > Disable R5900 support. There are some outstanding issues related
> > to ABI support and emulation accuracy, that were not understood
> > well during review process. Disable to avoid backward compatibility
> > issues.

If the issues you mentioned are "the R5900 tcg opcodes are not
implemented correctly", then this patch is OK, because no cpu can use
the R5900 opcodes.

At some point while reading your reviews, I understood the R5900
patches introduced incorrect behaviors for the non-R5900 cpus. In this
case this patch wouldn't suffice.

> Can you add:
>
> This reverts commit ed4f49ba9bb56ebca6987b1083255daf6c89b5de.
>
> Or
>
> Fixes: ed4f49ba9
>
> when applying?

Hoping I misinterpreted your reviews, then this patch is OK.
With one of the suggested comments:
Reviewed-by: Philippe Mathieu-Daudé <f4bug@amsat.org>

Then we will fix this for the 4.0 release.

Regards,

Phil.

> >
> > Signed-off-by: Aleksandar Markovic <aleksandar.markovic@imgtec.com>
> > ---
> >  target/mips/translate_init.inc.c | 59 ----------------------------------------
> >  1 file changed, 59 deletions(-)
> >
> > diff --git a/target/mips/translate_init.inc.c b/target/mips/translate_init.inc.c
> > index 85da4a2..acab097 100644
> > --- a/target/mips/translate_init.inc.c
> > +++ b/target/mips/translate_init.inc.c
> > @@ -411,65 +411,6 @@ const mips_def_t mips_defs[] =
> >          .mmu_type = MMU_TYPE_R4000,
> >      },
> >      {
> > -        /*
> > -         * The Toshiba TX System RISC TX79 Core Architecture manual
> > -         *
> > -         * https://wiki.qemu.org/File:C790.pdf
> > -         *
> > -         * describes the C790 processor that is a follow-up to the R5900.
> > -         * There are a few notable differences in that the R5900 FPU
> > -         *
> > -         * - is not IEEE 754-1985 compliant,
> > -         * - does not implement double format, and
> > -         * - its machine code is nonstandard.
> > -         */
> > -        .name = "R5900",
> > -        .CP0_PRid = 0x00002E00,
> > -        /* No L2 cache, icache size 32k, dcache size 32k, uncached coherency. */
> > -        .CP0_Config0 = (0x3 << 9) | (0x3 << 6) | (0x2 << CP0C0_K0),
> > -        .CP0_Status_rw_bitmask = 0xF4C79C1F,
> > -#ifdef CONFIG_USER_ONLY
> > -        /*
> > -         * R5900 hardware traps to the Linux kernel for IEEE 754-1985 and LL/SC
> > -         * emulation. For user only, QEMU is the kernel, so we emulate the traps
> > -         * by simply emulating the instructions directly.
> > -         *
> > -         * Note: Config1 is only used internally, the R5900 has only Config0.
> > -         */
> > -        .CP0_Config1 = (1 << CP0C1_FP) | (47 << CP0C1_MMU),
> > -        .CP0_LLAddr_rw_bitmask = 0xFFFFFFFF,
> > -        .CP0_LLAddr_shift = 4,
> > -        .CP1_fcr0 = (0x38 << FCR0_PRID) | (0x0 << FCR0_REV),
> > -        .CP1_fcr31 = 0,
> > -        .CP1_fcr31_rw_bitmask = 0x0183FFFF,
> > -#else
> > -        /*
> > -         * The R5900 COP1 FPU implements single-precision floating-point
> > -         * operations but is not entirely IEEE 754-1985 compatible. In
> > -         * particular,
> > -         *
> > -         * - NaN (not a number) and +/- infinities are not supported;
> > -         * - exception mechanisms are not fully supported;
> > -         * - denormalized numbers are not supported;
> > -         * - rounding towards nearest and +/- infinities are not supported;
> > -         * - computed results usually differs in the least significant bit;
> > -         * - saturations can differ more than the least significant bit.
> > -         *
> > -         * Since only rounding towards zero is supported, the two least
> > -         * significant bits of FCR31 are hardwired to 01.
> > -         *
> > -         * FPU emulation is disabled here until it is implemented.
> > -         *
> > -         * Note: Config1 is only used internally, the R5900 has only Config0.
> > -         */
> > -        .CP0_Config1 = (47 << CP0C1_MMU),
> > -#endif /* !CONFIG_USER_ONLY */
> > -        .SEGBITS = 32,
> > -        .PABITS = 32,
> > -        .insn_flags = CPU_R5900 | ASE_MMI,
> > -        .mmu_type = MMU_TYPE_R4000,
> > -    },
> > -    {
> >          /* A generic CPU supporting MIPS32 Release 6 ISA.
> >             FIXME: Support IEEE 754-2008 FP.
> >                    Eventually this should be replaced by a real CPU model. */
> > --
> > 2.7.4
> >
> >

Re: [Qemu-devel] [PATCH] target/mips: Disable R5900 support

[Aleksandar Markovic]

Philippe Mathieu-Daudé <philippe@mathieu-daude.net> wrote:

> Can you add:
> 
> This reverts commit ed4f49ba9bb56ebca6987b1083255daf6c89b5de.
> 
> Or
> 
> Fixes: ed4f49ba9
> 
> when applying?

Sure, I'll add something along that line.

> If the issues you mentioned are "the R5900 tcg opcodes are not
> implemented correctly", then this patch is OK, because no cpu
> can use the R5900 opcodes.
> 
> At some point while reading your reviews, I understood the R5900
> patches introduced incorrect behaviors for the non-R5900 cpus.
> In this case this patch wouldn't suffice.

This patch will be applied after the patches that Fredrik kindly
submitted under "Fix decoding..." series. Fredrik's patches will
remove any doubt about R5900 interfering other architectures, so,
as a consequence, this patch is sufficient and appropriate for
disabling R5900 support.

Additional clarification: This patch is to give more time for
all R5900 issues to be cleared in 3.1+ time frame. If R5900 code
was left enabled, we could have unpleasant backward compatibility
issues between R5900 support and different versions of QEMU.

The R5900 support is intended to be enabled again when all issues
are resolved and agreed upon.

Sincerely,
Aleksandar

Re: [Qemu-devel] [PATCH] target/mips: Disable R5900 support

[Fredrik Noring]

Hi Philippe,

On Tue, Nov 13, 2018 at 11:51:54PM +0100, Philippe Mathieu-Daudé wrote:
> On Tue, Nov 13, 2018 at 8:29 PM Philippe Mathieu-Daudé
> <philippe@mathieu-daude.net> wrote:
> > On Tue, Nov 13, 2018 at 8:08 PM Aleksandar Markovic
> > <aleksandar.markovic@rt-rk.com> wrote:
> > >
> > > From: Aleksandar Markovic <amarkovic@wavecomp.com>
> > >
> > > Disable R5900 support. There are some outstanding issues related
> > > to ABI support and emulation accuracy, that were not understood
> > > well during review process. Disable to avoid backward compatibility
> > > issues.
> 
> If the issues you mentioned are "the R5900 tcg opcodes are not
> implemented correctly", then this patch is OK, because no cpu can use
> the R5900 opcodes.

The issue is that the Linux kernel emulates certain opcodes (e.g. LL, SC,
RDHWR, DMULT, etc.) by psABI requirements. This is a common requirement
for many other MIPS implementations, not only the R5900. It is trivial to
do this with QEMU user mode, but evidently for whatever reason Aleksandar
does not permit this emulation.

> At some point while reading your reviews, I understood the R5900
> patches introduced incorrect behaviors for the non-R5900 cpus. In this
> case this patch wouldn't suffice.

No, that was never the case and I'm not aware of any such problems.
However, there is a refactoring series, and we have observed preexisting
bugs in the MIPS emulation, unrelated to the R5900. The opcode decoder
could also be improved, such as in asserting reserved instructions in
more cases where opcodes are invalid, etc.

> Hoping I misinterpreted your reviews, then this patch is OK.
> With one of the suggested comments:
> Reviewed-by: Philippe Mathieu-Daudé <f4bug@amsat.org>
> 
> Then we will fix this for the 4.0 release.

What exactly needs to be fixed regarding the psABI? The relevant opcodes
would need to stay, and not be prohibited and removed as Aleksandar has
suggested, since such opcode removal breaks the psABI requirements.

Finally, as Maciej explained in some detail, the document that Aleksandar
just recently requested is known to not exist, for any MIPS implementation,
so we are not going to make any progress on that either.

Fredrik

Re: [Qemu-devel] [PATCH] target/mips: Disable R5900 support

[Philippe Mathieu-Daudé]

On Wed, Nov 14, 2018 at 3:02 PM Aleksandar Markovic
<amarkovic@wavecomp.com> wrote:
> Philippe Mathieu-Daudé <philippe@mathieu-daude.net> wrote:
>
> > Can you add:
> >
> > This reverts commit ed4f49ba9bb56ebca6987b1083255daf6c89b5de.
> >
> > Or
> >
> > Fixes: ed4f49ba9
> >
> > when applying?
>
> Sure, I'll add something along that line.
>
> > If the issues you mentioned are "the R5900 tcg opcodes are not
> > implemented correctly", then this patch is OK, because no cpu
> > can use the R5900 opcodes.
> >
> > At some point while reading your reviews, I understood the R5900
> > patches introduced incorrect behaviors for the non-R5900 cpus.
> > In this case this patch wouldn't suffice.
>
> This patch will be applied after the patches that Fredrik kindly
> submitted under "Fix decoding..." series. Fredrik's patches will
> remove any doubt about R5900 interfering other architectures, so,
> as a consequence, this patch is sufficient and appropriate for
> disabling R5900 support.

Ah OK, now it makes sens.

> Additional clarification: This patch is to give more time for
> all R5900 issues to be cleared in 3.1+ time frame. If R5900 code
> was left enabled, we could have unpleasant backward compatibility
> issues between R5900 support and different versions of QEMU.
>
> The R5900 support is intended to be enabled again when all issues
> are resolved and agreed upon.

The commit description is clear enough, this is what I understood from it :)

Regards,

Phil.

Re: [Qemu-devel] [PATCH] target/mips: Disable R5900 support

[Philippe Mathieu-Daudé]

Hi Fredrik,

On Wed, Nov 14, 2018 at 4:31 PM Fredrik Noring <noring@nocrew.org> wrote:
> On Tue, Nov 13, 2018 at 11:51:54PM +0100, Philippe Mathieu-Daudé wrote:
[...]
> > At some point while reading your reviews, I understood the R5900
> > patches introduced incorrect behaviors for the non-R5900 cpus. In this
> > case this patch wouldn't suffice.
>
> No, that was never the case and I'm not aware of any such problems.
> However, there is a refactoring series, and we have observed preexisting
> bugs in the MIPS emulation, unrelated to the R5900. The opcode decoder
> could also be improved, such as in asserting reserved instructions in
> more cases where opcodes are invalid, etc.

Yes, we figure that out with Richard Henderson while playing with the
decodetree script.

> > Hoping I misinterpreted your reviews, then this patch is OK.
> > With one of the suggested comments:
> > Reviewed-by: Philippe Mathieu-Daudé <f4bug@amsat.org>
> >
> > Then we will fix this for the 4.0 release.
>
> What exactly needs to be fixed regarding the psABI? The relevant opcodes
> would need to stay, and not be prohibited and removed as Aleksandar has
> suggested, since such opcode removal breaks the psABI requirements.
>
> Finally, as Maciej explained in some detail, the document that Aleksandar
> just recently requested is known to not exist, for any MIPS implementation,
> so we are not going to make any progress on that either.

I think this was just bad timing with the QEMU release cycles, which
stressed Aleksandar which is the only MIPS maintainer.

Since the R5900 User is a new feature, no downstream distributions
ship it, so it is safer for the community to disable/delay it,
this means we have now 3 months to improve this code, while being more relaxed.

The QEMU community is big, we have different cultures, live in many
timezones, speak different languages, and beside all we are humans
with our daily lives :)
I guess we all sometime stress out, that's why I think, even if
disabling this feature is technically not required (as you seem to
suggest), it is sane to disable it and get back on track together.

I believe Aleksandar is paid to maintain the MIPS TCG/KVM codebase,
and the Malta/Boston boards, all code used by his employer, which is a
huge quantity of C code.
Now the project is community based, and I understand this as it is
open as contributions like yours, or boards not backed by the MIPS
shareholders/company.
The TCG maintenance is shared with Richard, but I think the rest is
too much for a single maintainer.
Somehow the community is failing here, putting too much pressure on Aleksandar.

Let's see how we can improve that during the next merge window,
hopefully the R5900 will be fixed, and eventually his little sister
R3900 will be joining too!

Regards,

Phil.

Re: [Qemu-devel] [PATCH] target/mips: Disable R5900 support

[Aleksandar Markovic]

Fredrik wrote:

> Philippe wrote:
>
> > Then we will fix this for the 4.0 release.
> 
> What exactly needs to be fixed regarding the psABI? The relevant opcodes
> would need to stay, and not be prohibited and removed as Aleksandar has
> suggested, since such opcode removal breaks the psABI requirements.
> 
> Finally, as Maciej explained in some detail, the document that Aleksandar
> just recently requested is known to not exist, for any MIPS implementation,
> so we are not going to make any progress on that either.
>

I am going to list all the issues that needs to be clarified and
resolved, including the reasons why they are issues at all, and
possible ways to resolve them. I can't do it right now, but it'll
happen relatively soon.

Sincerely,
Aleksandar