| Message ID | 20241029211607.2114845-6-peterx@redhat.com (mailing list archive) |
|---|---|
| State | New |
| Headers | show |
| Series | QOM: Singleton interface | expand |
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. With regards, Daniel
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,
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 ? With regards, Daniel
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 :| > >
diff --git a/hw/i386/x86-iommu.c b/hw/i386/x86-iommu.c index 60af896225..ec45b80306 100644 --- a/hw/i386/x86-iommu.c +++ b/hw/i386/x86-iommu.c @@ -26,6 +26,7 @@ #include "qemu/error-report.h" #include "trace.h" #include "sysemu/kvm.h" +#include "qom/object_interfaces.h" void x86_iommu_iec_register_notifier(X86IOMMUState *iommu, iec_notify_fn fn, void *data) @@ -79,9 +80,15 @@ void x86_iommu_irq_to_msi_message(X86IOMMUIrq *irq, MSIMessage *msg_out) X86IOMMUState *x86_iommu_get_default(void) { - MachineState *ms = MACHINE(qdev_get_machine()); - PCMachineState *pcms = - PC_MACHINE(object_dynamic_cast(OBJECT(ms), TYPE_PC_MACHINE)); + Object *machine = qdev_get_machine(); + PCMachineState *pcms; + + /* If machine has not been created, so is the vIOMMU */ + if (!machine) { + return NULL; + } + + pcms = PC_MACHINE(object_dynamic_cast(machine, TYPE_PC_MACHINE)); if (pcms && object_dynamic_cast(OBJECT(pcms->iommu), TYPE_X86_IOMMU_DEVICE)) { @@ -133,10 +140,19 @@ static Property x86_iommu_properties[] = { DEFINE_PROP_END_OF_LIST(), }; +static Object *x86_iommu_get_instance(void) +{ + return object_ref(OBJECT(x86_iommu_get_default())); +} + static void x86_iommu_class_init(ObjectClass *klass, void *data) { DeviceClass *dc = DEVICE_CLASS(klass); + SingletonClass *singleton = SINGLETON_CLASS(klass); + dc->realize = x86_iommu_realize; + singleton->get_instance = x86_iommu_get_instance; + device_class_set_props(dc, x86_iommu_properties); } @@ -152,6 +168,10 @@ static const TypeInfo x86_iommu_info = { .class_init = x86_iommu_class_init, .class_size = sizeof(X86IOMMUClass), .abstract = true, + .interfaces = (InterfaceInfo[]) { + { TYPE_SINGLETON }, + { } + } }; static void x86_iommu_register_types(void)
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. Note: after this patch, x86_iommu_get_default() might be called very early, even before machine is created (e.g., cmdline "-device intel-iommu,help"). In this case we need to be prepared with such, and that also means the default iommu is not yet around. Signed-off-by: Peter Xu <peterx@redhat.com> --- hw/i386/x86-iommu.c | 26 +++++++++++++++++++++++--- 1 file changed, 23 insertions(+), 3 deletions(-)