[RFC PATCH 00/25] exec: Add load/store API for aligned pointers

[RFC PATCH 00/25] exec: Add load/store API for aligned pointers

[Philippe Mathieu-Daudé]

Hi,

This series is the result of a chat with Stefan after looking at
Peter response to Bibo on a problem with unoptimized memcpy()
leading to atomic issues:
https://www.mail-archive.com/qemu-devel@nongnu.org/msg808209.html

MIPS R6 (32/64-bit) does support unaligned accesses, but for the
older releases the compiler can't optimize the __builtin_memcpy().

When a pointer is known to be aligned, we can skip the checks for
misalignment and directly access the data. The pair of virtio vring
fields happen to be aligned.

To be able to call the aligned functions from the virtio layer, we
have to fill the gap in multiple API layers.

The series is decomposed as:
- cleanups (1-6)
- clean ldst API using macros (7-13)
- add aligned ldst methods (14)
- add aligned memory methods (15-16)
- similar changes in virtio (17-24)
- use the new methods on vring aligned values (25)

There are some checkpatch errors related to the macro used.

Regards,

Phil.

Philippe Mathieu-Daudé (25):
  exec/memory_ldst_cached: Sort declarations
  exec/memory_ldst_phys: Sort declarations
  exec/memory_ldst: Use correct type sizes
  exec/memory_ldst_phys: Use correct type sizes
  exec/memory_ldst_cached: Use correct type size
  exec/memory: Use correct type size
  qemu/bswap: Introduce ST_CONVERT() macro
  qemu/bswap: Use ST_CONVERT() macro to emit 16-bit load/store functions
  qemu/bswap: Introduce LD_CONVERT() macro
  qemu/bswap: Use LD_CONVERT macro to emit 16-bit signed load/store code
  qemu/bswap: Use LD_CONVERT macro to emit 16-bit unsigned load/store
    code
  qemu/bswap: Use LDST_CONVERT macro to emit 32-bit load/store functions
  qemu/bswap: Use LDST_CONVERT macro to emit 64-bit load/store functions
  qemu/bswap: Introduce load/store for aligned pointer
  exec/memory: Add methods for aligned pointer access (address space)
  exec/memory: Add methods for aligned pointer access (physical memory)
  hw/virtio: Use correct type sizes
  hw/virtio: Introduce VIRTIO_LD_CONVERT() macro
  hw/virtio: Use LD_CONVERT macro to emit 16-bit unsigned load/store
    code
  hw/virtio: Introduce VIRTIO_ST_CONVERT() macro
  hw/virtio: Use ST_CONVERT() macro to emit 16-bit load/store functions
  hw/virtio: Use LDST_CONVERT macro to emit 32-bit load/store functions
  hw/virtio: Use LDST_CONVERT macro to emit 64-bit load/store functions
  hw/virtio: Add methods for aligned pointer access
  hw/virtio: Optimize accesses on vring aligned pointers

 docs/devel/loads-stores.rst           |  27 +--
 include/exec/memory.h                 |  18 +-
 include/hw/virtio/virtio-access.h     | 239 +++++++++-----------------
 include/qemu/bswap.h                  | 149 ++++++----------
 include/exec/memory_ldst.h.inc        |  16 +-
 include/exec/memory_ldst_cached.h.inc |  66 ++++---
 include/exec/memory_ldst_phys.h.inc   |  72 ++++----
 hw/virtio/virtio.c                    |  13 +-
 memory_ldst.c.inc                     |  20 +--
 9 files changed, 270 insertions(+), 350 deletions(-)

-- 
2.26.3

[RFC PATCH 01/25] exec/memory_ldst_cached: Sort declarations

[Philippe Mathieu-Daudé]

To ease the file review, sort the declarations by the size of
the access (8, 16, 32). Simple code movement, no logical change.

Signed-off-by: Philippe Mathieu-Daudé <philmd@redhat.com>
---
 include/exec/memory_ldst_cached.h.inc | 46 +++++++++++++--------------
 1 file changed, 23 insertions(+), 23 deletions(-)

diff --git a/include/exec/memory_ldst_cached.h.inc b/include/exec/memory_ldst_cached.h.inc
index 7bc8790d346..c33449d0cd5 100644
--- a/include/exec/memory_ldst_cached.h.inc
+++ b/include/exec/memory_ldst_cached.h.inc
@@ -24,6 +24,18 @@
 #define LD_P(size) \
     glue(glue(ld, size), glue(ENDIANNESS, _p))
 
+static inline uint32_t ADDRESS_SPACE_LD_CACHED(uw)(MemoryRegionCache *cache,
+    hwaddr addr, MemTxAttrs attrs, MemTxResult *result)
+{
+    assert(addr < cache->len && 2 <= cache->len - addr);
+    fuzz_dma_read_cb(cache->xlat + addr, 2, cache->mrs.mr);
+    if (likely(cache->ptr)) {
+        return LD_P(uw)(cache->ptr + addr);
+    } else {
+        return ADDRESS_SPACE_LD_CACHED_SLOW(uw)(cache, addr, attrs, result);
+    }
+}
+
 static inline uint32_t ADDRESS_SPACE_LD_CACHED(l)(MemoryRegionCache *cache,
     hwaddr addr, MemTxAttrs attrs, MemTxResult *result)
 {
@@ -48,18 +60,6 @@ static inline uint64_t ADDRESS_SPACE_LD_CACHED(q)(MemoryRegionCache *cache,
     }
 }
 
-static inline uint32_t ADDRESS_SPACE_LD_CACHED(uw)(MemoryRegionCache *cache,
-    hwaddr addr, MemTxAttrs attrs, MemTxResult *result)
-{
-    assert(addr < cache->len && 2 <= cache->len - addr);
-    fuzz_dma_read_cb(cache->xlat + addr, 2, cache->mrs.mr);
-    if (likely(cache->ptr)) {
-        return LD_P(uw)(cache->ptr + addr);
-    } else {
-        return ADDRESS_SPACE_LD_CACHED_SLOW(uw)(cache, addr, attrs, result);
-    }
-}
-
 #undef ADDRESS_SPACE_LD_CACHED
 #undef ADDRESS_SPACE_LD_CACHED_SLOW
 #undef LD_P
@@ -71,17 +71,6 @@ static inline uint32_t ADDRESS_SPACE_LD_CACHED(uw)(MemoryRegionCache *cache,
 #define ST_P(size) \
     glue(glue(st, size), glue(ENDIANNESS, _p))
 
-static inline void ADDRESS_SPACE_ST_CACHED(l)(MemoryRegionCache *cache,
-    hwaddr addr, uint32_t val, MemTxAttrs attrs, MemTxResult *result)
-{
-    assert(addr < cache->len && 4 <= cache->len - addr);
-    if (likely(cache->ptr)) {
-        ST_P(l)(cache->ptr + addr, val);
-    } else {
-        ADDRESS_SPACE_ST_CACHED_SLOW(l)(cache, addr, val, attrs, result);
-    }
-}
-
 static inline void ADDRESS_SPACE_ST_CACHED(w)(MemoryRegionCache *cache,
     hwaddr addr, uint32_t val, MemTxAttrs attrs, MemTxResult *result)
 {
@@ -93,6 +82,17 @@ static inline void ADDRESS_SPACE_ST_CACHED(w)(MemoryRegionCache *cache,
     }
 }
 
+static inline void ADDRESS_SPACE_ST_CACHED(l)(MemoryRegionCache *cache,
+    hwaddr addr, uint32_t val, MemTxAttrs attrs, MemTxResult *result)
+{
+    assert(addr < cache->len && 4 <= cache->len - addr);
+    if (likely(cache->ptr)) {
+        ST_P(l)(cache->ptr + addr, val);
+    } else {
+        ADDRESS_SPACE_ST_CACHED_SLOW(l)(cache, addr, val, attrs, result);
+    }
+}
+
 static inline void ADDRESS_SPACE_ST_CACHED(q)(MemoryRegionCache *cache,
     hwaddr addr, uint64_t val, MemTxAttrs attrs, MemTxResult *result)
 {
-- 
2.26.3

[RFC PATCH 02/25] exec/memory_ldst_phys: Sort declarations

[Philippe Mathieu-Daudé]

To ease the file review, sort the declarations by the size of
the access (8, 16, 32). Simple code movement, no logical change.

Signed-off-by: Philippe Mathieu-Daudé <philmd@redhat.com>
---
 include/exec/memory_ldst_phys.h.inc | 78 ++++++++++++++---------------
 1 file changed, 39 insertions(+), 39 deletions(-)

diff --git a/include/exec/memory_ldst_phys.h.inc b/include/exec/memory_ldst_phys.h.inc
index b9dd53c389d..4033795add7 100644
--- a/include/exec/memory_ldst_phys.h.inc
+++ b/include/exec/memory_ldst_phys.h.inc
@@ -20,6 +20,12 @@
  */
 
 #ifdef TARGET_ENDIANNESS
+static inline uint32_t glue(lduw_phys, SUFFIX)(ARG1_DECL, hwaddr addr)
+{
+    return glue(address_space_lduw, SUFFIX)(ARG1, addr,
+                                            MEMTXATTRS_UNSPECIFIED, NULL);
+}
+
 static inline uint32_t glue(ldl_phys, SUFFIX)(ARG1_DECL, hwaddr addr)
 {
     return glue(address_space_ldl, SUFFIX)(ARG1, addr,
@@ -32,10 +38,10 @@ static inline uint64_t glue(ldq_phys, SUFFIX)(ARG1_DECL, hwaddr addr)
                                            MEMTXATTRS_UNSPECIFIED, NULL);
 }
 
-static inline uint32_t glue(lduw_phys, SUFFIX)(ARG1_DECL, hwaddr addr)
+static inline void glue(stw_phys, SUFFIX)(ARG1_DECL, hwaddr addr, uint32_t val)
 {
-    return glue(address_space_lduw, SUFFIX)(ARG1, addr,
-                                            MEMTXATTRS_UNSPECIFIED, NULL);
+    glue(address_space_stw, SUFFIX)(ARG1, addr, val,
+                                    MEMTXATTRS_UNSPECIFIED, NULL);
 }
 
 static inline void glue(stl_phys, SUFFIX)(ARG1_DECL, hwaddr addr, uint32_t val)
@@ -44,18 +50,30 @@ static inline void glue(stl_phys, SUFFIX)(ARG1_DECL, hwaddr addr, uint32_t val)
                                     MEMTXATTRS_UNSPECIFIED, NULL);
 }
 
-static inline void glue(stw_phys, SUFFIX)(ARG1_DECL, hwaddr addr, uint32_t val)
-{
-    glue(address_space_stw, SUFFIX)(ARG1, addr, val,
-                                    MEMTXATTRS_UNSPECIFIED, NULL);
-}
-
 static inline void glue(stq_phys, SUFFIX)(ARG1_DECL, hwaddr addr, uint64_t val)
 {
     glue(address_space_stq, SUFFIX)(ARG1, addr, val,
                                     MEMTXATTRS_UNSPECIFIED, NULL);
 }
 #else
+static inline uint32_t glue(ldub_phys, SUFFIX)(ARG1_DECL, hwaddr addr)
+{
+    return glue(address_space_ldub, SUFFIX)(ARG1, addr,
+                                            MEMTXATTRS_UNSPECIFIED, NULL);
+}
+
+static inline uint32_t glue(lduw_le_phys, SUFFIX)(ARG1_DECL, hwaddr addr)
+{
+    return glue(address_space_lduw_le, SUFFIX)(ARG1, addr,
+                                               MEMTXATTRS_UNSPECIFIED, NULL);
+}
+
+static inline uint32_t glue(lduw_be_phys, SUFFIX)(ARG1_DECL, hwaddr addr)
+{
+    return glue(address_space_lduw_be, SUFFIX)(ARG1, addr,
+                                               MEMTXATTRS_UNSPECIFIED, NULL);
+}
+
 static inline uint32_t glue(ldl_le_phys, SUFFIX)(ARG1_DECL, hwaddr addr)
 {
     return glue(address_space_ldl_le, SUFFIX)(ARG1, addr,
@@ -80,36 +98,6 @@ static inline uint64_t glue(ldq_be_phys, SUFFIX)(ARG1_DECL, hwaddr addr)
                                               MEMTXATTRS_UNSPECIFIED, NULL);
 }
 
-static inline uint32_t glue(ldub_phys, SUFFIX)(ARG1_DECL, hwaddr addr)
-{
-    return glue(address_space_ldub, SUFFIX)(ARG1, addr,
-                                            MEMTXATTRS_UNSPECIFIED, NULL);
-}
-
-static inline uint32_t glue(lduw_le_phys, SUFFIX)(ARG1_DECL, hwaddr addr)
-{
-    return glue(address_space_lduw_le, SUFFIX)(ARG1, addr,
-                                               MEMTXATTRS_UNSPECIFIED, NULL);
-}
-
-static inline uint32_t glue(lduw_be_phys, SUFFIX)(ARG1_DECL, hwaddr addr)
-{
-    return glue(address_space_lduw_be, SUFFIX)(ARG1, addr,
-                                               MEMTXATTRS_UNSPECIFIED, NULL);
-}
-
-static inline void glue(stl_le_phys, SUFFIX)(ARG1_DECL, hwaddr addr, uint32_t val)
-{
-    glue(address_space_stl_le, SUFFIX)(ARG1, addr, val,
-                                       MEMTXATTRS_UNSPECIFIED, NULL);
-}
-
-static inline void glue(stl_be_phys, SUFFIX)(ARG1_DECL, hwaddr addr, uint32_t val)
-{
-    glue(address_space_stl_be, SUFFIX)(ARG1, addr, val,
-                                       MEMTXATTRS_UNSPECIFIED, NULL);
-}
-
 static inline void glue(stb_phys, SUFFIX)(ARG1_DECL, hwaddr addr, uint32_t val)
 {
     glue(address_space_stb, SUFFIX)(ARG1, addr, val,
@@ -128,6 +116,18 @@ static inline void glue(stw_be_phys, SUFFIX)(ARG1_DECL, hwaddr addr, uint32_t va
                                        MEMTXATTRS_UNSPECIFIED, NULL);
 }
 
+static inline void glue(stl_le_phys, SUFFIX)(ARG1_DECL, hwaddr addr, uint32_t val)
+{
+    glue(address_space_stl_le, SUFFIX)(ARG1, addr, val,
+                                       MEMTXATTRS_UNSPECIFIED, NULL);
+}
+
+static inline void glue(stl_be_phys, SUFFIX)(ARG1_DECL, hwaddr addr, uint32_t val)
+{
+    glue(address_space_stl_be, SUFFIX)(ARG1, addr, val,
+                                       MEMTXATTRS_UNSPECIFIED, NULL);
+}
+
 static inline void glue(stq_le_phys, SUFFIX)(ARG1_DECL, hwaddr addr, uint64_t val)
 {
     glue(address_space_stq_le, SUFFIX)(ARG1, addr, val,
-- 
2.26.3

[RFC PATCH 03/25] exec/memory_ldst: Use correct type sizes

[Philippe Mathieu-Daudé]

Use uint8_t for (unsigned) byte, and uint16_t for (unsigned)
16-bit word.

Signed-off-by: Philippe Mathieu-Daudé <philmd@redhat.com>
---
 include/exec/memory_ldst.h.inc | 16 ++++++++--------
 memory_ldst.c.inc              | 20 ++++++++++----------
 2 files changed, 18 insertions(+), 18 deletions(-)

diff --git a/include/exec/memory_ldst.h.inc b/include/exec/memory_ldst.h.inc
index 46e6c220d35..7c3a641f7ed 100644
--- a/include/exec/memory_ldst.h.inc
+++ b/include/exec/memory_ldst.h.inc
@@ -20,7 +20,7 @@
  */
 
 #ifdef TARGET_ENDIANNESS
-extern uint32_t glue(address_space_lduw, SUFFIX)(ARG1_DECL,
+extern uint16_t glue(address_space_lduw, SUFFIX)(ARG1_DECL,
     hwaddr addr, MemTxAttrs attrs, MemTxResult *result);
 extern uint32_t glue(address_space_ldl, SUFFIX)(ARG1_DECL,
     hwaddr addr, MemTxAttrs attrs, MemTxResult *result);
@@ -29,17 +29,17 @@ extern uint64_t glue(address_space_ldq, SUFFIX)(ARG1_DECL,
 extern void glue(address_space_stl_notdirty, SUFFIX)(ARG1_DECL,
     hwaddr addr, uint32_t val, MemTxAttrs attrs, MemTxResult *result);
 extern void glue(address_space_stw, SUFFIX)(ARG1_DECL,
-    hwaddr addr, uint32_t val, MemTxAttrs attrs, MemTxResult *result);
+    hwaddr addr, uint16_t val, MemTxAttrs attrs, MemTxResult *result);
 extern void glue(address_space_stl, SUFFIX)(ARG1_DECL,
     hwaddr addr, uint32_t val, MemTxAttrs attrs, MemTxResult *result);
 extern void glue(address_space_stq, SUFFIX)(ARG1_DECL,
     hwaddr addr, uint64_t val, MemTxAttrs attrs, MemTxResult *result);
 #else
-extern uint32_t glue(address_space_ldub, SUFFIX)(ARG1_DECL,
+extern uint8_t glue(address_space_ldub, SUFFIX)(ARG1_DECL,
     hwaddr addr, MemTxAttrs attrs, MemTxResult *result);
-extern uint32_t glue(address_space_lduw_le, SUFFIX)(ARG1_DECL,
+extern uint16_t glue(address_space_lduw_le, SUFFIX)(ARG1_DECL,
     hwaddr addr, MemTxAttrs attrs, MemTxResult *result);
-extern uint32_t glue(address_space_lduw_be, SUFFIX)(ARG1_DECL,
+extern uint16_t glue(address_space_lduw_be, SUFFIX)(ARG1_DECL,
     hwaddr addr, MemTxAttrs attrs, MemTxResult *result);
 extern uint32_t glue(address_space_ldl_le, SUFFIX)(ARG1_DECL,
     hwaddr addr, MemTxAttrs attrs, MemTxResult *result);
@@ -50,11 +50,11 @@ extern uint64_t glue(address_space_ldq_le, SUFFIX)(ARG1_DECL,
 extern uint64_t glue(address_space_ldq_be, SUFFIX)(ARG1_DECL,
     hwaddr addr, MemTxAttrs attrs, MemTxResult *result);
 extern void glue(address_space_stb, SUFFIX)(ARG1_DECL,
-    hwaddr addr, uint32_t val, MemTxAttrs attrs, MemTxResult *result);
+    hwaddr addr, uint8_t val, MemTxAttrs attrs, MemTxResult *result);
 extern void glue(address_space_stw_le, SUFFIX)(ARG1_DECL,
-    hwaddr addr, uint32_t val, MemTxAttrs attrs, MemTxResult *result);
+    hwaddr addr, uint16_t val, MemTxAttrs attrs, MemTxResult *result);
 extern void glue(address_space_stw_be, SUFFIX)(ARG1_DECL,
-    hwaddr addr, uint32_t val, MemTxAttrs attrs, MemTxResult *result);
+    hwaddr addr, uint16_t val, MemTxAttrs attrs, MemTxResult *result);
 extern void glue(address_space_stl_le, SUFFIX)(ARG1_DECL,
     hwaddr addr, uint32_t val, MemTxAttrs attrs, MemTxResult *result);
 extern void glue(address_space_stl_be, SUFFIX)(ARG1_DECL,
diff --git a/memory_ldst.c.inc b/memory_ldst.c.inc
index b56e9619674..84b868f2946 100644
--- a/memory_ldst.c.inc
+++ b/memory_ldst.c.inc
@@ -157,7 +157,7 @@ uint64_t glue(address_space_ldq_be, SUFFIX)(ARG1_DECL,
                                                     DEVICE_BIG_ENDIAN);
 }
 
-uint32_t glue(address_space_ldub, SUFFIX)(ARG1_DECL,
+uint8_t glue(address_space_ldub, SUFFIX)(ARG1_DECL,
     hwaddr addr, MemTxAttrs attrs, MemTxResult *result)
 {
     uint8_t *ptr;
@@ -193,7 +193,7 @@ uint32_t glue(address_space_ldub, SUFFIX)(ARG1_DECL,
 }
 
 /* warning: addr must be aligned */
-static inline uint32_t glue(address_space_lduw_internal, SUFFIX)(ARG1_DECL,
+static inline uint16_t glue(address_space_lduw_internal, SUFFIX)(ARG1_DECL,
     hwaddr addr, MemTxAttrs attrs, MemTxResult *result,
     enum device_endian endian)
 {
@@ -240,21 +240,21 @@ static inline uint32_t glue(address_space_lduw_internal, SUFFIX)(ARG1_DECL,
     return val;
 }
 
-uint32_t glue(address_space_lduw, SUFFIX)(ARG1_DECL,
+uint16_t glue(address_space_lduw, SUFFIX)(ARG1_DECL,
     hwaddr addr, MemTxAttrs attrs, MemTxResult *result)
 {
     return glue(address_space_lduw_internal, SUFFIX)(ARG1, addr, attrs, result,
                                                      DEVICE_NATIVE_ENDIAN);
 }
 
-uint32_t glue(address_space_lduw_le, SUFFIX)(ARG1_DECL,
+uint16_t glue(address_space_lduw_le, SUFFIX)(ARG1_DECL,
     hwaddr addr, MemTxAttrs attrs, MemTxResult *result)
 {
     return glue(address_space_lduw_internal, SUFFIX)(ARG1, addr, attrs, result,
                                                      DEVICE_LITTLE_ENDIAN);
 }
 
-uint32_t glue(address_space_lduw_be, SUFFIX)(ARG1_DECL,
+uint16_t glue(address_space_lduw_be, SUFFIX)(ARG1_DECL,
     hwaddr addr, MemTxAttrs attrs, MemTxResult *result)
 {
     return glue(address_space_lduw_internal, SUFFIX)(ARG1, addr, attrs, result,
@@ -366,7 +366,7 @@ void glue(address_space_stl_be, SUFFIX)(ARG1_DECL,
 }
 
 void glue(address_space_stb, SUFFIX)(ARG1_DECL,
-    hwaddr addr, uint32_t val, MemTxAttrs attrs, MemTxResult *result)
+    hwaddr addr, uint8_t val, MemTxAttrs attrs, MemTxResult *result)
 {
     uint8_t *ptr;
     MemoryRegion *mr;
@@ -398,7 +398,7 @@ void glue(address_space_stb, SUFFIX)(ARG1_DECL,
 
 /* warning: addr must be aligned */
 static inline void glue(address_space_stw_internal, SUFFIX)(ARG1_DECL,
-    hwaddr addr, uint32_t val, MemTxAttrs attrs,
+    hwaddr addr, uint16_t val, MemTxAttrs attrs,
     MemTxResult *result, enum device_endian endian)
 {
     uint8_t *ptr;
@@ -441,21 +441,21 @@ static inline void glue(address_space_stw_internal, SUFFIX)(ARG1_DECL,
 }
 
 void glue(address_space_stw, SUFFIX)(ARG1_DECL,
-    hwaddr addr, uint32_t val, MemTxAttrs attrs, MemTxResult *result)
+    hwaddr addr, uint16_t val, MemTxAttrs attrs, MemTxResult *result)
 {
     glue(address_space_stw_internal, SUFFIX)(ARG1, addr, val, attrs, result,
                                              DEVICE_NATIVE_ENDIAN);
 }
 
 void glue(address_space_stw_le, SUFFIX)(ARG1_DECL,
-    hwaddr addr, uint32_t val, MemTxAttrs attrs, MemTxResult *result)
+    hwaddr addr, uint16_t val, MemTxAttrs attrs, MemTxResult *result)
 {
     glue(address_space_stw_internal, SUFFIX)(ARG1, addr, val, attrs, result,
                                              DEVICE_LITTLE_ENDIAN);
 }
 
 void glue(address_space_stw_be, SUFFIX)(ARG1_DECL,
-    hwaddr addr, uint32_t val, MemTxAttrs attrs, MemTxResult *result)
+    hwaddr addr, uint16_t val, MemTxAttrs attrs, MemTxResult *result)
 {
     glue(address_space_stw_internal, SUFFIX)(ARG1, addr, val, attrs, result,
                                DEVICE_BIG_ENDIAN);
-- 
2.26.3

[RFC PATCH 04/25] exec/memory_ldst_phys: Use correct type sizes

[Philippe Mathieu-Daudé]

Use uint8_t for (unsigned) byte, and uint16_t for (unsigned)
16-bit word.

Signed-off-by: Philippe Mathieu-Daudé <philmd@redhat.com>
---
 include/exec/memory_ldst_phys.h.inc | 16 ++++++++--------
 1 file changed, 8 insertions(+), 8 deletions(-)

diff --git a/include/exec/memory_ldst_phys.h.inc b/include/exec/memory_ldst_phys.h.inc
index 4033795add7..ecd678610d1 100644
--- a/include/exec/memory_ldst_phys.h.inc
+++ b/include/exec/memory_ldst_phys.h.inc
@@ -20,7 +20,7 @@
  */
 
 #ifdef TARGET_ENDIANNESS
-static inline uint32_t glue(lduw_phys, SUFFIX)(ARG1_DECL, hwaddr addr)
+static inline uint16_t glue(lduw_phys, SUFFIX)(ARG1_DECL, hwaddr addr)
 {
     return glue(address_space_lduw, SUFFIX)(ARG1, addr,
                                             MEMTXATTRS_UNSPECIFIED, NULL);
@@ -38,7 +38,7 @@ static inline uint64_t glue(ldq_phys, SUFFIX)(ARG1_DECL, hwaddr addr)
                                            MEMTXATTRS_UNSPECIFIED, NULL);
 }
 
-static inline void glue(stw_phys, SUFFIX)(ARG1_DECL, hwaddr addr, uint32_t val)
+static inline void glue(stw_phys, SUFFIX)(ARG1_DECL, hwaddr addr, uint16_t val)
 {
     glue(address_space_stw, SUFFIX)(ARG1, addr, val,
                                     MEMTXATTRS_UNSPECIFIED, NULL);
@@ -56,19 +56,19 @@ static inline void glue(stq_phys, SUFFIX)(ARG1_DECL, hwaddr addr, uint64_t val)
                                     MEMTXATTRS_UNSPECIFIED, NULL);
 }
 #else
-static inline uint32_t glue(ldub_phys, SUFFIX)(ARG1_DECL, hwaddr addr)
+static inline uint8_t glue(ldub_phys, SUFFIX)(ARG1_DECL, hwaddr addr)
 {
     return glue(address_space_ldub, SUFFIX)(ARG1, addr,
                                             MEMTXATTRS_UNSPECIFIED, NULL);
 }
 
-static inline uint32_t glue(lduw_le_phys, SUFFIX)(ARG1_DECL, hwaddr addr)
+static inline uint16_t glue(lduw_le_phys, SUFFIX)(ARG1_DECL, hwaddr addr)
 {
     return glue(address_space_lduw_le, SUFFIX)(ARG1, addr,
                                                MEMTXATTRS_UNSPECIFIED, NULL);
 }
 
-static inline uint32_t glue(lduw_be_phys, SUFFIX)(ARG1_DECL, hwaddr addr)
+static inline uint16_t glue(lduw_be_phys, SUFFIX)(ARG1_DECL, hwaddr addr)
 {
     return glue(address_space_lduw_be, SUFFIX)(ARG1, addr,
                                                MEMTXATTRS_UNSPECIFIED, NULL);
@@ -98,19 +98,19 @@ static inline uint64_t glue(ldq_be_phys, SUFFIX)(ARG1_DECL, hwaddr addr)
                                               MEMTXATTRS_UNSPECIFIED, NULL);
 }
 
-static inline void glue(stb_phys, SUFFIX)(ARG1_DECL, hwaddr addr, uint32_t val)
+static inline void glue(stb_phys, SUFFIX)(ARG1_DECL, hwaddr addr, uint8_t val)
 {
     glue(address_space_stb, SUFFIX)(ARG1, addr, val,
                                     MEMTXATTRS_UNSPECIFIED, NULL);
 }
 
-static inline void glue(stw_le_phys, SUFFIX)(ARG1_DECL, hwaddr addr, uint32_t val)
+static inline void glue(stw_le_phys, SUFFIX)(ARG1_DECL, hwaddr addr, uint16_t val)
 {
     glue(address_space_stw_le, SUFFIX)(ARG1, addr, val,
                                        MEMTXATTRS_UNSPECIFIED, NULL);
 }
 
-static inline void glue(stw_be_phys, SUFFIX)(ARG1_DECL, hwaddr addr, uint32_t val)
+static inline void glue(stw_be_phys, SUFFIX)(ARG1_DECL, hwaddr addr, uint16_t val)
 {
     glue(address_space_stw_be, SUFFIX)(ARG1, addr, val,
                                        MEMTXATTRS_UNSPECIFIED, NULL);
-- 
2.26.3

[RFC PATCH 05/25] exec/memory_ldst_cached: Use correct type size

[Philippe Mathieu-Daudé]

Use uint16_t for (unsigned) 16-bit word.

Signed-off-by: Philippe Mathieu-Daudé <philmd@redhat.com>
---
 include/exec/memory_ldst_cached.h.inc | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/include/exec/memory_ldst_cached.h.inc b/include/exec/memory_ldst_cached.h.inc
index c33449d0cd5..d7834f852c4 100644
--- a/include/exec/memory_ldst_cached.h.inc
+++ b/include/exec/memory_ldst_cached.h.inc
@@ -24,7 +24,7 @@
 #define LD_P(size) \
     glue(glue(ld, size), glue(ENDIANNESS, _p))
 
-static inline uint32_t ADDRESS_SPACE_LD_CACHED(uw)(MemoryRegionCache *cache,
+static inline uint16_t ADDRESS_SPACE_LD_CACHED(uw)(MemoryRegionCache *cache,
     hwaddr addr, MemTxAttrs attrs, MemTxResult *result)
 {
     assert(addr < cache->len && 2 <= cache->len - addr);
@@ -72,7 +72,7 @@ static inline uint64_t ADDRESS_SPACE_LD_CACHED(q)(MemoryRegionCache *cache,
     glue(glue(st, size), glue(ENDIANNESS, _p))
 
 static inline void ADDRESS_SPACE_ST_CACHED(w)(MemoryRegionCache *cache,
-    hwaddr addr, uint32_t val, MemTxAttrs attrs, MemTxResult *result)
+    hwaddr addr, uint16_t val, MemTxAttrs attrs, MemTxResult *result)
 {
     assert(addr < cache->len && 2 <= cache->len - addr);
     if (likely(cache->ptr)) {
-- 
2.26.3

[RFC PATCH 06/25] exec/memory: Use correct type size

[Philippe Mathieu-Daudé]

Use uint8_t for (unsigned) byte.

Signed-off-by: Philippe Mathieu-Daudé <philmd@redhat.com>
---
 include/exec/memory.h | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/include/exec/memory.h b/include/exec/memory.h
index c8b90889241..175d7151a5d 100644
--- a/include/exec/memory.h
+++ b/include/exec/memory.h
@@ -2305,7 +2305,7 @@ static inline uint8_t address_space_ldub_cached(MemoryRegionCache *cache,
 }
 
 static inline void address_space_stb_cached(MemoryRegionCache *cache,
-    hwaddr addr, uint32_t val, MemTxAttrs attrs, MemTxResult *result)
+    hwaddr addr, uint8_t val, MemTxAttrs attrs, MemTxResult *result)
 {
     assert(addr < cache->len);
     if (likely(cache->ptr)) {
-- 
2.26.3

[RFC PATCH 07/25] qemu/bswap: Introduce ST_CONVERT() macro

[Philippe Mathieu-Daudé]

To be able to add more load/store operations,
introduce the ST_CONVERT() macro.

Signed-off-by: Philippe Mathieu-Daudé <philmd@redhat.com>
---
 include/qemu/bswap.h | 17 +++++++++++++++++
 1 file changed, 17 insertions(+)

diff --git a/include/qemu/bswap.h b/include/qemu/bswap.h
index 2d3bb8bbedd..86f5ded6acf 100644
--- a/include/qemu/bswap.h
+++ b/include/qemu/bswap.h
@@ -449,6 +449,23 @@ static inline void stq_be_p(void *ptr, uint64_t v)
     stq_he_p(ptr, be_bswap(v, 64));
 }
 
+#define ST_CONVERT_UNALIGNED(bits, vtype, size)\
+static inline void st ## size ## _he_p(void *ptr, vtype v)\
+{\
+    __builtin_memcpy(ptr, &v, sizeof(v));\
+}
+
+#define ST_CONVERT_END(endian, bits, vtype, size)\
+static inline void st ## size ## _ ## endian ## _p(void *ptr, vtype v)\
+{\
+    st ## size ## _he_p(ptr, glue(endian, _bswap)(v, bits));\
+}
+
+#define ST_CONVERT(bits, vtype, size)\
+    ST_CONVERT_UNALIGNED(bits, vtype, size)\
+    ST_CONVERT_END(le, bits, vtype, size)\
+    ST_CONVERT_END(be, bits, vtype, size)
+
 static inline unsigned long leul_to_cpu(unsigned long v)
 {
 #if HOST_LONG_BITS == 32
-- 
2.26.3

[RFC PATCH 08/25] qemu/bswap: Use ST_CONVERT() macro to emit 16-bit load/store functions

[Philippe Mathieu-Daudé]

Signed-off-by: Philippe Mathieu-Daudé <philmd@redhat.com>
---
 include/qemu/bswap.h | 17 ++---------------
 1 file changed, 2 insertions(+), 15 deletions(-)

diff --git a/include/qemu/bswap.h b/include/qemu/bswap.h
index 86f5ded6acf..4e2bd2e97ee 100644
--- a/include/qemu/bswap.h
+++ b/include/qemu/bswap.h
@@ -350,11 +350,6 @@ static inline int ldsw_he_p(const void *ptr)
     return r;
 }
 
-static inline void stw_he_p(void *ptr, uint16_t v)
-{
-    __builtin_memcpy(ptr, &v, sizeof(v));
-}
-
 static inline int ldl_he_p(const void *ptr)
 {
     int32_t r;
@@ -399,11 +394,6 @@ static inline uint64_t ldq_le_p(const void *ptr)
     return le_bswap(ldq_he_p(ptr), 64);
 }
 
-static inline void stw_le_p(void *ptr, uint16_t v)
-{
-    stw_he_p(ptr, le_bswap(v, 16));
-}
-
 static inline void stl_le_p(void *ptr, uint32_t v)
 {
     stl_he_p(ptr, le_bswap(v, 32));
@@ -434,11 +424,6 @@ static inline uint64_t ldq_be_p(const void *ptr)
     return be_bswap(ldq_he_p(ptr), 64);
 }
 
-static inline void stw_be_p(void *ptr, uint16_t v)
-{
-    stw_he_p(ptr, be_bswap(v, 16));
-}
-
 static inline void stl_be_p(void *ptr, uint32_t v)
 {
     stl_he_p(ptr, be_bswap(v, 32));
@@ -466,6 +451,8 @@ static inline void st ## size ## _ ## endian ## _p(void *ptr, vtype v)\
     ST_CONVERT_END(le, bits, vtype, size)\
     ST_CONVERT_END(be, bits, vtype, size)
 
+ST_CONVERT(16, uint16_t, w)
+
 static inline unsigned long leul_to_cpu(unsigned long v)
 {
 #if HOST_LONG_BITS == 32
-- 
2.26.3

Re: [RFC PATCH 08/25] qemu/bswap: Use ST_CONVERT() macro to emit 16-bit load/store functions

[Peter Maydell]

On Tue, 18 May 2021 at 19:37, Philippe Mathieu-Daudé <philmd@redhat.com> wrote:
>
> Signed-off-by: Philippe Mathieu-Daudé <philmd@redhat.com>
> ---
>  include/qemu/bswap.h | 17 ++---------------
>  1 file changed, 2 insertions(+), 15 deletions(-)
>
> diff --git a/include/qemu/bswap.h b/include/qemu/bswap.h
> index 86f5ded6acf..4e2bd2e97ee 100644
> --- a/include/qemu/bswap.h
> +++ b/include/qemu/bswap.h
> @@ -350,11 +350,6 @@ static inline int ldsw_he_p(const void *ptr)
>      return r;
>  }
>
> -static inline void stw_he_p(void *ptr, uint16_t v)
> -{
> -    __builtin_memcpy(ptr, &v, sizeof(v));
> -}
> -
>  static inline int ldl_he_p(const void *ptr)
>  {
>      int32_t r;
> @@ -399,11 +394,6 @@ static inline uint64_t ldq_le_p(const void *ptr)
>      return le_bswap(ldq_he_p(ptr), 64);
>  }
>
> -static inline void stw_le_p(void *ptr, uint16_t v)
> -{
> -    stw_he_p(ptr, le_bswap(v, 16));
> -}
> -
>  static inline void stl_le_p(void *ptr, uint32_t v)
>  {
>      stl_he_p(ptr, le_bswap(v, 32));
> @@ -434,11 +424,6 @@ static inline uint64_t ldq_be_p(const void *ptr)
>      return be_bswap(ldq_he_p(ptr), 64);
>  }
>
> -static inline void stw_be_p(void *ptr, uint16_t v)
> -{
> -    stw_he_p(ptr, be_bswap(v, 16));
> -}
> -
>  static inline void stl_be_p(void *ptr, uint32_t v)
>  {
>      stl_he_p(ptr, be_bswap(v, 32));
> @@ -466,6 +451,8 @@ static inline void st ## size ## _ ## endian ## _p(void *ptr, vtype v)\
>      ST_CONVERT_END(le, bits, vtype, size)\
>      ST_CONVERT_END(be, bits, vtype, size)
>
> +ST_CONVERT(16, uint16_t, w)
> +

Where we have a macro that emits a bunch of function declarations,
can we also add a comment that (a) documents the functions and
(b) explicitly lists the name of every generated function?
The latter in particular is really helpful for people who are
trying to find function declarations/definitions with 'grep'.
The comment above the definition and use of the CPU_CONVERT
macro in bswap.h is an example.

thanks
-- PMM

Re: [RFC PATCH 08/25] qemu/bswap: Use ST_CONVERT() macro to emit 16-bit load/store functions

[Philippe Mathieu-Daudé]

On 5/18/21 10:08 PM, Peter Maydell wrote:
> On Tue, 18 May 2021 at 19:37, Philippe Mathieu-Daudé <philmd@redhat.com> wrote:
>>
>> Signed-off-by: Philippe Mathieu-Daudé <philmd@redhat.com>
>> ---
>>  include/qemu/bswap.h | 17 ++---------------
>>  1 file changed, 2 insertions(+), 15 deletions(-)
>>
>> diff --git a/include/qemu/bswap.h b/include/qemu/bswap.h
>> index 86f5ded6acf..4e2bd2e97ee 100644
>> --- a/include/qemu/bswap.h
>> +++ b/include/qemu/bswap.h
>> @@ -350,11 +350,6 @@ static inline int ldsw_he_p(const void *ptr)
>>      return r;
>>  }
>>
>> -static inline void stw_he_p(void *ptr, uint16_t v)
>> -{
>> -    __builtin_memcpy(ptr, &v, sizeof(v));
>> -}
>> -
>>  static inline int ldl_he_p(const void *ptr)
>>  {
>>      int32_t r;
>> @@ -399,11 +394,6 @@ static inline uint64_t ldq_le_p(const void *ptr)
>>      return le_bswap(ldq_he_p(ptr), 64);
>>  }
>>
>> -static inline void stw_le_p(void *ptr, uint16_t v)
>> -{
>> -    stw_he_p(ptr, le_bswap(v, 16));
>> -}
>> -
>>  static inline void stl_le_p(void *ptr, uint32_t v)
>>  {
>>      stl_he_p(ptr, le_bswap(v, 32));
>> @@ -434,11 +424,6 @@ static inline uint64_t ldq_be_p(const void *ptr)
>>      return be_bswap(ldq_he_p(ptr), 64);
>>  }
>>
>> -static inline void stw_be_p(void *ptr, uint16_t v)
>> -{
>> -    stw_he_p(ptr, be_bswap(v, 16));
>> -}
>> -
>>  static inline void stl_be_p(void *ptr, uint32_t v)
>>  {
>>      stl_he_p(ptr, be_bswap(v, 32));
>> @@ -466,6 +451,8 @@ static inline void st ## size ## _ ## endian ## _p(void *ptr, vtype v)\
>>      ST_CONVERT_END(le, bits, vtype, size)\
>>      ST_CONVERT_END(be, bits, vtype, size)
>>
>> +ST_CONVERT(16, uint16_t, w)
>> +
> 
> Where we have a macro that emits a bunch of function declarations,
> can we also add a comment that (a) documents the functions and
> (b) explicitly lists the name of every generated function?
> The latter in particular is really helpful for people who are
> trying to find function declarations/definitions with 'grep'.
> The comment above the definition and use of the CPU_CONVERT
> macro in bswap.h is an example.

For (a) I think the current comment is still valid:

/*
 * the generic syntax is:
 *
 * load: ld{type}{sign}{size}_{endian}_p(ptr)
 *
 * store: st{type}{size}_{endian}_p(ptr, val)
 ...

For (b), certainly, I'll add that list.

Thanks,

Phil.

[RFC PATCH 09/25] qemu/bswap: Introduce LD_CONVERT() macro

[Philippe Mathieu-Daudé]

To be able to add more load/store operations,
introduce the LD_CONVERT() macro.

Signed-off-by: Philippe Mathieu-Daudé <philmd@redhat.com>
---
 include/qemu/bswap.h | 19 +++++++++++++++++++
 1 file changed, 19 insertions(+)

diff --git a/include/qemu/bswap.h b/include/qemu/bswap.h
index 4e2bd2e97ee..c2fd4f31d20 100644
--- a/include/qemu/bswap.h
+++ b/include/qemu/bswap.h
@@ -434,18 +434,37 @@ static inline void stq_be_p(void *ptr, uint64_t v)
     stq_he_p(ptr, be_bswap(v, 64));
 }
 
+#define LD_CONVERT_UNALIGNED(bits, rtype, vtype, size)\
+static inline rtype ld ## size ## _he_p(const void *ptr)\
+{\
+    vtype r;\
+    __builtin_memcpy(&r, ptr, sizeof(r));\
+    return r;\
+}
+
 #define ST_CONVERT_UNALIGNED(bits, vtype, size)\
 static inline void st ## size ## _he_p(void *ptr, vtype v)\
 {\
     __builtin_memcpy(ptr, &v, sizeof(v));\
 }
 
+#define LD_CONVERT_END(endian, bits, rtype, vtype, size)\
+static inline rtype ld ## size ## _ ## endian ## _p(const void *ptr)\
+{\
+    return (vtype)glue(endian, _bswap)(ld ## size ## _he_p(ptr), bits);\
+}
+
 #define ST_CONVERT_END(endian, bits, vtype, size)\
 static inline void st ## size ## _ ## endian ## _p(void *ptr, vtype v)\
 {\
     st ## size ## _he_p(ptr, glue(endian, _bswap)(v, bits));\
 }
 
+#define LD_CONVERT(bits, rtype, vtype, size)\
+    LD_CONVERT_UNALIGNED(bits, rtype, vtype, size)\
+    LD_CONVERT_END(le, bits, rtype, vtype, size)\
+    LD_CONVERT_END(be, bits, rtype, vtype, size)
+
 #define ST_CONVERT(bits, vtype, size)\
     ST_CONVERT_UNALIGNED(bits, vtype, size)\
     ST_CONVERT_END(le, bits, vtype, size)\
-- 
2.26.3

[RFC PATCH 10/25] qemu/bswap: Use LD_CONVERT macro to emit 16-bit signed load/store code

[Philippe Mathieu-Daudé]

Signed-off-by: Philippe Mathieu-Daudé <philmd@redhat.com>
---
 include/qemu/bswap.h | 18 +-----------------
 1 file changed, 1 insertion(+), 17 deletions(-)

diff --git a/include/qemu/bswap.h b/include/qemu/bswap.h
index c2fd4f31d20..af9b18f373d 100644
--- a/include/qemu/bswap.h
+++ b/include/qemu/bswap.h
@@ -343,13 +343,6 @@ static inline int lduw_he_p(const void *ptr)
     return r;
 }
 
-static inline int ldsw_he_p(const void *ptr)
-{
-    int16_t r;
-    __builtin_memcpy(&r, ptr, sizeof(r));
-    return r;
-}
-
 static inline int ldl_he_p(const void *ptr)
 {
     int32_t r;
@@ -379,11 +372,6 @@ static inline int lduw_le_p(const void *ptr)
     return (uint16_t)le_bswap(lduw_he_p(ptr), 16);
 }
 
-static inline int ldsw_le_p(const void *ptr)
-{
-    return (int16_t)le_bswap(lduw_he_p(ptr), 16);
-}
-
 static inline int ldl_le_p(const void *ptr)
 {
     return le_bswap(ldl_he_p(ptr), 32);
@@ -409,11 +397,6 @@ static inline int lduw_be_p(const void *ptr)
     return (uint16_t)be_bswap(lduw_he_p(ptr), 16);
 }
 
-static inline int ldsw_be_p(const void *ptr)
-{
-    return (int16_t)be_bswap(lduw_he_p(ptr), 16);
-}
-
 static inline int ldl_be_p(const void *ptr)
 {
     return be_bswap(ldl_he_p(ptr), 32);
@@ -471,6 +454,7 @@ static inline void st ## size ## _ ## endian ## _p(void *ptr, vtype v)\
     ST_CONVERT_END(be, bits, vtype, size)
 
 ST_CONVERT(16, uint16_t, w)
+LD_CONVERT(16, int, int16_t, sw)
 
 static inline unsigned long leul_to_cpu(unsigned long v)
 {
-- 
2.26.3

[RFC PATCH 11/25] qemu/bswap: Use LD_CONVERT macro to emit 16-bit unsigned load/store code

[Philippe Mathieu-Daudé]

Signed-off-by: Philippe Mathieu-Daudé <philmd@redhat.com>
---
 include/qemu/bswap.h | 18 +-----------------
 1 file changed, 1 insertion(+), 17 deletions(-)

diff --git a/include/qemu/bswap.h b/include/qemu/bswap.h
index af9b18f373d..ee86dc4ed8c 100644
--- a/include/qemu/bswap.h
+++ b/include/qemu/bswap.h
@@ -336,13 +336,6 @@ static inline void stb_p(void *ptr, uint8_t v)
  * of good performance.
  */
 
-static inline int lduw_he_p(const void *ptr)
-{
-    uint16_t r;
-    __builtin_memcpy(&r, ptr, sizeof(r));
-    return r;
-}
-
 static inline int ldl_he_p(const void *ptr)
 {
     int32_t r;
@@ -367,11 +360,6 @@ static inline void stq_he_p(void *ptr, uint64_t v)
     __builtin_memcpy(ptr, &v, sizeof(v));
 }
 
-static inline int lduw_le_p(const void *ptr)
-{
-    return (uint16_t)le_bswap(lduw_he_p(ptr), 16);
-}
-
 static inline int ldl_le_p(const void *ptr)
 {
     return le_bswap(ldl_he_p(ptr), 32);
@@ -392,11 +380,6 @@ static inline void stq_le_p(void *ptr, uint64_t v)
     stq_he_p(ptr, le_bswap(v, 64));
 }
 
-static inline int lduw_be_p(const void *ptr)
-{
-    return (uint16_t)be_bswap(lduw_he_p(ptr), 16);
-}
-
 static inline int ldl_be_p(const void *ptr)
 {
     return be_bswap(ldl_he_p(ptr), 32);
@@ -455,6 +438,7 @@ static inline void st ## size ## _ ## endian ## _p(void *ptr, vtype v)\
 
 ST_CONVERT(16, uint16_t, w)
 LD_CONVERT(16, int, int16_t, sw)
+LD_CONVERT(16, int, uint16_t, uw)
 
 static inline unsigned long leul_to_cpu(unsigned long v)
 {
-- 
2.26.3

[RFC PATCH 12/25] qemu/bswap: Use LDST_CONVERT macro to emit 32-bit load/store functions

[Philippe Mathieu-Daudé]

Signed-off-by: Philippe Mathieu-Daudé <philmd@redhat.com>
---
 include/qemu/bswap.h | 37 +++++--------------------------------
 1 file changed, 5 insertions(+), 32 deletions(-)

diff --git a/include/qemu/bswap.h b/include/qemu/bswap.h
index ee86dc4ed8c..a041be94a7a 100644
--- a/include/qemu/bswap.h
+++ b/include/qemu/bswap.h
@@ -336,18 +336,6 @@ static inline void stb_p(void *ptr, uint8_t v)
  * of good performance.
  */
 
-static inline int ldl_he_p(const void *ptr)
-{
-    int32_t r;
-    __builtin_memcpy(&r, ptr, sizeof(r));
-    return r;
-}
-
-static inline void stl_he_p(void *ptr, uint32_t v)
-{
-    __builtin_memcpy(ptr, &v, sizeof(v));
-}
-
 static inline uint64_t ldq_he_p(const void *ptr)
 {
     uint64_t r;
@@ -360,41 +348,21 @@ static inline void stq_he_p(void *ptr, uint64_t v)
     __builtin_memcpy(ptr, &v, sizeof(v));
 }
 
-static inline int ldl_le_p(const void *ptr)
-{
-    return le_bswap(ldl_he_p(ptr), 32);
-}
-
 static inline uint64_t ldq_le_p(const void *ptr)
 {
     return le_bswap(ldq_he_p(ptr), 64);
 }
 
-static inline void stl_le_p(void *ptr, uint32_t v)
-{
-    stl_he_p(ptr, le_bswap(v, 32));
-}
-
 static inline void stq_le_p(void *ptr, uint64_t v)
 {
     stq_he_p(ptr, le_bswap(v, 64));
 }
 
-static inline int ldl_be_p(const void *ptr)
-{
-    return be_bswap(ldl_he_p(ptr), 32);
-}
-
 static inline uint64_t ldq_be_p(const void *ptr)
 {
     return be_bswap(ldq_he_p(ptr), 64);
 }
 
-static inline void stl_be_p(void *ptr, uint32_t v)
-{
-    stl_he_p(ptr, be_bswap(v, 32));
-}
-
 static inline void stq_be_p(void *ptr, uint64_t v)
 {
     stq_he_p(ptr, be_bswap(v, 64));
@@ -436,9 +404,14 @@ static inline void st ## size ## _ ## endian ## _p(void *ptr, vtype v)\
     ST_CONVERT_END(le, bits, vtype, size)\
     ST_CONVERT_END(be, bits, vtype, size)
 
+#define LDST_CONVERT(bits, rtype, vtype, size)\
+    LD_CONVERT(bits, rtype, vtype, size)\
+    ST_CONVERT(bits, vtype, size)
+
 ST_CONVERT(16, uint16_t, w)
 LD_CONVERT(16, int, int16_t, sw)
 LD_CONVERT(16, int, uint16_t, uw)
+LDST_CONVERT(32, int, uint32_t, l)
 
 static inline unsigned long leul_to_cpu(unsigned long v)
 {
-- 
2.26.3

[RFC PATCH 13/25] qemu/bswap: Use LDST_CONVERT macro to emit 64-bit load/store functions

[Philippe Mathieu-Daudé]

Signed-off-by: Philippe Mathieu-Daudé <philmd@redhat.com>
---
 include/qemu/bswap.h | 33 +--------------------------------
 1 file changed, 1 insertion(+), 32 deletions(-)

diff --git a/include/qemu/bswap.h b/include/qemu/bswap.h
index a041be94a7a..4cd120ca014 100644
--- a/include/qemu/bswap.h
+++ b/include/qemu/bswap.h
@@ -336,38 +336,6 @@ static inline void stb_p(void *ptr, uint8_t v)
  * of good performance.
  */
 
-static inline uint64_t ldq_he_p(const void *ptr)
-{
-    uint64_t r;
-    __builtin_memcpy(&r, ptr, sizeof(r));
-    return r;
-}
-
-static inline void stq_he_p(void *ptr, uint64_t v)
-{
-    __builtin_memcpy(ptr, &v, sizeof(v));
-}
-
-static inline uint64_t ldq_le_p(const void *ptr)
-{
-    return le_bswap(ldq_he_p(ptr), 64);
-}
-
-static inline void stq_le_p(void *ptr, uint64_t v)
-{
-    stq_he_p(ptr, le_bswap(v, 64));
-}
-
-static inline uint64_t ldq_be_p(const void *ptr)
-{
-    return be_bswap(ldq_he_p(ptr), 64);
-}
-
-static inline void stq_be_p(void *ptr, uint64_t v)
-{
-    stq_he_p(ptr, be_bswap(v, 64));
-}
-
 #define LD_CONVERT_UNALIGNED(bits, rtype, vtype, size)\
 static inline rtype ld ## size ## _he_p(const void *ptr)\
 {\
@@ -412,6 +380,7 @@ ST_CONVERT(16, uint16_t, w)
 LD_CONVERT(16, int, int16_t, sw)
 LD_CONVERT(16, int, uint16_t, uw)
 LDST_CONVERT(32, int, uint32_t, l)
+LDST_CONVERT(64, uint64_t, uint64_t, q)
 
 static inline unsigned long leul_to_cpu(unsigned long v)
 {
-- 
2.26.3

[RFC PATCH 14/25] qemu/bswap: Introduce load/store for aligned pointer

[Philippe Mathieu-Daudé]

When the pointer alignment is known to be safe, we can
directly swap the data in place, without having to rely
on the compiler builtin code.

Load/store methods expecting aligned pointer use the 'a'
infix. For example to read a 16-bit unsigned value stored
in little endianess at an unaligned pointer:

  val = lduw_le_p(&unaligned_ptr);

then to store it in big endianess at an aligned pointer:

  stw_be_ap(&aligned_ptr, val);

Suggested-by: Stefan Hajnoczi <stefanha@redhat.com>
Signed-off-by: Philippe Mathieu-Daudé <philmd@redhat.com>
---
 docs/devel/loads-stores.rst | 27 ++++++++++++++++-----------
 include/qemu/bswap.h        | 22 ++++++++++++++++++++++
 2 files changed, 38 insertions(+), 11 deletions(-)

diff --git a/docs/devel/loads-stores.rst b/docs/devel/loads-stores.rst
index 568274baec0..88493ba1293 100644
--- a/docs/devel/loads-stores.rst
+++ b/docs/devel/loads-stores.rst
@@ -13,20 +13,21 @@ documentation of each API -- for that you should look at the
 documentation comments in the relevant header files.
 
 
-``ld*_p and st*_p``
-~~~~~~~~~~~~~~~~~~~
+``ld*_[a]p and st*_[a]p``
+~~~~~~~~~~~~~~~~~~~~~~~~~
 
 These functions operate on a host pointer, and should be used
 when you already have a pointer into host memory (corresponding
 to guest ram or a local buffer). They deal with doing accesses
 with the desired endianness and with correctly handling
-potentially unaligned pointer values.
+potentially unaligned pointer values. If the pointer alignment
+is known to be safe, then the aligned functions can be used.
 
 Function names follow the pattern:
 
-load: ``ld{sign}{size}_{endian}_p(ptr)``
+load: ``ld{sign}{size}_{endian}_{aligned}p(ptr)``
 
-store: ``st{size}_{endian}_p(ptr, val)``
+store: ``st{size}_{endian}_{aligned}p(ptr, val)``
 
 ``sign``
  - (empty) : for 32 or 64 bit sizes
@@ -49,24 +50,28 @@ The ``_{endian}`` infix is omitted for target-endian accesses.
 The target endian accessors are only available to source
 files which are built per-target.
 
+By using the ``_{aligned}`` infix, unsafe optimizations might be used,
+however unaligned pointer might trigger an exception and abort the
+process.
+
 There are also functions which take the size as an argument:
 
-load: ``ldn{endian}_p(ptr, sz)``
+load: ``ldn{endian}_{aligned}p(ptr, sz)``
 
 which performs an unsigned load of ``sz`` bytes from ``ptr``
 as an ``{endian}`` order value and returns it in a uint64_t.
 
-store: ``stn{endian}_p(ptr, sz, val)``
+store: ``stn{endian}_{aligned}p(ptr, sz, val)``
 
 which stores ``val`` to ``ptr`` as an ``{endian}`` order value
 of size ``sz`` bytes.
 
 
 Regexes for git grep
- - ``\<ld[us]\?[bwlq]\(_[hbl]e\)\?_p\>``
- - ``\<st[bwlq]\(_[hbl]e\)\?_p\>``
- - ``\<ldn_\([hbl]e\)?_p\>``
- - ``\<stn_\([hbl]e\)?_p\>``
+ - ``\<ld[us]\?[bwlq]\(_[hbl]e\)\?_a?p\>``
+ - ``\<st[bwlq]\(_[hbl]e\)\?_a?p\>``
+ - ``\<ldn_\([hbl]e\)?_a?p\>``
+ - ``\<stn_\([hbl]e\)?_a?p\>``
 
 ``cpu_{ld,st}*_mmuidx_ra``
 ~~~~~~~~~~~~~~~~~~~~~~~~~~
diff --git a/include/qemu/bswap.h b/include/qemu/bswap.h
index 4cd120ca014..3f272c3cb46 100644
--- a/include/qemu/bswap.h
+++ b/include/qemu/bswap.h
@@ -350,25 +350,47 @@ static inline void st ## size ## _he_p(void *ptr, vtype v)\
     __builtin_memcpy(ptr, &v, sizeof(v));\
 }
 
+#define LD_CONVERT_ALIGNED(bits, rtype, vtype, size)\
+static inline rtype ld ## size ## _he_ap(const void *ptr)\
+{\
+    return *(vtype *)ptr;\
+}
+
+#define ST_CONVERT_ALIGNED(bits, vtype, size)\
+static inline void st ## size ## _he_ap(void *ptr, vtype v)\
+{\
+    *(vtype *)ptr = v;\
+}
+
 #define LD_CONVERT_END(endian, bits, rtype, vtype, size)\
 static inline rtype ld ## size ## _ ## endian ## _p(const void *ptr)\
 {\
     return (vtype)glue(endian, _bswap)(ld ## size ## _he_p(ptr), bits);\
+}\
+static inline rtype ld ## size ## _ ## endian ## _ap(const void *ptr)\
+{\
+    return (vtype)glue(endian, _bswap)(ld ## size ## _he_ap(ptr), bits);\
 }
 
 #define ST_CONVERT_END(endian, bits, vtype, size)\
 static inline void st ## size ## _ ## endian ## _p(void *ptr, vtype v)\
 {\
     st ## size ## _he_p(ptr, glue(endian, _bswap)(v, bits));\
+}\
+static inline void st ## size ## _ ## endian ## _ap(void *ptr, vtype v)\
+{\
+    st ## size ## _he_ap(ptr, glue(endian, _bswap)(v, bits));\
 }
 
 #define LD_CONVERT(bits, rtype, vtype, size)\
     LD_CONVERT_UNALIGNED(bits, rtype, vtype, size)\
+    LD_CONVERT_ALIGNED(bits, rtype, vtype, size)\
     LD_CONVERT_END(le, bits, rtype, vtype, size)\
     LD_CONVERT_END(be, bits, rtype, vtype, size)
 
 #define ST_CONVERT(bits, vtype, size)\
     ST_CONVERT_UNALIGNED(bits, vtype, size)\
+    ST_CONVERT_ALIGNED(bits, vtype, size)\
     ST_CONVERT_END(le, bits, vtype, size)\
     ST_CONVERT_END(be, bits, vtype, size)
 
-- 
2.26.3

Re: [RFC PATCH 14/25] qemu/bswap: Introduce load/store for aligned pointer

[Peter Maydell]

On Tue, 18 May 2021 at 19:38, Philippe Mathieu-Daudé <philmd@redhat.com> wrote:
>
> When the pointer alignment is known to be safe, we can
> directly swap the data in place, without having to rely
> on the compiler builtin code.
>
> Load/store methods expecting aligned pointer use the 'a'
> infix. For example to read a 16-bit unsigned value stored
> in little endianess at an unaligned pointer:
>
>   val = lduw_le_p(&unaligned_ptr);
>
> then to store it in big endianess at an aligned pointer:
>
>   stw_be_ap(&aligned_ptr, val);

It sounded from the bug report as if the desired effect
was "this access is atomic". Nothing in the documentation here
makes that guarantee of the implementation -- it merely imposes
an extra requirement on the caller that the pointer alignment
is "safe" (which term it does not define...) and a valid
implementation would be to implement the "aligned" versions
identically to the "unaligned" versions...

Q: should the functions at the bottom of this stack of APIs
be using something from the atomic.h header? If not, why not?
Do we need any of the other atomic primitives ?

thanks
-- PMM

Re: [RFC PATCH 14/25] qemu/bswap: Introduce load/store for aligned pointer

[Philippe Mathieu-Daudé]

On 5/18/21 10:15 PM, Peter Maydell wrote:
> On Tue, 18 May 2021 at 19:38, Philippe Mathieu-Daudé <philmd@redhat.com> wrote:
>>
>> When the pointer alignment is known to be safe, we can
>> directly swap the data in place, without having to rely
>> on the compiler builtin code.
>>
>> Load/store methods expecting aligned pointer use the 'a'
>> infix. For example to read a 16-bit unsigned value stored
>> in little endianess at an unaligned pointer:
>>
>>   val = lduw_le_p(&unaligned_ptr);
>>
>> then to store it in big endianess at an aligned pointer:
>>
>>   stw_be_ap(&aligned_ptr, val);
> 
> It sounded from the bug report as if the desired effect
> was "this access is atomic". Nothing in the documentation here
> makes that guarantee of the implementation -- it merely imposes
> an extra requirement on the caller that the pointer alignment
> is "safe" (which term it does not define...) and a valid
> implementation would be to implement the "aligned" versions
> identically to the "unaligned" versions...
> 
> Q: should the functions at the bottom of this stack of APIs
> be using something from the atomic.h header? If not, why not?
> Do we need any of the other atomic primitives ?

I'll defer this question to Stefan/Paolo...

Re: [RFC PATCH 14/25] qemu/bswap: Introduce load/store for aligned pointer

[Stefan Hajnoczi]

[-- Attachment #1: Type: text/plain, Size: 2092 bytes --]

On Wed, May 19, 2021 at 07:56:51AM +0200, Philippe Mathieu-Daudé wrote:
> On 5/18/21 10:15 PM, Peter Maydell wrote:
> > On Tue, 18 May 2021 at 19:38, Philippe Mathieu-Daudé <philmd@redhat.com> wrote:
> >>
> >> When the pointer alignment is known to be safe, we can
> >> directly swap the data in place, without having to rely
> >> on the compiler builtin code.
> >>
> >> Load/store methods expecting aligned pointer use the 'a'
> >> infix. For example to read a 16-bit unsigned value stored
> >> in little endianess at an unaligned pointer:
> >>
> >>   val = lduw_le_p(&unaligned_ptr);
> >>
> >> then to store it in big endianess at an aligned pointer:
> >>
> >>   stw_be_ap(&aligned_ptr, val);
> > 
> > It sounded from the bug report as if the desired effect
> > was "this access is atomic". Nothing in the documentation here
> > makes that guarantee of the implementation -- it merely imposes
> > an extra requirement on the caller that the pointer alignment
> > is "safe" (which term it does not define...) and a valid
> > implementation would be to implement the "aligned" versions
> > identically to the "unaligned" versions...
> > 
> > Q: should the functions at the bottom of this stack of APIs
> > be using something from the atomic.h header? If not, why not?
> > Do we need any of the other atomic primitives ?
> 
> I'll defer this question to Stefan/Paolo...

Stricly speaking qatomic_read() and qatomic_set() are necessary for
__ATOMIC_RELAXED semantics.

The comment in atomic.h mentions this generally has no effect on
generated code. That's probably because aligned 16/32/64-bit accesses
don't tear on modern CPUs so no special instructions are needed. This is
why not using atomic.h usually works.

Even qatomic_read()/qatomic_set() is too strong since the doc comment
mentions it prevents the compiler moving other loads/stores past the
atomic operation. The hw/virtio/ code already has explicit memory
barriers and doesn't need the additional compiler barrier.

For safety I suggest using qatomic_read()/qatomic_set().

Stefan

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]

[RFC PATCH 15/25] exec/memory: Add methods for aligned pointer access (address space)

[Philippe Mathieu-Daudé]

Signed-off-by: Philippe Mathieu-Daudé <philmd@redhat.com>
---
 include/exec/memory.h                 | 11 +++++++++++
 include/exec/memory_ldst_cached.h.inc | 20 ++++++++++++++++----
 2 files changed, 27 insertions(+), 4 deletions(-)

diff --git a/include/exec/memory.h b/include/exec/memory.h
index 175d7151a5d..7eeabbceef3 100644
--- a/include/exec/memory.h
+++ b/include/exec/memory.h
@@ -2315,12 +2315,23 @@ static inline void address_space_stb_cached(MemoryRegionCache *cache,
     }
 }
 
+#define address_space_ldub_cached_aligned   address_space_ldub_cached
+#define address_space_stb_cached_aligned    address_space_stb_cached
+
 #define ENDIANNESS   _le
 #include "exec/memory_ldst_cached.h.inc"
 
 #define ENDIANNESS   _be
 #include "exec/memory_ldst_cached.h.inc"
 
+#define LDST_ALIGNED
+#define ENDIANNESS   _le
+#include "exec/memory_ldst_cached.h.inc"
+
+#define LDST_ALIGNED
+#define ENDIANNESS   _be
+#include "exec/memory_ldst_cached.h.inc"
+
 #define SUFFIX       _cached
 #define ARG1         cache
 #define ARG1_DECL    MemoryRegionCache *cache
diff --git a/include/exec/memory_ldst_cached.h.inc b/include/exec/memory_ldst_cached.h.inc
index d7834f852c4..db45e688c03 100644
--- a/include/exec/memory_ldst_cached.h.inc
+++ b/include/exec/memory_ldst_cached.h.inc
@@ -17,12 +17,21 @@
  * License along with this library; if not, see <http://www.gnu.org/licenses/>.
  */
 
+#ifdef LDST_ALIGNED
+#define FUNC_SUFFIX _cached_aligned
+#define LDST_SUFFIX _ap
+#undef LDST_ALIGNED
+#else
+#define FUNC_SUFFIX _cached
+#define LDST_SUFFIX _p
+#endif
+
 #define ADDRESS_SPACE_LD_CACHED(size) \
-    glue(glue(address_space_ld, size), glue(ENDIANNESS, _cached))
+    glue(glue(address_space_ld, size), glue(ENDIANNESS, FUNC_SUFFIX))
 #define ADDRESS_SPACE_LD_CACHED_SLOW(size) \
     glue(glue(address_space_ld, size), glue(ENDIANNESS, _cached_slow))
 #define LD_P(size) \
-    glue(glue(ld, size), glue(ENDIANNESS, _p))
+    glue(glue(ld, size), glue(ENDIANNESS, LDST_SUFFIX))
 
 static inline uint16_t ADDRESS_SPACE_LD_CACHED(uw)(MemoryRegionCache *cache,
     hwaddr addr, MemTxAttrs attrs, MemTxResult *result)
@@ -65,11 +74,11 @@ static inline uint64_t ADDRESS_SPACE_LD_CACHED(q)(MemoryRegionCache *cache,
 #undef LD_P
 
 #define ADDRESS_SPACE_ST_CACHED(size) \
-    glue(glue(address_space_st, size), glue(ENDIANNESS, _cached))
+    glue(glue(address_space_st, size), glue(ENDIANNESS, FUNC_SUFFIX))
 #define ADDRESS_SPACE_ST_CACHED_SLOW(size) \
     glue(glue(address_space_st, size), glue(ENDIANNESS, _cached_slow))
 #define ST_P(size) \
-    glue(glue(st, size), glue(ENDIANNESS, _p))
+    glue(glue(st, size), glue(ENDIANNESS, LDST_SUFFIX))
 
 static inline void ADDRESS_SPACE_ST_CACHED(w)(MemoryRegionCache *cache,
     hwaddr addr, uint16_t val, MemTxAttrs attrs, MemTxResult *result)
@@ -108,4 +117,7 @@ static inline void ADDRESS_SPACE_ST_CACHED(q)(MemoryRegionCache *cache,
 #undef ADDRESS_SPACE_ST_CACHED_SLOW
 #undef ST_P
 
+#undef FUNC_SUFFIX
+#undef LDST_SUFFIX
+
 #undef ENDIANNESS
-- 
2.26.3

[RFC PATCH 16/25] exec/memory: Add methods for aligned pointer access (physical memory)

[Philippe Mathieu-Daudé]

Signed-off-by: Philippe Mathieu-Daudé <philmd@redhat.com>
---
 include/exec/memory.h | 5 +++++
 1 file changed, 5 insertions(+)

diff --git a/include/exec/memory.h b/include/exec/memory.h
index 7eeabbceef3..28c0e68e7d8 100644
--- a/include/exec/memory.h
+++ b/include/exec/memory.h
@@ -2337,6 +2337,11 @@ static inline void address_space_stb_cached(MemoryRegionCache *cache,
 #define ARG1_DECL    MemoryRegionCache *cache
 #include "exec/memory_ldst_phys.h.inc"
 
+#define SUFFIX       _cached_aligned
+#define ARG1         cache
+#define ARG1_DECL    MemoryRegionCache *cache
+#include "exec/memory_ldst_phys.h.inc"
+
 /* address_space_cache_init: prepare for repeated access to a physical
  * memory region
  *
-- 
2.26.3

[RFC PATCH 17/25] hw/virtio: Use correct type sizes

[Philippe Mathieu-Daudé]

Use uint16_t for unsigned 16-bit data and uint32_t for unsigned 32-bit.

Signed-off-by: Philippe Mathieu-Daudé <philmd@redhat.com>
---
 include/hw/virtio/virtio-access.h | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/include/hw/virtio/virtio-access.h b/include/hw/virtio/virtio-access.h
index 6818a23a2d3..ae8c9feffc5 100644
--- a/include/hw/virtio/virtio-access.h
+++ b/include/hw/virtio/virtio-access.h
@@ -120,7 +120,7 @@ static inline void virtio_stq_p(VirtIODevice *vdev, void *ptr, uint64_t v)
     }
 }
 
-static inline int virtio_lduw_p(VirtIODevice *vdev, const void *ptr)
+static inline uint16_t virtio_lduw_p(VirtIODevice *vdev, const void *ptr)
 {
     if (virtio_access_is_big_endian(vdev)) {
         return lduw_be_p(ptr);
@@ -129,7 +129,7 @@ static inline int virtio_lduw_p(VirtIODevice *vdev, const void *ptr)
     }
 }
 
-static inline int virtio_ldl_p(VirtIODevice *vdev, const void *ptr)
+static inline uint32_t virtio_ldl_p(VirtIODevice *vdev, const void *ptr)
 {
     if (virtio_access_is_big_endian(vdev)) {
         return ldl_be_p(ptr);
-- 
2.26.3

Re: [RFC PATCH 17/25] hw/virtio: Use correct type sizes

[Richard Henderson]

On 5/18/21 1:36 PM, Philippe Mathieu-Daudé wrote:
> -static inline int virtio_lduw_p(VirtIODevice *vdev, const void *ptr)
> +static inline uint16_t virtio_lduw_p(VirtIODevice *vdev, const void *ptr)

While this one looks obviously correct,

> -static inline int virtio_ldl_p(VirtIODevice *vdev, const void *ptr)
> +static inline uint32_t virtio_ldl_p(VirtIODevice *vdev, const void *ptr)
>   {
>       if (virtio_access_is_big_endian(vdev)) {
>           return ldl_be_p(ptr);

this one isn't so obvious.

Are all of the users unsigned?  If so, should we rename it ldul?


r~

Re: [RFC PATCH 17/25] hw/virtio: Use correct type sizes

[Philippe Mathieu-Daudé]

On 5/20/21 6:27 PM, Richard Henderson wrote:
> On 5/18/21 1:36 PM, Philippe Mathieu-Daudé wrote:
>> -static inline int virtio_lduw_p(VirtIODevice *vdev, const void *ptr)
>> +static inline uint16_t virtio_lduw_p(VirtIODevice *vdev, const void
>> *ptr)
> 
> While this one looks obviously correct,
> 
>> -static inline int virtio_ldl_p(VirtIODevice *vdev, const void *ptr)
>> +static inline uint32_t virtio_ldl_p(VirtIODevice *vdev, const void *ptr)
>>   {
>>       if (virtio_access_is_big_endian(vdev)) {
>>           return ldl_be_p(ptr);
> 
> this one isn't so obvious.
> 
> Are all of the users unsigned?

All except this one for which I'm not sure:

hw/block/virtio-blk.c:137:            int p =
virtio_ldl_p(VIRTIO_DEVICE(s), &req->out.type);
hw/block/virtio-blk.c-138-            bool is_read = !(p &
VIRTIO_BLK_T_OUT);

--

hw/block/virtio-blk.c:183:    bool is_write_zeroes =
(virtio_ldl_p(VIRTIO_DEVICE(s), &req->out.type) &
hw/block/virtio-blk.c-184-
~VIRTIO_BLK_T_BARRIER) == VIRTIO_BLK_T_WRITE_ZEROES;
hw/block/virtio-blk.c-185-

/* Barrier before this op. */
#define VIRTIO_BLK_T_BARRIER    0x80000000

>  If so, should we rename it ldul?

OK.

[RFC PATCH 18/25] hw/virtio: Introduce VIRTIO_LD_CONVERT() macro

[Philippe Mathieu-Daudé]

To be able to add more load/store operations,
introduce the VIRTIO_LD_CONVERT() macro.

Signed-off-by: Philippe Mathieu-Daudé <philmd@redhat.com>
---
 include/hw/virtio/virtio-access.h | 29 +++++++++++++++++++++++++++++
 1 file changed, 29 insertions(+)

diff --git a/include/hw/virtio/virtio-access.h b/include/hw/virtio/virtio-access.h
index ae8c9feffc5..37e1e6ea535 100644
--- a/include/hw/virtio/virtio-access.h
+++ b/include/hw/virtio/virtio-access.h
@@ -39,6 +39,35 @@ static inline bool virtio_access_is_big_endian(VirtIODevice *vdev)
 #endif
 }
 
+#define VIRTIO_LD_CONVERT(size, rtype)\
+static inline rtype virtio_ld ## size ## _phys(VirtIODevice *vdev, hwaddr pa)\
+{\
+    AddressSpace *dma_as = vdev->dma_as;\
+\
+    if (virtio_access_is_big_endian(vdev)) {\
+        return ld ## size ## _be_phys(dma_as, pa);\
+    }\
+    return ld ## size ## _le_phys(dma_as, pa);\
+}\
+static inline rtype virtio_ld ## size ## _p(VirtIODevice *vdev,\
+                                            const void *ptr)\
+{\
+    if (virtio_access_is_big_endian(vdev)) {\
+        return ld ## size ## _be_p(ptr);\
+    } else {\
+        return ld ## size ## _le_p(ptr);\
+    }\
+}\
+static inline rtype virtio_ld ## size ## _phys_cached(VirtIODevice *vdev,\
+                                                      MemoryRegionCache *cache,\
+                                                      hwaddr pa)\
+{\
+    if (virtio_access_is_big_endian(vdev)) {\
+        return ld ## size ## _be_phys_cached(cache, pa);\
+    }\
+    return ld ## size ## _le_phys_cached(cache, pa);\
+}
+
 static inline uint16_t virtio_lduw_phys(VirtIODevice *vdev, hwaddr pa)
 {
     AddressSpace *dma_as = vdev->dma_as;
-- 
2.26.3

[RFC PATCH 19/25] hw/virtio: Use LD_CONVERT macro to emit 16-bit unsigned load/store code

[Philippe Mathieu-Daudé]

Signed-off-by: Philippe Mathieu-Daudé <philmd@redhat.com>
---
 include/hw/virtio/virtio-access.h | 29 +----------------------------
 1 file changed, 1 insertion(+), 28 deletions(-)

diff --git a/include/hw/virtio/virtio-access.h b/include/hw/virtio/virtio-access.h
index 37e1e6ea535..594247f6e35 100644
--- a/include/hw/virtio/virtio-access.h
+++ b/include/hw/virtio/virtio-access.h
@@ -68,15 +68,7 @@ static inline rtype virtio_ld ## size ## _phys_cached(VirtIODevice *vdev,\
     return ld ## size ## _le_phys_cached(cache, pa);\
 }
 
-static inline uint16_t virtio_lduw_phys(VirtIODevice *vdev, hwaddr pa)
-{
-    AddressSpace *dma_as = vdev->dma_as;
-
-    if (virtio_access_is_big_endian(vdev)) {
-        return lduw_be_phys(dma_as, pa);
-    }
-    return lduw_le_phys(dma_as, pa);
-}
+VIRTIO_LD_CONVERT(uw, uint16_t)
 
 static inline uint32_t virtio_ldl_phys(VirtIODevice *vdev, hwaddr pa)
 {
@@ -149,15 +141,6 @@ static inline void virtio_stq_p(VirtIODevice *vdev, void *ptr, uint64_t v)
     }
 }
 
-static inline uint16_t virtio_lduw_p(VirtIODevice *vdev, const void *ptr)
-{
-    if (virtio_access_is_big_endian(vdev)) {
-        return lduw_be_p(ptr);
-    } else {
-        return lduw_le_p(ptr);
-    }
-}
-
 static inline uint32_t virtio_ldl_p(VirtIODevice *vdev, const void *ptr)
 {
     if (virtio_access_is_big_endian(vdev)) {
@@ -185,16 +168,6 @@ static inline uint16_t virtio_tswap16(VirtIODevice *vdev, uint16_t s)
 #endif
 }
 
-static inline uint16_t virtio_lduw_phys_cached(VirtIODevice *vdev,
-                                               MemoryRegionCache *cache,
-                                               hwaddr pa)
-{
-    if (virtio_access_is_big_endian(vdev)) {
-        return lduw_be_phys_cached(cache, pa);
-    }
-    return lduw_le_phys_cached(cache, pa);
-}
-
 static inline uint32_t virtio_ldl_phys_cached(VirtIODevice *vdev,
                                               MemoryRegionCache *cache,
                                               hwaddr pa)
-- 
2.26.3

[RFC PATCH 20/25] hw/virtio: Introduce VIRTIO_ST_CONVERT() macro

[Philippe Mathieu-Daudé]

To be able to add more load/store operations,
introduce the VIRTIO_ST_CONVERT() macro.

Signed-off-by: Philippe Mathieu-Daudé <philmd@redhat.com>
---
 include/hw/virtio/virtio-access.h | 32 +++++++++++++++++++++++++++++++
 1 file changed, 32 insertions(+)

diff --git a/include/hw/virtio/virtio-access.h b/include/hw/virtio/virtio-access.h
index 594247f6e35..a86819ef2fe 100644
--- a/include/hw/virtio/virtio-access.h
+++ b/include/hw/virtio/virtio-access.h
@@ -68,6 +68,38 @@ static inline rtype virtio_ld ## size ## _phys_cached(VirtIODevice *vdev,\
     return ld ## size ## _le_phys_cached(cache, pa);\
 }
 
+#define VIRTIO_ST_CONVERT(size, vtype)\
+static inline void virtio_st## size ## _p(VirtIODevice *vdev,\
+                                          void *ptr, vtype v)\
+{\
+    if (virtio_access_is_big_endian(vdev)) {\
+        st## size ## _be_p(ptr, v);\
+    } else {\
+        st## size ## _le_p(ptr, v);\
+    }\
+}\
+static inline void virtio_st## size ## _phys(VirtIODevice *vdev,\
+                                             hwaddr pa, vtype value)\
+{\
+    AddressSpace *dma_as = vdev->dma_as;\
+\
+    if (virtio_access_is_big_endian(vdev)) {\
+        st## size ## _be_phys(dma_as, pa, value);\
+    } else {\
+        st## size ## _le_phys(dma_as, pa, value);\
+    }\
+}\
+static inline void virtio_st ## size ## _phys_cached(VirtIODevice *vdev,\
+                                                     MemoryRegionCache *cache,\
+                                                     hwaddr pa, vtype value)\
+{\
+    if (virtio_access_is_big_endian(vdev)) {\
+        st ## size ## _be_phys_cached(cache, pa, value);\
+    } else {\
+        st ## size ## _le_phys_cached(cache, pa, value);\
+    }\
+}
+
 VIRTIO_LD_CONVERT(uw, uint16_t)
 
 static inline uint32_t virtio_ldl_phys(VirtIODevice *vdev, hwaddr pa)
-- 
2.26.3

[RFC PATCH 21/25] hw/virtio: Use ST_CONVERT() macro to emit 16-bit load/store functions

[Philippe Mathieu-Daudé]

Signed-off-by: Philippe Mathieu-Daudé <philmd@redhat.com>
---
 include/hw/virtio/virtio-access.h | 33 +------------------------------
 1 file changed, 1 insertion(+), 32 deletions(-)

diff --git a/include/hw/virtio/virtio-access.h b/include/hw/virtio/virtio-access.h
index a86819ef2fe..4341af9cb0f 100644
--- a/include/hw/virtio/virtio-access.h
+++ b/include/hw/virtio/virtio-access.h
@@ -101,6 +101,7 @@ static inline void virtio_st ## size ## _phys_cached(VirtIODevice *vdev,\
 }
 
 VIRTIO_LD_CONVERT(uw, uint16_t)
+VIRTIO_ST_CONVERT(w, uint16_t)
 
 static inline uint32_t virtio_ldl_phys(VirtIODevice *vdev, hwaddr pa)
 {
@@ -122,18 +123,6 @@ static inline uint64_t virtio_ldq_phys(VirtIODevice *vdev, hwaddr pa)
     return ldq_le_phys(dma_as, pa);
 }
 
-static inline void virtio_stw_phys(VirtIODevice *vdev, hwaddr pa,
-                                   uint16_t value)
-{
-    AddressSpace *dma_as = vdev->dma_as;
-
-    if (virtio_access_is_big_endian(vdev)) {
-        stw_be_phys(dma_as, pa, value);
-    } else {
-        stw_le_phys(dma_as, pa, value);
-    }
-}
-
 static inline void virtio_stl_phys(VirtIODevice *vdev, hwaddr pa,
                                    uint32_t value)
 {
@@ -146,15 +135,6 @@ static inline void virtio_stl_phys(VirtIODevice *vdev, hwaddr pa,
     }
 }
 
-static inline void virtio_stw_p(VirtIODevice *vdev, void *ptr, uint16_t v)
-{
-    if (virtio_access_is_big_endian(vdev)) {
-        stw_be_p(ptr, v);
-    } else {
-        stw_le_p(ptr, v);
-    }
-}
-
 static inline void virtio_stl_p(VirtIODevice *vdev, void *ptr, uint32_t v)
 {
     if (virtio_access_is_big_endian(vdev)) {
@@ -220,17 +200,6 @@ static inline uint64_t virtio_ldq_phys_cached(VirtIODevice *vdev,
     return ldq_le_phys_cached(cache, pa);
 }
 
-static inline void virtio_stw_phys_cached(VirtIODevice *vdev,
-                                          MemoryRegionCache *cache,
-                                          hwaddr pa, uint16_t value)
-{
-    if (virtio_access_is_big_endian(vdev)) {
-        stw_be_phys_cached(cache, pa, value);
-    } else {
-        stw_le_phys_cached(cache, pa, value);
-    }
-}
-
 static inline void virtio_stl_phys_cached(VirtIODevice *vdev,
                                           MemoryRegionCache *cache,
                                           hwaddr pa, uint32_t value)
-- 
2.26.3

[RFC PATCH 22/25] hw/virtio: Use LDST_CONVERT macro to emit 32-bit load/store functions

[Philippe Mathieu-Daudé]

Signed-off-by: Philippe Mathieu-Daudé <philmd@redhat.com>
---
 include/hw/virtio/virtio-access.h | 66 +++----------------------------
 1 file changed, 5 insertions(+), 61 deletions(-)

diff --git a/include/hw/virtio/virtio-access.h b/include/hw/virtio/virtio-access.h
index 4341af9cb0f..0df52d190dc 100644
--- a/include/hw/virtio/virtio-access.h
+++ b/include/hw/virtio/virtio-access.h
@@ -100,18 +100,13 @@ static inline void virtio_st ## size ## _phys_cached(VirtIODevice *vdev,\
     }\
 }
 
+#define VIRTIO_LDST_CONVERT(size, rtype, vtype)\
+    VIRTIO_LD_CONVERT(size, rtype)\
+    VIRTIO_ST_CONVERT(size, vtype)
+
 VIRTIO_LD_CONVERT(uw, uint16_t)
 VIRTIO_ST_CONVERT(w, uint16_t)
-
-static inline uint32_t virtio_ldl_phys(VirtIODevice *vdev, hwaddr pa)
-{
-    AddressSpace *dma_as = vdev->dma_as;
-
-    if (virtio_access_is_big_endian(vdev)) {
-        return ldl_be_phys(dma_as, pa);
-    }
-    return ldl_le_phys(dma_as, pa);
-}
+VIRTIO_LDST_CONVERT(l, int, uint32_t)
 
 static inline uint64_t virtio_ldq_phys(VirtIODevice *vdev, hwaddr pa)
 {
@@ -123,27 +118,6 @@ static inline uint64_t virtio_ldq_phys(VirtIODevice *vdev, hwaddr pa)
     return ldq_le_phys(dma_as, pa);
 }
 
-static inline void virtio_stl_phys(VirtIODevice *vdev, hwaddr pa,
-                                   uint32_t value)
-{
-    AddressSpace *dma_as = vdev->dma_as;
-
-    if (virtio_access_is_big_endian(vdev)) {
-        stl_be_phys(dma_as, pa, value);
-    } else {
-        stl_le_phys(dma_as, pa, value);
-    }
-}
-
-static inline void virtio_stl_p(VirtIODevice *vdev, void *ptr, uint32_t v)
-{
-    if (virtio_access_is_big_endian(vdev)) {
-        stl_be_p(ptr, v);
-    } else {
-        stl_le_p(ptr, v);
-    }
-}
-
 static inline void virtio_stq_p(VirtIODevice *vdev, void *ptr, uint64_t v)
 {
     if (virtio_access_is_big_endian(vdev)) {
@@ -153,15 +127,6 @@ static inline void virtio_stq_p(VirtIODevice *vdev, void *ptr, uint64_t v)
     }
 }
 
-static inline uint32_t virtio_ldl_p(VirtIODevice *vdev, const void *ptr)
-{
-    if (virtio_access_is_big_endian(vdev)) {
-        return ldl_be_p(ptr);
-    } else {
-        return ldl_le_p(ptr);
-    }
-}
-
 static inline uint64_t virtio_ldq_p(VirtIODevice *vdev, const void *ptr)
 {
     if (virtio_access_is_big_endian(vdev)) {
@@ -180,16 +145,6 @@ static inline uint16_t virtio_tswap16(VirtIODevice *vdev, uint16_t s)
 #endif
 }
 
-static inline uint32_t virtio_ldl_phys_cached(VirtIODevice *vdev,
-                                              MemoryRegionCache *cache,
-                                              hwaddr pa)
-{
-    if (virtio_access_is_big_endian(vdev)) {
-        return ldl_be_phys_cached(cache, pa);
-    }
-    return ldl_le_phys_cached(cache, pa);
-}
-
 static inline uint64_t virtio_ldq_phys_cached(VirtIODevice *vdev,
                                               MemoryRegionCache *cache,
                                               hwaddr pa)
@@ -200,17 +155,6 @@ static inline uint64_t virtio_ldq_phys_cached(VirtIODevice *vdev,
     return ldq_le_phys_cached(cache, pa);
 }
 
-static inline void virtio_stl_phys_cached(VirtIODevice *vdev,
-                                          MemoryRegionCache *cache,
-                                          hwaddr pa, uint32_t value)
-{
-    if (virtio_access_is_big_endian(vdev)) {
-        stl_be_phys_cached(cache, pa, value);
-    } else {
-        stl_le_phys_cached(cache, pa, value);
-    }
-}
-
 static inline void virtio_tswap16s(VirtIODevice *vdev, uint16_t *s)
 {
     *s = virtio_tswap16(vdev, *s);
-- 
2.26.3

[RFC PATCH 23/25] hw/virtio: Use LDST_CONVERT macro to emit 64-bit load/store functions

[Philippe Mathieu-Daudé]

Signed-off-by: Philippe Mathieu-Daudé <philmd@redhat.com>
---
 include/hw/virtio/virtio-access.h | 39 +------------------------------
 1 file changed, 1 insertion(+), 38 deletions(-)

diff --git a/include/hw/virtio/virtio-access.h b/include/hw/virtio/virtio-access.h
index 0df52d190dc..ae66bbd74f9 100644
--- a/include/hw/virtio/virtio-access.h
+++ b/include/hw/virtio/virtio-access.h
@@ -107,34 +107,7 @@ static inline void virtio_st ## size ## _phys_cached(VirtIODevice *vdev,\
 VIRTIO_LD_CONVERT(uw, uint16_t)
 VIRTIO_ST_CONVERT(w, uint16_t)
 VIRTIO_LDST_CONVERT(l, int, uint32_t)
-
-static inline uint64_t virtio_ldq_phys(VirtIODevice *vdev, hwaddr pa)
-{
-    AddressSpace *dma_as = vdev->dma_as;
-
-    if (virtio_access_is_big_endian(vdev)) {
-        return ldq_be_phys(dma_as, pa);
-    }
-    return ldq_le_phys(dma_as, pa);
-}
-
-static inline void virtio_stq_p(VirtIODevice *vdev, void *ptr, uint64_t v)
-{
-    if (virtio_access_is_big_endian(vdev)) {
-        stq_be_p(ptr, v);
-    } else {
-        stq_le_p(ptr, v);
-    }
-}
-
-static inline uint64_t virtio_ldq_p(VirtIODevice *vdev, const void *ptr)
-{
-    if (virtio_access_is_big_endian(vdev)) {
-        return ldq_be_p(ptr);
-    } else {
-        return ldq_le_p(ptr);
-    }
-}
+VIRTIO_LDST_CONVERT(q, uint64_t, uint64_t)
 
 static inline uint16_t virtio_tswap16(VirtIODevice *vdev, uint16_t s)
 {
@@ -145,16 +118,6 @@ static inline uint16_t virtio_tswap16(VirtIODevice *vdev, uint16_t s)
 #endif
 }
 
-static inline uint64_t virtio_ldq_phys_cached(VirtIODevice *vdev,
-                                              MemoryRegionCache *cache,
-                                              hwaddr pa)
-{
-    if (virtio_access_is_big_endian(vdev)) {
-        return ldq_be_phys_cached(cache, pa);
-    }
-    return ldq_le_phys_cached(cache, pa);
-}
-
 static inline void virtio_tswap16s(VirtIODevice *vdev, uint16_t *s)
 {
     *s = virtio_tswap16(vdev, *s);
-- 
2.26.3

[RFC PATCH 24/25] hw/virtio: Add methods for aligned pointer access

[Philippe Mathieu-Daudé]

Suggested-by: Stefan Hajnoczi <stefanha@redhat.com>
Signed-off-by: Philippe Mathieu-Daudé <philmd@redhat.com>
---
 include/hw/virtio/virtio-access.h | 21 +++++++++++++++++++++
 1 file changed, 21 insertions(+)

diff --git a/include/hw/virtio/virtio-access.h b/include/hw/virtio/virtio-access.h
index ae66bbd74f9..5b20f004e12 100644
--- a/include/hw/virtio/virtio-access.h
+++ b/include/hw/virtio/virtio-access.h
@@ -66,6 +66,16 @@ static inline rtype virtio_ld ## size ## _phys_cached(VirtIODevice *vdev,\
         return ld ## size ## _be_phys_cached(cache, pa);\
     }\
     return ld ## size ## _le_phys_cached(cache, pa);\
+}\
+static inline rtype virtio_ld ## size ## _phys_cached_aligned(\
+                                                      VirtIODevice *vdev,\
+                                                      MemoryRegionCache *cache,\
+                                                      hwaddr pa)\
+{\
+    if (virtio_access_is_big_endian(vdev)) {\
+        return ld ## size ## _be_phys_cached_aligned(cache, pa);\
+    }\
+    return ld ## size ## _le_phys_cached_aligned(cache, pa);\
 }
 
 #define VIRTIO_ST_CONVERT(size, vtype)\
@@ -98,6 +108,17 @@ static inline void virtio_st ## size ## _phys_cached(VirtIODevice *vdev,\
     } else {\
         st ## size ## _le_phys_cached(cache, pa, value);\
     }\
+}\
+static inline void virtio_st ## size ## _phys_cached_aligned(\
+                                                     VirtIODevice *vdev,\
+                                                     MemoryRegionCache *cache,\
+                                                     hwaddr pa, vtype value)\
+{\
+    if (virtio_access_is_big_endian(vdev)) {\
+        st ## size ## _be_phys_cached_aligned(cache, pa, value);\
+    } else {\
+        st ## size ## _le_phys_cached_aligned(cache, pa, value);\
+    }\
 }
 
 #define VIRTIO_LDST_CONVERT(size, rtype, vtype)\
-- 
2.26.3

[RFC PATCH 25/25] hw/virtio: Optimize accesses on vring aligned pointers

[Philippe Mathieu-Daudé]

By the virtio spec [*] the vring is aligned, so the 'idx' and 'flag'
fields are also 16-bit aligned. Therefore we can use the load/store
*aligned* API to set the value.

[*] https://docs.oasis-open.org/virtio/virtio/v1.1/csprd01/virtio-v1.1-csprd01.html#x1-430008

Reported-by: Bibo Mao <maobibo@loongson.cn>
Inspired-by: Stefan Hajnoczi <stefanha@redhat.com>
Signed-off-by: Philippe Mathieu-Daudé <philmd@redhat.com>
---
 hw/virtio/virtio.c | 13 +++++++------
 1 file changed, 7 insertions(+), 6 deletions(-)

diff --git a/hw/virtio/virtio.c b/hw/virtio/virtio.c
index e02544b2df7..ebee9c4e643 100644
--- a/hw/virtio/virtio.c
+++ b/hw/virtio/virtio.c
@@ -296,7 +296,7 @@ static inline uint16_t vring_avail_flags(VirtQueue *vq)
         return 0;
     }
 
-    return virtio_lduw_phys_cached(vq->vdev, &caches->avail, pa);
+    return virtio_lduw_phys_cached_aligned(vq->vdev, &caches->avail, pa);
 }
 
 /* Called within rcu_read_lock().  */
@@ -309,7 +309,8 @@ static inline uint16_t vring_avail_idx(VirtQueue *vq)
         return 0;
     }
 
-    vq->shadow_avail_idx = virtio_lduw_phys_cached(vq->vdev, &caches->avail, pa);
+    vq->shadow_avail_idx = virtio_lduw_phys_cached_aligned(vq->vdev,
+                                                           &caches->avail, pa);
     return vq->shadow_avail_idx;
 }
 
@@ -359,7 +360,7 @@ static uint16_t vring_used_idx(VirtQueue *vq)
         return 0;
     }
 
-    return virtio_lduw_phys_cached(vq->vdev, &caches->used, pa);
+    return virtio_lduw_phys_cached_aligned(vq->vdev, &caches->used, pa);
 }
 
 /* Called within rcu_read_lock().  */
@@ -369,7 +370,7 @@ static inline void vring_used_idx_set(VirtQueue *vq, uint16_t val)
     hwaddr pa = offsetof(VRingUsed, idx);
 
     if (caches) {
-        virtio_stw_phys_cached(vq->vdev, &caches->used, pa, val);
+        virtio_stw_phys_cached_aligned(vq->vdev, &caches->used, pa, val);
         address_space_cache_invalidate(&caches->used, pa, sizeof(val));
     }
 
@@ -388,8 +389,8 @@ static inline void vring_used_flags_set_bit(VirtQueue *vq, int mask)
         return;
     }
 
-    flags = virtio_lduw_phys_cached(vq->vdev, &caches->used, pa);
-    virtio_stw_phys_cached(vdev, &caches->used, pa, flags | mask);
+    flags = virtio_lduw_phys_cached_aligned(vq->vdev, &caches->used, pa);
+    virtio_stw_phys_cached_aligned(vdev, &caches->used, pa, flags | mask);
     address_space_cache_invalidate(&caches->used, pa, sizeof(flags));
 }
 
-- 
2.26.3

Re: [RFC PATCH 00/25] exec: Add load/store API for aligned pointers

[no-reply]

Patchew URL: https://patchew.org/QEMU/20210518183655.1711377-1-philmd@redhat.com/



Hi,

This series seems to have some coding style problems. See output below for
more information:

Type: series
Message-id: 20210518183655.1711377-1-philmd@redhat.com
Subject: [RFC PATCH 00/25] exec: Add load/store API for aligned pointers

=== TEST SCRIPT BEGIN ===
#!/bin/bash
git rev-parse base > /dev/null || exit 0
git config --local diff.renamelimit 0
git config --local diff.renames True
git config --local diff.algorithm histogram
./scripts/checkpatch.pl --mailback base..
=== TEST SCRIPT END ===

Updating 3c8cf5a9c21ff8782164d1def7f44bd888713384
From https://github.com/patchew-project/qemu
 * [new tag]         patchew/20210518183655.1711377-1-philmd@redhat.com -> patchew/20210518183655.1711377-1-philmd@redhat.com
Switched to a new branch 'test'
b51497a hw/virtio: Optimize accesses on vring aligned pointers
f7d21cb hw/virtio: Add methods for aligned pointer access
3ad61c3 hw/virtio: Use LDST_CONVERT macro to emit 64-bit load/store functions
c03a99d hw/virtio: Use LDST_CONVERT macro to emit 32-bit load/store functions
5f7cb6e hw/virtio: Use ST_CONVERT() macro to emit 16-bit load/store functions
d95d346 hw/virtio: Introduce VIRTIO_ST_CONVERT() macro
fed8880 hw/virtio: Use LD_CONVERT macro to emit 16-bit unsigned load/store code
cfffb55 hw/virtio: Introduce VIRTIO_LD_CONVERT() macro
0c89b29 hw/virtio: Use correct type sizes
66d8c15 exec/memory: Add methods for aligned pointer access (physical memory)
9a169b5 exec/memory: Add methods for aligned pointer access (address space)
493016c qemu/bswap: Introduce load/store for aligned pointer
5cfe6c7 qemu/bswap: Use LDST_CONVERT macro to emit 64-bit load/store functions
4256f3f qemu/bswap: Use LDST_CONVERT macro to emit 32-bit load/store functions
5e69992 qemu/bswap: Use LD_CONVERT macro to emit 16-bit unsigned load/store code
0fa8f4d qemu/bswap: Use LD_CONVERT macro to emit 16-bit signed load/store code
2a6c3d1 qemu/bswap: Introduce LD_CONVERT() macro
e7b6792 qemu/bswap: Use ST_CONVERT() macro to emit 16-bit load/store functions
0d0c3e4 qemu/bswap: Introduce ST_CONVERT() macro
1ca0493 exec/memory: Use correct type size
3010882 exec/memory_ldst_cached: Use correct type size
0beaa26 exec/memory_ldst_phys: Use correct type sizes
88d6831 exec/memory_ldst: Use correct type sizes
057db27 exec/memory_ldst_phys: Sort declarations
3a92956 exec/memory_ldst_cached: Sort declarations

=== OUTPUT BEGIN ===
1/25 Checking commit 3a9295699cb2 (exec/memory_ldst_cached: Sort declarations)
2/25 Checking commit 057db27b4129 (exec/memory_ldst_phys: Sort declarations)
3/25 Checking commit 88d6831d2688 (exec/memory_ldst: Use correct type sizes)
4/25 Checking commit 0beaa26979fa (exec/memory_ldst_phys: Use correct type sizes)
5/25 Checking commit 3010882b5604 (exec/memory_ldst_cached: Use correct type size)
6/25 Checking commit 1ca0493ecfd1 (exec/memory: Use correct type size)
7/25 Checking commit 0d0c3e4e5c2d (qemu/bswap: Introduce ST_CONVERT() macro)
8/25 Checking commit e7b679286247 (qemu/bswap: Use ST_CONVERT() macro to emit 16-bit load/store functions)
9/25 Checking commit 2a6c3d1c4aad (qemu/bswap: Introduce LD_CONVERT() macro)
10/25 Checking commit 0fa8f4d7d229 (qemu/bswap: Use LD_CONVERT macro to emit 16-bit signed load/store code)
11/25 Checking commit 5e69992e047b (qemu/bswap: Use LD_CONVERT macro to emit 16-bit unsigned load/store code)
12/25 Checking commit 4256f3f7f36a (qemu/bswap: Use LDST_CONVERT macro to emit 32-bit load/store functions)
13/25 Checking commit 5cfe6c7a8fd6 (qemu/bswap: Use LDST_CONVERT macro to emit 64-bit load/store functions)
14/25 Checking commit 493016c0c84e (qemu/bswap: Introduce load/store for aligned pointer)
ERROR: space required after that close brace '}'
#119: FILE: include/qemu/bswap.h:369:
+}\

ERROR: space required after that close brace '}'
#129: FILE: include/qemu/bswap.h:379:
+}\

total: 2 errors, 0 warnings, 107 lines checked

Patch 14/25 has style problems, please review.  If any of these errors
are false positives report them to the maintainer, see
CHECKPATCH in MAINTAINERS.

15/25 Checking commit 9a169b58ddf3 (exec/memory: Add methods for aligned pointer access (address space))
16/25 Checking commit 66d8c1562136 (exec/memory: Add methods for aligned pointer access (physical memory))
17/25 Checking commit 0c89b296d850 (hw/virtio: Use correct type sizes)
18/25 Checking commit cfffb555f1ca (hw/virtio: Introduce VIRTIO_LD_CONVERT() macro)
ERROR: space required after that close brace '}'
#31: FILE: include/hw/virtio/virtio-access.h:49:
+    }\

ERROR: space required after that close brace '}'
#33: FILE: include/hw/virtio/virtio-access.h:51:
+}\

ERROR: space required after that close brace '}'
#41: FILE: include/hw/virtio/virtio-access.h:59:
+    }\

ERROR: space required after that close brace '}'
#42: FILE: include/hw/virtio/virtio-access.h:60:
+}\

ERROR: space required after that close brace '}'
#49: FILE: include/hw/virtio/virtio-access.h:67:
+    }\

total: 5 errors, 0 warnings, 35 lines checked

Patch 18/25 has style problems, please review.  If any of these errors
are false positives report them to the maintainer, see
CHECKPATCH in MAINTAINERS.

19/25 Checking commit fed888096371 (hw/virtio: Use LD_CONVERT macro to emit 16-bit unsigned load/store code)
20/25 Checking commit d95d346351f6 (hw/virtio: Introduce VIRTIO_ST_CONVERT() macro)
ERROR: space required after that close brace '}'
#32: FILE: include/hw/virtio/virtio-access.h:79:
+    }\

ERROR: space required after that close brace '}'
#33: FILE: include/hw/virtio/virtio-access.h:80:
+}\

ERROR: space required after that close brace '}'
#43: FILE: include/hw/virtio/virtio-access.h:90:
+    }\

ERROR: space required after that close brace '}'
#44: FILE: include/hw/virtio/virtio-access.h:91:
+}\

ERROR: space required after that close brace '}'
#53: FILE: include/hw/virtio/virtio-access.h:100:
+    }\

total: 5 errors, 0 warnings, 38 lines checked

Patch 20/25 has style problems, please review.  If any of these errors
are false positives report them to the maintainer, see
CHECKPATCH in MAINTAINERS.

21/25 Checking commit 5f7cb6e5bf21 (hw/virtio: Use ST_CONVERT() macro to emit 16-bit load/store functions)
22/25 Checking commit c03a99d5100a (hw/virtio: Use LDST_CONVERT macro to emit 32-bit load/store functions)
23/25 Checking commit 3ad61c32db83 (hw/virtio: Use LDST_CONVERT macro to emit 64-bit load/store functions)
24/25 Checking commit f7d21cb701d1 (hw/virtio: Add methods for aligned pointer access)
ERROR: space required after that close brace '}'
#22: FILE: include/hw/virtio/virtio-access.h:69:
+}\

ERROR: space required after that close brace '}'
#30: FILE: include/hw/virtio/virtio-access.h:77:
+    }\

ERROR: space required after that close brace '}'
#39: FILE: include/hw/virtio/virtio-access.h:111:
+}\

ERROR: space required after that close brace '}'
#49: FILE: include/hw/virtio/virtio-access.h:121:
+    }\

total: 4 errors, 0 warnings, 33 lines checked

Patch 24/25 has style problems, please review.  If any of these errors
are false positives report them to the maintainer, see
CHECKPATCH in MAINTAINERS.

25/25 Checking commit b51497a1e1f3 (hw/virtio: Optimize accesses on vring aligned pointers)
=== OUTPUT END ===

Test command exited with code: 1


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

Re: [RFC PATCH 00/25] exec: Add load/store API for aligned pointers

[Richard Henderson]

On 5/18/21 1:36 PM, Philippe Mathieu-Daudé wrote:
> The series is decomposed as:
> - cleanups (1-6)
> - clean ldst API using macros (7-13)
> - add aligned ldst methods (14)
> - add aligned memory methods (15-16)
> - similar changes in virtio (17-24)
> - use the new methods on vring aligned values (25)
> 
> There are some checkpatch errors related to the macro used.

I think we should emphasize the atomicness of the access as opposed to the 
alignedness.  That's the only thing that's important to virtio.

Thus s/aligned/atomic/g and use qatomic_read/qatomic_set.


r~

Re: [RFC PATCH 00/25] exec: Add load/store API for aligned pointers

[Philippe Mathieu-Daudé]

On 5/19/21 9:20 PM, Richard Henderson wrote:
> On 5/18/21 1:36 PM, Philippe Mathieu-Daudé wrote:
>> The series is decomposed as:
>> - cleanups (1-6)
>> - clean ldst API using macros (7-13)
>> - add aligned ldst methods (14)
>> - add aligned memory methods (15-16)
>> - similar changes in virtio (17-24)
>> - use the new methods on vring aligned values (25)
>>
>> There are some checkpatch errors related to the macro used.
> 
> I think we should emphasize the atomicness of the access as opposed to
> the alignedness.  That's the only thing that's important to virtio.
> 
> Thus s/aligned/atomic/g and use qatomic_read/qatomic_set.

OK.

Do you think patches 1-6, 17 (Use correct type sizes) could
go in meanwhile, or should I repost them separately?

Re: [RFC PATCH 00/25] exec: Add load/store API for aligned pointers

[Richard Henderson]

On 5/19/21 2:40 PM, Philippe Mathieu-Daudé wrote:
> On 5/19/21 9:20 PM, Richard Henderson wrote:
>> On 5/18/21 1:36 PM, Philippe Mathieu-Daudé wrote:
>>> The series is decomposed as:
>>> - cleanups (1-6)
>>> - clean ldst API using macros (7-13)
>>> - add aligned ldst methods (14)
>>> - add aligned memory methods (15-16)
>>> - similar changes in virtio (17-24)
>>> - use the new methods on vring aligned values (25)
>>>
>>> There are some checkpatch errors related to the macro used.
>>
>> I think we should emphasize the atomicness of the access as opposed to
>> the alignedness.  That's the only thing that's important to virtio.
>>
>> Thus s/aligned/atomic/g and use qatomic_read/qatomic_set.
> 
> OK.
> 
> Do you think patches 1-6, 17 (Use correct type sizes) could
> go in meanwhile, or should I repost them separately?
> 

I think 1-6 can be queued; I'm had a question about 17.


r~