Re: [PATCH RFC v2 5/7] x86/iommu: Make x86-iommu a singleton object

[Peter Xu <peterx@redhat.com>]

[-- Attachment #1: Type: text/plain, Size: 3288 bytes --]

On Wed, Oct 30, 2024, 9:08 a.m. Daniel P. Berrangé <berrange@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 :|
>
>

[-- Attachment #2: Type: text/html, Size: 4797 bytes --]