[libvirt] [PATCH 1/2] qemu: support for kvm-hint-dedicated performance hint
Menno Lageman
menno.lageman at oracle.com
Mon Aug 12 11:18:48 UTC 2019
On 12-08-19 11:23, Michal Privoznik wrote:
> On 8/9/19 5:19 PM, Menno Lageman wrote:
>> From: Wim ten Have <wim.ten.have at oracle.com>
>>
>> QEMU version 2.12.1 introduced a performance feature under commit
>> be7773268d98 ("target-i386: add KVM_HINTS_DEDICATED performance hint")
>>
>> This patch adds a new KVM feature 'hint-dedicated' to set this performance
>> hint for KVM guests. The feature is off by default.
>>
>> To enable this hint and have libvirt add "-cpu host,kvm-hint-dedicated=on"
>> to the QEMU command line, the following XML code needs to be added to the
>> guest's domain description in conjunction with CPU mode='host-passthrough'.
>>
>> <features>
>> <kvm>
>> <hint-dedicated state='on'/>
>> </kvm>
>> </features>
>> ...
>> <cpu mode='host-passthrough ... />
>>
>> Signed-off-by: Wim ten Have <wim.ten.have at oracle.com>
>> Signed-off-by: Menno Lageman <menno.lageman at oracle.com>
>> ---
>> docs/formatdomain.html.in | 7 +++++++
>> docs/schemas/domaincommon.rng | 5 +++++
>> src/conf/domain_conf.c | 4 ++++
>> src/conf/domain_conf.h | 1 +
>> src/qemu/qemu_command.c | 13 +++++++++++++
>> 5 files changed, 30 insertions(+)
>>
>> diff --git a/docs/formatdomain.html.in b/docs/formatdomain.html.in
>> index 6d084d7c0472..c9b18b57e1fc 100644
>> --- a/docs/formatdomain.html.in
>> +++ b/docs/formatdomain.html.in
>> @@ -2044,6 +2044,7 @@
>> </hyperv>
>> <kvm>
>> <hidden state='on'/>
>> + <hint-dedicated state='on'/>
>> </kvm>
>> <pvspinlock state='on'/>
>> <gic version='2'/>
>> @@ -2217,6 +2218,12 @@
>> <td>on, off</td>
>> <td><span class="since">1.2.8 (QEMU 2.1.0)</span></td>
>> </tr>
>> + <tr>
>> + <td>hint-dedicated</td>
>> + <td>Set the "dedicated physical CPU" performance hint ("-cpu kvm-hint-dedicated=on")</td>
>
> I'd rather not document the command line libvirt generates here. Not only it's an internal thing of libvirt, it'd also be a promise we might not keep up (if qemu changes way how this is configured for instance).
> But what we should document is what this feature actually is. I've tried to dig out KVM patches and now I have a faint idea, but we can't expect our users to go through patches when deciding whether to turn this on or not.
How about replacing it with "Allows a guest to enable optimizations when
running on dedicated vCPU" ?
>
>> + <td>on, off</td>
>> + <td><span class="since">5.7.0 (QEMU 2.12.1)</span></td>
>> + </tr>
>> </table>
>> </dd>
>> <dt><code>pmu</code></dt>
>> diff --git a/docs/schemas/domaincommon.rng b/docs/schemas/domaincommon.rng
>> index a0771da45b5d..08853f9d9e92 100644
>> --- a/docs/schemas/domaincommon.rng
>> +++ b/docs/schemas/domaincommon.rng
>> @@ -5965,6 +5965,11 @@
>> <ref name="featurestate"/>
>> </element>
>> </optional>
>> + <optional>
>> + <element name="hint-dedicated">
>> + <ref name="featurestate"/>
>> + </element>
>> + </optional>
>> </interleave>
>> </element>
>> </define>
>> diff --git a/src/conf/domain_conf.c b/src/conf/domain_conf.c
>> index 03312afaaff8..3907fcf6e5ca 100644
>> --- a/src/conf/domain_conf.c
>> +++ b/src/conf/domain_conf.c
>> @@ -202,6 +202,7 @@ VIR_ENUM_IMPL(virDomainHyperv,
>> VIR_ENUM_IMPL(virDomainKVM,
>> VIR_DOMAIN_KVM_LAST,
>> "hidden",
>> + "hint-dedicated",
>> );
>>
>> VIR_ENUM_IMPL(virDomainMsrsUnknown,
>> @@ -20412,6 +20413,7 @@ virDomainDefParseXML(xmlDocPtr xml,
>>
>> switch ((virDomainKVM) feature) {
>> case VIR_DOMAIN_KVM_HIDDEN:
>> + case VIR_DOMAIN_KVM_DEDICATED:
>> if (!(tmp = virXMLPropString(nodes[i], "state"))) {
>> virReportError(VIR_ERR_XML_ERROR,
>> _("missing 'state' attribute for "
>> @@ -22624,6 +22626,7 @@ virDomainDefFeaturesCheckABIStability(virDomainDefPtr src,
>> for (i = 0; i < VIR_DOMAIN_KVM_LAST; i++) {
>> switch ((virDomainKVM) i) {
>> case VIR_DOMAIN_KVM_HIDDEN:
>> + case VIR_DOMAIN_KVM_DEDICATED:
>> if (src->kvm_features[i] != dst->kvm_features[i]) {
>> virReportError(VIR_ERR_CONFIG_UNSUPPORTED,
>> _("State of KVM feature '%s' differs: "
>> @@ -28124,6 +28127,7 @@ virDomainDefFormatFeatures(virBufferPtr buf,
>> for (j = 0; j < VIR_DOMAIN_KVM_LAST; j++) {
>> switch ((virDomainKVM) j) {
>> case VIR_DOMAIN_KVM_HIDDEN:
>> + case VIR_DOMAIN_KVM_DEDICATED:
>> if (def->kvm_features[j])
>> virBufferAsprintf(&childBuf, "<%s state='%s'/>\n",
>> virDomainKVMTypeToString(j),
>> diff --git a/src/conf/domain_conf.h b/src/conf/domain_conf.h
>> index bce47443c858..f7423b1b6f89 100644
>> --- a/src/conf/domain_conf.h
>> +++ b/src/conf/domain_conf.h
>> @@ -1765,6 +1765,7 @@ typedef enum {
>>
>> typedef enum {
>> VIR_DOMAIN_KVM_HIDDEN = 0,
>> + VIR_DOMAIN_KVM_DEDICATED,
>>
>> VIR_DOMAIN_KVM_LAST
>> } virDomainKVM;
>> diff --git a/src/qemu/qemu_command.c b/src/qemu/qemu_command.c
>> index 71a36ff63a81..ab535af5efb6 100644
>> --- a/src/qemu/qemu_command.c
>> +++ b/src/qemu/qemu_command.c
>> @@ -7212,6 +7212,19 @@ qemuBuildCpuCommandLine(virCommandPtr cmd,
>> virBufferAddLit(&buf, ",kvm=off");
>> break;
>>
>> + case VIR_DOMAIN_KVM_DEDICATED:
>> + if (def->kvm_features[i] == VIR_TRISTATE_SWITCH_ON) {
>> + if (def->cpu && def->cpu->mode == VIR_CPU_MODE_HOST_PASSTHROUGH) {
>> + virBufferAddLit(&buf, ",kvm-hint-dedicated=on");
>> + } else {
>> + virReportError(VIR_ERR_CONFIG_UNSUPPORTED, "%s",
>> + _("kvm-hint-dedicated=on is only applicable when "
>> + "<cpu mode=\"host-passthrough\" .../> is in effect"));
>> + goto cleanup;
>> + }
>> + }
>> + break;
>
> Ou, I don't think this is the correct place to check for valid domain config. How about, you move the check somewhere to qemuDomainDefValidate() and leave here just the cmd line generator part?
> Also, the error message is kind of hairy.
>
> Long story short, this is what I have on mind:
>
> diff --git i/src/qemu/qemu_command.c w/src/qemu/qemu_command.c
> index ab535af5ef..f096e8f27e 100644
> --- i/src/qemu/qemu_command.c
> +++ w/src/qemu/qemu_command.c
> @@ -7213,16 +7213,8 @@ qemuBuildCpuCommandLine(virCommandPtr cmd,
> break;
>
> case VIR_DOMAIN_KVM_DEDICATED:
> - if (def->kvm_features[i] == VIR_TRISTATE_SWITCH_ON) {
> - if (def->cpu && def->cpu->mode == VIR_CPU_MODE_HOST_PASSTHROUGH) {
> - virBufferAddLit(&buf, ",kvm-hint-dedicated=on");
> - } else {
> - virReportError(VIR_ERR_CONFIG_UNSUPPORTED, "%s",
> - _("kvm-hint-dedicated=on is only applicable when "
> - "<cpu mode=\"host-passthrough\" .../> is in effect"));
> - goto cleanup;
> - }
> - }
> + if (def->kvm_features[i] == VIR_TRISTATE_SWITCH_ON)
> + virBufferAddLit(&buf, ",kvm-hint-dedicated=on");
> break;
>
> /* coverity[dead_error_begin] */
> diff --git i/src/qemu/qemu_domain.c w/src/qemu/qemu_domain.c
> index 937b461a8b..e1c31d58a4 100644
> --- i/src/qemu/qemu_domain.c
> +++ w/src/qemu/qemu_domain.c
> @@ -4698,6 +4698,14 @@ qemuDomainDefValidate(const virDomainDef *def,
> }
> }
>
> + if (def->kvm_features[VIR_DOMAIN_KVM_DEDICATED] == VIR_TRISTATE_SWITCH_ON &&
> + (!def->cpu || def->cpu->mode != VIR_CPU_MODE_HOST_PASSTHROUGH)) {
> + virReportError(VIR_ERR_CONFIG_UNSUPPORTED, "%s",
> + _("kvm-hint-dedicated=on is only applicable "
> + "for cpu host-passthrough"));
> + goto cleanup;
> + }
> +
> ret = 0;
>
> cleanup:
>
Yup, makes sense. I'll send a v2.
Thanks,
Menno
>
>
> Michal
>
More information about the libvir-list
mailing list