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

[Alexey Kardashevskiy]

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 <address@hidden>
>>>> [aik: added inheritance from parent if iommu is not set for the current 
>>>> bus]
>>>> Signed-off-by: Alexey Kardashevskiy <address@hidden>
>>>>
>>>> ---
>>>> 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