[libvirt] [PATCH v2] qemu: Deal with panic device on pSeries.
John Ferlan
jferlan at redhat.com
Wed May 27 13:44:58 UTC 2015
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");
>
More information about the libvir-list
mailing list