On Fri, Sep 15, 2023 at 10:37 AM Mattias Nissler <mniss...@rivosinc.com> wrote: > > On Wed, Sep 13, 2023 at 8:30 PM Peter Xu <pet...@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 > >
