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

Anton Johansson via posted 9 patches 8 months, 4 weeks ago
Patches applied successfully (tree, apply log)
git fetch https://github.com/patchew-project/qemu tags/patchew/20230807155706.9580-1-anjo@rev.ng
Maintainers: Cameron Esfahani <dirty@apple.com>, Roman Bolshakov <rbolshakov@ddn.com>, Paolo Bonzini <pbonzini@redhat.com>, Richard Henderson <richard.henderson@linaro.org>, Alexander Graf <agraf@csgraf.de>, Peter Maydell <peter.maydell@linaro.org>, Marcelo Tosatti <mtosatti@redhat.com>, Daniel Henrique Barboza <danielhb413@gmail.com>, "Cédric Le Goater" <clg@kaod.org>, David Gibson <david@gibson.dropbear.id.au>, Greg Kurz <groug@kaod.org>, Nicholas Piggin <npiggin@gmail.com>, Palmer Dabbelt <palmer@dabbelt.com>, Alistair Francis <alistair.francis@wdc.com>, Bin Meng <bin.meng@windriver.com>, Weiwei Li <liweiwei@iscas.ac.cn>, Liu Zhiwei <zhiwei_liu@linux.alibaba.com>, Yoshinori Sato <ysato@users.sourceforge.jp>, Halil Pasic <pasic@linux.ibm.com>, Christian Borntraeger <borntraeger@linux.ibm.com>, David Hildenbrand <david@redhat.com>, Ilya Leoshkevich <iii@linux.ibm.com>, Thomas Huth <thuth@redhat.com>
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(-)
[PATCH 0/9] Replace remaining target_ulong in system-mode accel
Posted by Anton Johansson via 8 months, 4 weeks ago
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
Posted by Richard Henderson 8 months, 3 weeks ago
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
Posted by Michael Tokarev 7 months, 1 week ago
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
Posted by Anton Johansson via 7 months, 1 week ago
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
Posted by Michael Tokarev 7 months, 1 week ago
[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
Posted by Anton Johansson via 7 months, 1 week ago
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