Re: [Qemu-devel] [PATCH 20/40] pci: use memory core for iommu support

[liu ping fan]

On Wed, May 8, 2013 at 2:30 AM, Peter Maydell <address@hidden> wrote:
> On 7 May 2013 15:16, Paolo Bonzini <address@hidden> wrote:
>> From: Avi Kivity <address@hidden>
>>
>> Use the new iommu support in the memory core for iommu support.  The only
>> user, spapr, is also converted, but it still provides a DMAContext
>> interface until the non-PCI bits switch to AddressSpace.
>>
>> Cc: Michael S. Tsirkin <address@hidden>
>> Signed-off-by: Avi Kivity <address@hidden>
>> Signed-off-by: Paolo Bonzini <address@hidden>
>> ---
>>  hw/pci/pci.c             |   53 
>> ++++++++++++++++++++++++++--------------------
>>  hw/ppc/spapr_pci.c       |   12 +++++++---
>>  include/hw/pci/pci.h     |    7 ++++-
>>  include/hw/pci/pci_bus.h |    5 ++-
>>  4 files changed, 46 insertions(+), 31 deletions(-)
>>
>> diff --git a/hw/pci/pci.c b/hw/pci/pci.c
>> index 16ed118..3eb397c 100644
>> --- a/hw/pci/pci.c
>> +++ b/hw/pci/pci.c
>> @@ -279,6 +279,16 @@ int pci_find_domain(const PCIBus *bus)
>>      return -1;
>>  }
>>
>> +static MemoryRegion *pci_default_iommu(PCIBus *bus, void *opaque, int devfn)
>> +{
>> +    /* FIXME: inherit memory region from bus creator */
>> +    return get_system_memory();
>> +}
>
> This seems a bit misnamed since it doesn't actually need to
> return an iommu MemoryRegion (and in fact in most cases it
> won't). What it's actually returning is "this is the memory
> region representing the view for bus master DMA devices to
> DMA into", I think.
>
Maybe pci_fake_iommu ?

> Also, technically speaking get_system_memory() is never the
> right answer, though in practice it's good enough for our
> purposes. (returning get_system_memory() would allow a bus
> master DMA device to access back into the PCI bus by
> DMAing to the address in system memory where the PCI host
> controller is mapped, which I'm guessing is not possible
> on real hardware.)
>
I think although it is rare case, but PCI device can fetch another's
data on its pci-ram, expecially pci-spec does not forbid it.

Regards,
Pingfan

> As you kind of imply with the FIXME comment here, I think
> what we probably actually eventually want is for pci_bus_init()
> and friends to have a parameter for the DMA MemoryRegion
> (but what this patch does is fine for now).
>
> Once these patches go in I could use this to do the
> versatile_pci SMAP registers, though that's more for
> completeness than anything else.
>
>> +
>> +static void pci_default_iommu_dtor(MemoryRegion *mr)
>> +{
>> +}
>> +
>>  static void pci_bus_init(PCIBus *bus, DeviceState *parent,
>>                           const char *name,
>>                           MemoryRegion *address_space_mem,
>> @@ -289,6 +299,7 @@ static void pci_bus_init(PCIBus *bus, DeviceState 
>> *parent,
>>      bus->devfn_min = devfn_min;
>>      bus->address_space_mem = address_space_mem;
>>      bus->address_space_io = address_space_io;
>> +    pci_setup_iommu(bus, pci_default_iommu, NULL, NULL);
>>
>>      /* host bridge */
>>      QLIST_INIT(&bus->child);
>> @@ -801,21 +812,15 @@ static PCIDevice *do_pci_register_device(PCIDevice 
>> *pci_dev, PCIBus *bus,
>>                       PCI_SLOT(devfn), PCI_FUNC(devfn), name, 
>> bus->devices[devfn]->name);
>>          return NULL;
>>      }
>> +
>>      pci_dev->bus = bus;
>> -    if (bus->dma_context_fn) {
>> -        pci_dev->dma = bus->dma_context_fn(bus, bus->dma_context_opaque, 
>> devfn);
>> -    } else {
>> -        /* FIXME: Make dma_context_fn use MemoryRegions instead, so this 
>> path is
>> -         * taken unconditionally */
>> -        /* FIXME: inherit memory region from bus creator */
>> -        memory_region_init_alias(&pci_dev->bus_master_enable_region, "bus 
>> master",
>> -                                 get_system_memory(), 0,
>> -                                 memory_region_size(get_system_memory()));
>> -        memory_region_set_enabled(&pci_dev->bus_master_enable_region, 
>> false);
>> -        address_space_init(&pci_dev->bus_master_as, 
>> &pci_dev->bus_master_enable_region);
>> -        pci_dev->dma = g_new(DMAContext, 1);
>> -        dma_context_init(pci_dev->dma, &pci_dev->bus_master_as);
>> -    }
>> +    pci_dev->iommu = bus->iommu_fn(bus, bus->iommu_opaque, devfn);
>> +    memory_region_init_alias(&pci_dev->bus_master_enable_region, "bus 
>> master",
>> +                             pci_dev->iommu, 0, 
>> memory_region_size(pci_dev->iommu));
>> +    memory_region_set_enabled(&pci_dev->bus_master_enable_region, false);
>> +    address_space_init(&pci_dev->bus_master_as, 
>> &pci_dev->bus_master_enable_region);
>> +    pci_dev->dma = g_new(DMAContext, 1);
>> +    dma_context_init(pci_dev->dma, &pci_dev->bus_master_as);
>>
>>      pci_dev->devfn = devfn;
>>      pstrcpy(pci_dev->name, sizeof(pci_dev->name), name);
>> @@ -870,12 +875,12 @@ static void do_pci_unregister_device(PCIDevice 
>> *pci_dev)
>>      pci_dev->bus->devices[pci_dev->devfn] = NULL;
>>      pci_config_free(pci_dev);
>>
>> -    if (!pci_dev->bus->dma_context_fn) {
>> -        address_space_destroy(&pci_dev->bus_master_as);
>> -        memory_region_destroy(&pci_dev->bus_master_enable_region);
>> -        g_free(pci_dev->dma);
>> -        pci_dev->dma = NULL;
>> -    }
>> +    address_space_destroy(&pci_dev->bus_master_as);
>> +    memory_region_del_subregion(&pci_dev->bus_master_enable_region, 
>> pci_dev->iommu);
>> +    pci_dev->bus->iommu_dtor_fn(pci_dev->iommu);
>> +    memory_region_destroy(&pci_dev->bus_master_enable_region);
>> +    g_free(pci_dev->dma);
>> +    pci_dev->dma = NULL;
>>  }
>>
>>  static void pci_unregister_io_regions(PCIDevice *pci_dev)
>> @@ -2234,10 +2239,12 @@ static void pci_device_class_init(ObjectClass 
>> *klass, void *data)
>>      k->props = pci_props;
>>  }
>>
>> -void pci_setup_iommu(PCIBus *bus, PCIDMAContextFunc fn, void *opaque)
>> +void pci_setup_iommu(PCIBus *bus, PCIIOMMUFunc fn, PCIIOMMUDestructorFunc 
>> dtor,
>> +                     void *opaque)
>>  {
>> -    bus->dma_context_fn = fn;
>> -    bus->dma_context_opaque = opaque;
>> +    bus->iommu_fn = fn;
>> +    bus->iommu_dtor_fn = dtor ? dtor : pci_default_iommu_dtor;
>> +    bus->iommu_opaque = opaque;
>>  }
>>
>>  static const TypeInfo pci_device_type_info = {
>> diff --git a/hw/ppc/spapr_pci.c b/hw/ppc/spapr_pci.c
>> index eb64a8f..ffbb45e 100644
>> --- a/hw/ppc/spapr_pci.c
>> +++ b/hw/ppc/spapr_pci.c
>> @@ -506,12 +506,11 @@ static const MemoryRegionOps spapr_msi_ops = {
>>  /*
>>   * PHB PCI device
>>   */
>> -static DMAContext *spapr_pci_dma_context_fn(PCIBus *bus, void *opaque,
>> -                                            int devfn)
>> +static MemoryRegion *spapr_pci_dma_iommu(PCIBus *bus, void *opaque, int 
>> devfn)
>>  {
>>      sPAPRPHBState *phb = opaque;
>>
>> -    return spapr_tce_get_dma(phb->tcet);
>> +    return spapr_tce_get_iommu(phb->tcet);
>>  }
>>
>>  static int spapr_phb_init(SysBusDevice *s)
>> @@ -651,7 +655,7 @@ static int spapr_phb_init(SysBusDevice *s)
>>          fprintf(stderr, "Unable to create TCE table for %s\n", 
>> sphb->dtbusname);
>>          return -1;
>>      }
>> -    pci_setup_iommu(bus, spapr_pci_dma_context_fn, sphb);
>> +    pci_setup_iommu(bus, spapr_pci_dma_iommu, NULL, sphb);
>>
>>      QLIST_INSERT_HEAD(&spapr->phbs, sphb, list);
>>
>> diff --git a/include/hw/pci/pci.h b/include/hw/pci/pci.h
>> index 8d075ab..7e7b0f4 100644
>> --- a/include/hw/pci/pci.h
>> +++ b/include/hw/pci/pci.h
>> @@ -242,6 +242,7 @@ struct PCIDevice {
>>      PCIIORegion io_regions[PCI_NUM_REGIONS];
>>      AddressSpace bus_master_as;
>>      MemoryRegion bus_master_enable_region;
>> +    MemoryRegion *iommu;
>>      DMAContext *dma;
>>
>>      /* do not access the following fields */
>> @@ -401,9 +402,11 @@ int pci_read_devaddr(Monitor *mon, const char *addr, 
>> int *domp, int *busp,
>>
>>  void pci_device_deassert_intx(PCIDevice *dev);
>>
>> -typedef DMAContext *(*PCIDMAContextFunc)(PCIBus *, void *, int);
>> +typedef MemoryRegion *(*PCIIOMMUFunc)(PCIBus *, void *, int);
>> +typedef void (*PCIIOMMUDestructorFunc)(MemoryRegion *mr);
>>
>> -void pci_setup_iommu(PCIBus *bus, PCIDMAContextFunc fn, void *opaque);
>> +void pci_setup_iommu(PCIBus *bus, PCIIOMMUFunc fn, PCIIOMMUDestructorFunc 
>> dtor,
>> +                     void *opaque);
>
> I'm not entirely convinced by this API. Some doc comments
> might help...
>
> thanks
> -- PMM