Re: [PATCH 1/8] accel: Replace `target_ulong` with `vaddr` in TB/TLB

[Anton Johansson]

On 4/23/23 11:09, Richard Henderson wrote:
> On 4/20/23 22:28, Anton Johansson wrote:
> > Changes pc and cs_base in TranslationBlock from target_ulong to vaddr.
> > Auxilliary structs and tb_*()/tlb_*() functions that depend on this
> > change are also updated to take a vaddr for guest virtual addresses.
> >
> > Signed-off-by: Anton Johansson <anjo@rev.ng>
> > ---
> >   accel/stubs/tcg-stub.c       |   2 +-
> >   accel/tcg/cpu-exec.c         |  49 +++++-----
> >   accel/tcg/cputlb.c           | 179 +++++++++++++++++------------------
> >   accel/tcg/internal.h         |   6 +-
> >   accel/tcg/tb-hash.h          |  12 +--
> >   accel/tcg/tb-jmp-cache.h     |   2 +-
> >   accel/tcg/tb-maint.c         |   2 +-
> >   accel/tcg/translate-all.c    |  15 +--
> >   include/exec/cpu-defs.h      |   4 +-
> >   include/exec/cpu_ldst.h      |   6 +-
> >   include/exec/exec-all.h      |  82 ++++++++--------
> >   include/qemu/plugin-memory.h |   2 +-
> >   12 files changed, 181 insertions(+), 180 deletions(-)
>
> This is too large and must be split.  In addition, there are places 
> where you must take more care with the replacement.

Agreed this patch is very large. I struggled a bit in chopping it up 
without breaking bisection, but I'll give it another go!


> > @@ -412,10 +412,11 @@ const void *HELPER(lookup_tb_ptr)(CPUArchState 
> > *env)
> >   {
> >       CPUState *cpu = env_cpu(env);
> >       TranslationBlock *tb;
> > -    target_ulong cs_base, pc;
> > +    vaddr cs_base = 0, pc = 0;
> >       uint32_t flags, cflags;
> >   -    cpu_get_tb_cpu_state(env, &pc, &cs_base, &flags);
> > +    cpu_get_tb_cpu_state(env, (target_ulong *) &pc,
> > +                         (target_ulong *) &cs_base, &flags);
>
> This simply will not work on big-endian hosts.
Ah of course! I'll pull in the changes updating the arguments of 
cpu_get_tb_cpu_state() to
avoid the pointer casts.


> > @@ -560,15 +557,15 @@ static void 
> > tlb_flush_page_by_mmuidx_async_0(CPUState *cpu,
> >   static void tlb_flush_page_by_mmuidx_async_1(CPUState *cpu,
> >                                                run_on_cpu_data data)
> >   {
> > -    target_ulong addr_and_idxmap = (target_ulong) data.target_ptr;
> > -    target_ulong addr = addr_and_idxmap & TARGET_PAGE_MASK;
> > +    vaddr addr_and_idxmap = (vaddr) data.target_ptr;
>
> run_on_cpu_data.target_ptr is already vaddr, no need for cast.

Right, will fix! Thanks


--
Anton Johansson,
rev.ng Labs Srl.