[Qemu-devel] [PATCH] memory: Set notdirty_mem_ops validator

[Qemu-devel] [PATCH] memory: Set notdirty_mem_ops validator

[Tony Nguyen]

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,
     },
     .impl = {
         .min_access_size = 1,
-- 
2.23.0

Re: [Qemu-devel] [PATCH] memory: Set notdirty_mem_ops validator

[Peter Xu]

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>

-- 
Peter Xu

Re: [Qemu-devel] [PATCH] memory: Set notdirty_mem_ops validator

[Peter Maydell]

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

Re: [Qemu-devel] [PATCH] memory: Set notdirty_mem_ops validator

[Tony Nguyen]

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.

Re: [Qemu-devel] [PATCH] memory: Set notdirty_mem_ops validator

[Peter Maydell]

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

Re: [Qemu-devel] [PATCH] memory: Set notdirty_mem_ops validator

[Peter Xu]

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,

-- 
Peter Xu

Re: [Qemu-devel] [PATCH] memory: Set notdirty_mem_ops validator

[Tony Nguyen]

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.

Re: [Qemu-devel] [PATCH] memory: Set notdirty_mem_ops validator

[Philippe Mathieu-Daudé]

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>

Re: [Qemu-devel] [PATCH] memory: Set notdirty_mem_ops validator

[Eric Blake]

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).

-- 
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

[Philippe Mathieu-Daudé]

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 */
};

Re: [Qemu-devel] [PATCH] memory: Set notdirty_mem_ops validator

[Eric Blake]

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

[Peter Maydell]

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

Re: [Qemu-devel] [PATCH] memory: Set notdirty_mem_ops validator

[Eric Blake]

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