[libvirt] Fwd: Re: [PATCH v2 3/3] conf: qemu: Add support for more HyperV Enlightenment features
Maxim Nestratov
mnestratov at virtuozzo.com
Mon Mar 28 16:40:45 UTC 2016
Just forwarding as my original letter seems to get lost, please ignore
if isn't.
28.03.2016 17:55, John Ferlan пишет:
>
> On 03/10/2016 07:43 AM, Nikolay Shirokovskiy wrote:
>> From: Maxim Nestratov <mnestratov at virtuozzo.com>
>>
>> This patch adds support for "vpindex", "runtime", "synic" and
>> "stimer" features available in qemu 2.5+.
> Should "vendor_id" be part of this list?
yes, sure
>
>> - 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 at virtuozzo.com>
>> ---
>> 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 @@
>> <relaxed state='on'/>
>> <vapic state='on'/>
>> <spinlocks state='on' retries='4096'/>
>> + <vpindex state='on'/>
>> + <runtime state='on'/>
>> + <synic state='on'/>
>> + <reset state='on'/>
>> + <vendor_id state='on' value='KVM Hv'/>
>> </hyperv>
>> <kvm>
>> <hidden state='on'/>
>> @@ -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.
let it be the first one (one character shorter :) )
>
> I see that 'hv-crash' was added in 1.5 (qemu commit id 'f2a53c9e'), but
> not added here - any reason why not?
just because it's already in libvirt (commit 59fc0d06)
>
> 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).
same reason (commit 600bca59)
>> </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
>
I'm OK with all the comments, push please
>> + 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'/>
>>
More information about the libvir-list
mailing list