On Fri, Apr 29, 2011 at 07:27:38AM -0700, Richard Henderson wrote:
> On 04/28/2011 02:57 PM, Richard Henderson wrote:
> > I've had a read through the patches posted in January. It all does
> > seem relatively sane. At least, I can readily see how I would apply
> > these interfaces to my Alpha port without trouble.
>
> I take that back, I see one rather annoying problem: the assumption
> that the translate function operates on some sort of standalone device.
>
> This assumption is present in two places:
Yeah, I noticed this problem too, though I think it's in the AMD IOMMU
support patch rather than the DMA translation core.
> > void pci_register_iommu(PCIDevice *dev, DMATranslateFunc *translate);
>
> Here, you're assuming that the IOMMU is a device on the PCI bus itself.
>
> While this may indeed be how the AMD-IOMMU presents itself for the
> convenience of the pc-minded operating system, that's certainly not how
> the hardware is implemented. In practice it's a function of the PCI
> host controller. And indeed, that's how it is presented to the system
> for the Sparc and Alpha controllers with which I am familiar. I assume
> it's similar for PowerPC, but I've never looked.
Yes, that's right. On pSeries platforms IOMMU translations are
configured using hcalls.
> > struct DMAMmu {
> > DeviceState *iommu;
> > DMATranslateFunc *translate;
> > QLIST_HEAD(memory_maps, DMAMemoryMap) memory_maps;
> > };
>
> Here, you're assuming that the "iommu state" is a standalone qdev.
>
> This is probably true most of the time, given that
>
> FROM_SYSBUS(PCIHostState, sysbus_from_qdev(dev))
>
> is true. However, the Alpha Typhoon chipset has two pci host
> controllers that are tied together in ways that would be
> difficult, or at least irritating, to represent as two separate
> qdev entities.
>
> I suggest that, like many other places we have callbacks, this
> should be an opaque value private to the translate function.
>
> In your AMD-IOMMU case you can then do
>
> pci_register_iommu(dev->bus, amd_iommu_translate, st);
>
> and avoid
>
> > PCIDevice *pci_dev = container_of(dev, PCIDevice, dma);
> > PCIDevice *iommu_dev = DO_UPCAST(PCIDevice, qdev, dev->mmu->iommu);
> > AMDIOMMUState *st = DO_UPCAST(AMDIOMMUState, dev, iommu_dev);
>
> at the beginning of your translate function. You currently have
> three levels of casting and pointer chasing; surely you can agree
> that having a single cast from void* is much easier to follow.
>
> In my Alpha case I can then do
>
> pci_register_iommu(pci_bus0, typhoon_iommu_translate, &state->pchip0);
> pci_register_iommu(pci_bus1, typhoon_iommu_translate, &state->pchip1);
>
> and be off to the races.
Yes, I think that makes sense.
--
David Gibson | I'll have my music baroque, and my code
david AT gibson.dropbear.id.au | minimalist, thank you. NOT _the_ _other_
| _way_ _around_!
http://www.ozlabs.org/~dgibson
