On Wed, Feb 28, 2024 at 08:07:47PM +0100, Heinrich Schuchardt wrote: > On 28.02.24 19:39, Peter Maydell wrote: > > 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.
Pure (and can also be stupid) question: why virtqueue_map_desc() would map to !direct mem? Shouldn't those buffers normally allocated from guest RAM? > > > > > > > > > > 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.) > > If we allow a bounce buffer only to be used once (via the in_use flag), why > do we allow only a single bounce buffer? > > Could address_space_map() allocate a new bounce buffer on every call and > address_space_unmap() deallocate it? > > Isn't the design with a single bounce buffer bound to fail with a > multi-threaded client as collision can be expected? See: https://lore.kernel.org/r/20240212080617.2559498-1-mniss...@rivosinc.com For some reason that series didn't land, but it seems to be helpful in this case too if e.g. there can be multiple of such devices. Thanks, -- Peter Xu