Ján Tomko <jtomko@xxxxxxxxxx> writes:
> 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@xxxxxxxxxx>
>>---
>> 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 @@
>> <vpindex state='on'/>
>> <runtime state='on'/>
>> <synic state='on'/>
>>- <stimer state='on'/>
>>+ <stimer state='on'>
>>+ <direct state='on'/>
>>+ </stimer>
>> <reset state='on'/>
>> <vendor_id state='on' value='KVM Hv'/>
>> <frequencies state='on'/>
>>@@ -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.
>
I don't anticipate any sub-features for stimer for the time being,
however, I wanted to make code look like what we already have
(e.g. virDomainKVM). We can, of course, simplify things if we consider
'direct' being the one and only.
>>+
>> 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.
Ok.
>
>> 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;
>
Yes, if we decide to drop the virDomainHypervStimer.
>> 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.
>
Yes if we still need it) I haven't looked at Jirka's series to check if
we still check CPUID for Hyper-V features or we only check QEMU command
line now.
>> }
>>
>> #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
>
Yes, I'll rebase on top of Jirka's series. Thanks!
>>
>> #define VIR_CPU_X86_DATA_INIT { 0 }
>>
>
> Jano
--
Vitaly
--
libvir-list mailing list
libvir-list@xxxxxxxxxx
https://www.redhat.com/mailman/listinfo/libvir-list
