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

[Peter Xu <peterx@redhat.com>]

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