[libvirt] [PATCH] conf: Ignore panic device on pSeries.
The guest firmware already provides the same functionality, so we can just safely drop the <panic/> element from the domain definition. Resolves: https://bugzilla.redhat.com/show_bug.cgi?id=1182388 --- src/conf/domain_conf.c | 17 +++++++----- .../qemuxml2argv-pseries-panic.args | 7 +++++ .../qemuxml2argv-pseries-panic.xml | 30 ++++++++++++++++++++++ tests/qemuxml2argvtest.c | 2 ++ .../qemuxml2xmlout-pseries-panic.xml | 29 +++++++++++++++++++++ tests/qemuxml2xmltest.c | 1 + 6 files changed, 80 insertions(+), 6 deletions(-) create mode 100644 tests/qemuxml2argvdata/qemuxml2argv-pseries-panic.args create mode 100644 tests/qemuxml2argvdata/qemuxml2argv-pseries-panic.xml create mode 100644 tests/qemuxml2xmloutdata/qemuxml2xmlout-pseries-panic.xml diff --git a/src/conf/domain_conf.c b/src/conf/domain_conf.c index 4cd36a1..a7d4efa 100644 --- a/src/conf/domain_conf.c +++ b/src/conf/domain_conf.c @@ -15478,13 +15478,18 @@ virDomainDefParseXML(xmlDocPtr xml, goto error; } if (n > 0) { - virDomainPanicDefPtr panic = - virDomainPanicDefParseXML(nodes[0]); - if (!panic) - goto error; + /* Ignore the panic device on pSeries, as the guest + * firmware already provides the same functionality */ + if (!(ARCH_IS_PPC64(def->os.arch) && + STRPREFIX(def->os.machine, "pseries"))) { + virDomainPanicDefPtr panic = + virDomainPanicDefParseXML(nodes[0]); + if (!panic) + goto error; - def->panic = panic; - VIR_FREE(nodes); + def->panic = panic; + VIR_FREE(nodes); + } } /* analysis of the shmem devices */ diff --git a/tests/qemuxml2argvdata/qemuxml2argv-pseries-panic.args b/tests/qemuxml2argvdata/qemuxml2argv-pseries-panic.args new file mode 100644 index 0000000..30e4b43 --- /dev/null +++ b/tests/qemuxml2argvdata/qemuxml2argv-pseries-panic.args @@ -0,0 +1,7 @@ +LC_ALL=C PATH=/bin HOME=/home/test USER=test LOGNAME=test QEMU_AUDIO_DRV=none \ +/usr/bin/qemu-system-ppc64 -S -M pseries -m 512 -smp 1 -nographic \ +-nodefconfig -nodefaults \ +-chardev socket,id=charmonitor,path=/tmp/test-monitor,server,nowait \ +-mon chardev=charmonitor,id=monitor,mode=readline -no-acpi -boot c -usb \ +-chardev pty,id=charserial0 \ +-device spapr-vty,chardev=charserial0,reg=0x30000000 diff --git a/tests/qemuxml2argvdata/qemuxml2argv-pseries-panic.xml b/tests/qemuxml2argvdata/qemuxml2argv-pseries-panic.xml new file mode 100644 index 0000000..9312975 --- /dev/null +++ b/tests/qemuxml2argvdata/qemuxml2argv-pseries-panic.xml @@ -0,0 +1,30 @@ +<domain type='qemu'> + <name>QEMUGuest1</name> + <uuid>1ccfd97d-5eb4-478a-bbe6-88d254c16db7</uuid> + <memory unit='KiB'>524288</memory> + <currentMemory unit='KiB'>524288</currentMemory> + <vcpu placement='static'>1</vcpu> + <os> + <type arch='ppc64' machine='pseries'>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-ppc64</emulator> + <controller type='usb' index='0'/> + <controller type='pci' index='0' model='pci-root'/> + <serial type='pty'> + <target port='0'/> + <address type='spapr-vio'/> + </serial> + <console type='pty'> + <target type='serial' port='0'/> + <address type='spapr-vio'/> + </console> + <memballoon model='none'/> + <panic/> + </devices> +</domain> diff --git a/tests/qemuxml2argvtest.c b/tests/qemuxml2argvtest.c index 97c7fba..5719d70 100644 --- a/tests/qemuxml2argvtest.c +++ b/tests/qemuxml2argvtest.c @@ -1360,6 +1360,8 @@ mymain(void) QEMU_CAPS_CHARDEV, QEMU_CAPS_DEVICE, QEMU_CAPS_NODEFCONFIG); DO_TEST("pseries-cpu-le", QEMU_CAPS_KVM, QEMU_CAPS_CPU_HOST, QEMU_CAPS_CHARDEV, QEMU_CAPS_DEVICE, QEMU_CAPS_NODEFCONFIG); + DO_TEST("pseries-panic", + QEMU_CAPS_CHARDEV, QEMU_CAPS_DEVICE, QEMU_CAPS_NODEFCONFIG); DO_TEST("disk-ide-drive-split", QEMU_CAPS_DRIVE, QEMU_CAPS_DEVICE, QEMU_CAPS_NODEFCONFIG, QEMU_CAPS_IDE_CD); diff --git a/tests/qemuxml2xmloutdata/qemuxml2xmlout-pseries-panic.xml b/tests/qemuxml2xmloutdata/qemuxml2xmlout-pseries-panic.xml new file mode 100644 index 0000000..8980847 --- /dev/null +++ b/tests/qemuxml2xmloutdata/qemuxml2xmlout-pseries-panic.xml @@ -0,0 +1,29 @@ +<domain type='qemu'> + <name>QEMUGuest1</name> + <uuid>1ccfd97d-5eb4-478a-bbe6-88d254c16db7</uuid> + <memory unit='KiB'>524288</memory> + <currentMemory unit='KiB'>524288</currentMemory> + <vcpu placement='static'>1</vcpu> + <os> + <type arch='ppc64' machine='pseries'>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-ppc64</emulator> + <controller type='usb' index='0'/> + <controller type='pci' index='0' model='pci-root'/> + <serial type='pty'> + <target port='0'/> + <address type='spapr-vio'/> + </serial> + <console type='pty'> + <target type='serial' port='0'/> + <address type='spapr-vio'/> + </console> + <memballoon model='none'/> + </devices> +</domain> diff --git a/tests/qemuxml2xmltest.c b/tests/qemuxml2xmltest.c index b611afd..5db6341 100644 --- a/tests/qemuxml2xmltest.c +++ b/tests/qemuxml2xmltest.c @@ -534,6 +534,7 @@ mymain(void) DO_TEST("virtio-rng-egd"); DO_TEST("pseries-nvram"); + DO_TEST_DIFFERENT("pseries-panic"); /* These tests generate different XML */ DO_TEST_DIFFERENT("balloon-device-auto"); -- 2.1.0
On Thu, May 07, 2015 at 06:40:52PM +0200, Andrea Bolognani wrote:
The guest firmware already provides the same functionality, so we can just safely drop the <panic/> element from the domain definition. Resolves: https://bugzilla.redhat.com/show_bug.cgi?id=1182388 --- src/conf/domain_conf.c | 17 +++++++----- .../qemuxml2argv-pseries-panic.args | 7 +++++ .../qemuxml2argv-pseries-panic.xml | 30 ++++++++++++++++++++++ tests/qemuxml2argvtest.c | 2 ++ .../qemuxml2xmlout-pseries-panic.xml | 29 +++++++++++++++++++++ tests/qemuxml2xmltest.c | 1 + 6 files changed, 80 insertions(+), 6 deletions(-) create mode 100644 tests/qemuxml2argvdata/qemuxml2argv-pseries-panic.args create mode 100644 tests/qemuxml2argvdata/qemuxml2argv-pseries-panic.xml create mode 100644 tests/qemuxml2xmloutdata/qemuxml2xmlout-pseries-panic.xml
diff --git a/src/conf/domain_conf.c b/src/conf/domain_conf.c index 4cd36a1..a7d4efa 100644 --- a/src/conf/domain_conf.c +++ b/src/conf/domain_conf.c @@ -15478,13 +15478,18 @@ virDomainDefParseXML(xmlDocPtr xml, goto error; } if (n > 0) { - virDomainPanicDefPtr panic = - virDomainPanicDefParseXML(nodes[0]); - if (!panic) - goto error; + /* Ignore the panic device on pSeries, as the guest + * firmware already provides the same functionality */ + if (!(ARCH_IS_PPC64(def->os.arch) && + STRPREFIX(def->os.machine, "pseries"))) { + virDomainPanicDefPtr panic = + virDomainPanicDefParseXML(nodes[0]); + if (!panic) + goto error;
- def->panic = panic; - VIR_FREE(nodes); + def->panic = panic; + VIR_FREE(nodes); + }
No, this is totally wrong thing todo. Any fix should be in the QEMU code, not the XML parser. Regards, Daniel -- |: http://berrange.com -o- http://www.flickr.com/photos/dberrange/ :| |: http://libvirt.org -o- http://virt-manager.org :| |: http://autobuild.org -o- http://search.cpan.org/~danberr/ :| |: http://entangle-photo.org -o- http://live.gnome.org/gtk-vnc :|
On 05/07/2015 12:40 PM, Andrea Bolognani wrote:
The guest firmware already provides the same functionality, so we can just safely drop the <panic/> element from the domain definition.
Resolves: https://bugzilla.redhat.com/show_bug.cgi?id=1182388
<devices><panic> is about a specific device though, which qemu ppc doesn't support. Even if the firmware provides the logical feature (getting PANICKED notifications from qemu), it doesn't support the device or any explicit qemu CLI config, so IMO it's correct to reject <panic>. Apps/users will just have to take that into account, along with all the other XML differences for x86 vs ppc64. An alternative could be the unconditionally add <panic> to ppc64 XML since the logical feature is always available... but you'd probably have to invent a new <address> scheme or something Instead I think it's just a documentation patch. Another thing from that bug: The error message 'your QEMU is too old to support pvpanic' isn't accurate for non-x86, so it's probably better to change it to 'this QEMU binary does not support pvpanic' or similar, maybe look at existing error messages to find a better example. Another comment below
--- src/conf/domain_conf.c | 17 +++++++----- .../qemuxml2argv-pseries-panic.args | 7 +++++ .../qemuxml2argv-pseries-panic.xml | 30 ++++++++++++++++++++++ tests/qemuxml2argvtest.c | 2 ++ .../qemuxml2xmlout-pseries-panic.xml | 29 +++++++++++++++++++++ tests/qemuxml2xmltest.c | 1 + 6 files changed, 80 insertions(+), 6 deletions(-) create mode 100644 tests/qemuxml2argvdata/qemuxml2argv-pseries-panic.args create mode 100644 tests/qemuxml2argvdata/qemuxml2argv-pseries-panic.xml create mode 100644 tests/qemuxml2xmloutdata/qemuxml2xmlout-pseries-panic.xml
diff --git a/src/conf/domain_conf.c b/src/conf/domain_conf.c index 4cd36a1..a7d4efa 100644 --- a/src/conf/domain_conf.c +++ b/src/conf/domain_conf.c @@ -15478,13 +15478,18 @@ virDomainDefParseXML(xmlDocPtr xml, goto error; } if (n > 0) { - virDomainPanicDefPtr panic = - virDomainPanicDefParseXML(nodes[0]); - if (!panic) - goto error; + /* Ignore the panic device on pSeries, as the guest + * firmware already provides the same functionality */ + if (!(ARCH_IS_PPC64(def->os.arch) && + STRPREFIX(def->os.machine, "pseries"))) { + virDomainPanicDefPtr panic = + virDomainPanicDefParseXML(nodes[0]); + if (!panic) + goto error;
- def->panic = panic; - VIR_FREE(nodes); + def->panic = panic; + VIR_FREE(nodes); + } }
FWIW, since this is hypervisor specific, this type of stuff shouldn't be in the (intended to be) generic domain_conf.c. Check out qemu_domain.c:qemuDomainDefPostParse instead - Cole
On Sun, 2015-05-10 at 17:50 -0400, Cole Robinson wrote:
<devices><panic> is about a specific device though, which qemu ppc doesn't support. Even if the firmware provides the logical feature (getting PANICKED notifications from qemu), it doesn't support the device or any explicit qemu CLI config, so IMO it's correct to reject <panic>. Apps/users will just have to take that into account, along with all the other XML differences for x86 vs ppc64.
An alternative could be the unconditionally add <panic> to ppc64 XML since the logical feature is always available... but you'd probably have to invent a new <address> scheme or something
Instead I think it's just a documentation patch.
I really like the alternative approach you suggested: it's the same feature after all, so the XML should be the same regardless of how it's actually implemented for the specific machine type. I've reworked my patch accordingly, I'm going to send a v2 in a few minutes. Please let me know what you think of it.
Another thing from that bug: The error message 'your QEMU is too old to support pvpanic' isn't accurate for non-x86, so it's probably better to change it to 'this QEMU binary does not support pvpanic' or similar, maybe look at existing error messages to find a better example.
I agree. I feel it belongs to a different commit, though.
FWIW, since this is hypervisor specific, this type of stuff shouldn't be in the (intended to be) generic domain_conf.c. Check out qemu_domain.c:qemuDomainDefPostParse instead
My mistake. Thanks both to you and Daniel for pointing that out. Cheers. -- Andrea Bolognani <abologna@redhat.com>
The guest firmware provides the same functionality as the pvpanic device, which is not available in QEMU on pSeries: make sure the XML reflects this fact by automatically adding a <panic/> element when not already present. On the other hand, unlike the pvpanic device, the guest firmware can't be configured, so report an error if an address has been provided in the XML. Resolves: https://bugzilla.redhat.com/show_bug.cgi?id=1182388 --- src/qemu/qemu_command.c | 25 ++++++++++++----- src/qemu/qemu_domain.c | 14 ++++++++++ .../qemuxml2argvdata/qemuxml2argv-pseries-disk.xml | 1 + .../qemuxml2argv-pseries-nvram.xml | 1 + .../qemuxml2argv-pseries-panic-address.xml | 32 ++++++++++++++++++++++ .../qemuxml2argv-pseries-panic-missing.args | 7 +++++ .../qemuxml2argv-pseries-panic-missing.xml | 29 ++++++++++++++++++++ .../qemuxml2argv-pseries-panic-no-address.args | 7 +++++ .../qemuxml2argv-pseries-panic-no-address.xml | 30 ++++++++++++++++++++ tests/qemuxml2argvtest.c | 6 ++++ .../qemuxml2xmlout-pseries-panic-missing.xml | 30 ++++++++++++++++++++ tests/qemuxml2xmltest.c | 2 ++ 12 files changed, 177 insertions(+), 7 deletions(-) create mode 100644 tests/qemuxml2argvdata/qemuxml2argv-pseries-panic-address.xml create mode 100644 tests/qemuxml2argvdata/qemuxml2argv-pseries-panic-missing.args create mode 100644 tests/qemuxml2argvdata/qemuxml2argv-pseries-panic-missing.xml create mode 100644 tests/qemuxml2argvdata/qemuxml2argv-pseries-panic-no-address.args create mode 100644 tests/qemuxml2argvdata/qemuxml2argv-pseries-panic-no-address.xml create mode 100644 tests/qemuxml2xmloutdata/qemuxml2xmlout-pseries-panic-missing.xml diff --git a/src/qemu/qemu_command.c b/src/qemu/qemu_command.c index 5d0a167..138a8b6 100644 --- a/src/qemu/qemu_command.c +++ b/src/qemu/qemu_command.c @@ -10737,13 +10737,28 @@ qemuBuildCommandLine(virConnectPtr conn, } if (def->panic) { - if (virQEMUCapsGet(qemuCaps, QEMU_CAPS_DEVICE_PANIC)) { + if (ARCH_IS_PPC64(def->os.arch) && STRPREFIX(def->os.machine, "pseries")) { + /* For pSeries guests, the firmware provides the same + * functionality of the pvpanic device. The address + * cannot be configured by the user */ + if (def->panic->info.type != VIR_DOMAIN_DEVICE_ADDRESS_TYPE_NONE) { + virReportError(VIR_ERR_CONFIG_UNSUPPORTED, "%s", + _("setting the panic device address is not " + "supported for pSeries guests")); + goto error; + } + } else { + if (!virQEMUCapsGet(qemuCaps, QEMU_CAPS_DEVICE_PANIC)) { + virReportError(VIR_ERR_CONFIG_UNSUPPORTED, "%s", + _("your QEMU is too old to support pvpanic")); + goto error; + } + if (def->panic->info.type == VIR_DOMAIN_DEVICE_ADDRESS_TYPE_ISA) { virCommandAddArg(cmd, "-device"); virCommandAddArgFormat(cmd, "pvpanic,ioport=%d", def->panic->info.addr.isa.iobase); - } else if (def->panic->info.type == - VIR_DOMAIN_DEVICE_ADDRESS_TYPE_NONE) { + } else if (def->panic->info.type == VIR_DOMAIN_DEVICE_ADDRESS_TYPE_NONE) { virCommandAddArgList(cmd, "-device", "pvpanic", NULL); } else { virReportError(VIR_ERR_CONFIG_UNSUPPORTED, "%s", @@ -10751,10 +10766,6 @@ qemuBuildCommandLine(virConnectPtr conn, "with ISA address type")); goto error; } - } else { - virReportError(VIR_ERR_CONFIG_UNSUPPORTED, "%s", - _("your QEMU is too old to support pvpanic")); - goto error; } } diff --git a/src/qemu/qemu_domain.c b/src/qemu/qemu_domain.c index fa8229f..557b0b6 100644 --- a/src/qemu/qemu_domain.c +++ b/src/qemu/qemu_domain.c @@ -911,6 +911,7 @@ qemuDomainDefPostParse(virDomainDefPtr def, bool addDefaultMemballoon = true; bool addDefaultUSBKBD = false; bool addDefaultUSBMouse = false; + bool addPanicDevice = false; if (def->os.bootloader || def->os.bootloaderArgs) { virReportError(VIR_ERR_CONFIG_UNSUPPORTED, "%s", @@ -963,6 +964,11 @@ qemuDomainDefPostParse(virDomainDefPtr def, addPCIRoot = true; addDefaultUSBKBD = true; addDefaultUSBMouse = true; + /* For pSeries guests, the firmware provides the same + * functionality of the pvpanic device, so automatically + * add the definition if not already present */ + if (STRPREFIX(def->os.machine, "pseries")) + addPanicDevice = true; break; case VIR_ARCH_ALPHA: @@ -1045,6 +1051,14 @@ qemuDomainDefPostParse(virDomainDefPtr def, VIR_DOMAIN_INPUT_BUS_USB) < 0) return -1; + if (addPanicDevice && !def->panic) { + virDomainPanicDefPtr panic; + if (VIR_ALLOC(panic) < 0) + return -1; + + def->panic = panic; + } + return 0; } diff --git a/tests/qemuxml2argvdata/qemuxml2argv-pseries-disk.xml b/tests/qemuxml2argvdata/qemuxml2argv-pseries-disk.xml index d9ae4af..3a96209 100644 --- a/tests/qemuxml2argvdata/qemuxml2argv-pseries-disk.xml +++ b/tests/qemuxml2argvdata/qemuxml2argv-pseries-disk.xml @@ -37,5 +37,6 @@ <model type='cirrus' vram='16384' heads='1'/> </video> <memballoon model='none'/> + <panic/> </devices> </domain> diff --git a/tests/qemuxml2argvdata/qemuxml2argv-pseries-nvram.xml b/tests/qemuxml2argvdata/qemuxml2argv-pseries-nvram.xml index 9703bd4..619186a 100644 --- a/tests/qemuxml2argvdata/qemuxml2argv-pseries-nvram.xml +++ b/tests/qemuxml2argvdata/qemuxml2argv-pseries-nvram.xml @@ -20,5 +20,6 @@ <nvram> <address type='spapr-vio' reg='0x4000'/> </nvram> + <panic/> </devices> </domain> diff --git a/tests/qemuxml2argvdata/qemuxml2argv-pseries-panic-address.xml b/tests/qemuxml2argvdata/qemuxml2argv-pseries-panic-address.xml new file mode 100644 index 0000000..e62ead8 --- /dev/null +++ b/tests/qemuxml2argvdata/qemuxml2argv-pseries-panic-address.xml @@ -0,0 +1,32 @@ +<domain type='qemu'> + <name>QEMUGuest1</name> + <uuid>1ccfd97d-5eb4-478a-bbe6-88d254c16db7</uuid> + <memory unit='KiB'>524288</memory> + <currentMemory unit='KiB'>524288</currentMemory> + <vcpu placement='static'>1</vcpu> + <os> + <type arch='ppc64' machine='pseries'>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-ppc64</emulator> + <controller type='usb' index='0'/> + <controller type='pci' index='0' model='pci-root'/> + <serial type='pty'> + <target port='0'/> + <address type='spapr-vio'/> + </serial> + <console type='pty'> + <target type='serial' port='0'/> + <address type='spapr-vio'/> + </console> + <memballoon model='none'/> + <panic> + <address type='isa' iobase='0x505'/> + </panic> + </devices> +</domain> diff --git a/tests/qemuxml2argvdata/qemuxml2argv-pseries-panic-missing.args b/tests/qemuxml2argvdata/qemuxml2argv-pseries-panic-missing.args new file mode 100644 index 0000000..30e4b43 --- /dev/null +++ b/tests/qemuxml2argvdata/qemuxml2argv-pseries-panic-missing.args @@ -0,0 +1,7 @@ +LC_ALL=C PATH=/bin HOME=/home/test USER=test LOGNAME=test QEMU_AUDIO_DRV=none \ +/usr/bin/qemu-system-ppc64 -S -M pseries -m 512 -smp 1 -nographic \ +-nodefconfig -nodefaults \ +-chardev socket,id=charmonitor,path=/tmp/test-monitor,server,nowait \ +-mon chardev=charmonitor,id=monitor,mode=readline -no-acpi -boot c -usb \ +-chardev pty,id=charserial0 \ +-device spapr-vty,chardev=charserial0,reg=0x30000000 diff --git a/tests/qemuxml2argvdata/qemuxml2argv-pseries-panic-missing.xml b/tests/qemuxml2argvdata/qemuxml2argv-pseries-panic-missing.xml new file mode 100644 index 0000000..8980847 --- /dev/null +++ b/tests/qemuxml2argvdata/qemuxml2argv-pseries-panic-missing.xml @@ -0,0 +1,29 @@ +<domain type='qemu'> + <name>QEMUGuest1</name> + <uuid>1ccfd97d-5eb4-478a-bbe6-88d254c16db7</uuid> + <memory unit='KiB'>524288</memory> + <currentMemory unit='KiB'>524288</currentMemory> + <vcpu placement='static'>1</vcpu> + <os> + <type arch='ppc64' machine='pseries'>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-ppc64</emulator> + <controller type='usb' index='0'/> + <controller type='pci' index='0' model='pci-root'/> + <serial type='pty'> + <target port='0'/> + <address type='spapr-vio'/> + </serial> + <console type='pty'> + <target type='serial' port='0'/> + <address type='spapr-vio'/> + </console> + <memballoon model='none'/> + </devices> +</domain> diff --git a/tests/qemuxml2argvdata/qemuxml2argv-pseries-panic-no-address.args b/tests/qemuxml2argvdata/qemuxml2argv-pseries-panic-no-address.args new file mode 100644 index 0000000..30e4b43 --- /dev/null +++ b/tests/qemuxml2argvdata/qemuxml2argv-pseries-panic-no-address.args @@ -0,0 +1,7 @@ +LC_ALL=C PATH=/bin HOME=/home/test USER=test LOGNAME=test QEMU_AUDIO_DRV=none \ +/usr/bin/qemu-system-ppc64 -S -M pseries -m 512 -smp 1 -nographic \ +-nodefconfig -nodefaults \ +-chardev socket,id=charmonitor,path=/tmp/test-monitor,server,nowait \ +-mon chardev=charmonitor,id=monitor,mode=readline -no-acpi -boot c -usb \ +-chardev pty,id=charserial0 \ +-device spapr-vty,chardev=charserial0,reg=0x30000000 diff --git a/tests/qemuxml2argvdata/qemuxml2argv-pseries-panic-no-address.xml b/tests/qemuxml2argvdata/qemuxml2argv-pseries-panic-no-address.xml new file mode 100644 index 0000000..9312975 --- /dev/null +++ b/tests/qemuxml2argvdata/qemuxml2argv-pseries-panic-no-address.xml @@ -0,0 +1,30 @@ +<domain type='qemu'> + <name>QEMUGuest1</name> + <uuid>1ccfd97d-5eb4-478a-bbe6-88d254c16db7</uuid> + <memory unit='KiB'>524288</memory> + <currentMemory unit='KiB'>524288</currentMemory> + <vcpu placement='static'>1</vcpu> + <os> + <type arch='ppc64' machine='pseries'>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-ppc64</emulator> + <controller type='usb' index='0'/> + <controller type='pci' index='0' model='pci-root'/> + <serial type='pty'> + <target port='0'/> + <address type='spapr-vio'/> + </serial> + <console type='pty'> + <target type='serial' port='0'/> + <address type='spapr-vio'/> + </console> + <memballoon model='none'/> + <panic/> + </devices> +</domain> diff --git a/tests/qemuxml2argvtest.c b/tests/qemuxml2argvtest.c index e67d909..28a42a0 100644 --- a/tests/qemuxml2argvtest.c +++ b/tests/qemuxml2argvtest.c @@ -1362,6 +1362,12 @@ mymain(void) QEMU_CAPS_CHARDEV, QEMU_CAPS_DEVICE, QEMU_CAPS_NODEFCONFIG); DO_TEST("pseries-cpu-le", QEMU_CAPS_KVM, QEMU_CAPS_CPU_HOST, QEMU_CAPS_CHARDEV, QEMU_CAPS_DEVICE, QEMU_CAPS_NODEFCONFIG); + DO_TEST("pseries-panic-missing", + QEMU_CAPS_CHARDEV, QEMU_CAPS_DEVICE, QEMU_CAPS_NODEFCONFIG); + DO_TEST("pseries-panic-no-address", + QEMU_CAPS_CHARDEV, QEMU_CAPS_DEVICE, QEMU_CAPS_NODEFCONFIG); + DO_TEST_FAILURE("pseries-panic-address", + QEMU_CAPS_CHARDEV, QEMU_CAPS_DEVICE, QEMU_CAPS_NODEFCONFIG); DO_TEST("disk-ide-drive-split", QEMU_CAPS_DRIVE, QEMU_CAPS_DEVICE, QEMU_CAPS_NODEFCONFIG, QEMU_CAPS_IDE_CD); diff --git a/tests/qemuxml2xmloutdata/qemuxml2xmlout-pseries-panic-missing.xml b/tests/qemuxml2xmloutdata/qemuxml2xmlout-pseries-panic-missing.xml new file mode 100644 index 0000000..9312975 --- /dev/null +++ b/tests/qemuxml2xmloutdata/qemuxml2xmlout-pseries-panic-missing.xml @@ -0,0 +1,30 @@ +<domain type='qemu'> + <name>QEMUGuest1</name> + <uuid>1ccfd97d-5eb4-478a-bbe6-88d254c16db7</uuid> + <memory unit='KiB'>524288</memory> + <currentMemory unit='KiB'>524288</currentMemory> + <vcpu placement='static'>1</vcpu> + <os> + <type arch='ppc64' machine='pseries'>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-ppc64</emulator> + <controller type='usb' index='0'/> + <controller type='pci' index='0' model='pci-root'/> + <serial type='pty'> + <target port='0'/> + <address type='spapr-vio'/> + </serial> + <console type='pty'> + <target type='serial' port='0'/> + <address type='spapr-vio'/> + </console> + <memballoon model='none'/> + <panic/> + </devices> +</domain> diff --git a/tests/qemuxml2xmltest.c b/tests/qemuxml2xmltest.c index 2c53d7c..c67a859 100644 --- a/tests/qemuxml2xmltest.c +++ b/tests/qemuxml2xmltest.c @@ -536,6 +536,8 @@ mymain(void) DO_TEST("virtio-rng-egd"); DO_TEST("pseries-nvram"); + DO_TEST_DIFFERENT("pseries-panic-missing"); + DO_TEST("pseries-panic-no-address"); /* These tests generate different XML */ DO_TEST_DIFFERENT("balloon-device-auto"); -- 2.1.0
On Fri, 2015-05-15 at 11:35 +0200, Andrea Bolognani wrote:
The guest firmware provides the same functionality as the pvpanic device, which is not available in QEMU on pSeries: make sure the XML reflects this fact by automatically adding a <panic/> element when not already present.
On the other hand, unlike the pvpanic device, the guest firmware can't be configured, so report an error if an address has been provided in the XML.
Resolves: https://bugzilla.redhat.com/show_bug.cgi?id=1182388
Ping :) -- Andrea Bolognani <abologna@redhat.com>
On 05/15/2015 05:35 AM, Andrea Bolognani wrote:
The guest firmware provides the same functionality as the pvpanic device, which is not available in QEMU on pSeries: make sure the XML reflects this fact by automatically adding a <panic/> element when not already present.
On the other hand, unlike the pvpanic device, the guest firmware can't be configured, so report an error if an address has been provided in the XML.
Resolves: https://bugzilla.redhat.com/show_bug.cgi?id=1182388 --- src/qemu/qemu_command.c | 25 ++++++++++++----- src/qemu/qemu_domain.c | 14 ++++++++++ .../qemuxml2argvdata/qemuxml2argv-pseries-disk.xml | 1 + .../qemuxml2argv-pseries-nvram.xml | 1 + .../qemuxml2argv-pseries-panic-address.xml | 32 ++++++++++++++++++++++ .../qemuxml2argv-pseries-panic-missing.args | 7 +++++ .../qemuxml2argv-pseries-panic-missing.xml | 29 ++++++++++++++++++++ .../qemuxml2argv-pseries-panic-no-address.args | 7 +++++ .../qemuxml2argv-pseries-panic-no-address.xml | 30 ++++++++++++++++++++ tests/qemuxml2argvtest.c | 6 ++++ .../qemuxml2xmlout-pseries-panic-missing.xml | 30 ++++++++++++++++++++ tests/qemuxml2xmltest.c | 2 ++ 12 files changed, 177 insertions(+), 7 deletions(-) create mode 100644 tests/qemuxml2argvdata/qemuxml2argv-pseries-panic-address.xml create mode 100644 tests/qemuxml2argvdata/qemuxml2argv-pseries-panic-missing.args create mode 100644 tests/qemuxml2argvdata/qemuxml2argv-pseries-panic-missing.xml create mode 100644 tests/qemuxml2argvdata/qemuxml2argv-pseries-panic-no-address.args create mode 100644 tests/qemuxml2argvdata/qemuxml2argv-pseries-panic-no-address.xml create mode 100644 tests/qemuxml2xmloutdata/qemuxml2xmlout-pseries-panic-missing.xml
This should be split into two patches.... The first one dealing with just the error/bug and the second dealing with adding a panic device by default for PPC*
diff --git a/src/qemu/qemu_command.c b/src/qemu/qemu_command.c index 5d0a167..138a8b6 100644 --- a/src/qemu/qemu_command.c +++ b/src/qemu/qemu_command.c @@ -10737,13 +10737,28 @@ qemuBuildCommandLine(virConnectPtr conn, }
if (def->panic) { - if (virQEMUCapsGet(qemuCaps, QEMU_CAPS_DEVICE_PANIC)) { + if (ARCH_IS_PPC64(def->os.arch) && STRPREFIX(def->os.machine, "pseries")) { + /* For pSeries guests, the firmware provides the same + * functionality of the pvpanic device. The address + * cannot be configured by the user */ + if (def->panic->info.type != VIR_DOMAIN_DEVICE_ADDRESS_TYPE_NONE) { + virReportError(VIR_ERR_CONFIG_UNSUPPORTED, "%s", + _("setting the panic device address is not " + "supported for pSeries guests")); + goto error; + }
Seems to be something that should documented in formatdomain.html.in - that is a panic device for PPC64 can not have an <address>
+ } else { + if (!virQEMUCapsGet(qemuCaps, QEMU_CAPS_DEVICE_PANIC)) { + virReportError(VIR_ERR_CONFIG_UNSUPPORTED, "%s", + _("your QEMU is too old to support pvpanic"));
Since you're moving it - use the opportunity to change the message to: "This QEMU doesn't support the panic device" Whether you go with pvpanic or panic just depends on how technically detailed you want to get - libvirt refers to it as panic, while QEMU refers to it as pvpanic... I'm ambivalent as to which to use since it points at the same root issue.
+ goto error; + } + if (def->panic->info.type == VIR_DOMAIN_DEVICE_ADDRESS_TYPE_ISA) { virCommandAddArg(cmd, "-device"); virCommandAddArgFormat(cmd, "pvpanic,ioport=%d", def->panic->info.addr.isa.iobase); - } else if (def->panic->info.type == - VIR_DOMAIN_DEVICE_ADDRESS_TYPE_NONE) { + } else if (def->panic->info.type == VIR_DOMAIN_DEVICE_ADDRESS_TYPE_NONE) {
This change makes the line > 80 characters - so it should be changed back to two lines.
virCommandAddArgList(cmd, "-device", "pvpanic", NULL); } else { virReportError(VIR_ERR_CONFIG_UNSUPPORTED, "%s", @@ -10751,10 +10766,6 @@ qemuBuildCommandLine(virConnectPtr conn, "with ISA address type")); goto error; } - } else { - virReportError(VIR_ERR_CONFIG_UNSUPPORTED, "%s", - _("your QEMU is too old to support pvpanic")); - goto error; } }
The above seems to be "patch 1" (bug fix) while the rest would be "patch 2" (feature change). Although I think swapping the "order" of the patches would cause an issue for git bisect..
diff --git a/src/qemu/qemu_domain.c b/src/qemu/qemu_domain.c index fa8229f..557b0b6 100644 --- a/src/qemu/qemu_domain.c +++ b/src/qemu/qemu_domain.c @@ -911,6 +911,7 @@ qemuDomainDefPostParse(virDomainDefPtr def, bool addDefaultMemballoon = true; bool addDefaultUSBKBD = false; bool addDefaultUSBMouse = false; + bool addPanicDevice = false;
if (def->os.bootloader || def->os.bootloaderArgs) { virReportError(VIR_ERR_CONFIG_UNSUPPORTED, "%s", @@ -963,6 +964,11 @@ qemuDomainDefPostParse(virDomainDefPtr def, addPCIRoot = true; addDefaultUSBKBD = true; addDefaultUSBMouse = true; + /* For pSeries guests, the firmware provides the same + * functionality of the pvpanic device, so automatically + * add the definition if not already present */ + if (STRPREFIX(def->os.machine, "pseries")) + addPanicDevice = true; break;
case VIR_ARCH_ALPHA: @@ -1045,6 +1051,14 @@ qemuDomainDefPostParse(virDomainDefPtr def, VIR_DOMAIN_INPUT_BUS_USB) < 0) return -1;
+ if (addPanicDevice && !def->panic) { + virDomainPanicDefPtr panic; + if (VIR_ALLOC(panic) < 0) + return -1; + + def->panic = panic; + } + return 0; }
Since this is being added as the default will there be issues with the virDomainPanicCheckABIStability()? That is for migration issues (and image save/restore type operations)? Similarly to above - an adjustment to formatdomain.html.in should be made to indicate that a panic device will be added to pSeries guest on PPC64 (see the NVRAM for the example). It's not configurable as it's part of the firmware... The tests below look good to me. John
diff --git a/tests/qemuxml2argvdata/qemuxml2argv-pseries-disk.xml b/tests/qemuxml2argvdata/qemuxml2argv-pseries-disk.xml index d9ae4af..3a96209 100644 --- a/tests/qemuxml2argvdata/qemuxml2argv-pseries-disk.xml +++ b/tests/qemuxml2argvdata/qemuxml2argv-pseries-disk.xml @@ -37,5 +37,6 @@ <model type='cirrus' vram='16384' heads='1'/> </video> <memballoon model='none'/> + <panic/> </devices> </domain> diff --git a/tests/qemuxml2argvdata/qemuxml2argv-pseries-nvram.xml b/tests/qemuxml2argvdata/qemuxml2argv-pseries-nvram.xml index 9703bd4..619186a 100644 --- a/tests/qemuxml2argvdata/qemuxml2argv-pseries-nvram.xml +++ b/tests/qemuxml2argvdata/qemuxml2argv-pseries-nvram.xml @@ -20,5 +20,6 @@ <nvram> <address type='spapr-vio' reg='0x4000'/> </nvram> + <panic/> </devices> </domain> diff --git a/tests/qemuxml2argvdata/qemuxml2argv-pseries-panic-address.xml b/tests/qemuxml2argvdata/qemuxml2argv-pseries-panic-address.xml new file mode 100644 index 0000000..e62ead8 --- /dev/null +++ b/tests/qemuxml2argvdata/qemuxml2argv-pseries-panic-address.xml @@ -0,0 +1,32 @@ +<domain type='qemu'> + <name>QEMUGuest1</name> + <uuid>1ccfd97d-5eb4-478a-bbe6-88d254c16db7</uuid> + <memory unit='KiB'>524288</memory> + <currentMemory unit='KiB'>524288</currentMemory> + <vcpu placement='static'>1</vcpu> + <os> + <type arch='ppc64' machine='pseries'>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-ppc64</emulator> + <controller type='usb' index='0'/> + <controller type='pci' index='0' model='pci-root'/> + <serial type='pty'> + <target port='0'/> + <address type='spapr-vio'/> + </serial> + <console type='pty'> + <target type='serial' port='0'/> + <address type='spapr-vio'/> + </console> + <memballoon model='none'/> + <panic> + <address type='isa' iobase='0x505'/> + </panic> + </devices> +</domain> diff --git a/tests/qemuxml2argvdata/qemuxml2argv-pseries-panic-missing.args b/tests/qemuxml2argvdata/qemuxml2argv-pseries-panic-missing.args new file mode 100644 index 0000000..30e4b43 --- /dev/null +++ b/tests/qemuxml2argvdata/qemuxml2argv-pseries-panic-missing.args @@ -0,0 +1,7 @@ +LC_ALL=C PATH=/bin HOME=/home/test USER=test LOGNAME=test QEMU_AUDIO_DRV=none \ +/usr/bin/qemu-system-ppc64 -S -M pseries -m 512 -smp 1 -nographic \ +-nodefconfig -nodefaults \ +-chardev socket,id=charmonitor,path=/tmp/test-monitor,server,nowait \ +-mon chardev=charmonitor,id=monitor,mode=readline -no-acpi -boot c -usb \ +-chardev pty,id=charserial0 \ +-device spapr-vty,chardev=charserial0,reg=0x30000000 diff --git a/tests/qemuxml2argvdata/qemuxml2argv-pseries-panic-missing.xml b/tests/qemuxml2argvdata/qemuxml2argv-pseries-panic-missing.xml new file mode 100644 index 0000000..8980847 --- /dev/null +++ b/tests/qemuxml2argvdata/qemuxml2argv-pseries-panic-missing.xml @@ -0,0 +1,29 @@ +<domain type='qemu'> + <name>QEMUGuest1</name> + <uuid>1ccfd97d-5eb4-478a-bbe6-88d254c16db7</uuid> + <memory unit='KiB'>524288</memory> + <currentMemory unit='KiB'>524288</currentMemory> + <vcpu placement='static'>1</vcpu> + <os> + <type arch='ppc64' machine='pseries'>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-ppc64</emulator> + <controller type='usb' index='0'/> + <controller type='pci' index='0' model='pci-root'/> + <serial type='pty'> + <target port='0'/> + <address type='spapr-vio'/> + </serial> + <console type='pty'> + <target type='serial' port='0'/> + <address type='spapr-vio'/> + </console> + <memballoon model='none'/> + </devices> +</domain> diff --git a/tests/qemuxml2argvdata/qemuxml2argv-pseries-panic-no-address.args b/tests/qemuxml2argvdata/qemuxml2argv-pseries-panic-no-address.args new file mode 100644 index 0000000..30e4b43 --- /dev/null +++ b/tests/qemuxml2argvdata/qemuxml2argv-pseries-panic-no-address.args @@ -0,0 +1,7 @@ +LC_ALL=C PATH=/bin HOME=/home/test USER=test LOGNAME=test QEMU_AUDIO_DRV=none \ +/usr/bin/qemu-system-ppc64 -S -M pseries -m 512 -smp 1 -nographic \ +-nodefconfig -nodefaults \ +-chardev socket,id=charmonitor,path=/tmp/test-monitor,server,nowait \ +-mon chardev=charmonitor,id=monitor,mode=readline -no-acpi -boot c -usb \ +-chardev pty,id=charserial0 \ +-device spapr-vty,chardev=charserial0,reg=0x30000000 diff --git a/tests/qemuxml2argvdata/qemuxml2argv-pseries-panic-no-address.xml b/tests/qemuxml2argvdata/qemuxml2argv-pseries-panic-no-address.xml new file mode 100644 index 0000000..9312975 --- /dev/null +++ b/tests/qemuxml2argvdata/qemuxml2argv-pseries-panic-no-address.xml @@ -0,0 +1,30 @@ +<domain type='qemu'> + <name>QEMUGuest1</name> + <uuid>1ccfd97d-5eb4-478a-bbe6-88d254c16db7</uuid> + <memory unit='KiB'>524288</memory> + <currentMemory unit='KiB'>524288</currentMemory> + <vcpu placement='static'>1</vcpu> + <os> + <type arch='ppc64' machine='pseries'>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-ppc64</emulator> + <controller type='usb' index='0'/> + <controller type='pci' index='0' model='pci-root'/> + <serial type='pty'> + <target port='0'/> + <address type='spapr-vio'/> + </serial> + <console type='pty'> + <target type='serial' port='0'/> + <address type='spapr-vio'/> + </console> + <memballoon model='none'/> + <panic/> + </devices> +</domain> diff --git a/tests/qemuxml2argvtest.c b/tests/qemuxml2argvtest.c index e67d909..28a42a0 100644 --- a/tests/qemuxml2argvtest.c +++ b/tests/qemuxml2argvtest.c @@ -1362,6 +1362,12 @@ mymain(void) QEMU_CAPS_CHARDEV, QEMU_CAPS_DEVICE, QEMU_CAPS_NODEFCONFIG); DO_TEST("pseries-cpu-le", QEMU_CAPS_KVM, QEMU_CAPS_CPU_HOST, QEMU_CAPS_CHARDEV, QEMU_CAPS_DEVICE, QEMU_CAPS_NODEFCONFIG); + DO_TEST("pseries-panic-missing", + QEMU_CAPS_CHARDEV, QEMU_CAPS_DEVICE, QEMU_CAPS_NODEFCONFIG); + DO_TEST("pseries-panic-no-address", + QEMU_CAPS_CHARDEV, QEMU_CAPS_DEVICE, QEMU_CAPS_NODEFCONFIG); + DO_TEST_FAILURE("pseries-panic-address", + QEMU_CAPS_CHARDEV, QEMU_CAPS_DEVICE, QEMU_CAPS_NODEFCONFIG); DO_TEST("disk-ide-drive-split", QEMU_CAPS_DRIVE, QEMU_CAPS_DEVICE, QEMU_CAPS_NODEFCONFIG, QEMU_CAPS_IDE_CD); diff --git a/tests/qemuxml2xmloutdata/qemuxml2xmlout-pseries-panic-missing.xml b/tests/qemuxml2xmloutdata/qemuxml2xmlout-pseries-panic-missing.xml new file mode 100644 index 0000000..9312975 --- /dev/null +++ b/tests/qemuxml2xmloutdata/qemuxml2xmlout-pseries-panic-missing.xml @@ -0,0 +1,30 @@ +<domain type='qemu'> + <name>QEMUGuest1</name> + <uuid>1ccfd97d-5eb4-478a-bbe6-88d254c16db7</uuid> + <memory unit='KiB'>524288</memory> + <currentMemory unit='KiB'>524288</currentMemory> + <vcpu placement='static'>1</vcpu> + <os> + <type arch='ppc64' machine='pseries'>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-ppc64</emulator> + <controller type='usb' index='0'/> + <controller type='pci' index='0' model='pci-root'/> + <serial type='pty'> + <target port='0'/> + <address type='spapr-vio'/> + </serial> + <console type='pty'> + <target type='serial' port='0'/> + <address type='spapr-vio'/> + </console> + <memballoon model='none'/> + <panic/> + </devices> +</domain> diff --git a/tests/qemuxml2xmltest.c b/tests/qemuxml2xmltest.c index 2c53d7c..c67a859 100644 --- a/tests/qemuxml2xmltest.c +++ b/tests/qemuxml2xmltest.c @@ -536,6 +536,8 @@ mymain(void) DO_TEST("virtio-rng-egd");
DO_TEST("pseries-nvram"); + DO_TEST_DIFFERENT("pseries-panic-missing"); + DO_TEST("pseries-panic-no-address");
/* These tests generate different XML */ DO_TEST_DIFFERENT("balloon-device-auto");
On Wed, 2015-05-27 at 09:44 -0400, John Ferlan wrote:
This should be split into two patches.... The first one dealing with just the error/bug and the second dealing with adding a panic device by default for PPC*
Thanks for the review, John! I've split the changes into separate commits, as suggested, and added a third one to improve the error message as well.
Seems to be something that should documented in formatdomain.html.in - that is a panic device for PPC64 can not have an <address>
It certainly should :) I've updated the documentation accordingly.
+ goto error; + } + if (def->panic->info.type == VIR_DOMAIN_DEVICE_ADDRESS_TYPE_ISA) { virCommandAddArg(cmd, "-device"); virCommandAddArgFormat(cmd, "pvpanic,ioport=%d", def->panic->info.addr.isa.iobase); - } else if (def->panic->info.type == - VIR_DOMAIN_DEVICE_ADDRESS_TYPE_NONE) { + } else if (def->panic->info.type == VIR_DOMAIN_DEVICE_ADDRESS_TYPE_NONE) {
This change makes the line > 80 characters - so it should be changed back to two lines.
I've changed the if-else cascade into a switch statement, which keeps the column count below 80 and looks better IMHO.
Since this is being added as the default will there be issues with the virDomainPanicCheckABIStability()? That is for migration issues (and image save/restore type operations)?
To be honest, I don't know nearly enough about migration to be able to tell whether this is the case. I'll look into it, but it would be great if at least the other two commits (bug fix) could be merged in the meantime. I'll post a v3 right away. Cheers. -- Andrea Bolognani Software Engineer - Virtualization Team $ python -c "print('a'.join(['', 'bologn', '@redh', 't.com']))"
participants (4)
-
Andrea Bolognani -
Cole Robinson -
Daniel P. Berrange -
John Ferlan