[Qemu-devel] [PATCH 1/2] target-mips/translate.c: Free TCG in OPC_DINSV

[Qemu-devel] [PATCH 1/2] target-mips/translate.c: Free TCG in OPC_DINSV

[Dongxue Zhang]

Free t0 and t1 in opcode OPC_DINSV.

Signed-off-by: Dongxue Zhang <elta.era@gmail.com>
---
 target-mips/translate.c | 3 +++
 1 file changed, 3 insertions(+)

diff --git a/target-mips/translate.c b/target-mips/translate.c
index d7b8c4d..c381366 100644
--- a/target-mips/translate.c
+++ b/target-mips/translate.c
@@ -15300,6 +15300,9 @@ static void decode_opc (CPUMIPSState *env, DisasContext *ctx)
                     gen_load_gpr(t1, rs);
 
                     gen_helper_dinsv(cpu_gpr[rt], cpu_env, t1, t0);
+
+                    tcg_temp_free(t0);
+                    tcg_temp_free(t1);
                     break;
                 }
             default:            /* Invalid */
-- 
1.8.1.2

[Qemu-devel] [PATCH 2/2] target-mips/translate.c: Add judgement for msb and lsb

[Dongxue Zhang]

Use 'if' to make sure the real msb greater than the lsb. As the compiler may
not do this.

Signed-off-by: Dongxue Zhang <elta.era@gmail.com>
---
 target-mips/translate.c | 9 +++++++++
 1 file changed, 9 insertions(+)

diff --git a/target-mips/translate.c b/target-mips/translate.c
index c381366..e2cce31 100644
--- a/target-mips/translate.c
+++ b/target-mips/translate.c
@@ -3946,14 +3946,23 @@ static void gen_bitops (DisasContext *ctx, uint32_t opc, int rt,
         break;
 #if defined(TARGET_MIPS64)
     case OPC_DINSM:
+        if (lsb > (msb + 32)) {
+            goto fail;
+        }
         gen_load_gpr(t0, rt);
         tcg_gen_deposit_tl(t0, t0, t1, lsb, msb + 32 - lsb + 1);
         break;
     case OPC_DINSU:
+        if (lsb > msb) {
+            goto fail;
+        }
         gen_load_gpr(t0, rt);
         tcg_gen_deposit_tl(t0, t0, t1, lsb + 32, msb - lsb + 1);
         break;
     case OPC_DINS:
+        if (lsb > msb) {
+            goto fail;
+        }
         gen_load_gpr(t0, rt);
         tcg_gen_deposit_tl(t0, t0, t1, lsb, msb - lsb + 1);
         break;
-- 
1.8.1.2

Re: [Qemu-devel] [PATCH 2/2] target-mips/translate.c: Add judgement for msb and lsb

[Aurelien Jarno]

On Mon, Jul 28, 2014 at 11:58:22PM +0800, Dongxue Zhang wrote:
> Use 'if' to make sure the real msb greater than the lsb. As the compiler may
> not do this.

What are you trying to fix exactly? These cases are defined as
"unpredictable" in the MIPS ISA manual, which is what is implemented in
QEMU. In addition on the MIPS64R2 implementations I tested (Cavium
Octeon, Loongson 3) these cases do not trigger a reserved
instruction exception.

> Signed-off-by: Dongxue Zhang <elta.era@gmail.com>
> ---
>  target-mips/translate.c | 9 +++++++++
>  1 file changed, 9 insertions(+)
> 
> diff --git a/target-mips/translate.c b/target-mips/translate.c
> index c381366..e2cce31 100644
> --- a/target-mips/translate.c
> +++ b/target-mips/translate.c
> @@ -3946,14 +3946,23 @@ static void gen_bitops (DisasContext *ctx, uint32_t opc, int rt,
>          break;
>  #if defined(TARGET_MIPS64)
>      case OPC_DINSM:
> +        if (lsb > (msb + 32)) {
> +            goto fail;
> +        }

This test is always false, as lsb and msb are 5 bits values.

>          gen_load_gpr(t0, rt);
>          tcg_gen_deposit_tl(t0, t0, t1, lsb, msb + 32 - lsb + 1);
>          break;
>      case OPC_DINSU:
> +        if (lsb > msb) {
> +            goto fail;
> +        }
>          gen_load_gpr(t0, rt);
>          tcg_gen_deposit_tl(t0, t0, t1, lsb + 32, msb - lsb + 1);
>          break;
>      case OPC_DINS:
> +        if (lsb > msb) {
> +            goto fail;
> +        }
>          gen_load_gpr(t0, rt);
>          tcg_gen_deposit_tl(t0, t0, t1, lsb, msb - lsb + 1);
>          break;
> -- 
> 1.8.1.2
> 
> 
> 

-- 
Aurelien Jarno                          GPG: 4096R/1DDD8C9B
aurelien@aurel32.net                 http://www.aurel32.net

Re: [Qemu-devel] [PATCH 2/2] target-mips/translate.c: Add judgement for msb and lsb

[Peter Maydell]

On 28 July 2014 22:42, Aurelien Jarno <aurelien@aurel32.net> wrote:
> On Mon, Jul 28, 2014 at 11:58:22PM +0800, Dongxue Zhang wrote:
>> Use 'if' to make sure the real msb greater than the lsb. As the compiler may
>> not do this.
>
> What are you trying to fix exactly? These cases are defined as
> "unpredictable" in the MIPS ISA manual, which is what is implemented in
> QEMU.

This may be true, but the TCG README doesn't define negative
lengths as being "unspecified behaviour" (ie guaranteed to at
least not crash even if the result isn't specified), and in fact the
implementation of tcg_gen_deposit will assert on negative lengths.
We shouldn't implement guest unpredictable cases as "crash QEMU".

thanks
-- PMM

Re: [Qemu-devel] [PATCH 2/2] target-mips/translate.c: Add judgement for msb and lsb

[Aurelien Jarno]

On Mon, Jul 28, 2014 at 11:01:02PM +0100, Peter Maydell wrote:
> On 28 July 2014 22:42, Aurelien Jarno <aurelien@aurel32.net> wrote:
> > On Mon, Jul 28, 2014 at 11:58:22PM +0800, Dongxue Zhang wrote:
> >> Use 'if' to make sure the real msb greater than the lsb. As the compiler may
> >> not do this.
> >
> > What are you trying to fix exactly? These cases are defined as
> > "unpredictable" in the MIPS ISA manual, which is what is implemented in
> > QEMU.
> 
> This may be true, but the TCG README doesn't define negative
> lengths as being "unspecified behaviour" (ie guaranteed to at
> least not crash even if the result isn't specified), and in fact the
> implementation of tcg_gen_deposit will assert on negative lengths.
> We shouldn't implement guest unpredictable cases as "crash QEMU".

Well I tried this code under QEMU, and it clearly doesn't crash. It
seems the assert are not enabled with the default configuration options.
That said I agree it's something to avoid, but I don't think triggering
a RI exception is the thing to do (even if it is correct according the
MIPS ISA manual) when real silicon output a random result instead.

-- 
Aurelien Jarno                          GPG: 4096R/1DDD8C9B
aurelien@aurel32.net                 http://www.aurel32.net

Re: [Qemu-devel] [PATCH 2/2] target-mips/translate.c: Add judgement for msb and lsb

[Peter Maydell]

On 28 July 2014 23:32, Aurelien Jarno <aurelien@aurel32.net> wrote:
> On Mon, Jul 28, 2014 at 11:01:02PM +0100, Peter Maydell wrote:
>> This may be true, but the TCG README doesn't define negative
>> lengths as being "unspecified behaviour" (ie guaranteed to at
>> least not crash even if the result isn't specified), and in fact the
>> implementation of tcg_gen_deposit will assert on negative lengths.
>> We shouldn't implement guest unpredictable cases as "crash QEMU".
>
> Well I tried this code under QEMU, and it clearly doesn't crash. It
> seems the assert are not enabled with the default configuration options.

Try --enable-debug...

> That said I agree it's something to avoid, but I don't think triggering
> a RI exception is the thing to do (even if it is correct according the
> MIPS ISA manual) when real silicon output a random result instead.

Yes, you could emit code to do that instead if you like.

thanks
-- PMM

Re: [Qemu-devel] [PATCH 2/2] target-mips/translate.c: Add judgement for msb and lsb

[Aurelien Jarno]

On Mon, Jul 28, 2014 at 11:34:30PM +0100, Peter Maydell wrote:
> On 28 July 2014 23:32, Aurelien Jarno <aurelien@aurel32.net> wrote:
> > On Mon, Jul 28, 2014 at 11:01:02PM +0100, Peter Maydell wrote:
> >> This may be true, but the TCG README doesn't define negative
> >> lengths as being "unspecified behaviour" (ie guaranteed to at
> >> least not crash even if the result isn't specified), and in fact the
> >> implementation of tcg_gen_deposit will assert on negative lengths.
> >> We shouldn't implement guest unpredictable cases as "crash QEMU".
> >
> > Well I tried this code under QEMU, and it clearly doesn't crash. It
> > seems the assert are not enabled with the default configuration options.
> 
> Try --enable-debug...

That's my point, it's only in debug mode, not in the default
configuration.

> > That said I agree it's something to avoid, but I don't think triggering
> > a RI exception is the thing to do (even if it is correct according the
> > MIPS ISA manual) when real silicon output a random result instead.
> 
> Yes, you could emit code to do that instead if you like.

When I said random, it didn't say in the sense of random generator, but
in the sense a result that might depend on the input value and the
silicon implementation. It would be silly to emit code just for that,
but it would be smart for example to skip the deposit op in that case
instead of triggering an exception.

-- 
Aurelien Jarno                          GPG: 4096R/1DDD8C9B
aurelien@aurel32.net                 http://www.aurel32.net

Re: [Qemu-devel] [PATCH 2/2] target-mips/translate.c: Add judgement for msb and lsb

[Elta]

On 07/29/2014 06:52 AM, Aurelien Jarno wrote:
> On Mon, Jul 28, 2014 at 11:34:30PM +0100, Peter Maydell wrote:
>> On 28 July 2014 23:32, Aurelien Jarno <aurelien@aurel32.net> wrote:
>>> On Mon, Jul 28, 2014 at 11:01:02PM +0100, Peter Maydell wrote:
>>>> This may be true, but the TCG README doesn't define negative
>>>> lengths as being "unspecified behaviour" (ie guaranteed to at
>>>> least not crash even if the result isn't specified), and in fact the
>>>> implementation of tcg_gen_deposit will assert on negative lengths.
>>>> We shouldn't implement guest unpredictable cases as "crash QEMU".
>>> Well I tried this code under QEMU, and it clearly doesn't crash. It
>>> seems the assert are not enabled with the default configuration options.
>> Try --enable-debug...
> That's my point, it's only in debug mode, not in the default
> configuration.

Maybe remove the tcg_debug_assert in tcg_gen_deposit_i64 and 
tcg_gen_deposit_i64
is a better way. But it may cause other mistake in other architecture, 
i'm not
sure.

>
>>> That said I agree it's something to avoid, but I don't think triggering
>>> a RI exception is the thing to do (even if it is correct according the
>>> MIPS ISA manual) when real silicon output a random result instead.
>> Yes, you could emit code to do that instead if you like.
> When I said random, it didn't say in the sense of random generator, but
> in the sense a result that might depend on the input value and the
> silicon implementation. It would be silly to emit code just for that,
> but it would be smart for example to skip the deposit op in that case
> instead of triggering an exception.
>
I think, debug mode shouldn't crash the qemu with an unpredictable 
operation,
so i want to fix it. And you say there shouldn't raise RI, i agree with you.

Or when lsb > msb, just leave the code and do nothing. What do you think 
about
this way?

Re: [Qemu-devel] [PATCH 2/2] target-mips/translate.c: Add judgement for msb and lsb

[Aurelien Jarno]

On Tue, Jul 29, 2014 at 08:41:08PM +0800, Elta wrote:
> I think, debug mode shouldn't crash the qemu with an unpredictable
> operation,
> so i want to fix it. And you say there shouldn't raise RI, i agree with you.

Agreed.

> Or when lsb > msb, just leave the code and do nothing. What do you
> think about
> this way?

Yes, you can use something like:

    if (lsb <= msb) {
        deposit(...)
    }

-- 
Aurelien Jarno                          GPG: 4096R/1DDD8C9B
aurelien@aurel32.net                 http://www.aurel32.net

Re: [Qemu-devel] [PATCH 2/2] target-mips/translate.c: Add judgement for msb and lsb

[Dongxue Zhang]

[-- Attachment #1: Type: text/plain, Size: 699 bytes --]

Ok, I got you. I will re-build a new patch for all the bitops.


2014-07-29 22:08 GMT+08:00 Aurelien Jarno <aurelien@aurel32.net>:

> On Tue, Jul 29, 2014 at 08:41:08PM +0800, Elta wrote:
> > I think, debug mode shouldn't crash the qemu with an unpredictable
> > operation,
> > so i want to fix it. And you say there shouldn't raise RI, i agree with
> you.
>
> Agreed.
>
> > Or when lsb > msb, just leave the code and do nothing. What do you
> > think about
> > this way?
>
> Yes, you can use something like:
>
>     if (lsb <= msb) {
>         deposit(...)
>     }
>
> --
> Aurelien Jarno                          GPG: 4096R/1DDD8C9B
> aurelien@aurel32.net                 http://www.aurel32.net
>

[-- Attachment #2: Type: text/html, Size: 1304 bytes --]

Re: [Qemu-devel] [PATCH 2/2] target-mips/translate.c: Add judgement for msb and lsb

[Peter Maydell]

On 28 July 2014 23:52, Aurelien Jarno <aurelien@aurel32.net> wrote:
> On Mon, Jul 28, 2014 at 11:34:30PM +0100, Peter Maydell wrote:
>> On 28 July 2014 23:32, Aurelien Jarno <aurelien@aurel32.net> wrote:
>> > On Mon, Jul 28, 2014 at 11:01:02PM +0100, Peter Maydell wrote:
>> >> This may be true, but the TCG README doesn't define negative
>> >> lengths as being "unspecified behaviour" (ie guaranteed to at
>> >> least not crash even if the result isn't specified), and in fact the
>> >> implementation of tcg_gen_deposit will assert on negative lengths.
>> >> We shouldn't implement guest unpredictable cases as "crash QEMU".
>> >
>> > Well I tried this code under QEMU, and it clearly doesn't crash. It
>> > seems the assert are not enabled with the default configuration options.
>>
>> Try --enable-debug...
>
> That's my point, it's only in debug mode, not in the default
> configuration.

Debug builds are pretty common though, it's not exactly
something obscure like "only crashes on SPARC hosts".

>> > That said I agree it's something to avoid, but I don't think triggering
>> > a RI exception is the thing to do (even if it is correct according the
>> > MIPS ISA manual) when real silicon output a random result instead.
>>
>> Yes, you could emit code to do that instead if you like.
>
> When I said random, it didn't say in the sense of random generator, but
> in the sense a result that might depend on the input value and the
> silicon implementation. It would be silly to emit code just for that,
> but it would be smart for example to skip the deposit op in that case
> instead of triggering an exception.

That's what I had in mind, yes.

thanks
-- PMM

Re: [Qemu-devel] [PATCH 1/2] target-mips/translate.c: Free TCG in OPC_DINSV

[Aurelien Jarno]

On Mon, Jul 28, 2014 at 11:58:21PM +0800, Dongxue Zhang wrote:
> Free t0 and t1 in opcode OPC_DINSV.
> 
> Signed-off-by: Dongxue Zhang <elta.era@gmail.com>
> ---
>  target-mips/translate.c | 3 +++
>  1 file changed, 3 insertions(+)
> 
> diff --git a/target-mips/translate.c b/target-mips/translate.c
> index d7b8c4d..c381366 100644
> --- a/target-mips/translate.c
> +++ b/target-mips/translate.c
> @@ -15300,6 +15300,9 @@ static void decode_opc (CPUMIPSState *env, DisasContext *ctx)
>                      gen_load_gpr(t1, rs);
>  
>                      gen_helper_dinsv(cpu_gpr[rt], cpu_env, t1, t0);
> +
> +                    tcg_temp_free(t0);
> +                    tcg_temp_free(t1);
>                      break;
>                  }
>              default:            /* Invalid */

Good catch, I applied the patch.

-- 
Aurelien Jarno                          GPG: 4096R/1DDD8C9B
aurelien@aurel32.net                 http://www.aurel32.net