On Thu, Oct 25, 2012 at 12:37:57PM +0200, Avi Kivity wrote: > Calling memory_region_destroy() in a transaction is illegal (and aborts), > as until the transaction is committed, the region remains live. > > Fix by moving destruction until after the transaction commits. This requires > having an extra set of regions, so the new and old regions can coexist. > > Signed-off-by: Avi Kivity <a...@redhat.com> > --- > > Please review, and check if this patch fixes things for you. > > hw/pci_bridge.c | 50 ++++++++++++++++++++++++++++---------------------- > hw/pci_internals.h | 24 ++++++++++++++++-------- > 2 files changed, 44 insertions(+), 30 deletions(-) > > diff --git a/hw/pci_bridge.c b/hw/pci_bridge.c > index 5c6455f..4680501 100644 > --- a/hw/pci_bridge.c > +++ b/hw/pci_bridge.c > @@ -151,58 +151,63 @@ static void pci_bridge_init_alias(PCIBridge *bridge, > MemoryRegion *alias, > memory_region_add_subregion_overlap(parent_space, base, alias, 1); > } > > -static void pci_bridge_cleanup_alias(MemoryRegion *alias, > - MemoryRegion *parent_space) > -{ > - memory_region_del_subregion(parent_space, alias); > - memory_region_destroy(alias); > -} > - > -static void pci_bridge_region_init(PCIBridge *br) > +static PCIBridgeWindows *pci_bridge_region_init(PCIBridge *br) > { > PCIBus *parent = br->dev.bus; > + PCIBridgeWindows *w = g_new(PCIBridgeWindows, 1); > uint16_t cmd = pci_get_word(br->dev.config + PCI_COMMAND); > > - pci_bridge_init_alias(br, &br->alias_pref_mem, > + pci_bridge_init_alias(br, &w->alias_pref_mem, > PCI_BASE_ADDRESS_MEM_PREFETCH, > "pci_bridge_pref_mem", > &br->address_space_mem, > parent->address_space_mem, > cmd & PCI_COMMAND_MEMORY); > - pci_bridge_init_alias(br, &br->alias_mem, > + pci_bridge_init_alias(br, &w->alias_mem, > PCI_BASE_ADDRESS_SPACE_MEMORY, > "pci_bridge_mem", > &br->address_space_mem, > parent->address_space_mem, > cmd & PCI_COMMAND_MEMORY); > - pci_bridge_init_alias(br, &br->alias_io, > + pci_bridge_init_alias(br, &w->alias_io, > PCI_BASE_ADDRESS_SPACE_IO, > "pci_bridge_io", > &br->address_space_io, > parent->address_space_io, > cmd & PCI_COMMAND_IO); > /* TODO: optinal VGA and VGA palette snooping support. */ > + > + return w; > } > > -static void pci_bridge_region_cleanup(PCIBridge *br) > +static void pci_bridge_region_del(PCIBridge *br, PCIBridgeWindows *w) > { > PCIBus *parent = br->dev.bus; > - pci_bridge_cleanup_alias(&br->alias_io, > - parent->address_space_io); > - pci_bridge_cleanup_alias(&br->alias_mem, > - parent->address_space_mem); > - pci_bridge_cleanup_alias(&br->alias_pref_mem, > - parent->address_space_mem); > + > + memory_region_del_subregion(parent->address_space_io, &w->alias_io); > + memory_region_del_subregion(parent->address_space_mem, &w->alias_mem); > + memory_region_del_subregion(parent->address_space_mem, > &w->alias_pref_mem); > +} > + > +static void pci_bridge_region_cleanup(PCIBridge *br, PCIBridgeWindows *w) > +{ > + memory_region_destroy(&w->alias_io); > + memory_region_destroy(&w->alias_mem); > + memory_region_destroy(&w->alias_pref_mem); > + g_free(w); > } > > static void pci_bridge_update_mappings(PCIBridge *br) > { > + PCIBridgeWindows *w = br->windows; > + > /* Make updates atomic to: handle the case of one VCPU updating the > bridge > * while another accesses an unaffected region. */ > memory_region_transaction_begin(); > - pci_bridge_region_cleanup(br); > - pci_bridge_region_init(br); > + pci_bridge_region_del(br, br->windows); > + br->windows = pci_bridge_region_init(br); > memory_region_transaction_commit(); > + pci_bridge_region_cleanup(br, w); > } > > /* default write_config function for PCI-to-PCI bridge */ > @@ -326,7 +331,7 @@ int pci_bridge_initfn(PCIDevice *dev) > memory_region_init(&br->address_space_mem, "pci_bridge_pci", INT64_MAX); > sec_bus->address_space_io = &br->address_space_io; > memory_region_init(&br->address_space_io, "pci_bridge_io", 65536); > - pci_bridge_region_init(br); > + br->windows = pci_bridge_region_init(br); > QLIST_INIT(&sec_bus->child); > QLIST_INSERT_HEAD(&parent->child, sec_bus, sibling); > return 0; > @@ -338,7 +343,8 @@ void pci_bridge_exitfn(PCIDevice *pci_dev) > PCIBridge *s = DO_UPCAST(PCIBridge, dev, pci_dev); > assert(QLIST_EMPTY(&s->sec_bus.child)); > QLIST_REMOVE(&s->sec_bus, sibling); > - pci_bridge_region_cleanup(s); > + pci_bridge_region_del(s, s->windows); > + pci_bridge_region_cleanup(s, s->windows); > memory_region_destroy(&s->address_space_mem); > memory_region_destroy(&s->address_space_io); > /* qbus_free() is called automatically by qdev_free() */ > diff --git a/hw/pci_internals.h b/hw/pci_internals.h > index c931b64..21d0ce6 100644 > --- a/hw/pci_internals.h > +++ b/hw/pci_internals.h > @@ -40,6 +40,19 @@ struct PCIBus { > int *irq_count; > }; > > +typedef struct PCIBridgeWindows PCIBridgeWindows; > + > +/* > + * Aliases for each of the address space windows that the bridge > + * can forward. Mapped into the bridge's parent's address space, > + * as subregions. > + */ > +struct PCIBridgeWindows { > + MemoryRegion alias_pref_mem; > + MemoryRegion alias_mem; > + MemoryRegion alias_io; > +}; > + > struct PCIBridge { > PCIDevice dev; > > @@ -55,14 +68,9 @@ struct PCIBridge { > */ > MemoryRegion address_space_mem; > MemoryRegion address_space_io; > - /* > - * Aliases for each of the address space windows that the bridge > - * can forward. Mapped into the bridge's parent's address space, > - * as subregions. > - */ > - MemoryRegion alias_pref_mem; > - MemoryRegion alias_mem; > - MemoryRegion alias_io; > + > + PCIBridgeWindows *windows; > + > pci_map_irq_fn map_irq; > const char *bus_name; > };
Tested-by: Aurelien Jarno <aurel...@aurel32.net> -- Aurelien Jarno GPG: 1024D/F1BCDB73 aurel...@aurel32.net http://www.aurel32.net