On Wed, 28 Feb 2024 at 18:28, Heinrich Schuchardt <heinrich.schucha...@canonical.com> wrote: > > On 28.02.24 16:06, Philippe Mathieu-Daudé wrote: > > Hi Heinrich, > > > > On 28/2/24 13:59, Heinrich Schuchardt wrote: > >> virtqueue_map_desc() is called with values of sz exceeding that may > >> exceed > >> TARGET_PAGE_SIZE. sz = 0x2800 has been observed. > >> > >> We only support a single bounce buffer. We have to avoid > >> virtqueue_map_desc() calling address_space_map() multiple times. > >> Otherwise > >> we see an error > >> > >> qemu: virtio: bogus descriptor or out of resources > >> > >> Increase the minimum size of the bounce buffer to 0x10000 which matches > >> the largest value of TARGET_PAGE_SIZE for all architectures. > >> > >> Signed-off-by: Heinrich Schuchardt <heinrich.schucha...@canonical.com> > >> --- > >> v2: > >> remove unrelated change > >> --- > >> system/physmem.c | 8 ++++++-- > >> 1 file changed, 6 insertions(+), 2 deletions(-) > >> > >> diff --git a/system/physmem.c b/system/physmem.c > >> index e3ebc19eef..3c82da1c86 100644 > >> --- a/system/physmem.c > >> +++ b/system/physmem.c > >> @@ -3151,8 +3151,12 @@ void *address_space_map(AddressSpace *as, > >> *plen = 0; > >> return NULL; > >> } > >> - /* Avoid unbounded allocations */ > >> - l = MIN(l, TARGET_PAGE_SIZE); > >> + /* > >> + * There is only one bounce buffer. The largest occuring > >> value of > >> + * parameter sz of virtqueue_map_desc() must fit into the bounce > >> + * buffer. > >> + */ > >> + l = MIN(l, 0x10000); > > > > Please define this magic value. Maybe ANY_TARGET_PAGE_SIZE or > > TARGETS_BIGGEST_PAGE_SIZE? > > > > Then along: > > QEMU_BUILD_BUG_ON(TARGET_PAGE_SIZE <= TARGETS_BIGGEST_PAGE_SIZE); > > Thank you Philippe for reviewing. > > TARGETS_BIGGEST_PAGE_SIZE does not fit as the value is not driven by the > page size. > How about MIN_BOUNCE_BUFFER_SIZE? > Is include/exec/memory.h the right include for the constant? > > I don't think that TARGET_PAGE_SIZE has any relevance for setting the > bounce buffer size. I only mentioned it to say that we are not > decreasing the value on any existing architecture. > > I don't know why TARGET_PAGE_SIZE ever got into this piece of code. > e3127ae0cdcd ("exec: reorganize address_space_map") does not provide a > reason for this choice. Maybe Paolo remembers.
The limitation to a page dates back to commit 6d16c2f88f2a in 2009, which was the first implementation of this function. I don't think there's a particular reason for that value beyond that it was probably a convenient value that was assumed to be likely "big enough". I think the idea with this bounce-buffer has always been that this isn't really a code path we expected to end up in very often -- it's supposed to be for when devices are doing DMA, which they will typically be doing to memory (backed by host RAM), not devices (backed by MMIO and needing a bounce buffer). So the whole mechanism is a bit "last fallback to stop things breaking entirely". The address_space_map() API says that it's allowed to return a subset of the range you ask for, so if the virtio code doesn't cope with the minimum being set to TARGET_PAGE_SIZE then either we need to fix that virtio code or we need to change the API of this function. (But I think you will also get a reduced range if you try to use it across a boundary between normal host-memory-backed RAM and a device MemoryRegion.) thanks -- PMM