Re: [PATCH 0/9] Replace remaining target_ulong in system-mode accel

Re: [PATCH 0/9] Replace remaining target_ulong in system-mode accel

[timothee.cocault]

Hi,

Maybe its too late for the 8.1 window, but I noticed that this patchset
fixes a segfault in qemu-system-ppc (and other 32 bits archs ?) introduced by
commit fb2c53c.
Therefore maybe it would be relevant to get merge it before 8.2.

I put the details below, please tell me if you prefer I file a bug.


If the guest accesses memory accross the 32-bits boundary (eg: fetching a dword
at 0xffffffff), do_ld4_mmu will make two calls to do_ld_beN (one for
0xffffffff, the other for 0x100000000).

In the second call, mmu_lookup1 will call tlb_hit(tlb_addr, addr) to see check
if the address is already in the TLB. If the first page is loaded, this
will result in a call to tlb_hit(0, 0x100000000) which returns 0,
telling it wrongly that the address belongs in page 0.
data->haddr will then be set to an out-of-bounds address.

The Patch 7/9 "include/exec: Widen tlb_hit/tlb_hit_page()" fixes that.


Example crash:

# asm dump
# 0x04: 3820ffff   li r1, -1
# 0x08: 81210000   lwz r9, 0(r1)
$ ./build/qemu-system-ppc -device loader,addr=4,data=0x3820ffff81210000,data-len=8,data-be=true -device loader,addr=0x4,cpu-num=0

Thread 3 "qemu-system-ppc" received signal SIGSEGV, Segmentation fault.
#0  0x0000555555cbf8c7 in do_ld_bytes_beN (p=0x7ffff6c4a2a0, ret_be=0x25) at ../accel/tcg/cputlb.c:2141
#1  0x0000555555cbfe41 in do_ld_beN (env=0x55555684faa0, p=0x7ffff6c4a2a0, ret_be=0x25, mmu_idx=0x3, type=MMU_DATA_LOAD, mop=MO_BEUL, ra=0x7fff7000014d) at ../accel/tcg/cputlb.c:2302
#2  0x0000555555cc088b in do_ld4_mmu (env=0x55555684faa0, addr=0xffffffff, oi=0x123, ra=0x7fff7000014d, access_type=MMU_DATA_LOAD) at ../accel/tcg/cputlb.c:2505
#3  0x0000555555cc092b in helper_ldul_mmu (env=0x55555684faa0, addr=0xffffffff, oi=0x123, retaddr=0x7fff7000014d) at ../accel/tcg/cputlb.c:2516
#4  0x00007fff70000190 in code_gen_buffer ()
#5  0x0000555555cab186 in cpu_tb_exec (cpu=0x55555684d2d0, itb=0x7fffb0000040, tb_exit=0x7ffff6c4a8b0) at ../accel/tcg/cpu-exec.c:457
#6  0x0000555555cabeaf in cpu_loop_exec_tb (cpu=0x55555684d2d0, tb=0x7fffb0000040, pc=0x4, last_tb=0x7ffff6c4a8c0, tb_exit=0x7ffff6c4a8b0) at ../accel/tcg/cpu-exec.c:919
#7  0x0000555555cac219 in cpu_exec_loop (cpu=0x55555684d2d0, sc=0x7ffff6c4a940) at ../accel/tcg/cpu-exec.c:1040
#8  0x0000555555cac2d7 in cpu_exec_setjmp (cpu=0x55555684d2d0, sc=0x7ffff6c4a940) at ../accel/tcg/cpu-exec.c:1057
#9  0x0000555555cac35e in cpu_exec (cpu=0x55555684d2d0) at ../accel/tcg/cpu-exec.c:1083
#10 0x0000555555ccb9a0 in tcg_cpus_exec (cpu=0x55555684d2d0) at ../accel/tcg/tcg-accel-ops.c:75
#11 0x0000555555cccef5 in rr_cpu_thread_fn (arg=0x55555684d2d0) at ../accel/tcg/tcg-accel-ops-rr.c:261
#12 0x0000555555e8d661 in qemu_thread_start (args=0x5555568b6310) at ../util/qemu-thread-posix.c:541
#13 0x00007ffff7a8c9eb in start_thread (arg=<optimized out>) at pthread_create.c:444
#14 0x00007ffff7b10dfc in clone3 () at ../sysdeps/unix/sysv/linux/x86_64/clone3.S:78

Regards,
Timothée.

Re: [PATCH 0/9] Replace remaining target_ulong in system-mode accel

[Michael Tokarev]

22.08.2023 22:02, timothee.cocault@gmail.com wrote:
> Hi,
> 
> Maybe its too late for the 8.1 window, but I noticed that this patchset
> fixes a segfault in qemu-system-ppc (and other 32 bits archs ?) introduced by
> commit fb2c53c.
> Therefore maybe it would be relevant to get merge it before 8.2.

It's definitely too later for 8.1.0 (which has been tagged earlier today,
and it has been too later at -rc4 already).  But it's not too late for
8.1.1 stable series, so a fix can be applied to stable-8.1 (Cc'ing
qemu-stable@).

It seems I can reproduce the crash.

Thanks,

/mjt

Re: [PATCH 0/9] Replace remaining target_ulong in system-mode accel

[Michael Tokarev]

22.08.2023 22:33, Michael Tokarev wrote:
> 22.08.2023 22:02, timothee.cocault@gmail.com wrote:
>> Hi,
>>
>> Maybe its too late for the 8.1 window, but I noticed that this patchset
>> fixes a segfault in qemu-system-ppc (and other 32 bits archs ?) introduced by
>> commit fb2c53c.
>> Therefore maybe it would be relevant to get merge it before 8.2.
> 
> It's definitely too later for 8.1.0 (which has been tagged earlier today,
> and it has been too later at -rc4 already).  But it's not too late for
> 8.1.1 stable series, so a fix can be applied to stable-8.1 (Cc'ing
> qemu-stable@).
> 
> It seems I can reproduce the crash.

The sigsegv (reported testcase) seems to be fixed by commit 7/9:

Author: Anton Johansson via <qemu-devel@nongnu.org>
Date:   Mon Aug 7 17:57:04 2023 +0200

     include/exec: Widen tlb_hit/tlb_hit_page()

     tlb_addr is changed from target_ulong to uint64_t to match the type of
     a CPUTLBEntry value, and the addressed is changed to vaddr.

     Signed-off-by: Anton Johansson <anjo@rev.ng>
     Reviewed-by: Richard Henderson <richard.henderson@linaro.org>

/mjt

[PATCH 0/9] Replace remaining target_ulong in system-mode accel

[Anton Johansson via]

This patchset replaces the remaining uses of target_ulong in the accel/
directory.  Specifically, the address type of a few kvm/hvf functions
is widened to vaddr, and the address type of the cpu_[st|ld]*()
functions is changed to abi_ptr (which is re-typedef'd to vaddr in
system mode).

As a starting point, my goal is to be able to build cputlb.c once for
system mode, and this is a step in that direction by reducing the
target-dependence of accel/.

* Changes in v2:
    - Removed explicit target_ulong casts from 3rd and 4th patches.

Anton Johansson (9):
  accel/kvm: Widen pc/saved_insn for kvm_sw_breakpoint
  accel/hvf: Widen pc/saved_insn for hvf_sw_breakpoint
  target: Use vaddr for kvm_arch_[insert|remove]_hw_breakpoint
  target: Use vaddr for hvf_arch_[insert|remove]_hw_breakpoint
  Replace target_ulong with abi_ptr in cpu_[st|ld]*()
  include/exec: typedef abi_ptr to vaddr in softmmu
  include/exec: Widen tlb_hit/tlb_hit_page()
  accel/tcg: Widen address arg. in tlb_compare_set()
  accel/tcg: Update run_on_cpu_data static assert

 accel/tcg/atomic_template.h  | 16 ++++++++--------
 include/exec/cpu-all.h       |  4 ++--
 include/exec/cpu_ldst.h      | 28 ++++++++++++++--------------
 include/sysemu/hvf.h         | 12 +++++-------
 include/sysemu/kvm.h         | 12 +++++-------
 accel/hvf/hvf-accel-ops.c    |  4 ++--
 accel/hvf/hvf-all.c          |  2 +-
 accel/kvm/kvm-all.c          |  3 +--
 accel/tcg/cputlb.c           | 17 +++++++++--------
 target/arm/hvf/hvf.c         |  4 ++--
 target/arm/kvm64.c           |  6 ++----
 target/i386/hvf/hvf.c        |  4 ++--
 target/i386/kvm/kvm.c        |  8 +++-----
 target/ppc/kvm.c             | 13 ++++++-------
 target/riscv/vector_helper.c |  2 +-
 target/rx/op_helper.c        |  6 +++---
 target/s390x/kvm/kvm.c       |  6 ++----
 17 files changed, 68 insertions(+), 79 deletions(-)

--
2.41.0

Re: [PATCH 0/9] Replace remaining target_ulong in system-mode accel

[Richard Henderson]

On 8/7/23 08:56, Anton Johansson wrote:
> This patchset replaces the remaining uses of target_ulong in the accel/
> directory.  Specifically, the address type of a few kvm/hvf functions
> is widened to vaddr, and the address type of the cpu_[st|ld]*()
> functions is changed to abi_ptr (which is re-typedef'd to vaddr in
> system mode).
> 
> As a starting point, my goal is to be able to build cputlb.c once for
> system mode, and this is a step in that direction by reducing the
> target-dependence of accel/.
> 
> * Changes in v2:
>      - Removed explicit target_ulong casts from 3rd and 4th patches.

Queued to for 8.2.


r~

Re: [PATCH 0/9] Replace remaining target_ulong in system-mode accel

[Michael Tokarev]

07.08.2023 18:56, Anton Johansson via wrote:
> This patchset replaces the remaining uses of target_ulong in the accel/
> directory.  Specifically, the address type of a few kvm/hvf functions
> is widened to vaddr, and the address type of the cpu_[st|ld]*()
> functions is changed to abi_ptr (which is re-typedef'd to vaddr in
> system mode).
> 
> As a starting point, my goal is to be able to build cputlb.c once for
> system mode, and this is a step in that direction by reducing the
> target-dependence of accel/.
> 
> * Changes in v2:
>      - Removed explicit target_ulong casts from 3rd and 4th patches.
> 
> Anton Johansson (9):
>    accel/kvm: Widen pc/saved_insn for kvm_sw_breakpoint
>    accel/hvf: Widen pc/saved_insn for hvf_sw_breakpoint
>    target: Use vaddr for kvm_arch_[insert|remove]_hw_breakpoint
>    target: Use vaddr for hvf_arch_[insert|remove]_hw_breakpoint
>    Replace target_ulong with abi_ptr in cpu_[st|ld]*()
>    include/exec: typedef abi_ptr to vaddr in softmmu
>    include/exec: Widen tlb_hit/tlb_hit_page()
>    accel/tcg: Widen address arg. in tlb_compare_set()
>    accel/tcg: Update run_on_cpu_data static assert

Pinging a relatively old patchset, - which fixes from here needs to
go to stable-8.1?

The context: https://lore.kernel.org/qemu-devel/20230721205827.7502-1-anjo@rev.ng/
And according to this email:

https://lore.kernel.org/qemu-devel/00e9e08eae1004ef67fe8dca3aaf5043e6863faa.camel@gmail.com/

at least "include/exec: Widen tlb_hit/tlb_hit_page()" should go to 8.1,
something else?

Thanks,

/mjt

Re: [PATCH 0/9] Replace remaining target_ulong in system-mode accel

[Anton Johansson via]

On 21/09/23, Michael Tokarev wrote:
> 07.08.2023 18:56, Anton Johansson via wrote:
> > This patchset replaces the remaining uses of target_ulong in the accel/
> > directory.  Specifically, the address type of a few kvm/hvf functions
> > is widened to vaddr, and the address type of the cpu_[st|ld]*()
> > functions is changed to abi_ptr (which is re-typedef'd to vaddr in
> > system mode).
> > 
> > As a starting point, my goal is to be able to build cputlb.c once for
> > system mode, and this is a step in that direction by reducing the
> > target-dependence of accel/.
> > 
> > * Changes in v2:
> >      - Removed explicit target_ulong casts from 3rd and 4th patches.
> > 
> > Anton Johansson (9):
> >    accel/kvm: Widen pc/saved_insn for kvm_sw_breakpoint
> >    accel/hvf: Widen pc/saved_insn for hvf_sw_breakpoint
> >    target: Use vaddr for kvm_arch_[insert|remove]_hw_breakpoint
> >    target: Use vaddr for hvf_arch_[insert|remove]_hw_breakpoint
> >    Replace target_ulong with abi_ptr in cpu_[st|ld]*()
> >    include/exec: typedef abi_ptr to vaddr in softmmu
> >    include/exec: Widen tlb_hit/tlb_hit_page()
> >    accel/tcg: Widen address arg. in tlb_compare_set()
> >    accel/tcg: Update run_on_cpu_data static assert
> 
> Pinging a relatively old patchset, - which fixes from here needs to
> go to stable-8.1?
> 
> The context: https://lore.kernel.org/qemu-devel/20230721205827.7502-1-anjo@rev.ng/
> And according to this email:
> 
> https://lore.kernel.org/qemu-devel/00e9e08eae1004ef67fe8dca3aaf5043e6863faa.camel@gmail.com/
> 
> at least "include/exec: Widen tlb_hit/tlb_hit_page()" should go to 8.1,
> something else?
> 
> Thanks,
> 
> /mjt

If the patch above is the only one needed to fix the segfault (haven't
tested myself), pulling it in isolation is fine as it doesn't depend on 
any of the other patches.

The rest of the patches can be delayed without issue.

-- 
Anton Johansson
rev.ng Labs Srl.

Re: [PATCH 0/9] Replace remaining target_ulong in system-mode accel

[Michael Tokarev]

[Trimming Cc list]

22.09.2023 13:45, Anton Johansson:
> On 21/09/23, Michael Tokarev wrote:

>>> Anton Johansson (9):
>>>     accel/kvm: Widen pc/saved_insn for kvm_sw_breakpoint
>>>     accel/hvf: Widen pc/saved_insn for hvf_sw_breakpoint
>>>     target: Use vaddr for kvm_arch_[insert|remove]_hw_breakpoint
>>>     target: Use vaddr for hvf_arch_[insert|remove]_hw_breakpoint
>>>     Replace target_ulong with abi_ptr in cpu_[st|ld]*()
>>>     include/exec: typedef abi_ptr to vaddr in softmmu
>>>     include/exec: Widen tlb_hit/tlb_hit_page()
>>>     accel/tcg: Widen address arg. in tlb_compare_set()
>>>     accel/tcg: Update run_on_cpu_data static assert
>>
>> Pinging a relatively old patchset, - which fixes from here needs to
>> go to stable-8.1?
...
> The rest of the patches can be delayed without issue.

Now I'm confused.  What do you mean "delayed"?
Should the rest be picked up for 8.1.2 or 8.1.3 or maybe 8.1.4?

The whole series has been accepted/applied to master, I asked which
changes should be picked up for stable-8.1, - there's no delay involved,
it is either picked up or not, either needed in stable or not.

Yes, "Widen tlb_hit/tlb_hit_page()" fixes a known bug and I picked
up that one, - unfortunately it missed 8.1.1 release.  The question
is about the other changes in this patch set, - do they fix other
similar, not yet discovered, bugs in other places, or not fixing
anything? Or should we delay picking them up until a bug is reported? :)

Thank you for the changes and the reply!

/mjt

Re: [PATCH 0/9] Replace remaining target_ulong in system-mode accel

[Anton Johansson via]

On 23/09/23, Michael Tokarev wrote:
> [Trimming Cc list]
> 
> 22.09.2023 13:45, Anton Johansson:
> > On 21/09/23, Michael Tokarev wrote:
> 
> > > > Anton Johansson (9):
> > > >     accel/kvm: Widen pc/saved_insn for kvm_sw_breakpoint
> > > >     accel/hvf: Widen pc/saved_insn for hvf_sw_breakpoint
> > > >     target: Use vaddr for kvm_arch_[insert|remove]_hw_breakpoint
> > > >     target: Use vaddr for hvf_arch_[insert|remove]_hw_breakpoint
> > > >     Replace target_ulong with abi_ptr in cpu_[st|ld]*()
> > > >     include/exec: typedef abi_ptr to vaddr in softmmu
> > > >     include/exec: Widen tlb_hit/tlb_hit_page()
> > > >     accel/tcg: Widen address arg. in tlb_compare_set()
> > > >     accel/tcg: Update run_on_cpu_data static assert
> > > 
> > > Pinging a relatively old patchset, - which fixes from here needs to
> > > go to stable-8.1?
> ...
> > The rest of the patches can be delayed without issue.
> 
> Now I'm confused.  What do you mean "delayed"?
> Should the rest be picked up for 8.1.2 or 8.1.3 or maybe 8.1.4?
> 
> The whole series has been accepted/applied to master, I asked which
> changes should be picked up for stable-8.1, - there's no delay involved,
> it is either picked up or not, either needed in stable or not.
> 
> Yes, "Widen tlb_hit/tlb_hit_page()" fixes a known bug and I picked
> up that one, - unfortunately it missed 8.1.1 release.  The question
> is about the other changes in this patch set, - do they fix other
> similar, not yet discovered, bugs in other places, or not fixing
> anything? Or should we delay picking them up until a bug is reported? :)
> 
> Thank you for the changes and the reply!
> 
> /mjt

Oh I see what you mean now, thanks for the clarification!:) I'm not that 
used to think in terms of what patches end up in stable.

The other patches in this series are refactors to reduce 
target-dependence in accel/, and they do not fix any bugs directly. 
Eventually we'll need to pick them up for compiling cputlb.c once for 
system mode etc., or other patches that depend on the refactor, but they 
are not critical to get in due to fixing some bug, that's what I meant 
by "can be delayed without issue".

How do you usually deal with these types of refactor heavy changes? 
Picking asap vs delaying until absolutely needed?

Thanks again for the explanation, I hope this was of some help.

//Anton

[PATCH 0/9] Replace remaining target_ulong in system-mode accel

[Anton Johansson via]

This patchset replaces the remaining uses of target_ulong in the accel/
directory.  Specifically, the address type of a few kvm/hvf functions
is widened to vaddr, and the address type of the cpu_[st|ld]*()
functions is changed to abi_ptr (which is re-typedef'd to vaddr in
system mode).

As a starting point, my goal is to be able to build cputlb.c once for
system mode, and this is a step in that direction by reducing the
target-dependence of accel/.

Anton Johansson (9):
  accel/kvm: Widen pc/saved_insn for kvm_sw_breakpoint
  accel/hvf: Widen pc/saved_insn for hvf_sw_breakpoint
  target: Use vaddr for kvm_arch_[insert|remove]_hw_breakpoint
  target: Use vaddr for hvf_arch_[insert|remove]_hw_breakpoint
  Replace target_ulong with abi_ptr in cpu_[st|ld]*()
  include/exec: typedef abi_ptr to vaddr in softmmu
  include/exec: Widen tlb_hit/tlb_hit_page()
  accel/tcg: Widen address arg. in tlb_compare_set()
  accel/tcg: Update run_on_cpu_data static assert

 accel/tcg/atomic_template.h  | 16 ++++++++--------
 include/exec/cpu-all.h       |  4 ++--
 include/exec/cpu_ldst.h      | 28 ++++++++++++++--------------
 include/sysemu/hvf.h         | 12 +++++-------
 include/sysemu/kvm.h         | 12 +++++-------
 accel/hvf/hvf-accel-ops.c    |  4 ++--
 accel/hvf/hvf-all.c          |  2 +-
 accel/kvm/kvm-all.c          |  3 +--
 accel/tcg/cputlb.c           | 17 +++++++++--------
 target/arm/hvf/hvf.c         | 14 ++++++++------
 target/arm/kvm64.c           | 16 ++++++++--------
 target/i386/hvf/hvf.c        |  4 ++--
 target/i386/kvm/kvm.c        | 15 +++++++--------
 target/ppc/kvm.c             | 15 +++++++--------
 target/riscv/vector_helper.c |  2 +-
 target/rx/op_helper.c        |  6 +++---
 target/s390x/kvm/kvm.c       | 11 +++++------
 17 files changed, 88 insertions(+), 93 deletions(-)

--
2.41.0