On 05/07/2015 12:40 PM, Andrea Bolognani 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. 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 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

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 05/15/2015 05:35 AM, Andrea Bolognani 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* Seems to be something that should documented in formatdomain.html.in - that is a panic device for PPC64 can not have an <address> 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. This change makes the line > 80 characters - so it should be changed back to two lines. 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.. 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