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:
> 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.
> 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.
r~
