[PATCH v2 0/4] physmem: Fix MemoryRegion for second access to cached MMIO Address Space

[PATCH v2 0/4] physmem: Fix MemoryRegion for second access to cached MMIO Address Space

[Jonathan Cameron via]

v2: (Thanks to Peter Xu for reviewing!)
- New patch 1 to rename addr1 to mr_addr in the interests of meaningful naming.
- Take advantage of a cached address space only allow for a single MR to simplify
  the new code.
- Various cleanups of indentation etc.
- Cover letter and some patch descriptions updated to reflect changes.
- Changes all called out in specific patches.

Issue seen testing virtio-blk-pci with CXL emulated interleave memory.
Tests were done on arm64, but the issue isn't architecture specific.
Note that some additional fixes are needed to TCG to be able to run far
enough to hit this on arm64 or x86. Most of these are now upstream
with exception of:

target/i386: Enable page walking from MMIO memory
https://lore.kernel.org/qemu-devel/20240219173153.12114-3-Jonathan.Cameron@huawei.com/

The address_space_read_cached_slow() and address_space_write_cached_slow()
functions query the MemoryRegion for the cached address space correctly
using address_space_translate_cached() but then call into
flatview_read_continue() / flatview_write_continue().
If the access is to a MMIO MemoryRegion and is bigger than the MemoryRegion
supports, the loop will query the MemoryRegion for the next access to use.
That query uses flatview_translate() but the address passed is suitable
for the cache, not the flatview. On my test setup that mean the second
8 bytes and onwards of the virtio descriptor was read from flash memory
at the beginning of the system address map, not the CXL emulated memory
where the descriptor was found.  Result happened to be all fs so easy to
spot.

Changes these calls to assume that the MemoryRegion does not change
as multiple acceses are perfomed to the MemoryRegionCache.
The first patch renames the addr1 parameter to the hopefully more
informative mr_addr.

To avoid duplicating most of the code, the next 2 patches factor out
the common parts of flatview_read_continue() and flatview_write_continue()
so they can be reused.

Write path has not been tested but it so similar to the read path I've
included it here.

Jonathan Cameron (4):
  physmem: Rename addr1 to more informative mr_addr in
    flatview_read/write() and similar
  physmem: Reduce local variable scope in flatview_read/write_continue()
  physmem: Factor out body of flatview_read/write_continue() loop
  physmem: Fix wrong address in large
    address_space_read/write_cached_slow()

 system/physmem.c | 260 +++++++++++++++++++++++++++++++----------------
 1 file changed, 170 insertions(+), 90 deletions(-)

-- 
2.39.2

[PATCH v2 1/4] physmem: Rename addr1 to more informative mr_addr in flatview_read/write() and similar

[Jonathan Cameron via]

The calls to flatview_read/write[_continue]() have parameters addr and
addr1 but the names give no indication of what they are addresses of.
Rename addr1 to mr_addr to reflect that it is the translated address
offset within the MemoryRegion returned by flatview_translate().
Similarly rename the parameter in address_space_read/write_cached_slow()

Suggested-by: Peter Xu <peterx@redhat.com>
Signed-off-by: Jonathan Cameron <Jonathan.Cameron@huawei.com>

---
v2: New patch.
- I have kept the renames to only the code I'm touching later in this
  series, but they could be applied much more widely.
---
 system/physmem.c | 50 ++++++++++++++++++++++++------------------------
 1 file changed, 25 insertions(+), 25 deletions(-)

diff --git a/system/physmem.c b/system/physmem.c
index 05997a7ca7..2704b780f6 100644
--- a/system/physmem.c
+++ b/system/physmem.c
@@ -2685,7 +2685,7 @@ static bool flatview_access_allowed(MemoryRegion *mr, MemTxAttrs attrs,
 static MemTxResult flatview_write_continue(FlatView *fv, hwaddr addr,
                                            MemTxAttrs attrs,
                                            const void *ptr,
-                                           hwaddr len, hwaddr addr1,
+                                           hwaddr len, hwaddr mr_addr,
                                            hwaddr l, MemoryRegion *mr)
 {
     uint8_t *ram_ptr;
@@ -2695,12 +2695,12 @@ static MemTxResult flatview_write_continue(FlatView *fv, hwaddr addr,
     const uint8_t *buf = ptr;
 
     for (;;) {
-        if (!flatview_access_allowed(mr, attrs, addr1, l)) {
+        if (!flatview_access_allowed(mr, attrs, mr_addr, l)) {
             result |= MEMTX_ACCESS_ERROR;
             /* Keep going. */
         } else if (!memory_access_is_direct(mr, true)) {
             release_lock |= prepare_mmio_access(mr);
-            l = memory_access_size(mr, l, addr1);
+            l = memory_access_size(mr, l, mr_addr);
             /* XXX: could force current_cpu to NULL to avoid
                potential bugs */
 
@@ -2715,13 +2715,13 @@ static MemTxResult flatview_write_continue(FlatView *fv, hwaddr addr,
                    (l == 8 && len >= 8));
 #endif
             val = ldn_he_p(buf, l);
-            result |= memory_region_dispatch_write(mr, addr1, val,
+            result |= memory_region_dispatch_write(mr, mr_addr, val,
                                                    size_memop(l), attrs);
         } else {
             /* RAM case */
-            ram_ptr = qemu_ram_ptr_length(mr->ram_block, addr1, &l, false);
+            ram_ptr = qemu_ram_ptr_length(mr->ram_block, mr_addr, &l, false);
             memmove(ram_ptr, buf, l);
-            invalidate_and_set_dirty(mr, addr1, l);
+            invalidate_and_set_dirty(mr, mr_addr, l);
         }
 
         if (release_lock) {
@@ -2738,7 +2738,7 @@ static MemTxResult flatview_write_continue(FlatView *fv, hwaddr addr,
         }
 
         l = len;
-        mr = flatview_translate(fv, addr, &addr1, &l, true, attrs);
+        mr = flatview_translate(fv, addr, &mr_addr, &l, true, attrs);
     }
 
     return result;
@@ -2749,22 +2749,22 @@ static MemTxResult flatview_write(FlatView *fv, hwaddr addr, MemTxAttrs attrs,
                                   const void *buf, hwaddr len)
 {
     hwaddr l;
-    hwaddr addr1;
+    hwaddr mr_addr;
     MemoryRegion *mr;
 
     l = len;
-    mr = flatview_translate(fv, addr, &addr1, &l, true, attrs);
+    mr = flatview_translate(fv, addr, &mr_addr, &l, true, attrs);
     if (!flatview_access_allowed(mr, attrs, addr, len)) {
         return MEMTX_ACCESS_ERROR;
     }
     return flatview_write_continue(fv, addr, attrs, buf, len,
-                                   addr1, l, mr);
+                                   mr_addr, l, mr);
 }
 
 /* Called within RCU critical section.  */
 MemTxResult flatview_read_continue(FlatView *fv, hwaddr addr,
                                    MemTxAttrs attrs, void *ptr,
-                                   hwaddr len, hwaddr addr1, hwaddr l,
+                                   hwaddr len, hwaddr mr_addr, hwaddr l,
                                    MemoryRegion *mr)
 {
     uint8_t *ram_ptr;
@@ -2775,14 +2775,14 @@ MemTxResult flatview_read_continue(FlatView *fv, hwaddr addr,
 
     fuzz_dma_read_cb(addr, len, mr);
     for (;;) {
-        if (!flatview_access_allowed(mr, attrs, addr1, l)) {
+        if (!flatview_access_allowed(mr, attrs, mr_addr, l)) {
             result |= MEMTX_ACCESS_ERROR;
             /* Keep going. */
         } else if (!memory_access_is_direct(mr, false)) {
             /* I/O case */
             release_lock |= prepare_mmio_access(mr);
-            l = memory_access_size(mr, l, addr1);
-            result |= memory_region_dispatch_read(mr, addr1, &val,
+            l = memory_access_size(mr, l, mr_addr);
+            result |= memory_region_dispatch_read(mr, mr_addr, &val,
                                                   size_memop(l), attrs);
 
             /*
@@ -2798,7 +2798,7 @@ MemTxResult flatview_read_continue(FlatView *fv, hwaddr addr,
             stn_he_p(buf, l, val);
         } else {
             /* RAM case */
-            ram_ptr = qemu_ram_ptr_length(mr->ram_block, addr1, &l, false);
+            ram_ptr = qemu_ram_ptr_length(mr->ram_block, mr_addr, &l, false);
             memcpy(buf, ram_ptr, l);
         }
 
@@ -2816,7 +2816,7 @@ MemTxResult flatview_read_continue(FlatView *fv, hwaddr addr,
         }
 
         l = len;
-        mr = flatview_translate(fv, addr, &addr1, &l, false, attrs);
+        mr = flatview_translate(fv, addr, &mr_addr, &l, false, attrs);
     }
 
     return result;
@@ -2827,16 +2827,16 @@ static MemTxResult flatview_read(FlatView *fv, hwaddr addr,
                                  MemTxAttrs attrs, void *buf, hwaddr len)
 {
     hwaddr l;
-    hwaddr addr1;
+    hwaddr mr_addr;
     MemoryRegion *mr;
 
     l = len;
-    mr = flatview_translate(fv, addr, &addr1, &l, false, attrs);
+    mr = flatview_translate(fv, addr, &mr_addr, &l, false, attrs);
     if (!flatview_access_allowed(mr, attrs, addr, len)) {
         return MEMTX_ACCESS_ERROR;
     }
     return flatview_read_continue(fv, addr, attrs, buf, len,
-                                  addr1, l, mr);
+                                  mr_addr, l, mr);
 }
 
 MemTxResult address_space_read_full(AddressSpace *as, hwaddr addr,
@@ -3359,15 +3359,15 @@ MemTxResult
 address_space_read_cached_slow(MemoryRegionCache *cache, hwaddr addr,
                                    void *buf, hwaddr len)
 {
-    hwaddr addr1, l;
+    hwaddr mr_addr, l;
     MemoryRegion *mr;
 
     l = len;
-    mr = address_space_translate_cached(cache, addr, &addr1, &l, false,
+    mr = address_space_translate_cached(cache, addr, &mr_addr, &l, false,
                                         MEMTXATTRS_UNSPECIFIED);
     return flatview_read_continue(cache->fv,
                                   addr, MEMTXATTRS_UNSPECIFIED, buf, len,
-                                  addr1, l, mr);
+                                  mr_addr, l, mr);
 }
 
 /* Called from RCU critical section. address_space_write_cached uses this
@@ -3377,15 +3377,15 @@ MemTxResult
 address_space_write_cached_slow(MemoryRegionCache *cache, hwaddr addr,
                                     const void *buf, hwaddr len)
 {
-    hwaddr addr1, l;
+    hwaddr mr_addr, l;
     MemoryRegion *mr;
 
     l = len;
-    mr = address_space_translate_cached(cache, addr, &addr1, &l, true,
+    mr = address_space_translate_cached(cache, addr, &mr_addr, &l, true,
                                         MEMTXATTRS_UNSPECIFIED);
     return flatview_write_continue(cache->fv,
                                    addr, MEMTXATTRS_UNSPECIFIED, buf, len,
-                                   addr1, l, mr);
+                                   mr_addr, l, mr);
 }
 
 #define ARG1_DECL                MemoryRegionCache *cache
-- 
2.39.2

Re: [PATCH v2 1/4] physmem: Rename addr1 to more informative mr_addr in flatview_read/write() and similar

[David Hildenbrand]

On 07.03.24 16:37, Jonathan Cameron wrote:
> The calls to flatview_read/write[_continue]() have parameters addr and
> addr1 but the names give no indication of what they are addresses of.
> Rename addr1 to mr_addr to reflect that it is the translated address
> offset within the MemoryRegion returned by flatview_translate().
> Similarly rename the parameter in address_space_read/write_cached_slow()
> 
> Suggested-by: Peter Xu <peterx@redhat.com>
> Signed-off-by: Jonathan Cameron <Jonathan.Cameron@huawei.com>
> 

Much cleaner indeed!

Reviewed-by: David Hildenbrand <david@redhat.com>

-- 
Cheers,

David / dhildenb

[PATCH v2 2/4] physmem: Reduce local variable scope in flatview_read/write_continue()

[Jonathan Cameron via]

Precursor to factoring out the inner loops for reuse.

Reviewed-by: Peter Xu <peterx@redhat.com>
Signed-off-by: Jonathan Cameron <Jonathan.Cameron@huawei.com>
---
v2: Picked up tag from Peter.
 system/physmem.c | 40 ++++++++++++++++++++--------------------
 1 file changed, 20 insertions(+), 20 deletions(-)

diff --git a/system/physmem.c b/system/physmem.c
index 2704b780f6..a64a96a3e5 100644
--- a/system/physmem.c
+++ b/system/physmem.c
@@ -2688,10 +2688,7 @@ static MemTxResult flatview_write_continue(FlatView *fv, hwaddr addr,
                                            hwaddr len, hwaddr mr_addr,
                                            hwaddr l, MemoryRegion *mr)
 {
-    uint8_t *ram_ptr;
-    uint64_t val;
     MemTxResult result = MEMTX_OK;
-    bool release_lock = false;
     const uint8_t *buf = ptr;
 
     for (;;) {
@@ -2699,7 +2696,9 @@ static MemTxResult flatview_write_continue(FlatView *fv, hwaddr addr,
             result |= MEMTX_ACCESS_ERROR;
             /* Keep going. */
         } else if (!memory_access_is_direct(mr, true)) {
-            release_lock |= prepare_mmio_access(mr);
+            uint64_t val;
+            bool release_lock = prepare_mmio_access(mr);
+
             l = memory_access_size(mr, l, mr_addr);
             /* XXX: could force current_cpu to NULL to avoid
                potential bugs */
@@ -2717,18 +2716,21 @@ static MemTxResult flatview_write_continue(FlatView *fv, hwaddr addr,
             val = ldn_he_p(buf, l);
             result |= memory_region_dispatch_write(mr, mr_addr, val,
                                                    size_memop(l), attrs);
+            if (release_lock) {
+                bql_unlock();
+            }
+
+
         } else {
             /* RAM case */
-            ram_ptr = qemu_ram_ptr_length(mr->ram_block, mr_addr, &l, false);
+
+            uint8_t *ram_ptr = qemu_ram_ptr_length(mr->ram_block, mr_addr, &l,
+                                                   false);
+
             memmove(ram_ptr, buf, l);
             invalidate_and_set_dirty(mr, mr_addr, l);
         }
 
-        if (release_lock) {
-            bql_unlock();
-            release_lock = false;
-        }
-
         len -= l;
         buf += l;
         addr += l;
@@ -2767,10 +2769,7 @@ MemTxResult flatview_read_continue(FlatView *fv, hwaddr addr,
                                    hwaddr len, hwaddr mr_addr, hwaddr l,
                                    MemoryRegion *mr)
 {
-    uint8_t *ram_ptr;
-    uint64_t val;
     MemTxResult result = MEMTX_OK;
-    bool release_lock = false;
     uint8_t *buf = ptr;
 
     fuzz_dma_read_cb(addr, len, mr);
@@ -2780,7 +2779,9 @@ MemTxResult flatview_read_continue(FlatView *fv, hwaddr addr,
             /* Keep going. */
         } else if (!memory_access_is_direct(mr, false)) {
             /* I/O case */
-            release_lock |= prepare_mmio_access(mr);
+            uint64_t val;
+            bool release_lock = prepare_mmio_access(mr);
+
             l = memory_access_size(mr, l, mr_addr);
             result |= memory_region_dispatch_read(mr, mr_addr, &val,
                                                   size_memop(l), attrs);
@@ -2796,17 +2797,16 @@ MemTxResult flatview_read_continue(FlatView *fv, hwaddr addr,
                    (l == 8 && len >= 8));
 #endif
             stn_he_p(buf, l, val);
+            if (release_lock) {
+                bql_unlock();
+            }
         } else {
             /* RAM case */
-            ram_ptr = qemu_ram_ptr_length(mr->ram_block, mr_addr, &l, false);
+            uint8_t *ram_ptr = qemu_ram_ptr_length(mr->ram_block, mr_addr, &l,
+                                                   false);
             memcpy(buf, ram_ptr, l);
         }
 
-        if (release_lock) {
-            bql_unlock();
-            release_lock = false;
-        }
-
         len -= l;
         buf += l;
         addr += l;
-- 
2.39.2

Re: [PATCH v2 2/4] physmem: Reduce local variable scope in flatview_read/write_continue()

[David Hildenbrand]

On 07.03.24 16:37, Jonathan Cameron wrote:
> Precursor to factoring out the inner loops for reuse.
> 
> Reviewed-by: Peter Xu <peterx@redhat.com>
> Signed-off-by: Jonathan Cameron <Jonathan.Cameron@huawei.com>
> ---

Reviewed-by: David Hildenbrand <david@redhat.com>

-- 
Cheers,

David / dhildenb

Re: [PATCH v2 2/4] physmem: Reduce local variable scope in flatview_read/write_continue()

[Philippe Mathieu-Daudé]

On 7/3/24 16:37, Jonathan Cameron wrote:
> Precursor to factoring out the inner loops for reuse.
> 
> Reviewed-by: Peter Xu <peterx@redhat.com>
> Signed-off-by: Jonathan Cameron <Jonathan.Cameron@huawei.com>
> ---
> v2: Picked up tag from Peter.
>   system/physmem.c | 40 ++++++++++++++++++++--------------------
>   1 file changed, 20 insertions(+), 20 deletions(-)

Nice finding. Similar pattern in system/memory_ldst.c.inc.

Reviewed-by: Philippe Mathieu-Daudé <philmd@linaro.org>

[PATCH v2 3/4] physmem: Factor out body of flatview_read/write_continue() loop

[Jonathan Cameron via]

This code will be reused for the address_space_cached accessors
shortly.

Also reduce scope of result variable now we aren't directly
calling this in the loop.

Signed-off-by: Jonathan Cameron <Jonathan.Cameron@huawei.com>
---
v2: Thanks to Peter Xu
- Fix alignment of code.
- Drop unused addr parameter.
- Carry through new mr_addr parameter name.
- RB not picked up as not sure what Peter will think wrt to
  resulting parameter ordering.
---
 system/physmem.c | 169 +++++++++++++++++++++++++++--------------------
 1 file changed, 99 insertions(+), 70 deletions(-)

diff --git a/system/physmem.c b/system/physmem.c
index a64a96a3e5..1264eab24b 100644
--- a/system/physmem.c
+++ b/system/physmem.c
@@ -2681,6 +2681,56 @@ static bool flatview_access_allowed(MemoryRegion *mr, MemTxAttrs attrs,
     return false;
 }
 
+static MemTxResult flatview_write_continue_step(MemTxAttrs attrs,
+                                                const uint8_t *buf,
+                                                hwaddr len, hwaddr mr_addr,
+                                                hwaddr *l, MemoryRegion *mr)
+{
+    if (!flatview_access_allowed(mr, attrs, mr_addr, *l)) {
+        return MEMTX_ACCESS_ERROR;
+    }
+
+    if (!memory_access_is_direct(mr, true)) {
+        uint64_t val;
+        MemTxResult result;
+        bool release_lock = prepare_mmio_access(mr);
+
+        *l = memory_access_size(mr, *l, mr_addr);
+        /*
+         * XXX: could force current_cpu to NULL to avoid
+         * potential bugs
+         */
+
+        /*
+         * Assure Coverity (and ourselves) that we are not going to OVERRUN
+         * the buffer by following ldn_he_p().
+         */
+#ifdef QEMU_STATIC_ANALYSIS
+        assert((*l == 1 && len >= 1) ||
+               (*l == 2 && len >= 2) ||
+               (*l == 4 && len >= 4) ||
+               (*l == 8 && len >= 8));
+#endif
+        val = ldn_he_p(buf, *l);
+        result = memory_region_dispatch_write(mr, mr_addr, val,
+                                              size_memop(*l), attrs);
+        if (release_lock) {
+            bql_unlock();
+        }
+
+        return result;
+    } else {
+        /* RAM case */
+        uint8_t *ram_ptr = qemu_ram_ptr_length(mr->ram_block, mr_addr, l,
+                                               false);
+
+        memmove(ram_ptr, buf, *l);
+        invalidate_and_set_dirty(mr, mr_addr, *l);
+
+        return MEMTX_OK;
+    }
+}
+
 /* Called within RCU critical section.  */
 static MemTxResult flatview_write_continue(FlatView *fv, hwaddr addr,
                                            MemTxAttrs attrs,
@@ -2692,44 +2742,8 @@ static MemTxResult flatview_write_continue(FlatView *fv, hwaddr addr,
     const uint8_t *buf = ptr;
 
     for (;;) {
-        if (!flatview_access_allowed(mr, attrs, mr_addr, l)) {
-            result |= MEMTX_ACCESS_ERROR;
-            /* Keep going. */
-        } else if (!memory_access_is_direct(mr, true)) {
-            uint64_t val;
-            bool release_lock = prepare_mmio_access(mr);
-
-            l = memory_access_size(mr, l, mr_addr);
-            /* XXX: could force current_cpu to NULL to avoid
-               potential bugs */
-
-            /*
-             * Assure Coverity (and ourselves) that we are not going to OVERRUN
-             * the buffer by following ldn_he_p().
-             */
-#ifdef QEMU_STATIC_ANALYSIS
-            assert((l == 1 && len >= 1) ||
-                   (l == 2 && len >= 2) ||
-                   (l == 4 && len >= 4) ||
-                   (l == 8 && len >= 8));
-#endif
-            val = ldn_he_p(buf, l);
-            result |= memory_region_dispatch_write(mr, mr_addr, val,
-                                                   size_memop(l), attrs);
-            if (release_lock) {
-                bql_unlock();
-            }
-
-
-        } else {
-            /* RAM case */
-
-            uint8_t *ram_ptr = qemu_ram_ptr_length(mr->ram_block, mr_addr, &l,
-                                                   false);
-
-            memmove(ram_ptr, buf, l);
-            invalidate_and_set_dirty(mr, mr_addr, l);
-        }
+        result |= flatview_write_continue_step(attrs, buf, len, mr_addr, &l,
+                                               mr);
 
         len -= l;
         buf += l;
@@ -2763,6 +2777,52 @@ static MemTxResult flatview_write(FlatView *fv, hwaddr addr, MemTxAttrs attrs,
                                    mr_addr, l, mr);
 }
 
+static MemTxResult flatview_read_continue_step(MemTxAttrs attrs, uint8_t *buf,
+                                               hwaddr len, hwaddr mr_addr,
+                                               hwaddr *l,
+                                               MemoryRegion *mr)
+{
+    if (!flatview_access_allowed(mr, attrs, mr_addr, *l)) {
+        return MEMTX_ACCESS_ERROR;
+    }
+
+    if (!memory_access_is_direct(mr, false)) {
+        /* I/O case */
+        uint64_t val;
+        MemTxResult result;
+        bool release_lock = prepare_mmio_access(mr);
+
+        *l = memory_access_size(mr, *l, mr_addr);
+        result = memory_region_dispatch_read(mr, mr_addr, &val, size_memop(*l),
+                                             attrs);
+
+        /*
+         * Assure Coverity (and ourselves) that we are not going to OVERRUN
+         * the buffer by following stn_he_p().
+         */
+#ifdef QEMU_STATIC_ANALYSIS
+        assert((*l == 1 && len >= 1) ||
+               (*l == 2 && len >= 2) ||
+               (*l == 4 && len >= 4) ||
+               (*l == 8 && len >= 8));
+#endif
+        stn_he_p(buf, *l, val);
+
+        if (release_lock) {
+            bql_unlock();
+        }
+        return result;
+    } else {
+        /* RAM case */
+        uint8_t *ram_ptr = qemu_ram_ptr_length(mr->ram_block, mr_addr, l,
+                                               false);
+
+        memcpy(buf, ram_ptr, *l);
+
+        return MEMTX_OK;
+    }
+}
+
 /* Called within RCU critical section.  */
 MemTxResult flatview_read_continue(FlatView *fv, hwaddr addr,
                                    MemTxAttrs attrs, void *ptr,
@@ -2774,38 +2834,7 @@ MemTxResult flatview_read_continue(FlatView *fv, hwaddr addr,
 
     fuzz_dma_read_cb(addr, len, mr);
     for (;;) {
-        if (!flatview_access_allowed(mr, attrs, mr_addr, l)) {
-            result |= MEMTX_ACCESS_ERROR;
-            /* Keep going. */
-        } else if (!memory_access_is_direct(mr, false)) {
-            /* I/O case */
-            uint64_t val;
-            bool release_lock = prepare_mmio_access(mr);
-
-            l = memory_access_size(mr, l, mr_addr);
-            result |= memory_region_dispatch_read(mr, mr_addr, &val,
-                                                  size_memop(l), attrs);
-
-            /*
-             * Assure Coverity (and ourselves) that we are not going to OVERRUN
-             * the buffer by following stn_he_p().
-             */
-#ifdef QEMU_STATIC_ANALYSIS
-            assert((l == 1 && len >= 1) ||
-                   (l == 2 && len >= 2) ||
-                   (l == 4 && len >= 4) ||
-                   (l == 8 && len >= 8));
-#endif
-            stn_he_p(buf, l, val);
-            if (release_lock) {
-                bql_unlock();
-            }
-        } else {
-            /* RAM case */
-            uint8_t *ram_ptr = qemu_ram_ptr_length(mr->ram_block, mr_addr, &l,
-                                                   false);
-            memcpy(buf, ram_ptr, l);
-        }
+        result |= flatview_read_continue_step(attrs, buf, len, mr_addr, &l, mr);
 
         len -= l;
         buf += l;
-- 
2.39.2

Re: [PATCH v2 3/4] physmem: Factor out body of flatview_read/write_continue() loop

[David Hildenbrand]

On 07.03.24 16:37, Jonathan Cameron wrote:
> This code will be reused for the address_space_cached accessors
> shortly.
> 
> Also reduce scope of result variable now we aren't directly
> calling this in the loop.
> 
> Signed-off-by: Jonathan Cameron <Jonathan.Cameron@huawei.com>
> ---
> v2: Thanks to Peter Xu
> - Fix alignment of code.
> - Drop unused addr parameter.
> - Carry through new mr_addr parameter name.
> - RB not picked up as not sure what Peter will think wrt to
>    resulting parameter ordering.
> ---

Reviewed-by: David Hildenbrand <david@redhat.com>

-- 
Cheers,

David / dhildenb

[PATCH v2 4/4] physmem: Fix wrong address in large address_space_read/write_cached_slow()

[Jonathan Cameron via]

If the access is bigger than the MemoryRegion supports,
flatview_read/write_continue() will attempt to update the Memory Region.
but the address passed to flatview_translate() is relative to the cache, not
to the FlatView.

On arm/virt with interleaved CXL memory emulation and virtio-blk-pci this
lead to the first part of descriptor being read from the CXL memory and the
second part from PA 0x8 which happens to be a blank region
of a flash chip and all ffs on this particular configuration.
Note this test requires the out of tree ARM support for CXL, but
the problem is more general.

Avoid this by adding new address_space_read_continue_cached()
and address_space_write_continue_cached() which share all the logic
with the flatview versions except for the MemoryRegion lookup which
is unnecessary as the MemoryRegionCache only covers one MemoryRegion.

Signed-off-by: Jonathan Cameron <Jonathan.Cameron@huawei.com>
---
v2: Review from Peter Xu
- Drop additional lookups of the MemoryRegion via
address_space_translate_cached() as it will always return the same
answer.
- Drop various parameters that are then unused.
- rename addr1 to mr_addr.
- Drop a fuzz_dma_read_cb(). Could put this back but it means
  carrying the address into the inner call and the only in tree
  fuzzer checks if it is normal RAM and if not does nothing anyway.
  We don't hit this path for normal RAM.
---
 system/physmem.c | 63 +++++++++++++++++++++++++++++++++++++++++++-----
 1 file changed, 57 insertions(+), 6 deletions(-)

diff --git a/system/physmem.c b/system/physmem.c
index 1264eab24b..701bea27dd 100644
--- a/system/physmem.c
+++ b/system/physmem.c
@@ -3381,6 +3381,59 @@ static inline MemoryRegion *address_space_translate_cached(
     return section.mr;
 }
 
+/* Called within RCU critical section.  */
+static MemTxResult address_space_write_continue_cached(MemTxAttrs attrs,
+                                                       const void *ptr,
+                                                       hwaddr len,
+                                                       hwaddr mr_addr,
+                                                       hwaddr l,
+                                                       MemoryRegion *mr)
+{
+    MemTxResult result = MEMTX_OK;
+    const uint8_t *buf = ptr;
+
+    for (;;) {
+        result |= flatview_write_continue_step(attrs, buf, len, mr_addr, &l,
+                                               mr);
+
+        len -= l;
+        buf += l;
+        mr_addr += l;
+
+        if (!len) {
+            break;
+        }
+
+        l = len;
+    }
+
+    return result;
+}
+
+/* Called within RCU critical section.  */
+static MemTxResult address_space_read_continue_cached(MemTxAttrs attrs,
+                                                      void *ptr, hwaddr len,
+                                                      hwaddr mr_addr, hwaddr l,
+                                                      MemoryRegion *mr)
+{
+    MemTxResult result = MEMTX_OK;
+    uint8_t *buf = ptr;
+
+    for (;;) {
+        result |= flatview_read_continue_step(attrs, buf, len, mr_addr, &l, mr);
+        len -= l;
+        buf += l;
+        mr_addr += l;
+
+        if (!len) {
+            break;
+        }
+        l = len;
+    }
+
+    return result;
+}
+
 /* Called from RCU critical section. address_space_read_cached uses this
  * out of line function when the target is an MMIO or IOMMU region.
  */
@@ -3394,9 +3447,8 @@ address_space_read_cached_slow(MemoryRegionCache *cache, hwaddr addr,
     l = len;
     mr = address_space_translate_cached(cache, addr, &mr_addr, &l, false,
                                         MEMTXATTRS_UNSPECIFIED);
-    return flatview_read_continue(cache->fv,
-                                  addr, MEMTXATTRS_UNSPECIFIED, buf, len,
-                                  mr_addr, l, mr);
+    return address_space_read_continue_cached(MEMTXATTRS_UNSPECIFIED,
+                                              buf, len, mr_addr, l, mr);
 }
 
 /* Called from RCU critical section. address_space_write_cached uses this
@@ -3412,9 +3464,8 @@ address_space_write_cached_slow(MemoryRegionCache *cache, hwaddr addr,
     l = len;
     mr = address_space_translate_cached(cache, addr, &mr_addr, &l, true,
                                         MEMTXATTRS_UNSPECIFIED);
-    return flatview_write_continue(cache->fv,
-                                   addr, MEMTXATTRS_UNSPECIFIED, buf, len,
-                                   mr_addr, l, mr);
+    return address_space_write_continue_cached(MEMTXATTRS_UNSPECIFIED,
+                                               buf, len, mr_addr, l, mr);
 }
 
 #define ARG1_DECL                MemoryRegionCache *cache
-- 
2.39.2

Re: [PATCH v2 4/4] physmem: Fix wrong address in large address_space_read/write_cached_slow()

[David Hildenbrand]

On 07.03.24 16:37, Jonathan Cameron wrote:
> If the access is bigger than the MemoryRegion supports,
> flatview_read/write_continue() will attempt to update the Memory Region.
> but the address passed to flatview_translate() is relative to the cache, not
> to the FlatView.
> 
> On arm/virt with interleaved CXL memory emulation and virtio-blk-pci this
> lead to the first part of descriptor being read from the CXL memory and the
> second part from PA 0x8 which happens to be a blank region
> of a flash chip and all ffs on this particular configuration.
> Note this test requires the out of tree ARM support for CXL, but
> the problem is more general.
> 
> Avoid this by adding new address_space_read_continue_cached()
> and address_space_write_continue_cached() which share all the logic
> with the flatview versions except for the MemoryRegion lookup which
> is unnecessary as the MemoryRegionCache only covers one MemoryRegion.
> 
> Signed-off-by: Jonathan Cameron <Jonathan.Cameron@huawei.com>
> ---
> v2: Review from Peter Xu
> - Drop additional lookups of the MemoryRegion via
> address_space_translate_cached() as it will always return the same
> answer.
> - Drop various parameters that are then unused.
> - rename addr1 to mr_addr.
> - Drop a fuzz_dma_read_cb(). Could put this back but it means
>    carrying the address into the inner call and the only in tree
>    fuzzer checks if it is normal RAM and if not does nothing anyway.
>    We don't hit this path for normal RAM.
> ---
>   system/physmem.c | 63 +++++++++++++++++++++++++++++++++++++++++++-----
>   1 file changed, 57 insertions(+), 6 deletions(-)
> 
> diff --git a/system/physmem.c b/system/physmem.c
> index 1264eab24b..701bea27dd 100644
> --- a/system/physmem.c
> +++ b/system/physmem.c
> @@ -3381,6 +3381,59 @@ static inline MemoryRegion *address_space_translate_cached(
>       return section.mr;
>   }
>   
> +/* Called within RCU critical section.  */
> +static MemTxResult address_space_write_continue_cached(MemTxAttrs attrs,
> +                                                       const void *ptr,
> +                                                       hwaddr len,
> +                                                       hwaddr mr_addr,
> +                                                       hwaddr l,
> +                                                       MemoryRegion *mr)

The only thing that is really confusing is

hwaddr len,
...
hwaddr l,

ehm, what?

... but it fits the style of flatview_read_continue(), so at least the 
level of confusion this creates is consistent with the other code.


Reviewed-by: David Hildenbrand <david@redhat.com>

-- 
Cheers,

David / dhildenb

Re: [PATCH v2 0/4] physmem: Fix MemoryRegion for second access to cached MMIO Address Space

[Peter Xu]

On Thu, Mar 07, 2024 at 03:37:06PM +0000, Jonathan Cameron wrote:
> v2: (Thanks to Peter Xu for reviewing!)
> - New patch 1 to rename addr1 to mr_addr in the interests of meaningful naming.
> - Take advantage of a cached address space only allow for a single MR to simplify
>   the new code.
> - Various cleanups of indentation etc.
> - Cover letter and some patch descriptions updated to reflect changes.
> - Changes all called out in specific patches.

All look good to me, thanks.  Having the new functions' first argument as
MemTxAttrs is slightly weird to me, but that's not a big deal and we can
clean it up later if wanted.  I guess it's good to fix this in 9.0 first as
it's a real bug even if not trivial to hit.

I queued it in my migration tree (with my "memory API" hat..).

I won't send a pull until next Monday.  Please shoot if there's any objections!

-- 
Peter Xu

Re: [PATCH v2 0/4] physmem: Fix MemoryRegion for second access to cached MMIO Address Space

[David Hildenbrand]

On 08.03.24 08:36, Peter Xu wrote:
> On Thu, Mar 07, 2024 at 03:37:06PM +0000, Jonathan Cameron wrote:
>> v2: (Thanks to Peter Xu for reviewing!)
>> - New patch 1 to rename addr1 to mr_addr in the interests of meaningful naming.
>> - Take advantage of a cached address space only allow for a single MR to simplify
>>    the new code.
>> - Various cleanups of indentation etc.
>> - Cover letter and some patch descriptions updated to reflect changes.
>> - Changes all called out in specific patches.
> 
> All look good to me, thanks.  Having the new functions' first argument as
> MemTxAttrs is slightly weird to me, but that's not a big deal and we can
> clean it up later if wanted.  I guess it's good to fix this in 9.0 first as
> it's a real bug even if not trivial to hit.
> 
> I queued it in my migration tree (with my "memory API" hat..).
> 
> I won't send a pull until next Monday.  Please shoot if there's any objections!
> 

Works for me, thanks!

-- 
Cheers,

David / dhildenb

Re: [PATCH v2 0/4] physmem: Fix MemoryRegion for second access to cached MMIO Address Space

[Philippe Mathieu-Daudé]

On 8/3/24 08:36, Peter Xu wrote:

> I queued it in my migration tree (with my "memory API" hat..).
> 
> I won't send a pull until next Monday.  Please shoot if there's any objections!

Thanks!