[Qemu-devel] [PATCH] target-sparc: fix register corruption in ldstub if there is no write permission

[Qemu-devel] [PATCH] target-sparc: fix register corruption in ldstub if there is no write permission

[Artyom Tarasenko]

Signed-off-by: Artyom Tarasenko <atar4qemu@gmail.com>
---
 target-sparc/translate.c | 5 ++++-
 1 file changed, 4 insertions(+), 1 deletion(-)

diff --git a/target-sparc/translate.c b/target-sparc/translate.c
index 5111cf0..065326c 100644
--- a/target-sparc/translate.c
+++ b/target-sparc/translate.c
@@ -5187,12 +5187,15 @@ printf("ops, illegal rdhpr\n");
                 case 0xd:       /* ldstub -- XXX: should be atomically */
                     {
                         TCGv r_const;
+                        TCGv tmp = tcg_temp_new();
 
                         gen_address_mask(dc, cpu_addr);
-                        tcg_gen_qemu_ld8u(cpu_val, cpu_addr, dc->mem_idx);
+                        tcg_gen_qemu_ld8u(tmp, cpu_addr, dc->mem_idx);
                         r_const = tcg_const_tl(0xff);
                         tcg_gen_qemu_st8(r_const, cpu_addr, dc->mem_idx);
+                        tcg_gen_mov_tl(cpu_val, tmp);
                         tcg_temp_free(r_const);
+                        tcg_temp_free(tmp);
                     }
                     break;
                 case 0x0f:
-- 
1.8.3.1

Re: [Qemu-devel] [PATCH] target-sparc: fix register corruption in ldstub if there is no write permission

[Mark Cave-Ayland]

On 24/06/16 13:34, Artyom Tarasenko wrote:

> Signed-off-by: Artyom Tarasenko <atar4qemu@gmail.com>
> ---
>  target-sparc/translate.c | 5 ++++-
>  1 file changed, 4 insertions(+), 1 deletion(-)
>
> diff --git a/target-sparc/translate.c b/target-sparc/translate.c
> index 5111cf0..065326c 100644
> --- a/target-sparc/translate.c
> +++ b/target-sparc/translate.c
> @@ -5187,12 +5187,15 @@ printf("ops, illegal rdhpr\n");
>                  case 0xd:       /* ldstub -- XXX: should be atomically */
>                      {
>                          TCGv r_const;
> +                        TCGv tmp = tcg_temp_new();
>
>                          gen_address_mask(dc, cpu_addr);
> -                        tcg_gen_qemu_ld8u(cpu_val, cpu_addr, dc->mem_idx);
> +                        tcg_gen_qemu_ld8u(tmp, cpu_addr, dc->mem_idx);
>                          r_const = tcg_const_tl(0xff);
>                          tcg_gen_qemu_st8(r_const, cpu_addr, dc->mem_idx);
> +                        tcg_gen_mov_tl(cpu_val, tmp);
>                          tcg_temp_free(r_const);
> +                        tcg_temp_free(tmp);
>                      }
>                      break;
>                  case 0x0f:
>

Looks like you beat me to it - I can confirm that this fixes the issue 
here for me. Whilst testing I noticed another regression under 
qemu-system-sparc, however bisection reveals that this isn't caused by a 
SPARC-specific patch (and can be followed up separately) so:

Tested-by: Mark Cave-Ayland <mark.cave-ayland@ilande.co.uk>


ATB,

Mark.

Re: [Qemu-devel] [PATCH] target-sparc: fix register corruption in ldstub if there is no write permission

[Richard Henderson]

On 06/24/2016 05:34 AM, Artyom Tarasenko wrote:
> Signed-off-by: Artyom Tarasenko <atar4qemu@gmail.com>
> ---
>  target-sparc/translate.c | 5 ++++-
>  1 file changed, 4 insertions(+), 1 deletion(-)

Reviewed-by: Richard Henderson <rth@twiddle.net>


r~

Re: [Qemu-devel] [PATCH] target-sparc: fix register corruption in ldstub if there is no write permission

[Richard Henderson]

On 06/24/2016 05:34 AM, Artyom Tarasenko wrote:
> Signed-off-by: Artyom Tarasenko <atar4qemu@gmail.com>
> ---
>  target-sparc/translate.c | 5 ++++-
>  1 file changed, 4 insertions(+), 1 deletion(-)
>
> diff --git a/target-sparc/translate.c b/target-sparc/translate.c
> index 5111cf0..065326c 100644
> --- a/target-sparc/translate.c
> +++ b/target-sparc/translate.c
> @@ -5187,12 +5187,15 @@ printf("ops, illegal rdhpr\n");
>                  case 0xd:       /* ldstub -- XXX: should be atomically */
>                      {
>                          TCGv r_const;
> +                        TCGv tmp = tcg_temp_new();
>
>                          gen_address_mask(dc, cpu_addr);
> -                        tcg_gen_qemu_ld8u(cpu_val, cpu_addr, dc->mem_idx);
> +                        tcg_gen_qemu_ld8u(tmp, cpu_addr, dc->mem_idx);
>                          r_const = tcg_const_tl(0xff);
>                          tcg_gen_qemu_st8(r_const, cpu_addr, dc->mem_idx);
> +                        tcg_gen_mov_tl(cpu_val, tmp);
>                          tcg_temp_free(r_const);
> +                        tcg_temp_free(tmp);

ldstub_asi has the same problem on mainline.

It looks like I fixed that one on my sparc branch though.


r~

Re: [Qemu-devel] [PATCH] target-sparc: fix register corruption in ldstub if there is no write permission

[Artyom Tarasenko]

On Fri, Jun 24, 2016 at 5:51 PM, Mark Cave-Ayland
<mark.cave-ayland@ilande.co.uk> wrote:
> On 24/06/16 13:34, Artyom Tarasenko wrote:
>
>> Signed-off-by: Artyom Tarasenko <atar4qemu@gmail.com>
>> ---
>>  target-sparc/translate.c | 5 ++++-
>>  1 file changed, 4 insertions(+), 1 deletion(-)
>>
>> diff --git a/target-sparc/translate.c b/target-sparc/translate.c
>> index 5111cf0..065326c 100644
>> --- a/target-sparc/translate.c
>> +++ b/target-sparc/translate.c
>> @@ -5187,12 +5187,15 @@ printf("ops, illegal rdhpr\n");
>>                  case 0xd:       /* ldstub -- XXX: should be atomically */
>>                      {
>>                          TCGv r_const;
>> +                        TCGv tmp = tcg_temp_new();
>>
>>                          gen_address_mask(dc, cpu_addr);
>> -                        tcg_gen_qemu_ld8u(cpu_val, cpu_addr,
>> dc->mem_idx);
>> +                        tcg_gen_qemu_ld8u(tmp, cpu_addr, dc->mem_idx);
>>                          r_const = tcg_const_tl(0xff);
>>                          tcg_gen_qemu_st8(r_const, cpu_addr, dc->mem_idx);
>> +                        tcg_gen_mov_tl(cpu_val, tmp);
>>                          tcg_temp_free(r_const);
>> +                        tcg_temp_free(tmp);
>>                      }
>>                      break;
>>                  case 0x0f:
>>
>
> Looks like you beat me to it - I can confirm that this fixes the issue here
> for me. Whilst testing I noticed another regression under qemu-system-sparc,
> however bisection reveals that this isn't caused by a SPARC-specific patch
> (and can be followed up separately) so:
>
> Tested-by: Mark Cave-Ayland <mark.cave-ayland@ilande.co.uk>
>

Good. Then we can route it via your tree. (With Richard's Reviewed-by)
I'm still worried why it didn't hit us before.

Artyom

-- 
Regards,
Artyom Tarasenko

SPARC and PPC PReP under qemu blog: http://tyom.blogspot.com/search/label/qemu

Re: [Qemu-devel] [PATCH] target-sparc: fix register corruption in ldstub if there is no write permission

[Mark Cave-Ayland]

On 24/06/16 16:58, Richard Henderson wrote:

> On 06/24/2016 05:34 AM, Artyom Tarasenko wrote:
>> Signed-off-by: Artyom Tarasenko <atar4qemu@gmail.com>
>> ---
>>  target-sparc/translate.c | 5 ++++-
>>  1 file changed, 4 insertions(+), 1 deletion(-)
>>
>> diff --git a/target-sparc/translate.c b/target-sparc/translate.c
>> index 5111cf0..065326c 100644
>> --- a/target-sparc/translate.c
>> +++ b/target-sparc/translate.c
>> @@ -5187,12 +5187,15 @@ printf("ops, illegal rdhpr\n");
>>                  case 0xd:       /* ldstub -- XXX: should be
>> atomically */
>>                      {
>>                          TCGv r_const;
>> +                        TCGv tmp = tcg_temp_new();
>>
>>                          gen_address_mask(dc, cpu_addr);
>> -                        tcg_gen_qemu_ld8u(cpu_val, cpu_addr,
>> dc->mem_idx);
>> +                        tcg_gen_qemu_ld8u(tmp, cpu_addr, dc->mem_idx);
>>                          r_const = tcg_const_tl(0xff);
>>                          tcg_gen_qemu_st8(r_const, cpu_addr,
>> dc->mem_idx);
>> +                        tcg_gen_mov_tl(cpu_val, tmp);
>>                          tcg_temp_free(r_const);
>> +                        tcg_temp_free(tmp);
>
> ldstub_asi has the same problem on mainline.
>
> It looks like I fixed that one on my sparc branch though.
>
>
> r~

In that case would it make sense to prepend this patch to a v4 respin of 
your latest SPARC patchset (as tested by Artyom)?


ATB,

Mark.

Re: [Qemu-devel] [PATCH] target-sparc: fix register corruption in ldstub if there is no write permission

[Mark Cave-Ayland]

On 24/06/16 17:01, Artyom Tarasenko wrote:

> On Fri, Jun 24, 2016 at 5:51 PM, Mark Cave-Ayland
> <mark.cave-ayland@ilande.co.uk> wrote:
>> On 24/06/16 13:34, Artyom Tarasenko wrote:
>>
>>> Signed-off-by: Artyom Tarasenko <atar4qemu@gmail.com>
>>> ---
>>>  target-sparc/translate.c | 5 ++++-
>>>  1 file changed, 4 insertions(+), 1 deletion(-)
>>>
>>> diff --git a/target-sparc/translate.c b/target-sparc/translate.c
>>> index 5111cf0..065326c 100644
>>> --- a/target-sparc/translate.c
>>> +++ b/target-sparc/translate.c
>>> @@ -5187,12 +5187,15 @@ printf("ops, illegal rdhpr\n");
>>>                  case 0xd:       /* ldstub -- XXX: should be atomically */
>>>                      {
>>>                          TCGv r_const;
>>> +                        TCGv tmp = tcg_temp_new();
>>>
>>>                          gen_address_mask(dc, cpu_addr);
>>> -                        tcg_gen_qemu_ld8u(cpu_val, cpu_addr,
>>> dc->mem_idx);
>>> +                        tcg_gen_qemu_ld8u(tmp, cpu_addr, dc->mem_idx);
>>>                          r_const = tcg_const_tl(0xff);
>>>                          tcg_gen_qemu_st8(r_const, cpu_addr, dc->mem_idx);
>>> +                        tcg_gen_mov_tl(cpu_val, tmp);
>>>                          tcg_temp_free(r_const);
>>> +                        tcg_temp_free(tmp);
>>>                      }
>>>                      break;
>>>                  case 0x0f:
>>>
>>
>> Looks like you beat me to it - I can confirm that this fixes the issue here
>> for me. Whilst testing I noticed another regression under qemu-system-sparc,
>> however bisection reveals that this isn't caused by a SPARC-specific patch
>> (and can be followed up separately) so:
>>
>> Tested-by: Mark Cave-Ayland <mark.cave-ayland@ilande.co.uk>
>>
>
> Good. Then we can route it via your tree. (With Richard's Reviewed-by)
> I'm still worried why it didn't hit us before.

Oops, looks like our mails overlapped. In that case I'll send a pull 
request ASAP.


ATB,

Mark.

[Qemu-devel] [PATCH] target-sparc: fix register corruption in ldstub if there is no write permission

[Mark Cave-Ayland]

From: Artyom Tarasenko <atar4qemu@gmail.com>

Signed-off-by: Artyom Tarasenko <atar4qemu@gmail.com>
Reviewed-by: Richard Henderson <rth@twiddle.net>
Signed-off-by: Mark Cave-Ayland <mark.cave-ayland@ilande.co.uk>
---
 target-sparc/translate.c |    5 ++++-
 1 file changed, 4 insertions(+), 1 deletion(-)

diff --git a/target-sparc/translate.c b/target-sparc/translate.c
index afd46b8..0f4faf7 100644
--- a/target-sparc/translate.c
+++ b/target-sparc/translate.c
@@ -4679,12 +4679,15 @@ static void disas_sparc_insn(DisasContext * dc, unsigned int insn)
                 case 0xd:       /* ldstub -- XXX: should be atomically */
                     {
                         TCGv r_const;
+                        TCGv tmp = tcg_temp_new();
 
                         gen_address_mask(dc, cpu_addr);
-                        tcg_gen_qemu_ld8u(cpu_val, cpu_addr, dc->mem_idx);
+                        tcg_gen_qemu_ld8u(tmp, cpu_addr, dc->mem_idx);
                         r_const = tcg_const_tl(0xff);
                         tcg_gen_qemu_st8(r_const, cpu_addr, dc->mem_idx);
+                        tcg_gen_mov_tl(cpu_val, tmp);
                         tcg_temp_free(r_const);
+                        tcg_temp_free(tmp);
                     }
                     break;
                 case 0x0f:
-- 
1.7.10.4