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

[David Hildenbrand <david@redhat.com>]

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