[libvirt] [PATCH 0/8] Refactor Intel IOMMU support
Another prerequisite for vSMMUv3 support. Well, technically I could go ahead and implement that without refactoring Intel IOMMU support first, but I'd rather not add more validation code to qemu_command. Andrea Bolognani (8): tests: Simplify Intel IOMMU testing tests: Use DO_TEST_CAPS_*() for Intel IOMMU tests: Add negative test for Intel IOMMU conf: Allow NULL for virDomainDeviceInfoCallback conf: Drop DOMAIN_DEVICE_ITERATE_GRAPHICS conf: Add IOMMU support to virDomainDeviceInfoIterate() qemu: Introduce qemuDomainDeviceDefValidateIOMMU() qemu: Tweak Intel IOMMU command line generation src/bhyve/bhyve_device.c | 4 ++ src/conf/domain_addr.c | 9 +++ src/conf/domain_conf.c | 59 ++++++++++++---- src/libxl/libxl_driver.c | 3 + src/qemu/qemu_command.c | 69 +++---------------- src/qemu/qemu_domain.c | 66 +++++++++++++++++- src/qemu/qemu_domain_address.c | 36 +++++++++- src/qemu/qemu_hotplug.c | 3 + ...tel-iommu-caching-mode.x86_64-latest.args} | 15 ++-- .../intel-iommu-caching-mode.xml | 23 +------ ...tel-iommu-device-iotlb.x86_64-latest.args} | 22 +++--- .../intel-iommu-device-iotlb.xml | 1 + ...rgs => intel-iommu-eim.x86_64-latest.args} | 15 ++-- tests/qemuxml2argvdata/intel-iommu-eim.xml | 1 + ... => intel-iommu-machine.x86_64-2.6.0.args} | 9 ++- .../qemuxml2argvdata/intel-iommu-machine.xml | 3 +- ...hine.xml => intel-iommu-wrong-machine.xml} | 8 +-- ...ne.args => intel-iommu.x86_64-latest.args} | 18 +++-- tests/qemuxml2argvdata/intel-iommu.xml | 1 + tests/qemuxml2argvtest.c | 34 ++------- ...ntel-iommu-caching-mode.x86_64-latest.xml} | 0 ...ntel-iommu-device-iotlb.x86_64-latest.xml} | 0 ....xml => intel-iommu-eim.x86_64-latest.xml} | 0 ...l => intel-iommu-machine.x86_64-2.6.0.xml} | 0 ...ommu.xml => intel-iommu.x86_64-latest.xml} | 0 tests/qemuxml2xmltest.c | 15 ++-- 26 files changed, 242 insertions(+), 172 deletions(-) rename tests/qemuxml2argvdata/{intel-iommu-device-iotlb.args => intel-iommu-caching-mode.x86_64-latest.args} (61%) rename tests/qemuxml2argvdata/{intel-iommu-caching-mode.args => intel-iommu-device-iotlb.x86_64-latest.args} (53%) rename tests/qemuxml2argvdata/{intel-iommu-eim.args => intel-iommu-eim.x86_64-latest.args} (62%) rename tests/qemuxml2argvdata/{intel-iommu.args => intel-iommu-machine.x86_64-2.6.0.args} (73%) copy tests/qemuxml2argvdata/{intel-iommu-machine.xml => intel-iommu-wrong-machine.xml} (71%) rename tests/qemuxml2argvdata/{intel-iommu-machine.args => intel-iommu.x86_64-latest.args} (55%) rename tests/qemuxml2xmloutdata/{intel-iommu-caching-mode.xml => intel-iommu-caching-mode.x86_64-latest.xml} (100%) rename tests/qemuxml2xmloutdata/{intel-iommu-device-iotlb.xml => intel-iommu-device-iotlb.x86_64-latest.xml} (100%) rename tests/qemuxml2xmloutdata/{intel-iommu-eim.xml => intel-iommu-eim.x86_64-latest.xml} (100%) rename tests/qemuxml2xmloutdata/{intel-iommu-machine.xml => intel-iommu-machine.x86_64-2.6.0.xml} (100%) rename tests/qemuxml2xmloutdata/{intel-iommu.xml => intel-iommu.x86_64-latest.xml} (100%) -- 2.21.0
Remove a bunch of irrelevant devices and make sure all input files explicitly opt out of USB controllers: the latter change will help later, when we start using DO_TEST_CAPS_LATEST(). Signed-off-by: Andrea Bolognani <abologna@redhat.com> --- .../intel-iommu-caching-mode.args | 9 +------- .../intel-iommu-caching-mode.xml | 23 +------------------ .../intel-iommu-device-iotlb.xml | 1 + tests/qemuxml2argvdata/intel-iommu-eim.xml | 1 + .../qemuxml2argvdata/intel-iommu-machine.xml | 1 + tests/qemuxml2argvdata/intel-iommu.xml | 1 + 6 files changed, 6 insertions(+), 30 deletions(-) diff --git a/tests/qemuxml2argvdata/intel-iommu-caching-mode.args b/tests/qemuxml2argvdata/intel-iommu-caching-mode.args index a6d25c59fb..1d1f35a38d 100644 --- a/tests/qemuxml2argvdata/intel-iommu-caching-mode.args +++ b/tests/qemuxml2argvdata/intel-iommu-caching-mode.args @@ -24,11 +24,4 @@ server,nowait \ -rtc base=utc \ -no-shutdown \ -no-acpi \ --device intel-iommu,intremap=on,caching-mode=on \ --device i82801b11-bridge,id=pci.1,bus=pcie.0,addr=0x1e \ --device pci-bridge,chassis_nr=2,id=pci.2,bus=pci.1,addr=0x0 \ --device ioh3420,port=0x10,chassis=3,id=pci.3,bus=pcie.0,addr=0x2 \ --device ich9-usb-ehci1,id=usb,bus=pci.2,addr=0x2.0x7 \ --netdev user,id=hostnet0 \ --device rtl8139,netdev=hostnet0,id=net0,mac=52:54:00:ab:0c:5c,bus=pci.2,\ -addr=0x1 +-device intel-iommu,intremap=on,caching-mode=on diff --git a/tests/qemuxml2argvdata/intel-iommu-caching-mode.xml b/tests/qemuxml2argvdata/intel-iommu-caching-mode.xml index 36a392403b..23e3702cda 100644 --- a/tests/qemuxml2argvdata/intel-iommu-caching-mode.xml +++ b/tests/qemuxml2argvdata/intel-iommu-caching-mode.xml @@ -18,31 +18,10 @@ <devices> <emulator>/usr/bin/qemu-system-x86_64</emulator> <controller type='pci' index='0' model='pcie-root'/> - <controller type='pci' index='1' model='dmi-to-pci-bridge'> - <model name='i82801b11-bridge'/> - <address type='pci' domain='0x0000' bus='0x00' slot='0x1e' function='0x0'/> - </controller> - <controller type='pci' index='2' model='pci-bridge'> - <model name='pci-bridge'/> - <target chassisNr='2'/> - <address type='pci' domain='0x0000' bus='0x01' slot='0x00' function='0x0'/> - </controller> - <controller type='pci' index='3' model='pcie-root-port'> - <model name='ioh3420'/> - <target chassis='3' port='0x10'/> - <address type='pci' domain='0x0000' bus='0x00' slot='0x02' function='0x0'/> - </controller> + <controller type='usb' index='0' model='none'/> <controller type='sata' index='0'> <address type='pci' domain='0x0000' bus='0x00' slot='0x1f' function='0x2'/> </controller> - <controller type='usb' index='0' model='ich9-ehci1'> - <address type='pci' domain='0x0000' bus='0x02' slot='0x02' function='0x7'/> - </controller> - <interface type='user'> - <mac address='52:54:00:ab:0c:5c'/> - <model type='rtl8139'/> - <address type='pci' domain='0x0000' bus='0x02' slot='0x01' function='0x0'/> - </interface> <input type='mouse' bus='ps2'/> <input type='keyboard' bus='ps2'/> <memballoon model='none'/> diff --git a/tests/qemuxml2argvdata/intel-iommu-device-iotlb.xml b/tests/qemuxml2argvdata/intel-iommu-device-iotlb.xml index 3eb08ab9af..fe365eedc8 100644 --- a/tests/qemuxml2argvdata/intel-iommu-device-iotlb.xml +++ b/tests/qemuxml2argvdata/intel-iommu-device-iotlb.xml @@ -18,6 +18,7 @@ <devices> <emulator>/usr/bin/qemu-system-x86_64</emulator> <controller type='pci' index='0' model='pcie-root'/> + <controller type='usb' index='0' model='none'/> <controller type='sata' index='0'> <address type='pci' domain='0x0000' bus='0x00' slot='0x1f' function='0x2'/> </controller> diff --git a/tests/qemuxml2argvdata/intel-iommu-eim.xml b/tests/qemuxml2argvdata/intel-iommu-eim.xml index 8642ed3499..9770f77383 100644 --- a/tests/qemuxml2argvdata/intel-iommu-eim.xml +++ b/tests/qemuxml2argvdata/intel-iommu-eim.xml @@ -18,6 +18,7 @@ <devices> <emulator>/usr/bin/qemu-system-x86_64</emulator> <controller type='pci' index='0' model='pcie-root'/> + <controller type='usb' index='0' model='none'/> <controller type='sata' index='0'> <address type='pci' domain='0x0000' bus='0x00' slot='0x1f' function='0x2'/> </controller> diff --git a/tests/qemuxml2argvdata/intel-iommu-machine.xml b/tests/qemuxml2argvdata/intel-iommu-machine.xml index 0961e4288d..90aba16156 100644 --- a/tests/qemuxml2argvdata/intel-iommu-machine.xml +++ b/tests/qemuxml2argvdata/intel-iommu-machine.xml @@ -15,6 +15,7 @@ <devices> <emulator>/usr/bin/qemu-system-x86_64</emulator> <controller type='pci' index='0' model='pcie-root'/> + <controller type='usb' index='0' model='none'/> <controller type='sata' index='0'> <address type='pci' domain='0x0000' bus='0x00' slot='0x1f' function='0x2'/> </controller> diff --git a/tests/qemuxml2argvdata/intel-iommu.xml b/tests/qemuxml2argvdata/intel-iommu.xml index 0961e4288d..90aba16156 100644 --- a/tests/qemuxml2argvdata/intel-iommu.xml +++ b/tests/qemuxml2argvdata/intel-iommu.xml @@ -15,6 +15,7 @@ <devices> <emulator>/usr/bin/qemu-system-x86_64</emulator> <controller type='pci' index='0' model='pcie-root'/> + <controller type='usb' index='0' model='none'/> <controller type='sata' index='0'> <address type='pci' domain='0x0000' bus='0x00' slot='0x1f' function='0x2'/> </controller> -- 2.21.0
On Mon, May 20, 2019 at 01:37:46PM +0200, Andrea Bolognani wrote:
Remove a bunch of irrelevant devices and make sure all input files explicitly opt out of USB controllers: the latter change will help later, when we start using DO_TEST_CAPS_LATEST().
Signed-off-by: Andrea Bolognani <abologna@redhat.com> --- .../intel-iommu-caching-mode.args | 9 +------- .../intel-iommu-caching-mode.xml | 23 +------------------ .../intel-iommu-device-iotlb.xml | 1 + tests/qemuxml2argvdata/intel-iommu-eim.xml | 1 + .../qemuxml2argvdata/intel-iommu-machine.xml | 1 + tests/qemuxml2argvdata/intel-iommu.xml | 1 + 6 files changed, 6 insertions(+), 30 deletions(-)
Reviewed-by: Ján Tomko <jtomko@redhat.com> Jano
Signed-off-by: Andrea Bolognani <abologna@redhat.com> --- ...tel-iommu-caching-mode.x86_64-latest.args} | 15 ++++++--- ...tel-iommu-device-iotlb.x86_64-latest.args} | 15 ++++++--- ...rgs => intel-iommu-eim.x86_64-latest.args} | 15 ++++++--- .../qemuxml2argvdata/intel-iommu-machine.args | 26 --------------- ... => intel-iommu-machine.x86_64-2.6.0.args} | 9 +++-- .../qemuxml2argvdata/intel-iommu-machine.xml | 2 +- ...mu.args => intel-iommu.x86_64-latest.args} | 15 ++++++--- tests/qemuxml2argvtest.c | 33 ++++--------------- ...ntel-iommu-caching-mode.x86_64-latest.xml} | 0 ...ntel-iommu-device-iotlb.x86_64-latest.xml} | 0 ....xml => intel-iommu-eim.x86_64-latest.xml} | 0 ...l => intel-iommu-machine.x86_64-2.6.0.xml} | 0 ...ommu.xml => intel-iommu.x86_64-latest.xml} | 0 tests/qemuxml2xmltest.c | 15 +++------ 14 files changed, 58 insertions(+), 87 deletions(-) rename tests/qemuxml2argvdata/{intel-iommu-device-iotlb.args => intel-iommu-caching-mode.x86_64-latest.args} (61%) copy tests/qemuxml2argvdata/{intel-iommu-caching-mode.args => intel-iommu-device-iotlb.x86_64-latest.args} (61%) rename tests/qemuxml2argvdata/{intel-iommu-eim.args => intel-iommu-eim.x86_64-latest.args} (62%) delete mode 100644 tests/qemuxml2argvdata/intel-iommu-machine.args rename tests/qemuxml2argvdata/{intel-iommu-caching-mode.args => intel-iommu-machine.x86_64-2.6.0.args} (73%) rename tests/qemuxml2argvdata/{intel-iommu.args => intel-iommu.x86_64-latest.args} (62%) rename tests/qemuxml2xmloutdata/{intel-iommu-caching-mode.xml => intel-iommu-caching-mode.x86_64-latest.xml} (100%) rename tests/qemuxml2xmloutdata/{intel-iommu-device-iotlb.xml => intel-iommu-device-iotlb.x86_64-latest.xml} (100%) rename tests/qemuxml2xmloutdata/{intel-iommu-eim.xml => intel-iommu-eim.x86_64-latest.xml} (100%) rename tests/qemuxml2xmloutdata/{intel-iommu-machine.xml => intel-iommu-machine.x86_64-2.6.0.xml} (100%) rename tests/qemuxml2xmloutdata/{intel-iommu.xml => intel-iommu.x86_64-latest.xml} (100%) diff --git a/tests/qemuxml2argvdata/intel-iommu-device-iotlb.args b/tests/qemuxml2argvdata/intel-iommu-caching-mode.x86_64-latest.args similarity index 61% rename from tests/qemuxml2argvdata/intel-iommu-device-iotlb.args rename to tests/qemuxml2argvdata/intel-iommu-caching-mode.x86_64-latest.args index 66c4110e93..3c88f0e38f 100644 --- a/tests/qemuxml2argvdata/intel-iommu-device-iotlb.args +++ b/tests/qemuxml2argvdata/intel-iommu-caching-mode.x86_64-latest.args @@ -8,20 +8,25 @@ XDG_CACHE_HOME=/tmp/lib/domain--1-QEMUGuest1/.cache \ XDG_CONFIG_HOME=/tmp/lib/domain--1-QEMUGuest1/.config \ QEMU_AUDIO_DRV=none \ /usr/bin/qemu-system-x86_64 \ --name QEMUGuest1 \ +-name guest=QEMUGuest1,debug-threads=on \ -S \ +-object secret,id=masterKey0,format=raw,\ +file=/tmp/lib/domain--1-QEMUGuest1/master-key.aes \ -machine q35,accel=kvm,usb=off,dump-guest-core=off,kernel_irqchip=split \ -m 214 \ --realtime mlock=off \ +-overcommit mem-lock=off \ -smp 1,sockets=1,cores=1,threads=1 \ -uuid c7a5fdbd-edaf-9455-926a-d65c16db1809 \ -display none \ -no-user-config \ -nodefaults \ --chardev socket,id=charmonitor,path=/tmp/lib/domain--1-QEMUGuest1/monitor.sock,\ -server,nowait \ +-chardev socket,id=charmonitor,fd=1729,server,nowait \ -mon chardev=charmonitor,id=monitor,mode=control \ -rtc base=utc \ -no-shutdown \ -no-acpi \ --device intel-iommu,intremap=on,device-iotlb=on +-boot strict=on \ +-device intel-iommu,intremap=on,caching-mode=on \ +-sandbox on,obsolete=deny,elevateprivileges=deny,spawn=deny,\ +resourcecontrol=deny \ +-msg timestamp=on diff --git a/tests/qemuxml2argvdata/intel-iommu-caching-mode.args b/tests/qemuxml2argvdata/intel-iommu-device-iotlb.x86_64-latest.args similarity index 61% copy from tests/qemuxml2argvdata/intel-iommu-caching-mode.args copy to tests/qemuxml2argvdata/intel-iommu-device-iotlb.x86_64-latest.args index 1d1f35a38d..ff201ec5e8 100644 --- a/tests/qemuxml2argvdata/intel-iommu-caching-mode.args +++ b/tests/qemuxml2argvdata/intel-iommu-device-iotlb.x86_64-latest.args @@ -8,20 +8,25 @@ XDG_CACHE_HOME=/tmp/lib/domain--1-QEMUGuest1/.cache \ XDG_CONFIG_HOME=/tmp/lib/domain--1-QEMUGuest1/.config \ QEMU_AUDIO_DRV=none \ /usr/bin/qemu-system-x86_64 \ --name QEMUGuest1 \ +-name guest=QEMUGuest1,debug-threads=on \ -S \ +-object secret,id=masterKey0,format=raw,\ +file=/tmp/lib/domain--1-QEMUGuest1/master-key.aes \ -machine q35,accel=kvm,usb=off,dump-guest-core=off,kernel_irqchip=split \ -m 214 \ --realtime mlock=off \ +-overcommit mem-lock=off \ -smp 1,sockets=1,cores=1,threads=1 \ -uuid c7a5fdbd-edaf-9455-926a-d65c16db1809 \ -display none \ -no-user-config \ -nodefaults \ --chardev socket,id=charmonitor,path=/tmp/lib/domain--1-QEMUGuest1/monitor.sock,\ -server,nowait \ +-chardev socket,id=charmonitor,fd=1729,server,nowait \ -mon chardev=charmonitor,id=monitor,mode=control \ -rtc base=utc \ -no-shutdown \ -no-acpi \ --device intel-iommu,intremap=on,caching-mode=on +-boot strict=on \ +-device intel-iommu,intremap=on,device-iotlb=on \ +-sandbox on,obsolete=deny,elevateprivileges=deny,spawn=deny,\ +resourcecontrol=deny \ +-msg timestamp=on diff --git a/tests/qemuxml2argvdata/intel-iommu-eim.args b/tests/qemuxml2argvdata/intel-iommu-eim.x86_64-latest.args similarity index 62% rename from tests/qemuxml2argvdata/intel-iommu-eim.args rename to tests/qemuxml2argvdata/intel-iommu-eim.x86_64-latest.args index 6e53b1cf96..53dee161a8 100644 --- a/tests/qemuxml2argvdata/intel-iommu-eim.args +++ b/tests/qemuxml2argvdata/intel-iommu-eim.x86_64-latest.args @@ -8,20 +8,25 @@ XDG_CACHE_HOME=/tmp/lib/domain--1-QEMUGuest1/.cache \ XDG_CONFIG_HOME=/tmp/lib/domain--1-QEMUGuest1/.config \ QEMU_AUDIO_DRV=none \ /usr/bin/qemu-system-x86_64 \ --name QEMUGuest1 \ +-name guest=QEMUGuest1,debug-threads=on \ -S \ +-object secret,id=masterKey0,format=raw,\ +file=/tmp/lib/domain--1-QEMUGuest1/master-key.aes \ -machine q35,accel=kvm,usb=off,dump-guest-core=off,kernel_irqchip=split \ -m 214 \ --realtime mlock=off \ +-overcommit mem-lock=off \ -smp 288,sockets=288,cores=1,threads=1 \ -uuid c7a5fdbd-edaf-9455-926a-d65c16db1809 \ -display none \ -no-user-config \ -nodefaults \ --chardev socket,id=charmonitor,path=/tmp/lib/domain--1-QEMUGuest1/monitor.sock,\ -server,nowait \ +-chardev socket,id=charmonitor,fd=1729,server,nowait \ -mon chardev=charmonitor,id=monitor,mode=control \ -rtc base=utc \ -no-shutdown \ -no-acpi \ --device intel-iommu,intremap=on,eim=on +-boot strict=on \ +-device intel-iommu,intremap=on,eim=on \ +-sandbox on,obsolete=deny,elevateprivileges=deny,spawn=deny,\ +resourcecontrol=deny \ +-msg timestamp=on diff --git a/tests/qemuxml2argvdata/intel-iommu-machine.args b/tests/qemuxml2argvdata/intel-iommu-machine.args deleted file mode 100644 index d6d6f0d5f6..0000000000 --- a/tests/qemuxml2argvdata/intel-iommu-machine.args +++ /dev/null @@ -1,26 +0,0 @@ -LC_ALL=C \ -PATH=/bin \ -HOME=/tmp/lib/domain--1-QEMUGuest1 \ -USER=test \ -LOGNAME=test \ -XDG_DATA_HOME=/tmp/lib/domain--1-QEMUGuest1/.local/share \ -XDG_CACHE_HOME=/tmp/lib/domain--1-QEMUGuest1/.cache \ -XDG_CONFIG_HOME=/tmp/lib/domain--1-QEMUGuest1/.config \ -QEMU_AUDIO_DRV=none \ -/usr/bin/qemu-system-x86_64 \ --name QEMUGuest1 \ --S \ --machine q35,accel=tcg,usb=off,dump-guest-core=off,iommu=on \ --m 214 \ --realtime mlock=off \ --smp 1,sockets=1,cores=1,threads=1 \ --uuid c7a5fdbd-edaf-9455-926a-d65c16db1809 \ --display none \ --no-user-config \ --nodefaults \ --chardev socket,id=charmonitor,path=/tmp/lib/domain--1-QEMUGuest1/monitor.sock,\ -server,nowait \ --mon chardev=charmonitor,id=monitor,mode=control \ --rtc base=utc \ --no-shutdown \ --no-acpi diff --git a/tests/qemuxml2argvdata/intel-iommu-caching-mode.args b/tests/qemuxml2argvdata/intel-iommu-machine.x86_64-2.6.0.args similarity index 73% rename from tests/qemuxml2argvdata/intel-iommu-caching-mode.args rename to tests/qemuxml2argvdata/intel-iommu-machine.x86_64-2.6.0.args index 1d1f35a38d..4ebb4d15d9 100644 --- a/tests/qemuxml2argvdata/intel-iommu-caching-mode.args +++ b/tests/qemuxml2argvdata/intel-iommu-machine.x86_64-2.6.0.args @@ -8,9 +8,11 @@ XDG_CACHE_HOME=/tmp/lib/domain--1-QEMUGuest1/.cache \ XDG_CONFIG_HOME=/tmp/lib/domain--1-QEMUGuest1/.config \ QEMU_AUDIO_DRV=none \ /usr/bin/qemu-system-x86_64 \ --name QEMUGuest1 \ +-name guest=QEMUGuest1,debug-threads=on \ -S \ --machine q35,accel=kvm,usb=off,dump-guest-core=off,kernel_irqchip=split \ +-object secret,id=masterKey0,format=raw,\ +file=/tmp/lib/domain--1-QEMUGuest1/master-key.aes \ +-machine pc-q35-2.6,accel=tcg,usb=off,dump-guest-core=off,iommu=on \ -m 214 \ -realtime mlock=off \ -smp 1,sockets=1,cores=1,threads=1 \ @@ -24,4 +26,5 @@ server,nowait \ -rtc base=utc \ -no-shutdown \ -no-acpi \ --device intel-iommu,intremap=on,caching-mode=on +-boot strict=on \ +-msg timestamp=on diff --git a/tests/qemuxml2argvdata/intel-iommu-machine.xml b/tests/qemuxml2argvdata/intel-iommu-machine.xml index 90aba16156..85027cd938 100644 --- a/tests/qemuxml2argvdata/intel-iommu-machine.xml +++ b/tests/qemuxml2argvdata/intel-iommu-machine.xml @@ -5,7 +5,7 @@ <currentMemory unit='KiB'>219100</currentMemory> <vcpu placement='static'>1</vcpu> <os> - <type arch='x86_64' machine='q35'>hvm</type> + <type arch='x86_64' machine='pc-q35-2.6'>hvm</type> <boot dev='hd'/> </os> <clock offset='utc'/> diff --git a/tests/qemuxml2argvdata/intel-iommu.args b/tests/qemuxml2argvdata/intel-iommu.x86_64-latest.args similarity index 62% rename from tests/qemuxml2argvdata/intel-iommu.args rename to tests/qemuxml2argvdata/intel-iommu.x86_64-latest.args index 0a5a49b847..b4c8ca30b2 100644 --- a/tests/qemuxml2argvdata/intel-iommu.args +++ b/tests/qemuxml2argvdata/intel-iommu.x86_64-latest.args @@ -8,20 +8,25 @@ XDG_CACHE_HOME=/tmp/lib/domain--1-QEMUGuest1/.cache \ XDG_CONFIG_HOME=/tmp/lib/domain--1-QEMUGuest1/.config \ QEMU_AUDIO_DRV=none \ /usr/bin/qemu-system-x86_64 \ --name QEMUGuest1 \ +-name guest=QEMUGuest1,debug-threads=on \ -S \ +-object secret,id=masterKey0,format=raw,\ +file=/tmp/lib/domain--1-QEMUGuest1/master-key.aes \ -machine q35,accel=tcg,usb=off,dump-guest-core=off \ -m 214 \ --realtime mlock=off \ +-overcommit mem-lock=off \ -smp 1,sockets=1,cores=1,threads=1 \ -uuid c7a5fdbd-edaf-9455-926a-d65c16db1809 \ -display none \ -no-user-config \ -nodefaults \ --chardev socket,id=charmonitor,path=/tmp/lib/domain--1-QEMUGuest1/monitor.sock,\ -server,nowait \ +-chardev socket,id=charmonitor,fd=1729,server,nowait \ -mon chardev=charmonitor,id=monitor,mode=control \ -rtc base=utc \ -no-shutdown \ -no-acpi \ --device intel-iommu +-boot strict=on \ +-device intel-iommu \ +-sandbox on,obsolete=deny,elevateprivileges=deny,spawn=deny,\ +resourcecontrol=deny \ +-msg timestamp=on diff --git a/tests/qemuxml2argvtest.c b/tests/qemuxml2argvtest.c index edc19ace6f..1c2a2b3905 100644 --- a/tests/qemuxml2argvtest.c +++ b/tests/qemuxml2argvtest.c @@ -2806,33 +2806,12 @@ mymain(void) QEMU_CAPS_USB_HUB); DO_TEST("acpi-table", NONE); - DO_TEST("intel-iommu", - QEMU_CAPS_DEVICE_INTEL_IOMMU); - DO_TEST("intel-iommu-machine", - QEMU_CAPS_MACHINE_IOMMU); - DO_TEST("intel-iommu-caching-mode", - QEMU_CAPS_MACHINE_KERNEL_IRQCHIP, - QEMU_CAPS_MACHINE_KERNEL_IRQCHIP_SPLIT, - QEMU_CAPS_DEVICE_PCI_BRIDGE, - QEMU_CAPS_DEVICE_DMI_TO_PCI_BRIDGE, - QEMU_CAPS_DEVICE_IOH3420, - QEMU_CAPS_ICH9_AHCI, - QEMU_CAPS_ICH9_USB_EHCI1, - QEMU_CAPS_DEVICE_INTEL_IOMMU, - QEMU_CAPS_INTEL_IOMMU_INTREMAP, - QEMU_CAPS_INTEL_IOMMU_CACHING_MODE); - DO_TEST("intel-iommu-eim", - QEMU_CAPS_MACHINE_KERNEL_IRQCHIP, - QEMU_CAPS_MACHINE_KERNEL_IRQCHIP_SPLIT, - QEMU_CAPS_INTEL_IOMMU_INTREMAP, - QEMU_CAPS_INTEL_IOMMU_EIM, - QEMU_CAPS_DEVICE_INTEL_IOMMU); - DO_TEST("intel-iommu-device-iotlb", - QEMU_CAPS_MACHINE_KERNEL_IRQCHIP, - QEMU_CAPS_MACHINE_KERNEL_IRQCHIP_SPLIT, - QEMU_CAPS_INTEL_IOMMU_INTREMAP, - QEMU_CAPS_INTEL_IOMMU_DEVICE_IOTLB, - QEMU_CAPS_DEVICE_INTEL_IOMMU); + + DO_TEST_CAPS_LATEST("intel-iommu"); + DO_TEST_CAPS_VER("intel-iommu-machine", "2.6.0"); + DO_TEST_CAPS_LATEST("intel-iommu-caching-mode"); + DO_TEST_CAPS_LATEST("intel-iommu-eim"); + DO_TEST_CAPS_LATEST("intel-iommu-device-iotlb"); DO_TEST("cpu-hotplug-startup", QEMU_CAPS_QUERY_HOTPLUGGABLE_CPUS); DO_TEST_PARSE_ERROR("cpu-hotplug-granularity", diff --git a/tests/qemuxml2xmloutdata/intel-iommu-caching-mode.xml b/tests/qemuxml2xmloutdata/intel-iommu-caching-mode.x86_64-latest.xml similarity index 100% rename from tests/qemuxml2xmloutdata/intel-iommu-caching-mode.xml rename to tests/qemuxml2xmloutdata/intel-iommu-caching-mode.x86_64-latest.xml diff --git a/tests/qemuxml2xmloutdata/intel-iommu-device-iotlb.xml b/tests/qemuxml2xmloutdata/intel-iommu-device-iotlb.x86_64-latest.xml similarity index 100% rename from tests/qemuxml2xmloutdata/intel-iommu-device-iotlb.xml rename to tests/qemuxml2xmloutdata/intel-iommu-device-iotlb.x86_64-latest.xml diff --git a/tests/qemuxml2xmloutdata/intel-iommu-eim.xml b/tests/qemuxml2xmloutdata/intel-iommu-eim.x86_64-latest.xml similarity index 100% rename from tests/qemuxml2xmloutdata/intel-iommu-eim.xml rename to tests/qemuxml2xmloutdata/intel-iommu-eim.x86_64-latest.xml diff --git a/tests/qemuxml2xmloutdata/intel-iommu-machine.xml b/tests/qemuxml2xmloutdata/intel-iommu-machine.x86_64-2.6.0.xml similarity index 100% rename from tests/qemuxml2xmloutdata/intel-iommu-machine.xml rename to tests/qemuxml2xmloutdata/intel-iommu-machine.x86_64-2.6.0.xml diff --git a/tests/qemuxml2xmloutdata/intel-iommu.xml b/tests/qemuxml2xmloutdata/intel-iommu.x86_64-latest.xml similarity index 100% rename from tests/qemuxml2xmloutdata/intel-iommu.xml rename to tests/qemuxml2xmloutdata/intel-iommu.x86_64-latest.xml diff --git a/tests/qemuxml2xmltest.c b/tests/qemuxml2xmltest.c index 3b4d7efa52..f11ded4fb3 100644 --- a/tests/qemuxml2xmltest.c +++ b/tests/qemuxml2xmltest.c @@ -1202,16 +1202,11 @@ mymain(void) QEMU_CAPS_DEVICE_VIRTIO_GPU_CCW); DO_TEST("video-none-device", NONE); - DO_TEST("intel-iommu", - QEMU_CAPS_DEVICE_INTEL_IOMMU); - DO_TEST("intel-iommu-machine", - QEMU_CAPS_MACHINE_IOMMU); - DO_TEST("intel-iommu-caching-mode", - QEMU_CAPS_DEVICE_PCI_BRIDGE, - QEMU_CAPS_DEVICE_DMI_TO_PCI_BRIDGE, - QEMU_CAPS_DEVICE_IOH3420); - DO_TEST("intel-iommu-eim", NONE); - DO_TEST("intel-iommu-device-iotlb", NONE); + DO_TEST_CAPS_LATEST("intel-iommu"); + DO_TEST_CAPS_VER("intel-iommu-machine", "2.6.0"); + DO_TEST_CAPS_LATEST("intel-iommu-caching-mode"); + DO_TEST_CAPS_LATEST("intel-iommu-eim"); + DO_TEST_CAPS_LATEST("intel-iommu-device-iotlb"); DO_TEST("cpu-check-none", NONE); DO_TEST("cpu-check-partial", NONE); -- 2.21.0
On Mon, May 20, 2019 at 01:37:47PM +0200, Andrea Bolognani wrote:
Signed-off-by: Andrea Bolognani <abologna@redhat.com> --- ...tel-iommu-caching-mode.x86_64-latest.args} | 15 ++++++--- ...tel-iommu-device-iotlb.x86_64-latest.args} | 15 ++++++--- ...rgs => intel-iommu-eim.x86_64-latest.args} | 15 ++++++--- .../qemuxml2argvdata/intel-iommu-machine.args | 26 --------------- ... => intel-iommu-machine.x86_64-2.6.0.args} | 9 +++-- .../qemuxml2argvdata/intel-iommu-machine.xml | 2 +- ...mu.args => intel-iommu.x86_64-latest.args} | 15 ++++++--- tests/qemuxml2argvtest.c | 33 ++++--------------- ...ntel-iommu-caching-mode.x86_64-latest.xml} | 0 ...ntel-iommu-device-iotlb.x86_64-latest.xml} | 0 ....xml => intel-iommu-eim.x86_64-latest.xml} | 0 ...l => intel-iommu-machine.x86_64-2.6.0.xml} | 0 ...ommu.xml => intel-iommu.x86_64-latest.xml} | 0 tests/qemuxml2xmltest.c | 15 +++------ 14 files changed, 58 insertions(+), 87 deletions(-) rename tests/qemuxml2argvdata/{intel-iommu-device-iotlb.args => intel-iommu-caching-mode.x86_64-latest.args} (61%) copy tests/qemuxml2argvdata/{intel-iommu-caching-mode.args => intel-iommu-device-iotlb.x86_64-latest.args} (61%) rename tests/qemuxml2argvdata/{intel-iommu-eim.args => intel-iommu-eim.x86_64-latest.args} (62%) delete mode 100644 tests/qemuxml2argvdata/intel-iommu-machine.args rename tests/qemuxml2argvdata/{intel-iommu-caching-mode.args => intel-iommu-machine.x86_64-2.6.0.args} (73%) rename tests/qemuxml2argvdata/{intel-iommu.args => intel-iommu.x86_64-latest.args} (62%) rename tests/qemuxml2xmloutdata/{intel-iommu-caching-mode.xml => intel-iommu-caching-mode.x86_64-latest.xml} (100%) rename tests/qemuxml2xmloutdata/{intel-iommu-device-iotlb.xml => intel-iommu-device-iotlb.x86_64-latest.xml} (100%) rename tests/qemuxml2xmloutdata/{intel-iommu-eim.xml => intel-iommu-eim.x86_64-latest.xml} (100%) rename tests/qemuxml2xmloutdata/{intel-iommu-machine.xml => intel-iommu-machine.x86_64-2.6.0.xml} (100%) rename tests/qemuxml2xmloutdata/{intel-iommu.xml => intel-iommu.x86_64-latest.xml} (100%)
diff --git a/tests/qemuxml2argvdata/intel-iommu-machine.xml b/tests/qemuxml2argvdata/intel-iommu-machine.xml index 90aba16156..85027cd938 100644 --- a/tests/qemuxml2argvdata/intel-iommu-machine.xml +++ b/tests/qemuxml2argvdata/intel-iommu-machine.xml @@ -5,7 +5,7 @@ <currentMemory unit='KiB'>219100</currentMemory> <vcpu placement='static'>1</vcpu> <os> - <type arch='x86_64' machine='q35'>hvm</type> + <type arch='x86_64' machine='pc-q35-2.6'>hvm</type>
Given that this is the only difference between intel-iommu-machine.xml and intel-iommu.xml, could you delete intel-iommu-machine.xml and use DO_TEST_CAPS_VER with 2.6.0? We won't be able to reuse the XML file due to only striping the machine aliases for latest XML files, but it will show we use the same input file.
<boot dev='hd'/> </os> <clock offset='utc'/> diff --git a/tests/qemuxml2argvtest.c b/tests/qemuxml2argvtest.c index edc19ace6f..1c2a2b3905 100644 --- a/tests/qemuxml2argvtest.c +++ b/tests/qemuxml2argvtest.c @@ -2806,33 +2806,12 @@ mymain(void) QEMU_CAPS_USB_HUB);
DO_TEST("acpi-table", NONE); - DO_TEST("intel-iommu", - QEMU_CAPS_DEVICE_INTEL_IOMMU); - DO_TEST("intel-iommu-machine", - QEMU_CAPS_MACHINE_IOMMU); - DO_TEST("intel-iommu-caching-mode", - QEMU_CAPS_MACHINE_KERNEL_IRQCHIP, - QEMU_CAPS_MACHINE_KERNEL_IRQCHIP_SPLIT, - QEMU_CAPS_DEVICE_PCI_BRIDGE, - QEMU_CAPS_DEVICE_DMI_TO_PCI_BRIDGE, - QEMU_CAPS_DEVICE_IOH3420, - QEMU_CAPS_ICH9_AHCI, - QEMU_CAPS_ICH9_USB_EHCI1, - QEMU_CAPS_DEVICE_INTEL_IOMMU, - QEMU_CAPS_INTEL_IOMMU_INTREMAP, - QEMU_CAPS_INTEL_IOMMU_CACHING_MODE); - DO_TEST("intel-iommu-eim", - QEMU_CAPS_MACHINE_KERNEL_IRQCHIP, - QEMU_CAPS_MACHINE_KERNEL_IRQCHIP_SPLIT, - QEMU_CAPS_INTEL_IOMMU_INTREMAP, - QEMU_CAPS_INTEL_IOMMU_EIM, - QEMU_CAPS_DEVICE_INTEL_IOMMU); - DO_TEST("intel-iommu-device-iotlb", - QEMU_CAPS_MACHINE_KERNEL_IRQCHIP, - QEMU_CAPS_MACHINE_KERNEL_IRQCHIP_SPLIT, - QEMU_CAPS_INTEL_IOMMU_INTREMAP, - QEMU_CAPS_INTEL_IOMMU_DEVICE_IOTLB, - QEMU_CAPS_DEVICE_INTEL_IOMMU); + + DO_TEST_CAPS_LATEST("intel-iommu"); + DO_TEST_CAPS_VER("intel-iommu-machine", "2.6.0"); + DO_TEST_CAPS_LATEST("intel-iommu-caching-mode"); + DO_TEST_CAPS_LATEST("intel-iommu-eim"); + DO_TEST_CAPS_LATEST("intel-iommu-device-iotlb");
DO_TEST("cpu-hotplug-startup", QEMU_CAPS_QUERY_HOTPLUGGABLE_CPUS); DO_TEST_PARSE_ERROR("cpu-hotplug-granularity", diff --git a/tests/qemuxml2xmltest.c b/tests/qemuxml2xmltest.c index 3b4d7efa52..f11ded4fb3 100644 --- a/tests/qemuxml2xmltest.c +++ b/tests/qemuxml2xmltest.c @@ -1202,16 +1202,11 @@ mymain(void) QEMU_CAPS_DEVICE_VIRTIO_GPU_CCW); DO_TEST("video-none-device", NONE);
- DO_TEST("intel-iommu", - QEMU_CAPS_DEVICE_INTEL_IOMMU); - DO_TEST("intel-iommu-machine", - QEMU_CAPS_MACHINE_IOMMU); - DO_TEST("intel-iommu-caching-mode", - QEMU_CAPS_DEVICE_PCI_BRIDGE, - QEMU_CAPS_DEVICE_DMI_TO_PCI_BRIDGE, - QEMU_CAPS_DEVICE_IOH3420); - DO_TEST("intel-iommu-eim", NONE); - DO_TEST("intel-iommu-device-iotlb", NONE); + DO_TEST_CAPS_LATEST("intel-iommu"); + DO_TEST_CAPS_VER("intel-iommu-machine", "2.6.0"); + DO_TEST_CAPS_LATEST("intel-iommu-caching-mode"); + DO_TEST_CAPS_LATEST("intel-iommu-eim"); + DO_TEST_CAPS_LATEST("intel-iommu-device-iotlb");
Reviewed-by: Ján Tomko <jtomko@redhat.com> Jano
On Tue, 2019-05-21 at 16:15 +0200, Ján Tomko wrote:
On Mon, May 20, 2019 at 01:37:47PM +0200, Andrea Bolognani wrote:
diff --git a/tests/qemuxml2argvdata/intel-iommu-machine.xml b/tests/qemuxml2argvdata/intel-iommu-machine.xml index 90aba16156..85027cd938 100644 --- a/tests/qemuxml2argvdata/intel-iommu-machine.xml +++ b/tests/qemuxml2argvdata/intel-iommu-machine.xml @@ -5,7 +5,7 @@ <currentMemory unit='KiB'>219100</currentMemory> <vcpu placement='static'>1</vcpu> <os> - <type arch='x86_64' machine='q35'>hvm</type> + <type arch='x86_64' machine='pc-q35-2.6'>hvm</type>
Given that this is the only difference between intel-iommu-machine.xml and intel-iommu.xml, could you delete intel-iommu-machine.xml and use DO_TEST_CAPS_VER with 2.6.0?
We won't be able to reuse the XML file due to only striping the machine aliases for latest XML files, but it will show we use the same input file.
I'm not sure I fully understand what you're suggesting... Do you mean squashing in something like the diff below? Personally I like the idea of using the same input file for different DO_TEST*() calls, highlighting how the environment is the only thing causing differences in the output. That said, in the past I've been told (I think by Peter?) doing so is not a good idea, so I've avoided it since. We'd also lose, as you mention yourself, the nice property of the output file being a symlink to the input file. tl;dr I'm perfectly happy using either the patch as-is or with the diff below squashed in; you guys tell me which one I should go for. diff --git a/tests/qemuxml2argvdata/intel-iommu-machine.x86_64-2.6.0.args b/tests/qemuxml2argvdata/intel-iommu.x86_64-2.6.0.args similarity index 100% rename from tests/qemuxml2argvdata/intel-iommu-machine.x86_64-2.6.0.args rename to tests/qemuxml2argvdata/intel-iommu.x86_64-2.6.0.args diff --git a/tests/qemuxml2argvtest.c b/tests/qemuxml2argvtest.c index 1c2a2b3905..09d37eb454 100644 --- a/tests/qemuxml2argvtest.c +++ b/tests/qemuxml2argvtest.c @@ -2808,7 +2808,7 @@ mymain(void) DO_TEST("acpi-table", NONE); DO_TEST_CAPS_LATEST("intel-iommu"); - DO_TEST_CAPS_VER("intel-iommu-machine", "2.6.0"); + DO_TEST_CAPS_VER("intel-iommu", "2.6.0"); DO_TEST_CAPS_LATEST("intel-iommu-caching-mode"); DO_TEST_CAPS_LATEST("intel-iommu-eim"); DO_TEST_CAPS_LATEST("intel-iommu-device-iotlb"); diff --git a/tests/qemuxml2xmloutdata/intel-iommu-machine.x86_64-2.6.0.xml b/tests/qemuxml2xmloutdata/intel-iommu-machine.x86_64-2.6.0.xml deleted file mode 120000 index dd29ce5ff0..0000000000 --- a/tests/qemuxml2xmloutdata/intel-iommu-machine.x86_64-2.6.0.xml +++ /dev/null @@ -1 +0,0 @@ -../qemuxml2argvdata/intel-iommu-machine.xml \ No newline at end of file diff --git a/tests/qemuxml2argvdata/intel-iommu-machine.xml b/tests/qemuxml2xmloutdata/intel-iommu.x86_64-2.6.0.xml similarity index 100% rename from tests/qemuxml2argvdata/intel-iommu-machine.xml rename to tests/qemuxml2xmloutdata/intel-iommu.x86_64-2.6.0.xml diff --git a/tests/qemuxml2xmltest.c b/tests/qemuxml2xmltest.c index f11ded4fb3..d1e7fe1015 100644 --- a/tests/qemuxml2xmltest.c +++ b/tests/qemuxml2xmltest.c @@ -1203,7 +1203,7 @@ mymain(void) DO_TEST("video-none-device", NONE); DO_TEST_CAPS_LATEST("intel-iommu"); - DO_TEST_CAPS_VER("intel-iommu-machine", "2.6.0"); + DO_TEST_CAPS_VER("intel-iommu", "2.6.0"); DO_TEST_CAPS_LATEST("intel-iommu-caching-mode"); DO_TEST_CAPS_LATEST("intel-iommu-eim"); DO_TEST_CAPS_LATEST("intel-iommu-device-iotlb"); -- Andrea Bolognani / Red Hat / Virtualization
On Wed, May 22, 2019 at 10:33:39AM +0200, Andrea Bolognani wrote:
On Tue, 2019-05-21 at 16:15 +0200, Ján Tomko wrote:
On Mon, May 20, 2019 at 01:37:47PM +0200, Andrea Bolognani wrote:
diff --git a/tests/qemuxml2argvdata/intel-iommu-machine.xml b/tests/qemuxml2argvdata/intel-iommu-machine.xml index 90aba16156..85027cd938 100644 --- a/tests/qemuxml2argvdata/intel-iommu-machine.xml +++ b/tests/qemuxml2argvdata/intel-iommu-machine.xml @@ -5,7 +5,7 @@ <currentMemory unit='KiB'>219100</currentMemory> <vcpu placement='static'>1</vcpu> <os> - <type arch='x86_64' machine='q35'>hvm</type> + <type arch='x86_64' machine='pc-q35-2.6'>hvm</type>
Given that this is the only difference between intel-iommu-machine.xml and intel-iommu.xml, could you delete intel-iommu-machine.xml and use DO_TEST_CAPS_VER with 2.6.0?
We won't be able to reuse the XML file due to only striping the machine aliases for latest XML files, but it will show we use the same input file.
I'm not sure I fully understand what you're suggesting... Do you mean squashing in something like the diff below?
Yes.
Personally I like the idea of using the same input file for different DO_TEST*() calls, highlighting how the environment is the only thing causing differences in the output. That said, in the past I've been told (I think by Peter?) doing so is not a good idea, so I've avoided it since.
For xml->argv test, the outputs are very different. But the xml->xml test only changes the machine type, which is IMO not worth including another input file.
We'd also lose, as you mention yourself, the nice property of the output file being a symlink to the input file.
tl;dr I'm perfectly happy using either the patch as-is or with the diff below squashed in; you guys tell me which one I should go for.
With this diff squashed in, please. Jano
On Wed, May 22, 2019 at 12:34:18 +0200, Ján Tomko wrote:
On Wed, May 22, 2019 at 10:33:39AM +0200, Andrea Bolognani wrote:
On Tue, 2019-05-21 at 16:15 +0200, Ján Tomko wrote:
On Mon, May 20, 2019 at 01:37:47PM +0200, Andrea Bolognani wrote:
diff --git a/tests/qemuxml2argvdata/intel-iommu-machine.xml b/tests/qemuxml2argvdata/intel-iommu-machine.xml index 90aba16156..85027cd938 100644 --- a/tests/qemuxml2argvdata/intel-iommu-machine.xml +++ b/tests/qemuxml2argvdata/intel-iommu-machine.xml @@ -5,7 +5,7 @@ <currentMemory unit='KiB'>219100</currentMemory> <vcpu placement='static'>1</vcpu> <os> - <type arch='x86_64' machine='q35'>hvm</type> + <type arch='x86_64' machine='pc-q35-2.6'>hvm</type>
Given that this is the only difference between intel-iommu-machine.xml and intel-iommu.xml, could you delete intel-iommu-machine.xml and use DO_TEST_CAPS_VER with 2.6.0?
We won't be able to reuse the XML file due to only striping the machine aliases for latest XML files, but it will show we use the same input file.
I'm not sure I fully understand what you're suggesting... Do you mean squashing in something like the diff below?
Yes.
Personally I like the idea of using the same input file for different DO_TEST*() calls, highlighting how the environment is the only thing causing differences in the output. That said, in the past I've been told (I think by Peter?) doing so is not a good idea, so I've avoided it since.
For xml->argv test, the outputs are very different. But the xml->xml test only changes the machine type, which is IMO not worth including another input file.
Note that the xml->argv code specifically deletes the default machine type alias in the 'latest' tests capability data so that the substitution is skipped. This is to avoid having change the files every time we bump the latest caps file. For specific version tests we do want to do this so that we can also excercise the default machine type code. This means also we should do the same in the XML2XML tests. If the files differ only in the machine type default I don't think it's worth having (except the case when we are specifically testing machine type substitution).
On Wed, 2019-05-22 at 13:45 +0200, Peter Krempa wrote:
On Wed, May 22, 2019 at 12:34:18 +0200, Ján Tomko wrote:
On Wed, May 22, 2019 at 10:33:39AM +0200, Andrea Bolognani wrote:
Personally I like the idea of using the same input file for different DO_TEST*() calls, highlighting how the environment is the only thing causing differences in the output. That said, in the past I've been told (I think by Peter?) doing so is not a good idea, so I've avoided it since.
For xml->argv test, the outputs are very different. But the xml->xml test only changes the machine type, which is IMO not worth including another input file.
Note that the xml->argv code specifically deletes the default machine type alias in the 'latest' tests capability data so that the substitution is skipped. This is to avoid having change the files every time we bump the latest caps file.
For specific version tests we do want to do this so that we can also excercise the default machine type code. This means also we should do the same in the XML2XML tests.
If the files differ only in the machine type default I don't think it's worth having (except the case when we are specifically testing machine type substitution).
It looks like I was misremembering then. I've squashed in the diff Jano suggested, and pushed the first three patches of the series. Thanks for looking at it :) -- Andrea Bolognani / Red Hat / Virtualization
Make sure validation is working as intended by trying to use Intel IOMMU with the i440fx machine type, though we know it's q35 only, and expecting an error to be returned. Signed-off-by: Andrea Bolognani <abologna@redhat.com> --- .../intel-iommu-wrong-machine.xml | 24 +++++++++++++++++++ tests/qemuxml2argvtest.c | 1 + 2 files changed, 25 insertions(+) create mode 100644 tests/qemuxml2argvdata/intel-iommu-wrong-machine.xml diff --git a/tests/qemuxml2argvdata/intel-iommu-wrong-machine.xml b/tests/qemuxml2argvdata/intel-iommu-wrong-machine.xml new file mode 100644 index 0000000000..ab116f83b3 --- /dev/null +++ b/tests/qemuxml2argvdata/intel-iommu-wrong-machine.xml @@ -0,0 +1,24 @@ +<domain type='qemu'> + <name>QEMUGuest1</name> + <uuid>c7a5fdbd-edaf-9455-926a-d65c16db1809</uuid> + <memory unit='KiB'>219100</memory> + <currentMemory unit='KiB'>219100</currentMemory> + <vcpu placement='static'>1</vcpu> + <os> + <type arch='x86_64' machine='pc'>hvm</type> + <boot dev='hd'/> + </os> + <clock offset='utc'/> + <on_poweroff>destroy</on_poweroff> + <on_reboot>restart</on_reboot> + <on_crash>destroy</on_crash> + <devices> + <emulator>/usr/bin/qemu-system-x86_64</emulator> + <controller type='pci' index='0' model='pci-root'/> + <controller type='usb' index='0' model='none'/> + <input type='mouse' bus='ps2'/> + <input type='keyboard' bus='ps2'/> + <memballoon model='none'/> + <iommu model='intel'/> + </devices> +</domain> diff --git a/tests/qemuxml2argvtest.c b/tests/qemuxml2argvtest.c index 1c2a2b3905..0e83acac86 100644 --- a/tests/qemuxml2argvtest.c +++ b/tests/qemuxml2argvtest.c @@ -2812,6 +2812,7 @@ mymain(void) DO_TEST_CAPS_LATEST("intel-iommu-caching-mode"); DO_TEST_CAPS_LATEST("intel-iommu-eim"); DO_TEST_CAPS_LATEST("intel-iommu-device-iotlb"); + DO_TEST_FAILURE("intel-iommu-wrong-machine", NONE); DO_TEST("cpu-hotplug-startup", QEMU_CAPS_QUERY_HOTPLUGGABLE_CPUS); DO_TEST_PARSE_ERROR("cpu-hotplug-granularity", -- 2.21.0
On Mon, May 20, 2019 at 01:37:48PM +0200, Andrea Bolognani wrote:
Make sure validation is working as intended by trying to use Intel IOMMU with the i440fx machine type, though we know it's q35 only, and expecting an error to be returned.
Signed-off-by: Andrea Bolognani <abologna@redhat.com> --- .../intel-iommu-wrong-machine.xml | 24 +++++++++++++++++++ tests/qemuxml2argvtest.c | 1 + 2 files changed, 25 insertions(+) create mode 100644 tests/qemuxml2argvdata/intel-iommu-wrong-machine.xml
Reviewed-by: Ján Tomko <jtomko@redhat.com> Jano
The virDomainDeviceInfoIterate() function was initially written with the expectation that all devices would embed a virDomainDeviceInfo, and thus the user-provided callback would never be passed NULL; however, that doesn't really represent reality, as multiple devices don't have any virDomainDeviceInfo associated with them. Since we still want to be able to iterate over those devices, clarify that callbacks are expected to be able to handle NULL being passed to them, and update all existing callbacks so that they actually do so. Signed-off-by: Andrea Bolognani <abologna@redhat.com> --- src/bhyve/bhyve_device.c | 4 ++++ src/conf/domain_addr.c | 9 +++++++++ src/conf/domain_conf.c | 34 +++++++++++++++++++++++++++++++- src/libxl/libxl_driver.c | 3 +++ src/qemu/qemu_domain_address.c | 36 +++++++++++++++++++++++++++++++--- src/qemu/qemu_hotplug.c | 3 +++ 6 files changed, 85 insertions(+), 4 deletions(-) diff --git a/src/bhyve/bhyve_device.c b/src/bhyve/bhyve_device.c index 201044d9e6..8c897cbd8d 100644 --- a/src/bhyve/bhyve_device.c +++ b/src/bhyve/bhyve_device.c @@ -37,6 +37,10 @@ bhyveCollectPCIAddress(virDomainDefPtr def ATTRIBUTE_UNUSED, void *opaque) { int ret = -1; + + if (!info) + return 0; + if (info->type == VIR_DOMAIN_DEVICE_ADDRESS_TYPE_DRIVE) return 0; diff --git a/src/conf/domain_addr.c b/src/conf/domain_addr.c index 04c4e6d7e1..548af89682 100644 --- a/src/conf/domain_addr.c +++ b/src/conf/domain_addr.c @@ -1468,6 +1468,9 @@ virDomainCCWAddressAllocate(virDomainDefPtr def ATTRIBUTE_UNUSED, virDomainDeviceInfoPtr info, void *data) { + if (!info) + return 0; + return virDomainCCWAddressAssign(info, data, true); } @@ -1477,6 +1480,9 @@ virDomainCCWAddressValidate(virDomainDefPtr def ATTRIBUTE_UNUSED, virDomainDeviceInfoPtr info, void *data) { + if (!info) + return 0; + return virDomainCCWAddressAssign(info, data, false); } @@ -1694,6 +1700,9 @@ virDomainVirtioSerialAddrReserve(virDomainDefPtr def ATTRIBUTE_UNUSED, bool b; ssize_t i; + if (!info) + return 0; + if (!virDomainVirtioSerialAddrIsComplete(info)) return 0; diff --git a/src/conf/domain_conf.c b/src/conf/domain_conf.c index a3a514136b..f42d331341 100644 --- a/src/conf/domain_conf.c +++ b/src/conf/domain_conf.c @@ -4049,6 +4049,9 @@ virDomainDefHasDeviceAddressIterator(virDomainDefPtr def ATTRIBUTE_UNUSED, { virDomainDeviceInfoPtr needle = opaque; + if (!info) + return 0; + /* break iteration if the info was found */ if (virDomainDeviceInfoAddressIsEqual(info, needle)) return -1; @@ -4297,6 +4300,21 @@ virDomainDeviceInfoIterateInternal(virDomainDefPtr def, } +/** + * virDomainDeviceInfoIterate: + * @def: domain definition + * @cb: callback + * @opaque: user data + * + * Call @cb for each device in @def. + * + * Note that some devices might not have a virDomainDeviceInfoPtr associated + * with them, in which case the corresponding argument passed to the callback + * will be NULL: @cb should be written to account for this possibility, which + * usually involves returning early. + * + * Return: 0 on success, <0 on failure + */ int virDomainDeviceInfoIterate(virDomainDefPtr def, virDomainDeviceInfoCallback cb, @@ -5543,6 +5561,9 @@ virDomainDefCollectBootOrder(virDomainDefPtr def ATTRIBUTE_UNUSED, virHashTablePtr bootHash = data; VIR_AUTOFREE(char *) order = NULL; + if (!info) + return 0; + if (info->bootIndex == 0) return 0; @@ -6383,7 +6404,12 @@ virDomainDeviceDefValidateAliasesIterator(virDomainDefPtr def, void *opaque) { struct virDomainDefValidateAliasesData *data = opaque; - const char *alias = info->alias; + const char *alias; + + if (!info) + return 0; + + alias = info->alias; if (!virDomainDeviceAliasIsUserAlias(alias)) return 0; @@ -28767,6 +28793,9 @@ virDomainDeviceInfoCheckBootIndex(virDomainDefPtr def ATTRIBUTE_UNUSED, { virDomainCompatibleDeviceDataPtr data = opaque; + if (!info) + return 0; + /* Ignore the device we're about to update */ if (data->oldInfo == info) return 0; @@ -29856,6 +29885,9 @@ virDomainDefFindDeviceCallback(virDomainDefPtr def ATTRIBUTE_UNUSED, { virDomainDefFindDeviceCallbackData *data = opaque; + if (!info) + return 0; + if (STREQ_NULLABLE(info->alias, data->devAlias)) { *data->dev = *dev; return -1; diff --git a/src/libxl/libxl_driver.c b/src/libxl/libxl_driver.c index 2b9c6f1866..560f278761 100644 --- a/src/libxl/libxl_driver.c +++ b/src/libxl/libxl_driver.c @@ -3593,6 +3593,9 @@ libxlComparePCIDevice(virDomainDefPtr def ATTRIBUTE_UNUSED, { virDomainDeviceInfoPtr info2 = opaque; + if (!info1) + return 0; + if (info1->type != VIR_DOMAIN_DEVICE_ADDRESS_TYPE_PCI || info2->type != VIR_DOMAIN_DEVICE_ADDRESS_TYPE_PCI) return 0; diff --git a/src/qemu/qemu_domain_address.c b/src/qemu/qemu_domain_address.c index 4b99e8ca93..5bef29b9df 100644 --- a/src/qemu/qemu_domain_address.c +++ b/src/qemu/qemu_domain_address.c @@ -169,6 +169,9 @@ qemuDomainSpaprVIOFindByReg(virDomainDefPtr def ATTRIBUTE_UNUSED, { virDomainDeviceInfoPtr target = opaque; + if (!info) + return 0; + if (info->type != VIR_DOMAIN_DEVICE_ADDRESS_TYPE_SPAPRVIO) return 0; @@ -427,6 +430,9 @@ qemuDomainHasVirtioMMIODevicesCallback(virDomainDefPtr def ATTRIBUTE_UNUSED, virDomainDeviceInfoPtr info, void *opaque) { + if (!info) + return 0; + if (info->type == VIR_DOMAIN_DEVICE_ADDRESS_TYPE_VIRTIO_MMIO) { /* We can stop iterating as soon as we find the first * virtio-mmio device */ @@ -1083,6 +1089,9 @@ qemuDomainFillDevicePCIConnectFlagsIter(virDomainDefPtr def ATTRIBUTE_UNUSED, { qemuDomainFillDevicePCIConnectFlagsIterData *data = opaque; + if (!info) + return 0; + info->pciConnectFlags = qemuDomainDeviceCalculatePCIConnectFlags(dev, data->driver, data->pcieFlags, @@ -1139,6 +1148,9 @@ qemuDomainFillDevicePCIExtensionFlagsIter(virDomainDefPtr def ATTRIBUTE_UNUSED, { virQEMUCapsPtr qemuCaps = opaque; + if (!info) + return 0; + info->pciAddrExtFlags = qemuDomainDeviceCalculatePCIAddressExtensionFlags(qemuCaps, dev); @@ -1188,6 +1200,9 @@ qemuDomainFindUnusedIsolationGroupIter(virDomainDefPtr def ATTRIBUTE_UNUSED, { unsigned int *isolationGroup = opaque; + if (!info) + return 0; + if (info->isolationGroup == *isolationGroup) return -1; @@ -1479,7 +1494,12 @@ qemuDomainAssignPCIAddressExtension(virDomainDefPtr def ATTRIBUTE_UNUSED, void *opaque) { virDomainPCIAddressSetPtr addrs = opaque; - virPCIDeviceAddressPtr addr = &info->addr.pci; + virPCIDeviceAddressPtr addr; + + if (!info) + return 0; + + addr = &info->addr.pci; if (info->type == VIR_DOMAIN_DEVICE_ADDRESS_TYPE_PCI) addr->extFlags = info->pciAddrExtFlags; @@ -1498,7 +1518,12 @@ qemuDomainCollectPCIAddress(virDomainDefPtr def ATTRIBUTE_UNUSED, { virDomainPCIAddressSetPtr addrs = opaque; int ret = -1; - virPCIDeviceAddressPtr addr = &info->addr.pci; + virPCIDeviceAddressPtr addr; + + if (!info) + return 0; + + addr = &info->addr.pci; if (!virDeviceInfoPCIAddressIsPresent(info) || ((device->type == VIR_DOMAIN_DEVICE_HOSTDEV) && @@ -1590,7 +1615,12 @@ qemuDomainCollectPCIAddressExtension(virDomainDefPtr def ATTRIBUTE_UNUSED, void *opaque) { virDomainPCIAddressSetPtr addrs = opaque; - virPCIDeviceAddressPtr addr = &info->addr.pci; + virPCIDeviceAddressPtr addr; + + if (!info) + return 0; + + addr = &info->addr.pci; if (info->type == VIR_DOMAIN_DEVICE_ADDRESS_TYPE_PCI) addr->extFlags = info->pciAddrExtFlags; diff --git a/src/qemu/qemu_hotplug.c b/src/qemu/qemu_hotplug.c index 59ff88565d..e275685a60 100644 --- a/src/qemu/qemu_hotplug.c +++ b/src/qemu/qemu_hotplug.c @@ -4415,6 +4415,9 @@ static int qemuComparePCIDevice(virDomainDefPtr def ATTRIBUTE_UNUSED, { virDomainDeviceInfoPtr info2 = opaque; + if (!info1->type) + return 0; + if (info1->type != VIR_DOMAIN_DEVICE_ADDRESS_TYPE_PCI || info2->type != VIR_DOMAIN_DEVICE_ADDRESS_TYPE_PCI) return 0; -- 2.21.0
On Mon, May 20, 2019 at 01:37:49PM +0200, Andrea Bolognani wrote:
The virDomainDeviceInfoIterate() function was initially written with the expectation that all devices would embed a virDomainDeviceInfo, and thus the user-provided callback would never be passed NULL; however, that doesn't really represent reality, as multiple devices don't have any virDomainDeviceInfo associated with them.
virDomainDeviceInfoIterate is specifically meant for iterating over device infos. Commit 88d24aaccc1e31ff1ce682f9496cf08cc7f7c216 : conf: domain: Introduce virDomainDeviceIterateFlags documented this function as iterating over devices (but did not implement this for every device) and then commit dd45c2710f6fd2d4f8a47f97960532d0e0091e7d conf: domain: gfx: Iterate over graphics devices when doing validation added one of them. Of course, there is a huge overlap between iterating over all devices and just those with an info, but since the callers do request *Info* I don't think they should expect it to be NULL. Jano
Since we still want to be able to iterate over those devices, clarify that callbacks are expected to be able to handle NULL being passed to them, and update all existing callbacks so that they actually do so.
Signed-off-by: Andrea Bolognani <abologna@redhat.com> --- src/bhyve/bhyve_device.c | 4 ++++ src/conf/domain_addr.c | 9 +++++++++ src/conf/domain_conf.c | 34 +++++++++++++++++++++++++++++++- src/libxl/libxl_driver.c | 3 +++ src/qemu/qemu_domain_address.c | 36 +++++++++++++++++++++++++++++++--- src/qemu/qemu_hotplug.c | 3 +++ 6 files changed, 85 insertions(+), 4 deletions(-)
On Mon, 2019-05-20 at 14:59 +0200, Ján Tomko wrote:
On Mon, May 20, 2019 at 01:37:49PM +0200, Andrea Bolognani wrote:
The virDomainDeviceInfoIterate() function was initially written with the expectation that all devices would embed a virDomainDeviceInfo, and thus the user-provided callback would never be passed NULL; however, that doesn't really represent reality, as multiple devices don't have any virDomainDeviceInfo associated with them.
virDomainDeviceInfoIterate is specifically meant for iterating over device infos.
Commit 88d24aaccc1e31ff1ce682f9496cf08cc7f7c216 : conf: domain: Introduce virDomainDeviceIterateFlags
documented this function as iterating over devices (but did not implement this for every device) and then
commit dd45c2710f6fd2d4f8a47f97960532d0e0091e7d conf: domain: gfx: Iterate over graphics devices when doing validation
added one of them.
Yeah, I'm aware of the history here, and the next patch partially reverts the second commit.
Of course, there is a huge overlap between iterating over all devices and just those with an info, but since the callers do request *Info* I don't think they should expect it to be NULL.
I realize that. I even toyed with the idea of renaming virDomainDeviceInfoIterate() to virDomainDeviceIterate() to get rid of the inconsistency, but I didn't feel like it would be worth the churn and though that documenting the expectations would be enough. I'd be fine renaming it, though. Personally I don't think adding a new virDomainDeviceIterateFlags for each virDomainDeviceInfo-less device class is a good solution, as it leaves the door open to wiring up a validate callback that looks like it would be called but actually isn't: this is what caused dd45c27, and what happened to me while I was moving the validation code for Intel IOMMU from qemu_command to domain_conf. -- Andrea Bolognani / Red Hat / Virtualization
On Mon, May 20, 2019 at 03:24:06PM +0200, Andrea Bolognani wrote:
On Mon, 2019-05-20 at 14:59 +0200, Ján Tomko wrote:
On Mon, May 20, 2019 at 01:37:49PM +0200, Andrea Bolognani wrote:
The virDomainDeviceInfoIterate() function was initially written with the expectation that all devices would embed a virDomainDeviceInfo, and thus the user-provided callback would never be passed NULL; however, that doesn't really represent reality, as multiple devices don't have any virDomainDeviceInfo associated with them.
virDomainDeviceInfoIterate is specifically meant for iterating over device infos.
Commit 88d24aaccc1e31ff1ce682f9496cf08cc7f7c216 : conf: domain: Introduce virDomainDeviceIterateFlags
documented this function as iterating over devices (but did not implement this for every device) and then
commit dd45c2710f6fd2d4f8a47f97960532d0e0091e7d conf: domain: gfx: Iterate over graphics devices when doing validation
added one of them.
Yeah, I'm aware of the history here, and the next patch partially reverts the second commit.
Of course, there is a huge overlap between iterating over all devices and just those with an info, but since the callers do request *Info* I don't think they should expect it to be NULL.
I realize that. I even toyed with the idea of renaming virDomainDeviceInfoIterate() to virDomainDeviceIterate() to get rid of the inconsistency, but I didn't feel like it would be worth the churn and though that documenting the expectations would be enough.
I think there would be less churn that way, see my proposal: Message-Id: <cover.1558448715.git.jtomko@redhat.com> https://www.redhat.com/archives/libvir-list/2019-May/msg00607.html
I'd be fine renaming it, though.
Personally I don't think adding a new virDomainDeviceIterateFlags for each virDomainDeviceInfo-less device class is a good solution,
Agreed.
as it leaves the door open to wiring up a validate callback that looks like it would be called but actually isn't: this is what caused dd45c27, and what happened to me while I was moving the validation code for Intel IOMMU from qemu_command to domain_conf.
Jano
On Tue, 2019-05-21 at 16:28 +0200, Ján Tomko wrote:
On Mon, May 20, 2019 at 03:24:06PM +0200, Andrea Bolognani wrote:
On Mon, 2019-05-20 at 14:59 +0200, Ján Tomko wrote:
Of course, there is a huge overlap between iterating over all devices and just those with an info, but since the callers do request *Info* I don't think they should expect it to be NULL.
I realize that. I even toyed with the idea of renaming virDomainDeviceInfoIterate() to virDomainDeviceIterate() to get rid of the inconsistency, but I didn't feel like it would be worth the churn and though that documenting the expectations would be enough.
I think there would be less churn that way, see my proposal: Message-Id: <cover.1558448715.git.jtomko@redhat.com> https://www.redhat.com/archives/libvir-list/2019-May/msg00607.html
Well of course you're avoiding most of the churn I was mentioning: you're adding a new function instead of renaming the existing one! That's cheating ;) I don't much like the idea of adding a separate function that does almost the same thing but not quite, because that will ultimately result in the same situation we have now: someone will add a new callback and (reasonably) expect it to be called for all devices, but that won't happen because the original code uses the DeviceInfo variant instead of the Device one. That's unnecessary friction. Not to mention that your new function also happens to iterate over all consoles, which the existing variant doesn't. That's an extra little inconsistency that we really don't need to introduce. So I still prefer my approach. As said earlier, I'm also not entirely convinced keeping the existing function name is a good idea, so I'll happily rename it at the same time as I'm updating and documenting its contract. Especially once that's done, I don't see any problem with passing the callback a pointer that might be NULL and expecting the user to check before using it, as that's par for the course when using the C language. -- Andrea Bolognani / Red Hat / Virtualization
On Wed, May 22, 2019 at 03:36:59PM +0200, Andrea Bolognani wrote:
On Tue, 2019-05-21 at 16:28 +0200, Ján Tomko wrote:
On Mon, May 20, 2019 at 03:24:06PM +0200, Andrea Bolognani wrote:
On Mon, 2019-05-20 at 14:59 +0200, Ján Tomko wrote:
Of course, there is a huge overlap between iterating over all devices and just those with an info, but since the callers do request *Info* I don't think they should expect it to be NULL.
I realize that. I even toyed with the idea of renaming virDomainDeviceInfoIterate() to virDomainDeviceIterate() to get rid of the inconsistency, but I didn't feel like it would be worth the churn and though that documenting the expectations would be enough.
I think there would be less churn that way, see my proposal: Message-Id: <cover.1558448715.git.jtomko@redhat.com> https://www.redhat.com/archives/libvir-list/2019-May/msg00607.html
Well of course you're avoiding most of the churn I was mentioning: you're adding a new function instead of renaming the existing one! That's cheating ;)
I don't much like the idea of adding a separate function that does almost the same thing but not quite, because that will ultimately result in the same situation we have now: someone will add a new callback and (reasonably) expect it to be called for all devices, but that won't happen because the original code uses the DeviceInfo variant instead of the Device one. That's unnecessary friction.
So is having to argue about not putting if (!info) into every single internal function that should not have been called with a NULL info in the first place. Jano
On Wed, 2019-05-22 at 16:12 +0200, Ján Tomko wrote:
On Wed, May 22, 2019 at 03:36:59PM +0200, Andrea Bolognani wrote:
I don't much like the idea of adding a separate function that does almost the same thing but not quite, because that will ultimately result in the same situation we have now: someone will add a new callback and (reasonably) expect it to be called for all devices, but that won't happen because the original code uses the DeviceInfo variant instead of the Device one. That's unnecessary friction.
So is having to argue about not putting if (!info) into every single internal function that should not have been called with a NULL info in the first place.
Well, it's pretty clear at this point that we're not likely to ever reach an agreement. Let's just go with (some variation of) your version and then move on to something more productive. -- Andrea Bolognani / Red Hat / Virtualization
This was a hack needed because virDomainGraphicsDef doesn't embed a virDomainDeviceInfo, but now that we have clarified the corresponding pointer can be NULL we no longer need to special-case graphics and can drop the flag. This commit is best viewed with 'git show -w'. Signed-off-by: Andrea Bolognani <abologna@redhat.com> --- src/conf/domain_conf.c | 18 ++++++------------ 1 file changed, 6 insertions(+), 12 deletions(-) diff --git a/src/conf/domain_conf.c b/src/conf/domain_conf.c index f42d331341..16820424be 100644 --- a/src/conf/domain_conf.c +++ b/src/conf/domain_conf.c @@ -4080,7 +4080,6 @@ virDomainSkipBackcompatConsole(virDomainDefPtr def, enum { DOMAIN_DEVICE_ITERATE_ALL_CONSOLES = 1 << 0, - DOMAIN_DEVICE_ITERATE_GRAPHICS = 1 << 1 } virDomainDeviceIterateFlags; /* @@ -4246,15 +4245,11 @@ virDomainDeviceInfoIterateInternal(virDomainDefPtr def, return rc; } - /* If the flag below is set, make sure @cb can handle @info being NULL, as - * graphics don't have any boot info */ - if (iteratorFlags & DOMAIN_DEVICE_ITERATE_GRAPHICS) { - device.type = VIR_DOMAIN_DEVICE_GRAPHICS; - for (i = 0; i < def->ngraphics; i++) { - device.data.graphics = def->graphics[i]; - if ((rc = cb(def, &device, NULL, opaque)) != 0) - return rc; - } + device.type = VIR_DOMAIN_DEVICE_GRAPHICS; + for (i = 0; i < def->ngraphics; i++) { + device.data.graphics = def->graphics[i]; + if ((rc = cb(def, &device, NULL, opaque)) != 0) + return rc; } /* Coverity is not very happy with this - all dead_error_condition */ @@ -6951,8 +6946,7 @@ virDomainDefValidate(virDomainDefPtr def, /* iterate the devices */ if (virDomainDeviceInfoIterateInternal(def, virDomainDefValidateDeviceIterator, - (DOMAIN_DEVICE_ITERATE_ALL_CONSOLES | - DOMAIN_DEVICE_ITERATE_GRAPHICS), + DOMAIN_DEVICE_ITERATE_ALL_CONSOLES, &data) < 0) return -1; -- 2.21.0
On Mon, May 20, 2019 at 01:37:50PM +0200, Andrea Bolognani wrote:
This was a hack needed because virDomainGraphicsDef doesn't embed a virDomainDeviceInfo, but now that we have clarified the corresponding pointer can be NULL we no longer need to special-case graphics and can drop the flag.
This commit is best viewed with 'git show -w'.
Signed-off-by: Andrea Bolognani <abologna@redhat.com> ---
If the previous patch is accepted: Reviewed-by: Erik Skultety <eskultet@redhat.com>
Now that we can safely iterate over devices that don't have an associated virDomainDeviceInfo, supporting IOMMU devices is no longer a problem. Signed-off-by: Andrea Bolognani <abologna@redhat.com> --- src/conf/domain_conf.c | 7 +++++++ 1 file changed, 7 insertions(+) diff --git a/src/conf/domain_conf.c b/src/conf/domain_conf.c index 16820424be..eda424dbef 100644 --- a/src/conf/domain_conf.c +++ b/src/conf/domain_conf.c @@ -4252,6 +4252,13 @@ virDomainDeviceInfoIterateInternal(virDomainDefPtr def, return rc; } + if (def->iommu) { + device.type = VIR_DOMAIN_DEVICE_IOMMU; + device.data.iommu = def->iommu; + if ((rc = cb(def, &device, NULL, opaque)) != 0) + return rc; + } + /* Coverity is not very happy with this - all dead_error_condition */ #if !STATIC_ANALYSIS /* This switch statement is here to trigger compiler warning when adding -- 2.21.0
On Mon, May 20, 2019 at 01:37:51PM +0200, Andrea Bolognani wrote:
Now that we can safely iterate over devices that don't have an associated virDomainDeviceInfo, supporting IOMMU devices is no longer a problem.
Actually, not having a DeviceInfo for the IOMMU device might be a bug here. I omitted it because there's no address to be specified, but there might be a use for an alias here. Jano
Signed-off-by: Andrea Bolognani <abologna@redhat.com> --- src/conf/domain_conf.c | 7 +++++++ 1 file changed, 7 insertions(+)
diff --git a/src/conf/domain_conf.c b/src/conf/domain_conf.c index 16820424be..eda424dbef 100644 --- a/src/conf/domain_conf.c +++ b/src/conf/domain_conf.c @@ -4252,6 +4252,13 @@ virDomainDeviceInfoIterateInternal(virDomainDefPtr def, return rc; }
+ if (def->iommu) { + device.type = VIR_DOMAIN_DEVICE_IOMMU; + device.data.iommu = def->iommu; + if ((rc = cb(def, &device, NULL, opaque)) != 0) + return rc; + } + /* Coverity is not very happy with this - all dead_error_condition */ #if !STATIC_ANALYSIS /* This switch statement is here to trigger compiler warning when adding -- 2.21.0
-- libvir-list mailing list libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list
On Mon, 2019-05-20 at 15:03 +0200, Ján Tomko wrote:
On Mon, May 20, 2019 at 01:37:51PM +0200, Andrea Bolognani wrote:
Now that we can safely iterate over devices that don't have an associated virDomainDeviceInfo, supporting IOMMU devices is no longer a problem.
Actually, not having a DeviceInfo for the IOMMU device might be a bug here. I omitted it because there's no address to be specified, but there might be a use for an alias here.
What would be the point of having an alias for IOMMU devices, when there can only be one per guest? -- Andrea Bolognani / Red Hat / Virtualization
Device validation should not have to wait until command line generation time. Moving the code to a separate function also allows us to avoid some unnecessary repetition. Signed-off-by: Andrea Bolognani <abologna@redhat.com> --- src/qemu/qemu_command.c | 56 ---------------------------------- src/qemu/qemu_domain.c | 66 +++++++++++++++++++++++++++++++++++++++- tests/qemuxml2argvtest.c | 2 +- 3 files changed, 66 insertions(+), 58 deletions(-) diff --git a/src/qemu/qemu_command.c b/src/qemu/qemu_command.c index aae2f43044..e16c748d31 100644 --- a/src/qemu/qemu_command.c +++ b/src/qemu/qemu_command.c @@ -6935,60 +6935,11 @@ qemuBuildIOMMUCommandLine(virCommandPtr cmd, if (!iommu) return 0; - switch (iommu->model) { - case VIR_DOMAIN_IOMMU_MODEL_INTEL: - if (iommu->intremap != VIR_TRISTATE_SWITCH_ABSENT && - !virQEMUCapsGet(qemuCaps, QEMU_CAPS_INTEL_IOMMU_INTREMAP)) { - virReportError(VIR_ERR_CONFIG_UNSUPPORTED, "%s", - _("iommu: interrupt remapping is not supported " - "with this QEMU binary")); - return -1; - } - if (iommu->caching_mode != VIR_TRISTATE_SWITCH_ABSENT && - !virQEMUCapsGet(qemuCaps, QEMU_CAPS_INTEL_IOMMU_CACHING_MODE)) { - virReportError(VIR_ERR_CONFIG_UNSUPPORTED, "%s", - _("iommu: caching mode is not supported " - "with this QEMU binary")); - return -1; - } - if (iommu->eim != VIR_TRISTATE_SWITCH_ABSENT && - !virQEMUCapsGet(qemuCaps, QEMU_CAPS_INTEL_IOMMU_EIM)) { - virReportError(VIR_ERR_CONFIG_UNSUPPORTED, "%s", - _("iommu: eim is not supported " - "with this QEMU binary")); - return -1; - } - if (iommu->iotlb != VIR_TRISTATE_SWITCH_ABSENT && - !virQEMUCapsGet(qemuCaps, QEMU_CAPS_INTEL_IOMMU_DEVICE_IOTLB)) { - virReportError(VIR_ERR_CONFIG_UNSUPPORTED, "%s", - _("iommu: device IOTLB is not supported " - "with this QEMU binary")); - return -1; - } - break; - case VIR_DOMAIN_IOMMU_MODEL_LAST: - break; - } - if (virQEMUCapsGet(qemuCaps, QEMU_CAPS_MACHINE_IOMMU)) return 0; /* Already handled via -machine */ switch (iommu->model) { case VIR_DOMAIN_IOMMU_MODEL_INTEL: - if (!virQEMUCapsGet(qemuCaps, QEMU_CAPS_DEVICE_INTEL_IOMMU)) { - virReportError(VIR_ERR_CONFIG_UNSUPPORTED, - _("IOMMU device: '%s' is not supported with " - "this QEMU binary"), - virDomainIOMMUModelTypeToString(iommu->model)); - return -1; - } - if (!qemuDomainIsQ35(def)) { - virReportError(VIR_ERR_CONFIG_UNSUPPORTED, - _("IOMMU device: '%s' is only supported with " - "Q35 machines"), - virDomainIOMMUModelTypeToString(iommu->model)); - return -1; - } virBufferAddLit(&opts, "intel-iommu"); if (iommu->intremap != VIR_TRISTATE_SWITCH_ABSENT) { virBufferAsprintf(&opts, ",intremap=%s", @@ -7648,13 +7599,6 @@ qemuBuildMachineCommandLine(virCommandPtr cmd, virQEMUCapsGet(qemuCaps, QEMU_CAPS_MACHINE_IOMMU)) { switch (def->iommu->model) { case VIR_DOMAIN_IOMMU_MODEL_INTEL: - if (!qemuDomainIsQ35(def)) { - virReportError(VIR_ERR_CONFIG_UNSUPPORTED, - _("IOMMU device: '%s' is only supported with " - "Q35 machines"), - virDomainIOMMUModelTypeToString(def->iommu->model)); - return -1; - } virBufferAddLit(&buf, ",iommu=on"); break; case VIR_DOMAIN_IOMMU_MODEL_LAST: diff --git a/src/qemu/qemu_domain.c b/src/qemu/qemu_domain.c index 3290c5d490..e50e84a3b2 100644 --- a/src/qemu/qemu_domain.c +++ b/src/qemu/qemu_domain.c @@ -6083,6 +6083,67 @@ qemuDomainDeviceDefValidateMemballoon(const virDomainMemballoonDef *memballoon, } +static int +qemuDomainDeviceDefValidateIOMMU(const virDomainIOMMUDef *iommu, + const virDomainDef *def, + virQEMUCapsPtr qemuCaps) +{ + switch (iommu->model) { + case VIR_DOMAIN_IOMMU_MODEL_INTEL: + if (!qemuDomainIsQ35(def)) { + virReportError(VIR_ERR_CONFIG_UNSUPPORTED, + _("IOMMU device: '%s' is only supported with " + "Q35 machines"), + virDomainIOMMUModelTypeToString(iommu->model)); + return -1; + } + if (!virQEMUCapsGet(qemuCaps, QEMU_CAPS_DEVICE_INTEL_IOMMU) && + !virQEMUCapsGet(qemuCaps, QEMU_CAPS_MACHINE_IOMMU)) { + virReportError(VIR_ERR_CONFIG_UNSUPPORTED, + _("IOMMU device: '%s' is not supported with " + "this QEMU binary"), + virDomainIOMMUModelTypeToString(iommu->model)); + return -1; + } + if (iommu->intremap != VIR_TRISTATE_SWITCH_ABSENT && + !virQEMUCapsGet(qemuCaps, QEMU_CAPS_INTEL_IOMMU_INTREMAP)) { + virReportError(VIR_ERR_CONFIG_UNSUPPORTED, "%s", + _("iommu: interrupt remapping is not supported " + "with this QEMU binary")); + return -1; + } + if (iommu->caching_mode != VIR_TRISTATE_SWITCH_ABSENT && + !virQEMUCapsGet(qemuCaps, QEMU_CAPS_INTEL_IOMMU_CACHING_MODE)) { + virReportError(VIR_ERR_CONFIG_UNSUPPORTED, "%s", + _("iommu: caching mode is not supported " + "with this QEMU binary")); + return -1; + } + if (iommu->eim != VIR_TRISTATE_SWITCH_ABSENT && + !virQEMUCapsGet(qemuCaps, QEMU_CAPS_INTEL_IOMMU_EIM)) { + virReportError(VIR_ERR_CONFIG_UNSUPPORTED, "%s", + _("iommu: eim is not supported " + "with this QEMU binary")); + return -1; + } + if (iommu->iotlb != VIR_TRISTATE_SWITCH_ABSENT && + !virQEMUCapsGet(qemuCaps, QEMU_CAPS_INTEL_IOMMU_DEVICE_IOTLB)) { + virReportError(VIR_ERR_CONFIG_UNSUPPORTED, "%s", + _("iommu: device IOTLB is not supported " + "with this QEMU binary")); + return -1; + } + break; + + case VIR_DOMAIN_IOMMU_MODEL_LAST: + default: + virReportEnumRangeError(virDomainIOMMUModel, iommu->model); + } + + return 0; +} + + static int qemuDomainDeviceDefValidateZPCIAddress(virDomainDeviceInfoPtr info, virQEMUCapsPtr qemuCaps) @@ -6195,6 +6256,10 @@ qemuDomainDeviceDefValidate(const virDomainDeviceDef *dev, ret = qemuDomainDeviceDefValidateMemballoon(dev->data.memballoon, qemuCaps); break; + case VIR_DOMAIN_DEVICE_IOMMU: + ret = qemuDomainDeviceDefValidateIOMMU(dev->data.iommu, def, qemuCaps); + break; + case VIR_DOMAIN_DEVICE_LEASE: case VIR_DOMAIN_DEVICE_FS: case VIR_DOMAIN_DEVICE_SOUND: @@ -6203,7 +6268,6 @@ qemuDomainDeviceDefValidate(const virDomainDeviceDef *dev, case VIR_DOMAIN_DEVICE_SHMEM: case VIR_DOMAIN_DEVICE_MEMORY: case VIR_DOMAIN_DEVICE_PANIC: - case VIR_DOMAIN_DEVICE_IOMMU: case VIR_DOMAIN_DEVICE_NONE: case VIR_DOMAIN_DEVICE_LAST: break; diff --git a/tests/qemuxml2argvtest.c b/tests/qemuxml2argvtest.c index 0e83acac86..55db5f6fca 100644 --- a/tests/qemuxml2argvtest.c +++ b/tests/qemuxml2argvtest.c @@ -2812,7 +2812,7 @@ mymain(void) DO_TEST_CAPS_LATEST("intel-iommu-caching-mode"); DO_TEST_CAPS_LATEST("intel-iommu-eim"); DO_TEST_CAPS_LATEST("intel-iommu-device-iotlb"); - DO_TEST_FAILURE("intel-iommu-wrong-machine", NONE); + DO_TEST_PARSE_ERROR("intel-iommu-wrong-machine", NONE); DO_TEST("cpu-hotplug-startup", QEMU_CAPS_QUERY_HOTPLUGGABLE_CPUS); DO_TEST_PARSE_ERROR("cpu-hotplug-granularity", -- 2.21.0
On Mon, May 20, 2019 at 01:37:52PM +0200, Andrea Bolognani wrote:
Device validation should not have to wait until command line generation time. Moving the code to a separate function also allows us to avoid some unnecessary repetition.
Signed-off-by: Andrea Bolognani <abologna@redhat.com> --- src/qemu/qemu_command.c | 56 ---------------------------------- src/qemu/qemu_domain.c | 66 +++++++++++++++++++++++++++++++++++++++- tests/qemuxml2argvtest.c | 2 +- 3 files changed, 66 insertions(+), 58 deletions(-)
Reviewed-by: Ján Tomko <jtomko@redhat.com> Jano
Mostly add comments explaining why there are two capabilites for the same feature and how they interact. Signed-off-by: Andrea Bolognani <abologna@redhat.com> --- src/qemu/qemu_command.c | 13 ++++++++++--- 1 file changed, 10 insertions(+), 3 deletions(-) diff --git a/src/qemu/qemu_command.c b/src/qemu/qemu_command.c index e16c748d31..73f570c419 100644 --- a/src/qemu/qemu_command.c +++ b/src/qemu/qemu_command.c @@ -6935,8 +6935,12 @@ qemuBuildIOMMUCommandLine(virCommandPtr cmd, if (!iommu) return 0; - if (virQEMUCapsGet(qemuCaps, QEMU_CAPS_MACHINE_IOMMU)) - return 0; /* Already handled via -machine */ + /* qemuDomainDeviceDefValidate() already made sure we have one of + * QEMU_CAPS_DEVICE_INTEL_IOMMU or QEMU_CAPS_MACHINE_IOMMU: here we + * handle the former case, while the latter is taken care of in + * qemuBuildMachineCommandLine() */ + if (!virQEMUCapsGet(qemuCaps, QEMU_CAPS_DEVICE_INTEL_IOMMU)) + return 0; switch (iommu->model) { case VIR_DOMAIN_IOMMU_MODEL_INTEL: @@ -7594,7 +7598,10 @@ qemuBuildMachineCommandLine(virCommandPtr cmd, } } - /* We don't report errors on missing cap here - -device code will do that */ + /* qemuDomainDeviceDefValidate() already made sure we have one of + * QEMU_CAPS_DEVICE_INTEL_IOMMU or QEMU_CAPS_MACHINE_IOMMU: here we + * handle the latter case, while the former is taken care of in + * qemuBuildIOMMUCommandLine() */ if (def->iommu && virQEMUCapsGet(qemuCaps, QEMU_CAPS_MACHINE_IOMMU)) { switch (def->iommu->model) { -- 2.21.0
On Mon, May 20, 2019 at 01:37:53PM +0200, Andrea Bolognani wrote:
Mostly add comments explaining why there are two capabilites for the same feature and how they interact.
Signed-off-by: Andrea Bolognani <abologna@redhat.com> --- src/qemu/qemu_command.c | 13 ++++++++++--- 1 file changed, 10 insertions(+), 3 deletions(-)
Reviewed-by: Ján Tomko <jtomko@redhat.com> Jano
participants (4)
-
Andrea Bolognani -
Erik Skultety -
Ján Tomko -
Peter Krempa