[Qemu-devel] [PATCH 0/2] HPPA tcg fixes

[Qemu-devel] [PATCH 0/2] HPPA tcg fixes

[Sven Schnelle]

Hi Richard,

here are two patches for HPPA tcg. QEMU was crashing with a tcg assert
because dead temporaries where used. This could be observed at the end·
of a HP-UX 11.11 installation, or by running the STARBASE X11 demos in
HP-UX 10.20.

Thanks,
Sven

Sven Schnelle (2):
  target/hppa: prevent trashing of temporary in trans_mtctl()
  target/hppa: prevent trashing of temporary in do_depw_sar()

 target/hppa/translate.c | 15 ++++++++++-----
 1 file changed, 10 insertions(+), 5 deletions(-)

-- 
2.23.0.rc1

[Qemu-devel] [PATCH 1/2] target/hppa: prevent trashing of temporary in trans_mtctl()

[Sven Schnelle]

nullify_over() calls brcond which destroys all temporaries.

Signed-off-by: Sven Schnelle <svens@stackframe.org>
---
 target/hppa/translate.c | 5 ++++-
 1 file changed, 4 insertions(+), 1 deletion(-)

diff --git a/target/hppa/translate.c b/target/hppa/translate.c
index 53e17d8963..b12525d535 100644
--- a/target/hppa/translate.c
+++ b/target/hppa/translate.c
@@ -2214,10 +2214,11 @@ static bool trans_mtsp(DisasContext *ctx, arg_mtsp *a)
 static bool trans_mtctl(DisasContext *ctx, arg_mtctl *a)
 {
     unsigned ctl = a->t;
-    TCGv_reg reg = load_gpr(ctx, a->r);
+    TCGv_reg reg;
     TCGv_reg tmp;
 
     if (ctl == CR_SAR) {
+        reg = load_gpr(ctx, a->r);
         tmp = tcg_temp_new();
         tcg_gen_andi_reg(tmp, reg, TARGET_REGISTER_BITS - 1);
         save_or_nullify(ctx, cpu_sar, tmp);
@@ -2232,6 +2233,8 @@ static bool trans_mtctl(DisasContext *ctx, arg_mtctl *a)
 
 #ifndef CONFIG_USER_ONLY
     nullify_over(ctx);
+    reg = load_gpr(ctx, a->r);
+
     switch (ctl) {
     case CR_IT:
         gen_helper_write_interval_timer(cpu_env, reg);
-- 
2.23.0.rc1

[Qemu-devel] [PATCH 2/2] target/hppa: prevent trashing of temporary in do_depw_sar()

[Sven Schnelle]

nullify_over() calls brcond which destroys all temporaries.

Signed-off-by: Sven Schnelle <svens@stackframe.org>
---
 target/hppa/translate.c | 10 ++++++----
 1 file changed, 6 insertions(+), 4 deletions(-)

diff --git a/target/hppa/translate.c b/target/hppa/translate.c
index b12525d535..c1b2822f60 100644
--- a/target/hppa/translate.c
+++ b/target/hppa/translate.c
@@ -3404,10 +3404,6 @@ static bool do_depw_sar(DisasContext *ctx, unsigned rt, unsigned c,
     TCGv_reg mask, tmp, shift, dest;
     unsigned msb = 1U << (len - 1);
 
-    if (c) {
-        nullify_over(ctx);
-    }
-
     dest = dest_gpr(ctx, rt);
     shift = tcg_temp_new();
     tmp = tcg_temp_new();
@@ -3440,11 +3436,17 @@ static bool do_depw_sar(DisasContext *ctx, unsigned rt, unsigned c,
 
 static bool trans_depw_sar(DisasContext *ctx, arg_depw_sar *a)
 {
+    if (a->c) {
+        nullify_over(ctx);
+    }
     return do_depw_sar(ctx, a->t, a->c, a->nz, a->clen, load_gpr(ctx, a->r));
 }
 
 static bool trans_depwi_sar(DisasContext *ctx, arg_depwi_sar *a)
 {
+    if (a->c) {
+        nullify_over(ctx);
+    }
     return do_depw_sar(ctx, a->t, a->c, a->nz, a->clen, load_const(ctx, a->i));
 }
 
-- 
2.23.0.rc1

Re: [Qemu-devel] [PATCH 2/2] target/hppa: prevent trashing of temporary in do_depw_sar()

[Philippe Mathieu-Daudé]

Hi Sven,

On 9/13/19 12:17 PM, Sven Schnelle wrote:
> nullify_over() calls brcond which destroys all temporaries.
> 
> Signed-off-by: Sven Schnelle <svens@stackframe.org>
> ---
>  target/hppa/translate.c | 10 ++++++----
>  1 file changed, 6 insertions(+), 4 deletions(-)
> 
> diff --git a/target/hppa/translate.c b/target/hppa/translate.c
> index b12525d535..c1b2822f60 100644
> --- a/target/hppa/translate.c
> +++ b/target/hppa/translate.c
> @@ -3404,10 +3404,6 @@ static bool do_depw_sar(DisasContext *ctx, unsigned rt, unsigned c,
>      TCGv_reg mask, tmp, shift, dest;
>      unsigned msb = 1U << (len - 1);
>  
> -    if (c) {
> -        nullify_over(ctx);
> -    }
> -
>      dest = dest_gpr(ctx, rt);
>      shift = tcg_temp_new();
>      tmp = tcg_temp_new();
> @@ -3440,11 +3436,17 @@ static bool do_depw_sar(DisasContext *ctx, unsigned rt, unsigned c,
>  
>  static bool trans_depw_sar(DisasContext *ctx, arg_depw_sar *a)
>  {
> +    if (a->c) {
> +        nullify_over(ctx);
> +    }
>      return do_depw_sar(ctx, a->t, a->c, a->nz, a->clen, load_gpr(ctx, a->r));
>  }
>  
>  static bool trans_depwi_sar(DisasContext *ctx, arg_depwi_sar *a)
>  {
> +    if (a->c) {
> +        nullify_over(ctx);
> +    }

I don't see how this patch helps or change anything, isn't it the same?
You clean in the caller rather than the callee.

>      return do_depw_sar(ctx, a->t, a->c, a->nz, a->clen, load_const(ctx, a->i));
>  }
>  
> 

Re: [Qemu-devel] [PATCH 2/2] target/hppa: prevent trashing of temporary in do_depw_sar()

[Sven Schnelle]

Hi Philippe,

On Fri, Sep 13, 2019 at 12:58:14PM +0200, Philippe Mathieu-Daudé wrote:
> Hi Sven,
> 
> On 9/13/19 12:17 PM, Sven Schnelle wrote:
> > nullify_over() calls brcond which destroys all temporaries.
> > 
> > Signed-off-by: Sven Schnelle <svens@stackframe.org>
> > ---
> >  target/hppa/translate.c | 10 ++++++----
> >  1 file changed, 6 insertions(+), 4 deletions(-)
> > 
> > diff --git a/target/hppa/translate.c b/target/hppa/translate.c
> > index b12525d535..c1b2822f60 100644
> > --- a/target/hppa/translate.c
> > +++ b/target/hppa/translate.c
> > @@ -3404,10 +3404,6 @@ static bool do_depw_sar(DisasContext *ctx, unsigned rt, unsigned c,
> >      TCGv_reg mask, tmp, shift, dest;
> >      unsigned msb = 1U << (len - 1);
> >  
> > -    if (c) {
> > -        nullify_over(ctx);
> > -    }
> > -
> >      dest = dest_gpr(ctx, rt);
> >      shift = tcg_temp_new();
> >      tmp = tcg_temp_new();
> > @@ -3440,11 +3436,17 @@ static bool do_depw_sar(DisasContext *ctx, unsigned rt, unsigned c,
> >  
> >  static bool trans_depw_sar(DisasContext *ctx, arg_depw_sar *a)
> >  {
> > +    if (a->c) {
> > +        nullify_over(ctx);
> > +    }
> >      return do_depw_sar(ctx, a->t, a->c, a->nz, a->clen, load_gpr(ctx, a->r));
> >  }
> >  
> >  static bool trans_depwi_sar(DisasContext *ctx, arg_depwi_sar *a)
> >  {
> > +    if (a->c) {
> > +        nullify_over(ctx);
> > +    }
> 
> I don't see how this patch helps or change anything, isn't it the same?
> You clean in the caller rather than the callee.

The Problem is that load_gpr()/load_const() allocate a temporary, which
gets destroyed in do_depw_sar() when nullify_over() is called. If we
move the nullify_over() before doing the load_gpr()/load_const() this
doesn't happen.

> >      return do_depw_sar(ctx, a->t, a->c, a->nz, a->clen, load_const(ctx, a->i));
> >  }
> >  
> >

Regards
Sven

Re: [Qemu-devel] [PATCH 2/2] target/hppa: prevent trashing of temporary in do_depw_sar()

[Philippe Mathieu-Daudé]

On 9/13/19 1:08 PM, Sven Schnelle wrote:
> Hi Philippe,
> 
> On Fri, Sep 13, 2019 at 12:58:14PM +0200, Philippe Mathieu-Daudé wrote:
>> Hi Sven,
>>
>> On 9/13/19 12:17 PM, Sven Schnelle wrote:
>>> nullify_over() calls brcond which destroys all temporaries.
>>>
>>> Signed-off-by: Sven Schnelle <svens@stackframe.org>
>>> ---
>>>  target/hppa/translate.c | 10 ++++++----
>>>  1 file changed, 6 insertions(+), 4 deletions(-)
>>>
>>> diff --git a/target/hppa/translate.c b/target/hppa/translate.c
>>> index b12525d535..c1b2822f60 100644
>>> --- a/target/hppa/translate.c
>>> +++ b/target/hppa/translate.c
>>> @@ -3404,10 +3404,6 @@ static bool do_depw_sar(DisasContext *ctx, unsigned rt, unsigned c,
>>>      TCGv_reg mask, tmp, shift, dest;
>>>      unsigned msb = 1U << (len - 1);
>>>  
>>> -    if (c) {
>>> -        nullify_over(ctx);
>>> -    }
>>> -
>>>      dest = dest_gpr(ctx, rt);
>>>      shift = tcg_temp_new();
>>>      tmp = tcg_temp_new();
>>> @@ -3440,11 +3436,17 @@ static bool do_depw_sar(DisasContext *ctx, unsigned rt, unsigned c,
>>>  
>>>  static bool trans_depw_sar(DisasContext *ctx, arg_depw_sar *a)
>>>  {
>>> +    if (a->c) {
>>> +        nullify_over(ctx);
>>> +    }
>>>      return do_depw_sar(ctx, a->t, a->c, a->nz, a->clen, load_gpr(ctx, a->r));
>>>  }
>>>  
>>>  static bool trans_depwi_sar(DisasContext *ctx, arg_depwi_sar *a)
>>>  {
>>> +    if (a->c) {
>>> +        nullify_over(ctx);
>>> +    }
>>
>> I don't see how this patch helps or change anything, isn't it the same?
>> You clean in the caller rather than the callee.
> 
> The Problem is that load_gpr()/load_const() allocate a temporary, which
> gets destroyed in do_depw_sar() when nullify_over() is called. If we
> move the nullify_over() before doing the load_gpr()/load_const() this
> doesn't happen.

Ah! The 'val' argument... I missed that, thanks!

Maybe we can add a comment to make it clearer:

   if (a->c) {
       /*
        * nullify here to not free the load_gpr() arg before
        * calling depw_sar.
        */
       nullify_over(ctx);
   }
   return do_depw_sar(ctx, a->t, a->c, a->nz, a->clen, load_gpr(ctx, a->r));

(also in the other function).

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

>>>      return do_depw_sar(ctx, a->t, a->c, a->nz, a->clen, load_const(ctx, a->i));
>>>  }
>>>  
>>>
> 
> Regards
> Sven
> 

Re: [Qemu-devel] [PATCH 0/2] HPPA tcg fixes

[Richard Henderson]

On 9/13/19 6:17 AM, Sven Schnelle wrote:
> Sven Schnelle (2):
>   target/hppa: prevent trashing of temporary in trans_mtctl()
>   target/hppa: prevent trashing of temporary in do_depw_sar()
> 
>  target/hppa/translate.c | 15 ++++++++++-----
>  1 file changed, 10 insertions(+), 5 deletions(-)

Thanks, queued.


r~