Re: [Qemu-devel] Supporting emulation of IOMMUs

[David Gibson]

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