On 8/9/19 5:19 PM, Menno Lageman wrote: > From: Wim ten Have <wim.ten.have(a)oracle.com&gt; > > 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(a)oracle.com&gt; > Signed-off-by: Menno Lageman <menno.lageman(a)oracle.com&gt; > --- > 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 @@ > &lt;/hyperv&gt; > &lt;kvm&gt; > &lt;hidden state='on'/&gt; > + &lt;hint-dedicated state='on'/&gt; > &lt;/kvm&gt; > &lt;pvspinlock state='on'/&gt; > &lt;gic version='2'/&gt; > @@ -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.

> + <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:

Michal

On 8/12/19 1:18 PM, Menno Lageman wrote: > 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(a)oracle.com&gt; >>> >>> 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(a)oracle.com&gt; >>> Signed-off-by: Menno Lageman <menno.lageman(a)oracle.com&gt; >>> --- >>>    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 @@ >>>      &lt;/hyperv&gt; >>>      &lt;kvm&gt; >>>        &lt;hidden state='on'/&gt; >>> +    &lt;hint-dedicated state='on'/&gt; >>>      &lt;/kvm&gt; >>>      &lt;pvspinlock state='on'/&gt; >>>      &lt;gic version='2'/&gt; >>> @@ -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" ? What do you mean by 'dedicated vCPU'? So far I only know dedicated physical CPU (or CPU core) - and that's not easy to set up, you'll need numa pinning, cpu pinning, isolcpus, and whatnot. Also, what opimizations does it enable?

I'm not saying we need to put everything in the docs, but also I don't want the docs to be undestandable by developers who developed given feature. Michal -- libvir-list mailing list libvir-list(a)redhat.com https://www.redhat.com/mailman/listinfo/libvir-list