On Fri, 1 Mar 2024 13:44:01 +0800
Peter Xu <pet...@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.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?
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
> >
>
