[Qemu-devel] [PATCH v3 00/29] s390x/tcg: mem_helper: Fault-safe handling

David Hildenbrand posted 29 patches 4 years, 7 months ago
Test docker-clang@ubuntu failed
Test FreeBSD failed
Test checkpatch failed
Test docker-quick@centos7 failed
Patches applied successfully (tree, apply log)
git fetch https://github.com/patchew-project/qemu tags/patchew/20190916135806.1269-1-david@redhat.com
Maintainers: David Hildenbrand <david@redhat.com>, Cornelia Huck <cohuck@redhat.com>, Richard Henderson <rth@twiddle.net>
target/s390x/cpu.h              |   4 +
target/s390x/helper.h           |   2 +-
target/s390x/insn-data.def      |   2 +-
target/s390x/mem_helper.c       | 743 ++++++++++++++++++++++----------
target/s390x/translate.c        |  12 +-
tests/tcg/s390x/Makefile.target |   1 +
tests/tcg/s390x/mvo.c           |  25 ++
7 files changed, 564 insertions(+), 225 deletions(-)
create mode 100644 tests/tcg/s390x/mvo.c
[Qemu-devel] [PATCH v3 00/29] s390x/tcg: mem_helper: Fault-safe handling
Posted by David Hildenbrand 4 years, 7 months ago
This series fixes a bunch of issues related to some mem helpers and makes
sure that they are fault-safe, meaning no system state is modified in case
a fault is triggered.

I can spot tons of other issues with other mem helpers that will have
to be fixed later. Also, fault-safe handling for some instructions
(especially TR) might be harder to implement (you don't know what will
actually be accessed upfront - we might need a buffer and go over
inputs twice). Focusing on the MOVE instructions for now.

----

Newer versions of glibc use memcpy() in memmove() for forward moves. The
implementation makese use of MVC. The TCG implementation of MVC is
currently not able to handle faults reliably when crossing pages. MVC
can cross with 256 bytes at most two pages.

In case we get a fault on the second page, we already moved data. When
continuing after the fault we might try to move already overwritten data,
which is very bad in case we have overlapping data on a forward move.

Triggered for now only by rpmbuild (crashes when checking the spec file)
and rpm (database corruptions). This fixes installing Fedora rawhide (31)
under TCG.

This was horrible to debug as it barely triggers and we fail at completely
different places.

Cc: Stefano Brivio <sbrivio@redhat.com>
Cc: Florian Weimer <fweimer@redhat.com>
Cc: Dan Horák <dan@danny.cz>
Cc: Cole Robinson <crobinso@redhat.com>

v2 -> v3:
- "s390x/tcg: MVCL: Zero out unused bits of address"
-- Do single deposit for 24/31-bit
- "s390x/tcg: MVCL: Process max 4k bytes at a time"
-- Use max of 4k instead of 2k, limiting to single pages
- "s390x/tcg: MVCLU/MVCLE: Process max 4k bytes at a time"
-- Limit to single pages
- "s390x/tcg: MVCOS: Lengths are 32 bit in 24/31-bit mode"
-- Added
- "s390x/tcg: MVCS/MVCP: Properly wrap the length"
-- Properly use 32 instead of 31 bit.
- "s390x/tcg: MVST: Fix storing back the addresses to registers"
-- Read R0 implicitly
- "s390x/tcg: Fault-safe memset"
-- Speed up TLB_NOTDIRTY handling
-- Move single-page access to helper function
-- Pass access structure to access_memset()
-- Replace access_prepare() by previous access_prepare_idx()
- "s390x/tcg: Fault-safe memmove"
-- Pass access structure to access_memmove()
-- Speed up TLB_NOTDIRTY handling when accessing single bytes
- The other fault-safe handling patches were adapted to work with the
  changed access functions. mmu_idx is now always passed to
  access_prepare() from the helpers.

v1 -> v2:
- Include many fixes
- Fix more instructions
- Use the new probe_access() function
- Include "tests/tcg: target/s390x: Test MVO"

David Hildenbrand (29):
  s390x/tcg: Reset exception_index to -1 instead of 0
  s390x/tcg: MVCL: Zero out unused bits of address
  s390x/tcg: MVCL: Detect destructive overlaps
  s390x/tcg: MVCL: Process max 4k bytes at a time
  s390x/tcg: MVC: Increment the length once
  s390x/tcg: MVC: Use is_destructive_overlap()
  s390x/tcg: MVPG: Check for specification exceptions
  s390x/tcg: MVPG: Properly wrap the addresses
  s390x/tcg: MVCLU/MVCLE: Process max 4k bytes at a time
  s390x/tcg: MVCS/MVCP: Check for special operation exceptions
  s390x/tcg: MVCOS: Lengths are 32 bit in 24/31-bit mode
  s390x/tcg: MVCS/MVCP: Properly wrap the length
  s390x/tcg: MVST: Check for specification exceptions
  s390x/tcg: MVST: Fix storing back the addresses to registers
  s390x/tcg: Always use MMU_USER_IDX for CONFIG_USER_ONLY
  s390x/tcg: Fault-safe memset
  s390x/tcg: Fault-safe memmove
  s390x/tcg: MVCS/MVCP: Use access_memmove()
  s390x/tcg: MVC: Fault-safe handling on destructive overlaps
  s390x/tcg: MVCLU: Fault-safe handling
  s390x/tcg: OC: Fault-safe handling
  s390x/tcg: XC: Fault-safe handling
  s390x/tcg: NC: Fault-safe handling
  s390x/tcg: MVCIN: Fault-safe handling
  s390x/tcg: MVN: Fault-safe handling
  s390x/tcg: MVZ: Fault-safe handling
  s390x/tcg: MVST: Fault-safe handling
  s390x/tcg: MVO: Fault-safe handling
  tests/tcg: target/s390x: Test MVO

 target/s390x/cpu.h              |   4 +
 target/s390x/helper.h           |   2 +-
 target/s390x/insn-data.def      |   2 +-
 target/s390x/mem_helper.c       | 743 ++++++++++++++++++++++----------
 target/s390x/translate.c        |  12 +-
 tests/tcg/s390x/Makefile.target |   1 +
 tests/tcg/s390x/mvo.c           |  25 ++
 7 files changed, 564 insertions(+), 225 deletions(-)
 create mode 100644 tests/tcg/s390x/mvo.c

-- 
2.21.0


Re: [Qemu-devel] [PATCH v3 00/29] s390x/tcg: mem_helper: Fault-safe handling
Posted by David Hildenbrand 4 years, 7 months ago
On 16.09.19 15:57, David Hildenbrand wrote:
> This series fixes a bunch of issues related to some mem helpers and makes
> sure that they are fault-safe, meaning no system state is modified in case
> a fault is triggered.
> 
> I can spot tons of other issues with other mem helpers that will have
> to be fixed later. Also, fault-safe handling for some instructions
> (especially TR) might be harder to implement (you don't know what will
> actually be accessed upfront - we might need a buffer and go over
> inputs twice). Focusing on the MOVE instructions for now.
> 
> ----
> 
> Newer versions of glibc use memcpy() in memmove() for forward moves. The
> implementation makese use of MVC. The TCG implementation of MVC is
> currently not able to handle faults reliably when crossing pages. MVC
> can cross with 256 bytes at most two pages.
> 
> In case we get a fault on the second page, we already moved data. When
> continuing after the fault we might try to move already overwritten data,
> which is very bad in case we have overlapping data on a forward move.
> 
> Triggered for now only by rpmbuild (crashes when checking the spec file)
> and rpm (database corruptions). This fixes installing Fedora rawhide (31)
> under TCG.
> 
> This was horrible to debug as it barely triggers and we fail at completely
> different places.
> 
> Cc: Stefano Brivio <sbrivio@redhat.com>
> Cc: Florian Weimer <fweimer@redhat.com>
> Cc: Dan Horák <dan@danny.cz>
> Cc: Cole Robinson <crobinso@redhat.com>
> 
> v2 -> v3:
> - "s390x/tcg: MVCL: Zero out unused bits of address"
> -- Do single deposit for 24/31-bit
> - "s390x/tcg: MVCL: Process max 4k bytes at a time"
> -- Use max of 4k instead of 2k, limiting to single pages
> - "s390x/tcg: MVCLU/MVCLE: Process max 4k bytes at a time"
> -- Limit to single pages
> - "s390x/tcg: MVCOS: Lengths are 32 bit in 24/31-bit mode"
> -- Added
> - "s390x/tcg: MVCS/MVCP: Properly wrap the length"
> -- Properly use 32 instead of 31 bit.
> - "s390x/tcg: MVST: Fix storing back the addresses to registers"
> -- Read R0 implicitly
> - "s390x/tcg: Fault-safe memset"
> -- Speed up TLB_NOTDIRTY handling
> -- Move single-page access to helper function
> -- Pass access structure to access_memset()
> -- Replace access_prepare() by previous access_prepare_idx()
> - "s390x/tcg: Fault-safe memmove"
> -- Pass access structure to access_memmove()
> -- Speed up TLB_NOTDIRTY handling when accessing single bytes
> - The other fault-safe handling patches were adapted to work with the
>   changed access functions. mmu_idx is now always passed to
>   access_prepare() from the helpers.
> 
> v1 -> v2:
> - Include many fixes
> - Fix more instructions
> - Use the new probe_access() function
> - Include "tests/tcg: target/s390x: Test MVO"
> 
> David Hildenbrand (29):
>   s390x/tcg: Reset exception_index to -1 instead of 0
>   s390x/tcg: MVCL: Zero out unused bits of address
>   s390x/tcg: MVCL: Detect destructive overlaps
>   s390x/tcg: MVCL: Process max 4k bytes at a time
>   s390x/tcg: MVC: Increment the length once
>   s390x/tcg: MVC: Use is_destructive_overlap()
>   s390x/tcg: MVPG: Check for specification exceptions
>   s390x/tcg: MVPG: Properly wrap the addresses
>   s390x/tcg: MVCLU/MVCLE: Process max 4k bytes at a time
>   s390x/tcg: MVCS/MVCP: Check for special operation exceptions
>   s390x/tcg: MVCOS: Lengths are 32 bit in 24/31-bit mode
>   s390x/tcg: MVCS/MVCP: Properly wrap the length
>   s390x/tcg: MVST: Check for specification exceptions
>   s390x/tcg: MVST: Fix storing back the addresses to registers
>   s390x/tcg: Always use MMU_USER_IDX for CONFIG_USER_ONLY
>   s390x/tcg: Fault-safe memset
>   s390x/tcg: Fault-safe memmove
>   s390x/tcg: MVCS/MVCP: Use access_memmove()
>   s390x/tcg: MVC: Fault-safe handling on destructive overlaps
>   s390x/tcg: MVCLU: Fault-safe handling
>   s390x/tcg: OC: Fault-safe handling
>   s390x/tcg: XC: Fault-safe handling
>   s390x/tcg: NC: Fault-safe handling
>   s390x/tcg: MVCIN: Fault-safe handling
>   s390x/tcg: MVN: Fault-safe handling
>   s390x/tcg: MVZ: Fault-safe handling
>   s390x/tcg: MVST: Fault-safe handling
>   s390x/tcg: MVO: Fault-safe handling
>   tests/tcg: target/s390x: Test MVO
> 
>  target/s390x/cpu.h              |   4 +
>  target/s390x/helper.h           |   2 +-
>  target/s390x/insn-data.def      |   2 +-
>  target/s390x/mem_helper.c       | 743 ++++++++++++++++++++++----------
>  target/s390x/translate.c        |  12 +-
>  tests/tcg/s390x/Makefile.target |   1 +
>  tests/tcg/s390x/mvo.c           |  25 ++
>  7 files changed, 564 insertions(+), 225 deletions(-)
>  create mode 100644 tests/tcg/s390x/mvo.c
> 

As long as there are no further comments, this series is ready to go
(only one patch description needs a fixup).

Conny, how do you prefer to upstream this stuff? (remembering that
you'll be on vacation soon).

-- 

Thanks,

David / dhildenb

Re: [Qemu-devel] [PATCH v3 00/29] s390x/tcg: mem_helper: Fault-safe handling
Posted by Cornelia Huck 4 years, 7 months ago
On Wed, 18 Sep 2019 10:25:15 +0200
David Hildenbrand <david@redhat.com> wrote:

> On 16.09.19 15:57, David Hildenbrand wrote:
> > This series fixes a bunch of issues related to some mem helpers and makes
> > sure that they are fault-safe, meaning no system state is modified in case
> > a fault is triggered.
> > 
> > I can spot tons of other issues with other mem helpers that will have
> > to be fixed later. Also, fault-safe handling for some instructions
> > (especially TR) might be harder to implement (you don't know what will
> > actually be accessed upfront - we might need a buffer and go over
> > inputs twice). Focusing on the MOVE instructions for now.
> > 
> > ----
> > 
> > Newer versions of glibc use memcpy() in memmove() for forward moves. The
> > implementation makese use of MVC. The TCG implementation of MVC is
> > currently not able to handle faults reliably when crossing pages. MVC
> > can cross with 256 bytes at most two pages.
> > 
> > In case we get a fault on the second page, we already moved data. When
> > continuing after the fault we might try to move already overwritten data,
> > which is very bad in case we have overlapping data on a forward move.
> > 
> > Triggered for now only by rpmbuild (crashes when checking the spec file)
> > and rpm (database corruptions). This fixes installing Fedora rawhide (31)
> > under TCG.
> > 
> > This was horrible to debug as it barely triggers and we fail at completely
> > different places.
> > 
> > Cc: Stefano Brivio <sbrivio@redhat.com>
> > Cc: Florian Weimer <fweimer@redhat.com>
> > Cc: Dan Horák <dan@danny.cz>
> > Cc: Cole Robinson <crobinso@redhat.com>
> > 
> > v2 -> v3:
> > - "s390x/tcg: MVCL: Zero out unused bits of address"
> > -- Do single deposit for 24/31-bit
> > - "s390x/tcg: MVCL: Process max 4k bytes at a time"
> > -- Use max of 4k instead of 2k, limiting to single pages
> > - "s390x/tcg: MVCLU/MVCLE: Process max 4k bytes at a time"
> > -- Limit to single pages
> > - "s390x/tcg: MVCOS: Lengths are 32 bit in 24/31-bit mode"
> > -- Added
> > - "s390x/tcg: MVCS/MVCP: Properly wrap the length"
> > -- Properly use 32 instead of 31 bit.
> > - "s390x/tcg: MVST: Fix storing back the addresses to registers"
> > -- Read R0 implicitly
> > - "s390x/tcg: Fault-safe memset"
> > -- Speed up TLB_NOTDIRTY handling
> > -- Move single-page access to helper function
> > -- Pass access structure to access_memset()
> > -- Replace access_prepare() by previous access_prepare_idx()
> > - "s390x/tcg: Fault-safe memmove"
> > -- Pass access structure to access_memmove()
> > -- Speed up TLB_NOTDIRTY handling when accessing single bytes
> > - The other fault-safe handling patches were adapted to work with the
> >   changed access functions. mmu_idx is now always passed to
> >   access_prepare() from the helpers.
> > 
> > v1 -> v2:
> > - Include many fixes
> > - Fix more instructions
> > - Use the new probe_access() function
> > - Include "tests/tcg: target/s390x: Test MVO"
> > 
> > David Hildenbrand (29):
> >   s390x/tcg: Reset exception_index to -1 instead of 0
> >   s390x/tcg: MVCL: Zero out unused bits of address
> >   s390x/tcg: MVCL: Detect destructive overlaps
> >   s390x/tcg: MVCL: Process max 4k bytes at a time
> >   s390x/tcg: MVC: Increment the length once
> >   s390x/tcg: MVC: Use is_destructive_overlap()
> >   s390x/tcg: MVPG: Check for specification exceptions
> >   s390x/tcg: MVPG: Properly wrap the addresses
> >   s390x/tcg: MVCLU/MVCLE: Process max 4k bytes at a time
> >   s390x/tcg: MVCS/MVCP: Check for special operation exceptions
> >   s390x/tcg: MVCOS: Lengths are 32 bit in 24/31-bit mode
> >   s390x/tcg: MVCS/MVCP: Properly wrap the length
> >   s390x/tcg: MVST: Check for specification exceptions
> >   s390x/tcg: MVST: Fix storing back the addresses to registers
> >   s390x/tcg: Always use MMU_USER_IDX for CONFIG_USER_ONLY
> >   s390x/tcg: Fault-safe memset
> >   s390x/tcg: Fault-safe memmove
> >   s390x/tcg: MVCS/MVCP: Use access_memmove()
> >   s390x/tcg: MVC: Fault-safe handling on destructive overlaps
> >   s390x/tcg: MVCLU: Fault-safe handling
> >   s390x/tcg: OC: Fault-safe handling
> >   s390x/tcg: XC: Fault-safe handling
> >   s390x/tcg: NC: Fault-safe handling
> >   s390x/tcg: MVCIN: Fault-safe handling
> >   s390x/tcg: MVN: Fault-safe handling
> >   s390x/tcg: MVZ: Fault-safe handling
> >   s390x/tcg: MVST: Fault-safe handling
> >   s390x/tcg: MVO: Fault-safe handling
> >   tests/tcg: target/s390x: Test MVO
> > 
> >  target/s390x/cpu.h              |   4 +
> >  target/s390x/helper.h           |   2 +-
> >  target/s390x/insn-data.def      |   2 +-
> >  target/s390x/mem_helper.c       | 743 ++++++++++++++++++++++----------
> >  target/s390x/translate.c        |  12 +-
> >  tests/tcg/s390x/Makefile.target |   1 +
> >  tests/tcg/s390x/mvo.c           |  25 ++
> >  7 files changed, 564 insertions(+), 225 deletions(-)
> >  create mode 100644 tests/tcg/s390x/mvo.c
> >   
> 
> As long as there are no further comments, this series is ready to go
> (only one patch description needs a fixup).

I don't have any :)

> 
> Conny, how do you prefer to upstream this stuff? (remembering that
> you'll be on vacation soon).

I'll happily process a pull request from you, as long as I can send a
pull request myself on Thu or Fri latest.

Re: [Qemu-devel] [PATCH v3 00/29] s390x/tcg: mem_helper: Fault-safe handling
Posted by David Hildenbrand 4 years, 7 months ago
On 18.09.19 11:26, Cornelia Huck wrote:
> On Wed, 18 Sep 2019 10:25:15 +0200
> David Hildenbrand <david@redhat.com> wrote:
> 
>> On 16.09.19 15:57, David Hildenbrand wrote:
>>> This series fixes a bunch of issues related to some mem helpers and makes
>>> sure that they are fault-safe, meaning no system state is modified in case
>>> a fault is triggered.
>>>
>>> I can spot tons of other issues with other mem helpers that will have
>>> to be fixed later. Also, fault-safe handling for some instructions
>>> (especially TR) might be harder to implement (you don't know what will
>>> actually be accessed upfront - we might need a buffer and go over
>>> inputs twice). Focusing on the MOVE instructions for now.
>>>
>>> ----
>>>
>>> Newer versions of glibc use memcpy() in memmove() for forward moves. The
>>> implementation makese use of MVC. The TCG implementation of MVC is
>>> currently not able to handle faults reliably when crossing pages. MVC
>>> can cross with 256 bytes at most two pages.
>>>
>>> In case we get a fault on the second page, we already moved data. When
>>> continuing after the fault we might try to move already overwritten data,
>>> which is very bad in case we have overlapping data on a forward move.
>>>
>>> Triggered for now only by rpmbuild (crashes when checking the spec file)
>>> and rpm (database corruptions). This fixes installing Fedora rawhide (31)
>>> under TCG.
>>>
>>> This was horrible to debug as it barely triggers and we fail at completely
>>> different places.
>>>
>>> Cc: Stefano Brivio <sbrivio@redhat.com>
>>> Cc: Florian Weimer <fweimer@redhat.com>
>>> Cc: Dan Horák <dan@danny.cz>
>>> Cc: Cole Robinson <crobinso@redhat.com>
>>>
>>> v2 -> v3:
>>> - "s390x/tcg: MVCL: Zero out unused bits of address"
>>> -- Do single deposit for 24/31-bit
>>> - "s390x/tcg: MVCL: Process max 4k bytes at a time"
>>> -- Use max of 4k instead of 2k, limiting to single pages
>>> - "s390x/tcg: MVCLU/MVCLE: Process max 4k bytes at a time"
>>> -- Limit to single pages
>>> - "s390x/tcg: MVCOS: Lengths are 32 bit in 24/31-bit mode"
>>> -- Added
>>> - "s390x/tcg: MVCS/MVCP: Properly wrap the length"
>>> -- Properly use 32 instead of 31 bit.
>>> - "s390x/tcg: MVST: Fix storing back the addresses to registers"
>>> -- Read R0 implicitly
>>> - "s390x/tcg: Fault-safe memset"
>>> -- Speed up TLB_NOTDIRTY handling
>>> -- Move single-page access to helper function
>>> -- Pass access structure to access_memset()
>>> -- Replace access_prepare() by previous access_prepare_idx()
>>> - "s390x/tcg: Fault-safe memmove"
>>> -- Pass access structure to access_memmove()
>>> -- Speed up TLB_NOTDIRTY handling when accessing single bytes
>>> - The other fault-safe handling patches were adapted to work with the
>>>   changed access functions. mmu_idx is now always passed to
>>>   access_prepare() from the helpers.
>>>
>>> v1 -> v2:
>>> - Include many fixes
>>> - Fix more instructions
>>> - Use the new probe_access() function
>>> - Include "tests/tcg: target/s390x: Test MVO"
>>>
>>> David Hildenbrand (29):
>>>   s390x/tcg: Reset exception_index to -1 instead of 0
>>>   s390x/tcg: MVCL: Zero out unused bits of address
>>>   s390x/tcg: MVCL: Detect destructive overlaps
>>>   s390x/tcg: MVCL: Process max 4k bytes at a time
>>>   s390x/tcg: MVC: Increment the length once
>>>   s390x/tcg: MVC: Use is_destructive_overlap()
>>>   s390x/tcg: MVPG: Check for specification exceptions
>>>   s390x/tcg: MVPG: Properly wrap the addresses
>>>   s390x/tcg: MVCLU/MVCLE: Process max 4k bytes at a time
>>>   s390x/tcg: MVCS/MVCP: Check for special operation exceptions
>>>   s390x/tcg: MVCOS: Lengths are 32 bit in 24/31-bit mode
>>>   s390x/tcg: MVCS/MVCP: Properly wrap the length
>>>   s390x/tcg: MVST: Check for specification exceptions
>>>   s390x/tcg: MVST: Fix storing back the addresses to registers
>>>   s390x/tcg: Always use MMU_USER_IDX for CONFIG_USER_ONLY
>>>   s390x/tcg: Fault-safe memset
>>>   s390x/tcg: Fault-safe memmove
>>>   s390x/tcg: MVCS/MVCP: Use access_memmove()
>>>   s390x/tcg: MVC: Fault-safe handling on destructive overlaps
>>>   s390x/tcg: MVCLU: Fault-safe handling
>>>   s390x/tcg: OC: Fault-safe handling
>>>   s390x/tcg: XC: Fault-safe handling
>>>   s390x/tcg: NC: Fault-safe handling
>>>   s390x/tcg: MVCIN: Fault-safe handling
>>>   s390x/tcg: MVN: Fault-safe handling
>>>   s390x/tcg: MVZ: Fault-safe handling
>>>   s390x/tcg: MVST: Fault-safe handling
>>>   s390x/tcg: MVO: Fault-safe handling
>>>   tests/tcg: target/s390x: Test MVO
>>>
>>>  target/s390x/cpu.h              |   4 +
>>>  target/s390x/helper.h           |   2 +-
>>>  target/s390x/insn-data.def      |   2 +-
>>>  target/s390x/mem_helper.c       | 743 ++++++++++++++++++++++----------
>>>  target/s390x/translate.c        |  12 +-
>>>  tests/tcg/s390x/Makefile.target |   1 +
>>>  tests/tcg/s390x/mvo.c           |  25 ++
>>>  7 files changed, 564 insertions(+), 225 deletions(-)
>>>  create mode 100644 tests/tcg/s390x/mvo.c
>>>   
>>
>> As long as there are no further comments, this series is ready to go
>> (only one patch description needs a fixup).
> 
> I don't have any :)
> 
>>
>> Conny, how do you prefer to upstream this stuff? (remembering that
>> you'll be on vacation soon).
> 
> I'll happily process a pull request from you, as long as I can send a
> pull request myself on Thu or Fri latest.
> 

Alright, I'll send on later today. Cheers!

-- 

Thanks,

David / dhildenb