[PATCH 0/4] target/s390x: CC fixes

Ilya Leoshkevich posted 4 patches 1 year ago
Patches applied successfully (tree, apply log)
git fetch https://github.com/patchew-project/qemu tags/patchew/20231031053718.347100-1-iii@linux.ibm.com
Maintainers: Richard Henderson <richard.henderson@linaro.org>, David Hildenbrand <david@redhat.com>, Ilya Leoshkevich <iii@linux.ibm.com>, Thomas Huth <thuth@redhat.com>
There is a newer version of this series
target/s390x/tcg/translate.c    | 10 ++++---
tests/tcg/s390x/Makefile.target |  2 ++
tests/tcg/s390x/clc.c           | 48 +++++++++++++++++++++++++++++++++
tests/tcg/s390x/laalg.c         | 27 +++++++++++++++++++
4 files changed, 84 insertions(+), 3 deletions(-)
create mode 100644 tests/tcg/s390x/clc.c
create mode 100644 tests/tcg/s390x/laalg.c
[PATCH 0/4] target/s390x: CC fixes
Posted by Ilya Leoshkevich 1 year ago
Hi,

This series fixes two issues with updating CC. David was suggesting a
bigger rewrite [1], but I did not dare do this (yet). Instead, these
are targeted fixes: patch 1 helps with installing Fedora, and patch 3
addresses something I noticed when reviewing the code. Patches 2 and 4
are tests.

Best regards,
Ilya

[1] https://gitlab.com/qemu-project/qemu/-/issues/1865#note_1545060658

Ilya Leoshkevich (4):
  target/s390x: Fix CLC corrupting cc_src
  tests/tcg/s390x: Test CLC with inaccessible second operand
  target/s390x: Fix LAALG not updating cc_src
  tests/tcg/s390x: Test LAALG with negative cc_src

 target/s390x/tcg/translate.c    | 10 ++++---
 tests/tcg/s390x/Makefile.target |  2 ++
 tests/tcg/s390x/clc.c           | 48 +++++++++++++++++++++++++++++++++
 tests/tcg/s390x/laalg.c         | 27 +++++++++++++++++++
 4 files changed, 84 insertions(+), 3 deletions(-)
 create mode 100644 tests/tcg/s390x/clc.c
 create mode 100644 tests/tcg/s390x/laalg.c

-- 
2.41.0
Re: [PATCH 0/4] target/s390x: CC fixes
Posted by David Hildenbrand 1 year ago
On 31.10.23 06:32, Ilya Leoshkevich wrote:
> Hi,
> 
> This series fixes two issues with updating CC. David was suggesting a
> bigger rewrite [1], but I did not dare do this (yet). Instead, these

I started coding that up but was distracted by other things; last time I 
looked at that, I concluded that the way we are calculating the carry in 
not suitable when we're doing two additions (like ADD LOGICAL WITH CARRY).

In addition, the way cout_addu32/cout_addu64 relies on cc_src/cc_dst is 
wrong in some occurrences.

> are targeted fixes: patch 1 helps with installing Fedora, and patch 3
> addresses something I noticed when reviewing the code. Patches 2 and 4
> are tests.

I assume you are only scratching the surface with your fixes and we 
might be better off just doing CC_OP_ADDU / CC_OP_SUBU like CC_OP_ADD_32 
/ CC_OP_ADD_64 / ... to fix all issues involved.

> 
> Best regards,
> Ilya
> 
> [1] https://gitlab.com/qemu-project/qemu/-/issues/1865#note_1545060658
> 
> Ilya Leoshkevich (4):
>    target/s390x: Fix CLC corrupting cc_src
>    tests/tcg/s390x: Test CLC with inaccessible second operand
>    target/s390x: Fix LAALG not updating cc_src
>    tests/tcg/s390x: Test LAALG with negative cc_src
> 
>   target/s390x/tcg/translate.c    | 10 ++++---
>   tests/tcg/s390x/Makefile.target |  2 ++
>   tests/tcg/s390x/clc.c           | 48 +++++++++++++++++++++++++++++++++
>   tests/tcg/s390x/laalg.c         | 27 +++++++++++++++++++
>   4 files changed, 84 insertions(+), 3 deletions(-)
>   create mode 100644 tests/tcg/s390x/clc.c
>   create mode 100644 tests/tcg/s390x/laalg.c
> 

-- 
Cheers,

David / dhildenb
Re: [PATCH 0/4] target/s390x: CC fixes
Posted by Ilya Leoshkevich 1 year ago
On Tue, 2023-10-31 at 09:38 +0100, David Hildenbrand wrote:
> On 31.10.23 06:32, Ilya Leoshkevich wrote:
> > Hi,
> > 
> > This series fixes two issues with updating CC. David was suggesting
> > a
> > bigger rewrite [1], but I did not dare do this (yet). Instead,
> > these
> 
> I started coding that up but was distracted by other things; last
> time I 
> looked at that, I concluded that the way we are calculating the carry
> in 
> not suitable when we're doing two additions (like ADD LOGICAL WITH
> CARRY).

Do you per chance remember any details? IIUC the code in question is:

static DisasJumpType op_addc64(DisasContext *s, DisasOps *o)
{
    compute_carry(s);

    TCGv_i64 zero = tcg_constant_i64(0);
    tcg_gen_add2_i64(o->out, cc_src, o->in1, zero, cc_src, zero);
    tcg_gen_add2_i64(o->out, cc_src, o->out, cc_src, o->in2, zero);

    return DISAS_NEXT;
}

This looks correct to me, because the 128-bit result of the first
addition is passed fully to the second addition, and the upper half is
always either 0 or 1.

I played with chaining ADD-LOGICAL-WITH-CARRYs with various inputs, and
could not produce any wrong calculations in the emulation.

Best regards,
Ilya

[...]
Re: [PATCH 0/4] target/s390x: CC fixes
Posted by David Hildenbrand 1 year ago
On 03.11.23 17:44, Ilya Leoshkevich wrote:
> On Tue, 2023-10-31 at 09:38 +0100, David Hildenbrand wrote:
>> On 31.10.23 06:32, Ilya Leoshkevich wrote:
>>> Hi,
>>>
>>> This series fixes two issues with updating CC. David was suggesting
>>> a
>>> bigger rewrite [1], but I did not dare do this (yet). Instead,
>>> these
>>
>> I started coding that up but was distracted by other things; last
>> time I
>> looked at that, I concluded that the way we are calculating the carry
>> in
>> not suitable when we're doing two additions (like ADD LOGICAL WITH
>> CARRY).
> 
> Do you per chance remember any details? IIUC the code in question is:

Unfortunately, I don't. I thought there would be a case where we could 
overflow twice, and result in a carry value of 2. Or some other weird 
corner case where the result would not be expressive.

Maybe I was daydreaming, let me see if I can re-discover what I found 
(should have taken notes but was just briefly looking at this).

If not, your fixes might be just good enough.

-- 
Cheers,

David / dhildenb