| Message ID | 20230901060118.379-1-zhiwei_liu@linux.alibaba.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | [1/1] accel/tcg: Fix the comment for CPUTLBEntryFull | expand |
On 8/31/23 23:01, LIU Zhiwei wrote: > When memory region is ram, the lower TARGET_PAGE_BITS is not the > physical section number. Instead, its value is always 0. > > Add comment and assert to make it clear. > > Signed-off-by: LIU Zhiwei<zhiwei_liu@linux.alibaba.com> > --- > accel/tcg/cputlb.c | 11 +++++++---- > include/exec/cpu-defs.h | 12 ++++++------ > 2 files changed, 13 insertions(+), 10 deletions(-) Reviewed-by: Richard Henderson <richard.henderson@linaro.org> Queued to tcg-next. r~
On 01/09/2023 07:01, LIU Zhiwei wrote: > When memory region is ram, the lower TARGET_PAGE_BITS is not the > physical section number. Instead, its value is always 0. > > Add comment and assert to make it clear. > > Signed-off-by: LIU Zhiwei <zhiwei_liu@linux.alibaba.com> > --- > accel/tcg/cputlb.c | 11 +++++++---- > include/exec/cpu-defs.h | 12 ++++++------ > 2 files changed, 13 insertions(+), 10 deletions(-) > > diff --git a/accel/tcg/cputlb.c b/accel/tcg/cputlb.c > index d68fa6867c..a1ebf75068 100644 > --- a/accel/tcg/cputlb.c > +++ b/accel/tcg/cputlb.c > @@ -1192,6 +1192,7 @@ void tlb_set_page_full(CPUState *cpu, int mmu_idx, > write_flags = read_flags; > if (is_ram) { > iotlb = memory_region_get_ram_addr(section->mr) + xlat; > + assert(!(iotlb & ~TARGET_PAGE_MASK)); > /* > * Computing is_clean is expensive; avoid all that unless > * the page is actually writable. > @@ -1254,10 +1255,12 @@ void tlb_set_page_full(CPUState *cpu, int mmu_idx, > > /* refill the tlb */ > /* > - * At this point iotlb contains a physical section number in the lower > - * TARGET_PAGE_BITS, and either > - * + the ram_addr_t of the page base of the target RAM (RAM) > - * + the offset within section->mr of the page base (I/O, ROMD) > + * When memory region is ram, iotlb contains a TARGET_PAGE_BITS > + * aligned ram_addr_t of the page base of the target RAM. > + * Otherwise, iotlb contains > + * - a physical section number in the lower TARGET_PAGE_BITS > + * - the offset within section->mr of the page base (I/O, ROMD) with the > + * TARGET_PAGE_BITS masked off. > * We subtract addr_page (which is page aligned and thus won't > * disturb the low bits) to give an offset which can be added to the > * (non-page-aligned) vaddr of the eventual memory access to get > diff --git a/include/exec/cpu-defs.h b/include/exec/cpu-defs.h > index fb4c8d480f..350287852e 100644 > --- a/include/exec/cpu-defs.h > +++ b/include/exec/cpu-defs.h > @@ -100,12 +100,12 @@ > typedef struct CPUTLBEntryFull { > /* > * @xlat_section contains: > - * - in the lower TARGET_PAGE_BITS, a physical section number > - * - with the lower TARGET_PAGE_BITS masked off, an offset which > - * must be added to the virtual address to obtain: > - * + the ram_addr_t of the target RAM (if the physical section > - * number is PHYS_SECTION_NOTDIRTY or PHYS_SECTION_ROM) > - * + the offset within the target MemoryRegion (otherwise) > + * - For ram, an offset which must be added to the virtual address > + * to obtain the ram_addr_t of the target RAM > + * - For other memory regions, > + * + in the lower TARGET_PAGE_BITS, the physical section number > + * + with the TARGET_PAGE_BITS masked off, the offset within > + * the target MemoryRegion > */ > hwaddr xlat_section; Someone sent me a test case that triggers the assert() introduced by this commit dff1ab6 ("accel/tcg: Fix the comment for CPUTLBEntryFull") for qemu-system-m68k which is still present in git master. The reproducer is easy: 1. Grab the machine ROM file from https://www.ilande.co.uk/tmp/qemu/tQuadra800.rom 2. Create an empty declaration ROM greater than 4K: dd if=/dev/zero of=/tmp/badrom bs=512 count=12 3. Start QEMU like this: qemu-system-m68k -M q800 -bios tQuadra800.rom \ -device nubus-macfb,romfile=/tmp/badrom The QEMU process hits the assert() with the following backtrace: (gdb) bt #0 0x00007ffff58a9d3c in ?? () from /lib/x86_64-linux-gnu/libc.so.6 #1 0x00007ffff585af32 in raise () from /lib/x86_64-linux-gnu/libc.so.6 #2 0x00007ffff5845472 in abort () from /lib/x86_64-linux-gnu/libc.so.6 #3 0x00007ffff5845395 in ?? () from /lib/x86_64-linux-gnu/libc.so.6 #4 0x00007ffff5853e32 in __assert_fail () from /lib/x86_64-linux-gnu/libc.so.6 #5 0x0000555555942e0a in tlb_set_page_full (cpu=0x55555618d4a0, mmu_idx=0, addr=4244631552, full=0x7fffe7d7f7c0) at ../accel/tcg/cputlb.c:1171 #6 0x00005555559432a0 in tlb_set_page_with_attrs (cpu=0x55555618d4a0, addr=4244631552, paddr=4244631552, attrs=..., prot=7, mmu_idx=0, size=4096) at ../accel/tcg/cputlb.c:1290 #7 0x0000555555943305 in tlb_set_page (cpu=0x55555618d4a0, addr=4244631552, paddr=4244631552, prot=7, mmu_idx=0, size=4096) at ../accel/tcg/cputlb.c:1297 #8 0x000055555588aade in m68k_cpu_tlb_fill (cs=0x55555618d4a0, address=4244635647, size=1, qemu_access_type=MMU_DATA_LOAD, mmu_idx=0, probe=false, retaddr=140734805255937) at ../target/m68k/helper.c:1018 #9 0x0000555555943367 in tlb_fill (cpu=0x55555618d4a0, addr=4244635647, size=1, access_type=MMU_DATA_LOAD, mmu_idx=0, retaddr=140734805255937) at ../accel/tcg/cputlb.c:1315 #10 0x0000555555945d78 in mmu_lookup1 (cpu=0x55555618d4a0, data=0x7fffe7d7fa00, mmu_idx=0, access_type=MMU_DATA_LOAD, ra=140734805255937) at ../accel/tcg/cputlb.c:1713 #11 0x0000555555946081 in mmu_lookup (cpu=0x55555618d4a0, addr=4244635647, oi=3712, ra=140734805255937, type=MMU_DATA_LOAD, l=0x7fffe7d7fa00) at ../accel/tcg/cputlb.c:1803 #12 0x000055555594742b in do_ld1_mmu (cpu=0x55555618d4a0, addr=4244635647, oi=3712, ra=140734805255937, access_type=MMU_DATA_LOAD) at ../accel/tcg/cputlb.c:2377 #13 0x0000555555948f17 in helper_ldub_mmu (env=0x55555618fc60, addr=4244635647, oi=3712, retaddr=140734805255937) at ../accel/tcg/ldst_common.c.inc:19 #14 0x00007fff6013286c in code_gen_buffer () #15 0x00005555559308ff in cpu_tb_exec (cpu=0x55555618d4a0, itb=0x7fffa0132480, tb_exit=0x7fffe7d80030) at ../accel/tcg/cpu-exec.c:458 #16 0x000055555593160a in cpu_loop_exec_tb (cpu=0x55555618d4a0, tb=0x7fffa0132480, pc=1082158370, last_tb=0x7fffe7d80040, tb_exit=0x7fffe7d80030) at ../accel/tcg/cpu-exec.c:920 #17 0x000055555593196a in cpu_exec_loop (cpu=0x55555618d4a0, sc=0x7fffe7d800c0) at ../accel/tcg/cpu-exec.c:1041 #18 0x0000555555931a28 in cpu_exec_setjmp (cpu=0x55555618d4a0, sc=0x7fffe7d800c0) at ../accel/tcg/cpu-exec.c:1058 #19 0x0000555555931aaf in cpu_exec (cpu=0x55555618d4a0) at ../accel/tcg/cpu-exec.c:1084 #20 0x00005555559560ad in tcg_cpus_exec (cpu=0x55555618d4a0) at ../accel/tcg/tcg-accel-ops.c:76 #21 0x00005555559575c2 in rr_cpu_thread_fn (arg=0x55555618d4a0) at ../accel/tcg/tcg-accel-ops-rr.c:261 #22 0x0000555555b61f25 in qemu_thread_start (args=0x555556347a10) at ../util/qemu-thread-posix.c:541 #23 0x00007ffff58a8044 in ?? () from /lib/x86_64-linux-gnu/libc.so.6 #24 0x00007ffff592861c in ?? () from /lib/x86_64-linux-gnu/libc.so.6 ATB, Mark.
On 2023/11/28 21:04, Mark Cave-Ayland wrote: > On 01/09/2023 07:01, LIU Zhiwei wrote: > >> When memory region is ram, the lower TARGET_PAGE_BITS is not the >> physical section number. Instead, its value is always 0. >> >> Add comment and assert to make it clear. >> >> Signed-off-by: LIU Zhiwei <zhiwei_liu@linux.alibaba.com> >> --- >> accel/tcg/cputlb.c | 11 +++++++---- >> include/exec/cpu-defs.h | 12 ++++++------ >> 2 files changed, 13 insertions(+), 10 deletions(-) >> >> diff --git a/accel/tcg/cputlb.c b/accel/tcg/cputlb.c >> index d68fa6867c..a1ebf75068 100644 >> --- a/accel/tcg/cputlb.c >> +++ b/accel/tcg/cputlb.c >> @@ -1192,6 +1192,7 @@ void tlb_set_page_full(CPUState *cpu, int mmu_idx, >> write_flags = read_flags; >> if (is_ram) { >> iotlb = memory_region_get_ram_addr(section->mr) + xlat; >> + assert(!(iotlb & ~TARGET_PAGE_MASK)); >> /* >> * Computing is_clean is expensive; avoid all that unless >> * the page is actually writable. >> @@ -1254,10 +1255,12 @@ void tlb_set_page_full(CPUState *cpu, int >> mmu_idx, >> /* refill the tlb */ >> /* >> - * At this point iotlb contains a physical section number in the >> lower >> - * TARGET_PAGE_BITS, and either >> - * + the ram_addr_t of the page base of the target RAM (RAM) >> - * + the offset within section->mr of the page base (I/O, ROMD) >> + * When memory region is ram, iotlb contains a TARGET_PAGE_BITS >> + * aligned ram_addr_t of the page base of the target RAM. >> + * Otherwise, iotlb contains >> + * - a physical section number in the lower TARGET_PAGE_BITS >> + * - the offset within section->mr of the page base (I/O, ROMD) >> with the >> + * TARGET_PAGE_BITS masked off. >> * We subtract addr_page (which is page aligned and thus won't >> * disturb the low bits) to give an offset which can be added >> to the >> * (non-page-aligned) vaddr of the eventual memory access to get >> diff --git a/include/exec/cpu-defs.h b/include/exec/cpu-defs.h >> index fb4c8d480f..350287852e 100644 >> --- a/include/exec/cpu-defs.h >> +++ b/include/exec/cpu-defs.h >> @@ -100,12 +100,12 @@ >> typedef struct CPUTLBEntryFull { >> /* >> * @xlat_section contains: >> - * - in the lower TARGET_PAGE_BITS, a physical section number >> - * - with the lower TARGET_PAGE_BITS masked off, an offset which >> - * must be added to the virtual address to obtain: >> - * + the ram_addr_t of the target RAM (if the physical section >> - * number is PHYS_SECTION_NOTDIRTY or PHYS_SECTION_ROM) >> - * + the offset within the target MemoryRegion (otherwise) >> + * - For ram, an offset which must be added to the virtual address >> + * to obtain the ram_addr_t of the target RAM >> + * - For other memory regions, >> + * + in the lower TARGET_PAGE_BITS, the physical section number >> + * + with the TARGET_PAGE_BITS masked off, the offset within >> + * the target MemoryRegion >> */ >> hwaddr xlat_section; > > Someone sent me a test case that triggers the assert() introduced by > this commit dff1ab6 ("accel/tcg: Fix the comment for CPUTLBEntryFull") > for qemu-system-m68k which is still present in git master. The > reproducer is easy: > > 1. Grab the machine ROM file from > https://www.ilande.co.uk/tmp/qemu/tQuadra800.rom > > 2. Create an empty declaration ROM greater than 4K: > > dd if=/dev/zero of=/tmp/badrom bs=512 count=12 > > 3. Start QEMU like this: > > qemu-system-m68k -M q800 -bios tQuadra800.rom \ > -device nubus-macfb,romfile=/tmp/badrom > > The QEMU process hits the assert() with the following backtrace: > > (gdb) bt > #0 0x00007ffff58a9d3c in ?? () from /lib/x86_64-linux-gnu/libc.so.6 > #1 0x00007ffff585af32 in raise () from /lib/x86_64-linux-gnu/libc.so.6 > #2 0x00007ffff5845472 in abort () from /lib/x86_64-linux-gnu/libc.so.6 > #3 0x00007ffff5845395 in ?? () from /lib/x86_64-linux-gnu/libc.so.6 > #4 0x00007ffff5853e32 in __assert_fail () from > /lib/x86_64-linux-gnu/libc.so.6 > #5 0x0000555555942e0a in tlb_set_page_full (cpu=0x55555618d4a0, > mmu_idx=0, addr=4244631552, full=0x7fffe7d7f7c0) at > ../accel/tcg/cputlb.c:1171 > #6 0x00005555559432a0 in tlb_set_page_with_attrs (cpu=0x55555618d4a0, > addr=4244631552, paddr=4244631552, attrs=..., prot=7, mmu_idx=0, > size=4096) at ../accel/tcg/cputlb.c:1290 > #7 0x0000555555943305 in tlb_set_page (cpu=0x55555618d4a0, > addr=4244631552, paddr=4244631552, prot=7, mmu_idx=0, size=4096) at > ../accel/tcg/cputlb.c:1297 > #8 0x000055555588aade in m68k_cpu_tlb_fill (cs=0x55555618d4a0, > address=4244635647, size=1, qemu_access_type=MMU_DATA_LOAD, mmu_idx=0, > probe=false, retaddr=140734805255937) at ../target/m68k/helper.c:1018 > #9 0x0000555555943367 in tlb_fill (cpu=0x55555618d4a0, > addr=4244635647, size=1, access_type=MMU_DATA_LOAD, mmu_idx=0, > retaddr=140734805255937) at ../accel/tcg/cputlb.c:1315 > #10 0x0000555555945d78 in mmu_lookup1 (cpu=0x55555618d4a0, > data=0x7fffe7d7fa00, mmu_idx=0, access_type=MMU_DATA_LOAD, > ra=140734805255937) at ../accel/tcg/cputlb.c:1713 > #11 0x0000555555946081 in mmu_lookup (cpu=0x55555618d4a0, > addr=4244635647, oi=3712, ra=140734805255937, type=MMU_DATA_LOAD, > l=0x7fffe7d7fa00) at ../accel/tcg/cputlb.c:1803 > #12 0x000055555594742b in do_ld1_mmu (cpu=0x55555618d4a0, > addr=4244635647, oi=3712, ra=140734805255937, > access_type=MMU_DATA_LOAD) at ../accel/tcg/cputlb.c:2377 > #13 0x0000555555948f17 in helper_ldub_mmu (env=0x55555618fc60, > addr=4244635647, oi=3712, retaddr=140734805255937) at > ../accel/tcg/ldst_common.c.inc:19 > #14 0x00007fff6013286c in code_gen_buffer () > #15 0x00005555559308ff in cpu_tb_exec (cpu=0x55555618d4a0, > itb=0x7fffa0132480, tb_exit=0x7fffe7d80030) at > ../accel/tcg/cpu-exec.c:458 > #16 0x000055555593160a in cpu_loop_exec_tb (cpu=0x55555618d4a0, > tb=0x7fffa0132480, pc=1082158370, last_tb=0x7fffe7d80040, > tb_exit=0x7fffe7d80030) at ../accel/tcg/cpu-exec.c:920 > #17 0x000055555593196a in cpu_exec_loop (cpu=0x55555618d4a0, > sc=0x7fffe7d800c0) at ../accel/tcg/cpu-exec.c:1041 > #18 0x0000555555931a28 in cpu_exec_setjmp (cpu=0x55555618d4a0, > sc=0x7fffe7d800c0) at ../accel/tcg/cpu-exec.c:1058 > #19 0x0000555555931aaf in cpu_exec (cpu=0x55555618d4a0) at > ../accel/tcg/cpu-exec.c:1084 > #20 0x00005555559560ad in tcg_cpus_exec (cpu=0x55555618d4a0) at > ../accel/tcg/tcg-accel-ops.c:76 > #21 0x00005555559575c2 in rr_cpu_thread_fn (arg=0x55555618d4a0) at > ../accel/tcg/tcg-accel-ops-rr.c:261 > #22 0x0000555555b61f25 in qemu_thread_start (args=0x555556347a10) at > ../util/qemu-thread-posix.c:541 > #23 0x00007ffff58a8044 in ?? () from /lib/x86_64-linux-gnu/libc.so.6 > #24 0x00007ffff592861c in ?? () from /lib/x86_64-linux-gnu/libc.so.6 > Hi Mark, The nubus-macfb device create a section not aligned to the TARGET_PAGE_BITS. That is the reason why it fails the assert. But that's OK. It is my error. I have sent a patch to de-assert it. I am not sure whether it can be merged into the 8.2. Thanks, Zhiwei > > ATB, > > Mark.
diff --git a/accel/tcg/cputlb.c b/accel/tcg/cputlb.c index d68fa6867c..a1ebf75068 100644 --- a/accel/tcg/cputlb.c +++ b/accel/tcg/cputlb.c @@ -1192,6 +1192,7 @@ void tlb_set_page_full(CPUState *cpu, int mmu_idx, write_flags = read_flags; if (is_ram) { iotlb = memory_region_get_ram_addr(section->mr) + xlat; + assert(!(iotlb & ~TARGET_PAGE_MASK)); /* * Computing is_clean is expensive; avoid all that unless * the page is actually writable. @@ -1254,10 +1255,12 @@ void tlb_set_page_full(CPUState *cpu, int mmu_idx, /* refill the tlb */ /* - * At this point iotlb contains a physical section number in the lower - * TARGET_PAGE_BITS, and either - * + the ram_addr_t of the page base of the target RAM (RAM) - * + the offset within section->mr of the page base (I/O, ROMD) + * When memory region is ram, iotlb contains a TARGET_PAGE_BITS + * aligned ram_addr_t of the page base of the target RAM. + * Otherwise, iotlb contains + * - a physical section number in the lower TARGET_PAGE_BITS + * - the offset within section->mr of the page base (I/O, ROMD) with the + * TARGET_PAGE_BITS masked off. * We subtract addr_page (which is page aligned and thus won't * disturb the low bits) to give an offset which can be added to the * (non-page-aligned) vaddr of the eventual memory access to get diff --git a/include/exec/cpu-defs.h b/include/exec/cpu-defs.h index fb4c8d480f..350287852e 100644 --- a/include/exec/cpu-defs.h +++ b/include/exec/cpu-defs.h @@ -100,12 +100,12 @@ typedef struct CPUTLBEntryFull { /* * @xlat_section contains: - * - in the lower TARGET_PAGE_BITS, a physical section number - * - with the lower TARGET_PAGE_BITS masked off, an offset which - * must be added to the virtual address to obtain: - * + the ram_addr_t of the target RAM (if the physical section - * number is PHYS_SECTION_NOTDIRTY or PHYS_SECTION_ROM) - * + the offset within the target MemoryRegion (otherwise) + * - For ram, an offset which must be added to the virtual address + * to obtain the ram_addr_t of the target RAM + * - For other memory regions, + * + in the lower TARGET_PAGE_BITS, the physical section number + * + with the TARGET_PAGE_BITS masked off, the offset within + * the target MemoryRegion */ hwaddr xlat_section;
When memory region is ram, the lower TARGET_PAGE_BITS is not the physical section number. Instead, its value is always 0. Add comment and assert to make it clear. Signed-off-by: LIU Zhiwei <zhiwei_liu@linux.alibaba.com> --- accel/tcg/cputlb.c | 11 +++++++---- include/exec/cpu-defs.h | 12 ++++++------ 2 files changed, 13 insertions(+), 10 deletions(-)