Re: [Qemu-devel] [PATCH] pci: avoid destroying bridge address space windows in a transaction

[Michael S. Tsirkin]

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