On Thu, Feb 15, 2024 at 02:28:17PM +0000, Jonathan Cameron wrote:
Can we rename the subject?
physmem: Fix wrong MR in large address_space_read/write_cached_slow()
IMHO "wrong MR" is misleading, as the MR was wrong only because the address
passed over is wrong at the first place. Perhaps s/MR/addr/?
> 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.
>
> Signed-off-by: Jonathan Cameron <jonathan.came...@huawei.com>
> ---
> system/physmem.c | 78 ++++++++++++++++++++++++++++++++++++++++++++----
> 1 file changed, 72 insertions(+), 6 deletions(-)
>
[...]
> +/* Called within RCU critical section. */
> +static MemTxResult address_space_read_continue_cached(MemoryRegionCache
> *cache,
> + hwaddr addr,
> + MemTxAttrs attrs,
> + void *ptr, hwaddr len,
> + hwaddr addr1, hwaddr l,
> + MemoryRegion *mr)
It looks like "addr" (of flatview AS) is not needed for a cached RW (see
below), because we should have a stable (cached) MR to operate anyway?
How about we also use "mr_addr" as the single addr of the MR, then drop
addr1?
> +{
> + MemTxResult result = MEMTX_OK;
> + uint8_t *buf = ptr;
> +
> + fuzz_dma_read_cb(addr, len, mr);
> + for (;;) {
> +
Remove empty line?
> + result |= flatview_read_continue_step(addr, attrs, buf, len, addr1,
> + &l, mr);
> + len -= l;
> + buf += l;
> + addr += l;
> +
> + if (!len) {
> + break;
> + }
> + l = len;
> +
> + mr = address_space_translate_cached(cache, addr, &addr1, &l, false,
> + attrs);
Here IIUC the mr will always be the same as before? If so, maybe "mr_addr
+= l" should be enough?
(similar comment applies to the writer side too)
> + }
> +
> + 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.
> */
> @@ -3390,9 +3456,9 @@ address_space_read_cached_slow(MemoryRegionCache
> *cache, hwaddr addr,
> l = len;
> mr = address_space_translate_cached(cache, addr, &addr1, &l, false,
> MEMTXATTRS_UNSPECIFIED);
> - return flatview_read_continue(cache->fv,
> - addr, MEMTXATTRS_UNSPECIFIED, buf, len,
> - addr1, l, mr);
> + return address_space_read_continue_cached(cache, addr,
> + MEMTXATTRS_UNSPECIFIED, buf,
> len,
> + addr1, l, mr);
> }
>
> /* Called from RCU critical section. address_space_write_cached uses this
> @@ -3408,9 +3474,9 @@ address_space_write_cached_slow(MemoryRegionCache
> *cache, hwaddr addr,
> l = len;
> mr = address_space_translate_cached(cache, addr, &addr1, &l, true,
> MEMTXATTRS_UNSPECIFIED);
> - return flatview_write_continue(cache->fv,
> - addr, MEMTXATTRS_UNSPECIFIED, buf, len,
> - addr1, l, mr);
> + return address_space_write_continue_cached(cache, addr,
> + MEMTXATTRS_UNSPECIFIED,
> + buf, len, addr1, l, mr);
> }
>
> #define ARG1_DECL MemoryRegionCache *cache
> --
> 2.39.2
>
--
Peter Xu
