Re: [Qemu-devel] [PATCH v2] pci: Introduce helper to retrieve a PCI device's DMA address space

[David Gibson <david@gibson.dropbear.id.au>]

[-- Attachment #1: Type: text/plain, Size: 2989 bytes --]

On Fri, Aug 09, 2013 at 07:48:16PM +1000, Alexey Kardashevskiy wrote:
> On 08/09/2013 07:40 PM, Paolo Bonzini wrote:
> > Il 09/08/2013 10:49, Alexey Kardashevskiy ha scritto:
> >> A PCI device's DMA address space (possibly an IOMMU) is returned by a
> >> method on the PCIBus.  At the moment that only has one caller, so the
> >> method is simply open coded.  We'll need another caller for VFIO, so
> >> this patch introduces a helper/wrapper function.
> >>
> >> Signed-off-by: David Gibson <david@gibson.dropbear.id.au>
> >> [aik: added inheritance from parent if iommu is not set for the current bus]
> >> Signed-off-by: Alexey Kardashevskiy <aik@ozlabs.ru>
> >>
> >> ---
> >> Changes:
> >> v2:
> >> * added inheritance, needed for a pci-bridge on spapr-ppc64
> >> * pci_iommu_as renamed to pci_device_iommu_address_space
> >> ---
> >>  hw/pci/pci.c         | 22 ++++++++++++++++------
> >>  include/hw/pci/pci.h |  1 +
> >>  2 files changed, 17 insertions(+), 6 deletions(-)
> >>
> >> diff --git a/hw/pci/pci.c b/hw/pci/pci.c
> >> index 4c004f5..0072b54 100644
> >> --- a/hw/pci/pci.c
> >> +++ b/hw/pci/pci.c
> >> @@ -812,12 +812,7 @@ static PCIDevice *do_pci_register_device(PCIDevice *pci_dev, PCIBus *bus,
> >>      }
> >>  
> >>      pci_dev->bus = bus;
> >> -    if (bus->iommu_fn) {
> >> -        dma_as = bus->iommu_fn(bus, bus->iommu_opaque, devfn);
> >> -    } else {
> >> -        /* FIXME: inherit memory region from bus creator */
> >> -        dma_as = &address_space_memory;
> >> -    }
> >> +    dma_as = pci_device_iommu_address_space(pci_dev);
> >>  
> >>      memory_region_init_alias(&pci_dev->bus_master_enable_region,
> >>                               OBJECT(pci_dev), "bus master",
> >> @@ -2239,6 +2234,21 @@ static void pci_device_class_init(ObjectClass *klass, void *data)
> >>      k->props = pci_props;
> >>  }
> >>  
> >> +AddressSpace *pci_device_iommu_address_space(PCIDevice *dev)
> >> +{
> >> +    PCIBus *bus = PCI_BUS(dev->bus);
> >> +
> >> +    if (bus->iommu_fn) {
> >> +        return bus->iommu_fn(bus, bus->iommu_opaque, dev->devfn);
> >> +    }
> >> +
> >> +    if (bus->parent_dev) {
> >> +        return pci_device_iommu_address_space(bus->parent_dev);
> >> +    }
> > 
> > No, this would fail if bus->parent_dev is not NULL but not a PCI device
> > either.
> 
> parent_dev is of the PCIDevice* type, how can it be not a PCI device? :-/

It will be a PCI device, though the confusion is understandable.
There is a parent_dev in both PCIBus, which is always a PCI device if
non-NULL, as well as a parent_dev in BusState, which can be any sort
of device.  If both are non-NULL they will be equal apart from type.

So bus->parent_dev is safe here, bus->qbus.parent_dev would not be.

-- 
David Gibson			| I'll have my music baroque, and my code
david AT gibson.dropbear.id.au	| minimalist, thank you.  NOT _the_ _other_
				| _way_ _around_!
http://www.ozlabs.org/~dgibson

[-- Attachment #2: Type: application/pgp-signature, Size: 198 bytes --]