[libvirt] [PATCH v2 1/2] conf: add xml element devices/pvpanic
This patch adds a new xml element devices/pvpanic to support qemu device pvpanic. It can be used to receive guest panic notification. Signed-off-by: Hu Tao <hutao@cn.fujitsu.com> --- docs/formatdomain.html.in | 25 +++++++++++++++++ src/conf/domain_conf.c | 68 +++++++++++++++++++++++++++++++++++++++++++++++ src/conf/domain_conf.h | 9 +++++++ 3 files changed, 102 insertions(+) diff --git a/docs/formatdomain.html.in b/docs/formatdomain.html.in index 1850a2b..0a72baa 100644 --- a/docs/formatdomain.html.in +++ b/docs/formatdomain.html.in @@ -5080,6 +5080,31 @@ qemu-kvm -net nic,model=? /dev/null </dd> </dl> + <h4><a name="elementsPvpanic">pvpanic device</a></h4> + <p> + pvpanic device enables libvirt to receive panic notification from a QEMU + guest. + <span class="since">Since 1.3.0, QEMU and KVM only</span> + </p> + <p> + Example: usage of pvpanic configuration + </p> +<pre> + ... + <devices> + <pvpanic ioport='0x505'/> + </devices> + ... +</pre> + <dl> + <dt><code>ioport</code></dt> + <dd> + <p> + ioport used by pvpanic. + </p> + </dd> + </dl> + <h3><a name="seclabel">Security label</a></h3> <p> diff --git a/src/conf/domain_conf.c b/src/conf/domain_conf.c index 140eb80..1b8f66f 100644 --- a/src/conf/domain_conf.c +++ b/src/conf/domain_conf.c @@ -1876,6 +1876,11 @@ virDomainResourceDefFree(virDomainResourceDefPtr resource) VIR_FREE(resource); } +void +virDomainPvpanicDefFree(virDomainPvpanicDefPtr pvpanic) +{ + VIR_FREE(pvpanic); +} void virDomainDefFree(virDomainDefPtr def) { @@ -1964,6 +1969,8 @@ void virDomainDefFree(virDomainDefPtr def) virDomainTPMDefFree(def->tpm); + virDomainPvpanicDefFree(def->pvpanic); + VIR_FREE(def->idmap.uidmap); VIR_FREE(def->idmap.gidmap); @@ -10626,6 +10633,31 @@ cleanup: return idmap; } +static virDomainPvpanicDefPtr +virDomainPvpanicDefParseXML(xmlNodePtr node) +{ + char *ioport = NULL; + virDomainPvpanicDefPtr pvpanic; + + if (VIR_ALLOC(pvpanic) < 0) + return NULL; + + ioport = virXMLPropString(node, "ioport"); + if (!ioport) { + pvpanic->ioport = -1; + } else { + if (virStrToLong_i(ioport, NULL, 0, &pvpanic->ioport) < 0) { + virReportError(VIR_ERR_INTERNAL_ERROR, "%s", + _("Cannot parse <pvpanic> 'ioport' attribute")); + goto error; + } + } + + return pvpanic; +error: + virDomainPvpanicDefFree(pvpanic); + return NULL; +} /* Parse the XML definition for a vcpupin or emulatorpin. * @@ -12453,6 +12485,27 @@ virDomainDefParseXML(xmlDocPtr xml, } VIR_FREE(nodes); + /* analysis of the pvpanic devices */ + def->pvpanic = NULL; + if ((n = virXPathNodeSet("./devices/pvpanic", ctxt, &nodes)) < 0) { + goto error; + } + if (n > 1) { + virReportError(VIR_ERR_INTERNAL_ERROR, "%s", + _("only a single pvpanic device is supported")); + goto error; + } + if (n > 0) { + virDomainPvpanicDefPtr pvpanic = + virDomainPvpanicDefParseXML(nodes[0]); + if (!pvpanic) + goto error; + + def->pvpanic = pvpanic; + VIR_FREE(nodes); + } + + /* analysis of the user namespace mapping */ if ((n = virXPathNodeSet("./idmap/uid", ctxt, &nodes)) < 0) goto error; @@ -15715,6 +15768,18 @@ virDomainWatchdogDefFormat(virBufferPtr buf, return 0; } +static int virDomainPvpanicDefFormat(virBufferPtr buf, + virDomainPvpanicDefPtr def) +{ + if (def->ioport > 0) { + virBufferAsprintf(buf, " <pvpanic ioport='%#x'/>\n", + def->ioport); + } else { + virBufferAsprintf(buf, " <pvpanic/>\n"); + } + + return 0; +} static int virDomainRNGDefFormat(virBufferPtr buf, @@ -17138,6 +17203,9 @@ virDomainDefFormatInternal(virDomainDefPtr def, if (def->nvram) virDomainNVRAMDefFormat(buf, def->nvram, flags); + if (def->pvpanic) + virDomainPvpanicDefFormat(buf, def->pvpanic); + virBufferAddLit(buf, " </devices>\n"); virBufferAdjustIndent(buf, 2); diff --git a/src/conf/domain_conf.h b/src/conf/domain_conf.h index 4561ccc..9395852 100644 --- a/src/conf/domain_conf.h +++ b/src/conf/domain_conf.h @@ -126,6 +126,9 @@ typedef virDomainIdMapEntry *virDomainIdMapEntryPtr; typedef struct _virDomainIdMapDef virDomainIdMapDef; typedef virDomainIdMapDef *virDomainIdMapDefPtr; +typedef struct _virDomainPvpanicDef virDomainPvpanicDef; +typedef virDomainPvpanicDef *virDomainPvpanicDefPtr; + /* Flags for the 'type' field in virDomainDeviceDef */ typedef enum { VIR_DOMAIN_DEVICE_NONE = 0, @@ -1910,6 +1913,10 @@ struct _virDomainIdMapDef { }; +struct _virDomainPvpanicDef { + int ioport; +}; + void virBlkioDeviceWeightArrayClear(virBlkioDeviceWeightPtr deviceWeights, int ndevices); @@ -2061,6 +2068,7 @@ struct _virDomainDef { virSysinfoDefPtr sysinfo; virDomainRedirFilterDefPtr redirfilter; virDomainRNGDefPtr rng; + virDomainPvpanicDefPtr pvpanic; void *namespaceData; virDomainXMLNamespace ns; @@ -2204,6 +2212,7 @@ virDomainObjPtr virDomainObjListFindByName(virDomainObjListPtr doms, bool virDomainObjTaint(virDomainObjPtr obj, enum virDomainTaintFlags taint); +void virDomainPvpanicDefFree(virDomainPvpanicDefPtr pvpanic); void virDomainResourceDefFree(virDomainResourceDefPtr resource); void virDomainGraphicsDefFree(virDomainGraphicsDefPtr def); void virDomainInputDefFree(virDomainInputDefPtr def); -- 1.7.11.7
This patch will add -device pvpanic to qemu command line if user enables pvpanic in domain xml and the qemu version supports pvpanic. Signed-off-by: Hu Tao <hutao@cn.fujitsu.com> --- src/qemu/qemu_capabilities.c | 3 +++ src/qemu/qemu_capabilities.h | 2 ++ src/qemu/qemu_command.c | 10 ++++++++++ 3 files changed, 15 insertions(+) diff --git a/src/qemu/qemu_capabilities.c b/src/qemu/qemu_capabilities.c index 548b988..ae2c192 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 { @@ -1394,6 +1396,7 @@ struct virQEMUCapsStringFlags virQEMUCapsObjectTypes[] = { { "usb-storage", QEMU_CAPS_DEVICE_USB_STORAGE }, { "virtio-mmio", QEMU_CAPS_DEVICE_VIRTIO_MMIO }, { "ich9-intel-hda", QEMU_CAPS_DEVICE_ICH9_INTEL_HDA }, + { "pvpanic", QEMU_CAPS_DEVICE_PVPANIC }, }; static struct virQEMUCapsStringFlags virQEMUCapsObjectPropsVirtioBlk[] = { diff --git a/src/qemu/qemu_capabilities.h b/src/qemu/qemu_capabilities.h index 02d47c6..0abe0b9 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_DEVICE_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..6310bb2 100644 --- a/src/qemu/qemu_command.c +++ b/src/qemu/qemu_command.c @@ -9588,6 +9588,16 @@ qemuBuildCommandLine(virConnectPtr conn, goto error; } + if (def->pvpanic && + virQEMUCapsGet(qemuCaps, QEMU_CAPS_DEVICE_PVPANIC)) { + if (def->pvpanic->ioport > 0) { + virCommandAddArg(cmd, "-device"); + virCommandAddArgFormat(cmd, "pvpanic,ioport=%d", + def->pvpanic->ioport); + } else + virCommandAddArgList(cmd, "-device", "pvpanic", NULL); + } + if (mlock) { unsigned long long memKB; -- 1.7.11.7
On 12/02/13 07:11, Hu Tao wrote:
This patch will add -device pvpanic to qemu command line if user enables pvpanic in domain xml and the qemu version supports pvpanic.
Signed-off-by: Hu Tao <hutao@cn.fujitsu.com> --- src/qemu/qemu_capabilities.c | 3 +++ src/qemu/qemu_capabilities.h | 2 ++ src/qemu/qemu_command.c | 10 ++++++++++ 3 files changed, 15 insertions(+)
...
diff --git a/src/qemu/qemu_command.c b/src/qemu/qemu_command.c index 763417f..6310bb2 100644 --- a/src/qemu/qemu_command.c +++ b/src/qemu/qemu_command.c @@ -9588,6 +9588,16 @@ qemuBuildCommandLine(virConnectPtr conn, goto error; }
+ if (def->pvpanic && + virQEMUCapsGet(qemuCaps, QEMU_CAPS_DEVICE_PVPANIC)) { + if (def->pvpanic->ioport > 0) { + virCommandAddArg(cmd, "-device"); + virCommandAddArgFormat(cmd, "pvpanic,ioport=%d", + def->pvpanic->ioport); + } else + virCommandAddArgList(cmd, "-device", "pvpanic", NULL);
If pvpanic is requested, but not available in qemu, libvirt should error out instead of silently starting the VM without the device.
+ } + if (mlock) { unsigned long long memKB;
Also as said in review of 1/2. You need to add tests for the new device. Peter
On Mon, Dec 02, 2013 at 11:15:17AM +0100, Peter Krempa wrote:
On 12/02/13 07:11, Hu Tao wrote:
This patch will add -device pvpanic to qemu command line if user enables pvpanic in domain xml and the qemu version supports pvpanic.
Signed-off-by: Hu Tao <hutao@cn.fujitsu.com> --- src/qemu/qemu_capabilities.c | 3 +++ src/qemu/qemu_capabilities.h | 2 ++ src/qemu/qemu_command.c | 10 ++++++++++ 3 files changed, 15 insertions(+)
...
diff --git a/src/qemu/qemu_command.c b/src/qemu/qemu_command.c index 763417f..6310bb2 100644 --- a/src/qemu/qemu_command.c +++ b/src/qemu/qemu_command.c @@ -9588,6 +9588,16 @@ qemuBuildCommandLine(virConnectPtr conn, goto error; }
+ if (def->pvpanic && + virQEMUCapsGet(qemuCaps, QEMU_CAPS_DEVICE_PVPANIC)) { + if (def->pvpanic->ioport > 0) { + virCommandAddArg(cmd, "-device"); + virCommandAddArgFormat(cmd, "pvpanic,ioport=%d", + def->pvpanic->ioport); + } else + virCommandAddArgList(cmd, "-device", "pvpanic", NULL);
If pvpanic is requested, but not available in qemu, libvirt should error out instead of silently starting the VM without the device.
+ } + if (mlock) { unsigned long long memKB;
Also as said in review of 1/2. You need to add tests for the new device.
Thanks.
On 12/01/2013 11:11 PM, Hu Tao wrote:
This patch will add -device pvpanic to qemu command line if user enables pvpanic in domain xml and the qemu version supports pvpanic.
Signed-off-by: Hu Tao <hutao@cn.fujitsu.com> --- src/qemu/qemu_capabilities.c | 3 +++ src/qemu/qemu_capabilities.h | 2 ++ src/qemu/qemu_command.c | 10 ++++++++++ 3 files changed, 15 insertions(+)
In addition to Peter's comments,
+++ 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_DEVICE_PVPANIC = 160, /* -device pvpanic */
Alignment looks odd here.
+ if (def->pvpanic && + virQEMUCapsGet(qemuCaps, QEMU_CAPS_DEVICE_PVPANIC)) { + if (def->pvpanic->ioport > 0) {
Again, is port 0 a valid port (given that you initialized it to -1)? I know we haven't been doing a good job of domxml-from-native, but in this particular case, can we also fix the command line parser to turn '-device pvpanic' into the appropriate pvpanic device allocation?
+ } else
Style. If you used {} for 'if', you must also use it for 'else' (I'm still not sure how to enforce it by syntax-check, but it's on my list of things that would be nice to have). -- Eric Blake eblake redhat com +1-919-301-3266 Libvirt virtualization library http://libvirt.org
On Mon, Dec 02, 2013 at 02:38:09PM -0700, Eric Blake wrote:
On 12/01/2013 11:11 PM, Hu Tao wrote:
This patch will add -device pvpanic to qemu command line if user enables pvpanic in domain xml and the qemu version supports pvpanic.
Signed-off-by: Hu Tao <hutao@cn.fujitsu.com> --- src/qemu/qemu_capabilities.c | 3 +++ src/qemu/qemu_capabilities.h | 2 ++ src/qemu/qemu_command.c | 10 ++++++++++ 3 files changed, 15 insertions(+)
In addition to Peter's comments,
+++ 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_DEVICE_PVPANIC = 160, /* -device pvpanic */
Alignment looks odd here.
It aligns as most of the enums do. It doesn't look alike the above two because their names are too long.
+ if (def->pvpanic && + virQEMUCapsGet(qemuCaps, QEMU_CAPS_DEVICE_PVPANIC)) { + if (def->pvpanic->ioport > 0) {
Again, is port 0 a valid port (given that you initialized it to -1)?
I know we haven't been doing a good job of domxml-from-native, but in this particular case, can we also fix the command line parser to turn '-device pvpanic' into the appropriate pvpanic device allocation?
In the case of native->xml, I think we'd better to keep port 0 in xml. I'll fix it.
+ } else
Style. If you used {} for 'if', you must also use it for 'else' (I'm still not sure how to enforce it by syntax-check, but it's on my list of things that would be nice to have).
Thanks.
On 12/02/13 07:11, Hu Tao wrote:
This patch adds a new xml element devices/pvpanic to support qemu device pvpanic. It can be used to receive guest panic notification.
Signed-off-by: Hu Tao <hutao@cn.fujitsu.com> --- docs/formatdomain.html.in | 25 +++++++++++++++++ src/conf/domain_conf.c | 68 +++++++++++++++++++++++++++++++++++++++++++++++ src/conf/domain_conf.h | 9 +++++++ 3 files changed, 102 insertions(+)
A few issues I see at first glance: 1) you didn't add ABI compatibility check for the pvpanic device 2) XML->XML tests are missing 3) RNG schemas for the new element are missing 4) XML->qemu commandline tests are missing (in 2/2)
diff --git a/docs/formatdomain.html.in b/docs/formatdomain.html.in index 1850a2b..0a72baa 100644 --- a/docs/formatdomain.html.in +++ b/docs/formatdomain.html.in @@ -5080,6 +5080,31 @@ qemu-kvm -net nic,model=? /dev/null </dd> </dl>
+ <h4><a name="elementsPvpanic">pvpanic device</a></h4> + <p> + pvpanic device enables libvirt to receive panic notification from a QEMU + guest. + <span class="since">Since 1.3.0, QEMU and KVM only</span>
1.3.0? the since tag is supposed to contain a libvirt version. 1.3.0 will not happen that soon. 1.2.1 is what you are looking for.
+ </p> + <p> + Example: usage of pvpanic configuration + </p> +<pre> + ... + <devices> + <pvpanic ioport='0x505'/> + </devices> + ... +</pre> + <dl> + <dt><code>ioport</code></dt> + <dd> + <p> + ioport used by pvpanic. + </p> + </dd> + </dl> + <h3><a name="seclabel">Security label</a></h3>
<p> diff --git a/src/conf/domain_conf.c b/src/conf/domain_conf.c index 140eb80..1b8f66f 100644 --- a/src/conf/domain_conf.c +++ b/src/conf/domain_conf.c
...
@@ -15715,6 +15768,18 @@ virDomainWatchdogDefFormat(virBufferPtr buf, return 0; }
+static int virDomainPvpanicDefFormat(virBufferPtr buf, + virDomainPvpanicDefPtr def) +{ + if (def->ioport > 0) { + virBufferAsprintf(buf, " <pvpanic ioport='%#x'/>\n", + def->ioport); + } else { + virBufferAsprintf(buf, " <pvpanic/>\n");
Would break syntax-check. For static strings use virBufferAddLit.
+ } + + return 0; +}
static int virDomainRNGDefFormat(virBufferPtr buf,
On Mon, Dec 02, 2013 at 11:13:33AM +0100, Peter Krempa wrote:
On 12/02/13 07:11, Hu Tao wrote:
This patch adds a new xml element devices/pvpanic to support qemu device pvpanic. It can be used to receive guest panic notification.
Signed-off-by: Hu Tao <hutao@cn.fujitsu.com> --- docs/formatdomain.html.in | 25 +++++++++++++++++ src/conf/domain_conf.c | 68 +++++++++++++++++++++++++++++++++++++++++++++++ src/conf/domain_conf.h | 9 +++++++ 3 files changed, 102 insertions(+)
A few issues I see at first glance:
1) you didn't add ABI compatibility check for the pvpanic device 2) XML->XML tests are missing 3) RNG schemas for the new element are missing 4) XML->qemu commandline tests are missing (in 2/2)
Thanks, I'll add the missing parts in next version.
diff --git a/docs/formatdomain.html.in b/docs/formatdomain.html.in index 1850a2b..0a72baa 100644 --- a/docs/formatdomain.html.in +++ b/docs/formatdomain.html.in @@ -5080,6 +5080,31 @@ qemu-kvm -net nic,model=? /dev/null </dd> </dl>
+ <h4><a name="elementsPvpanic">pvpanic device</a></h4> + <p> + pvpanic device enables libvirt to receive panic notification from a QEMU + guest. + <span class="since">Since 1.3.0, QEMU and KVM only</span>
1.3.0? the since tag is supposed to contain a libvirt version. 1.3.0 will not happen that soon. 1.2.1 is what you are looking for.
OK.
+ </p> + <p> + Example: usage of pvpanic configuration + </p> +<pre> + ... + <devices> + <pvpanic ioport='0x505'/> + </devices> + ... +</pre> + <dl> + <dt><code>ioport</code></dt> + <dd> + <p> + ioport used by pvpanic. + </p> + </dd> + </dl> + <h3><a name="seclabel">Security label</a></h3>
<p> diff --git a/src/conf/domain_conf.c b/src/conf/domain_conf.c index 140eb80..1b8f66f 100644 --- a/src/conf/domain_conf.c +++ b/src/conf/domain_conf.c
...
@@ -15715,6 +15768,18 @@ virDomainWatchdogDefFormat(virBufferPtr buf, return 0; }
+static int virDomainPvpanicDefFormat(virBufferPtr buf, + virDomainPvpanicDefPtr def) +{ + if (def->ioport > 0) { + virBufferAsprintf(buf, " <pvpanic ioport='%#x'/>\n", + def->ioport); + } else { + virBufferAsprintf(buf, " <pvpanic/>\n");
Would break syntax-check. For static strings use virBufferAddLit.
OK.
+ } + + return 0; +}
static int virDomainRNGDefFormat(virBufferPtr buf,
On 12/01/2013 11:11 PM, Hu Tao wrote:
This patch adds a new xml element devices/pvpanic to support qemu device pvpanic. It can be used to receive guest panic notification.
Signed-off-by: Hu Tao <hutao@cn.fujitsu.com> --- docs/formatdomain.html.in | 25 +++++++++++++++++ src/conf/domain_conf.c | 68 +++++++++++++++++++++++++++++++++++++++++++++++ src/conf/domain_conf.h | 9 +++++++ 3 files changed, 102 insertions(+)
In addition to Peter's review: when sending a series, it helps to include a 0/2 cover letter (git send-email --cover-letter).
diff --git a/docs/formatdomain.html.in b/docs/formatdomain.html.in index 1850a2b..0a72baa 100644 --- a/docs/formatdomain.html.in +++ b/docs/formatdomain.html.in @@ -5080,6 +5080,31 @@ qemu-kvm -net nic,model=? /dev/null </dd> </dl>
+ <h4><a name="elementsPvpanic">pvpanic device</a></h4>
pvpanic is a qemu term, but I could see the feasibility of other hypervisors having a paravirt device with a sole purpose of notifying the host about panics. Do we want to come up with a more generic name?
+ <devices> + <pvpanic ioport='0x505'/> + </devices> + ... +</pre> + <dl> + <dt><code>ioport</code></dt> + <dd> + <p> + ioport used by pvpanic.
Probably worth documenting that 0x505 is the default port, and that most users don't need to specify the ioport.
+ ioport = virXMLPropString(node, "ioport"); + if (!ioport) { + pvpanic->ioport = -1; + } else { + if (virStrToLong_i(ioport, NULL, 0, &pvpanic->ioport) < 0) { + virReportError(VIR_ERR_INTERNAL_ERROR, "%s",
VIR_ERR_INTERNAL_ERROR is probably the wrong type, since this is easily triggered by a user. I know it's copy and paste from other code, but these days, VIR_ERR_XML_ERROR is preferred in new code reporting a parse error. Should virDomainDeviceType be enhanced to include this device type? And if so, you need to modify at least virDomainDeviceDefFree() to handle the new enum value.
+static int virDomainPvpanicDefFormat(virBufferPtr buf, + virDomainPvpanicDefPtr def) +{ + if (def->ioport > 0) {
Isn't this an off-by-one if someone explicitly requests port 0 (since your parser initializes to -1 when left unspecified)? -- Eric Blake eblake redhat com +1-919-301-3266 Libvirt virtualization library http://libvirt.org
On Mon, Dec 02, 2013 at 02:34:44PM -0700, Eric Blake wrote:
On 12/01/2013 11:11 PM, Hu Tao wrote:
This patch adds a new xml element devices/pvpanic to support qemu device pvpanic. It can be used to receive guest panic notification.
Signed-off-by: Hu Tao <hutao@cn.fujitsu.com> --- docs/formatdomain.html.in | 25 +++++++++++++++++ src/conf/domain_conf.c | 68 +++++++++++++++++++++++++++++++++++++++++++++++ src/conf/domain_conf.h | 9 +++++++ 3 files changed, 102 insertions(+)
In addition to Peter's review:
when sending a series, it helps to include a 0/2 cover letter (git send-email --cover-letter).
OK, will include it in next version.
diff --git a/docs/formatdomain.html.in b/docs/formatdomain.html.in index 1850a2b..0a72baa 100644 --- a/docs/formatdomain.html.in +++ b/docs/formatdomain.html.in @@ -5080,6 +5080,31 @@ qemu-kvm -net nic,model=? /dev/null </dd> </dl>
+ <h4><a name="elementsPvpanic">pvpanic device</a></h4>
pvpanic is a qemu term, but I could see the feasibility of other hypervisors having a paravirt device with a sole purpose of notifying the host about panics. Do we want to come up with a more generic name?
I'd call it 'panic notification', but it doesn't sound like a device name. Advise?
+ <devices> + <pvpanic ioport='0x505'/> + </devices> + ... +</pre> + <dl> + <dt><code>ioport</code></dt> + <dd> + <p> + ioport used by pvpanic.
Probably worth documenting that 0x505 is the default port, and that most users don't need to specify the ioport.
OK.
+ ioport = virXMLPropString(node, "ioport"); + if (!ioport) { + pvpanic->ioport = -1; + } else { + if (virStrToLong_i(ioport, NULL, 0, &pvpanic->ioport) < 0) { + virReportError(VIR_ERR_INTERNAL_ERROR, "%s",
VIR_ERR_INTERNAL_ERROR is probably the wrong type, since this is easily triggered by a user. I know it's copy and paste from other code, but these days, VIR_ERR_XML_ERROR is preferred in new code reporting a parse error.
Should virDomainDeviceType be enhanced to include this device type? And if so, you need to modify at least virDomainDeviceDefFree() to handle the new enum value.
OK.
+static int virDomainPvpanicDefFormat(virBufferPtr buf, + virDomainPvpanicDefPtr def) +{ + if (def->ioport > 0) {
Isn't this an off-by-one if someone explicitly requests port 0 (since your parser initializes to -1 when left unspecified)?
port 0 means disable the device, so there is no need to add it when port is 0. But if you'd prefer to let the device handle port itself, then it's OK to add it in the case. -- Regards, Hu Tao
On 12/02/2013 06:32 PM, Hu Tao wrote:
On Mon, Dec 02, 2013 at 02:34:44PM -0700, Eric Blake wrote:
On 12/01/2013 11:11 PM, Hu Tao wrote:
This patch adds a new xml element devices/pvpanic to support qemu device pvpanic. It can be used to receive guest panic notification.
Signed-off-by: Hu Tao <hutao@cn.fujitsu.com> --- docs/formatdomain.html.in | 25 +++++++++++++++++ src/conf/domain_conf.c | 68 +++++++++++++++++++++++++++++++++++++++++++++++ src/conf/domain_conf.h | 9 +++++++ 3 files changed, 102 insertions(+)
+static int virDomainPvpanicDefFormat(virBufferPtr buf, + virDomainPvpanicDefPtr def) +{ + if (def->ioport > 0) {
Isn't this an off-by-one if someone explicitly requests port 0 (since your parser initializes to -1 when left unspecified)?
port 0 means disable the device, so there is no need to add it when port is 0. But if you'd prefer to let the device handle port itself, then it's OK to add it in the case.
Then don't initialize port to -1. Instead, in your parser reject an explicit setting of 0, and use the default of 0 to mean unspecified port. -- Eric Blake eblake redhat com +1-919-301-3266 Libvirt virtualization library http://libvirt.org
On Mon, Dec 02, 2013 at 02:34:44PM -0700, Eric Blake wrote:
On 12/01/2013 11:11 PM, Hu Tao wrote:
This patch adds a new xml element devices/pvpanic to support qemu device pvpanic. It can be used to receive guest panic notification.
Signed-off-by: Hu Tao <hutao@cn.fujitsu.com> --- docs/formatdomain.html.in | 25 +++++++++++++++++ src/conf/domain_conf.c | 68 +++++++++++++++++++++++++++++++++++++++++++++++ src/conf/domain_conf.h | 9 +++++++ 3 files changed, 102 insertions(+)
In addition to Peter's review:
when sending a series, it helps to include a 0/2 cover letter (git send-email --cover-letter).
diff --git a/docs/formatdomain.html.in b/docs/formatdomain.html.in index 1850a2b..0a72baa 100644 --- a/docs/formatdomain.html.in +++ b/docs/formatdomain.html.in @@ -5080,6 +5080,31 @@ qemu-kvm -net nic,model=? /dev/null </dd> </dl>
+ <h4><a name="elementsPvpanic">pvpanic device</a></h4>
pvpanic is a qemu term, but I could see the feasibility of other hypervisors having a paravirt device with a sole purpose of notifying the host about panics. Do we want to come up with a more generic name?
Give it a generic name is easy, but what about attributes? different hypervisors may have different paravirt devices with different attributes, we can't just mix attributes of unrelated devices into one generic device. Make the devices concrete and accept/reject it if hypervisors recognize it or not is better.
On 12/03/2013 11:31 PM, Hu Tao wrote:
pvpanic is a qemu term, but I could see the feasibility of other hypervisors having a paravirt device with a sole purpose of notifying the host about panics. Do we want to come up with a more generic name?
Give it a generic name is easy, but what about attributes? different hypervisors may have different paravirt devices with different attributes, we can't just mix attributes of unrelated devices into one generic device. Make the devices concrete and accept/reject it if hypervisors recognize it or not is better.
Then do what we've done in the past - separate the presence of a type of device from the driver-specific attributes, something like: <devices> <panic> <driver type='qemu' ioport='0x505'/> </panic> ... where we use the type='' field of <driver> to toggle between a discriminated union of any other per-hypervisor specific attributes. By default, you don't need a <driver> subelement; requesting <panic/> is sufficient to use the defaults for the current hypervisor. Dan, do you have any thoughts on the best representation to use? Or is Hu's original proposal of: <pvpanic ioport='0x505'/> sufficient? -- Eric Blake eblake redhat com +1-919-301-3266 Libvirt virtualization library http://libvirt.org
On Wed, Dec 04, 2013 at 08:39:58AM -0700, Eric Blake wrote:
On 12/03/2013 11:31 PM, Hu Tao wrote:
pvpanic is a qemu term, but I could see the feasibility of other hypervisors having a paravirt device with a sole purpose of notifying the host about panics. Do we want to come up with a more generic name?
Give it a generic name is easy, but what about attributes? different hypervisors may have different paravirt devices with different attributes, we can't just mix attributes of unrelated devices into one generic device. Make the devices concrete and accept/reject it if hypervisors recognize it or not is better.
Then do what we've done in the past - separate the presence of a type of device from the driver-specific attributes, something like:
<devices> <panic> <driver type='qemu' ioport='0x505'/> </panic> ...
where we use the type='' field of <driver> to toggle between a discriminated union of any other per-hypervisor specific attributes. By default, you don't need a <driver> subelement; requesting <panic/> is sufficient to use the defaults for the current hypervisor.
Dan, do you have any thoughts on the best representation to use? Or is Hu's original proposal of:
<pvpanic ioport='0x505'/>
I'm not a fan of doing a special case attribute for 'ioport' - this is something something that should be part of an <address> element, since ioport numbers are a generic addressing concept for many devices. eg ISA serial / parallel ports have IRQ / IO ports IIUC. 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 12/04/2013 08:42 AM, Daniel P. Berrange wrote:
Dan, do you have any thoughts on the best representation to use? Or is Hu's original proposal of:
<pvpanic ioport='0x505'/>
I'm not a fan of doing a special case attribute for 'ioport' - this is something something that should be part of an <address> element, since ioport numbers are a generic addressing concept for many devices. eg ISA serial / parallel ports have IRQ / IO ports IIUC.
So something more like: <pvpanic> <address type='ioport' slot='0x505'/> </pvpanic> and introducing a new type='ioport' namespace into the <address> XML since it is yet another numbering system for guest-visible addressing? -- Eric Blake eblake redhat com +1-919-301-3266 Libvirt virtualization library http://libvirt.org
On Wed, Dec 04, 2013 at 08:46:53AM -0700, Eric Blake wrote:
On 12/04/2013 08:42 AM, Daniel P. Berrange wrote:
Dan, do you have any thoughts on the best representation to use? Or is Hu's original proposal of:
<pvpanic ioport='0x505'/>
I'm not a fan of doing a special case attribute for 'ioport' - this is something something that should be part of an <address> element, since ioport numbers are a generic addressing concept for many devices. eg ISA serial / parallel ports have IRQ / IO ports IIUC.
So something more like:
<pvpanic> <address type='ioport' slot='0x505'/> </pvpanic>
and introducing a new type='ioport' namespace into the <address> XML since it is yet another numbering system for guest-visible addressing?
Yes, I'm not sure I'd call the type 'ioport' - the address type reflects the bus/controller type that the device is associated with. What is the "bus" type that a pvpanic device is attached to ? Is it a ISA bus device, or is it a "platform" device or something else ? eg it might be appropriate to use <address type='platform' ioport='0x666'/> 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, Dec 04, 2013 at 03:53:17PM +0000, Daniel P. Berrange wrote:
On Wed, Dec 04, 2013 at 08:46:53AM -0700, Eric Blake wrote:
On 12/04/2013 08:42 AM, Daniel P. Berrange wrote:
Dan, do you have any thoughts on the best representation to use? Or is Hu's original proposal of:
<pvpanic ioport='0x505'/>
I'm not a fan of doing a special case attribute for 'ioport' - this is something something that should be part of an <address> element, since ioport numbers are a generic addressing concept for many devices. eg ISA serial / parallel ports have IRQ / IO ports IIUC.
So something more like:
<pvpanic> <address type='ioport' slot='0x505'/> </pvpanic>
and introducing a new type='ioport' namespace into the <address> XML since it is yet another numbering system for guest-visible addressing?
Yes, I'm not sure I'd call the type 'ioport' - the address type reflects the bus/controller type that the device is associated with. What is the "bus" type that a pvpanic device is attached to ? Is it a ISA bus device, or is it a "platform" device or something else ? eg it might be appropriate to use
<address type='platform' ioport='0x666'/>
It's an ISA device. So the address should be: <address type='isa' ioport='0x505'/> ?
On Thu, Dec 05, 2013 at 10:41:13AM +0800, Hu Tao wrote:
On Wed, Dec 04, 2013 at 03:53:17PM +0000, Daniel P. Berrange wrote:
On Wed, Dec 04, 2013 at 08:46:53AM -0700, Eric Blake wrote:
On 12/04/2013 08:42 AM, Daniel P. Berrange wrote:
Dan, do you have any thoughts on the best representation to use? Or is Hu's original proposal of:
<pvpanic ioport='0x505'/>
I'm not a fan of doing a special case attribute for 'ioport' - this is something something that should be part of an <address> element, since ioport numbers are a generic addressing concept for many devices. eg ISA serial / parallel ports have IRQ / IO ports IIUC.
So something more like:
<pvpanic> <address type='ioport' slot='0x505'/> </pvpanic>
and introducing a new type='ioport' namespace into the <address> XML since it is yet another numbering system for guest-visible addressing?
Yes, I'm not sure I'd call the type 'ioport' - the address type reflects the bus/controller type that the device is associated with. What is the "bus" type that a pvpanic device is attached to ? Is it a ISA bus device, or is it a "platform" device or something else ? eg it might be appropriate to use
<address type='platform' ioport='0x666'/>
It's an ISA device. So the address should be:
<address type='isa' ioport='0x505'/>
Ok. It looks like it does not require an IRQ line though IIUC. For the general ISA address type though, we want to represent both ioport and IRQ values. So I guess we need the IRQ attribute to be optional in some manner. 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 Thu, Dec 05, 2013 at 10:07:40AM +0000, Daniel P. Berrange wrote:
On Thu, Dec 05, 2013 at 10:41:13AM +0800, Hu Tao wrote:
On Wed, Dec 04, 2013 at 03:53:17PM +0000, Daniel P. Berrange wrote:
On Wed, Dec 04, 2013 at 08:46:53AM -0700, Eric Blake wrote:
On 12/04/2013 08:42 AM, Daniel P. Berrange wrote:
Dan, do you have any thoughts on the best representation to use? Or is Hu's original proposal of:
<pvpanic ioport='0x505'/>
I'm not a fan of doing a special case attribute for 'ioport' - this is something something that should be part of an <address> element, since ioport numbers are a generic addressing concept for many devices. eg ISA serial / parallel ports have IRQ / IO ports IIUC.
So something more like:
<pvpanic> <address type='ioport' slot='0x505'/> </pvpanic>
and introducing a new type='ioport' namespace into the <address> XML since it is yet another numbering system for guest-visible addressing?
Yes, I'm not sure I'd call the type 'ioport' - the address type reflects the bus/controller type that the device is associated with. What is the "bus" type that a pvpanic device is attached to ? Is it a ISA bus device, or is it a "platform" device or something else ? eg it might be appropriate to use
<address type='platform' ioport='0x666'/>
It's an ISA device. So the address should be:
<address type='isa' ioport='0x505'/>
Ok. It looks like it does not require an IRQ line though IIUC. For the general ISA address type though, we want to represent both ioport and IRQ values. So I guess we need the IRQ attribute to be optional in some manner.
Just to confirm it, so the general ISA address looks like: <address type='isa' ioport='0xaaa' irq='123'/> ? I checked attributes of several qemu ISA devices, iobase and irq are common, but some device(isa-ide) has iobase2. Should we handle it as well?
On Thu, Dec 05, 2013 at 06:47:51PM +0800, Hu Tao wrote:
On Thu, Dec 05, 2013 at 10:07:40AM +0000, Daniel P. Berrange wrote:
On Thu, Dec 05, 2013 at 10:41:13AM +0800, Hu Tao wrote:
On Wed, Dec 04, 2013 at 03:53:17PM +0000, Daniel P. Berrange wrote:
On Wed, Dec 04, 2013 at 08:46:53AM -0700, Eric Blake wrote:
On 12/04/2013 08:42 AM, Daniel P. Berrange wrote:
> Dan, do you have any thoughts on the best representation to use? Or is > Hu's original proposal of: > > <pvpanic ioport='0x505'/>
I'm not a fan of doing a special case attribute for 'ioport' - this is something something that should be part of an <address> element, since ioport numbers are a generic addressing concept for many devices. eg ISA serial / parallel ports have IRQ / IO ports IIUC.
So something more like:
<pvpanic> <address type='ioport' slot='0x505'/> </pvpanic>
and introducing a new type='ioport' namespace into the <address> XML since it is yet another numbering system for guest-visible addressing?
Yes, I'm not sure I'd call the type 'ioport' - the address type reflects the bus/controller type that the device is associated with. What is the "bus" type that a pvpanic device is attached to ? Is it a ISA bus device, or is it a "platform" device or something else ? eg it might be appropriate to use
<address type='platform' ioport='0x666'/>
It's an ISA device. So the address should be:
<address type='isa' ioport='0x505'/>
Ok. It looks like it does not require an IRQ line though IIUC. For the general ISA address type though, we want to represent both ioport and IRQ values. So I guess we need the IRQ attribute to be optional in some manner.
Just to confirm it, so the general ISA address looks like:
<address type='isa' ioport='0xaaa' irq='123'/> ?
Yes, looks good.
I checked attributes of several qemu ISA devices, iobase and irq are common, but some device(isa-ide) has iobase2. Should we handle it as well?
I guess we can just have an optional ioport2 attribute too if we ever need to support that devices. i'd just ignore it for now though to keep life simple. 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 :|
participants (4)
-
Daniel P. Berrange -
Eric Blake -
Hu Tao -
Peter Krempa