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.
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?
Best regards
Heinrich
