Re: Crash with CXL + TCG on 8.2: Was Re: qemu cxl memory expander shows numa_node -1

[Jonathan Cameron <Jonathan.Cameron@huawei.com>]

On Thu, 1 Feb 2024 13:56:09 -0500
Gregory Price <gregory.price@memverge.com> wrote:

> On Thu, Feb 01, 2024 at 06:04:26PM +0000, Peter Maydell wrote:
> > On Thu, 1 Feb 2024 at 17:25, Alex Bennée <alex.bennee@linaro.org> wrote:  
> > >
> > > Jonathan Cameron <Jonathan.Cameron@Huawei.com> writes:  
> > > >> > #21 0x0000555555ca3e5d in do_st8_mmu (cpu=0x5555578e0cb0, addr=23937, val=18386491784638059520, oi=6, ra=140736029817822) at ../../accel/tcg/cputlb.c:2853
> > > >> > #22 0x00007fffa9107c63 in code_gen_buffer ()  
> > > >>
> > > >> No thats different - we are actually writing to the MMIO region here.
> > > >> But the fact we hit cpu_abort because we can't find the TB we are
> > > >> executing is a little problematic.
> > > >>
> > > >> Does ra properly point to the code buffer here?  
> > > >
> > > > Err.  How would I tell?  
> > >
> > > (gdb) p/x 140736029817822
> > > $1 = 0x7fffa9107bde
> > >
> > > seems off because code_gen_buffer starts at 0x00007fffa9107c63  
> > 
> > The code_gen_buffer doesn't *start* at 0x00007fffa9107c63 --
> > that is our return address into it, which is to say it's just
> > after the call insn to the do_st8_mmu helper. The 'ra' argument
> > to the helper function is going to be a number slightly lower
> > than that, because it points within the main lump of generated
> > code for the TB, whereas the helper call is done as part of
> > an out-of-line lump of common code at the end of the TB.
> > 
> > The 'ra' here is fine -- the problem is that we don't
> > pass it all the way down the callstack and instead end
> > up using 0 as a 'ra' within the ptw code.
> > 
> > -- PMM  
> 
> Is there any particular reason not to, as below?
> ~Gregory
> 
One patch blindly applied. 
New exciting trace...
Thread 5 "qemu-system-x86" received signal SIGABRT, Aborted.
[Switching to Thread 0x7ffff4efe6c0 (LWP 16503)]
__pthread_kill_implementation (no_tid=0, signo=6, threadid=<optimized out>) at ./nptl/pthread_kill.c:44
Download failed: Invalid argument.  Continuing without source file ./nptl/./nptl/pthread_kill.c.
44      ./nptl/pthread_kill.c: No such file or directory.
(gdb) bt
#0  __pthread_kill_implementation (no_tid=0, signo=6, threadid=<optimized out>) at ./nptl/pthread_kill.c:44
#1  __pthread_kill_internal (signo=6, threadid=<optimized out>) at ./nptl/pthread_kill.c:78
#2  __GI___pthread_kill (threadid=<optimized out>, signo=signo@entry=6) at ./nptl/pthread_kill.c:89
#3  0x00007ffff77c43b6 in __GI_raise (sig=sig@entry=6) at ../sysdeps/posix/raise.c:26
#4  0x00007ffff77aa87c in __GI_abort () at ./stdlib/abort.c:79
#5  0x00007ffff7b2ed1e in  () at /lib/x86_64-linux-gnu/libglib-2.0.so.0
#6  0x00007ffff7b9622e in g_assertion_message_expr () at /lib/x86_64-linux-gnu/libglib-2.0.so.0
#7  0x0000555555ab1929 in bql_lock_impl (file=0x555556049122 "../../accel/tcg/cputlb.c", line=2033) at ../../system/cpus.c:524
#8  bql_lock_impl (file=file@entry=0x555556049122 "../../accel/tcg/cputlb.c", line=line@entry=2033) at ../../system/cpus.c:520
#9  0x0000555555c9f7d6 in do_ld_mmio_beN (cpu=0x5555578e0cb0, full=0x7ffe88012950, ret_be=ret_be@entry=0, addr=19595792376, size=size@entry=8, mmu_idx=4, type=MMU_DATA_LOAD, ra=0) at ../../accel/tcg/cputlb.c:2033
#10 0x0000555555ca0fbd in do_ld_8 (cpu=cpu@entry=0x5555578e0cb0, p=p@entry=0x7ffff4efd1d0, mmu_idx=<optimized out>, type=type@entry=MMU_DATA_LOAD, memop=<optimized out>, ra=ra@entry=0) at ../../accel/tcg/cputlb.c:2356
#11 0x0000555555ca341f in do_ld8_mmu (cpu=cpu@entry=0x5555578e0cb0, addr=addr@entry=19595792376, oi=oi@entry=52, ra=0, ra@entry=52, access_type=access_type@entry=MMU_DATA_LOAD) at ../../accel/tcg/cputlb.c:2439
#12 0x0000555555ca5f59 in cpu_ldq_mmu (ra=52, oi=52, addr=19595792376, env=0x5555578e3470) at ../../accel/tcg/ldst_common.c.inc:169
#13 cpu_ldq_le_mmuidx_ra (env=0x5555578e3470, addr=19595792376, mmu_idx=<optimized out>, ra=ra@entry=0) at ../../accel/tcg/ldst_common.c.inc:301
#14 0x0000555555b4b5fc in ptw_ldq (ra=0, in=0x7ffff4efd320) at ../../target/i386/tcg/sysemu/excp_helper.c:98
#15 ptw_ldq (ra=0, in=0x7ffff4efd320) at ../../target/i386/tcg/sysemu/excp_helper.c:93
#16 mmu_translate (env=env@entry=0x5555578e3470, in=0x7ffff4efd3e0, out=0x7ffff4efd3b0, err=err@entry=0x7ffff4efd3c0, ra=ra@entry=0) at ../../target/i386/tcg/sysemu/excp_helper.c:174
#17 0x0000555555b4c4b3 in get_physical_address (ra=0, err=0x7ffff4efd3c0, out=0x7ffff4efd3b0, mmu_idx=0, access_type=MMU_DATA_LOAD, addr=18446741874686299840, env=0x5555578e3470) at ../../target/i386/tcg/sysemu/excp_helper.c:580
#18 x86_cpu_tlb_fill (cs=0x5555578e0cb0, addr=18446741874686299840, size=<optimized out>, access_type=MMU_DATA_LOAD, mmu_idx=0, probe=<optimized out>, retaddr=0) at ../../target/i386/tcg/sysemu/excp_helper.c:606
#19 0x0000555555ca0ee9 in tlb_fill (retaddr=0, mmu_idx=0, access_type=MMU_DATA_LOAD, size=<optimized out>, addr=18446741874686299840, cpu=0x7ffff4efd540) at ../../accel/tcg/cputlb.c:1315
#20 mmu_lookup1 (cpu=cpu@entry=0x5555578e0cb0, data=data@entry=0x7ffff4efd540, mmu_idx=0, access_type=access_type@entry=MMU_DATA_LOAD, ra=ra@entry=0) at ../../accel/tcg/cputlb.c:1713
#21 0x0000555555ca2c61 in mmu_lookup (cpu=cpu@entry=0x5555578e0cb0, addr=addr@entry=18446741874686299840, oi=oi@entry=32, ra=ra@entry=0, type=type@entry=MMU_DATA_LOAD, l=l@entry=0x7ffff4efd540) at ../../accel/tcg/cputlb.c:1803
#22 0x0000555555ca3165 in do_ld4_mmu (cpu=cpu@entry=0x5555578e0cb0, addr=addr@entry=18446741874686299840, oi=oi@entry=32, ra=ra@entry=0, access_type=access_type@entry=MMU_DATA_LOAD) at ../../accel/tcg/cputlb.c:2416
#23 0x0000555555ca5ef9 in cpu_ldl_mmu (ra=0, oi=32, addr=18446741874686299840, env=0x5555578e3470) at ../../accel/tcg/ldst_common.c.inc:158
#24 cpu_ldl_le_mmuidx_ra (env=env@entry=0x5555578e3470, addr=addr@entry=18446741874686299840, mmu_idx=<optimized out>, ra=ra@entry=0) at ../../accel/tcg/ldst_common.c.inc:294
#25 0x0000555555bb6cdd in do_interrupt64 (is_hw=1, next_eip=18446744072399775809, error_code=0, is_int=0, intno=236, env=0x5555578e3470) at ../../target/i386/tcg/seg_helper.c:889
#26 do_interrupt_all (cpu=cpu@entry=0x5555578e0cb0, intno=236, is_int=is_int@entry=0, error_code=error_code@entry=0, next_eip=next_eip@entry=0, is_hw=is_hw@entry=1) at ../../target/i386/tcg/seg_helper.c:1130
#27 0x0000555555bb87da in do_interrupt_x86_hardirq (env=env@entry=0x5555578e3470, intno=<optimized out>, is_hw=is_hw@entry=1) at ../../target/i386/tcg/seg_helper.c:1162
#28 0x0000555555b5039c in x86_cpu_exec_interrupt (cs=0x5555578e0cb0, interrupt_request=<optimized out>) at ../../target/i386/tcg/sysemu/seg_helper.c:197
#29 0x0000555555c94480 in cpu_handle_interrupt (last_tb=<synthetic pointer>, cpu=0x5555578e0cb0) at ../../accel/tcg/cpu-exec.c:844
#30 cpu_exec_loop (cpu=cpu@entry=0x5555578e0cb0, sc=sc@entry=0x7ffff4efd7b0) at ../../accel/tcg/cpu-exec.c:951
#31 0x0000555555c94791 in cpu_exec_setjmp (cpu=cpu@entry=0x5555578e0cb0, sc=sc@entry=0x7ffff4efd7b0) at ../../accel/tcg/cpu-exec.c:1029
#32 0x0000555555c94f7c in cpu_exec (cpu=cpu@entry=0x5555578e0cb0) at ../../accel/tcg/cpu-exec.c:1055
#33 0x0000555555cb9043 in tcg_cpu_exec (cpu=cpu@entry=0x5555578e0cb0) at ../../accel/tcg/tcg-accel-ops.c:76
#34 0x0000555555cb91a0 in mttcg_cpu_thread_fn (arg=arg@entry=0x5555578e0cb0) at ../../accel/tcg/tcg-accel-ops-mttcg.c:95
#35 0x0000555555e57270 in qemu_thread_start (args=0x555557956000) at ../../util/qemu-thread-posix.c:541
#36 0x00007ffff78176ba in start_thread (arg=<optimized out>) at ./nptl/pthread_create.c:444
#37 0x00007ffff78a60d0 in clone3 () at ../sysdeps/unix/sysv/linux/x86_64/clone3.S:81

> 
> diff --git a/target/i386/tcg/sysemu/excp_helper.c b/target/i386/tcg/sysemu/excp_helper.c
> index 5b86f439ad..2f581b9bfb 100644
> --- a/target/i386/tcg/sysemu/excp_helper.c
> +++ b/target/i386/tcg/sysemu/excp_helper.c
> @@ -59,14 +59,14 @@ typedef struct PTETranslate {
>      hwaddr gaddr;
>  } PTETranslate;
> 
> -static bool ptw_translate(PTETranslate *inout, hwaddr addr)
> +static bool ptw_translate(PTETranslate *inout, hwaddr addr, uint64_t ra)
>  {
>      CPUTLBEntryFull *full;
>      int flags;
> 
>      inout->gaddr = addr;
>      flags = probe_access_full(inout->env, addr, 0, MMU_DATA_STORE,
> -                              inout->ptw_idx, true, &inout->haddr, &full, 0);
> +                              inout->ptw_idx, true, &inout->haddr, &full, ra);
> 
>      if (unlikely(flags & TLB_INVALID_MASK)) {
>          TranslateFault *err = inout->err;
> @@ -82,20 +82,20 @@ static bool ptw_translate(PTETranslate *inout, hwaddr addr)
>      return true;
>  }
> 
> -static inline uint32_t ptw_ldl(const PTETranslate *in)
> +static inline uint32_t ptw_ldl(const PTETranslate *in, uint64_t ra)
>  {
>      if (likely(in->haddr)) {
>          return ldl_p(in->haddr);
>      }
> -    return cpu_ldl_mmuidx_ra(in->env, in->gaddr, in->ptw_idx, 0);
> +    return cpu_ldl_mmuidx_ra(in->env, in->gaddr, in->ptw_idx, ra);
>  }
> 
> -static inline uint64_t ptw_ldq(const PTETranslate *in)
> +static inline uint64_t ptw_ldq(const PTETranslate *in, uint64_t ra)
>  {
>      if (likely(in->haddr)) {
>          return ldq_p(in->haddr);
>      }
> -    return cpu_ldq_mmuidx_ra(in->env, in->gaddr, in->ptw_idx, 0);
> +    return cpu_ldq_mmuidx_ra(in->env, in->gaddr, in->ptw_idx, ra);
>  }
> 
>  /*
> @@ -132,7 +132,8 @@ static inline bool ptw_setl(const PTETranslate *in, uint32_t old, uint32_t set)
>  }
> 
>  static bool mmu_translate(CPUX86State *env, const TranslateParams *in,
> -                          TranslateResult *out, TranslateFault *err)
> +                          TranslateResult *out, TranslateFault *err,
> +                          uint64_t ra)
>  {
>      const int32_t a20_mask = x86_get_a20_mask(env);
>      const target_ulong addr = in->addr;
> @@ -166,11 +167,11 @@ static bool mmu_translate(CPUX86State *env, const TranslateParams *in,
>                   */
>                  pte_addr = ((in->cr3 & ~0xfff) +
>                              (((addr >> 48) & 0x1ff) << 3)) & a20_mask;
> -                if (!ptw_translate(&pte_trans, pte_addr)) {
> +                if (!ptw_translate(&pte_trans, pte_addr, ra)) {
>                      return false;
>                  }
>              restart_5:
> -                pte = ptw_ldq(&pte_trans);
> +                pte = ptw_ldq(&pte_trans, ra);
>                  if (!(pte & PG_PRESENT_MASK)) {
>                      goto do_fault;
>                  }
> @@ -191,11 +192,11 @@ static bool mmu_translate(CPUX86State *env, const TranslateParams *in,
>               */
>              pte_addr = ((pte & PG_ADDRESS_MASK) +
>                          (((addr >> 39) & 0x1ff) << 3)) & a20_mask;
> -            if (!ptw_translate(&pte_trans, pte_addr)) {
> +            if (!ptw_translate(&pte_trans, pte_addr, ra)) {
>                  return false;
>              }
>          restart_4:
> -            pte = ptw_ldq(&pte_trans);
> +            pte = ptw_ldq(&pte_trans, ra);
>              if (!(pte & PG_PRESENT_MASK)) {
>                  goto do_fault;
>              }
> @@ -212,11 +213,11 @@ static bool mmu_translate(CPUX86State *env, const TranslateParams *in,
>               */
>              pte_addr = ((pte & PG_ADDRESS_MASK) +
>                          (((addr >> 30) & 0x1ff) << 3)) & a20_mask;
> -            if (!ptw_translate(&pte_trans, pte_addr)) {
> +            if (!ptw_translate(&pte_trans, pte_addr, ra)) {
>                  return false;
>              }
>          restart_3_lma:
> -            pte = ptw_ldq(&pte_trans);
> +            pte = ptw_ldq(&pte_trans, ra);
>              if (!(pte & PG_PRESENT_MASK)) {
>                  goto do_fault;
>              }
> @@ -239,12 +240,12 @@ static bool mmu_translate(CPUX86State *env, const TranslateParams *in,
>               * Page table level 3
>               */
>              pte_addr = ((in->cr3 & ~0x1f) + ((addr >> 27) & 0x18)) & a20_mask;
> -            if (!ptw_translate(&pte_trans, pte_addr)) {
> +            if (!ptw_translate(&pte_trans, pte_addr, ra)) {
>                  return false;
>              }
>              rsvd_mask |= PG_HI_USER_MASK;
>          restart_3_nolma:
> -            pte = ptw_ldq(&pte_trans);
> +            pte = ptw_ldq(&pte_trans, ra);
>              if (!(pte & PG_PRESENT_MASK)) {
>                  goto do_fault;
>              }
> @@ -262,11 +263,11 @@ static bool mmu_translate(CPUX86State *env, const TranslateParams *in,
>           */
>          pte_addr = ((pte & PG_ADDRESS_MASK) +
>                      (((addr >> 21) & 0x1ff) << 3)) & a20_mask;
> -        if (!ptw_translate(&pte_trans, pte_addr)) {
> +        if (!ptw_translate(&pte_trans, pte_addr, ra)) {
>              return false;
>          }
>      restart_2_pae:
> -        pte = ptw_ldq(&pte_trans);
> +        pte = ptw_ldq(&pte_trans, ra);
>          if (!(pte & PG_PRESENT_MASK)) {
>              goto do_fault;
>          }
> @@ -289,10 +290,10 @@ static bool mmu_translate(CPUX86State *env, const TranslateParams *in,
>           */
>          pte_addr = ((pte & PG_ADDRESS_MASK) +
>                      (((addr >> 12) & 0x1ff) << 3)) & a20_mask;
> -        if (!ptw_translate(&pte_trans, pte_addr)) {
> +        if (!ptw_translate(&pte_trans, pte_addr, ra)) {
>              return false;
>          }
> -        pte = ptw_ldq(&pte_trans);
> +        pte = ptw_ldq(&pte_trans, ra);
>          if (!(pte & PG_PRESENT_MASK)) {
>              goto do_fault;
>          }
> @@ -307,11 +308,11 @@ static bool mmu_translate(CPUX86State *env, const TranslateParams *in,
>           * Page table level 2
>           */
>          pte_addr = ((in->cr3 & ~0xfff) + ((addr >> 20) & 0xffc)) & a20_mask;
> -        if (!ptw_translate(&pte_trans, pte_addr)) {
> +        if (!ptw_translate(&pte_trans, pte_addr, ra)) {
>              return false;
>          }
>      restart_2_nopae:
> -        pte = ptw_ldl(&pte_trans);
> +        pte = ptw_ldl(&pte_trans, ra);
>          if (!(pte & PG_PRESENT_MASK)) {
>              goto do_fault;
>          }
> @@ -336,10 +337,10 @@ static bool mmu_translate(CPUX86State *env, const TranslateParams *in,
>           * Page table level 1
>           */
>          pte_addr = ((pte & ~0xfffu) + ((addr >> 10) & 0xffc)) & a20_mask;
> -        if (!ptw_translate(&pte_trans, pte_addr)) {
> +        if (!ptw_translate(&pte_trans, pte_addr, ra)) {
>              return false;
>          }
> -        pte = ptw_ldl(&pte_trans);
> +        pte = ptw_ldl(&pte_trans, ra);
>          if (!(pte & PG_PRESENT_MASK)) {
>              goto do_fault;
>          }
> @@ -529,7 +530,8 @@ static G_NORETURN void raise_stage2(CPUX86State *env, TranslateFault *err,
> 
>  static bool get_physical_address(CPUX86State *env, vaddr addr,
>                                   MMUAccessType access_type, int mmu_idx,
> -                                 TranslateResult *out, TranslateFault *err)
> +                                 TranslateResult *out, TranslateFault *err,
> +                                 uint64_t ra)
>  {
>      TranslateParams in;
>      bool use_stage2 = env->hflags2 & HF2_NPT_MASK;
> @@ -548,7 +550,7 @@ static bool get_physical_address(CPUX86State *env, vaddr addr,
>              in.mmu_idx = MMU_USER_IDX;
>              in.ptw_idx = MMU_PHYS_IDX;
> 
> -            if (!mmu_translate(env, &in, out, err)) {
> +            if (!mmu_translate(env, &in, out, err, ra)) {
>                  err->stage2 = S2_GPA;
>                  return false;
>              }
> @@ -575,7 +577,7 @@ static bool get_physical_address(CPUX86State *env, vaddr addr,
>                      return false;
>                  }
>              }
> -            return mmu_translate(env, &in, out, err);
> +            return mmu_translate(env, &in, out, err, ra);
>          }
>          break;
>      }
> @@ -601,7 +603,7 @@ bool x86_cpu_tlb_fill(CPUState *cs, vaddr addr, int size,
>      TranslateResult out;
>      TranslateFault err;
> 
> -    if (get_physical_address(env, addr, access_type, mmu_idx, &out, &err)) {
> +    if (get_physical_address(env, addr, access_type, mmu_idx, &out, &err, retaddr)) {
>          /*
>           * Even if 4MB pages, we map only one 4KB page in the cache to
>           * avoid filling it too fast.