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.Cameron@huawei.com>
---
system/physmem.c | 78 ++++++++++++++++++++++++++++++++++++++++++++----
1 file changed, 72 insertions(+), 6 deletions(-)
diff --git a/system/physmem.c b/system/physmem.c
index 74f92bb3b8..43b37942cf 100644
--- a/system/physmem.c
+++ b/system/physmem.c
@@ -3377,6 +3377,72 @@ static inline MemoryRegion *address_space_translate_cached(
return section.mr;
}
+/* Called within RCU critical section. */
+static MemTxResult address_space_write_continue_cached(MemoryRegionCache *cache,
+ hwaddr addr,
+ MemTxAttrs attrs,
+ const void *ptr,
+ hwaddr len, hwaddr addr1,
+ hwaddr l,
+ MemoryRegion *mr)
+{
+ MemTxResult result = MEMTX_OK;
+ const uint8_t *buf = ptr;
+
+ for (;;) {
+
+ result |= flatview_write_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, true,
+ attrs);
+ }
+
+ return result;
+}
+
+/* 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)
+{
+ MemTxResult result = MEMTX_OK;
+ uint8_t *buf = ptr;
+
+ fuzz_dma_read_cb(addr, len, mr);
+ for (;;) {
+
+ 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);
+ }
+
+ 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.2On 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.Cameron@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
On Fri, 1 Mar 2024 13:44:01 +0800
Peter Xu <peterx@redhat.com> wrote:
> 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.Cameron@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?
Agreed, but also need to drop the fuzz_dma_read_cb().
However given the first thing that is checked by the only in tree fuzzing
code is whether we are dealing with RAM, I think that's fine.
>
> > +{
> > + 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?
>
I had the same thought originally but couldn't convince myself that there
was no route to end up with a different MR here. I don't yet
have a good enough grip on how this all fits together so I particularly
appreciate your help.
With hindsight I should have called this out as a question in this patch set.
Can drop passing in cache as well given it is no longer used within
this function.
Thanks,
Jonathan
> (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
> >
>
© 2016 - 2024 Red Hat, Inc.