Hi Michael & Paolo,
On Fri, Apr 05, 2024 at 08:30:43PM +0300, Michael Tokarev wrote:
> Date: Fri, 5 Apr 2024 20:30:43 +0300
> From: Michael Tokarev <m...@tls.msk.ru>
> Subject: Re: [PATCH] target/i386: fix direction of "32-bit MMU" test
>
> 01.04.2024 09:02, Michael Tokarev:
>
> > Anyone can guess why this rather trivial and obviously correct patch causes
> > segfaults
> > in a few tests in staging-7.2 - when run in tcg mode, namely:
> >
> > pxe-test
> > migration-test
> > boot-serial-test
> > bios-tables-test
> > vmgenid-test
> > cdrom-test
> >
> > When reverting this single commit from staging-7.2, it all works fine again.
>
> It sigsegvs in probe_access_internal():
>
> CPUTLBEntry *entry = tlb_entry(env, mmu_idx, addr); -- this one returns
> NULL,
>
> and next there's a call
>
> tlb_addr = tlb_read_ofs(entry, elt_ofs);
>
> which fails.
>
> #0 0x0000555555c5de8a in tlb_read_ofs (ofs=8, entry=0x0) at
> 7.2/accel/tcg/cputlb.c:1455
> #1 probe_access_internal
> (env=0x555556a862a0, addr=4294967280, fault_size=fault_size@entry=1,
> access_type=access_type@entry=MMU_INST_FETCH, mmu_idx=5,
> nonfault=nonfault@entry=false, phost=0x7fffea4d32a0, pfull=0x7fffea4d3298,
> retaddr=0)
> at 7.2/accel/tcg/cputlb.c:1555
> #2 0x0000555555c62aba in get_page_addr_code_hostp
> (env=<optimized out>, addr=addr@entry=4294967280, hostp=hostp@entry=0x0)
> at 7.2/accel/tcg/cputlb.c:1691
> #3 0x0000555555c52b54 in get_page_addr_code (addr=4294967280, env=<optimized
> out>)
> at 7.2/include/exec/exec-all.h:714
> #4 tb_htable_lookup
> (cpu=cpu@entry=0x555556a85530, pc=pc@entry=4294967280,
> cs_base=cs_base@entry=4294901760, flags=flags@entry=64,
> cflags=cflags@entry=4278190080) at 7.2/accel/tcg/cpu-exec.c:236
> #5 0x0000555555c53e8e in tb_lookup
> (cflags=4278190080, flags=64, cs_base=4294901760, pc=4294967280,
> cpu=0x555556a85530)
> at 7.2/accel/tcg/cpu-exec.c:270
> #6 cpu_exec (cpu=cpu@entry=0x555556a85530) at 7.2/accel/tcg/cpu-exec.c:1001
> #7 0x0000555555c75d2f in tcg_cpus_exec (cpu=cpu@entry=0x555556a85530)
> at 7.2/accel/tcg/tcg-accel-ops.c:69
> #8 0x0000555555c75e80 in mttcg_cpu_thread_fn (arg=arg@entry=0x555556a85530)
> at 7.2/accel/tcg/tcg-accel-ops-mttcg.c:95
> #9 0x0000555555ded098 in qemu_thread_start (args=0x555556adac40)
> at 7.2/util/qemu-thread-posix.c:505
> #10 0x00007ffff5793134 in start_thread (arg=<optimized out>)
> #11 0x00007ffff58137dc in clone3 ()
>
I debugged it manually, and found the problem occurs in tlb_index() with
mmu_idx=5.
For v7.2, the maximum mmu index supported by i386 is 4 (since
NB_MMU_MODES = 5 defined in target/i386/cpu-param.h).
On Michael's 7.2-i386-mmu-idx tree, the commit 9fc3a7828d25 ("target/i386:
use separate MMU indexes for 32-bit accesses") introduced more indexes
without relaxing the NB_MMU_MODES for i386.
Before this fix, probe_access_internal() just got the wrong mmu_idx as 4,
and it's not out of bounds. After this fix, the right mmu_idx=5 is truly
out of bounds.
On the master branch, there's no such issue since the commits ffd824f3f32d
("include/exec: Set default NB_MMU_MODES to 16") and 6787318a5d86
("target/i386: Remove NB_MMU_MODES define") relaxed upper limit of MMU
index for i386.
So these 2 commits should also be picked (with these 2 commits, tests in
"make check" passed).
However, the cleanup for NB_MMU_MODES is a big series (total 23 patches,
from the commit ffd824f3f32d ("include/exec: Set default NB_MMU_MODES to
16") to 00da6b49a227 ("include/exec: Remove guards around NB_MMU_MODES")),
maybe changes in other arches should be picked together?
In addition, I think maybe we could add assert() in tlb_index() and
tlb_entry() to ensure the mmu_idx not exceed the limit of NB_MMU_MODES.
But I'm a little unsure if this will hurt performance, because these 2
helpers look like the hotspot functions.
-Zhao
