[libvirt] [PATCH] qemu: add support for -device pvpanic
qemu removes the builtin pvpanic device for all qemu versions since 1.7, in order to support <on_crash>, '-device pvpanic' has to be added to qemu command line. Signed-off-by: Hu Tao <hutao@cn.fujitsu.com> --- src/qemu/qemu_capabilities.c | 8 ++++++++ src/qemu/qemu_capabilities.h | 2 ++ src/qemu/qemu_command.c | 4 ++++ 3 files changed, 14 insertions(+) diff --git a/src/qemu/qemu_capabilities.c b/src/qemu/qemu_capabilities.c index 548b988..7783997 100644 --- a/src/qemu/qemu_capabilities.c +++ b/src/qemu/qemu_capabilities.c @@ -243,6 +243,8 @@ VIR_ENUM_IMPL(virQEMUCaps, QEMU_CAPS_LAST, "virtio-mmio", "ich9-intel-hda", "kvm-pit-lost-tick-policy", + + "pvpanic", /* 160 */ ); struct _virQEMUCaps { @@ -1198,6 +1200,9 @@ virQEMUCapsComputeCmdFlags(const char *help, virQEMUCapsSet(qemuCaps, QEMU_CAPS_VNC_SHARE_POLICY); } + if (version >= 1005000) + virQEMUCapsSet(qemuCaps, QEMU_CAPS_PVPANIC); + return 0; } @@ -2561,6 +2566,9 @@ virQEMUCapsInitQMPMonitor(virQEMUCapsPtr qemuCaps, if (qemuCaps->version >= 1006000) virQEMUCapsSet(qemuCaps, QEMU_CAPS_DEVICE_VIDEO_PRIMARY); + if (qemuCaps->version >= 1005000) + virQEMUCapsSet(qemuCaps, QEMU_CAPS_PVPANIC); + if (virQEMUCapsProbeQMPCommands(qemuCaps, mon) < 0) goto cleanup; if (virQEMUCapsProbeQMPEvents(qemuCaps, mon) < 0) diff --git a/src/qemu/qemu_capabilities.h b/src/qemu/qemu_capabilities.h index 02d47c6..06d2fac 100644 --- a/src/qemu/qemu_capabilities.h +++ b/src/qemu/qemu_capabilities.h @@ -199,6 +199,8 @@ enum virQEMUCapsFlags { QEMU_CAPS_DEVICE_ICH9_INTEL_HDA = 158, /* -device ich9-intel-hda */ QEMU_CAPS_KVM_PIT_TICK_POLICY = 159, /* kvm-pit.lost_tick_policy */ + QEMU_CAPS_PVPANIC = 160, /* -device pvpanic */ + QEMU_CAPS_LAST, /* this must always be the last item */ }; diff --git a/src/qemu/qemu_command.c b/src/qemu/qemu_command.c index 763417f..b1307a3 100644 --- a/src/qemu/qemu_command.c +++ b/src/qemu/qemu_command.c @@ -9588,6 +9588,10 @@ qemuBuildCommandLine(virConnectPtr conn, goto error; } + if (virQEMUCapsGet(qemuCaps, QEMU_CAPS_PVPANIC)) { + virCommandAddArgList(cmd, "-device", "pvpanic", NULL); + } + if (mlock) { unsigned long long memKB; -- 1.8.3.1
On 11/27/13 09:41, Hu Tao wrote:
qemu removes the builtin pvpanic device for all qemu versions since 1.7, in order to support <on_crash>, '-device pvpanic' has to be added to qemu command line.
Signed-off-by: Hu Tao <hutao@cn.fujitsu.com> --- src/qemu/qemu_capabilities.c | 8 ++++++++ src/qemu/qemu_capabilities.h | 2 ++ src/qemu/qemu_command.c | 4 ++++ 3 files changed, 14 insertions(+)
diff --git a/src/qemu/qemu_capabilities.c b/src/qemu/qemu_capabilities.c index 548b988..7783997 100644 --- a/src/qemu/qemu_capabilities.c +++ b/src/qemu/qemu_capabilities.c @@ -243,6 +243,8 @@ VIR_ENUM_IMPL(virQEMUCaps, QEMU_CAPS_LAST, "virtio-mmio", "ich9-intel-hda", "kvm-pit-lost-tick-policy", + + "pvpanic", /* 160 */ );
struct _virQEMUCaps { @@ -1198,6 +1200,9 @@ virQEMUCapsComputeCmdFlags(const char *help, virQEMUCapsSet(qemuCaps, QEMU_CAPS_VNC_SHARE_POLICY); }
+ if (version >= 1005000) + virQEMUCapsSet(qemuCaps, QEMU_CAPS_PVPANIC); +
Hard coding the version number is not a good idea. Libvirt uses qmp monitor queries to determine supported devices. Please add this in the virQEMUCapsObjectTypes structure instead, which will do the qmp detection for you.
return 0; }
@@ -2561,6 +2566,9 @@ virQEMUCapsInitQMPMonitor(virQEMUCapsPtr qemuCaps, if (qemuCaps->version >= 1006000) virQEMUCapsSet(qemuCaps, QEMU_CAPS_DEVICE_VIDEO_PRIMARY);
+ if (qemuCaps->version >= 1005000) + virQEMUCapsSet(qemuCaps, QEMU_CAPS_PVPANIC); +
Same here ... this should be autoprobed by a function using the structure above.
if (virQEMUCapsProbeQMPCommands(qemuCaps, mon) < 0) goto cleanup; if (virQEMUCapsProbeQMPEvents(qemuCaps, mon) < 0) diff --git a/src/qemu/qemu_capabilities.h b/src/qemu/qemu_capabilities.h index 02d47c6..06d2fac 100644 --- a/src/qemu/qemu_capabilities.h +++ b/src/qemu/qemu_capabilities.h @@ -199,6 +199,8 @@ enum virQEMUCapsFlags { QEMU_CAPS_DEVICE_ICH9_INTEL_HDA = 158, /* -device ich9-intel-hda */ QEMU_CAPS_KVM_PIT_TICK_POLICY = 159, /* kvm-pit.lost_tick_policy */
+ QEMU_CAPS_PVPANIC = 160, /* -device pvpanic */ + QEMU_CAPS_LAST, /* this must always be the last item */ };
diff --git a/src/qemu/qemu_command.c b/src/qemu/qemu_command.c index 763417f..b1307a3 100644 --- a/src/qemu/qemu_command.c +++ b/src/qemu/qemu_command.c @@ -9588,6 +9588,10 @@ qemuBuildCommandLine(virConnectPtr conn, goto error; }
+ if (virQEMUCapsGet(qemuCaps, QEMU_CAPS_PVPANIC)) { + virCommandAddArgList(cmd, "-device", "pvpanic", NULL); + } +
I remember discussions saying that it's NOT a good idea to enable this stuff always. As a result, this device is not being added by qemu as you described above. Shouldn't we only add this if the user enables <on_crash> actions?
if (mlock) { unsigned long long memKB;
Peter
On 11/27/2013 03:39 AM, Peter Krempa wrote:
On 11/27/13 09:41, Hu Tao wrote:
qemu removes the builtin pvpanic device for all qemu versions since 1.7, in order to support <on_crash>, '-device pvpanic' has to be added to qemu command line.
Signed-off-by: Hu Tao <hutao@cn.fujitsu.com> ---
I remember discussions saying that it's NOT a good idea to enable this stuff always. As a result, this device is not being added by qemu as you described above. Shouldn't we only add this if the user enables <on_crash> actions?
You are precisely right; we MUST add a new entry under <devices> in the <domain> XML before enabling this device. See these threads for some ideas (although recall that qemu has been fixed in the meantime to state that any distro shipping a qemu with pvpanic enabled by default can be considered buggy, and that libvirt can now assume that pvpanic will not happen without an explicit '-device pvpanic'): https://www.redhat.com/archives/libvir-list/2013-August/msg01184.html https://www.redhat.com/archives/libvir-list/2013-August/msg01136.html -- Eric Blake eblake redhat com +1-919-301-3266 Libvirt virtualization library http://libvirt.org
On Wed, Nov 27, 2013 at 06:15:27AM -0700, Eric Blake wrote:
On 11/27/2013 03:39 AM, Peter Krempa wrote:
On 11/27/13 09:41, Hu Tao wrote:
qemu removes the builtin pvpanic device for all qemu versions since 1.7, in order to support <on_crash>, '-device pvpanic' has to be added to qemu command line.
Signed-off-by: Hu Tao <hutao@cn.fujitsu.com> ---
I remember discussions saying that it's NOT a good idea to enable this stuff always. As a result, this device is not being added by qemu as you described above. Shouldn't we only add this if the user enables <on_crash> actions?
You are precisely right; we MUST add a new entry under <devices> in the <domain> XML before enabling this device.
Is a entry under <devices> for pvpanic still needed? What I thought is that it is natural to enable pvpanic when user enables <on_crash>, he/she even has no need to know about pvpanic.
See these threads for some ideas (although recall that qemu has been fixed in the meantime to state that any distro shipping a qemu with pvpanic enabled by default can be considered buggy, and that libvirt can now assume that pvpanic will not happen without an explicit '-device pvpanic'): https://www.redhat.com/archives/libvir-list/2013-August/msg01184.html https://www.redhat.com/archives/libvir-list/2013-August/msg01136.html
IIRC, at the time of the thread, the pvpanic entry under <devices> is for addressing the compatibility of qemu 1.5(has builtin pvpanic) and qemu 1.6 and after(has no builtin pvpanic). Since now qemu removes builtin pvpanic for all versions, I think there is no need for pvpanic entry in xml. -- Regards, Hu Tao
On Thu, Nov 28, 2013 at 04:09:00PM +0800, Hu Tao wrote:
On Wed, Nov 27, 2013 at 06:15:27AM -0700, Eric Blake wrote:
On 11/27/2013 03:39 AM, Peter Krempa wrote:
On 11/27/13 09:41, Hu Tao wrote:
qemu removes the builtin pvpanic device for all qemu versions since 1.7, in order to support <on_crash>, '-device pvpanic' has to be added to qemu command line.
Signed-off-by: Hu Tao <hutao@cn.fujitsu.com> ---
I remember discussions saying that it's NOT a good idea to enable this stuff always. As a result, this device is not being added by qemu as you described above. Shouldn't we only add this if the user enables <on_crash> actions?
You are precisely right; we MUST add a new entry under <devices> in the <domain> XML before enabling this device.
Is a entry under <devices> for pvpanic still needed? What I thought is that it is natural to enable pvpanic when user enables <on_crash>, he/she even has no need to know about pvpanic.
No, the <on_crash> elements are *soley* about setting policy. They must never have any impact on hardware visibility. 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 Wed, Nov 27, 2013 at 11:39:49AM +0100, Peter Krempa wrote:
On 11/27/13 09:41, Hu Tao wrote:
qemu removes the builtin pvpanic device for all qemu versions since 1.7, in order to support <on_crash>, '-device pvpanic' has to be added to qemu command line.
Signed-off-by: Hu Tao <hutao@cn.fujitsu.com> --- src/qemu/qemu_capabilities.c | 8 ++++++++ src/qemu/qemu_capabilities.h | 2 ++ src/qemu/qemu_command.c | 4 ++++ 3 files changed, 14 insertions(+)
diff --git a/src/qemu/qemu_capabilities.c b/src/qemu/qemu_capabilities.c index 548b988..7783997 100644 --- a/src/qemu/qemu_capabilities.c +++ b/src/qemu/qemu_capabilities.c @@ -243,6 +243,8 @@ VIR_ENUM_IMPL(virQEMUCaps, QEMU_CAPS_LAST, "virtio-mmio", "ich9-intel-hda", "kvm-pit-lost-tick-policy", + + "pvpanic", /* 160 */ );
struct _virQEMUCaps { @@ -1198,6 +1200,9 @@ virQEMUCapsComputeCmdFlags(const char *help, virQEMUCapsSet(qemuCaps, QEMU_CAPS_VNC_SHARE_POLICY); }
+ if (version >= 1005000) + virQEMUCapsSet(qemuCaps, QEMU_CAPS_PVPANIC); +
Hard coding the version number is not a good idea. Libvirt uses qmp monitor queries to determine supported devices.
Please add this in the virQEMUCapsObjectTypes structure instead, which will do the qmp detection for you.
Thanks for advising, I'll do it in next version.
return 0; }
@@ -2561,6 +2566,9 @@ virQEMUCapsInitQMPMonitor(virQEMUCapsPtr qemuCaps, if (qemuCaps->version >= 1006000) virQEMUCapsSet(qemuCaps, QEMU_CAPS_DEVICE_VIDEO_PRIMARY);
+ if (qemuCaps->version >= 1005000) + virQEMUCapsSet(qemuCaps, QEMU_CAPS_PVPANIC); +
Same here ... this should be autoprobed by a function using the structure above.
if (virQEMUCapsProbeQMPCommands(qemuCaps, mon) < 0) goto cleanup; if (virQEMUCapsProbeQMPEvents(qemuCaps, mon) < 0) diff --git a/src/qemu/qemu_capabilities.h b/src/qemu/qemu_capabilities.h index 02d47c6..06d2fac 100644 --- a/src/qemu/qemu_capabilities.h +++ b/src/qemu/qemu_capabilities.h @@ -199,6 +199,8 @@ enum virQEMUCapsFlags { QEMU_CAPS_DEVICE_ICH9_INTEL_HDA = 158, /* -device ich9-intel-hda */ QEMU_CAPS_KVM_PIT_TICK_POLICY = 159, /* kvm-pit.lost_tick_policy */
+ QEMU_CAPS_PVPANIC = 160, /* -device pvpanic */ + QEMU_CAPS_LAST, /* this must always be the last item */ };
diff --git a/src/qemu/qemu_command.c b/src/qemu/qemu_command.c index 763417f..b1307a3 100644 --- a/src/qemu/qemu_command.c +++ b/src/qemu/qemu_command.c @@ -9588,6 +9588,10 @@ qemuBuildCommandLine(virConnectPtr conn, goto error; }
+ if (virQEMUCapsGet(qemuCaps, QEMU_CAPS_PVPANIC)) { + virCommandAddArgList(cmd, "-device", "pvpanic", NULL); + } +
I remember discussions saying that it's NOT a good idea to enable this stuff always. As a result, this device is not being added by qemu as you described above. Shouldn't we only add this if the user enables <on_crash> actions?
Yes we should. When I was writing the patch, I found that def->onCrash has a default value even when user doesn't set <on_crash>. I must be reading the code wrong. Any, let me fix it in next version.
participants (4)
-
Daniel P. Berrange -
Eric Blake -
Hu Tao -
Peter Krempa