On 08/12/2013 07:02 PM, Michael S. Tsirkin wrote: > On Mon, Aug 12, 2013 at 12:36:57AM +1000, Alexey Kardashevskiy wrote: >> On 08/11/2013 11:58 PM, Michael S. Tsirkin wrote: >>> On Sat, Aug 10, 2013 at 01:09:08AM +1000, Alexey Kardashevskiy wrote: >>>> 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. >>>> >>>> If IOMMU is not set, the pci_device_iommu_address_space() function >>>> returns the parent's IOMMU skipping the "bus master" address space as >>>> otherwise proper emulation would require more effort for no benefit. >>>> >>>> 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: >>>> v3: >>>> * added comment about ignoring bus master address space >>>> >>>> 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 | 24 ++++++++++++++++++------ >>>> include/hw/pci/pci.h | 1 + >>>> 2 files changed, 19 insertions(+), 6 deletions(-) >>>> >>>> diff --git a/hw/pci/pci.c b/hw/pci/pci.c >>>> index 4c004f5..dbfa395 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,23 @@ 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) { >>>> + /** We are ignoring the bus master DMA bit of the bridge >>>> + * as it would complicate things such as VFIO for no good reason >>>> */ >>> >>> /* >>> * Always >>> * like >>> * this >>> */ >>> >>> /** Never >>> * like this */ >> >> >> Hm. I thought I saw a lot of those but it was the kernel :) >> btw may comments start with "/**" (with no text in that line but still) - >> what is the difference to "/*"? > > /** are normally for automated generation of docbook from code. > I don't think we do that for QEMU, but in any case, this > would be only any good for properly formatted comments > at top of a function. > >> >>> The comment should be improved I think. >>> I would put it like this: >>> /* >>> * Note: this does not check bus master enable bit on device or >>> * any of the pci to pci bridges above it, it's up to the caller to >>> * check that before initiating the transaction. >>> * >>> * TODO: design a mechanism for callers to do this without >>> * doing bus scans on data path. >>> */ >> >> What exactly do you call here "bus scans"? > > Probably better as 'PCI hierarchy walk'. > >> >>> Would you like me to queue this on the pci tree? If yes I can >>> tweak the comment myself, no need to repost. >> >> Yes, please. Your tree is fine. Thanks! >> > > OK, I'll apply this. > Thanks!
I could not see this patch in the pull request you send today. Something happened to the patch what I should fix? Thanks! -- Alexey