Re: [PATCH v3 1/5] softmmu: Per-AddressSpace bounce buffering

[Mattias Nissler <mnissler@rivosinc.com>]

On Fri, Sep 15, 2023 at 10:37 AM Mattias Nissler <mnissler@rivosinc.com> wrote:
>
> On Wed, Sep 13, 2023 at 8:30 PM Peter Xu <peterx@redhat.com> wrote:
> >
> > On Thu, Sep 07, 2023 at 06:04:06AM -0700, Mattias Nissler wrote:
> > > @@ -3105,6 +3105,9 @@ void address_space_init(AddressSpace *as, MemoryRegion *root, const char *name)
> > >      as->ioeventfds = NULL;
> > >      QTAILQ_INIT(&as->listeners);
> > >      QTAILQ_INSERT_TAIL(&address_spaces, as, address_spaces_link);
> > > +    as->bounce.in_use = false;
> > > +    qemu_mutex_init(&as->map_client_list_lock);
> > > +    QLIST_INIT(&as->map_client_list);
> > >      as->name = g_strdup(name ? name : "anonymous");
> > >      address_space_update_topology(as);
> > >      address_space_update_ioeventfds(as);
> >
> > Missing the counterpart in do_address_space_destroy()?
>
> Of course, thanks for pointing this out.
>
> >
> > Perhaps we should assert on having no one using the buffer, or on the
> > client list too.
>
> I agree it makes sense to put these assertions, but let me dig a bit
> and do some experiments to see whether these hold true in practice.

To close the loop here: I've experimented a bit to try whether I can
get the shutdown path to trigger the assertions by terminating the
qemu process with mappings present. I tried xhci (for usb_packet_map),
e1000e (for net_tx_pkt_add_raw_fragment_pci), and nvme (for
dma-helpers), and some of them with hacked Linux kernels in attempts
to create problematic situations. I found that cleanup of mappings
seems to work correctly already, I wasn't able to trigger the
assertions I added in do_address_space_destroy. That doesn't prove
absence of a code path that would trigger them, but then that would
just indicate a bug in device model cleanup code that should be fixed
anyways.

>
> >
> > Thanks,
> >
> > --
> > Peter Xu
> >