Re: [PATCH 0/9] Replace remaining target_ulong in system-mode accel
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
Re: [PATCH 0/9] Replace remaining target_ulong in system-mode accel
[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
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-a...@rev.ng/ > And according to this email: > > https://lore.kernel.org/qemu-devel/00e9e08eae1004ef67fe8dca3aaf5043e6863faa.ca...@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
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-a...@rev.ng/ And according to this email: https://lore.kernel.org/qemu-devel/00e9e08eae1004ef67fe8dca3aaf5043e6863faa.ca...@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
22.08.2023 22:33, Michael Tokarev wrote: 22.08.2023 22:02, timothee.coca...@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 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 Reviewed-by: Richard Henderson /mjt
Re: [PATCH 0/9] Replace remaining target_ulong in system-mode accel
22.08.2023 22:02, timothee.coca...@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
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 0x), do_ld4_mmu will make two calls to do_ld_beN (one for 0x, the other for 0x1). 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, 0x1) 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: 3820 li r1, -1 # 0x08: 8121 lwz r9, 0(r1) $ ./build/qemu-system-ppc -device loader,addr=4,data=0x38208121,data-len=8,data-be=true -device loader,addr=0x4,cpu-num=0 Thread 3 "qemu-system-ppc" received signal SIGSEGV, Segmentation fault. #0 0x55cbf8c7 in do_ld_bytes_beN (p=0x76c4a2a0, ret_be=0x25) at ../accel/tcg/cputlb.c:2141 #1 0x55cbfe41 in do_ld_beN (env=0x5684faa0, p=0x76c4a2a0, ret_be=0x25, mmu_idx=0x3, type=MMU_DATA_LOAD, mop=MO_BEUL, ra=0x7fff714d) at ../accel/tcg/cputlb.c:2302 #2 0x55cc088b in do_ld4_mmu (env=0x5684faa0, addr=0x, oi=0x123, ra=0x7fff714d, access_type=MMU_DATA_LOAD) at ../accel/tcg/cputlb.c:2505 #3 0x55cc092b in helper_ldul_mmu (env=0x5684faa0, addr=0x, oi=0x123, retaddr=0x7fff714d) at ../accel/tcg/cputlb.c:2516 #4 0x7fff7190 in code_gen_buffer () #5 0x55cab186 in cpu_tb_exec (cpu=0x5684d2d0, itb=0x7fffb040, tb_exit=0x76c4a8b0) at ../accel/tcg/cpu-exec.c:457 #6 0x55cabeaf in cpu_loop_exec_tb (cpu=0x5684d2d0, tb=0x7fffb040, pc=0x4, last_tb=0x76c4a8c0, tb_exit=0x76c4a8b0) at ../accel/tcg/cpu-exec.c:919 #7 0x55cac219 in cpu_exec_loop (cpu=0x5684d2d0, sc=0x76c4a940) at ../accel/tcg/cpu-exec.c:1040 #8 0x55cac2d7 in cpu_exec_setjmp (cpu=0x5684d2d0, sc=0x76c4a940) at ../accel/tcg/cpu-exec.c:1057 #9 0x55cac35e in cpu_exec (cpu=0x5684d2d0) at ../accel/tcg/cpu-exec.c:1083 #10 0x55ccb9a0 in tcg_cpus_exec (cpu=0x5684d2d0) at ../accel/tcg/tcg-accel-ops.c:75 #11 0x55cccef5 in rr_cpu_thread_fn (arg=0x5684d2d0) at ../accel/tcg/tcg-accel-ops-rr.c:261 #12 0x55e8d661 in qemu_thread_start (args=0x568b6310) at ../util/qemu-thread-posix.c:541 #13 0x77a8c9eb in start_thread (arg=) at pthread_create.c:444 #14 0x77b10dfc 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
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~
