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

[Peter Xu]

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?

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