Re: [Qemu-devel] Supporting emulation of IOMMUs

[Richard Henderson <rth@twiddle.net>]

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~