[0/3] physmem: Fix MemoryRegion for second access to cached MMIO Address Space

Message

Jonathan Cameron Feb. 15, 2024, 2:28 p.m. UTC

Issue seen testing virtio-blk-pci with CXL emulated interleave memory.
Tests were done on arm64, but the issue isn't architecture specific.
Note that some additional fixes are needed to TCG to be able to run far
enough to hit this on arm64 or x86. They are issues so I'll post separate
series shortly.

The address_space_read_cached_slow() and address_space_write_cached_slow()
functions query the MemoryRegion for the cached address space correctly
using address_space_translate_cached() but then call into
flatview_read_continue() / flatview_write_continue()
If the access is to a MMIO MemoryRegion and is bigger than the MemoryRegion
supports, the loop will query the MemoryRegion for the next access to use.
That query uses flatview_translate() but the address passed is suitable
for the cache, not the flatview. On my test setup that mean the second
8 bytes and onwards of the virtio descriptor was read from flash memory
at the beginning of the system address map, not the CXL emulated memory
where the descriptor was found.  Result happened to be all fs so easy to
spot.

Changes these calls to use address_space_translate_cached() to get the
correct MemoryRegion for the cache. To avoid duplicating most of the
code, the first 2 patches factor out the common parts of
flatview_read_continue() and flatview_write_continue() so they can
be reused.

Write path has not been tested but it so similar to the read path I've
included it here.

Jonathan Cameron (3):
  physmem: Reduce local variable scope in flatview_read/write_continue()
  physmem: Factor out body of flatview_read/write_continue() loop
  physmem: Fix wrong MR in large address_space_read/write_cached_slow()

 system/physmem.c | 245 ++++++++++++++++++++++++++++++++---------------
 1 file changed, 170 insertions(+), 75 deletions(-)

Comments

Jonathan Cameron Feb. 29, 2024, 10:49 a.m. UTC | #1

On Thu, 15 Feb 2024 14:28:14 +0000
Jonathan Cameron via <[e-mail]> wrote:

Any comments?  Almost all the other fixes I need for CXL memory to
work as normal ram are queued up so I'd love it if we can solve this one as
well.

This looks like a big series, but it's really just a refactor + trivial
addition - so shouldn't be too scary!

Jonathan

> Issue seen testing virtio-blk-pci with CXL emulated interleave memory.
> Tests were done on arm64, but the issue isn't architecture specific.
> Note that some additional fixes are needed to TCG to be able to run far
> enough to hit this on arm64 or x86. They are issues so I'll post separate
> series shortly.
> 
> The address_space_read_cached_slow() and address_space_write_cached_slow()
> functions query the MemoryRegion for the cached address space correctly
> using address_space_translate_cached() but then call into
> flatview_read_continue() / flatview_write_continue()
> If the access is to a MMIO MemoryRegion and is bigger than the MemoryRegion
> supports, the loop will query the MemoryRegion for the next access to use.
> That query uses flatview_translate() but the address passed is suitable
> for the cache, not the flatview. On my test setup that mean the second
> 8 bytes and onwards of the virtio descriptor was read from flash memory
> at the beginning of the system address map, not the CXL emulated memory
> where the descriptor was found.  Result happened to be all fs so easy to
> spot.
> 
> Changes these calls to use address_space_translate_cached() to get the
> correct MemoryRegion for the cache. To avoid duplicating most of the
> code, the first 2 patches factor out the common parts of
> flatview_read_continue() and flatview_write_continue() so they can
> be reused.
> 
> Write path has not been tested but it so similar to the read path I've
> included it here.
> 
> Jonathan Cameron (3):
>   physmem: Reduce local variable scope in flatview_read/write_continue()
>   physmem: Factor out body of flatview_read/write_continue() loop
>   physmem: Fix wrong MR in large address_space_read/write_cached_slow()
> 
>  system/physmem.c | 245 ++++++++++++++++++++++++++++++++---------------
>  1 file changed, 170 insertions(+), 75 deletions(-)
>