On Tue, Aug 27, 2013 at 03:01:03PM +1000, Alexey Kardashevskiy wrote: > 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! >
Nope, I did rebases and moved this out to the not-ready part by mistake. But it's still on my tree so it will get there in the next pull, I plan it about next Sunday. > > > -- > Alexey