Il 09/08/2013 12:13, Alexey Kardashevskiy ha scritto: > On 08/09/2013 07:53 PM, Paolo Bonzini wrote: >> Il 09/08/2013 11:48, Alexey Kardashevskiy ha scritto: >>> 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 <da...@gibson.dropbear.id.au> >>>>> [aik: added inheritance from parent if iommu is not set for the current >>>>> bus] >>>>> Signed-off-by: Alexey Kardashevskiy <a...@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? :-/ >> >> Doh, I misread the code, I thought it was the "parent" field in >> BusState. Why do we have parent_dev at all? > > The code is too old? Don't know. > > >>>> You can use object_dynamic_cast to convert the parent_dev to >>>> PCIDevice, and if the cast succeeds you call the new function. >>>> >>>> Perhaps you could make the new function take a PCIBus instead. >>>> Accessing the PCIDevice's IOMMU address space (as opposed to the >>>> bus-master address space) doesn't make much sense, VFIO is really a >>>> special case. Putting the new API on the bus side instead looks better. >>>> >>>> (BTW, do you need to enable bus-master DMA on PCI bridges, to do DMA for >>>> devices sitting on the secondary bus?) >>> >>> It happens naturally I guess when linux enables devices. >> >> Yes, but then using the IOMMU address space would be wrong; you would >> have to use the bus-master address space as a base for the child's >> bus-master address space. > > > Like this? Works too. > > diff --git a/hw/pci/pci.c b/hw/pci/pci.c > index 8bdcedc..a4c70e6 100644 > --- a/hw/pci/pci.c > +++ b/hw/pci/pci.c > @@ -2247,23 +2247,23 @@ 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); > } > > - return &address_space_memory; > + return &dev->bus_master_as; > }
I was thinking more like this: if (bus->parent_dev) { - return pci_device_iommu_address_space(bus->parent_dev); + /* Take parent device's bus-master enable bit into account. */ + return pci_get_address_space(bus->parent_dev); } + /* Not a secondary bus and no IOMMU. Use system memory. */ return &address_space_memory; Paolo > > >> Also, we would have to fix the x86 firmware. >> >> Paolo >> > >