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?
>> 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.
Also, we would have to fix the x86 firmware.
Paolo
