Patchwork [2/5] pci: Don't create an address space object for every PCI device

login
register
mail settings
Submitter David Gibson
Date April 24, 2013, 12:01 p.m.
Message ID <1366804881-553-3-git-send-email-david@gibson.dropbear.id.au>
Download mbox | patch
Permalink /patch/239177/
State New
Headers show

Comments

David Gibson - April 24, 2013, 12:01 p.m.
Commit 817dcc5368988b023c5e1d3f1444fd370c77c6a9 "pci: give each device its
own address space" did exactly what the comment suggests, it creates an
AddressSpace object for every PCI device to represent the address space
it uses for DMA.  All the objects so constructed are basically identical
to address_space_memory.

While its true that PCI devices can have different effective DMA address
spaces, they usually don't.  Further, the PCI structure already had a way
to represent its DMA address space, through its DMAContext pointer.  The
way those are assigned through an optional callback in the bus also
addresses the "FIXME: inherit memory region from bus creator".

So while its true that the DMAContext handling needs to be better
integrated with the MemoryRegion and AddressSpace handling, that commit
wasn't actually a step in the right direction for it.  Since then, the
DMAContext has been extended so it can backend onto an AddressSpace, and
thereby, a MemoryRegion.  Effectively a DMAContext is now an AddressSpace
with iommu translation handling on top.

Therefore, this patch essentially reverts the earlier commit, making all
PCI devices by default shared the global dma_context_memory which backs
onto main system memory.  Those cases which need to set up different DMA
address spaces for each PCI device should supply a suitable dma_context_fn
in the bus to correctly assign / create suitable DMAContext structures.

Signed-off-by: David Gibson <david@gibson.dropbear.id.au>
---
 hw/pci/pci.c         |   14 ++------------
 include/hw/pci/pci.h |    1 -
 2 files changed, 2 insertions(+), 13 deletions(-)
Paolo Bonzini - April 24, 2013, 12:34 p.m.
Il 24/04/2013 14:01, David Gibson ha scritto:
> So while its true that the DMAContext handling needs to be better
> integrated with the MemoryRegion and AddressSpace handling, that commit
> wasn't actually a step in the right direction for it.  Since then, the
> DMAContext has been extended so it can backend onto an AddressSpace, and
> thereby, a MemoryRegion.  Effectively a DMAContext is now an AddressSpace
> with iommu translation handling on top.
> 
> Therefore, this patch essentially reverts the earlier commit, making all
> PCI devices by default shared the global dma_context_memory which backs
> onto main system memory.  Those cases which need to set up different DMA
> address spaces for each PCI device should supply a suitable dma_context_fn
> in the bus to correctly assign / create suitable DMAContext structures.

I think this will be handled correctly when I submit IOMMU AddressSpace
patches (next week or so).  The structure will be

        PCI device 1                           PCI device 2
    ---------------------------------------------------------------
       AddressSpace 1                          AddressSpace 2
           |                                       |
           | (enable/disable)                      | (enable/disable)
           '-------------------.  .----------------'
                               v  v
                         IOMMU AddressSpace
                                |
                                | (translation)
                                v
                           system memory

VFIO will be able to access the IOMMU AddressSpace simply via
pci_dev->iommu, and that field will be ==-identical  for different PCI
devices.

Paolo
David Gibson - April 24, 2013, 1:07 p.m.
On Wed, Apr 24, 2013 at 02:34:21PM +0200, Paolo Bonzini wrote:
> Il 24/04/2013 14:01, David Gibson ha scritto:
> > So while its true that the DMAContext handling needs to be better
> > integrated with the MemoryRegion and AddressSpace handling, that commit
> > wasn't actually a step in the right direction for it.  Since then, the
> > DMAContext has been extended so it can backend onto an AddressSpace, and
> > thereby, a MemoryRegion.  Effectively a DMAContext is now an AddressSpace
> > with iommu translation handling on top.
> > 
> > Therefore, this patch essentially reverts the earlier commit, making all
> > PCI devices by default shared the global dma_context_memory which backs
> > onto main system memory.  Those cases which need to set up different DMA
> > address spaces for each PCI device should supply a suitable dma_context_fn
> > in the bus to correctly assign / create suitable DMAContext structures.
> 
> I think this will be handled correctly when I submit IOMMU AddressSpace
> patches (next week or so).  The structure will be
> 
>         PCI device 1                           PCI device 2
>     ---------------------------------------------------------------
>        AddressSpace 1                          AddressSpace 2
>            |                                       |
>            | (enable/disable)                      | (enable/disable)
>            '-------------------.  .----------------'
>                                v  v
>                          IOMMU AddressSpace
>                                 |
>                                 | (translation)
>                                 v
>                            system memory
> 
> VFIO will be able to access the IOMMU AddressSpace simply via
> pci_dev->iommu, and that field will be ==-identical  for different PCI
> devices.

Hrm, ok.  Still seems excessively complicated, but as long as there's
a way to get from the pci device to the common address space, I can
work with that.

Patch

diff --git a/hw/pci/pci.c b/hw/pci/pci.c
index 2fdd4b2..038be92 100644
--- a/hw/pci/pci.c
+++ b/hw/pci/pci.c
@@ -843,12 +843,7 @@  static PCIDevice *do_pci_register_device(PCIDevice *pci_dev, PCIBus *bus,
     if (bus->dma_context_fn) {
         pci_dev->dma = bus->dma_context_fn(bus, bus->dma_context_opaque, devfn);
     } else {
-        /* FIXME: Make dma_context_fn use MemoryRegions instead, so this path is
-         * taken unconditionally */
-        /* FIXME: inherit memory region from bus creator */
-        address_space_init(&pci_dev->bus_master_as, get_system_memory());
-        pci_dev->dma = g_new(DMAContext, 1);
-        dma_context_init(pci_dev->dma, &pci_dev->bus_master_as, NULL, NULL, NULL);
+        pci_dev->dma = &dma_context_memory;
     }
     pci_dev->devfn = devfn;
     pstrcpy(pci_dev->name, sizeof(pci_dev->name), name);
@@ -902,12 +897,7 @@  static void do_pci_unregister_device(PCIDevice *pci_dev)
     qemu_free_irqs(pci_dev->irq);
     pci_dev->bus->devices[pci_dev->devfn] = NULL;
     pci_config_free(pci_dev);
-
-    if (!pci_dev->bus->dma_context_fn) {
-        address_space_destroy(&pci_dev->bus_master_as);
-        g_free(pci_dev->dma);
-        pci_dev->dma = NULL;
-    }
+    pci_dev->dma = NULL;
 }
 
 static void pci_unregister_io_regions(PCIDevice *pci_dev)
diff --git a/include/hw/pci/pci.h b/include/hw/pci/pci.h
index 8f682cc..206da1f 100644
--- a/include/hw/pci/pci.h
+++ b/include/hw/pci/pci.h
@@ -240,7 +240,6 @@  struct PCIDevice {
     int32_t devfn;
     char name[64];
     PCIIORegion io_regions[PCI_NUM_REGIONS];
-    AddressSpace bus_master_as;
     DMAContext *dma;
 
     /* do not access the following fields */