2:41 a.m.
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(a)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
4:39 a.m.
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(a)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
7:15 a.m.
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(a)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
2:09 a.m.
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(a)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
4:30 a.m.
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(a)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 :|
1:57 a.m.
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(a)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.
3984
days inactive
3985
days old
5 comments
4 participants
participants (4)
-
Daniel P. Berrange -
Eric Blake -
Hu Tao -
Peter Krempa