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

[Cornelia Huck <cohuck@redhat.com>]

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.