[libvirt] [PATCH v2 0/7] Check TSC frequency before starting QEMU
Version 2: - new patches (1 and 2) fixing qemuargv2xmltest, which was probing host CPU 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. And to let libvirt client decide whether it should even start the migration to a specific host, this series adds host's TSC frequency and scaling support to the host CPU capabilities XML. https://bugzilla.redhat.com/show_bug.cgi?id=1641702 Jiri Denemark (7): qemu: Make virQEMUCapsProbeHostCPUForEmulator more generic qemuargv2xmltest: Use mocked virQEMUCapsProbeHostCPU util: Add virHostCPUGetTscInfo conf: Report TSC frequency in host CPU capabilities cpu_x86: Fix placement of *CheckFeature functions cpu_x86: Probe TSC frequency and scaling support qemu: Check TSC frequency before starting QEMU src/conf/cpu_conf.c | 48 +++++++++++++++++++++ src/conf/cpu_conf.h | 2 + src/cpu/cpu_x86.c | 81 ++++++++++++++++++++---------------- src/qemu/qemu_capabilities.c | 14 +++---- src/qemu/qemu_capspriv.h | 5 +-- src/qemu/qemu_process.c | 53 +++++++++++++++++++++++ src/util/virhostcpu.c | 71 +++++++++++++++++++++++++++++++ src/util/virhostcpu.h | 11 +++++ tests/Makefile.am | 3 +- tests/qemuargv2xmltest.c | 3 +- tests/qemucpumock.c | 5 +-- 11 files changed, 243 insertions(+), 53 deletions(-) -- 2.21.0
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@redhat.com> --- 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
On Mon, Jun 03, 2019 at 02:27:52PM +0200, Jiri Denemark wrote:
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@redhat.com> --- src/qemu/qemu_capabilities.c | 14 ++++++-------- src/qemu/qemu_capspriv.h | 5 ++--- tests/qemucpumock.c | 5 ++--- 3 files changed, 10 insertions(+), 14 deletions(-)
Reviewed-by: Ján Tomko <jtomko@redhat.com> Jano
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@redhat.com> --- 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 Mon, Jun 03, 2019 at 02:27:53PM +0200, Jiri Denemark wrote:
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@redhat.com> --- tests/Makefile.am | 3 ++- tests/qemuargv2xmltest.c | 3 ++- 2 files changed, 4 insertions(+), 2 deletions(-)
Reviewed-by: Ján Tomko <jtomko@redhat.com> Jano
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@redhat.com> --- 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
On Mon, Jun 03, 2019 at 02:27:54PM +0200, Jiri Denemark wrote:
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@redhat.com> --- src/util/virhostcpu.c | 71 +++++++++++++++++++++++++++++++++++++++++++ src/util/virhostcpu.h | 11 +++++++ 2 files changed, 82 insertions(+)
Reviewed-by: Ján Tomko <jtomko@redhat.com> Jano
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@redhat.com> --- 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
On Mon, Jun 03, 2019 at 02:27:55PM +0200, Jiri Denemark wrote:
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@redhat.com> --- src/conf/cpu_conf.c | 48 +++++++++++++++++++++++++++++++++++++++++++++ src/conf/cpu_conf.h | 2 ++ 2 files changed, 50 insertions(+)
Reviewed-by: Ján Tomko <jtomko@redhat.com> Jano
Commit 0a97486e09 moved them outside #ifdef, but after virCPUx86GetHost, which will start calling them in the following patch. Signed-off-by: Jiri Denemark <jdenemar@redhat.com> --- 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 02:27:56PM +0200, Jiri Denemark wrote:
Commit 0a97486e09 moved them outside #ifdef, but after virCPUx86GetHost, which will start calling them in the following patch.
Signed-off-by: Jiri Denemark <jdenemar@redhat.com> --- 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)
Unrelated whitespace change. Please push it separately. Reviewed-by: Ján Tomko <jtomko@redhat.com> Jano
On Mon, Jun 03, 2019 at 14:51:41 +0200, Ján Tomko wrote:
On Mon, Jun 03, 2019 at 02:27:56PM +0200, Jiri Denemark wrote:
Commit 0a97486e09 moved them outside #ifdef, but after virCPUx86GetHost, which will start calling them in the following patch.
Signed-off-by: Jiri Denemark <jdenemar@redhat.com> --- 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)
Unrelated whitespace change. Please push it separately.
It's actually related since it was introduced by commit 0a97486e09 :-) I reverted that commit and moved the function to the right place. Jirka
On Mon, Jun 03, 2019 at 14:51:41 +0200, Ján Tomko wrote:
On Mon, Jun 03, 2019 at 02:27:56PM +0200, Jiri Denemark wrote:
Commit 0a97486e09 moved them outside #ifdef, but after virCPUx86GetHost, which will start calling them in the following patch.
Signed-off-by: Jiri Denemark <jdenemar@redhat.com> --- src/cpu/cpu_x86.c | 72 +++++++++++++++++++++++------------------------ 1 file changed, 35 insertions(+), 37 deletions(-) ... @@ -2707,8 +2742,6 @@ cpuidSet(uint32_t base, virCPUDataPtr data) }
- - static int virCPUx86GetHost(virCPUDefPtr cpu, virDomainCapsCPUModelsPtr models)
Unrelated whitespace change. Please push it separately.
OK, I pushed the whitespace change now. The rest will go after the release. 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@redhat.com> --- 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
On Mon, Jun 03, 2019 at 02:27:57PM +0200, Jiri Denemark wrote:
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@redhat.com> --- src/cpu/cpu_x86.c | 9 +++++++++ 1 file changed, 9 insertions(+)
Reviewed-by: Ján Tomko <jtomko@redhat.com> Jano
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@redhat.com> --- 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
On Mon, Jun 03, 2019 at 02:27:58PM +0200, Jiri Denemark wrote:
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@redhat.com> --- src/qemu/qemu_process.c | 53 +++++++++++++++++++++++++++++++++++++++++ 1 file changed, 53 insertions(+)
Reviewed-by: Ján Tomko <jtomko@redhat.com> Jano
On Mon, Jun 03, 2019 at 14:52:40 +0200, Ján Tomko wrote:
On Mon, Jun 03, 2019 at 02:27:58PM +0200, Jiri Denemark wrote:
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@redhat.com> --- src/qemu/qemu_process.c | 53 +++++++++++++++++++++++++++++++++++++++++ 1 file changed, 53 insertions(+)
Reviewed-by: Ján Tomko <jtomko@redhat.com>
Thanks for the review and I'm sorry for forgetting to add the Reviewed-by line into the commits before pushing. Jirka
participants (2)
-
Jiri Denemark -
Ján Tomko