| Message ID | 20190902012647.1761-1-tony.nguyen@bt.com |
|---|---|
| State | New |
| Headers | show |
| Series | memory: Set notdirty_mem_ops validator | expand |
On Mon, Sep 02, 2019 at 11:26:47AM +1000, Tony Nguyen wrote: > Existing read rejecting validator was mistakenly cleared. > > Reads dispatched to io_mem_notdirty then segfaults as there is no read > handler. > > Signed-off-by: Tony Nguyen <tony.nguyen@bt.com> Reviewed-by: Peter Xu <peterx@redhat.com>
On Mon, 2 Sep 2019 at 02:36, Tony Nguyen <tony.nguyen@bt.com> wrote: > > Existing read rejecting validator was mistakenly cleared. > > Reads dispatched to io_mem_notdirty then segfaults as there is no read > handler. Do you have the commit hash for where we introduced the bug that this is fixing? thanks -- PMM
On Tue, Sep 03, 2019 at 11:25:28AM +0100, Peter Maydell wrote: > On Mon, 2 Sep 2019 at 02:36, Tony Nguyen <tony.nguyen@bt.com> wrote: > > > > Existing read rejecting validator was mistakenly cleared. > > > > Reads dispatched to io_mem_notdirty then segfaults as there is no read > > handler. > > Do you have the commit hash for where we introduced the > bug that this is fixing? > > thanks > -- PMM > ad52878f97610757390148fe5d5b4cc5ad15c585. Please feel free to amend my commit message. I do not understand why sun4u booting Solaris 10 triggers the bug.
On Tue, 3 Sep 2019 at 17:47, Tony Nguyen <tony.nguyen@bt.com> wrote: > > On Tue, Sep 03, 2019 at 11:25:28AM +0100, Peter Maydell wrote: > > On Mon, 2 Sep 2019 at 02:36, Tony Nguyen <tony.nguyen@bt.com> wrote: > > > > > > Existing read rejecting validator was mistakenly cleared. > > > > > > Reads dispatched to io_mem_notdirty then segfaults as there is no read > > > handler. > > > > Do you have the commit hash for where we introduced the > > bug that this is fixing? > > > > thanks > > -- PMM > > > > ad52878f97610757390148fe5d5b4cc5ad15c585. > > Please feel free to amend my commit message. Thanks. > I do not understand why sun4u booting Solaris 10 triggers the bug. Do you have a backtrace of QEMU from the segfault? I'm having trouble thinking of what the situation is when we'd try to invoke the read handler on io_mem_notdirty... thanks -- PMM
On Tue, Sep 03, 2019 at 05:50:56PM +0100, Peter Maydell wrote: > On Tue, 3 Sep 2019 at 17:47, Tony Nguyen <tony.nguyen@bt.com> wrote: > > > > On Tue, Sep 03, 2019 at 11:25:28AM +0100, Peter Maydell wrote: > > > On Mon, 2 Sep 2019 at 02:36, Tony Nguyen <tony.nguyen@bt.com> wrote: > > > > > > > > Existing read rejecting validator was mistakenly cleared. > > > > > > > > Reads dispatched to io_mem_notdirty then segfaults as there is no read > > > > handler. > > > > > > Do you have the commit hash for where we introduced the > > > bug that this is fixing? > > > > > > thanks > > > -- PMM > > > > > > > ad52878f97610757390148fe5d5b4cc5ad15c585. > > > > Please feel free to amend my commit message. > > Thanks. > > > I do not understand why sun4u booting Solaris 10 triggers the bug. > > Do you have a backtrace of QEMU from the segfault? I'm having trouble > thinking of what the situation is when we'd try to invoke the > read handler on io_mem_notdirty... I've no good understanding of how PHYS_SECTION_NOTDIRTY is used yet... though from what I understand that's the thing this patch wants to fix. Because after the broken commit, this line will be overwritten: .valid.accepts = notdirty_mem_accepts, and accept() will be reset to NULL. With that, memory_region_access_valid(is_write=false) could return valid now (so a read could happen), while it should never, logically? Regards,
On Tue, Sep 03, 2019 at 05:50:56PM +0100, Peter Maydell wrote: > Do you have a backtrace of QEMU from the segfault? I'm having trouble > thinking of what the situation is when we'd try to invoke the > read handler on io_mem_notdirty... Using tcg-next https://github.com/rth7680/qemu/commit/c25c283df0f08582df29f1d5d7be1516b851532d. #0 0x0000000000000000 in () #1 0x000055a694329099 in memory_region_read_with_attrs_accessor (mr=0x55a69503c6c0 <io_mem_notdirty>, addr=3874654208, value=0x7fdac32c40e8, size=4, shift=0, mask=4294967295, attrs=...) at /home/tony/dev/qemu/memory.c:461 #2 0x000055a6943293fd in access_with_adjusted_size (addr=3874654208, value=0x7fdac32c40e8, size=4, access_size_min=1, access_size_max=8, access_fn= 0x55a69432903d <memory_region_read_with_attrs_accessor>, mr=0x55a69503c6c0 <io_mem_notdirty>, attrs=...) at /home/tony/dev/qemu/memory.c:559 #3 0x000055a69432c239 in memory_region_dispatch_read1 (mr=0x55a69503c6c0 <io_mem_notdirty>, addr=3874654208, pval=0x7fdac32c40e8, size=4, attrs=...) at /home/tony/dev/qemu/memory.c:1429 #4 0x000055a69432c2c9 in memory_region_dispatch_read (mr=0x55a69503c6c0 <io_mem_notdirty>, addr=3874654208, pval=0x7fdac32c40e8, op=MO_32, attrs=...) at /home/tony/dev/qemu/memory.c:1451 #5 0x000055a694341e4f in io_readx (env=0x55a695942da0, iotl=0x7fdabcdee440, mmu_idx=2, addr=3298570569728, retaddr=140577648555520, access_type=MMU_DATA_LOAD, op=MO_32) at /home/tony/dev/qemu/accel/tcg/cputlb.c:923 #6 0x000055a69434493e in full_be_ldul_mmu (full_load=0x55a69434458a <full_be_ldul_mmu>, code_read=false, op=MO_BEUL, retaddr=140577648555520, oi=162, addr=3298570569728, env=0x55a695942da0) at /home/tony/dev/qemu/accel/tcg/cputlb.c:1346 #7 0x000055a69434493e in full_be_ldul_mmu (env=0x55a695942da0, addr=3298570569728, oi=162, retaddr=140577648555520) at /home/tony/dev/qemu/accel/tcg/cputlb.c:1469 #8 0x000055a694344bd5 in helper_be_ldul_mmu (env=0x55a695942da0, addr=3298570569728, oi=162, retaddr=140577648555520) at /home/tony/dev/qemu/accel/tcg/cputlb.c:1476 #9 0x00007fdac8ce3639 in () #10 0x0000000004000000 in () #11 0x00007fdabc000a10 in () #12 0x00007fdac32c42a0 in () #13 0x000055a6942d8795 in tcg_temp_free_internal (ts=0x7fdabc0652e0) at /home/tony/dev/qemu/tcg/tcg.c:1330 In frame 5 iotlbentry->addr is 18446740779013636097. Perhaps not a sane value? Defines in target/sparc/cpu-params.h and include/exec/cpu-all.h: TARGET_PAGE_BITS 13 TARGET_PAGE_SIZE (1 << TARGET_PAGE_BITS) TARGET_PAGE_MASK ~(TARGET_PAGE_SIZE - 1) iotlb_to_section resolves (iotlbentry->addr & ~TARGET_PAGE_MASK) to 1, which is io_mem_notdirty. (gdb) frame 5 #5 0x000055a694341e4f inv=0x55a695942da0, iotlbentry=0x7fdabcdee440, mmu_idx=2, addr=3298570569728, retaddr=140577648555520, access_type=MMU_DATA_LOAD, op=MO_32) at /home/tony/dev/qemu/accel/tcg/cputlb.c:923 (gdb) print iotlbentry->addr $1 = 18446740779013636097 (gdb) print iotlbentry->attrs $2 = {unspecified = 0, secure = 0, user = 0, requester_id = 0, byte_swap = 1, target_tlb_bit0 = 0, target_tlb_bit1 = 0, target_tlb_bit2 = 0} (gdb) print cpu->cpu_ases[0]->memory_dispatch->map.sections[1] $3 = {mr = 0x55a69503c6c0 <io_mem_notdirty>, fv = 0x7fdabc86ca00, offset_within_region = 0, size = 0x00000000000000010000000000000000, offset_within_address_space = 0, readonly = false, nonvolatile = false} Watching sun4u Solaris 10 boot messages, it occurs when sunhme PCI device is configured.
On 9/2/19 3:26 AM, Tony Nguyen wrote: > Existing read rejecting validator was mistakenly cleared. > > Reads dispatched to io_mem_notdirty then segfaults as there is no read > handler. > > Signed-off-by: Tony Nguyen <tony.nguyen@bt.com> > --- > exec.c | 2 +- > 1 file changed, 1 insertion(+), 1 deletion(-) > > diff --git a/exec.c b/exec.c > index 1df966d17a..05d664541f 100644 > --- a/exec.c > +++ b/exec.c > @@ -2796,12 +2796,12 @@ static bool notdirty_mem_accepts(void *opaque, hwaddr addr, > > static const MemoryRegionOps notdirty_mem_ops = { > .write = notdirty_mem_write, > - .valid.accepts = notdirty_mem_accepts, > .endianness = DEVICE_NATIVE_ENDIAN, > .valid = { > .min_access_size = 1, > .max_access_size = 8, > .unaligned = false, > + .accepts = notdirty_mem_accepts, I'm surprised the compiler doesn't emit any warning... > }, > .impl = { > .min_access_size = 1, > mcayland provided a verbose backtrace running Solaris, can we amend it to this commit? Thread 4 "qemu-system-spa" received signal SIGSEGV, Segmentation fault. [Switching to Thread 0x7ffff1d44700 (LWP 23749)] 0x0000000000000000 in ?? () (gdb) bt #0 0x0000000000000000 in () #1 0x00005555557eae4c in memory_region_read_with_attrs_accessor (mr=0x55555633d360 <io_mem_notdirty>, addr=531677168, value=0x7ffff1d42eb8, size=4, shift=0, mask=4294967295, attrs=...) at /home/build/src/qemu/git/qemu/memory.c:461 #2 0x00005555557eb1c4 in access_with_adjusted_size (addr=531677168, value=0x7ffff1d42eb8, size=4, access_size_min=1, access_size_max=8, access_fn= 0x5555557eadf0 <memory_region_read_with_attrs_accessor>, mr=0x55555633d360 <io_mem_notdirty>, attrs=...) at /home/build/src/qemu/git/qemu/memory.c:559 #3 0x00005555557edeb0 in memory_region_dispatch_read1 (mr=0x55555633d360 <io_mem_notdirty>, addr=531677168, pval=0x7ffff1d42eb8, size=4, attrs=...) at /home/build/src/qemu/git/qemu/memory.c:1429 #4 0x00005555557edf47 in memory_region_dispatch_read (mr=0x55555633d360 <io_mem_notdirty>, addr=531677168, pval=0x7ffff1d42eb8, op=MO_32, attrs=...) at /home/build/src/qemu/git/qemu/memory.c:1451 #5 0x0000555555803846 in io_readx (env=0x5555564b15c0, iotlbentry=0x7fffe831e190, mmu_idx=2, addr=1880588272, retaddr=140736889685638, access_type=MMU_DATA_LOAD, op=MO_32) at /home/build/src/qemu/git/qemu/accel/tcg/cputlb.c:923 #6 0x00005555558063ca in load_helper (full_load=0x555555805ffb <full_be_ldul_mmu>, code_read=false, op=MO_BEUL, retaddr=140736889685638, oi=162, addr=1880588272, env=0x5555564b15c0) at /home/build/src/qemu/git/qemu/accel/tcg/cputlb.c:1346 #7 0x00005555558063ca in full_be_ldul_mmu (env=0x5555564b15c0, addr=1880588272, oi=162, retaddr=140736889685638) at /home/build/src/qemu/git/qemu/accel/tcg/cputlb.c:1469 #8 0x0000555555806665 in helper_be_ldul_mmu (env=0x5555564b15c0, addr=1880588272, oi=162, retaddr=140736889685638) at /home/build/src/qemu/git/qemu/accel/tcg/cputlb.c:1476 #9 0x00007fffdc5106cd in code_gen_buffer () #10 0x00005555558280da in cpu_tb_exec (cpu=0x5555564a8820, itb=0x7fffdc50f7c0 <code_gen_buffer+5306259>) at /home/build/src/qemu/git/qemu/accel/tcg/cpu-exec.c:172 #11 0x0000555555828ec7 in cpu_loop_exec_tb (cpu=0x5555564a8820, tb=0x7fffdc50f7c0 <code_gen_buffer+5306259>, last_tb=0x7ffff1d43598, tb_exit=0x7ffff1d43590) at /home/build/src/qemu/git/qemu/accel/tcg/cpu-exec.c:620 #12 0x00005555558291d5 in cpu_exec (cpu=0x5555564a8820) at /home/build/src/qemu/git/qemu/accel/tcg/cpu-exec.c:731 #13 0x00005555557dc460 in tcg_cpu_exec (cpu=0x5555564a8820) at /home/build/src/qemu/git/qemu/cpus.c:1445 #14 0x00005555557dc76b in qemu_tcg_rr_cpu_thread_fn (arg=0x5555564a8820) at /home/build/src/qemu/git/qemu/cpus.c:1547 #15 0x0000555555c562d4 in qemu_thread_start (args=0x5555564c8020) at /home/build/src/qemu/git/qemu/util/qemu-thread-posix.c:502 #16 0x00007ffff6296fa3 in start_thread (arg=<optimized out>) at pthread_create.c:486 #17 0x00007ffff61c74cf in clone () at ../sysdeps/unix/sysv/linux/x86_64/clone.S:95 (gdb) Reviewed-by: Philippe Mathieu-Daudé <philmd@redhat.com>
On 9/6/19 3:28 AM, Philippe Mathieu-Daudé wrote: > On 9/2/19 3:26 AM, Tony Nguyen wrote: >> Existing read rejecting validator was mistakenly cleared. >> >> Reads dispatched to io_mem_notdirty then segfaults as there is no read >> handler. >> >> Signed-off-by: Tony Nguyen <tony.nguyen@bt.com> >> --- >> exec.c | 2 +- >> 1 file changed, 1 insertion(+), 1 deletion(-) >> >> diff --git a/exec.c b/exec.c >> index 1df966d17a..05d664541f 100644 >> --- a/exec.c >> +++ b/exec.c >> @@ -2796,12 +2796,12 @@ static bool notdirty_mem_accepts(void *opaque, hwaddr addr, >> >> static const MemoryRegionOps notdirty_mem_ops = { >> .write = notdirty_mem_write, >> - .valid.accepts = notdirty_mem_accepts, >> .endianness = DEVICE_NATIVE_ENDIAN, >> .valid = { >> .min_access_size = 1, >> .max_access_size = 8, >> .unaligned = false, >> + .accepts = notdirty_mem_accepts, > > I'm surprised the compiler doesn't emit any warning... Same here. But reading https://en.cppreference.com/w/c/language/struct_initialization, this is compliant behavior: "However, when an initializer begins with a left open brace, its current object is fully re-initialized and any prior explicit initializers for any of its subobjects are ignored:" so it is worth filing a gcc bug asking for a QoI improvement in adding a warning (since the code does not violate the C standard, but does cause surprises in the reinitialization of omitted members in the later {} to go back to 0 in spite of the earlier initialization by nested name).
On 9/6/19 3:08 PM, Eric Blake wrote: > On 9/6/19 3:28 AM, Philippe Mathieu-Daudé wrote: >> On 9/2/19 3:26 AM, Tony Nguyen wrote: >>> Existing read rejecting validator was mistakenly cleared. >>> >>> Reads dispatched to io_mem_notdirty then segfaults as there is no read >>> handler. >>> >>> Signed-off-by: Tony Nguyen <tony.nguyen@bt.com> >>> --- >>> exec.c | 2 +- >>> 1 file changed, 1 insertion(+), 1 deletion(-) >>> >>> diff --git a/exec.c b/exec.c >>> index 1df966d17a..05d664541f 100644 >>> --- a/exec.c >>> +++ b/exec.c >>> @@ -2796,12 +2796,12 @@ static bool notdirty_mem_accepts(void *opaque, hwaddr addr, >>> >>> static const MemoryRegionOps notdirty_mem_ops = { >>> .write = notdirty_mem_write, >>> - .valid.accepts = notdirty_mem_accepts, >>> .endianness = DEVICE_NATIVE_ENDIAN, >>> .valid = { >>> .min_access_size = 1, >>> .max_access_size = 8, >>> .unaligned = false, >>> + .accepts = notdirty_mem_accepts, >> >> I'm surprised the compiler doesn't emit any warning... > > Same here. > > But reading > https://en.cppreference.com/w/c/language/struct_initialization, this is > compliant behavior: > > "However, when an initializer begins with a left open brace, its current > object is fully re-initialized and any prior explicit initializers for > any of its subobjects are ignored:" > > so it is worth filing a gcc bug asking for a QoI improvement in adding a > warning (since the code does not violate the C standard, but does cause > surprises in the reinitialization of omitted members in the later {} to > go back to 0 in spite of the earlier initialization by nested name). Just remembered another case of (correct) reinitialization in hw/arm/palm.c:101: static struct { int row; int column; } palmte_keymap[0x80] = { [0 ... 0x7f] = { -1, -1 }, [0x3b] = { 0, 0 }, /* F1 -> Calendar */ [0x3c] = { 1, 0 }, /* F2 -> Contacts */ [0x3d] = { 2, 0 }, /* F3 -> Tasks List */ [0x3e] = { 3, 0 }, /* F4 -> Note Pad */ [0x01] = { 4, 0 }, /* Esc -> Power */ [0x4b] = { 0, 1 }, /* Left */ [0x50] = { 1, 1 }, /* Down */ [0x48] = { 2, 1 }, /* Up */ [0x4d] = { 3, 1 }, /* Right */ [0x4c] = { 4, 1 }, /* Centre */ [0x39] = { 4, 1 }, /* Spc -> Centre */ };
On 9/6/19 8:24 AM, Philippe Mathieu-Daudé wrote: >>>> static const MemoryRegionOps notdirty_mem_ops = { >>>> .write = notdirty_mem_write, >>>> - .valid.accepts = notdirty_mem_accepts, >>>> .endianness = DEVICE_NATIVE_ENDIAN, >>>> .valid = { >>>> .min_access_size = 1, >>>> .max_access_size = 8, >>>> .unaligned = false, >>>> + .accepts = notdirty_mem_accepts, >>> >>> I'm surprised the compiler doesn't emit any warning... >> >> Same here. >> >> But reading >> https://en.cppreference.com/w/c/language/struct_initialization, this is >> compliant behavior: >> >> "However, when an initializer begins with a left open brace, its current >> object is fully re-initialized and any prior explicit initializers for >> any of its subobjects are ignored:" >> >> so it is worth filing a gcc bug asking for a QoI improvement in adding a >> warning (since the code does not violate the C standard, but does cause >> surprises in the reinitialization of omitted members in the later {} to >> go back to 0 in spite of the earlier initialization by nested name). > > Just remembered another case of (correct) reinitialization in > hw/arm/palm.c:101: We use nested reinitialization in multiple places. A gcc warning would have to be discriminating enough to NOT warn merely when something is listed twice...: > > static struct { > int row; > int column; > } palmte_keymap[0x80] = { > [0 ... 0x7f] = { -1, -1 }, > [0x3b] = { 0, 0 }, /* F1 -> Calendar */ Here, [0x3b].row and [0x3b].column are listed twice, but both of the second listings are explicit. Similarly, in qobject/json-lexer.c: static const uint8_t json_lexer[][256] = { /* Relies on default initialization to IN_ERROR! */ ... /* * Two start states: * - IN_START recognizes JSON tokens with our string extensions * - IN_START_INTERP additionally recognizes interpolation. */ [IN_START ... IN_START_INTERP] = { ['"'] = IN_DQ_STRING, ... ['\n'] = IN_START, }, [IN_START_INTERP]['%'] = IN_INTERP, }; Done that way on purpose (I actually remember scratching my head on the best way to compress things while reviewing Markus' patch that ended up as 2cbd15aa6f; it took me a couple of tries off-list to end up at that override). ...rather, the gcc warning that I envision should ONLY be when a later partial ={} causes a zero-initialization override of an earlier explicit .member, and NOT when a later complete ={} or explicit member overrides an earlier init (whether the earlier one was explicit due to .member or implicit due to partial ={}).
On Wed, 4 Sep 2019 at 03:41, Peter Xu <peterx@redhat.com> wrote: > On Tue, Sep 03, 2019 at 05:50:56PM +0100, Peter Maydell wrote: > > Do you have a backtrace of QEMU from the segfault? I'm having trouble > > thinking of what the situation is when we'd try to invoke the > > read handler on io_mem_notdirty... > > I've no good understanding of how PHYS_SECTION_NOTDIRTY is used > yet... though from what I understand that's the thing this patch wants > to fix. Because after the broken commit, this line will be > overwritten: > > .valid.accepts = notdirty_mem_accepts, > > and accept() will be reset to NULL. > > With that, memory_region_access_valid(is_write=false) could return > valid now (so a read could happen), while it should never, logically? Having looked into this a bit further, I think that the problem here is that in commit 30d7e098d5c38644359 we accidentally removed the code for TLB_RECHECK-type TLB entries that handled the "actually this is a RAM access" case after repeating the tlb_fill(). So instead of read accesses to notdirty-mem going through that code and never getting into the io_readx() function, now they do. That combined with the bug where we made the .valid.accepts assignment stop working means you can get into this segfault. This is quite rare because I think only Arm M-profile CPUs and Sparc when using the 'invert endian' page table bit (ie Solaris guests doing PCI stuff) will do this. If we apply this patch to reinstate .valid.accepts then instead of a segfault we'll get a guest exception; which is still not the right behaviour. So I think we need to: (1) fix the cputlb code to reinstate the "handle RAM immediately" codepath (2) either allow reads to notdirty-mem MRs (ie make them just read from the host backing RAM), or define them to be a QEMU bug and make them assert immediately the read is attempted thanks -- PMM
On 9/6/19 8:44 AM, Eric Blake wrote: > On 9/6/19 8:24 AM, Philippe Mathieu-Daudé wrote: > >>>>> static const MemoryRegionOps notdirty_mem_ops = { >>>>> .write = notdirty_mem_write, >>>>> - .valid.accepts = notdirty_mem_accepts, >>>>> .endianness = DEVICE_NATIVE_ENDIAN, >>>>> .valid = { >>>>> .min_access_size = 1, >>>>> .max_access_size = 8, >>>>> .unaligned = false, >>>>> + .accepts = notdirty_mem_accepts, >>>> >>>> I'm surprised the compiler doesn't emit any warning... >>> >>> Same here. Actually, I just played with -Woverride-init in gcc 9.2.1 (and clang's comparable -Winitializer-overrides, which we intentionally disable during configure), and they come pretty close - both compilers DO flag when an implicit zero-initialization due to partial ={} overrides an earlier initialization. But sadly, they also warn when one specific init of a smaller subobject overrides another earlier specific init of a larger subobject such as an array range operator. So qobject/json-lexer.c and others fail to compile under the existing warning option, which is why we disable it during configure (clang has it as part of -Wall; gcc only has it as part of -Wextra which we do not use). In researching further, I see: https://gcc.gnu.org/bugzilla/show_bug.cgi?id=24010#c4 which explains why -Woverride-init is NOT part of gcc's -Wall, precisely because of our range pre-initialization usage. So I filed: https://gcc.gnu.org/bugzilla/show_bug.cgi?id=91688 seeing if the gcc devs would consider splitting into -Woverride-init=[12], where 1 only flags a larger subobject overriding an earlier smaller one (would have caught our bug) and 2 flags an equal-size or smaller subobject overriding an earlier large one (which we would not use, because we rely on that for range pre-initialization).
diff --git a/exec.c b/exec.c index 1df966d17a..05d664541f 100644 --- a/exec.c +++ b/exec.c @@ -2796,12 +2796,12 @@ static bool notdirty_mem_accepts(void *opaque, hwaddr addr, static const MemoryRegionOps notdirty_mem_ops = { .write = notdirty_mem_write, - .valid.accepts = notdirty_mem_accepts, .endianness = DEVICE_NATIVE_ENDIAN, .valid = { .min_access_size = 1, .max_access_size = 8, .unaligned = false, + .accepts = notdirty_mem_accepts, }, .impl = { .min_access_size = 1,
Existing read rejecting validator was mistakenly cleared. Reads dispatched to io_mem_notdirty then segfaults as there is no read handler. Signed-off-by: Tony Nguyen <tony.nguyen@bt.com> --- exec.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-)