Re: [PATCH 1/2] target/s390x: Fix MVC not always invalidating translation blocks

[Ilya Leoshkevich]

On Tue, 2025-01-28 at 10:56 +0100, David Hildenbrand wrote:
> On 28.01.25 01:12, Ilya Leoshkevich wrote:
> > Node.js crashes in qemu-system-s390x with random SIGSEGVs /
> > SIGILLs.
> > 
> > The v8 JIT used by Node.js can garbage collect and overwrite unused
> > code. Overwriting is performed by
> > WritableJitAllocation::CopyCode(),
> > which ultimately calls memcpy(). For certain sizes, memcpy() uses
> > the
> > MVC instruction.
> > 
> > QEMU implements MVC and other similar instructions using helpers.
> > While
> > TCG store ops invalidate affected translation blocks automatically,
> > helpers must do this manually by calling probe_access_flags(). The
> > MVC
> > helper does this using the access_prepare() -> access_prepare_nf()
> > ->
> > s390_probe_access() -> probe_access_flags() call chain.
> > 
> > At the last step of this chain, the store size is replaced with 0.
> > This
> > causes the probe_access_flags() -> notdirty_write() ->
> > tb_invalidate_phys_range_fast() chain to miss some translation
> > blocks.
> 
> We added the size parameter in:
> 
> commit 1770b2f2d3d6fe8f1e2d61692692264cac44340d
> Author: Daniel Henrique Barboza <dbarboza@ventanamicro.com>
> Date:   Thu Feb 23 20:44:24 2023 -0300
> 
>      accel/tcg: Add 'size' param to probe_access_flags()
>      
>      probe_access_flags() as it is today uses probe_access_full(),
> which in
>      turn uses probe_access_internal() with size = 0.
> probe_access_internal()
>      then uses the size to call the tlb_fill() callback for the given
> CPU.
>      This size param ('fault_size' as probe_access_internal() calls
> it) is
>      ignored by most existing .tlb_fill callback implementations,
> e.g.
>      arm_cpu_tlb_fill(), ppc_cpu_tlb_fill(), x86_cpu_tlb_fill() and
>      mips_cpu_tlb_fill() to name a few.
> 
> 
> And added support for more than one byte for the notdirty case in
> 
> commit e2faabee78ff127848f59892747d4c07c56de033
> Author: Jessica Clarke <jrtc27@jrtc27.com>
> Date:   Fri Nov 10 21:43:03 2023 -0800
> 
>      accel/tcg: Forward probe size on to notdirty_write
>      
>      Without this, we just dirty a single byte, and so if the caller
> writes
>      more than one byte to the host memory then we won't have
> invalidated any
>      translation blocks that start after the first byte and overlap
> those
>      writes.
> 
> So I guess it's rather hard to find a "Fixes:" tag, because likely
> it's been
> broken forever?

Yes, I thought about this too and gave up.
However, I now wonder if we still need one for non-philosophical, but
rather practical backporting reasons? Then

Fixes: e2faabee78ff ("accel/tcg: Forward probe size on to
notdirty_write")

(v8.2.0+) should be the right one I think?

> > When this happens, QEMU executes a mix of old and new code. This
> > quickly leads to either a SIGSEGV or a SIGILL in case the old code
> > ends in the middle of a new instruction.
> > 
> > Fix by passing the true size.
> 
> 
> Reviewed-by: David Hildenbrand <david@redhat.com>