On Thu, Sep 12, 2024 at 03:27:55PM +0100, Peter Maydell wrote:
> On Mon, 19 Aug 2024 at 14:56, Mattias Nissler <mniss...@rivosinc.com> 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>
> > Reviewed-by: Philippe Mathieu-Daudé <phi...@linaro.org>
> > Acked-by: Peter Xu <pet...@redhat.com>
> > ---
>
> > @@ -3251,28 +3265,40 @@ void *address_space_map(AddressSpace *as,
> > mr = flatview_translate(fv, addr, &xlat, &l, is_write, attrs);
> >
> > if (!memory_access_is_direct(mr, is_write)) {
> > - if (qatomic_xchg(&as->bounce.in_use, true)) {
> > + size_t used = qatomic_read(&as->bounce_buffer_size);
> > + for (;;) {
> > + hwaddr alloc = MIN(as->max_bounce_buffer_size - used, l);
> > + size_t new_size = used + alloc;
> > + size_t actual =
> > + qatomic_cmpxchg(&as->bounce_buffer_size, used, new_size);
> > + if (actual == used) {
> > + l = alloc;
> > + break;
> > + }
> > + used = actual;
> > + }
> > +
> > + if (l == 0) {
> > *plen = 0;
> > return NULL;
> > }
> > - /* Avoid unbounded allocations */
> > - l = MIN(l, TARGET_PAGE_SIZE);
> > - as->bounce.buffer = qemu_memalign(TARGET_PAGE_SIZE, l);
> > - as->bounce.addr = addr;
> > - as->bounce.len = l;
> >
> > + BounceBuffer *bounce = g_malloc0(l + sizeof(BounceBuffer));
> > + bounce->magic = BOUNCE_BUFFER_MAGIC;
> > memory_region_ref(mr);
> > - as->bounce.mr = mr;
> > + bounce->mr = mr;
> > + bounce->addr = addr;
> > + bounce->len = l;
> > +
> > if (!is_write) {
> > flatview_read(fv, addr, MEMTXATTRS_UNSPECIFIED,
> > - as->bounce.buffer, l);
> > + bounce->buffer, l);
> > }
> >
> > *plen = l;
> > - return as->bounce.buffer;
> > + return bounce->buffer;
>
> Coverity is pretty unhappy about this trick, because it isn't able
> to recognise that we can figure out the address of 'bounce'
> from the address of 'bounce->buffer' and free it in the
> address_space_unmap() code, so it thinks that every use
> of address_space_map(), pci_dma_map(), etc, is a memory leak.
> We can mark all those as false positives, of course, but it got
> me wondering whether maybe we should have this function return
> a struct that has all the information address_space_unmap()
> needs rather than relying on it being able to figure it out
> from the host memory pointer...
Indeed that sounds like a viable option. Looks like we don't have a lot of
address_space_map() users.
There might be some challenge on users who cache the results (rather than
immediately unmap() after using the buffer in the same function). Still
doable but the changeset can spread all over, and also testing is harder if
just to get some coverage.
Not sure whether Mattias have some more clue when he's working on that.
Thanks,
--
Peter Xu
