Re: [libvirt] [PATCH 4/6] conf: add support for Direct Mode for Hyper-V Synthetic timers

On Thu, Jul 25, 2019 at 03:52:16PM +0200, Vitaly Kuznetsov wrote: >Support 'Direct Mode' for Hyper-V Synthetic Timers in domain config. >Make it 'stimer' enlightenment option as it is not a separate thing. > >Signed-off-by: Vitaly Kuznetsov <vkuznets(a)redhat.com&gt; >--- > docs/formatdomain.html.in | 10 ++- > docs/schemas/domaincommon.rng | 16 +++- > src/conf/domain_conf.c | 138 +++++++++++++++++++++++++++++++--- > src/conf/domain_conf.h | 8 ++ > src/cpu/cpu_x86.c | 51 +++++++------ > src/cpu/cpu_x86_data.h | 2 + > src/libvirt_private.syms | 2 + > 7 files changed, 187 insertions(+), 40 deletions(-) > >diff --git a/docs/formatdomain.html.in b/docs/formatdomain.html.in >index 1aaddb6d9b..a0723edad1 100644 >--- a/docs/formatdomain.html.in >+++ b/docs/formatdomain.html.in >@@ -2033,7 +2033,9 @@ > &lt;vpindex state='on'/&gt; > &lt;runtime state='on'/&gt; > &lt;synic state='on'/&gt; >- &lt;stimer state='on'/&gt; >+ &lt;stimer state='on'&gt; >+ &lt;direct state='on'/&gt; >+ &lt;/stimer&gt; > &lt;reset state='on'/&gt; > &lt;vendor_id state='on' value='KVM Hv'/&gt; > &lt;frequencies state='on'/&gt; >@@ -2148,9 +2150,9 @@ > </tr> > <tr> > <td>stimer</td> >- <td>Enable SynIC timers</td> >- <td>on, off</td> >- <td><span class="since">1.3.3 (QEMU 2.6)</span></td> >+ <td>Enable SynIC timers, optionally with Direct Mode support</td> >+ <td>on, off; direct - on,off</td> >+ <td><span class="since">1.3.3 (QEMU 2.6), direct mode 5.6.0 (QEMU 4.1)</span></td> > </tr> > <tr> > <td>reset</td> >diff --git a/docs/schemas/domaincommon.rng b/docs/schemas/domaincommon.rng >index 763480440c..8cf1995748 100644 >--- a/docs/schemas/domaincommon.rng >+++ b/docs/schemas/domaincommon.rng >@@ -5896,7 +5896,7 @@ > </optional> > <optional> > <element name="stimer"> >- <ref name="featurestate"/> >+ <ref name="stimer"/> > </element> > </optional> > <optional> >@@ -5945,6 +5945,20 @@ > </element> > </define> > >+ <!-- Hyper-V stimer features --> >+ <define name="stimer"> >+ <interleave> >+ <optional> >+ <ref name="featurestate"/> >+ </optional> >+ <optional> >+ <element name="direct"> >+ <ref name="featurestate"/> >+ </element> >+ </optional> >+ </interleave> >+ </define> >+ > <!-- Optional KVM features --> > <define name="kvm"> > <element name="kvm"> >diff --git a/src/conf/domain_conf.c b/src/conf/domain_conf.c >index 0574c69a46..779b4ed880 100644 >--- a/src/conf/domain_conf.c >+++ b/src/conf/domain_conf.c >@@ -197,6 +197,11 @@ VIR_ENUM_IMPL(virDomainHyperv, > "evmcs", > ); > >+VIR_ENUM_IMPL(virDomainHypervStimer, >+ VIR_DOMAIN_HYPERV_STIMER_LAST, >+ "direct", >+); Do you anticipate more stimer "sub"-features in the future? Having an enum with one value just to loop over an array with one element and then switch()-ing across all the possible value seems like overkill.

>+ > VIR_ENUM_IMPL(virDomainKVM, > VIR_DOMAIN_KVM_LAST, > "hidden", >@@ -20359,6 +20364,51 @@ virDomainDefParseXML(xmlDocPtr xml, > ctxt->node = node; > } > >+ if (def->features[VIR_DOMAIN_HYPERV_STIMER] == VIR_TRISTATE_SWITCH_ON) { >+ int feature; >+ int value; >+ if ((n = virXPathNodeSet("./features/hyperv/stimer/*", ctxt, &nodes)) < 0) >+ goto error; >+ >+ for (i = 0; i < n; i++) { >+ feature = virDomainHypervStimerTypeFromString((const char *)nodes[i]->name); >+ if (feature < 0) { >+ virReportError(VIR_ERR_CONFIG_UNSUPPORTED, >+ _("unsupported Hyper-V stimer feature: %s"), >+ nodes[i]->name); >+ goto error; >+ } >+ >+ switch ((virDomainHypervStimer) feature) { >+ case VIR_DOMAIN_HYPERV_STIMER_DIRECT: >+ if (!(tmp = virXMLPropString(nodes[i], "state"))) { >+ virReportError(VIR_ERR_XML_ERROR, >+ _("missing 'state' attribute for " >+ "Hyper-V stimer feature '%s'"), >+ nodes[i]->name); >+ goto error; >+ } >+ >+ if ((value = virTristateSwitchTypeFromString(tmp)) < 0) { >+ virReportError(VIR_ERR_CONFIG_UNSUPPORTED, >+ _("invalid value of state argument " >+ "for Hyper-V stimer feature '%s'"), >+ nodes[i]->name); >+ goto error; >+ } >+ >+ VIR_FREE(tmp); >+ def->hyperv_stimer_features[feature] = value; >+ break; >+ >+ /* coverity[dead_error_begin] */ >+ case VIR_DOMAIN_HYPERV_STIMER_LAST: >+ break; >+ } >+ } >+ VIR_FREE(nodes); >+ } >+ > if (def->features[VIR_DOMAIN_FEATURE_KVM] == VIR_TRISTATE_SWITCH_ON) { > int feature; > int value; >@@ -22583,6 +22633,29 @@ virDomainDefFeaturesCheckABIStability(virDomainDefPtr src, > } > } > >+ if (src->hyperv_features[VIR_DOMAIN_HYPERV_STIMER] == VIR_TRISTATE_SWITCH_ON) { >+ for (i = 0; i < VIR_DOMAIN_HYPERV_STIMER_LAST; i++) { >+ switch ((virDomainHypervStimer) i) { >+ case VIR_DOMAIN_HYPERV_STIMER_DIRECT: >+ if (src->hyperv_stimer_features[i] != dst->hyperv_stimer_features[i]) { >+ virReportError(VIR_ERR_CONFIG_UNSUPPORTED, >+ _("State of HyperV stimer feature '%s' differs: " >+ "source: '%s', destination: '%s'"), >+ virDomainHypervStimerTypeToString(i), >+ virTristateSwitchTypeToString(src->hyperv_stimer_features[i]), >+ virTristateSwitchTypeToString(dst->hyperv_stimer_features[i])); >+ return false; >+ } >+ >+ break; >+ >+ /* coverity[dead_error_begin] */ >+ case VIR_DOMAIN_HYPERV_STIMER_LAST: >+ break; >+ } >+ } >+ } >+ > /* kvm */ > if (src->features[VIR_DOMAIN_FEATURE_KVM] == VIR_TRISTATE_SWITCH_ON) { > for (i = 0; i < VIR_DOMAIN_KVM_LAST; i++) { >@@ -28017,6 +28090,8 @@ virDomainDefFormatFeatures(virBufferPtr buf, > virBufferAddLit(&childBuf, "<hyperv>\n"); > virBufferAdjustIndent(&childBuf, 2); > for (j = 0; j < VIR_DOMAIN_HYPERV_LAST; j++) { >+ size_t k; >+ > if (def->hyperv_features[j] == VIR_TRISTATE_SWITCH_ABSENT) > continue; > >@@ -28031,35 +28106,76 @@ virDomainDefFormatFeatures(virBufferPtr buf, > 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: > case VIR_DOMAIN_HYPERV_FREQUENCIES: > case VIR_DOMAIN_HYPERV_REENLIGHTENMENT: > case VIR_DOMAIN_HYPERV_TLBFLUSH: > case VIR_DOMAIN_HYPERV_IPI: > case VIR_DOMAIN_HYPERV_EVMCS: >+ virBufferAddLit(&childBuf, "/>\n"); Changing all the cases to print the ending tag themselves in a separate commit first would make this one look nicer.

> break; > >- case VIR_DOMAIN_HYPERV_SPINLOCKS: >- if (def->hyperv_features[j] != VIR_TRISTATE_SWITCH_ON) >+ case VIR_DOMAIN_HYPERV_STIMER: >+ if (def->hyperv_features[j] != VIR_TRISTATE_SWITCH_ON) { >+ virBufferAddLit(&childBuf, "/>\n"); >+ break; >+ } >+ >diff --git a/src/conf/domain_conf.h b/src/conf/domain_conf.h >index 48b0af4b04..fc12887fc3 100644 >--- a/src/conf/domain_conf.h >+++ b/src/conf/domain_conf.h >@@ -1762,6 +1762,12 @@ typedef enum { > VIR_DOMAIN_HYPERV_LAST > } virDomainHyperv; > >+typedef enum { >+ VIR_DOMAIN_HYPERV_STIMER_DIRECT = 0, >+ >+ VIR_DOMAIN_HYPERV_STIMER_LAST >+} virDomainHypervStimer; >+ > typedef enum { > VIR_DOMAIN_KVM_HIDDEN = 0, > >@@ -2400,6 +2406,7 @@ struct _virDomainDef { > int kvm_features[VIR_DOMAIN_KVM_LAST]; > int msrs_features[VIR_DOMAIN_MSRS_LAST]; > unsigned int hyperv_spinlocks; >+ int hyperv_stimer_features[VIR_DOMAIN_HYPERV_STIMER_LAST]; How about: int hyperv_stimer_direct;

> virGICVersion gic_version; > virDomainHPTResizing hpt_resizing; > unsigned long long hpt_maxpagesize; /* Stored in KiB */ >@@ -3420,6 +3427,7 @@ VIR_ENUM_DECL(virDomainGraphicsSpiceStreamingMode); > VIR_ENUM_DECL(virDomainGraphicsSpiceMouseMode); > VIR_ENUM_DECL(virDomainGraphicsVNCSharePolicy); > VIR_ENUM_DECL(virDomainHyperv); >+VIR_ENUM_DECL(virDomainHypervStimer); > VIR_ENUM_DECL(virDomainKVM); > VIR_ENUM_DECL(virDomainMsrsUnknown); > VIR_ENUM_DECL(virDomainRNGModel); >diff --git a/src/cpu/cpu_x86.c b/src/cpu/cpu_x86.c >index 55b55da784..4fb9e6a4df 100644 >--- a/src/cpu/cpu_x86.c >+++ b/src/cpu/cpu_x86.c >@@ -59,9 +59,9 @@ struct _virCPUx86Feature { > { .type = VIR_CPU_X86_DATA_CPUID, \ > .data = { .cpuid = {__VA_ARGS__} } } > >-#define KVM_FEATURE_DEF(Name, Eax_in, Eax) \ >+#define KVM_FEATURE_DEF(Name, Eax_in, Eax, Edx) \ > static virCPUx86DataItem Name ## _data[] = { \ >- CPUID(.eax_in = Eax_in, .eax = Eax), \ >+ CPUID(.eax_in = Eax_in, .eax = Eax, .edx = Edx), \ Another change that can be separated.

> } > > #define KVM_FEATURE(Name) \ >@@ -74,49 +74,51 @@ struct _virCPUx86Feature { > } > > KVM_FEATURE_DEF(VIR_CPU_x86_KVM_CLOCKSOURCE, >- 0x40000001, 0x00000001); >+ 0x40000001, 0x00000001, 0x0); > KVM_FEATURE_DEF(VIR_CPU_x86_KVM_NOP_IO_DELAY, >- 0x40000001, 0x00000002); >+ 0x40000001, 0x00000002, 0x0); > KVM_FEATURE_DEF(VIR_CPU_x86_KVM_MMU_OP, >- 0x40000001, 0x00000004); >+ 0x40000001, 0x00000004, 0x0); > KVM_FEATURE_DEF(VIR_CPU_x86_KVM_CLOCKSOURCE2, >- 0x40000001, 0x00000008); >+ 0x40000001, 0x00000008, 0x0); > KVM_FEATURE_DEF(VIR_CPU_x86_KVM_ASYNC_PF, >- 0x40000001, 0x00000010); >+ 0x40000001, 0x00000010, 0x0); > KVM_FEATURE_DEF(VIR_CPU_x86_KVM_STEAL_TIME, >- 0x40000001, 0x00000020); >+ 0x40000001, 0x00000020, 0x0); > KVM_FEATURE_DEF(VIR_CPU_x86_KVM_PV_EOI, >- 0x40000001, 0x00000040); >+ 0x40000001, 0x00000040, 0x0); > KVM_FEATURE_DEF(VIR_CPU_x86_KVM_PV_UNHALT, >- 0x40000001, 0x00000080); >+ 0x40000001, 0x00000080, 0x0); > KVM_FEATURE_DEF(VIR_CPU_x86_KVM_CLOCKSOURCE_STABLE_BIT, >- 0x40000001, 0x01000000); >+ 0x40000001, 0x01000000, 0x0); Take a look at Jirka's series which removes most of these. > KVM_FEATURE_DEF(VIR_CPU_x86_KVM_HV_RUNTIME, >- 0x40000003, 0x00000001); >+ 0x40000003, 0x00000001, 0x0); > KVM_FEATURE_DEF(VIR_CPU_x86_KVM_HV_SYNIC, >- 0x40000003, 0x00000004); >+ 0x40000003, 0x00000004, 0x0); > KVM_FEATURE_DEF(VIR_CPU_x86_KVM_HV_STIMER, >- 0x40000003, 0x00000008); >+ 0x40000003, 0x00000008, 0x0); > KVM_FEATURE_DEF(VIR_CPU_x86_KVM_HV_RELAXED, >- 0x40000003, 0x00000020); >+ 0x40000003, 0x00000020, 0x0); > KVM_FEATURE_DEF(VIR_CPU_x86_KVM_HV_SPINLOCKS, >- 0x40000003, 0x00000022); >+ 0x40000003, 0x00000022, 0x0); > KVM_FEATURE_DEF(VIR_CPU_x86_KVM_HV_VAPIC, >- 0x40000003, 0x00000030); >+ 0x40000003, 0x00000030, 0x0); > KVM_FEATURE_DEF(VIR_CPU_x86_KVM_HV_VPINDEX, >- 0x40000003, 0x00000040); >+ 0x40000003, 0x00000040, 0x0); > KVM_FEATURE_DEF(VIR_CPU_x86_KVM_HV_RESET, >- 0x40000003, 0x00000080); >+ 0x40000003, 0x00000080, 0x0); > KVM_FEATURE_DEF(VIR_CPU_x86_KVM_HV_FREQUENCIES, >- 0x40000003, 0x00000800); >+ 0x40000003, 0x00000800, 0x0); > KVM_FEATURE_DEF(VIR_CPU_x86_KVM_HV_REENLIGHTENMENT, >- 0x40000003, 0x00002000); >+ 0x40000003, 0x00002000, 0x0); > KVM_FEATURE_DEF(VIR_CPU_x86_KVM_HV_TLBFLUSH, >- 0x40000004, 0x00000004); >+ 0x40000004, 0x00000004, 0x0); > KVM_FEATURE_DEF(VIR_CPU_x86_KVM_HV_IPI, >- 0x40000004, 0x00000400); >+ 0x40000004, 0x00000400, 0x0); > KVM_FEATURE_DEF(VIR_CPU_x86_KVM_HV_EVMCS, >- 0x40000004, 0x00004000); >+ 0x40000004, 0x00004000, 0x0); >+KVM_FEATURE_DEF(VIR_CPU_x86_KVM_HV_STIMER_DIRECT, >+ 0x40000003, 0x0, 0x00080000); > > static virCPUx86Feature x86_kvm_features[] = > { >@@ -142,6 +144,7 @@ static virCPUx86Feature x86_kvm_features[] = > KVM_FEATURE(VIR_CPU_x86_KVM_HV_TLBFLUSH), > KVM_FEATURE(VIR_CPU_x86_KVM_HV_IPI), > KVM_FEATURE(VIR_CPU_x86_KVM_HV_EVMCS), >+ KVM_FEATURE(VIR_CPU_x86_KVM_HV_STIMER_DIRECT), > }; > > typedef struct _virCPUx86Model virCPUx86Model; >diff --git a/src/cpu/cpu_x86_data.h b/src/cpu/cpu_x86_data.h >index f3f4d7ab9c..198f19e037 100644 >--- a/src/cpu/cpu_x86_data.h >+++ b/src/cpu/cpu_x86_data.h >@@ -72,6 +72,8 @@ struct _virCPUx86MSR { > #define VIR_CPU_x86_KVM_HV_IPI "__kvm_hv_ipi" > #define VIR_CPU_x86_KVM_HV_EVMCS "__kvm_hv_evmcs" > >+/* Hyper-V Synthetic Timer (virDomainHypervStimer) features */ >+#define VIR_CPU_x86_KVM_HV_STIMER_DIRECT "__kvm_hv_stimer_direct" "hv-stimer-direct" to correctly detect its availability

> > #define VIR_CPU_X86_DATA_INIT { 0 } > Jano