Re: [PATCH v2 4/4] physmem: Fix wrong address in large address_space_read/write_cached_slow()

[David Hildenbrand <david@redhat.com>]

On 07.03.24 16:37, Jonathan Cameron wrote:
> 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 which
> is unnecessary as the MemoryRegionCache only covers one MemoryRegion.
> 
> Signed-off-by: Jonathan Cameron <Jonathan.Cameron@huawei.com>
> ---
> v2: Review from Peter Xu
> - Drop additional lookups of the MemoryRegion via
> address_space_translate_cached() as it will always return the same
> answer.
> - Drop various parameters that are then unused.
> - rename addr1 to mr_addr.
> - Drop a fuzz_dma_read_cb(). Could put this back but it means
>    carrying the address into the inner call and the only in tree
>    fuzzer checks if it is normal RAM and if not does nothing anyway.
>    We don't hit this path for normal RAM.
> ---
>   system/physmem.c | 63 +++++++++++++++++++++++++++++++++++++++++++-----
>   1 file changed, 57 insertions(+), 6 deletions(-)
> 
> diff --git a/system/physmem.c b/system/physmem.c
> index 1264eab24b..701bea27dd 100644
> --- a/system/physmem.c
> +++ b/system/physmem.c
> @@ -3381,6 +3381,59 @@ static inline MemoryRegion *address_space_translate_cached(
>       return section.mr;
>   }
>   
> +/* Called within RCU critical section.  */
> +static MemTxResult address_space_write_continue_cached(MemTxAttrs attrs,
> +                                                       const void *ptr,
> +                                                       hwaddr len,
> +                                                       hwaddr mr_addr,
> +                                                       hwaddr l,
> +                                                       MemoryRegion *mr)

The only thing that is really confusing is

hwaddr len,
...
hwaddr l,

ehm, what?

... but it fits the style of flatview_read_continue(), so at least the 
level of confusion this creates is consistent with the other code.


Reviewed-by: David Hildenbrand <david@redhat.com>

-- 
Cheers,

David / dhildenb