[PATCH 0/2] exec/memory: Enforce checking MemTxResult values

[PATCH 0/2] exec/memory: Enforce checking MemTxResult values

[Philippe Mathieu-Daudé]

Various places ignore the MemTxResult indicator of
transaction failed. Some cases might be justified
(DMA?) while other are probably bugs. To avoid
ignoring transaction errors, suggestion is to mark
functions returning MemTxResult with
warn_unused_result attribute.

Philippe Mathieu-Daudé (2):
  exec/memory: Let address_space_read/write_cached() propagate
    MemTxResult
  exec/memory: Emit warning when MemTxResult is ignored

 include/exec/memory.h | 50 +++++++++++++++++++++++++++----------------
 exec.c                | 16 +++++++-------
 2 files changed, 40 insertions(+), 26 deletions(-)

-- 
2.21.3

[PATCH 1/2] exec/memory: Let address_space_read/write_cached() propagate MemTxResult

[Philippe Mathieu-Daudé]

Both address_space_read_cached_slow() and
address_space_write_cached_slow() return a MemTxResult type.
Do not discard it, return it to the caller.

Signed-off-by: Philippe Mathieu-Daudé <f4bug@amsat.org>
---
 include/exec/memory.h | 19 +++++++++++--------
 exec.c                | 16 ++++++++--------
 2 files changed, 19 insertions(+), 16 deletions(-)

diff --git a/include/exec/memory.h b/include/exec/memory.h
index e000bd2f97..5e8c009169 100644
--- a/include/exec/memory.h
+++ b/include/exec/memory.h
@@ -2343,10 +2343,11 @@ void *qemu_map_ram_ptr(RAMBlock *ram_block, ram_addr_t addr);
 
 /* Internal functions, part of the implementation of address_space_read_cached
  * and address_space_write_cached.  */
-void address_space_read_cached_slow(MemoryRegionCache *cache,
-                                    hwaddr addr, void *buf, hwaddr len);
-void address_space_write_cached_slow(MemoryRegionCache *cache,
-                                     hwaddr addr, const void *buf, hwaddr len);
+MemTxResult address_space_read_cached_slow(MemoryRegionCache *cache,
+                                           hwaddr addr, void *buf, hwaddr len);
+MemTxResult address_space_write_cached_slow(MemoryRegionCache *cache,
+                                            hwaddr addr, const void *buf,
+                                            hwaddr len);
 
 static inline bool memory_access_is_direct(MemoryRegion *mr, bool is_write)
 {
@@ -2411,15 +2412,16 @@ MemTxResult address_space_read(AddressSpace *as, hwaddr addr,
  * @buf: buffer with the data transferred
  * @len: length of the data transferred
  */
-static inline void
+static inline MemTxResult
 address_space_read_cached(MemoryRegionCache *cache, hwaddr addr,
                           void *buf, hwaddr len)
 {
     assert(addr < cache->len && len <= cache->len - addr);
     if (likely(cache->ptr)) {
         memcpy(buf, cache->ptr + addr, len);
+        return MEMTX_OK;
     } else {
-        address_space_read_cached_slow(cache, addr, buf, len);
+        return address_space_read_cached_slow(cache, addr, buf, len);
     }
 }
 
@@ -2431,15 +2433,16 @@ address_space_read_cached(MemoryRegionCache *cache, hwaddr addr,
  * @buf: buffer with the data transferred
  * @len: length of the data transferred
  */
-static inline void
+static inline MemTxResult
 address_space_write_cached(MemoryRegionCache *cache, hwaddr addr,
                            const void *buf, hwaddr len)
 {
     assert(addr < cache->len && len <= cache->len - addr);
     if (likely(cache->ptr)) {
         memcpy(cache->ptr + addr, buf, len);
+        return MEMTX_OK;
     } else {
-        address_space_write_cached_slow(cache, addr, buf, len);
+        return address_space_write_cached_slow(cache, addr, buf, len);
     }
 }
 
diff --git a/exec.c b/exec.c
index 5162f0d12f..877b51cc5c 100644
--- a/exec.c
+++ b/exec.c
@@ -3716,7 +3716,7 @@ static inline MemoryRegion *address_space_translate_cached(
 /* Called from RCU critical section. address_space_read_cached uses this
  * out of line function when the target is an MMIO or IOMMU region.
  */
-void
+MemTxResult
 address_space_read_cached_slow(MemoryRegionCache *cache, hwaddr addr,
                                    void *buf, hwaddr len)
 {
@@ -3726,15 +3726,15 @@ address_space_read_cached_slow(MemoryRegionCache *cache, hwaddr addr,
     l = len;
     mr = address_space_translate_cached(cache, addr, &addr1, &l, false,
                                         MEMTXATTRS_UNSPECIFIED);
-    flatview_read_continue(cache->fv,
-                           addr, MEMTXATTRS_UNSPECIFIED, buf, len,
-                           addr1, l, mr);
+    return flatview_read_continue(cache->fv,
+                                  addr, MEMTXATTRS_UNSPECIFIED, buf, len,
+                                  addr1, l, mr);
 }
 
 /* Called from RCU critical section. address_space_write_cached uses this
  * out of line function when the target is an MMIO or IOMMU region.
  */
-void
+MemTxResult
 address_space_write_cached_slow(MemoryRegionCache *cache, hwaddr addr,
                                     const void *buf, hwaddr len)
 {
@@ -3744,9 +3744,9 @@ address_space_write_cached_slow(MemoryRegionCache *cache, hwaddr addr,
     l = len;
     mr = address_space_translate_cached(cache, addr, &addr1, &l, true,
                                         MEMTXATTRS_UNSPECIFIED);
-    flatview_write_continue(cache->fv,
-                            addr, MEMTXATTRS_UNSPECIFIED, buf, len,
-                            addr1, l, mr);
+    return flatview_write_continue(cache->fv,
+                                   addr, MEMTXATTRS_UNSPECIFIED, buf, len,
+                                   addr1, l, mr);
 }
 
 #define ARG1_DECL                MemoryRegionCache *cache
-- 
2.21.3

[RFC PATCH 2/2] exec/memory: Emit warning when MemTxResult is ignored

[Philippe Mathieu-Daudé]

When a function from the memory subsystem return a MemTxResult
to indicate that the transaction failed, this return value
must not be ignored by the caller. Mark all these functions
with the QEMU_WARN_UNUSED_RESULT attribute, to prevent users
to ignore possible failed transactions.

Signed-off-by: Philippe Mathieu-Daudé <f4bug@amsat.org>
---
RFC because it doesn't build. But before going thru each caller,
let's talk on the list if this change makes sense.
---
 include/exec/memory.h | 39 +++++++++++++++++++++++++--------------
 1 file changed, 25 insertions(+), 14 deletions(-)

diff --git a/include/exec/memory.h b/include/exec/memory.h
index 5e8c009169..95668d1628 100644
--- a/include/exec/memory.h
+++ b/include/exec/memory.h
@@ -161,12 +161,14 @@ struct MemoryRegionOps {
                                    hwaddr addr,
                                    uint64_t *data,
                                    unsigned size,
-                                   MemTxAttrs attrs);
+                                   MemTxAttrs attrs)
+                                   QEMU_WARN_UNUSED_RESULT;
     MemTxResult (*write_with_attrs)(void *opaque,
                                     hwaddr addr,
                                     uint64_t data,
                                     unsigned size,
-                                    MemTxAttrs attrs);
+                                    MemTxAttrs attrs)
+                                    QEMU_WARN_UNUSED_RESULT;
 
     enum device_endian endianness;
     /* Guest-visible constraints: */
@@ -1989,7 +1991,8 @@ MemTxResult memory_region_dispatch_read(MemoryRegion *mr,
                                         hwaddr addr,
                                         uint64_t *pval,
                                         MemOp op,
-                                        MemTxAttrs attrs);
+                                        MemTxAttrs attrs)
+                                        QEMU_WARN_UNUSED_RESULT;
 /**
  * memory_region_dispatch_write: perform a write directly to the specified
  * MemoryRegion.
@@ -2004,7 +2007,8 @@ MemTxResult memory_region_dispatch_write(MemoryRegion *mr,
                                          hwaddr addr,
                                          uint64_t data,
                                          MemOp op,
-                                         MemTxAttrs attrs);
+                                         MemTxAttrs attrs)
+                                         QEMU_WARN_UNUSED_RESULT;
 
 /**
  * address_space_init: initializes an address space
@@ -2053,7 +2057,8 @@ void address_space_remove_listeners(AddressSpace *as);
  */
 MemTxResult address_space_rw(AddressSpace *as, hwaddr addr,
                              MemTxAttrs attrs, void *buf,
-                             hwaddr len, bool is_write);
+                             hwaddr len, bool is_write)
+                             QEMU_WARN_UNUSED_RESULT;
 
 /**
  * address_space_write: write to address space.
@@ -2070,7 +2075,8 @@ MemTxResult address_space_rw(AddressSpace *as, hwaddr addr,
  */
 MemTxResult address_space_write(AddressSpace *as, hwaddr addr,
                                 MemTxAttrs attrs,
-                                const void *buf, hwaddr len);
+                                const void *buf, hwaddr len)
+                                QEMU_WARN_UNUSED_RESULT;
 
 /**
  * address_space_write_rom: write to address space, including ROM.
@@ -2096,7 +2102,8 @@ MemTxResult address_space_write(AddressSpace *as, hwaddr addr,
  */
 MemTxResult address_space_write_rom(AddressSpace *as, hwaddr addr,
                                     MemTxAttrs attrs,
-                                    const void *buf, hwaddr len);
+                                    const void *buf, hwaddr len)
+                                    QEMU_WARN_UNUSED_RESULT;
 
 /* address_space_ld*: load from an address space
  * address_space_st*: store to an address space
@@ -2334,20 +2341,24 @@ void address_space_unmap(AddressSpace *as, void *buffer, hwaddr len,
 
 /* Internal functions, part of the implementation of address_space_read.  */
 MemTxResult address_space_read_full(AddressSpace *as, hwaddr addr,
-                                    MemTxAttrs attrs, void *buf, hwaddr len);
+                                    MemTxAttrs attrs, void *buf, hwaddr len)
+                                    QEMU_WARN_UNUSED_RESULT;
 MemTxResult flatview_read_continue(FlatView *fv, hwaddr addr,
                                    MemTxAttrs attrs, void *buf,
                                    hwaddr len, hwaddr addr1, hwaddr l,
-                                   MemoryRegion *mr);
+                                   MemoryRegion *mr)
+                                   QEMU_WARN_UNUSED_RESULT;
 void *qemu_map_ram_ptr(RAMBlock *ram_block, ram_addr_t addr);
 
 /* Internal functions, part of the implementation of address_space_read_cached
  * and address_space_write_cached.  */
 MemTxResult address_space_read_cached_slow(MemoryRegionCache *cache,
-                                           hwaddr addr, void *buf, hwaddr len);
+                                           hwaddr addr, void *buf, hwaddr len)
+                                           QEMU_WARN_UNUSED_RESULT;
 MemTxResult address_space_write_cached_slow(MemoryRegionCache *cache,
                                             hwaddr addr, const void *buf,
-                                            hwaddr len);
+                                            hwaddr len)
+                                            QEMU_WARN_UNUSED_RESULT;
 
 static inline bool memory_access_is_direct(MemoryRegion *mr, bool is_write)
 {
@@ -2373,7 +2384,7 @@ static inline bool memory_access_is_direct(MemoryRegion *mr, bool is_write)
  * @buf: buffer with the data transferred
  * @len: length of the data transferred
  */
-static inline __attribute__((__always_inline__))
+static inline __attribute__((__always_inline__)) QEMU_WARN_UNUSED_RESULT
 MemTxResult address_space_read(AddressSpace *as, hwaddr addr,
                                MemTxAttrs attrs, void *buf,
                                hwaddr len)
@@ -2412,7 +2423,7 @@ MemTxResult address_space_read(AddressSpace *as, hwaddr addr,
  * @buf: buffer with the data transferred
  * @len: length of the data transferred
  */
-static inline MemTxResult
+static inline MemTxResult QEMU_WARN_UNUSED_RESULT
 address_space_read_cached(MemoryRegionCache *cache, hwaddr addr,
                           void *buf, hwaddr len)
 {
@@ -2433,7 +2444,7 @@ address_space_read_cached(MemoryRegionCache *cache, hwaddr addr,
  * @buf: buffer with the data transferred
  * @len: length of the data transferred
  */
-static inline MemTxResult
+static inline MemTxResult QEMU_WARN_UNUSED_RESULT
 address_space_write_cached(MemoryRegionCache *cache, hwaddr addr,
                            const void *buf, hwaddr len)
 {
-- 
2.21.3

Re: [PATCH 0/2] exec/memory: Enforce checking MemTxResult values

[no-reply]

Patchew URL: https://patchew.org/QEMU/20200517164817.5371-1-f4bug@amsat.org/



Hi,

This series failed the docker-quick@centos7 build test. Please find the testing commands and
their output below. If you have Docker installed, you can probably reproduce it
locally.

=== TEST SCRIPT BEGIN ===
#!/bin/bash
make docker-image-centos7 V=1 NETWORK=1
time make docker-test-quick@centos7 SHOW_ENV=1 J=14 NETWORK=1
=== TEST SCRIPT END ===

  CC      hw/cpu/core.o
In file included from /tmp/qemu-test/src/hw/core/loader.c:327:0:
/tmp/qemu-test/src/include/hw/elf_ops.h: In function 'load_elf64':
/tmp/qemu-test/src/include/hw/elf_ops.h:556:40: error: ignoring return value of 'address_space_write', declared with attribute warn_unused_result [-Werror=unused-result]
                     address_space_write(as ? as : &address_space_memory,
                                        ^
In file included from /tmp/qemu-test/src/hw/core/loader.c:305:0:
/tmp/qemu-test/src/include/hw/elf_ops.h: In function 'load_elf32':
/tmp/qemu-test/src/include/hw/elf_ops.h:556:40: error: ignoring return value of 'address_space_write', declared with attribute warn_unused_result [-Werror=unused-result]
                     address_space_write(as ? as : &address_space_memory,
                                        ^
/tmp/qemu-test/src/hw/core/loader.c: In function 'rom_reset':
/tmp/qemu-test/src/hw/core/loader.c:1149:36: error: ignoring return value of 'address_space_write_rom', declared with attribute warn_unused_result [-Werror=unused-result]
             address_space_write_rom(rom->as, rom->addr, MEMTXATTRS_UNSPECIFIED,
                                    ^
cc1: all warnings being treated as errors
make: *** [hw/core/loader.o] Error 1
make: *** Waiting for unfinished jobs....
Traceback (most recent call last):
  File "./tests/docker/docker.py", line 664, in <module>
---
    raise CalledProcessError(retcode, cmd)
subprocess.CalledProcessError: Command '['sudo', '-n', 'docker', 'run', '--label', 'com.qemu.instance.uuid=f2955b005f4543d8a28b7197af9c37a0', '-u', '1001', '--security-opt', 'seccomp=unconfined', '--rm', '-e', 'TARGET_LIST=', '-e', 'EXTRA_CONFIGURE_OPTS=', '-e', 'V=', '-e', 'J=14', '-e', 'DEBUG=', '-e', 'SHOW_ENV=1', '-e', 'CCACHE_DIR=/var/tmp/ccache', '-v', '/home/patchew/.cache/qemu-docker-ccache:/var/tmp/ccache:z', '-v', '/var/tmp/patchew-tester-tmp-ov2cj0l6/src/docker-src.2020-05-17-14.03.09.21320:/var/tmp/qemu:z,ro', 'qemu:centos7', '/var/tmp/qemu/run', 'test-quick']' returned non-zero exit status 2.
filter=--filter=label=com.qemu.instance.uuid=f2955b005f4543d8a28b7197af9c37a0
make[1]: *** [docker-run] Error 1
make[1]: Leaving directory `/var/tmp/patchew-tester-tmp-ov2cj0l6/src'
make: *** [docker-run-test-quick@centos7] Error 2

real    3m37.618s
user    0m9.689s


The full log is available at
http://patchew.org/logs/20200517164817.5371-1-f4bug@amsat.org/testing.docker-quick@centos7/?type=message.
---
Email generated automatically by Patchew [https://patchew.org/].
Please send your feedback to patchew-devel@redhat.com

Re: [PATCH 0/2] exec/memory: Enforce checking MemTxResult values

[no-reply]

Patchew URL: https://patchew.org/QEMU/20200517164817.5371-1-f4bug@amsat.org/



Hi,

This series failed the asan build test. Please find the testing commands and
their output below. If you have Docker installed, you can probably reproduce it
locally.

=== TEST SCRIPT BEGIN ===
#!/bin/bash
export ARCH=x86_64
make docker-image-fedora V=1 NETWORK=1
time make docker-test-debug@fedora TARGET_LIST=x86_64-softmmu J=14 NETWORK=1
=== TEST SCRIPT END ===

  CC      hw/display/ati_2d.o
  CC      hw/display/ati_dbg.o
In file included from /tmp/qemu-test/src/hw/core/loader.c:305:
/tmp/qemu-test/src/include/hw/elf_ops.h:556:21: error: ignoring return value of function declared with 'warn_unused_result' attribute [-Werror,-Wunused-result]
                    address_space_write(as ? as : &address_space_memory,
                    ^~~~~~~~~~~~~~~~~~~ ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
In file included from /tmp/qemu-test/src/hw/core/loader.c:327:
/tmp/qemu-test/src/include/hw/elf_ops.h:556:21: error: ignoring return value of function declared with 'warn_unused_result' attribute [-Werror,-Wunused-result]
                    address_space_write(as ? as : &address_space_memory,
                    ^~~~~~~~~~~~~~~~~~~ ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
/tmp/qemu-test/src/hw/core/loader.c:1149:13: error: ignoring return value of function declared with 'warn_unused_result' attribute [-Werror,-Wunused-result]
            address_space_write_rom(rom->as, rom->addr, MEMTXATTRS_UNSPECIFIED,
            ^~~~~~~~~~~~~~~~~~~~~~~ ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
3 errors generated.
make: *** [/tmp/qemu-test/src/rules.mak:69: hw/core/loader.o] Error 1
make: *** Waiting for unfinished jobs....
Traceback (most recent call last):
  File "./tests/docker/docker.py", line 664, in <module>
---
    raise CalledProcessError(retcode, cmd)
subprocess.CalledProcessError: Command '['sudo', '-n', 'docker', 'run', '--label', 'com.qemu.instance.uuid=7ba3b942a61f4047b03d411633b03028', '-u', '1001', '--security-opt', 'seccomp=unconfined', '--rm', '-e', 'TARGET_LIST=x86_64-softmmu', '-e', 'EXTRA_CONFIGURE_OPTS=', '-e', 'V=', '-e', 'J=14', '-e', 'DEBUG=', '-e', 'SHOW_ENV=', '-e', 'CCACHE_DIR=/var/tmp/ccache', '-v', '/home/patchew/.cache/qemu-docker-ccache:/var/tmp/ccache:z', '-v', '/var/tmp/patchew-tester-tmp-3dfs_s0u/src/docker-src.2020-05-17-14.08.31.28840:/var/tmp/qemu:z,ro', 'qemu:fedora', '/var/tmp/qemu/run', 'test-debug']' returned non-zero exit status 2.
filter=--filter=label=com.qemu.instance.uuid=7ba3b942a61f4047b03d411633b03028
make[1]: *** [docker-run] Error 1
make[1]: Leaving directory `/var/tmp/patchew-tester-tmp-3dfs_s0u/src'
make: *** [docker-run-test-debug@fedora] Error 2

real    4m49.028s
user    0m9.839s


The full log is available at
http://patchew.org/logs/20200517164817.5371-1-f4bug@amsat.org/testing.asan/?type=message.
---
Email generated automatically by Patchew [https://patchew.org/].
Please send your feedback to patchew-devel@redhat.com

Re: [PATCH 0/2] exec/memory: Enforce checking MemTxResult values

[no-reply]

Patchew URL: https://patchew.org/QEMU/20200517164817.5371-1-f4bug@amsat.org/



Hi,

This series failed the docker-mingw@fedora build test. Please find the testing commands and
their output below. If you have Docker installed, you can probably reproduce it
locally.

=== TEST SCRIPT BEGIN ===
#! /bin/bash
export ARCH=x86_64
make docker-image-fedora V=1 NETWORK=1
time make docker-test-mingw@fedora J=14 NETWORK=1
=== TEST SCRIPT END ===

  CC      hw/display/bcm2835_fb.o
In file included from /tmp/qemu-test/src/hw/core/loader.c:327:
/tmp/qemu-test/src/include/hw/elf_ops.h: In function 'load_elf64':
/tmp/qemu-test/src/include/hw/elf_ops.h:556:21: error: ignoring return value of 'address_space_write', declared with attribute warn_unused_result [-Werror=unused-result]
                     address_space_write(as ? as : &address_space_memory,
                     ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
                                         addr, MEMTXATTRS_UNSPECIFIED,
---
                                         ~~~~~~~~~~~~~~~~
In file included from /tmp/qemu-test/src/hw/core/loader.c:305:
/tmp/qemu-test/src/include/hw/elf_ops.h: In function 'load_elf32':
/tmp/qemu-test/src/include/hw/elf_ops.h:556:21: error: ignoring return value of 'address_space_write', declared with attribute warn_unused_result [-Werror=unused-result]
                     address_space_write(as ? as : &address_space_memory,
                     ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
                                         addr, MEMTXATTRS_UNSPECIFIED,
---
                                         data, file_size);
                                         ~~~~~~~~~~~~~~~~
/tmp/qemu-test/src/hw/core/loader.c: In function 'rom_reset':
/tmp/qemu-test/src/hw/core/loader.c:1149:13: error: ignoring return value of 'address_space_write_rom', declared with attribute warn_unused_result [-Werror=unused-result]
             address_space_write_rom(rom->as, rom->addr, MEMTXATTRS_UNSPECIFIED,
             ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
                                     rom->data, rom->datasize);
                                     ~~~~~~~~~~~~~~~~~~~~~~~~~
cc1: all warnings being treated as errors
make: *** [/tmp/qemu-test/src/rules.mak:69: hw/core/loader.o] Error 1
make: *** Waiting for unfinished jobs....
Traceback (most recent call last):
  File "./tests/docker/docker.py", line 664, in <module>
---
    raise CalledProcessError(retcode, cmd)
subprocess.CalledProcessError: Command '['sudo', '-n', 'docker', 'run', '--label', 'com.qemu.instance.uuid=cbc78cc15c714fd48ef89d560cf39ed2', '-u', '1001', '--security-opt', 'seccomp=unconfined', '--rm', '-e', 'TARGET_LIST=', '-e', 'EXTRA_CONFIGURE_OPTS=', '-e', 'V=', '-e', 'J=14', '-e', 'DEBUG=', '-e', 'SHOW_ENV=', '-e', 'CCACHE_DIR=/var/tmp/ccache', '-v', '/home/patchew/.cache/qemu-docker-ccache:/var/tmp/ccache:z', '-v', '/var/tmp/patchew-tester-tmp-3un9zd0v/src/docker-src.2020-05-17-14.15.52.2490:/var/tmp/qemu:z,ro', 'qemu:fedora', '/var/tmp/qemu/run', 'test-mingw']' returned non-zero exit status 2.
filter=--filter=label=com.qemu.instance.uuid=cbc78cc15c714fd48ef89d560cf39ed2
make[1]: *** [docker-run] Error 1
make[1]: Leaving directory `/var/tmp/patchew-tester-tmp-3un9zd0v/src'
make: *** [docker-run-test-mingw@fedora] Error 2

real    3m18.237s
user    0m9.100s


The full log is available at
http://patchew.org/logs/20200517164817.5371-1-f4bug@amsat.org/testing.docker-mingw@fedora/?type=message.
---
Email generated automatically by Patchew [https://patchew.org/].
Please send your feedback to patchew-devel@redhat.com

Re: [PATCH 0/2] exec/memory: Enforce checking MemTxResult values

[Peter Maydell]

On Sun, 17 May 2020 at 17:48, Philippe Mathieu-Daudé <f4bug@amsat.org> wrote:
>
> Various places ignore the MemTxResult indicator of
> transaction failed. Some cases might be justified
> (DMA?) while other are probably bugs. To avoid
> ignoring transaction errors, suggestion is to mark
> functions returning MemTxResult with
> warn_unused_result attribute.

Not necessarily a bad idea, but don't we have an awful
lot of places that ignore the result that we'd need
to fix first?

thanks
-- PMM

Re: [PATCH 0/2] exec/memory: Enforce checking MemTxResult values

[Philippe Mathieu-Daudé]

On 5/18/20 11:46 AM, Peter Maydell wrote:
> On Sun, 17 May 2020 at 17:48, Philippe Mathieu-Daudé <f4bug@amsat.org> wrote:
>>
>> Various places ignore the MemTxResult indicator of
>> transaction failed. Some cases might be justified
>> (DMA?) while other are probably bugs. To avoid
>> ignoring transaction errors, suggestion is to mark
>> functions returning MemTxResult with
>> warn_unused_result attribute.
> 
> Not necessarily a bad idea, but don't we have an awful
> lot of places that ignore the result that we'd need
> to fix first?

Yes, there are various places to fix. I wanted to have a preview first,
and not start working on this if it is later rejected. Most of the cases
are hardware specific and require studying each hardware behavior.

Re: [PATCH 0/2] exec/memory: Enforce checking MemTxResult values

[Peter Maydell]

On Mon, 18 May 2020 at 12:20, Philippe Mathieu-Daudé <f4bug@amsat.org> wrote:
> On 5/18/20 11:46 AM, Peter Maydell wrote:
> > Not necessarily a bad idea, but don't we have an awful
> > lot of places that ignore the result that we'd need
> > to fix first?
>
> Yes, there are various places to fix. I wanted to have a preview first,
> and not start working on this if it is later rejected. Most of the cases
> are hardware specific and require studying each hardware behavior.

Well, patches that fix "we should check and handle errors but
we don't" bugs are pretty uncontroversial.

How ugly does the code for "call the function and deliberately ignore
the result in a way that doesn't trigger the warning" look ? Assuming
it's reasonably straightforward to write code for a device that
really does ignore the transaction status then I don't think that
there would be a problem with adding the warn-unused-result attributes,
if and when we got all the existing instances fixed.

The other part of this is really just priorities: it seems
likely that a lot of the places which ignore the return code
are going to be in devices which we don't care strongly
about, so if fixing them all is going to take a long time
because we have to look up the details of dozens of obscure
bits of hardware, then maybe there's more important cleanup
we might prefer to do first. It depends a bit on whether
there are 30 of these callsites, or 3...

thanks
-- PMM

Re: [PATCH 1/2] exec/memory: Let address_space_read/write_cached() propagate MemTxResult

[Paolo Bonzini]

On 17/05/20 18:48, Philippe Mathieu-Daudé wrote:
> Both address_space_read_cached_slow() and
> address_space_write_cached_slow() return a MemTxResult type.
> Do not discard it, return it to the caller.
> 
> Signed-off-by: Philippe Mathieu-Daudé <f4bug@amsat.org>
> ---
>  include/exec/memory.h | 19 +++++++++++--------
>  exec.c                | 16 ++++++++--------
>  2 files changed, 19 insertions(+), 16 deletions(-)
> 
> diff --git a/include/exec/memory.h b/include/exec/memory.h
> index e000bd2f97..5e8c009169 100644
> --- a/include/exec/memory.h
> +++ b/include/exec/memory.h
> @@ -2343,10 +2343,11 @@ void *qemu_map_ram_ptr(RAMBlock *ram_block, ram_addr_t addr);
>  
>  /* Internal functions, part of the implementation of address_space_read_cached
>   * and address_space_write_cached.  */
> -void address_space_read_cached_slow(MemoryRegionCache *cache,
> -                                    hwaddr addr, void *buf, hwaddr len);
> -void address_space_write_cached_slow(MemoryRegionCache *cache,
> -                                     hwaddr addr, const void *buf, hwaddr len);
> +MemTxResult address_space_read_cached_slow(MemoryRegionCache *cache,
> +                                           hwaddr addr, void *buf, hwaddr len);
> +MemTxResult address_space_write_cached_slow(MemoryRegionCache *cache,
> +                                            hwaddr addr, const void *buf,
> +                                            hwaddr len);
>  
>  static inline bool memory_access_is_direct(MemoryRegion *mr, bool is_write)
>  {
> @@ -2411,15 +2412,16 @@ MemTxResult address_space_read(AddressSpace *as, hwaddr addr,
>   * @buf: buffer with the data transferred
>   * @len: length of the data transferred
>   */
> -static inline void
> +static inline MemTxResult
>  address_space_read_cached(MemoryRegionCache *cache, hwaddr addr,
>                            void *buf, hwaddr len)
>  {
>      assert(addr < cache->len && len <= cache->len - addr);
>      if (likely(cache->ptr)) {
>          memcpy(buf, cache->ptr + addr, len);
> +        return MEMTX_OK;
>      } else {
> -        address_space_read_cached_slow(cache, addr, buf, len);
> +        return address_space_read_cached_slow(cache, addr, buf, len);
>      }
>  }
>  
> @@ -2431,15 +2433,16 @@ address_space_read_cached(MemoryRegionCache *cache, hwaddr addr,
>   * @buf: buffer with the data transferred
>   * @len: length of the data transferred
>   */
> -static inline void
> +static inline MemTxResult
>  address_space_write_cached(MemoryRegionCache *cache, hwaddr addr,
>                             const void *buf, hwaddr len)
>  {
>      assert(addr < cache->len && len <= cache->len - addr);
>      if (likely(cache->ptr)) {
>          memcpy(cache->ptr + addr, buf, len);
> +        return MEMTX_OK;
>      } else {
> -        address_space_write_cached_slow(cache, addr, buf, len);
> +        return address_space_write_cached_slow(cache, addr, buf, len);
>      }
>  }
>  
> diff --git a/exec.c b/exec.c
> index 5162f0d12f..877b51cc5c 100644
> --- a/exec.c
> +++ b/exec.c
> @@ -3716,7 +3716,7 @@ static inline MemoryRegion *address_space_translate_cached(
>  /* Called from RCU critical section. address_space_read_cached uses this
>   * out of line function when the target is an MMIO or IOMMU region.
>   */
> -void
> +MemTxResult
>  address_space_read_cached_slow(MemoryRegionCache *cache, hwaddr addr,
>                                     void *buf, hwaddr len)
>  {
> @@ -3726,15 +3726,15 @@ address_space_read_cached_slow(MemoryRegionCache *cache, hwaddr addr,
>      l = len;
>      mr = address_space_translate_cached(cache, addr, &addr1, &l, false,
>                                          MEMTXATTRS_UNSPECIFIED);
> -    flatview_read_continue(cache->fv,
> -                           addr, MEMTXATTRS_UNSPECIFIED, buf, len,
> -                           addr1, l, mr);
> +    return flatview_read_continue(cache->fv,
> +                                  addr, MEMTXATTRS_UNSPECIFIED, buf, len,
> +                                  addr1, l, mr);
>  }
>  
>  /* Called from RCU critical section. address_space_write_cached uses this
>   * out of line function when the target is an MMIO or IOMMU region.
>   */
> -void
> +MemTxResult
>  address_space_write_cached_slow(MemoryRegionCache *cache, hwaddr addr,
>                                      const void *buf, hwaddr len)
>  {
> @@ -3744,9 +3744,9 @@ address_space_write_cached_slow(MemoryRegionCache *cache, hwaddr addr,
>      l = len;
>      mr = address_space_translate_cached(cache, addr, &addr1, &l, true,
>                                          MEMTXATTRS_UNSPECIFIED);
> -    flatview_write_continue(cache->fv,
> -                            addr, MEMTXATTRS_UNSPECIFIED, buf, len,
> -                            addr1, l, mr);
> +    return flatview_write_continue(cache->fv,
> +                                   addr, MEMTXATTRS_UNSPECIFIED, buf, len,
> +                                   addr1, l, mr);
>  }
>  
>  #define ARG1_DECL                MemoryRegionCache *cache
> 

Queued patch 1, thanks.

Paolo