On Wed, Oct 30, 2024 at 10:33:23AM +0000, Daniel P. Berrangé wrote: > On Tue, Oct 29, 2024 at 05:16:05PM -0400, Peter Xu wrote: > > X86 IOMMUs cannot be created more than one on a system yet. Make it a > > singleton so it guards the system from accidentally create yet another > > IOMMU object when one already presents. > > > > Now if someone tries to create more than one, e.g., via: > > > > ./qemu -M q35 -device intel-iommu -device intel-iommu > > > > The error will change from: > > > > qemu-system-x86_64: -device intel-iommu: QEMU does not support multiple > > vIOMMUs for x86 yet. > > > > To: > > > > qemu-system-x86_64: -device intel-iommu: Class 'intel-iommu' only > > supports one instance > > > > Unfortunately, yet we can't remove the singleton check in the machine > > hook (pc_machine_device_pre_plug_cb), because there can also be > > virtio-iommu involved, which doesn't share a common parent class yet. > > > > But with this, it should be closer to reach that goal to check singleton by > > QOM one day. > > Looking at the other iommu impls, I noticed that they all have something > in common, in that they call pci_setup_iommu from their realize() > function to register their set of callback functions. > > This pci_setup_iommu can happily be called multiple times and just > over-writes previously registered callbacks. I wonder if this is a better > place to diagnose incorrect usage of multiple impls. If pci_setup_iommu > raised an error, it wouldn't matter that virtio-iommu doesn't share > a common parent with intel-iommu. This would also perhaps be better for > a future heterogeneous machine types, where it might be valid to have > multiple iommus concurrently. Checking at the resource setup/registration > point reflects where the physical constraint comes from.
There can still be side effects that vIOMMU code, at least so far, consider it the only object even during init/realize. E.g. vtd_decide_config() has kvm_enable_x2apic() calls which we definitely don't want to be triggered during machine running. The pci_setup_iommu() idea could work indeed but it might still need cleanups here and there all over the places. In general, I was trying to have this as a show case, so in this case it's indeed not required. But I wonder whether other devices / objects can also benefit from it, knowing some of them can only have one instance. I used to believe it could be pretty common in QEMU, but maybe I'm wrong. If there's none of such elsewhere, then I agree the singleton idea may not be that helpful. Thanks, -- Peter Xu
