Phil, Did you accidentally miss this in your pull request [1] or did you leave it out intentionally?
It's still missing the Reviewed bit after the atomic cmpxchg change prompted by the size_t switch, so that might be the reason - I'm just trying to figure out what the next step is for this patch. Thanks, Mattias [1] https://lists.nongnu.org/archive/html/qemu-devel/2024-05/msg01712.html On Wed, May 8, 2024 at 8:33 AM Mattias Nissler <mniss...@rivosinc.com> wrote: > > On Tue, May 7, 2024 at 4:46 PM Philippe Mathieu-Daudé <phi...@linaro.org> > wrote: > > > > On 7/5/24 16:04, Mattias Nissler wrote: > > > On Tue, May 7, 2024 at 2:57 PM Philippe Mathieu-Daudé <phi...@linaro.org> > > > wrote: > > >> > > >> On 7/5/24 11:42, Mattias Nissler wrote: > > >>> When DMA memory can't be directly accessed, as is the case when > > >>> running the device model in a separate process without shareable DMA > > >>> file descriptors, bounce buffering is used. > > >>> > > >>> It is not uncommon for device models to request mapping of several DMA > > >>> regions at the same time. Examples include: > > >>> * net devices, e.g. when transmitting a packet that is split across > > >>> several TX descriptors (observed with igb) > > >>> * USB host controllers, when handling a packet with multiple data > > >>> TRBs > > >>> (observed with xhci) > > >>> > > >>> Previously, qemu only provided a single bounce buffer per AddressSpace > > >>> and would fail DMA map requests while the buffer was already in use. In > > >>> turn, this would cause DMA failures that ultimately manifest as hardware > > >>> errors from the guest perspective. > > >>> > > >>> This change allocates DMA bounce buffers dynamically instead of > > >>> supporting only a single buffer. Thus, multiple DMA mappings work > > >>> correctly also when RAM can't be mmap()-ed. > > >>> > > >>> The total bounce buffer allocation size is limited individually for each > > >>> AddressSpace. The default limit is 4096 bytes, matching the previous > > >>> maximum buffer size. A new x-max-bounce-buffer-size parameter is > > >>> provided to configure the limit for PCI devices. > > >>> > > >>> Signed-off-by: Mattias Nissler <mniss...@rivosinc.com> > > >>> --- > > >>> hw/pci/pci.c | 8 ++++ > > >>> include/exec/memory.h | 14 +++---- > > >>> include/hw/pci/pci_device.h | 3 ++ > > >>> system/memory.c | 5 ++- > > >>> system/physmem.c | 82 > > >>> ++++++++++++++++++++++++++----------- > > >>> 5 files changed, 76 insertions(+), 36 deletions(-) > > > > > > >>> /** > > >>> * struct AddressSpace: describes a mapping of addresses to > > >>> #MemoryRegion objects > > >>> @@ -1143,8 +1137,10 @@ struct AddressSpace { > > >>> QTAILQ_HEAD(, MemoryListener) listeners; > > >>> QTAILQ_ENTRY(AddressSpace) address_spaces_link; > > >>> > > >>> - /* Bounce buffer to use for this address space. */ > > >>> - BounceBuffer bounce; > > >>> + /* Maximum DMA bounce buffer size used for indirect memory map > > >>> requests */ > > >>> + uint32_t max_bounce_buffer_size; > > >> > > >> Alternatively size_t. > > > > > > While switching things over, I was surprised to find that > > > DEFINE_PROP_SIZE wants a uint64_t field rather than a size_t field. > > > There is a DEFINE_PROP_SIZE32 variant for uint32_t though. Considering > > > my options, assuming that we want to use size_t for everything other > > > than the property: > > > > > > (1) Make PCIDevice::max_bounce_buffer_size size_t and have the > > > preprocessor select DEFINE_PROP_SIZE/DEFINE_PROP_SIZE32. This makes > > > the qdev property type depend on the host. Ugh. > > > > > > (2) Make PCIDevice::max_bounce_buffer_size uint64_t and clamp if > > > needed when used. Weird to allow larger values that are then clamped, > > > although it probably doesn't matter in practice since address space is > > > limited to 4GB anyways. > > > > > > (3) Make PCIDevice::max_bounce_buffer_size uint32_t and accept the > > > limitation that the largest bounce buffer limit is 4GB even on 64-bit > > > hosts. > > > > > > #3 seemed most pragmatic, so I'll go with that. > > > > LGTM, thanks for updating. > > No problem, can I ask you to provide a formal R-B on the v10 #4 patch > [1] then, so the series will be ready to go in? > > [1] https://lists.nongnu.org/archive/html/qemu-devel/2024-05/msg01382.html > > > > > > > > > > > >> > > >>> + /* Total size of bounce buffers currently allocated, atomically > > >>> accessed */ > > >>> + uint32_t bounce_buffer_size; > > >> > > >> Ditto. > >
