Re: [PATCH 3/3] physmem: Fix wrong MR in large address_space_read/write_cached_slow()

[Jonathan Cameron]

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
> >   
>