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

[David Hildenbrand <david@redhat.com>]

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