[libvirt] [PATCH 0/2] Hyper-v crash feature support
A new Hyper-V cpu feature 'hv_crash' was added to QEMU. The feature will become available in v2.5.0. This patch adds support for this feature. Dmitry Andreev (2): conf: add crash to hyperv features qemu: add hv_crash support docs/formatdomain.html.in | 7 +++++++ docs/schemas/domaincommon.rng | 5 +++++ src/conf/domain_conf.c | 6 +++++- src/conf/domain_conf.h | 1 + src/qemu/qemu_command.c | 2 ++ tests/qemuxml2argvdata/qemuxml2argv-hyperv-off.xml | 1 + tests/qemuxml2argvdata/qemuxml2argv-hyperv.args | 4 ++-- tests/qemuxml2argvdata/qemuxml2argv-hyperv.xml | 1 + 8 files changed, 24 insertions(+), 3 deletions(-) -- 1.8.3.1
Add crash CPU feature for Hyper-V. Hyper-V crash MSR's can be used by Hyper-V based guests to notify about occurred guest crash. XML: <features> <hyperv> <crash state='on'/> </hyperv> </features> --- docs/formatdomain.html.in | 7 +++++++ docs/schemas/domaincommon.rng | 5 +++++ src/conf/domain_conf.c | 6 +++++- src/conf/domain_conf.h | 1 + 4 files changed, 18 insertions(+), 1 deletion(-) diff --git a/docs/formatdomain.html.in b/docs/formatdomain.html.in index c88b032..87abeb7 100644 --- a/docs/formatdomain.html.in +++ b/docs/formatdomain.html.in @@ -1460,6 +1460,7 @@ <relaxed state='on'/> <vapic state='on'/> <spinlocks state='on' retries='4096'/> + <crash state='on'/> </hyperv> <kvm> <hidden state='on'/> @@ -1535,6 +1536,12 @@ <td>on, off; retries - at least 4095</td> <td><span class="since">1.1.0 (QEMU only)</span></td> </tr> + <tr> + <td>crash</td> + <td>Enable guest crash notification</td> + <td>on, off</td> + <td><span class="since">2.5.0 (QEMU only)</span></td> + </tr> </table> </dd> <dt><code>pvspinlock</code></dt> diff --git a/docs/schemas/domaincommon.rng b/docs/schemas/domaincommon.rng index f196177..07e73df 100644 --- a/docs/schemas/domaincommon.rng +++ b/docs/schemas/domaincommon.rng @@ -4827,6 +4827,11 @@ </optional> </element> </optional> + <optional> + <element name="crash"> + <ref name="featurestate"/> + </element> + </optional> </interleave> </element> </define> diff --git a/src/conf/domain_conf.c b/src/conf/domain_conf.c index 0c559d2..00f291d 100644 --- a/src/conf/domain_conf.c +++ b/src/conf/domain_conf.c @@ -159,7 +159,8 @@ VIR_ENUM_IMPL(virDomainCapabilitiesPolicy, VIR_DOMAIN_CAPABILITIES_POLICY_LAST, VIR_ENUM_IMPL(virDomainHyperv, VIR_DOMAIN_HYPERV_LAST, "relaxed", "vapic", - "spinlocks") + "spinlocks", + "crash") VIR_ENUM_IMPL(virDomainKVM, VIR_DOMAIN_KVM_LAST, "hidden") @@ -15466,6 +15467,7 @@ virDomainDefParseXML(xmlDocPtr xml, switch ((virDomainHyperv) feature) { case VIR_DOMAIN_HYPERV_RELAXED: case VIR_DOMAIN_HYPERV_VAPIC: + case VIR_DOMAIN_HYPERV_CRASH: if (!(tmp = virXPathString("string(./@state)", ctxt))) { virReportError(VIR_ERR_XML_ERROR, _("missing 'state' attribute for " @@ -17535,6 +17537,7 @@ virDomainDefFeaturesCheckABIStability(virDomainDefPtr src, switch ((virDomainHyperv) i) { case VIR_DOMAIN_HYPERV_RELAXED: case VIR_DOMAIN_HYPERV_VAPIC: + case VIR_DOMAIN_HYPERV_CRASH: if (src->hyperv_features[i] != dst->hyperv_features[i]) { virReportError(VIR_ERR_CONFIG_UNSUPPORTED, _("State of HyperV enlightenment " @@ -22091,6 +22094,7 @@ virDomainDefFormatInternal(virDomainDefPtr def, switch ((virDomainHyperv) j) { case VIR_DOMAIN_HYPERV_RELAXED: case VIR_DOMAIN_HYPERV_VAPIC: + case VIR_DOMAIN_HYPERV_CRASH: if (def->hyperv_features[j]) virBufferAsprintf(buf, "<%s state='%s'/>\n", virDomainHypervTypeToString(j), diff --git a/src/conf/domain_conf.h b/src/conf/domain_conf.h index fd4ef82..4f35b6c 100644 --- a/src/conf/domain_conf.h +++ b/src/conf/domain_conf.h @@ -1692,6 +1692,7 @@ typedef enum { VIR_DOMAIN_HYPERV_RELAXED = 0, VIR_DOMAIN_HYPERV_VAPIC, VIR_DOMAIN_HYPERV_SPINLOCKS, + VIR_DOMAIN_HYPERV_CRASH, VIR_DOMAIN_HYPERV_LAST } virDomainHyperv; -- 1.8.3.1
On 10/26/2015 01:23 PM, Dmitry Andreev wrote:
Add crash CPU feature for Hyper-V. Hyper-V crash MSR's can be used by Hyper-V based guests to notify about occurred guest crash.
XML: <features> <hyperv> <crash state='on'/> </hyperv> </features>
Looks good to me, ACK. By the way, which commit has added this feature to qemu? Is it this one http://git.qemu.org/?p=qemu.git;a=commitdiff;h=f2a53c9e05a24352a0f9740db0539... ?
--- docs/formatdomain.html.in | 7 +++++++ docs/schemas/domaincommon.rng | 5 +++++ src/conf/domain_conf.c | 6 +++++- src/conf/domain_conf.h | 1 + 4 files changed, 18 insertions(+), 1 deletion(-)
diff --git a/docs/formatdomain.html.in b/docs/formatdomain.html.in index c88b032..87abeb7 100644 --- a/docs/formatdomain.html.in +++ b/docs/formatdomain.html.in @@ -1460,6 +1460,7 @@ <relaxed state='on'/> <vapic state='on'/> <spinlocks state='on' retries='4096'/> + <crash state='on'/> </hyperv> <kvm> <hidden state='on'/> @@ -1535,6 +1536,12 @@ <td>on, off; retries - at least 4095</td> <td><span class="since">1.1.0 (QEMU only)</span></td> </tr> + <tr> + <td>crash</td> + <td>Enable guest crash notification</td> + <td>on, off</td> + <td><span class="since">2.5.0 (QEMU only)</span></td> + </tr> </table> </dd> <dt><code>pvspinlock</code></dt> diff --git a/docs/schemas/domaincommon.rng b/docs/schemas/domaincommon.rng index f196177..07e73df 100644 --- a/docs/schemas/domaincommon.rng +++ b/docs/schemas/domaincommon.rng @@ -4827,6 +4827,11 @@ </optional> </element> </optional> + <optional> + <element name="crash"> + <ref name="featurestate"/> + </element> + </optional> </interleave> </element> </define> diff --git a/src/conf/domain_conf.c b/src/conf/domain_conf.c index 0c559d2..00f291d 100644 --- a/src/conf/domain_conf.c +++ b/src/conf/domain_conf.c @@ -159,7 +159,8 @@ VIR_ENUM_IMPL(virDomainCapabilitiesPolicy, VIR_DOMAIN_CAPABILITIES_POLICY_LAST, VIR_ENUM_IMPL(virDomainHyperv, VIR_DOMAIN_HYPERV_LAST, "relaxed", "vapic", - "spinlocks") + "spinlocks", + "crash")
VIR_ENUM_IMPL(virDomainKVM, VIR_DOMAIN_KVM_LAST, "hidden") @@ -15466,6 +15467,7 @@ virDomainDefParseXML(xmlDocPtr xml, switch ((virDomainHyperv) feature) { case VIR_DOMAIN_HYPERV_RELAXED: case VIR_DOMAIN_HYPERV_VAPIC: + case VIR_DOMAIN_HYPERV_CRASH: if (!(tmp = virXPathString("string(./@state)", ctxt))) { virReportError(VIR_ERR_XML_ERROR, _("missing 'state' attribute for " @@ -17535,6 +17537,7 @@ virDomainDefFeaturesCheckABIStability(virDomainDefPtr src, switch ((virDomainHyperv) i) { case VIR_DOMAIN_HYPERV_RELAXED: case VIR_DOMAIN_HYPERV_VAPIC: + case VIR_DOMAIN_HYPERV_CRASH: if (src->hyperv_features[i] != dst->hyperv_features[i]) { virReportError(VIR_ERR_CONFIG_UNSUPPORTED, _("State of HyperV enlightenment " @@ -22091,6 +22094,7 @@ virDomainDefFormatInternal(virDomainDefPtr def, switch ((virDomainHyperv) j) { case VIR_DOMAIN_HYPERV_RELAXED: case VIR_DOMAIN_HYPERV_VAPIC: + case VIR_DOMAIN_HYPERV_CRASH: if (def->hyperv_features[j]) virBufferAsprintf(buf, "<%s state='%s'/>\n", virDomainHypervTypeToString(j), diff --git a/src/conf/domain_conf.h b/src/conf/domain_conf.h index fd4ef82..4f35b6c 100644 --- a/src/conf/domain_conf.h +++ b/src/conf/domain_conf.h @@ -1692,6 +1692,7 @@ typedef enum { VIR_DOMAIN_HYPERV_RELAXED = 0, VIR_DOMAIN_HYPERV_VAPIC, VIR_DOMAIN_HYPERV_SPINLOCKS, + VIR_DOMAIN_HYPERV_CRASH,
VIR_DOMAIN_HYPERV_LAST } virDomainHyperv;
On Mon, Oct 26, 2015 at 13:23:32 +0300, Dmitry Andreev wrote:
Add crash CPU feature for Hyper-V. Hyper-V crash MSR's can be used by Hyper-V based guests to notify about occurred guest crash.
XML: <features> <hyperv> <crash state='on'/> </hyperv> </features>
Sounds like this is related to an existing panic device we already support. So what does enabling hv_crash do in QEMU? Is it an additional channel to a panic device or is the panic device still needed even if hv_crash is enabled? In any case, I think we should map this somehow to the panic device instead of copying 1:1 the way QEMU enables hv_crash. Just to make it clear, NACK until the design is sorted out. Jirka
Add crash CPU feature for Hyper-V. Hyper-V crash MSR's can be used by Hyper-V based guests to notify about occurred guest crash.
XML: <features> <hyperv> <crash state='on'/> </hyperv> </features> Sounds like this is related to an existing panic device we already support. So what does enabling hv_crash do in QEMU? Is it an additional channel to a panic device or is the panic device still needed even if hv_crash is enabled? In any case, I think we should map this somehow to
On Mon, Oct 26, 2015 at 13:23:32 +0300, Dmitry Andreev wrote: the panic device instead of copying 1:1 the way QEMU enables hv_crash.
On 05.11.2015 14:06, Jiri Denemark wrote: pvpanic and Hyper-V crash are independent ways for guest to notify about OS crash. Both ways rise the 'qemu guest panicked' event. Domain can have both hv_crash and pvpanic enabled at the same time. pvpanic is in <devices> section in domain configuration because it is an ISA device. Hyper-V crash is a hypervisor's feature, which enables a set of model-specific registers. Guest can use this registers to send notification and store additional information about a crash. This is a part of Microsoft hypervisor interface. That's why I think hv_crash should be in <features> section.
Just to make it clear, NACK until the design is sorted out.
Jirka
-- libvir-list mailing list libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list
On 05/11/2015 14:54, Dmitry Andreev wrote:
Add crash CPU feature for Hyper-V. Hyper-V crash MSR's can be used by Hyper-V based guests to notify about occurred guest crash.
XML: <features> <hyperv> <crash state='on'/> </hyperv> </features> Sounds like this is related to an existing panic device we already support. So what does enabling hv_crash do in QEMU? Is it an additional channel to a panic device or is the panic device still needed even if hv_crash is enabled? In any case, I think we should map this somehow to the panic device instead of copying 1:1 the way QEMU enables hv_crash.
pvpanic and Hyper-V crash are independent ways for guest to notify about OS crash. Both ways rise the 'qemu guest panicked' event. Domain can have both hv_crash and pvpanic enabled at the same time.
pvpanic is in <devices> section in domain configuration because it is an ISA device. Hyper-V crash is a hypervisor's feature, which enables a set of model-specific registers. Guest can use this registers to send notification and store additional information about a crash. This is a part of Microsoft hypervisor interface.
That's why I think hv_crash should be in <features> section.
I agree. Paolo
Paolo, Jiri, can I do something more for this patch to be accepted? On 05.11.2015 17:32, Paolo Bonzini wrote:
On 05/11/2015 14:54, Dmitry Andreev wrote:
Add crash CPU feature for Hyper-V. Hyper-V crash MSR's can be used by Hyper-V based guests to notify about occurred guest crash.
XML: <features> <hyperv> <crash state='on'/> </hyperv> </features> Sounds like this is related to an existing panic device we already support. So what does enabling hv_crash do in QEMU? Is it an additional channel to a panic device or is the panic device still needed even if hv_crash is enabled? In any case, I think we should map this somehow to the panic device instead of copying 1:1 the way QEMU enables hv_crash. pvpanic and Hyper-V crash are independent ways for guest to notify about OS crash. Both ways rise the 'qemu guest panicked' event. Domain can have both hv_crash and pvpanic enabled at the same time.
pvpanic is in <devices> section in domain configuration because it is an ISA device. Hyper-V crash is a hypervisor's feature, which enables a set of model-specific registers. Guest can use this registers to send notification and store additional information about a crash. This is a part of Microsoft hypervisor interface.
That's why I think hv_crash should be in <features> section. I agree.
Paolo
On 11/10/2015 06:55 PM, Dmitry Andreev wrote:
Paolo, Jiri, can I do something more for this patch to be accepted?
On 05.11.2015 17:32, Paolo Bonzini wrote:
On 05/11/2015 14:54, Dmitry Andreev wrote:
Add crash CPU feature for Hyper-V. Hyper-V crash MSR's can be used by Hyper-V based guests to notify about occurred guest crash.
XML: <features> <hyperv> <crash state='on'/> </hyperv> </features> Sounds like this is related to an existing panic device we already support. So what does enabling hv_crash do in QEMU? Is it an additional channel to a panic device or is the panic device still needed even if hv_crash is enabled? In any case, I think we should map this somehow to the panic device instead of copying 1:1 the way QEMU enables hv_crash. pvpanic and Hyper-V crash are independent ways for guest to notify about OS crash. Both ways rise the 'qemu guest panicked' event. Domain can have both hv_crash and pvpanic enabled at the same time.
pvpanic is in <devices> section in domain configuration because it is an ISA device. Hyper-V crash is a hypervisor's feature, which enables a set of model-specific registers. Guest can use this registers to send notification and store additional information about a crash. This is a part of Microsoft hypervisor interface.
That's why I think hv_crash should be in <features> section. I agree.
Paolo
Please. This feature is very interesting for us for guest debugging especially during guest installation where specific drivers are not available or not ready or not that easy to supply. Den
"Denis V. Lunev" <den@virtuozzo.com> writes:
On 11/10/2015 06:55 PM, Dmitry Andreev wrote:
Paolo, Jiri, can I do something more for this patch to be accepted?
On 05.11.2015 17:32, Paolo Bonzini wrote:
On 05/11/2015 14:54, Dmitry Andreev wrote:
Add crash CPU feature for Hyper-V. Hyper-V crash MSR's can be used by Hyper-V based guests to notify about occurred guest crash.
XML: <features> <hyperv> <crash state='on'/> </hyperv> </features> Sounds like this is related to an existing panic device we already support. So what does enabling hv_crash do in QEMU? Is it an additional channel to a panic device or is the panic device still needed even if hv_crash is enabled? In any case, I think we should map this somehow to the panic device instead of copying 1:1 the way QEMU enables hv_crash. pvpanic and Hyper-V crash are independent ways for guest to notify about OS crash. Both ways rise the 'qemu guest panicked' event. Domain can have both hv_crash and pvpanic enabled at the same time.
pvpanic is in <devices> section in domain configuration because it is an ISA device. Hyper-V crash is a hypervisor's feature, which enables a set of model-specific registers. Guest can use this registers to send notification and store additional information about a crash. This is a part of Microsoft hypervisor interface.
That's why I think hv_crash should be in <features> section. I agree.
Paolo
Please.
This feature is very interesting for us for guest debugging especially during guest installation where specific drivers are not available or not ready or not that easy to supply.
+1 while I think it would make more sense to propagate crash information (HV_X64_MSR_CRASH_P0..P4 MSR) all the way up to libvirt (and even further to libvirt users) instead of just triggering 'guest panic', enabling the feature for Windows guests looks like a good starting point. -- Vitaly
On Thu, Nov 05, 2015 at 16:54:02 +0300, Dmitry Andreev wrote:
Add crash CPU feature for Hyper-V. Hyper-V crash MSR's can be used by Hyper-V based guests to notify about occurred guest crash.
XML: <features> <hyperv> <crash state='on'/> </hyperv> </features> Sounds like this is related to an existing panic device we already support. So what does enabling hv_crash do in QEMU? Is it an additional channel to a panic device or is the panic device still needed even if hv_crash is enabled? In any case, I think we should map this somehow to
On Mon, Oct 26, 2015 at 13:23:32 +0300, Dmitry Andreev wrote: the panic device instead of copying 1:1 the way QEMU enables hv_crash.
On 05.11.2015 14:06, Jiri Denemark wrote: pvpanic and Hyper-V crash are independent ways for guest to notify about OS crash. Both ways rise the 'qemu guest panicked' event. Domain can have both hv_crash and pvpanic enabled at the same time.
pvpanic is in <devices> section in domain configuration because it is an ISA device. Hyper-V crash is a hypervisor's feature, which enables a set of model-specific registers. Guest can use this registers to send notification and store additional information about a crash. This is a part of Microsoft hypervisor interface.
Device or not, I don't really like having two distinct places to configure similar functionality. <device> <panic model='hyperv'/> will do just fine IMO.
That's why I think hv_crash should be in <features> section.
Just to make it clear, NACK until the design is sorted out.
I concur. Peter
On Wed, Nov 11, 2015 at 08:34:02 +0100, Peter Krempa wrote:
On Thu, Nov 05, 2015 at 16:54:02 +0300, Dmitry Andreev wrote:
Add crash CPU feature for Hyper-V. Hyper-V crash MSR's can be used by Hyper-V based guests to notify about occurred guest crash.
XML: <features> <hyperv> <crash state='on'/> </hyperv> </features> Sounds like this is related to an existing panic device we already support. So what does enabling hv_crash do in QEMU? Is it an additional channel to a panic device or is the panic device still needed even if hv_crash is enabled? In any case, I think we should map this somehow to
On Mon, Oct 26, 2015 at 13:23:32 +0300, Dmitry Andreev wrote: the panic device instead of copying 1:1 the way QEMU enables hv_crash.
On 05.11.2015 14:06, Jiri Denemark wrote: pvpanic and Hyper-V crash are independent ways for guest to notify about OS crash. Both ways rise the 'qemu guest panicked' event. Domain can have both hv_crash and pvpanic enabled at the same time.
pvpanic is in <devices> section in domain configuration because it is an ISA device. Hyper-V crash is a hypervisor's feature, which enables a set of model-specific registers. Guest can use this registers to send notification and store additional information about a crash. This is a part of Microsoft hypervisor interface.
Device or not, I don't really like having two distinct places to configure similar functionality.
<device> <panic model='hyperv'/>
will do just fine IMO.
Yeah, this what I was thinking about. After all, we already do something similar on Power. The guest panic notification is an integral part of the platform itself so there's no device we need to add. To reflect this in our domain XMLs, we just always add <device> <panic/> </device> Thus using <panic> element for hv_crash seems like the best approach to me. We'd have just one place for configuring all kinds of guest crash notifications. The question is what the XML should look like. The form suggested by Peter looks good, but then we should probably add the model attribute to all panic devices to make it consistent. So a theoretical XML using all currently supported panic "devices" would be: <device> <panic model='hyperv'/> <!-- hv_crash --> <panic model='isa'/> <!-- pvpanic --> <panic model='pseries'/> <!-- Power --> </device> 'pseries' model would only be allowed on Power, while the others would only be allowed on x86. We'd need to automatically add model='...' to existing device, but it should be pretty easy. Any older libvirt would just ignore the attribute completely and a new libvirt would add model='isa' on x86 and model='pseries' on Power. Jirka
Adding colleague to CC On 11.11.2015 11:34, Jiri Denemark wrote:
On Thu, Nov 05, 2015 at 16:54:02 +0300, Dmitry Andreev wrote:
Add crash CPU feature for Hyper-V. Hyper-V crash MSR's can be used by Hyper-V based guests to notify about occurred guest crash.
XML: <features> <hyperv> <crash state='on'/> </hyperv> </features> Sounds like this is related to an existing panic device we already support. So what does enabling hv_crash do in QEMU? Is it an additional channel to a panic device or is the panic device still needed even if hv_crash is enabled? In any case, I think we should map this somehow to
On Mon, Oct 26, 2015 at 13:23:32 +0300, Dmitry Andreev wrote: the panic device instead of copying 1:1 the way QEMU enables hv_crash.
On 05.11.2015 14:06, Jiri Denemark wrote: pvpanic and Hyper-V crash are independent ways for guest to notify about OS crash. Both ways rise the 'qemu guest panicked' event. Domain can have both hv_crash and pvpanic enabled at the same time.
pvpanic is in <devices> section in domain configuration because it is an ISA device. Hyper-V crash is a hypervisor's feature, which enables a set of model-specific registers. Guest can use this registers to send notification and store additional information about a crash. This is a part of Microsoft hypervisor interface.
Device or not, I don't really like having two distinct places to configure similar functionality.
<device> <panic model='hyperv'/>
will do just fine IMO. Yeah, this what I was thinking about. After all, we already do something similar on Power. The guest panic notification is an integral part of
On Wed, Nov 11, 2015 at 08:34:02 +0100, Peter Krempa wrote: the platform itself so there's no device we need to add. To reflect this in our domain XMLs, we just always add
<device> <panic/> </device>
Thus using <panic> element for hv_crash seems like the best approach to me. We'd have just one place for configuring all kinds of guest crash notifications. The question is what the XML should look like. The form suggested by Peter looks good, but then we should probably add the model attribute to all panic devices to make it consistent. So a theoretical XML using all currently supported panic "devices" would be:
<device> <panic model='hyperv'/> <!-- hv_crash --> <panic model='isa'/> <!-- pvpanic --> <panic model='pseries'/> <!-- Power --> </device>
'pseries' model would only be allowed on Power, while the others would only be allowed on x86. We'd need to automatically add model='...' to existing device, but it should be pretty easy. Any older libvirt would just ignore the attribute completely and a new libvirt would add model='isa' on x86 and model='pseries' on Power.
Jirka
-- libvir-list mailing list libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list
On 11/11/2015 11:57 AM, Dmitry Andreev wrote:
Adding colleague to CC
On 11.11.2015 11:34, Jiri Denemark wrote:
On Thu, Nov 05, 2015 at 16:54:02 +0300, Dmitry Andreev wrote:
Add crash CPU feature for Hyper-V. Hyper-V crash MSR's can be used by Hyper-V based guests to notify about occurred guest crash.
XML: <features> <hyperv> <crash state='on'/> </hyperv> </features> Sounds like this is related to an existing panic device we already support. So what does enabling hv_crash do in QEMU? Is it an additional channel to a panic device or is the panic device still needed even if hv_crash is enabled? In any case, I think we should map this somehow to
On Mon, Oct 26, 2015 at 13:23:32 +0300, Dmitry Andreev wrote: the panic device instead of copying 1:1 the way QEMU enables hv_crash.
On 05.11.2015 14:06, Jiri Denemark wrote: pvpanic and Hyper-V crash are independent ways for guest to notify about OS crash. Both ways rise the 'qemu guest panicked' event. Domain can have both hv_crash and pvpanic enabled at the same time.
pvpanic is in <devices> section in domain configuration because it is an ISA device. Hyper-V crash is a hypervisor's feature, which enables a set of model-specific registers. Guest can use this registers to send notification and store additional information about a crash. This is a part of Microsoft hypervisor interface.
Device or not, I don't really like having two distinct places to configure similar functionality.
<device> <panic model='hyperv'/>
will do just fine IMO. Yeah, this what I was thinking about. After all, we already do something similar on Power. The guest panic notification is an integral part of
On Wed, Nov 11, 2015 at 08:34:02 +0100, Peter Krempa wrote: the platform itself so there's no device we need to add. To reflect this in our domain XMLs, we just always add
<device> <panic/> </device>
Thus using <panic> element for hv_crash seems like the best approach to me. We'd have just one place for configuring all kinds of guest crash notifications. The question is what the XML should look like. The form suggested by Peter looks good, but then we should probably add the model attribute to all panic devices to make it consistent. So a theoretical XML using all currently supported panic "devices" would be:
<device> <panic model='hyperv'/> <!-- hv_crash --> <panic model='isa'/> <!-- pvpanic --> <panic model='pseries'/> <!-- Power --> </device>
'pseries' model would only be allowed on Power, while the others would only be allowed on x86. We'd need to automatically add model='...' to existing device, but it should be pretty easy. Any older libvirt would just ignore the attribute completely and a new libvirt would add model='isa' on x86 and model='pseries' on Power.
Jirka
QEMU accepts this option through HyperV set of CPU options. In QEMU this declared as follows:
static Property x86_cpu_properties[] = { DEFINE_PROP_BOOL("pmu", X86CPU, enable_pmu, false), { .name = "hv-spinlocks", .info = &qdev_prop_spinlocks }, DEFINE_PROP_BOOL("hv-relaxed", X86CPU, hyperv_relaxed_timing, false), DEFINE_PROP_BOOL("hv-vapic", X86CPU, hyperv_vapic, false), DEFINE_PROP_BOOL("hv-time", X86CPU, hyperv_time, false), DEFINE_PROP_BOOL("hv-crash", X86CPU, hyperv_crash, false), DEFINE_PROP_BOOL("hv-reset", X86CPU, hyperv_reset, false), DEFINE_PROP_BOOL("hv-vpindex", X86CPU, hyperv_vpindex, false), DEFINE_PROP_BOOL("hv-runtime", X86CPU, hyperv_runtime, false), DEFINE_PROP_BOOL("check", X86CPU, check_cpuid, true), DEFINE_PROP_BOOL("enforce", X86CPU, enforce_cpuid, false), DEFINE_PROP_BOOL("kvm", X86CPU, expose_kvm, true), DEFINE_PROP_UINT32("level", X86CPU, env.cpuid_level, 0), DEFINE_PROP_UINT32("xlevel", X86CPU, env.cpuid_xlevel, 0), DEFINE_PROP_UINT32("xlevel2", X86CPU, env.cpuid_xlevel2, 0), DEFINE_PROP_STRING("hv-vendor-id", X86CPU, hyperv_vendor_id), DEFINE_PROP_END_OF_LIST() }; Thus your proposal means that in this case we will have to add this option to CPU part of the command line from devices part. Is this correct? That is why we proposing to follow this way and keep HyperV CPU properties in one place. And, finally, 'no', this is not a device from QEMU perspective of view. Den
On 11/11/2015 10:09, Denis V. Lunev wrote:
QEMU accepts this option through HyperV set of CPU options. Thus your proposal means that in this case we will have to add this option to CPU part of the command line from devices part.
Is this correct?
And, finally, 'no', this is not a device from QEMU perspective of view.
That doesn't really matter. It's okay if libvirt and QEMU model feature in a slightly different manner. For an example, see how kvmclock is changed from a <timer> element to an option of -cpu. I think I now agree with the libvirt folks. The effects of hv_crash are the same as the effects of <panic>, for example the VM will not anymore obey the "reboot on BSoD" setting, so it should be added to <panic>. Paolo
On Wed, Nov 11, 2015 at 12:09:33PM +0300, Denis V. Lunev wrote:
On 11/11/2015 11:57 AM, Dmitry Andreev wrote:
Adding colleague to CC
On 11.11.2015 11:34, Jiri Denemark wrote:
On Thu, Nov 05, 2015 at 16:54:02 +0300, Dmitry Andreev wrote:
On Mon, Oct 26, 2015 at 13:23:32 +0300, Dmitry Andreev wrote: >Add crash CPU feature for Hyper-V. Hyper-V crash MSR's can be used >by Hyper-V based guests to notify about occurred guest crash. > >XML: ><features> > <hyperv> > <crash state='on'/> > </hyperv> ></features> Sounds like this is related to an existing panic device we already support. So what does enabling hv_crash do in QEMU? Is it an additional channel to a panic device or is the panic device still needed even if hv_crash is enabled? In any case, I think we should map this somehow to the panic device instead of copying 1:1 the way QEMU enables hv_crash.
On 05.11.2015 14:06, Jiri Denemark wrote: pvpanic and Hyper-V crash are independent ways for guest to notify about OS crash. Both ways rise the 'qemu guest panicked' event. Domain can have both hv_crash and pvpanic enabled at the same time.
pvpanic is in <devices> section in domain configuration because it is an ISA device. Hyper-V crash is a hypervisor's feature, which enables a set of model-specific registers. Guest can use this registers to send notification and store additional information about a crash. This is a part of Microsoft hypervisor interface.
Device or not, I don't really like having two distinct places to configure similar functionality.
<device> <panic model='hyperv'/>
will do just fine IMO. Yeah, this what I was thinking about. After all, we already do something similar on Power. The guest panic notification is an integral part of
On Wed, Nov 11, 2015 at 08:34:02 +0100, Peter Krempa wrote: the platform itself so there's no device we need to add. To reflect this in our domain XMLs, we just always add
<device> <panic/> </device>
Thus using <panic> element for hv_crash seems like the best approach to me. We'd have just one place for configuring all kinds of guest crash notifications. The question is what the XML should look like. The form suggested by Peter looks good, but then we should probably add the model attribute to all panic devices to make it consistent. So a theoretical XML using all currently supported panic "devices" would be:
<device> <panic model='hyperv'/> <!-- hv_crash --> <panic model='isa'/> <!-- pvpanic --> <panic model='pseries'/> <!-- Power --> </device>
'pseries' model would only be allowed on Power, while the others would only be allowed on x86. We'd need to automatically add model='...' to existing device, but it should be pretty easy. Any older libvirt would just ignore the attribute completely and a new libvirt would add model='isa' on x86 and model='pseries' on Power.
Jirka
QEMU accepts this option through HyperV set of CPU options. In QEMU this declared as follows:
static Property x86_cpu_properties[] = { DEFINE_PROP_BOOL("pmu", X86CPU, enable_pmu, false), { .name = "hv-spinlocks", .info = &qdev_prop_spinlocks }, DEFINE_PROP_BOOL("hv-relaxed", X86CPU, hyperv_relaxed_timing, false), DEFINE_PROP_BOOL("hv-vapic", X86CPU, hyperv_vapic, false), DEFINE_PROP_BOOL("hv-time", X86CPU, hyperv_time, false), DEFINE_PROP_BOOL("hv-crash", X86CPU, hyperv_crash, false), DEFINE_PROP_BOOL("hv-reset", X86CPU, hyperv_reset, false), DEFINE_PROP_BOOL("hv-vpindex", X86CPU, hyperv_vpindex, false), DEFINE_PROP_BOOL("hv-runtime", X86CPU, hyperv_runtime, false), DEFINE_PROP_BOOL("check", X86CPU, check_cpuid, true), DEFINE_PROP_BOOL("enforce", X86CPU, enforce_cpuid, false), DEFINE_PROP_BOOL("kvm", X86CPU, expose_kvm, true), DEFINE_PROP_UINT32("level", X86CPU, env.cpuid_level, 0), DEFINE_PROP_UINT32("xlevel", X86CPU, env.cpuid_xlevel, 0), DEFINE_PROP_UINT32("xlevel2", X86CPU, env.cpuid_xlevel2, 0), DEFINE_PROP_STRING("hv-vendor-id", X86CPU, hyperv_vendor_id), DEFINE_PROP_END_OF_LIST() };
Thus your proposal means that in this case we will have to add this option to CPU part of the command line from devices part.
Is this correct?
That is why we proposing to follow this way and keep HyperV CPU properties in one place.
And, finally, 'no', this is not a device from QEMU perspective of view.
Having libvirt model XML in exactly the same way as QEMU models its command line is an explicit *non-goal* of libvirt. Libvirt aims to provide an XML representation that is generic across arbitrary hypervisor platforms. So it is fully expected that we will model things differently from QEMU in some cases where there is a better way todo things from the libvirt POV. All that matters is that the approach we choose in libvirt is sufficiently expressive to cover the range of features of the underlying platforms. 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 11/11/2015 12:30 PM, Daniel P. Berrange wrote:
On Wed, Nov 11, 2015 at 12:09:33PM +0300, Denis V. Lunev wrote:
On 11/11/2015 11:57 AM, Dmitry Andreev wrote:
Adding colleague to CC
On 11.11.2015 11:34, Jiri Denemark wrote:
On Thu, Nov 05, 2015 at 16:54:02 +0300, Dmitry Andreev wrote:
On 05.11.2015 14:06, Jiri Denemark wrote: > On Mon, Oct 26, 2015 at 13:23:32 +0300, Dmitry Andreev wrote: >> Add crash CPU feature for Hyper-V. Hyper-V crash MSR's can be used >> by Hyper-V based guests to notify about occurred guest crash. >> >> XML: >> <features> >> <hyperv> >> <crash state='on'/> >> </hyperv> >> </features> > Sounds like this is related to an existing panic device we already > support. So what does enabling hv_crash do in QEMU? Is it an > additional > channel to a panic device or is the panic device still needed even if > hv_crash is enabled? In any case, I think we should map this > somehow to > the panic device instead of copying 1:1 the way QEMU enables > hv_crash. pvpanic and Hyper-V crash are independent ways for guest to notify about OS crash. Both ways rise the 'qemu guest panicked' event. Domain can have both hv_crash and pvpanic enabled at the same time.
pvpanic is in <devices> section in domain configuration because it is an ISA device. Hyper-V crash is a hypervisor's feature, which enables a set of model-specific registers. Guest can use this registers to send notification and store additional information about a crash. This is a part of Microsoft hypervisor interface. Device or not, I don't really like having two distinct places to configure similar functionality.
<device> <panic model='hyperv'/>
will do just fine IMO. Yeah, this what I was thinking about. After all, we already do something similar on Power. The guest panic notification is an integral part of
On Wed, Nov 11, 2015 at 08:34:02 +0100, Peter Krempa wrote: the platform itself so there's no device we need to add. To reflect this in our domain XMLs, we just always add
<device> <panic/> </device>
Thus using <panic> element for hv_crash seems like the best approach to me. We'd have just one place for configuring all kinds of guest crash notifications. The question is what the XML should look like. The form suggested by Peter looks good, but then we should probably add the model attribute to all panic devices to make it consistent. So a theoretical XML using all currently supported panic "devices" would be:
<device> <panic model='hyperv'/> <!-- hv_crash --> <panic model='isa'/> <!-- pvpanic --> <panic model='pseries'/> <!-- Power --> </device>
'pseries' model would only be allowed on Power, while the others would only be allowed on x86. We'd need to automatically add model='...' to existing device, but it should be pretty easy. Any older libvirt would just ignore the attribute completely and a new libvirt would add model='isa' on x86 and model='pseries' on Power.
Jirka
QEMU accepts this option through HyperV set of CPU options. In QEMU this declared as follows:
static Property x86_cpu_properties[] = { DEFINE_PROP_BOOL("pmu", X86CPU, enable_pmu, false), { .name = "hv-spinlocks", .info = &qdev_prop_spinlocks }, DEFINE_PROP_BOOL("hv-relaxed", X86CPU, hyperv_relaxed_timing, false), DEFINE_PROP_BOOL("hv-vapic", X86CPU, hyperv_vapic, false), DEFINE_PROP_BOOL("hv-time", X86CPU, hyperv_time, false), DEFINE_PROP_BOOL("hv-crash", X86CPU, hyperv_crash, false), DEFINE_PROP_BOOL("hv-reset", X86CPU, hyperv_reset, false), DEFINE_PROP_BOOL("hv-vpindex", X86CPU, hyperv_vpindex, false), DEFINE_PROP_BOOL("hv-runtime", X86CPU, hyperv_runtime, false), DEFINE_PROP_BOOL("check", X86CPU, check_cpuid, true), DEFINE_PROP_BOOL("enforce", X86CPU, enforce_cpuid, false), DEFINE_PROP_BOOL("kvm", X86CPU, expose_kvm, true), DEFINE_PROP_UINT32("level", X86CPU, env.cpuid_level, 0), DEFINE_PROP_UINT32("xlevel", X86CPU, env.cpuid_xlevel, 0), DEFINE_PROP_UINT32("xlevel2", X86CPU, env.cpuid_xlevel2, 0), DEFINE_PROP_STRING("hv-vendor-id", X86CPU, hyperv_vendor_id), DEFINE_PROP_END_OF_LIST() };
Thus your proposal means that in this case we will have to add this option to CPU part of the command line from devices part.
Is this correct?
That is why we proposing to follow this way and keep HyperV CPU properties in one place.
And, finally, 'no', this is not a device from QEMU perspective of view. Having libvirt model XML in exactly the same way as QEMU models its command line is an explicit *non-goal* of libvirt.
Libvirt aims to provide an XML representation that is generic across arbitrary hypervisor platforms. So it is fully expected that we will model things differently from QEMU in some cases where there is a better way todo things from the libvirt POV.
All that matters is that the approach we choose in libvirt is sufficiently expressive to cover the range of features of the underlying platforms.
Regards, Daniel ok. We will follow this way. No problem.
We have just need a clear agreement all parts will be happy with this approach. Thank you for proposal. Den
On Wed, Nov 11, 2015 at 12:09:33 +0300, Denis V. Lunev wrote:
Adding colleague to CC
On 11.11.2015 11:34, Jiri Denemark wrote: ...
Yeah, this what I was thinking about. After all, we already do something similar on Power. The guest panic notification is an integral part of the platform itself so there's no device we need to add. To reflect this in our domain XMLs, we just always add
<device> <panic/> </device>
Thus using <panic> element for hv_crash seems like the best approach to me. We'd have just one place for configuring all kinds of guest crash notifications. The question is what the XML should look like. The form suggested by Peter looks good, but then we should probably add the model attribute to all panic devices to make it consistent. So a theoretical XML using all currently supported panic "devices" would be:
<device> <panic model='hyperv'/> <!-- hv_crash --> <panic model='isa'/> <!-- pvpanic --> <panic model='pseries'/> <!-- Power --> </device>
'pseries' model would only be allowed on Power, while the others would only be allowed on x86. We'd need to automatically add model='...' to existing device, but it should be pretty easy. Any older libvirt would just ignore the attribute completely and a new libvirt would add model='isa' on x86 and model='pseries' on Power.
Jirka
QEMU accepts this option through HyperV set of CPU options. ... Thus your proposal means that in this case we will have to add
On 11/11/2015 11:57 AM, Dmitry Andreev wrote: this option to CPU part of the command line from devices part.
Is this correct?
Yes.
And, finally, 'no', this is not a device from QEMU perspective of view.
I know. It's not a device on Power either. In libvirt we try to make things consistent across various platforms and hypervisors. We were not always successful in this usually because our original design of some features were not future proof enough. But we can easily maintain consistency in this case, we already have a panic "device" and hv_crash can be easily mapped into it. If sticking with QEMU way of configuring various features was our goal, all other hyperv features would rather be enabled within <cpu> element. But we enable them differently in domain XML because it just didn't fit very well in libvirt's <cpu> element. Thus using device/panic for hv_crash even if it's not a device is IMHO better because it keeps all crash notification config consistent. Anyone wishing to know whether guest crash events notifications are enabled for a domain can just look for device/panic element without thinking about all possible implementations which are hidden inside libvirt anyway. Jirka
XML: <features> <hyperv> <crash state='on'/> </hyperv> </features> QEMU command line: qemu -cpu <cpu_model>,hv_crash --- src/qemu/qemu_command.c | 2 ++ tests/qemuxml2argvdata/qemuxml2argv-hyperv-off.xml | 1 + tests/qemuxml2argvdata/qemuxml2argv-hyperv.args | 4 ++-- tests/qemuxml2argvdata/qemuxml2argv-hyperv.xml | 1 + 4 files changed, 6 insertions(+), 2 deletions(-) diff --git a/src/qemu/qemu_command.c b/src/qemu/qemu_command.c index 8824541..e0693af 100644 --- a/src/qemu/qemu_command.c +++ b/src/qemu/qemu_command.c @@ -7643,6 +7643,7 @@ qemuBuildCpuArgStr(virQEMUDriverPtr driver, switch ((virDomainHyperv) i) { case VIR_DOMAIN_HYPERV_RELAXED: case VIR_DOMAIN_HYPERV_VAPIC: + case VIR_DOMAIN_HYPERV_CRASH: if (def->hyperv_features[i] == VIR_TRISTATE_SWITCH_ON) virBufferAsprintf(&buf, ",hv_%s", virDomainHypervTypeToString(i)); @@ -12826,6 +12827,7 @@ qemuParseCommandLineCPU(virDomainDefPtr dom, switch ((virDomainHyperv) f) { case VIR_DOMAIN_HYPERV_RELAXED: case VIR_DOMAIN_HYPERV_VAPIC: + case VIR_DOMAIN_HYPERV_CRASH: if (value) { virReportError(VIR_ERR_CONFIG_UNSUPPORTED, _("HyperV feature '%s' should not " diff --git a/tests/qemuxml2argvdata/qemuxml2argv-hyperv-off.xml b/tests/qemuxml2argvdata/qemuxml2argv-hyperv-off.xml index 4ec16d5..7c0b02b 100644 --- a/tests/qemuxml2argvdata/qemuxml2argv-hyperv-off.xml +++ b/tests/qemuxml2argvdata/qemuxml2argv-hyperv-off.xml @@ -14,6 +14,7 @@ <relaxed state='off'/> <vapic state='off'/> <spinlocks state='off'/> + <crash state='off'/> </hyperv> </features> <clock offset='utc'/> diff --git a/tests/qemuxml2argvdata/qemuxml2argv-hyperv.args b/tests/qemuxml2argvdata/qemuxml2argv-hyperv.args index 91b3570..d278acc 100644 --- a/tests/qemuxml2argvdata/qemuxml2argv-hyperv.args +++ b/tests/qemuxml2argvdata/qemuxml2argv-hyperv.args @@ -1,5 +1,5 @@ LC_ALL=C PATH=/bin HOME=/home/test USER=test LOGNAME=test QEMU_AUDIO_DRV=none \ /usr/bin/qemu -S -M pc \ --cpu qemu32,hv_relaxed,hv_vapic,hv_spinlocks=0x2fff -m 214 -smp 6 -nographic -monitor \ -unix:/tmp/test-monitor,server,nowait -boot n -usb -net none -serial none \ +-cpu qemu32,hv_relaxed,hv_vapic,hv_spinlocks=0x2fff,hv_crash -m 214 -smp 6 -nographic \ +-monitor unix:/tmp/test-monitor,server,nowait -boot n -usb -net none -serial none \ -parallel none diff --git a/tests/qemuxml2argvdata/qemuxml2argv-hyperv.xml b/tests/qemuxml2argvdata/qemuxml2argv-hyperv.xml index 5b80040..1740ebf 100644 --- a/tests/qemuxml2argvdata/qemuxml2argv-hyperv.xml +++ b/tests/qemuxml2argvdata/qemuxml2argv-hyperv.xml @@ -14,6 +14,7 @@ <relaxed state='on'/> <vapic state='on'/> <spinlocks state='on' retries='12287'/> + <crash state='on'/> </hyperv> </features> <clock offset='utc'/> -- 1.8.3.1
On 10/26/2015 01:23 PM, Dmitry Andreev wrote:
XML: <features> <hyperv> <crash state='on'/> </hyperv> </features>
QEMU command line: qemu -cpu <cpu_model>,hv_crash
ACK from me, but I think someone else should look at these patches.
--- src/qemu/qemu_command.c | 2 ++ tests/qemuxml2argvdata/qemuxml2argv-hyperv-off.xml | 1 + tests/qemuxml2argvdata/qemuxml2argv-hyperv.args | 4 ++-- tests/qemuxml2argvdata/qemuxml2argv-hyperv.xml | 1 + 4 files changed, 6 insertions(+), 2 deletions(-)
diff --git a/src/qemu/qemu_command.c b/src/qemu/qemu_command.c index 8824541..e0693af 100644 --- a/src/qemu/qemu_command.c +++ b/src/qemu/qemu_command.c @@ -7643,6 +7643,7 @@ qemuBuildCpuArgStr(virQEMUDriverPtr driver, switch ((virDomainHyperv) i) { case VIR_DOMAIN_HYPERV_RELAXED: case VIR_DOMAIN_HYPERV_VAPIC: + case VIR_DOMAIN_HYPERV_CRASH: if (def->hyperv_features[i] == VIR_TRISTATE_SWITCH_ON) virBufferAsprintf(&buf, ",hv_%s", virDomainHypervTypeToString(i)); @@ -12826,6 +12827,7 @@ qemuParseCommandLineCPU(virDomainDefPtr dom, switch ((virDomainHyperv) f) { case VIR_DOMAIN_HYPERV_RELAXED: case VIR_DOMAIN_HYPERV_VAPIC: + case VIR_DOMAIN_HYPERV_CRASH: if (value) { virReportError(VIR_ERR_CONFIG_UNSUPPORTED, _("HyperV feature '%s' should not " diff --git a/tests/qemuxml2argvdata/qemuxml2argv-hyperv-off.xml b/tests/qemuxml2argvdata/qemuxml2argv-hyperv-off.xml index 4ec16d5..7c0b02b 100644 --- a/tests/qemuxml2argvdata/qemuxml2argv-hyperv-off.xml +++ b/tests/qemuxml2argvdata/qemuxml2argv-hyperv-off.xml @@ -14,6 +14,7 @@ <relaxed state='off'/> <vapic state='off'/> <spinlocks state='off'/> + <crash state='off'/> </hyperv> </features> <clock offset='utc'/> diff --git a/tests/qemuxml2argvdata/qemuxml2argv-hyperv.args b/tests/qemuxml2argvdata/qemuxml2argv-hyperv.args index 91b3570..d278acc 100644 --- a/tests/qemuxml2argvdata/qemuxml2argv-hyperv.args +++ b/tests/qemuxml2argvdata/qemuxml2argv-hyperv.args @@ -1,5 +1,5 @@ LC_ALL=C PATH=/bin HOME=/home/test USER=test LOGNAME=test QEMU_AUDIO_DRV=none \ /usr/bin/qemu -S -M pc \ --cpu qemu32,hv_relaxed,hv_vapic,hv_spinlocks=0x2fff -m 214 -smp 6 -nographic -monitor \ -unix:/tmp/test-monitor,server,nowait -boot n -usb -net none -serial none \ +-cpu qemu32,hv_relaxed,hv_vapic,hv_spinlocks=0x2fff,hv_crash -m 214 -smp 6 -nographic \ +-monitor unix:/tmp/test-monitor,server,nowait -boot n -usb -net none -serial none \ -parallel none diff --git a/tests/qemuxml2argvdata/qemuxml2argv-hyperv.xml b/tests/qemuxml2argvdata/qemuxml2argv-hyperv.xml index 5b80040..1740ebf 100644 --- a/tests/qemuxml2argvdata/qemuxml2argv-hyperv.xml +++ b/tests/qemuxml2argvdata/qemuxml2argv-hyperv.xml @@ -14,6 +14,7 @@ <relaxed state='on'/> <vapic state='on'/> <spinlocks state='on' retries='12287'/> + <crash state='on'/> </hyperv> </features> <clock offset='utc'/>
participants (8)
-
Daniel P. Berrange -
Denis V. Lunev -
Dmitry Andreev -
Dmitry Guryanov -
Jiri Denemark -
Paolo Bonzini -
Peter Krempa -
Vitaly Kuznetsov