Patchwork [1/5] pci: Simpler implementation of PCI_COMMAND_MASTER bit

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

Comments

David Gibson - April 24, 2013, 12:01 p.m.
In commit 1c380f9460522f32c8dd2577b2a53d518ec91c6d "pci: honor
PCI_COMMAND_MASTER" the PCI_COMMAND_MASTER bit of the PCI command register
was implemented by toggling the enable bit on a memory region alias
interposed between the PCI device's dma address space and the main
system memory region.

Introducing an extra alias region for every PCI device just to implement
that bit seems like serious overkill.  Furthermore, it doesn't work when
there's a (guest side) iommu present, since that uses a different path for
constructing the PCI device's dma address space.

This patch removes the aliased window, instead implementing
PCI_COMMAND_MASTER with tests in the PCI DMA functions.

Signed-off-by: David Gibson <david@gibson.dropbear.id.au>
---
 hw/pci/pci.c         |   14 +-------------
 include/hw/pci/pci.h |   19 ++++++++++++++-----
 2 files changed, 15 insertions(+), 18 deletions(-)
Paolo Bonzini - April 24, 2013, 12:36 p.m.
Il 24/04/2013 14:01, David Gibson ha scritto:
> In commit 1c380f9460522f32c8dd2577b2a53d518ec91c6d "pci: honor
> PCI_COMMAND_MASTER" the PCI_COMMAND_MASTER bit of the PCI command register
> was implemented by toggling the enable bit on a memory region alias
> interposed between the PCI device's dma address space and the main
> system memory region.
> 
> Introducing an extra alias region for every PCI device just to implement
> that bit seems like serious overkill.  Furthermore, it doesn't work when
> there's a (guest side) iommu present, since that uses a different path for
> constructing the PCI device's dma address space.
> 
> This patch removes the aliased window, instead implementing
> PCI_COMMAND_MASTER with tests in the PCI DMA functions.
> 
> Signed-off-by: David Gibson <david@gibson.dropbear.id.au>

This doesn't work.

You have no guarantee that PCI devices use the PCI DMA functions.  The
device could just pass the DMAContext to another function, and indeed
the OHCI controller does exactly that.

This will be even simpler after IOMMU/DMAContext are also unified in the
AddressSpace framework.

Paolo
David Gibson - April 24, 2013, 1:06 p.m.
On Wed, Apr 24, 2013 at 02:36:46PM +0200, Paolo Bonzini wrote:
> Il 24/04/2013 14:01, David Gibson ha scritto:
> > In commit 1c380f9460522f32c8dd2577b2a53d518ec91c6d "pci: honor
> > PCI_COMMAND_MASTER" the PCI_COMMAND_MASTER bit of the PCI command register
> > was implemented by toggling the enable bit on a memory region alias
> > interposed between the PCI device's dma address space and the main
> > system memory region.
> > 
> > Introducing an extra alias region for every PCI device just to implement
> > that bit seems like serious overkill.  Furthermore, it doesn't work when
> > there's a (guest side) iommu present, since that uses a different path for
> > constructing the PCI device's dma address space.
> > 
> > This patch removes the aliased window, instead implementing
> > PCI_COMMAND_MASTER with tests in the PCI DMA functions.
> > 
> > Signed-off-by: David Gibson <david@gibson.dropbear.id.au>
> 
> This doesn't work.

Well.. say rather that it fails to work in a different set of
circumstances from those in which the current scheme fails to work.

> You have no guarantee that PCI devices use the PCI DMA functions.  The
> device could just pass the DMAContext to another function, and indeed
> the OHCI controller does exactly that.

Ah, good point.  Drat.

> This will be even simpler after IOMMU/DMAContext are also unified in the
> AddressSpace framework.
> 
> Paolo
>

Patch

diff --git a/hw/pci/pci.c b/hw/pci/pci.c
index 051da67..2fdd4b2 100644
--- a/hw/pci/pci.c
+++ b/hw/pci/pci.c
@@ -425,10 +425,6 @@  static int get_pci_config_device(QEMUFile *f, void *pv, size_t size)
 
     pci_update_mappings(s);
 
-    memory_region_set_enabled(&s->bus_master_enable_region,
-                              pci_get_word(s->config + PCI_COMMAND)
-                              & PCI_COMMAND_MASTER);
-
     g_free(config);
     return 0;
 }
@@ -850,11 +846,7 @@  static PCIDevice *do_pci_register_device(PCIDevice *pci_dev, PCIBus *bus,
         /* FIXME: Make dma_context_fn use MemoryRegions instead, so this path is
          * taken unconditionally */
         /* FIXME: inherit memory region from bus creator */
-        memory_region_init_alias(&pci_dev->bus_master_enable_region, "bus master",
-                                 get_system_memory(), 0,
-                                 memory_region_size(get_system_memory()));
-        memory_region_set_enabled(&pci_dev->bus_master_enable_region, false);
-        address_space_init(&pci_dev->bus_master_as, &pci_dev->bus_master_enable_region);
+        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);
     }
@@ -913,7 +905,6 @@  static void do_pci_unregister_device(PCIDevice *pci_dev)
 
     if (!pci_dev->bus->dma_context_fn) {
         address_space_destroy(&pci_dev->bus_master_as);
-        memory_region_destroy(&pci_dev->bus_master_enable_region);
         g_free(pci_dev->dma);
         pci_dev->dma = NULL;
     }
@@ -1201,9 +1192,6 @@  void pci_default_write_config(PCIDevice *d, uint32_t addr, uint32_t val, int l)
 
     if (range_covers_byte(addr, l, PCI_COMMAND)) {
         pci_update_irq_disabled(d, was_irq_disabled);
-        memory_region_set_enabled(&d->bus_master_enable_region,
-                                  pci_get_word(d->config + PCI_COMMAND)
-                                    & PCI_COMMAND_MASTER);
     }
 
     msi_write_config(d, addr, val, l);
diff --git a/include/hw/pci/pci.h b/include/hw/pci/pci.h
index 7e5986a..8f682cc 100644
--- a/include/hw/pci/pci.h
+++ b/include/hw/pci/pci.h
@@ -241,7 +241,6 @@  struct PCIDevice {
     char name[64];
     PCIIORegion io_regions[PCI_NUM_REGIONS];
     AddressSpace bus_master_as;
-    MemoryRegion bus_master_enable_region;
     DMAContext *dma;
 
     /* do not access the following fields */
@@ -648,8 +647,12 @@  static inline DMAContext *pci_dma_context(PCIDevice *dev)
 static inline int pci_dma_rw(PCIDevice *dev, dma_addr_t addr,
                              void *buf, dma_addr_t len, DMADirection dir)
 {
-    dma_memory_rw(pci_dma_context(dev), addr, buf, len, dir);
-    return 0;
+    if (pci_get_word(dev->config + PCI_COMMAND) & PCI_COMMAND_MASTER) {
+        dma_memory_rw(pci_dma_context(dev), addr, buf, len, dir);
+        return 0;
+    } else {
+        return -EPERM;
+    }
 }
 
 static inline int pci_dma_read(PCIDevice *dev, dma_addr_t addr,
@@ -668,12 +671,18 @@  static inline int pci_dma_write(PCIDevice *dev, dma_addr_t addr,
     static inline uint##_bits##_t ld##_l##_pci_dma(PCIDevice *dev,      \
                                                    dma_addr_t addr)     \
     {                                                                   \
-        return ld##_l##_dma(pci_dma_context(dev), addr);                \
+        if (pci_get_word(dev->config + PCI_COMMAND) & PCI_COMMAND_MASTER) { \
+            return ld##_l##_dma(pci_dma_context(dev), addr);            \
+        } else {                                                        \
+            return -1;                                                  \
+        }                                                               \
     }                                                                   \
     static inline void st##_s##_pci_dma(PCIDevice *dev,                 \
                                         dma_addr_t addr, uint##_bits##_t val) \
     {                                                                   \
-        st##_s##_dma(pci_dma_context(dev), addr, val);                  \
+        if (pci_get_word(dev->config + PCI_COMMAND) & PCI_COMMAND_MASTER) { \
+            st##_s##_dma(pci_dma_context(dev), addr, val);              \
+        }                                                               \
     }
 
 PCI_DMA_DEFINE_LDST(ub, b, 8);