Re: [PATCH 1/1] accel/tcg: Fix the comment for CPUTLBEntryFull
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
---
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 0x758a9d3c in ?? () from /lib/x86_64-linux-gnu/libc.so.6
#1 0x7585af32 in raise () from /lib/x86_64-linux-gnu/libc.so.6
#2 0x75845472 in abort () from /lib/x86_64-linux-gnu/libc.so.6
#3 0x75845395 in ?? () from /lib/x86_64-linux-gnu/libc.so.6
#4 0x75853e32 in __assert_fail () from
/lib/x86_64-linux-gnu/libc.so.6
#5 0x55942e0a in tlb_set_page_full (cpu=0x5618d4a0,
mmu_idx=0, addr=4244631552, full=0x7fffe7d7f7c0) at
../accel/tcg/cputlb.c:1171
#6 0x559432a0 in tlb_set_page_with_attrs (cpu=0x5618d4a0,
addr=4244631552, paddr=4244631552, attrs=..., prot=7, mmu_idx=0,
size=4096) at ../accel/tcg/cputlb.c:1290
#7 0x55943305 in tlb_set_page (cpu=0x5618d4a0,
addr=4244631552, paddr=4244631552, prot=7, mmu_idx=0, size=4096) at
../accel/tcg/cputlb.c:1297
#8 0x5588aade in m68k_cpu_tlb_fill (cs=0x5618d4a0,
address=4244635647, size=1, qemu_access_type=MMU_DATA_LOAD, mmu_idx=0,
probe=false, retaddr=140734805255937) at ../target/m68k/helper.c:1018
#9 0x55943367 in tlb_fill (cpu=0x5618d4a0,
addr=4244635647, size=1, access_type=MMU_DATA_LOAD, mmu_idx=0,
retaddr=140734805255937) at ../accel/tcg/cputlb.c:1315
#10 0x55945d78 in mmu_lookup1 (cpu=0x5618d4a0,
data=0x7fffe7d7fa00, mmu_idx=0, access_type=MMU_DATA_LOAD,
ra=140734805255937) at ../accel/tcg/cputlb.c:1713
#11 0x55946081 in mmu_lookup (cpu=0x5618d4a0,
Re: [PATCH 1/1] accel/tcg: Fix the comment for CPUTLBEntryFull
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
---
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 0x758a9d3c in ?? () from /lib/x86_64-linux-gnu/libc.so.6
#1 0x7585af32 in raise () from /lib/x86_64-linux-gnu/libc.so.6
#2 0x75845472 in abort () from /lib/x86_64-linux-gnu/libc.so.6
#3 0x75845395 in ?? () from /lib/x86_64-linux-gnu/libc.so.6
#4 0x75853e32 in __assert_fail () from /lib/x86_64-linux-gnu/libc.so.6
#5 0x55942e0a in tlb_set_page_full (cpu=0x5618d4a0, mmu_idx=0,
addr=4244631552, full=0x7fffe7d7f7c0) at ../accel/tcg/cputlb.c:1171
#6 0x559432a0 in tlb_set_page_with_attrs (cpu=0x5618d4a0,
addr=4244631552, paddr=4244631552, attrs=..., prot=7, mmu_idx=0, size=4096) at
../accel/tcg/cputlb.c:1290
#7 0x55943305 in tlb_set_page (cpu=0x5618d4a0, addr=4244631552,
paddr=4244631552, prot=7, mmu_idx=0, size=4096) at ../accel/tcg/cputlb.c:1297
#8 0x5588aade in m68k_cpu_tlb_fill (cs=0x5618d4a0, address=4244635647,
size=1, qemu_access_type=MMU_DATA_LOAD, mmu_idx=0, probe=false,
retaddr=140734805255937) at ../target/m68k/helper.c:1018
#9 0x55943367 in tlb_fill (cpu=0x5618d4a0, addr=4244635647, size=1,
access_type=MMU_DATA_LOAD, mmu_idx=0, retaddr=140734805255937) at
../accel/tcg/cputlb.c:1315
#10 0x55945d78 in mmu_lookup1 (cpu=0x5618d4a0, data=0x7fffe7d7fa00,
mmu_idx=0, access_type=MMU_DATA_LOAD, ra=140734805255937) at ../accel/tcg/cputlb.c:1713
#11 0x55946081 in mmu_lookup (cpu=0x5618d4a0, addr=4244635647, oi=3712,
ra=140734805255937, type=MMU_DATA_LOAD,
Re: [PATCH 1/1] accel/tcg: Fix the comment for CPUTLBEntryFull
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 --- accel/tcg/cputlb.c | 11 +++ include/exec/cpu-defs.h | 12 ++-- 2 files changed, 13 insertions(+), 10 deletions(-) Reviewed-by: Richard Henderson Queued to tcg-next. r~
