Re: [PATCH] softmmu: Support concurrent bounce buffers

[Mattias Nissler]

On Tue, Sep 10, 2024 at 5:44 PM Peter Maydell <peter.maydell@linaro.org> wrote:
>
> On Tue, 10 Sept 2024 at 15:53, Michael S. Tsirkin <mst@redhat.com> wrote:
> >
> > On Mon, Aug 19, 2024 at 06:54:54AM -0700, 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 <mnissler@rivosinc.com>
> > > Reviewed-by: Philippe Mathieu-Daudé <philmd@linaro.org>
> > > Acked-by: Peter Xu <peterx@redhat.com>
> > > ---
> > > This patch is split out from my "Support message-based DMA in vfio-user 
> > > server"
> > > series. With the series having been partially applied, I'm splitting this 
> > > one
> > > out as the only remaining patch to system emulation code in the hope to
> > > simplify getting it landed. The code has previously been reviewed by 
> > > Stefan
> > > Hajnoczi and Peter Xu. This latest version includes changes to switch the
> > > bounce buffer size bookkeeping to `size_t` as requested and LGTM'd by 
> > > Phil in
> > > v9.
> > > ---
> > >  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(-)
> > >
> > > diff --git a/hw/pci/pci.c b/hw/pci/pci.c
> > > index fab86d0567..d2caf3ee8b 100644
> > > --- a/hw/pci/pci.c
> > > +++ b/hw/pci/pci.c
> > > @@ -85,6 +85,8 @@ static Property pci_props[] = {
> > >                      QEMU_PCIE_ERR_UNC_MASK_BITNR, true),
> > >      DEFINE_PROP_BIT("x-pcie-ari-nextfn-1", PCIDevice, cap_present,
> > >                      QEMU_PCIE_ARI_NEXTFN_1_BITNR, false),
> > > +    DEFINE_PROP_SIZE32("x-max-bounce-buffer-size", PCIDevice,
> > > +                     max_bounce_buffer_size, 
> > > DEFAULT_MAX_BOUNCE_BUFFER_SIZE),
> > >      DEFINE_PROP_END_OF_LIST()
> > >  };
> > >
> >
> > I'm a bit puzzled by now there being two fields named
> > max_bounce_buffer_size, one directly controllable by
> > a property.

One is one the pci device, the other is on the address space. The
former can be set via a command line parameter, and that value is used
to initialize the field on the address space, which is then consulted
when allocating bounce buffers.

I'm not sure which aspect of this is unclear and/or deserves
additional commenting - let me know and I'll be happy to send a patch.

> >
> > Pls add code comments explaining how they are related.
> >
> >
> > Also, what is the point of adding a property without
> > making it part of an API? No one will be able to rely on
> > it working.
>
> Note that this patch is already upstream as commit 637b0aa13.
>
> thanks
> -- PMM