[libvirt] Fwd: Re: [PATCH v2 3/3] conf: qemu: Add support for more HyperV Enlightenment features

On 03/10/2016 07:43 AM, Nikolay Shirokovskiy wrote: > From: Maxim Nestratov <mnestratov(a)virtuozzo.com&gt; > > This patch adds support for "vpindex", "runtime", "synic" and > "stimer" features available in qemu 2.5+. Should "vendor_id" be part of this list?

> - When Hyper-V "vpindex" is on, guest can use MSR HV_X64_MSR_VP_INDEX > to get virtual processor ID. > > - Hyper-V "runtime" enlightement feature allows to use MSR > HV_X64_MSR_VP_RUNTIME to get the time the virtual processor consumes > running guest code, as well as the time the hypervisor spends running > code on behalf of that guest. > > - Hyper-V "synic" stands for Synthetic Interrupt Controller, which is > lapic extension controlled via MSRs. > > - Hyper-V "stimer" switches on Hyper-V SynIC timers MSR's support. > Guest can setup and use fired by host events (SynIC interrupt and > appropriate timer expiration message) as guest clock events > > - Hyper-V "reset" allows guest to reset VM. > > - Hyper-V "vendor_id" exposes hypervisor vendor id to guest. > > Signed-off-by: Nikolay Shirokovskiy <nshirokovskiy(a)virtuozzo.com&gt; > --- > docs/formatdomain.html.in | 41 +++++++++++++ > docs/schemas/domaincommon.rng | 37 ++++++++++++ > src/conf/domain_conf.c | 69 +++++++++++++++++++++- > src/conf/domain_conf.h | 9 +++ > src/qemu/qemu_command.c | 11 ++++ > src/qemu/qemu_parse_command.c | 20 +++++++ > tests/qemuxml2argvdata/qemuxml2argv-hyperv-off.xml | 6 ++ > tests/qemuxml2argvdata/qemuxml2argv-hyperv.args | 3 +- > tests/qemuxml2argvdata/qemuxml2argv-hyperv.xml | 6 ++ > .../qemuxml2xmlout-hyperv-off.xml | 6 ++ > tests/qemuxml2xmloutdata/qemuxml2xmlout-hyperv.xml | 6 ++ > 11 files changed, 212 insertions(+), 2 deletions(-) > > diff --git a/docs/formatdomain.html.in b/docs/formatdomain.html.in > index 4e49228..ce8b43a 100644 > --- a/docs/formatdomain.html.in > +++ b/docs/formatdomain.html.in > @@ -1460,6 +1460,11 @@ > &lt;relaxed state='on'/&gt; > &lt;vapic state='on'/&gt; > &lt;spinlocks state='on' retries='4096'/&gt; > + &lt;vpindex state='on'/&gt; > + &lt;runtime state='on'/&gt; > + &lt;synic state='on'/&gt; > + &lt;reset state='on'/&gt; > + &lt;vendor_id state='on' value='KVM Hv'/&gt; > &lt;/hyperv&gt; > &lt;kvm&gt; > &lt;hidden state='on'/&gt; > @@ -1535,6 +1540,42 @@ > <td>on, off; retries - at least 4095</td> > <td><span class="since">1.1.0 (QEMU only)</span></td> > </tr> > + <tr> > + <td>vpindex</td> > + <td>Virtual processor index</td> > + <td> on, off</td> > + <td><span class="since">1.3.3 (QEMU only)</span></td> > + </tr> > + <tr> > + <td>runtime</td> > + <td>Processor time spent on running guest code and on behalf of guest code</td> > + <td> on, off</td> > + <td><span class="since">1.3.3 (QEMU only)</span></td> > + </tr> > + <tr> > + <td>synic</td> > + <td>Enable Synthetic Interrupt Controller (SyNIC)</td> > + <td> on, off</td> > + <td><span class="since">1.3.3 (QEMU only)</span></td> > + </tr> > + <tr> > + <td>stimer</td> > + <td>Enable SyNIC timers</td> > + <td> on, off</td> > + <td><span class="since">1.3.3 (QEMU only)</span></td> > + </tr> > + <tr> > + <td>reset</td> > + <td>Enable hypervisor reset</td> > + <td> on, off</td> > + <td><span class="since">1.3.3 (QEMU only)</span></td> > + </tr> > + <tr> > + <td>vendor_id</td> > + <td>Set hypervisor vendor id</td> > + <td>on, off; value - string, up to 12 characters</td> > + <td><span class="since">1.3.3 (QEMU only)</span></td> > + </tr> s/QEMU only/requires QEMU 1.5/g or s/QEMU only/QEMU 1.5 or later)/g Do you have a preference? The first one usually implies the "or later". Since there's no caps check for the feature, we should at least document when they were added.

I see that 'hv-crash' was added in 1.5 (qemu commit id 'f2a53c9e'), but not added here - any reason why not?

In order to be consistent, the others probably should be updated as well - each was qemu 2.0 or later. There was also 'hv-time' added in 2.0 (qemu commit id '48a5f3bcb') - should that be added to (perhaps a separate patch though).

> </table> > </dd> > <dt><code>pvspinlock</code></dt> > diff --git a/docs/schemas/domaincommon.rng b/docs/schemas/domaincommon.rng > index 6ca937c..8861a22 100644 > --- a/docs/schemas/domaincommon.rng > +++ b/docs/schemas/domaincommon.rng > @@ -4885,6 +4885,43 @@ > </optional> > </element> > </optional> > + <optional> > + <element name="vpindex"> > + <ref name="featurestate"/> > + </element> > + </optional> > + <optional> > + <element name="runtime"> > + <ref name="featurestate"/> > + </element> > + </optional> > + <optional> > + <element name="synic"> > + <ref name="featurestate"/> > + </element> > + </optional> > + <optional> > + <element name="stimer"> > + <ref name="featurestate"/> > + </element> > + </optional> > + <optional> > + <element name="reset"> > + <ref name="featurestate"/> > + </element> > + </optional> > + <optional> > + <element name="vendor_id"> > + <ref name="featurestate"/> > + <optional> > + <attribute name="value"> > + <data type="string"> > + <param name='pattern'>[^,]{0,12}</param> > + </data> > + </attribute> > + </optional> > + </element> > + </optional> > </interleave> > </element> > </define> > diff --git a/src/conf/domain_conf.c b/src/conf/domain_conf.c > index 6799b86..8ba2e2e 100644 > --- a/src/conf/domain_conf.c > +++ b/src/conf/domain_conf.c > @@ -145,7 +145,13 @@ VIR_ENUM_IMPL(virDomainCapabilitiesPolicy, VIR_DOMAIN_CAPABILITIES_POLICY_LAST, > VIR_ENUM_IMPL(virDomainHyperv, VIR_DOMAIN_HYPERV_LAST, > "relaxed", > "vapic", > - "spinlocks") > + "spinlocks", > + "vpindex", > + "runtime", > + "synic", > + "stimer", > + "reset", > + "vendor_id") > > VIR_ENUM_IMPL(virDomainKVM, VIR_DOMAIN_KVM_LAST, > "hidden") > @@ -2594,6 +2600,7 @@ void virDomainDefFree(virDomainDefPtr def) > VIR_FREE(def->emulator); > VIR_FREE(def->description); > VIR_FREE(def->title); > + VIR_FREE(def->hyperv_vendor_id); > > virBlkioDeviceArrayClear(def->blkio.devices, > def->blkio.ndevices); > @@ -15606,6 +15613,11 @@ virDomainDefParseXML(xmlDocPtr xml, > switch ((virDomainHyperv) feature) { > case VIR_DOMAIN_HYPERV_RELAXED: > case VIR_DOMAIN_HYPERV_VAPIC: > + case VIR_DOMAIN_HYPERV_VPINDEX: > + case VIR_DOMAIN_HYPERV_RUNTIME: > + case VIR_DOMAIN_HYPERV_SYNIC: > + case VIR_DOMAIN_HYPERV_STIMER: > + case VIR_DOMAIN_HYPERV_RESET: > break; > > case VIR_DOMAIN_HYPERV_SPINLOCKS: > @@ -15627,6 +15639,33 @@ virDomainDefParseXML(xmlDocPtr xml, > } > break; > > + case VIR_DOMAIN_HYPERV_VENDOR_ID: > + if (value != VIR_TRISTATE_SWITCH_ON) > + break; > + > + if (!(def->hyperv_vendor_id = virXPathString("string(./@value)", > + ctxt))) { ^ Small alignment issue (over one more) > + virReportError(VIR_ERR_XML_ERROR, "%s", > + _("missing 'value' attribute for " > + "HyperV feature 'vendor_id'")); > + goto error; > + } > + > + if (strlen(def->hyperv_vendor_id) > VIR_DOMAIN_HYPERV_VENDOR_ID_MAX) { > + virReportError(VIR_ERR_XML_ERROR, > + _("HyperV vendor_id value must not be more " > + "than %d charachters long."), s/charachters long/characters/ > + VIR_DOMAIN_HYPERV_VENDOR_ID_MAX); > + goto error; > + } > + > + /* ensure that the string can be passed to qemu*/ ^^ s/"qemu*/"/"qemu */"/ > + if (strchr(def->hyperv_vendor_id, ',')) { > + virReportError(VIR_ERR_XML_ERROR, "%s", > + _("HyperV vendor_id value is invalid")); > + goto error; > + } > + > /* coverity[dead_error_begin] */ > case VIR_DOMAIN_HYPERV_LAST: > break; > @@ -17629,6 +17668,11 @@ virDomainDefFeaturesCheckABIStability(virDomainDefPtr src, > switch ((virDomainHyperv) i) { > case VIR_DOMAIN_HYPERV_RELAXED: > case VIR_DOMAIN_HYPERV_VAPIC: > + case VIR_DOMAIN_HYPERV_VPINDEX: > + case VIR_DOMAIN_HYPERV_RUNTIME: > + case VIR_DOMAIN_HYPERV_SYNIC: > + case VIR_DOMAIN_HYPERV_STIMER: > + case VIR_DOMAIN_HYPERV_RESET: > if (src->hyperv_features[i] != dst->hyperv_features[i]) { > virReportError(VIR_ERR_CONFIG_UNSUPPORTED, > _("State of HyperV enlightenment " > @@ -17654,6 +17698,17 @@ virDomainDefFeaturesCheckABIStability(virDomainDefPtr src, > } > break; > > + case VIR_DOMAIN_HYPERV_VENDOR_ID: > + if (STRNEQ_NULLABLE(src->hyperv_vendor_id, dst->hyperv_vendor_id)) { > + virReportError(VIR_ERR_CONFIG_UNSUPPORTED, > + _("HyperV vendor_id differs: " > + "source: '%s', destination: '%s'"), > + src->hyperv_vendor_id, > + dst->hyperv_vendor_id); > + return false; > + } > + break; > + > /* coverity[dead_error_begin] */ > case VIR_DOMAIN_HYPERV_LAST: > break; > @@ -22322,6 +22377,11 @@ virDomainDefFormatInternal(virDomainDefPtr def, > switch ((virDomainHyperv) j) { > case VIR_DOMAIN_HYPERV_RELAXED: > case VIR_DOMAIN_HYPERV_VAPIC: > + case VIR_DOMAIN_HYPERV_VPINDEX: > + case VIR_DOMAIN_HYPERV_RUNTIME: > + case VIR_DOMAIN_HYPERV_SYNIC: > + case VIR_DOMAIN_HYPERV_STIMER: > + case VIR_DOMAIN_HYPERV_RESET: > break; > > case VIR_DOMAIN_HYPERV_SPINLOCKS: > @@ -22331,6 +22391,13 @@ virDomainDefFormatInternal(virDomainDefPtr def, > def->hyperv_spinlocks); > break; > > + case VIR_DOMAIN_HYPERV_VENDOR_ID: > + if (def->hyperv_features[j] != VIR_TRISTATE_SWITCH_ON) > + break; > + virBufferEscapeString(buf, " value='%s'", > + def->hyperv_vendor_id); > + break; > + > /* coverity[dead_error_begin] */ > case VIR_DOMAIN_HYPERV_LAST: > break; > diff --git a/src/conf/domain_conf.h b/src/conf/domain_conf.h > index 832ca4f..79ff55e 100644 > --- a/src/conf/domain_conf.h > +++ b/src/conf/domain_conf.h > @@ -1699,10 +1699,18 @@ typedef enum { > VIR_DOMAIN_FEATURE_LAST > } virDomainFeature; > > +# define VIR_DOMAIN_HYPERV_VENDOR_ID_MAX 12 > + > typedef enum { > VIR_DOMAIN_HYPERV_RELAXED = 0, > VIR_DOMAIN_HYPERV_VAPIC, > VIR_DOMAIN_HYPERV_SPINLOCKS, > + VIR_DOMAIN_HYPERV_VPINDEX, > + VIR_DOMAIN_HYPERV_RUNTIME, > + VIR_DOMAIN_HYPERV_SYNIC, > + VIR_DOMAIN_HYPERV_STIMER, > + VIR_DOMAIN_HYPERV_RESET, > + VIR_DOMAIN_HYPERV_VENDOR_ID, > > VIR_DOMAIN_HYPERV_LAST > } virDomainHyperv; > @@ -2238,6 +2246,7 @@ struct _virDomainDef { > int kvm_features[VIR_DOMAIN_KVM_LAST]; > unsigned int hyperv_spinlocks; > virGICVersion gic_version; > + char *hyperv_vendor_id; > > /* These options are of type virTristateSwitch: ON = keep, OFF = drop */ > int caps_features[VIR_DOMAIN_CAPS_FEATURE_LAST]; > diff --git a/src/qemu/qemu_command.c b/src/qemu/qemu_command.c > index 000c29d..f87511c 100644 > --- a/src/qemu/qemu_command.c > +++ b/src/qemu/qemu_command.c > @@ -5039,6 +5039,11 @@ qemuBuildCpuCommandLine(virCommandPtr cmd, > switch ((virDomainHyperv) i) { > case VIR_DOMAIN_HYPERV_RELAXED: > case VIR_DOMAIN_HYPERV_VAPIC: > + case VIR_DOMAIN_HYPERV_VPINDEX: > + case VIR_DOMAIN_HYPERV_RUNTIME: > + case VIR_DOMAIN_HYPERV_SYNIC: > + case VIR_DOMAIN_HYPERV_STIMER: > + case VIR_DOMAIN_HYPERV_RESET: > if (def->hyperv_features[i] == VIR_TRISTATE_SWITCH_ON) > virBufferAsprintf(&buf, ",hv_%s", > virDomainHypervTypeToString(i)); > @@ -5050,6 +5055,12 @@ qemuBuildCpuCommandLine(virCommandPtr cmd, > def->hyperv_spinlocks); > break; > > + case VIR_DOMAIN_HYPERV_VENDOR_ID: > + if (def->hyperv_features[i] == VIR_TRISTATE_SWITCH_ON) > + virBufferAsprintf(&buf, ",hv_vendor_id=%s", > + def->hyperv_vendor_id); > + break; > + > /* coverity[dead_error_begin] */ > case VIR_DOMAIN_HYPERV_LAST: > break; > diff --git a/src/qemu/qemu_parse_command.c b/src/qemu/qemu_parse_command.c > index 60e3d69..6635190 100644 > --- a/src/qemu/qemu_parse_command.c > +++ b/src/qemu/qemu_parse_command.c > @@ -1539,6 +1539,11 @@ qemuParseCommandLineCPU(virDomainDefPtr dom, > switch ((virDomainHyperv) f) { > case VIR_DOMAIN_HYPERV_RELAXED: > case VIR_DOMAIN_HYPERV_VAPIC: > + case VIR_DOMAIN_HYPERV_VPINDEX: > + case VIR_DOMAIN_HYPERV_RUNTIME: > + case VIR_DOMAIN_HYPERV_SYNIC: > + case VIR_DOMAIN_HYPERV_STIMER: > + case VIR_DOMAIN_HYPERV_RESET: > if (value) { > virReportError(VIR_ERR_CONFIG_UNSUPPORTED, > _("HyperV feature '%s' should not " > @@ -1566,6 +1571,21 @@ qemuParseCommandLineCPU(virDomainDefPtr dom, > dom->hyperv_spinlocks = 0xFFF; > break; > > + case VIR_DOMAIN_HYPERV_VENDOR_ID: > + dom->hyperv_features[f] = VIR_TRISTATE_SWITCH_ON; > + if (!value) { > + virReportError(VIR_ERR_CONFIG_UNSUPPORTED, "%s", > + _("missing HyperV vendor_id value")); > + goto cleanup; > + } > + > + if (VIR_STRDUP(dom->hyperv_vendor_id, value) < 0) > + goto cleanup; > + > + if (strlen(dom->hyperv_vendor_id) > VIR_DOMAIN_HYPERV_VENDOR_ID_MAX) > + dom->hyperv_vendor_id[VIR_DOMAIN_HYPERV_VENDOR_ID_MAX] = '\0'; Since qemu does check for a length of 12 characters, I don't think this check is necessary. Cutting it off as 12 could mean the generated guest XML wouldn't match... I'd be in favor of removing. If you're OK with the above changes, I can make the adjustments and push the 3 patches - let me know. John

> + break; > + > case VIR_DOMAIN_HYPERV_LAST: > break; > } > diff --git a/tests/qemuxml2argvdata/qemuxml2argv-hyperv-off.xml b/tests/qemuxml2argvdata/qemuxml2argv-hyperv-off.xml > index 1067f64..fe08463 100644 > --- a/tests/qemuxml2argvdata/qemuxml2argv-hyperv-off.xml > +++ b/tests/qemuxml2argvdata/qemuxml2argv-hyperv-off.xml > @@ -14,6 +14,12 @@ > <relaxed state='off'/> > <vapic state='off'/> > <spinlocks state='off'/> > + <vpindex state='off'/> > + <runtime state='off'/> > + <synic state='off'/> > + <stimer state='off'/> > + <reset state='off'/> > + <vendor_id state='off'/> > </hyperv> > </features> > <clock offset='utc'/> > diff --git a/tests/qemuxml2argvdata/qemuxml2argv-hyperv.args b/tests/qemuxml2argvdata/qemuxml2argv-hyperv.args > index 141844a..32846a2 100644 > --- a/tests/qemuxml2argvdata/qemuxml2argv-hyperv.args > +++ b/tests/qemuxml2argvdata/qemuxml2argv-hyperv.args > @@ -8,7 +8,8 @@ QEMU_AUDIO_DRV=none \ > -name QEMUGuest1 \ > -S \ > -M pc \ > --cpu qemu32,hv_relaxed,hv_vapic,hv_spinlocks=0x2fff \ > +-cpu 'qemu32,hv_relaxed,hv_vapic,hv_spinlocks=0x2fff,hv_vpindex,hv_runtime,\ > +hv_synic,hv_stimer,hv_reset,hv_vendor_id=KVM Hv' \ > -m 214 \ > -smp 6 \ > -uuid c7a5fdbd-edaf-9455-926a-d65c16db1809 \ > diff --git a/tests/qemuxml2argvdata/qemuxml2argv-hyperv.xml b/tests/qemuxml2argvdata/qemuxml2argv-hyperv.xml > index 2b8f332..a47013b 100644 > --- a/tests/qemuxml2argvdata/qemuxml2argv-hyperv.xml > +++ b/tests/qemuxml2argvdata/qemuxml2argv-hyperv.xml > @@ -14,6 +14,12 @@ > <relaxed state='on'/> > <vapic state='on'/> > <spinlocks state='on' retries='12287'/> > + <vpindex state='on'/> > + <runtime state='on'/> > + <synic state='on'/> > + <stimer state='on'/> > + <reset state='on'/> > + <vendor_id state='on' value='KVM Hv'/> > </hyperv> > </features> > <clock offset='utc'/> > diff --git a/tests/qemuxml2xmloutdata/qemuxml2xmlout-hyperv-off.xml b/tests/qemuxml2xmloutdata/qemuxml2xmlout-hyperv-off.xml > index b09c447..6163a5d 100644 > --- a/tests/qemuxml2xmloutdata/qemuxml2xmlout-hyperv-off.xml > +++ b/tests/qemuxml2xmloutdata/qemuxml2xmlout-hyperv-off.xml > @@ -14,6 +14,12 @@ > <relaxed state='off'/> > <vapic state='off'/> > <spinlocks state='off'/> > + <vpindex state='off'/> > + <runtime state='off'/> > + <synic state='off'/> > + <stimer state='off'/> > + <reset state='off'/> > + <vendor_id state='off'/> > </hyperv> > </features> > <clock offset='utc'/> > diff --git a/tests/qemuxml2xmloutdata/qemuxml2xmlout-hyperv.xml b/tests/qemuxml2xmloutdata/qemuxml2xmlout-hyperv.xml > index a79115c..c11c273 100644 > --- a/tests/qemuxml2xmloutdata/qemuxml2xmlout-hyperv.xml > +++ b/tests/qemuxml2xmloutdata/qemuxml2xmlout-hyperv.xml > @@ -14,6 +14,12 @@ > <relaxed state='on'/> > <vapic state='on'/> > <spinlocks state='on' retries='12287'/> > + <vpindex state='on'/> > + <runtime state='on'/> > + <synic state='on'/> > + <stimer state='on'/> > + <reset state='on'/> > + <vendor_id state='on' value='KVM Hv'/> > </hyperv> > </features> > <clock offset='utc'/> >