[Top][All Lists]
[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]
Re: [Qemu-devel] [PATCH] pci: avoid destroying bridge address space wind
From: |
Michael S. Tsirkin |
Subject: |
Re: [Qemu-devel] [PATCH] pci: avoid destroying bridge address space windows in a transaction |
Date: |
Mon, 29 Oct 2012 17:10:41 +0200 |
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 <address@hidden>
Acked-by: Michael S. Tsirkin <address@hidden>
> ---
>
> 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
[Qemu-devel] [PATCH] pci: avoid destroying bridge address space windows in a transaction, Avi Kivity, 2012/10/25