The function is renamed as virQEMUCapsProbeHostCPU and it does not get the list of allowed CPU models from qemuCaps anymore. This is responsibility is moved to the caller. The result is just a very thin wrapper around virCPUGetHost mostly required mocking in tests. The generic function is used in place of a direct call to virCPUGetHost in virQEMUCapsInitHostCPUModel to make sure tests don't accidentally probe host CPU. Signed-off-by: Jiri Denemark <jdenemar(a)redhat.com&gt; --- src/qemu/qemu_capabilities.c | 14 ++++++-------- src/qemu/qemu_capspriv.h | 5 ++--- tests/qemucpumock.c | 5 ++--- 3 files changed, 10 insertions(+), 14 deletions(-) diff --git a/src/qemu/qemu_capabilities.c b/src/qemu/qemu_capabilities.c index a827bd24e3..5cddae06c6 100644 --- a/src/qemu/qemu_capabilities.c +++ b/src/qemu/qemu_capabilities.c @@ -880,12 +880,10 @@ virQEMUCapsInitGuestFromBinary(virCapsPtr caps, virCPUDefPtr -virQEMUCapsProbeHostCPUForEmulator(virArch hostArch, - virQEMUCapsPtr qemuCaps, - virDomainVirtType type) +virQEMUCapsProbeHostCPU(virArch hostArch, + virDomainCapsCPUModelsPtr models) { - return virCPUGetHost(hostArch, VIR_CPU_TYPE_GUEST, NULL, - virQEMUCapsGetCPUDefinitions(qemuCaps, type)); + return virCPUGetHost(hostArch, VIR_CPU_TYPE_GUEST, NULL, models); } @@ -3049,7 +3047,8 @@ virQEMUCapsInitHostCPUModel(virQEMUCapsPtr qemuCaps, } else if (rc == 1) { VIR_DEBUG("No host CPU model info from QEMU; probing host CPU directly"); - hostCPU = virQEMUCapsProbeHostCPUForEmulator(hostArch, qemuCaps, type); + hostCPU = virQEMUCapsProbeHostCPU(hostArch, + virQEMUCapsGetCPUDefinitions(qemuCaps, type)); if (!hostCPU || virCPUDefCopyModelFilter(cpu, hostCPU, true, virQEMUCapsCPUFilterFeatures, @@ -3062,8 +3061,7 @@ virQEMUCapsInitHostCPUModel(virQEMUCapsPtr qemuCaps, goto error; } else if (type == VIR_DOMAIN_VIRT_KVM && virCPUGetHostIsSupported(qemuCaps->arch)) { - if (!(fullCPU = virCPUGetHost(qemuCaps->arch, VIR_CPU_TYPE_GUEST, - NULL, NULL))) + if (!(fullCPU = virQEMUCapsProbeHostCPU(qemuCaps->arch, NULL))) goto error; if (!(cpuExpanded = virCPUDefCopy(cpu)) || diff --git a/src/qemu/qemu_capspriv.h b/src/qemu/qemu_capspriv.h index 2d059bee8c..3c129cbf6c 100644 --- a/src/qemu/qemu_capspriv.h +++ b/src/qemu/qemu_capspriv.h @@ -82,9 +82,8 @@ virQEMUCapsGetCPUModelX86Data(qemuMonitorCPUModelInfoPtr model, bool migratable); virCPUDefPtr -virQEMUCapsProbeHostCPUForEmulator(virArch hostArch, - virQEMUCapsPtr qemuCaps, - virDomainVirtType type) ATTRIBUTE_NOINLINE; +virQEMUCapsProbeHostCPU(virArch hostArch, + virDomainCapsCPUModelsPtr models) ATTRIBUTE_NOINLINE; void virQEMUCapsSetGICCapabilities(virQEMUCapsPtr qemuCaps, diff --git a/tests/qemucpumock.c b/tests/qemucpumock.c index e028ada8eb..501738df36 100644 --- a/tests/qemucpumock.c +++ b/tests/qemucpumock.c @@ -27,9 +27,8 @@ virCPUDefPtr -virQEMUCapsProbeHostCPUForEmulator(virArch hostArch ATTRIBUTE_UNUSED, - virQEMUCapsPtr qemuCaps ATTRIBUTE_UNUSED, - virDomainVirtType type ATTRIBUTE_UNUSED) +virQEMUCapsProbeHostCPU(virArch hostArch ATTRIBUTE_UNUSED, + virDomainCapsCPUModelsPtr models ATTRIBUTE_UNUSED) { const char *model = getenv("VIR_TEST_MOCK_FAKE_HOST_CPU"); -- 2.21.0

The qemuTestParseCapabilitiesArch call would eventually lead to the host CPU being probed via virCPUGetHost. Let's divert this to a mocked version already used by the qemuxml2argvtest. Signed-off-by: Jiri Denemark <jdenemar(a)redhat.com&gt; --- tests/Makefile.am | 3 ++- tests/qemuargv2xmltest.c | 3 ++- 2 files changed, 4 insertions(+), 2 deletions(-) diff --git a/tests/Makefile.am b/tests/Makefile.am index 67375b3c19..6b17d99501 100644 --- a/tests/Makefile.am +++ b/tests/Makefile.am @@ -585,7 +585,8 @@ qemuxml2xmltest_LDADD = $(qemu_LDADDS) $(LDADDS) qemuargv2xmltest_SOURCES = \ qemuargv2xmltest.c testutilsqemu.c testutilsqemu.h \ testutils.c testutils.h -qemuargv2xmltest_LDADD = $(qemu_LDADDS) $(LDADDS) +qemuargv2xmltest_LDADD = libqemutestdriver.la \ + $(LDADDS) qemumonitorjsontest_SOURCES = \ qemumonitorjsontest.c \ diff --git a/tests/qemuargv2xmltest.c b/tests/qemuargv2xmltest.c index 2a51e22318..06e32898a2 100644 --- a/tests/qemuargv2xmltest.c +++ b/tests/qemuargv2xmltest.c @@ -309,7 +309,8 @@ mymain(void) return ret == 0 ? EXIT_SUCCESS : EXIT_FAILURE; } -VIR_TEST_MAIN(mymain) +VIR_TEST_MAIN_PRELOAD(mymain, + abs_builddir "/.libs/qemucpumock.so") #else -- 2.21.0

On a KVM x86_64 host which supports invariant TSC this function can be used to detect the TSC frequency and the availability of TSC scaling. The magic MSR numbers required to check if VMX scaling is supported on the host are documented in Volume 3 of the Intel® 64 and IA-32 Architectures Software Developer’s Manual. Signed-off-by: Jiri Denemark <jdenemar(a)redhat.com&gt; --- src/util/virhostcpu.c | 71 +++++++++++++++++++++++++++++++++++++++++++ src/util/virhostcpu.h | 11 +++++++ 2 files changed, 82 insertions(+) diff --git a/src/util/virhostcpu.c b/src/util/virhostcpu.c index f4a62c74fa..8c00804b0e 100644 --- a/src/util/virhostcpu.c +++ b/src/util/virhostcpu.c @@ -1324,6 +1324,69 @@ virHostCPUGetMSR(unsigned long index, return virHostCPUGetMSRFromKVM(index, msr); } + +# define VMX_PROCBASED_CTLS2_MSR 0x48b +# define VMX_USE_TSC_SCALING (1 << 25) + +/* + * This function should only be called when the host CPU supports invariant TSC + * (invtsc CPUID feature). + * + * Returns pointer to the TSC info structure on success, + * NULL when TSC cannot be probed otherwise. + */ +virHostCPUTscInfoPtr +virHostCPUGetTscInfo(void) +{ + virHostCPUTscInfoPtr info; + VIR_AUTOCLOSE kvmFd = -1; + VIR_AUTOCLOSE vmFd = -1; + VIR_AUTOCLOSE vcpuFd = -1; + uint64_t msr = 0; + int rc; + + if ((kvmFd = open(KVM_DEVICE, O_RDONLY)) < 0) { + virReportSystemError(errno, _("Unable to open %s"), KVM_DEVICE); + return NULL; + } + + if ((vmFd = ioctl(kvmFd, KVM_CREATE_VM, 0)) < 0) { + virReportSystemError(errno, "%s", + _("Unable to create KVM VM for TSC probing")); + return NULL; + } + + if ((vcpuFd = ioctl(vmFd, KVM_CREATE_VCPU, 0)) < 0) { + virReportSystemError(errno, "%s", + _("Unable to create KVM vCPU for TSC probing")); + return NULL; + } + + if ((rc = ioctl(vcpuFd, KVM_GET_TSC_KHZ)) < 0) { + virReportSystemError(errno, "%s", + _("Unable to probe TSC frequency")); + return NULL; + } + + if (VIR_ALLOC(info) < 0) + return NULL; + + info->frequency = rc * 1000ULL; + + if (virHostCPUGetMSR(VMX_PROCBASED_CTLS2_MSR, &msr) == 0) { + /* High 32 bits of the MSR value indicate whether specific control + * can be set to 1. */ + msr >>= 32; + + info->scaling = virTristateBoolFromBool(!!(msr & VMX_USE_TSC_SCALING)); + } + + VIR_DEBUG("Detected TSC frequency %llu Hz, scaling %s", + info->frequency, virTristateBoolTypeToString(info->scaling)); + + return info; +} + #else int @@ -1335,6 +1398,14 @@ virHostCPUGetMSR(unsigned long index ATTRIBUTE_UNUSED, return -1; } +virHostCPUTscInfoPtr +virHostCPUGetTscInfo(void) +{ + virReportSystemError(ENOSYS, "%s", + _("Probing TSC is not supported on this platform")); + return NULL; +} + #endif /* HAVE_LINUX_KVM_H && defined(KVM_GET_MSRS) && \ (defined(__i386__) || defined(__x86_64__)) && \ (defined(__linux__) || defined(__FreeBSD__)) */ diff --git a/src/util/virhostcpu.h b/src/util/virhostcpu.h index 0d20dbef61..b822bc11a8 100644 --- a/src/util/virhostcpu.h +++ b/src/util/virhostcpu.h @@ -25,6 +25,15 @@ # include "internal.h" # include "virarch.h" # include "virbitmap.h" +# include "virenum.h" + + +typedef struct _virHostCPUTscInfo virHostCPUTscInfo; +typedef virHostCPUTscInfo *virHostCPUTscInfoPtr; +struct _virHostCPUTscInfo { + unsigned long long frequency; + virTristateBool scaling; +}; int virHostCPUGetStats(int cpuNum, @@ -69,4 +78,6 @@ unsigned int virHostCPUGetMicrocodeVersion(void); int virHostCPUGetMSR(unsigned long index, uint64_t *msr); +virHostCPUTscInfoPtr virHostCPUGetTscInfo(void); + #endif /* LIBVIRT_VIRHOSTCPU_H */ -- 2.21.0

This patch adds a new <counter name='tsc' frequency='N' scaling='on|off'/> element into the host CPU capabilities XML. Signed-off-by: Jiri Denemark <jdenemar(a)redhat.com&gt; --- src/conf/cpu_conf.c | 48 +++++++++++++++++++++++++++++++++++++++++++++ src/conf/cpu_conf.h | 2 ++ 2 files changed, 50 insertions(+) diff --git a/src/conf/cpu_conf.c b/src/conf/cpu_conf.c index bd2beab33e..dc46e7f57a 100644 --- a/src/conf/cpu_conf.c +++ b/src/conf/cpu_conf.c @@ -112,6 +112,7 @@ virCPUDefFree(virCPUDefPtr def) virCPUDefFreeModel(def); VIR_FREE(def->cache); + VIR_FREE(def->tsc); VIR_FREE(def); } @@ -233,6 +234,13 @@ virCPUDefCopyWithoutModel(const virCPUDef *cpu) *copy->cache = *cpu->cache; } + if (cpu->tsc) { + if (VIR_ALLOC(copy->tsc) < 0) + goto error; + + *copy->tsc = *cpu->tsc; + } + return copy; error: @@ -286,6 +294,8 @@ virCPUDefParseXML(xmlXPathContextPtr ctxt, char *cpuMode; char *fallback = NULL; char *vendor_id = NULL; + char *tscScaling = NULL; + virHostCPUTscInfoPtr tsc = NULL; int ret = -1; *cpu = NULL; @@ -402,6 +412,32 @@ virCPUDefParseXML(xmlXPathContextPtr ctxt, _("invalid microcode version")); goto cleanup; } + + if (virXPathBoolean("boolean(./counter[@name='tsc'])", ctxt) > 0) { + if (VIR_ALLOC(tsc) < 0) + goto cleanup; + + if (virXPathULongLong("./counter[@name='tsc']/@frequency", ctxt, + &tsc->frequency) < 0) { + virReportError(VIR_ERR_CONFIG_UNSUPPORTED, "%s", + _("Invalid TSC frequency")); + goto cleanup; + } + + tscScaling = virXPathString("string(./counter[@name='tsc']/@scaling)", + ctxt); + if (tscScaling) { + int scaling = virTristateBoolTypeFromString(tscScaling); + if (scaling < 0) { + virReportError(VIR_ERR_CONFIG_UNSUPPORTED, "%s", + _("Invalid TSC scaling attribute")); + goto cleanup; + } + tsc->scaling = scaling; + } + + VIR_STEAL_PTR(def->tsc, tsc); + } } if (!(def->model = virXPathString("string(./model[1])", ctxt)) && @@ -587,6 +623,8 @@ virCPUDefParseXML(xmlXPathContextPtr ctxt, VIR_FREE(fallback); VIR_FREE(vendor_id); VIR_FREE(nodes); + VIR_FREE(tscScaling); + VIR_FREE(tsc); virCPUDefFree(def); return ret; } @@ -744,6 +782,16 @@ virCPUDefFormatBuf(virBufferPtr buf, virBufferAsprintf(buf, "<microcode version='%u'/>\n", def->microcodeVersion); + if (def->type == VIR_CPU_TYPE_HOST && def->tsc) { + virBufferAddLit(buf, "<counter name='tsc'"); + virBufferAsprintf(buf, " frequency='%llu'", def->tsc->frequency); + if (def->tsc->scaling) { + virBufferAsprintf(buf, " scaling='%s'", + virTristateBoolTypeToString(def->tsc->scaling)); + } + virBufferAddLit(buf, "/>\n"); + } + if (def->sockets && def->cores && def->threads) { virBufferAddLit(buf, "<topology"); virBufferAsprintf(buf, " sockets='%u'", def->sockets); diff --git a/src/conf/cpu_conf.h b/src/conf/cpu_conf.h index c98db65693..51bf744fab 100644 --- a/src/conf/cpu_conf.h +++ b/src/conf/cpu_conf.h @@ -28,6 +28,7 @@ # include "virarch.h" # include "numa_conf.h" # include "virenum.h" +# include "virhostcpu.h" # define VIR_CPU_VENDOR_ID_LENGTH 12 @@ -139,6 +140,7 @@ struct _virCPUDef { size_t nfeatures_max; virCPUFeatureDefPtr features; virCPUCacheDefPtr cache; + virHostCPUTscInfoPtr tsc; }; -- 2.21.0

Commit 0a97486e09 moved them outside #ifdef, but after virCPUx86GetHost, which will start calling them in the following patch. Signed-off-by: Jiri Denemark <jdenemar(a)redhat.com&gt; --- src/cpu/cpu_x86.c | 72 +++++++++++++++++++++++------------------------ 1 file changed, 35 insertions(+), 37 deletions(-) diff --git a/src/cpu/cpu_x86.c b/src/cpu/cpu_x86.c index f05bfa24e0..cb03123787 100644 --- a/src/cpu/cpu_x86.c +++ b/src/cpu/cpu_x86.c @@ -2378,6 +2378,41 @@ x86Encode(virArch arch, } +static int +virCPUx86CheckFeature(const virCPUDef *cpu, + const char *name) +{ + int ret = -1; + virCPUx86MapPtr map; + virCPUx86ModelPtr model = NULL; + + if (!(map = virCPUx86GetMap())) + return -1; + + if (!(model = x86ModelFromCPU(cpu, map, -1))) + goto cleanup; + + ret = x86FeatureInData(name, &model->data, map); + + cleanup: + x86ModelFree(model); + return ret; +} + + +static int +virCPUx86DataCheckFeature(const virCPUData *data, + const char *name) +{ + virCPUx86MapPtr map; + + if (!(map = virCPUx86GetMap())) + return -1; + + return x86FeatureInData(name, &data->data.x86, map); +} + + #if defined(__i386__) || defined(__x86_64__) static inline void cpuidCall(virCPUx86CPUID *cpuid) @@ -2707,8 +2742,6 @@ cpuidSet(uint32_t base, virCPUDataPtr data) } - - static int virCPUx86GetHost(virCPUDefPtr cpu, virDomainCapsCPUModelsPtr models) @@ -2736,41 +2769,6 @@ virCPUx86GetHost(virCPUDefPtr cpu, #endif -static int -virCPUx86CheckFeature(const virCPUDef *cpu, - const char *name) -{ - int ret = -1; - virCPUx86MapPtr map; - virCPUx86ModelPtr model = NULL; - - if (!(map = virCPUx86GetMap())) - return -1; - - if (!(model = x86ModelFromCPU(cpu, map, -1))) - goto cleanup; - - ret = x86FeatureInData(name, &model->data, map); - - cleanup: - x86ModelFree(model); - return ret; -} - - -static int -virCPUx86DataCheckFeature(const virCPUData *data, - const char *name) -{ - virCPUx86MapPtr map; - - if (!(map = virCPUx86GetMap())) - return -1; - - return x86FeatureInData(name, &data->data.x86, map); -} - - static virCPUDefPtr virCPUx86Baseline(virCPUDefPtr *cpus, unsigned int ncpus, -- 2.21.0

On Mon, Jun 03, 2019 at 14:51:41 +0200, Ján Tomko wrote: It's actually related since it was introduced by commit 0a97486e09 :-) I reverted that commit and moved the function to the right place. Jirka

When the host CPU supports invariant TSC the host CPU definition created by virCPUx86GetHost will contain (unless probing fails for some reason) addition TSC related data. Signed-off-by: Jiri Denemark <jdenemar(a)redhat.com&gt; --- src/cpu/cpu_x86.c | 9 +++++++++ 1 file changed, 9 insertions(+) diff --git a/src/cpu/cpu_x86.c b/src/cpu/cpu_x86.c index cb03123787..689b6cdaf5 100644 --- a/src/cpu/cpu_x86.c +++ b/src/cpu/cpu_x86.c @@ -2762,6 +2762,15 @@ virCPUx86GetHost(virCPUDefPtr cpu, ret = x86DecodeCPUData(cpu, cpuData, models); cpu->microcodeVersion = virHostCPUGetMicrocodeVersion(); + /* Probing for TSC frequency makes sense only if the CPU supports + * invariant TSC (Linux calls this constant_tsc in /proc/cpuinfo). */ + if (virCPUx86DataCheckFeature(cpuData, "invtsc") == 1) { + VIR_DEBUG("Checking invariant TSC frequency"); + cpu->tsc = virHostCPUGetTscInfo(); + } else { + VIR_DEBUG("Host CPU does not support invariant TSC"); + } + cleanup: virCPUx86DataFree(cpuData); return ret; -- 2.21.0

When migrating a domain with invtsc CPU feature enabled, the TSC frequency of the destination host must match the frequency used when the domain was started on the source host or the destination host has to support TSC scaling. If the frequencies do not match and the destination host does not support TSC scaling, QEMU will fail to set the right TSC frequency when starting vCPUs on the destination and thus migration will fail. However, this is quite late since both host might have spent significant time transferring memory and perhaps even storage data. By adding the check to libvirt we can let migration fail before any data starts to be sent over. If for some reason libvirt is unable to detect the host's TSC frequency or scaling support, we'll just let QEMU try and the migration will either succeed or fail later. Luckily, we mandate TSC frequency to be explicitly set in the domain XML to even allow migration of domains with invtsc. We can just check whether the requested frequency is compatible with the current host before starting QEMU. https://bugzilla.redhat.com/show_bug.cgi?id=1641702 Signed-off-by: Jiri Denemark <jdenemar(a)redhat.com&gt; --- src/qemu/qemu_process.c | 53 +++++++++++++++++++++++++++++++++++++++++ 1 file changed, 53 insertions(+) diff --git a/src/qemu/qemu_process.c b/src/qemu/qemu_process.c index fa5909e9be..481daa937e 100644 --- a/src/qemu/qemu_process.c +++ b/src/qemu/qemu_process.c @@ -5282,6 +5282,56 @@ qemuProcessStartValidateDisks(virDomainObjPtr vm, } +static int +qemuProcessStartValidateTSC(virDomainObjPtr vm, + virCapsPtr caps) +{ + size_t i; + unsigned long long freq = 0; + virHostCPUTscInfoPtr tsc; + + for (i = 0; i < vm->def->clock.ntimers; i++) { + virDomainTimerDefPtr timer = vm->def->clock.timers[i]; + + if (timer->name == VIR_DOMAIN_TIMER_NAME_TSC && + timer->frequency > 0) { + freq = timer->frequency; + break; + } + } + + if (freq == 0) + return 0; + + VIR_DEBUG("Requested TSC frequency %llu Hz", freq); + + if (!caps->host.cpu || !caps->host.cpu->tsc) { + VIR_DEBUG("Host TSC frequency could not be probed"); + return 0; + } + + tsc = caps->host.cpu->tsc; + VIR_DEBUG("Host TSC frequency %llu Hz, scaling %s", + tsc->frequency, virTristateBoolTypeToString(tsc->scaling)); + + if (freq == tsc->frequency || tsc->scaling == VIR_TRISTATE_BOOL_YES) + return 0; + + if (tsc->scaling == VIR_TRISTATE_BOOL_ABSENT) { + VIR_DEBUG("TSC frequencies do not match and scaling support is " + "unknown, QEMU will try and possibly fail later"); + return 0; + } + + virReportError(VIR_ERR_CONFIG_UNSUPPORTED, + _("Requested TSC frequency %llu Hz does not match " + "host (%llu Hz) and TSC scaling is not supported " + "by the host CPU"), + freq, tsc->frequency); + return -1; +} + + /** * qemuProcessStartValidate: * @vm: domain object @@ -5346,6 +5396,9 @@ qemuProcessStartValidate(virQEMUDriverPtr driver, if (qemuProcessStartValidateDisks(vm, qemuCaps) < 0) return -1; + if (qemuProcessStartValidateTSC(vm, caps) < 0) + return -1; + VIR_DEBUG("Checking for any possible (non-fatal) issues"); qemuProcessStartWarnShmem(vm); -- 2.21.0