Bug: physmem: address_space_read_cached_slow() accesses wrong MemoryRegion on latter part of large reads.

[Jonathan Cameron]

Hi All,

The continuing saga of a getting CXL emulation to play well with using the
memory as normal RAM ran into (hopefully) a last issue.

When running my boot image via virtio-blk-pci and having deliberately forced 
some
buffers to end up in the CXL memory via
$ numactl --membind=1 ls
then on shut down I was getting a failure to allocate a large enough DMA buffer
even with Mattias' set to increase the size and an extra patch to apply that to
the main system memory (as virtio ignores iommu and PCI address space).
gdb/bt pointed at virtqueue_split_pop() bt pointed later than the actual problem
as descriptors were loaded but corrupt here:
https://elixir.bootlin.com/qemu/latest/source/hw/virtio/virtio.c#L1573
https://elixir.bootlin.com/qemu/latest/source/hw/virtio/virtio.c#L306

After a chase (via a highly suspicious read size of 0xffffffff it turns
out it was issuing a 16 byte read via address_space_read_cached_slow() and
getting a very odd answer.

The first 8 bytes was the correct virtio descriptor content the second 8
were coming from the flash region at physical address 0 onwards on arm-virt.

With those breadcrumbs the problem was (I think) easy to find.

https://elixir.bootlin.com/qemu/latest/source/include/exec/memory.h#L3029
address_space_read_cached_slow() operates on a MemoryRegionCache with the
hwaddr being relative to the start of that cached region.

However it calls flatview_read_continue()  That's fine as long as we can
do the read in one go.


For normal memory flatview_read_continue() will deal with splitting a read up
but on each iteration it loads the mr via
mr = flatview_translate(fv, addr, &addr1, &l, false, attrs);
to cope with reads that cross MemoryRegions.
https://elixir.bootlin.com/qemu/latest/source/system/physmem.c#L2737

Unfortunately the addr passed in here is the one addressing into the
MemoryRegionCache offset by whatever we already read.
Which for this bug example that was 0x8 (in the flash memory).

Assuming I have correctly identified the problem.
One potential fix is to define a new

MemtxResult address_space_read_cached_continue(MemoryRegionCache *cache,
                                               hwaddr addr, MemTxAttr attrs,
                                               void *ptr, hwaddr len,
                                               hwaddr addr1, hwaddr l,
                                               MemoryRegion *mr)

That is nearly identical to flatview_read_continue() but with the 
mr = flatview_translate() replaced with
mr = address_space_translate_cached(cache, addr, &addr1, &l, false, attrs)

That's a bit ugly though given the duplicated code but any other change
is going to involve some more invasive splitting out of utility functions
to share all but the outer loop.

I don't currently have a test hitting it but assume
flatview_write_continue() in address_space_write_cached_slow() has the
same problem.

Jonathan

p.s. Will tidy this and the rest of my house of cards up then post it.
I suspect we'll carry on hitting QEMU limitations with the CXL emulation
but for now I have x86 and ARM setups that work with TCG.
Hmm. Need to spend some time getting regressions tests in place :(