Re: [Qemu-devel] [PATCH] memory: Set notdirty_mem_ops validator
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). -- Eric Blake, Principal Software Engineer Red Hat, Inc. +1-919-301-3226 Virtualization: qemu.org | libvirt.org
Re: [Qemu-devel] [PATCH] memory: Set notdirty_mem_ops validator
On Wed, 4 Sep 2019 at 03:41, Peter Xu 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
Re: [Qemu-devel] [PATCH] memory: Set notdirty_mem_ops validator
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 ={}). -- Eric Blake, Principal Software Engineer Red Hat, Inc. +1-919-301-3226 Virtualization: qemu.org | libvirt.org
Re: [Qemu-devel] [PATCH] memory: Set notdirty_mem_ops validator
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 >>> --- >>> 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 */ };
Re: [Qemu-devel] [PATCH] memory: Set notdirty_mem_ops validator
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 >> --- >> 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). -- Eric Blake, Principal Software Engineer Red Hat, Inc. +1-919-301-3226 Virtualization: qemu.org | libvirt.org
Re: [Qemu-devel] [PATCH] memory: Set notdirty_mem_ops validator
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 > --- > 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 0x71d44700 (LWP 23749)] 0x in ?? () (gdb) bt #0 0x in () #1 0x557eae4c in memory_region_read_with_attrs_accessor (mr=0x5633d360 , addr=531677168, value=0x71d42eb8, size=4, shift=0, mask=4294967295, attrs=...) at /home/build/src/qemu/git/qemu/memory.c:461 #2 0x557eb1c4 in access_with_adjusted_size (addr=531677168, value=0x71d42eb8, size=4, access_size_min=1, access_size_max=8, access_fn= 0x557eadf0 , mr=0x5633d360 , attrs=...) at /home/build/src/qemu/git/qemu/memory.c:559 #3 0x557edeb0 in memory_region_dispatch_read1 (mr=0x5633d360 , addr=531677168, pval=0x71d42eb8, size=4, attrs=...) at /home/build/src/qemu/git/qemu/memory.c:1429 #4 0x557edf47 in memory_region_dispatch_read (mr=0x5633d360 , addr=531677168, pval=0x71d42eb8, op=MO_32, attrs=...) at /home/build/src/qemu/git/qemu/memory.c:1451 #5 0x55803846 in io_readx (env=0x564b15c0, 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 0x558063ca in load_helper (full_load=0x55805ffb , code_read=false, op=MO_BEUL, retaddr=140736889685638, oi=162, addr=1880588272, env=0x564b15c0) at /home/build/src/qemu/git/qemu/accel/tcg/cputlb.c:1346 #7 0x558063ca in full_be_ldul_mmu (env=0x564b15c0, addr=1880588272, oi=162, retaddr=140736889685638) at /home/build/src/qemu/git/qemu/accel/tcg/cputlb.c:1469 #8 0x55806665 in helper_be_ldul_mmu (env=0x564b15c0, addr=1880588272, oi=162, retaddr=140736889685638) at /home/build/src/qemu/git/qemu/accel/tcg/cputlb.c:1476 #9 0x7fffdc5106cd in code_gen_buffer () #10 0x558280da in cpu_tb_exec (cpu=0x564a8820, itb=0x7fffdc50f7c0 ) at /home/build/src/qemu/git/qemu/accel/tcg/cpu-exec.c:172 #11 0x55828ec7 in cpu_loop_exec_tb (cpu=0x564a8820, tb=0x7fffdc50f7c0 , last_tb=0x71d43598, tb_exit=0x71d43590) at /home/build/src/qemu/git/qemu/accel/tcg/cpu-exec.c:620 #12 0x558291d5 in cpu_exec (cpu=0x564a8820) at /home/build/src/qemu/git/qemu/accel/tcg/cpu-exec.c:731 #13 0x557dc460 in tcg_cpu_exec (cpu=0x564a8820) at /home/build/src/qemu/git/qemu/cpus.c:1445 #14 0x557dc76b in qemu_tcg_rr_cpu_thread_fn (arg=0x564a8820) at /home/build/src/qemu/git/qemu/cpus.c:1547 #15 0x55c562d4 in qemu_thread_start (args=0x564c8020) at /home/build/src/qemu/git/qemu/util/qemu-thread-posix.c:502 #16 0x76296fa3 in start_thread (arg=) at pthread_create.c:486 #17 0x761c74cf in clone () at ../sysdeps/unix/sysv/linux/x86_64/clone.S:95 (gdb) Reviewed-by: Philippe Mathieu-Daudé
Re: [Qemu-devel] [PATCH] memory: Set notdirty_mem_ops validator
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 0x in () #1 0x55a694329099 in memory_region_read_with_attrs_accessor (mr=0x55a69503c6c0 , addr=3874654208, value=0x7fdac32c40e8, size=4, shift=0, mask=4294967295, attrs=...) at /home/tony/dev/qemu/memory.c:461 #2 0x55a6943293fd in access_with_adjusted_size (addr=3874654208, value=0x7fdac32c40e8, size=4, access_size_min=1, access_size_max=8, access_fn= 0x55a69432903d , mr=0x55a69503c6c0 , attrs=...) at /home/tony/dev/qemu/memory.c:559 #3 0x55a69432c239 in memory_region_dispatch_read1 (mr=0x55a69503c6c0 , addr=3874654208, pval=0x7fdac32c40e8, size=4, attrs=...) at /home/tony/dev/qemu/memory.c:1429 #4 0x55a69432c2c9 in memory_region_dispatch_read (mr=0x55a69503c6c0 , addr=3874654208, pval=0x7fdac32c40e8, op=MO_32, attrs=...) at /home/tony/dev/qemu/memory.c:1451 #5 0x55a694341e4f in io_readx (env=0x55a695942da0, iotl=0x7fdabcdee440, mmu_idx=2, addr=3298570569728, retaddr=14057764820, access_type=MMU_DATA_LOAD, op=MO_32) at /home/tony/dev/qemu/accel/tcg/cputlb.c:923 #6 0x55a69434493e in full_be_ldul_mmu (full_load=0x55a69434458a , code_read=false, op=MO_BEUL, retaddr=14057764820, oi=162, addr=3298570569728, env=0x55a695942da0) at /home/tony/dev/qemu/accel/tcg/cputlb.c:1346 #7 0x55a69434493e in full_be_ldul_mmu (env=0x55a695942da0, addr=3298570569728, oi=162, retaddr=14057764820) at /home/tony/dev/qemu/accel/tcg/cputlb.c:1469 #8 0x55a694344bd5 in helper_be_ldul_mmu (env=0x55a695942da0, addr=3298570569728, oi=162, retaddr=14057764820) at /home/tony/dev/qemu/accel/tcg/cputlb.c:1476 #9 0x7fdac8ce3639 in () #10 0x0400 in () #11 0x7fdabc000a10 in () #12 0x7fdac32c42a0 in () #13 0x55a6942d8795 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 0x55a694341e4f inv=0x55a695942da0, iotlbentry=0x7fdabcdee440, mmu_idx=2, addr=3298570569728, retaddr=14057764820, 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 , fv = 0x7fdabc86ca00, offset_within_region = 0, size = 0x0001, offset_within_address_space = 0, readonly = false, nonvolatile = false} Watching sun4u Solaris 10 boot messages, it occurs when sunhme PCI device is configured.
Re: [Qemu-devel] [PATCH] memory: Set notdirty_mem_ops validator
On Tue, Sep 03, 2019 at 05:50:56PM +0100, Peter Maydell wrote: > On Tue, 3 Sep 2019 at 17:47, Tony Nguyen wrote: > > > > On Tue, Sep 03, 2019 at 11:25:28AM +0100, Peter Maydell wrote: > > > On Mon, 2 Sep 2019 at 02:36, Tony Nguyen 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, -- Peter Xu
Re: [Qemu-devel] [PATCH] memory: Set notdirty_mem_ops validator
On Tue, 3 Sep 2019 at 17:47, Tony Nguyen wrote: > > On Tue, Sep 03, 2019 at 11:25:28AM +0100, Peter Maydell wrote: > > On Mon, 2 Sep 2019 at 02:36, Tony Nguyen 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
Re: [Qemu-devel] [PATCH] memory: Set notdirty_mem_ops validator
On Tue, Sep 03, 2019 at 11:25:28AM +0100, Peter Maydell wrote: > On Mon, 2 Sep 2019 at 02:36, Tony Nguyen 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.
Re: [Qemu-devel] [PATCH] memory: Set notdirty_mem_ops validator
On Mon, 2 Sep 2019 at 02:36, Tony Nguyen 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
Re: [Qemu-devel] [PATCH] memory: Set notdirty_mem_ops validator
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 Reviewed-by: Peter Xu -- Peter Xu