| Message ID | 1351161477-6040-1-git-send-email-avi@redhat.com |
|---|---|
| State | New |
| Headers | show |
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 <avi@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 <aurelien@aurel32.net>
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 <avi@redhat.com> Acked-by: Michael S. Tsirkin <mst@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; > }; > -- > 1.7.12
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; };
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 <avi@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(-)