On Wed, Oct 30, 2024, 9:08 a.m. Daniel P. Berrangé <berra...@redhat.com> wrote:
> On Wed, Oct 30, 2024 at 09:01:03AM -0400, Peter Xu wrote: > > 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. > > The side effects surely don't matter, because when we hit the error > scenario, we'll propagate that up the stack until something calls > exit(), since this is a cold boot path, rather than hotplug ? > Yes, intel iommus are not hot pluggable so it shouldn't be a major concern. But my point is we could have similar devices that either operate on globals or system wide behaviors. Singleton may properly protect it from ever being created. > With regards, > Daniel > -- > |: https://berrange.com -o- > https://www.flickr.com/photos/dberrange :| > |: https://libvirt.org -o- > https://fstop138.berrange.com :| > |: https://entangle-photo.org -o- > https://www.instagram.com/dberrange :| > >
