[Top][All Lists]
[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]
Re: [Qemu-devel] Supporting emulation of IOMMUs
From: |
Richard Henderson |
Subject: |
Re: [Qemu-devel] Supporting emulation of IOMMUs |
Date: |
Fri, 29 Apr 2011 07:27:38 -0700 |
User-agent: |
Mozilla/5.0 (X11; U; Linux x86_64; en-US; rv:1.9.2.15) Gecko/20110307 Fedora/3.1.9-0.39.b3pre.fc14 Thunderbird/3.1.9 |
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~