[libvirt] [PATCH v2 0/7] Finish support for IA32_ARCH_CAPABILITIES MSR features
As promised in v1 this series contains a few additional patches which limit usage of MSR features to QEMU that supports "unavailable-features" CPU property. This series intentionally enables MSR features for all QEMU versions before restricting the usage to make this restriction more visible in the test results. Version 2: - all but 2 patches from version 1 were pushed - 5 new patches Jiri Denemark (7): conf: Introduce virCPUDefCheckFeatures cpu_x86: Turn virCPUx86DataIteratorInit into a function cpu_x86: Introduce virCPUx86FeatureIsMSR cpu_x86: Read CPU features from IA32_ARCH_CAPABILITIES MSR cpu_map: Introduce IA32_ARCH_CAPABILITIES MSR features qemu: Forbid MSR features with old QEMU qemu: Drop MSR features from host-model with old QEMU src/conf/cpu_conf.c | 33 +++++++ src/conf/cpu_conf.h | 6 ++ src/cpu/cpu_x86.c | 96 +++++++++++++++++-- src/cpu/cpu_x86.h | 3 + src/cpu_map/x86_features.xml | 20 ++++ src/libvirt_private.syms | 2 + src/qemu/qemu_capabilities.c | 16 ++++ src/qemu/qemu_process.c | 29 +++++- .../x86_64-cpuid-Core-i7-7600U-enabled.xml | 1 + .../x86_64-cpuid-Core-i7-7600U-json.xml | 1 + ...86_64-cpuid-Xeon-Platinum-8268-enabled.xml | 1 + .../x86_64-cpuid-Xeon-Platinum-8268-guest.xml | 4 + .../x86_64-cpuid-Xeon-Platinum-8268-host.xml | 4 + .../x86_64-cpuid-Xeon-Platinum-8268-json.xml | 3 + .../qemu_4.1.0.x86_64.xml | 1 + 15 files changed, 207 insertions(+), 13 deletions(-) -- 2.22.0
This API can be used to check whether a CPU definition contains features matching a given filter. Signed-off-by: Jiri Denemark <jdenemar@redhat.com> --- src/conf/cpu_conf.c | 33 +++++++++++++++++++++++++++++++++ src/conf/cpu_conf.h | 6 ++++++ src/libvirt_private.syms | 1 + 3 files changed, 40 insertions(+) diff --git a/src/conf/cpu_conf.c b/src/conf/cpu_conf.c index 675d214c50..7d16a05283 100644 --- a/src/conf/cpu_conf.c +++ b/src/conf/cpu_conf.c @@ -930,6 +930,39 @@ virCPUDefFilterFeatures(virCPUDefPtr cpu, } +/** + * virCPUDefCheckFeatures: + * + * Check CPU features for which @filter reports true and store them in a NULL + * terminated list returned via @features. + * + * Returns the number of features matching @filter or -1 on error. + */ +int +virCPUDefCheckFeatures(virCPUDefPtr cpu, + virCPUDefFeatureFilter filter, + void *opaque, + char ***features) +{ + VIR_AUTOSTRINGLIST list = NULL; + size_t n = 0; + size_t i; + + *features = NULL; + + for (i = 0; i < cpu->nfeatures; i++) { + if (filter(cpu->features[i].name, opaque)) { + if (virStringListAdd(&list, cpu->features[i].name) < 0) + return -1; + n++; + } + } + + VIR_STEAL_PTR(*features, list); + return n; +} + + bool virCPUDefIsEqual(virCPUDefPtr src, virCPUDefPtr dst, diff --git a/src/conf/cpu_conf.h b/src/conf/cpu_conf.h index 906ef5368e..19ce816ec2 100644 --- a/src/conf/cpu_conf.h +++ b/src/conf/cpu_conf.h @@ -224,6 +224,12 @@ virCPUDefFilterFeatures(virCPUDefPtr cpu, virCPUDefFeatureFilter filter, void *opaque); +int +virCPUDefCheckFeatures(virCPUDefPtr cpu, + virCPUDefFeatureFilter filter, + void *opaque, + char ***features); + virCPUDefPtr * virCPUDefListParse(const char **xmlCPUs, unsigned int ncpus, diff --git a/src/libvirt_private.syms b/src/libvirt_private.syms index ab2e4bc6fe..90a6d10666 100644 --- a/src/libvirt_private.syms +++ b/src/libvirt_private.syms @@ -73,6 +73,7 @@ virCapabilitiesSetNetPrefix; virCPUCacheModeTypeFromString; virCPUCacheModeTypeToString; virCPUDefAddFeature; +virCPUDefCheckFeatures; virCPUDefCopy; virCPUDefCopyModel; virCPUDefCopyModelFilter; -- 2.22.0
On Thu, Jun 20, 2019 at 12:53:36AM +0200, Jiri Denemark wrote:
This API can be used to check whether a CPU definition contains features matching a given filter.
Signed-off-by: Jiri Denemark <jdenemar@redhat.com> --- src/conf/cpu_conf.c | 33 +++++++++++++++++++++++++++++++++ src/conf/cpu_conf.h | 6 ++++++ src/libvirt_private.syms | 1 + 3 files changed, 40 insertions(+)
Reviewed-by: Ján Tomko <jtomko@redhat.com> Jano
Until now, this was a macro usable for direct initialization when a variable is defined. Turning the macro into a function makes it more general. Signed-off-by: Jiri Denemark <jdenemar@redhat.com> --- src/cpu/cpu_x86.c | 34 ++++++++++++++++++++++++---------- 1 file changed, 24 insertions(+), 10 deletions(-) diff --git a/src/cpu/cpu_x86.c b/src/cpu/cpu_x86.c index b6a94d483a..6eef5cef00 100644 --- a/src/cpu/cpu_x86.c +++ b/src/cpu/cpu_x86.c @@ -189,8 +189,13 @@ struct _virCPUx86DataIterator { }; -#define virCPUx86DataIteratorInit(data) \ - { data, -1 } +static void +virCPUx86DataIteratorInit(virCPUx86DataIteratorPtr iterator, + const virCPUx86Data *data) +{ + virCPUx86DataIterator iter = { data, -1 }; + *iterator = iter; +} static bool @@ -540,9 +545,10 @@ static int x86DataAdd(virCPUx86Data *data1, const virCPUx86Data *data2) { - virCPUx86DataIterator iter = virCPUx86DataIteratorInit(data2); + virCPUx86DataIterator iter; virCPUx86DataItemPtr item; + virCPUx86DataIteratorInit(&iter, data2); while ((item = virCPUx86DataNext(&iter))) { if (virCPUx86DataAddItem(data1, item) < 0) return -1; @@ -556,10 +562,11 @@ static void x86DataSubtract(virCPUx86Data *data1, const virCPUx86Data *data2) { - virCPUx86DataIterator iter = virCPUx86DataIteratorInit(data1); + virCPUx86DataIterator iter; virCPUx86DataItemPtr item1; virCPUx86DataItemPtr item2; + virCPUx86DataIteratorInit(&iter, data1); while ((item1 = virCPUx86DataNext(&iter))) { item2 = virCPUx86DataGet(data2, item1); virCPUx86DataItemClearBits(item1, item2); @@ -571,10 +578,11 @@ static void x86DataIntersect(virCPUx86Data *data1, const virCPUx86Data *data2) { - virCPUx86DataIterator iter = virCPUx86DataIteratorInit(data1); + virCPUx86DataIterator iter; virCPUx86DataItemPtr item1; virCPUx86DataItemPtr item2; + virCPUx86DataIteratorInit(&iter, data1); while ((item1 = virCPUx86DataNext(&iter))) { item2 = virCPUx86DataGet(data2, item1); if (item2) @@ -588,8 +596,9 @@ x86DataIntersect(virCPUx86Data *data1, static bool x86DataIsEmpty(virCPUx86Data *data) { - virCPUx86DataIterator iter = virCPUx86DataIteratorInit(data); + virCPUx86DataIterator iter; + virCPUx86DataIteratorInit(&iter, data); return !virCPUx86DataNext(&iter); } @@ -598,10 +607,11 @@ static bool x86DataIsSubset(const virCPUx86Data *data, const virCPUx86Data *subset) { - virCPUx86DataIterator iter = virCPUx86DataIteratorInit((virCPUx86Data *)subset); + virCPUx86DataIterator iter; const virCPUx86DataItem *item; const virCPUx86DataItem *itemSubset; + virCPUx86DataIteratorInit(&iter, subset); while ((itemSubset = virCPUx86DataNext(&iter))) { if (!(item = virCPUx86DataGet(data, itemSubset)) || !virCPUx86DataItemMatchMasked(item, itemSubset)) @@ -1314,11 +1324,13 @@ x86ModelCompare(virCPUx86ModelPtr model1, virCPUx86ModelPtr model2) { virCPUx86CompareResult result = EQUAL; - virCPUx86DataIterator iter1 = virCPUx86DataIteratorInit(&model1->data); - virCPUx86DataIterator iter2 = virCPUx86DataIteratorInit(&model2->data); + virCPUx86DataIterator iter1; + virCPUx86DataIterator iter2; virCPUx86DataItemPtr item1; virCPUx86DataItemPtr item2; + virCPUx86DataIteratorInit(&iter1, &model1->data); + virCPUx86DataIteratorInit(&iter2, &model2->data); while ((item1 = virCPUx86DataNext(&iter1))) { virCPUx86CompareResult match = SUPERSET; @@ -1624,10 +1636,12 @@ virCPUx86GetMap(void) static char * virCPUx86DataFormat(const virCPUData *data) { - virCPUx86DataIterator iter = virCPUx86DataIteratorInit(&data->data.x86); + virCPUx86DataIterator iter; virCPUx86DataItemPtr item; virBuffer buf = VIR_BUFFER_INITIALIZER; + virCPUx86DataIteratorInit(&iter, &data->data.x86); + virBufferAddLit(&buf, "<cpudata arch='x86'>\n"); while ((item = virCPUx86DataNext(&iter))) { virCPUx86CPUIDPtr cpuid; -- 2.22.0
On Thu, Jun 20, 2019 at 12:53:37AM +0200, Jiri Denemark wrote:
Until now, this was a macro usable for direct initialization when a variable is defined. Turning the macro into a function makes it more general.
Signed-off-by: Jiri Denemark <jdenemar@redhat.com> --- src/cpu/cpu_x86.c | 34 ++++++++++++++++++++++++---------- 1 file changed, 24 insertions(+), 10 deletions(-)
Reviewed-by: Ján Tomko <jtomko@redhat.com> Jano
This function may be used as a virCPUDefFeatureFilter callback for virCPUDefCheckFeatures, virCPUDefFilerFeatures, and similar functions to filter or pick out features reported via MSR. Signed-off-by: Jiri Denemark <jdenemar@redhat.com> --- src/cpu/cpu_x86.c | 40 ++++++++++++++++++++++++++++++++++++++++ src/cpu/cpu_x86.h | 3 +++ src/libvirt_private.syms | 1 + 3 files changed, 44 insertions(+) diff --git a/src/cpu/cpu_x86.c b/src/cpu/cpu_x86.c index 6eef5cef00..a7ec0f7095 100644 --- a/src/cpu/cpu_x86.c +++ b/src/cpu/cpu_x86.c @@ -3362,6 +3362,46 @@ virCPUx86DataAddFeature(virCPUDataPtr cpuData, } +/** + * virCPUx86FeatureIsMSR: + * @name CPU feature name + * @opaque NULL or a pointer to bool + * + * This is a callback for functions filtering features in virCPUDef. + * + * Checks whether a given CPU feature is reported via MSR. When @opaque is NULL + * or a pointer to true, the function will pick out (return true for) MSR + * features. If @opaque is a pointer to false, the logic will be inverted and + * the function will filter out (return false for) MSR features. + */ +bool +virCPUx86FeatureIsMSR(const char *name, + void *opaque) +{ + virCPUx86FeaturePtr feature; + virCPUx86DataIterator iter; + virCPUx86DataItemPtr item; + virCPUx86MapPtr map; + bool inverted = false; + + if (opaque) + inverted = !*(bool *)opaque; + + if ((!(map = virCPUx86GetMap()) || + !(feature = x86FeatureFind(map, name))) && + !(feature = x86FeatureFindInternal(name))) + return inverted ? true : false; + + virCPUx86DataIteratorInit(&iter, &feature->data); + while ((item = virCPUx86DataNext(&iter))) { + if (item->type == VIR_CPU_X86_DATA_MSR) + return inverted ? false : true; + } + + return inverted ? true : false; +} + + struct cpuArchDriver cpuDriverX86 = { .name = "x86", .arch = archs, diff --git a/src/cpu/cpu_x86.h b/src/cpu/cpu_x86.h index 28ae46647a..8ae78d5e34 100644 --- a/src/cpu/cpu_x86.h +++ b/src/cpu/cpu_x86.h @@ -40,3 +40,6 @@ uint32_t virCPUx86DataGetSignature(virCPUDataPtr cpuData, int virCPUx86DataSetVendor(virCPUDataPtr cpuData, const char *vendor); + +bool virCPUx86FeatureIsMSR(const char *name, + void *opaque); diff --git a/src/libvirt_private.syms b/src/libvirt_private.syms index 90a6d10666..aa64580d63 100644 --- a/src/libvirt_private.syms +++ b/src/libvirt_private.syms @@ -1268,6 +1268,7 @@ virCPUx86DataAdd; virCPUx86DataGetSignature; virCPUx86DataSetSignature; virCPUx86DataSetVendor; +virCPUx86FeatureIsMSR; # datatypes.h -- 2.22.0
On Thu, Jun 20, 2019 at 12:53:38AM +0200, Jiri Denemark wrote:
This function may be used as a virCPUDefFeatureFilter callback for virCPUDefCheckFeatures, virCPUDefFilerFeatures, and similar functions to filter or pick out features reported via MSR.
Signed-off-by: Jiri Denemark <jdenemar@redhat.com> --- src/cpu/cpu_x86.c | 40 ++++++++++++++++++++++++++++++++++++++++ src/cpu/cpu_x86.h | 3 +++ src/libvirt_private.syms | 1 + 3 files changed, 44 insertions(+)
diff --git a/src/cpu/cpu_x86.c b/src/cpu/cpu_x86.c index 6eef5cef00..a7ec0f7095 100644 --- a/src/cpu/cpu_x86.c +++ b/src/cpu/cpu_x86.c @@ -3362,6 +3362,46 @@ virCPUx86DataAddFeature(virCPUDataPtr cpuData, }
+/** + * virCPUx86FeatureIsMSR: + * @name CPU feature name + * @opaque NULL or a pointer to bool + * + * This is a callback for functions filtering features in virCPUDef. + * + * Checks whether a given CPU feature is reported via MSR. When @opaque is NULL + * or a pointer to true, the function will pick out (return true for) MSR + * features. If @opaque is a pointer to false, the logic will be inverted and + * the function will filter out (return false for) MSR features. + */
Uhm. What? Do I understand it correctly? @opaque | *opaque | IsMSR | return --------+---------+-------+-------- NULL | SEGV | true | true 0xBEEF | true | true | true 0xBEEF | false | true | false --------+---------+-------+-------- First, it feels odd that 'false' is the value resulting in changed behavior. I'd rather see it act differently when the bool pointer is present and points to true. Second, by using a pointer to bool AND not requiring opaque to be passed, we essentially have a tri-state bool. I'd suggest (ab)using the void pointer to store the value directly, we do have the bool var = opaque != NULL; pattern elsewhere in the code.
+bool +virCPUx86FeatureIsMSR(const char *name, + void *opaque)
Can you name this virCPUx86FeatureFilterMSR, leaving virCPUx86FeatureIsMSR for a static helper function that will perform the actual lookup and this function would (possibly) invert the result? All the ternary operators make the function harder to follow.
+{ + virCPUx86FeaturePtr feature; + virCPUx86DataIterator iter; + virCPUx86DataItemPtr item; + virCPUx86MapPtr map; + bool inverted = false; + + if (opaque) + inverted = !*(bool *)opaque; + + if ((!(map = virCPUx86GetMap()) ||
This condition would be more readable if it were split in two.
+ !(feature = x86FeatureFind(map, name))) && + !(feature = x86FeatureFindInternal(name))) + return inverted ? true : false; + + virCPUx86DataIteratorInit(&iter, &feature->data); + while ((item = virCPUx86DataNext(&iter))) { + if (item->type == VIR_CPU_X86_DATA_MSR) + return inverted ? false : true; + } + + return inverted ? true : false; +} + + struct cpuArchDriver cpuDriverX86 = { .name = "x86", .arch = archs,
Jano
On Thu, Jun 20, 2019 at 10:16:36 +0200, Ján Tomko wrote:
On Thu, Jun 20, 2019 at 12:53:38AM +0200, Jiri Denemark wrote:
This function may be used as a virCPUDefFeatureFilter callback for virCPUDefCheckFeatures, virCPUDefFilerFeatures, and similar functions to filter or pick out features reported via MSR.
Signed-off-by: Jiri Denemark <jdenemar@redhat.com> --- src/cpu/cpu_x86.c | 40 ++++++++++++++++++++++++++++++++++++++++ src/cpu/cpu_x86.h | 3 +++ src/libvirt_private.syms | 1 + 3 files changed, 44 insertions(+)
diff --git a/src/cpu/cpu_x86.c b/src/cpu/cpu_x86.c index 6eef5cef00..a7ec0f7095 100644 --- a/src/cpu/cpu_x86.c +++ b/src/cpu/cpu_x86.c @@ -3362,6 +3362,46 @@ virCPUx86DataAddFeature(virCPUDataPtr cpuData, }
+/** + * virCPUx86FeatureIsMSR: + * @name CPU feature name + * @opaque NULL or a pointer to bool + * + * This is a callback for functions filtering features in virCPUDef. + * + * Checks whether a given CPU feature is reported via MSR. When @opaque is NULL + * or a pointer to true, the function will pick out (return true for) MSR + * features. If @opaque is a pointer to false, the logic will be inverted and + * the function will filter out (return false for) MSR features. + */
Uhm. What? Do I understand it correctly?
@opaque | *opaque | IsMSR | return --------+---------+-------+-------- NULL | SEGV | true | true 0xBEEF | true | true | true 0xBEEF | false | true | false --------+---------+-------+--------
First, it feels odd that 'false' is the value resulting in changed behavior. I'd rather see it act differently when the bool pointer is present and points to true.
I actually tried both variants and could not decide which one is better. And this means both are in fact ugly :-) So when changing virCPUx86FeatureIsMSR into a helper, I created two simple wrappers instead of playing with opaque pointer. Jirka
This functions may be used as a virCPUDefFeatureFilter callbacks for virCPUDefCheckFeatures, virCPUDefFilerFeatures, and similar functions to select (virCPUx86FeatureFilterSelectMSR) or drop (virCPUx86FeatureFilterDropMSR) features reported via MSR. Signed-off-by: Jiri Denemark <jdenemar@redhat.com> --- Notes: Version 2: - new patch Version 3: - former virCPUx86FeatureIsMSR function was split in two filters and a common helper src/cpu/cpu_x86.c | 57 ++++++++++++++++++++++++++++++++++++++++ src/cpu/cpu_x86.h | 6 +++++ src/libvirt_private.syms | 3 ++- 3 files changed, 65 insertions(+), 1 deletion(-) diff --git a/src/cpu/cpu_x86.c b/src/cpu/cpu_x86.c index 6eef5cef00..4624ebaff5 100644 --- a/src/cpu/cpu_x86.c +++ b/src/cpu/cpu_x86.c @@ -3362,6 +3362,63 @@ virCPUx86DataAddFeature(virCPUDataPtr cpuData, } +static bool +virCPUx86FeatureIsMSR(const char *name) +{ + virCPUx86FeaturePtr feature; + virCPUx86DataIterator iter; + virCPUx86DataItemPtr item; + virCPUx86MapPtr map; + + if (!(map = virCPUx86GetMap())) + return false; + + if (!(feature = x86FeatureFind(map, name)) && + !(feature = x86FeatureFindInternal(name))) + return false; + + virCPUx86DataIteratorInit(&iter, &feature->data); + while ((item = virCPUx86DataNext(&iter))) { + if (item->type == VIR_CPU_X86_DATA_MSR) + return true; + } + + return false; +} + + +/** + * virCPUx86FeatureFilterSelectMSR: + * + * This is a callback for functions filtering features in virCPUDef. The result + * will contain only MSR features. + * + * Returns true if @name is an MSR feature, false otherwise. + */ +bool +virCPUx86FeatureFilterSelectMSR(const char *name, + void *opaque ATTRIBUTE_UNUSED) +{ + return virCPUx86FeatureIsMSR(name); +} + + +/** + * virCPUx86FeatureFilterDropMSR: + * + * This is a callback for functions filtering features in virCPUDef. The result + * will not contain any MSR feature. + * + * Returns true if @name is not an MSR feature, false otherwise. + */ +bool +virCPUx86FeatureFilterDropMSR(const char *name, + void *opaque ATTRIBUTE_UNUSED) +{ + return !virCPUx86FeatureIsMSR(name); +} + + struct cpuArchDriver cpuDriverX86 = { .name = "x86", .arch = archs, diff --git a/src/cpu/cpu_x86.h b/src/cpu/cpu_x86.h index 28ae46647a..3ea38d1701 100644 --- a/src/cpu/cpu_x86.h +++ b/src/cpu/cpu_x86.h @@ -40,3 +40,9 @@ uint32_t virCPUx86DataGetSignature(virCPUDataPtr cpuData, int virCPUx86DataSetVendor(virCPUDataPtr cpuData, const char *vendor); + +bool virCPUx86FeatureFilterSelectMSR(const char *name, + void *opaque); + +bool virCPUx86FeatureFilterDropMSR(const char *name, + void *opaque); diff --git a/src/libvirt_private.syms b/src/libvirt_private.syms index 90a6d10666..c06fa67d6a 100644 --- a/src/libvirt_private.syms +++ b/src/libvirt_private.syms @@ -1268,7 +1268,8 @@ virCPUx86DataAdd; virCPUx86DataGetSignature; virCPUx86DataSetSignature; virCPUx86DataSetVendor; - +virCPUx86FeatureFilterDropMSR; +virCPUx86FeatureFilterSelectMSR; # datatypes.h virConnectClass; -- 2.22.0
On Thu, Jun 20, 2019 at 01:48:30PM +0200, Jiri Denemark wrote:
This functions may be used as a virCPUDefFeatureFilter callbacks for virCPUDefCheckFeatures, virCPUDefFilerFeatures, and similar functions to select (virCPUx86FeatureFilterSelectMSR) or drop (virCPUx86FeatureFilterDropMSR) features reported via MSR.
Signed-off-by: Jiri Denemark <jdenemar@redhat.com> ---
Notes: Version 2: - new patch
Version 3: - former virCPUx86FeatureIsMSR function was split in two filters and a common helper
src/cpu/cpu_x86.c | 57 ++++++++++++++++++++++++++++++++++++++++ src/cpu/cpu_x86.h | 6 +++++ src/libvirt_private.syms | 3 ++- 3 files changed, 65 insertions(+), 1 deletion(-)
Reviewed-by: Ján Tomko <jtomko@redhat.com> Jano
This is used by the host capabilities code to construct host CPU definition. Signed-off-by: Jiri Denemark <jdenemar@redhat.com> Reviewed-by: Ján Tomko <jtomko@redhat.com> --- src/cpu/cpu_x86.c | 22 ++++++++++++++++++++++ 1 file changed, 22 insertions(+) diff --git a/src/cpu/cpu_x86.c b/src/cpu/cpu_x86.c index a7ec0f7095..5f051950de 100644 --- a/src/cpu/cpu_x86.c +++ b/src/cpu/cpu_x86.c @@ -2773,6 +2773,28 @@ virCPUx86GetHost(virCPUDefPtr cpu, cpuidSet(CPUX86_EXTENDED, cpuData) < 0) goto cleanup; + /* Read the IA32_ARCH_CAPABILITIES MSR (0x10a) if supported. + * This is best effort since there might be no way to read the MSR + * when we are not running as root. */ + if (virCPUx86DataCheckFeature(cpuData, "arch-capabilities") == 1) { + uint64_t msr; + unsigned long index = 0x10a; + + if (virHostCPUGetMSR(index, &msr) == 0) { + virCPUx86DataItem item = { + .type = VIR_CPU_X86_DATA_MSR, + .data.msr = { + .index = index, + .eax = msr & 0xffffffff, + .edx = msr >> 32, + }, + }; + + if (virCPUx86DataAdd(cpuData, &item) < 0) + return -1; + } + } + ret = x86DecodeCPUData(cpu, cpuData, models); cpu->microcodeVersion = virHostCPUGetMicrocodeVersion(); -- 2.22.0
On Thu, Jun 20, 2019 at 12:53:39AM +0200, Jiri Denemark wrote:
This is used by the host capabilities code to construct host CPU definition.
Signed-off-by: Jiri Denemark <jdenemar@redhat.com> Reviewed-by: Ján Tomko <jtomko@redhat.com> --- src/cpu/cpu_x86.c | 22 ++++++++++++++++++++++ 1 file changed, 22 insertions(+)
Reviewed-by: Ján Tomko <jtomko@redhat.com> Jano
On 6/19/19 6:53 PM, Jiri Denemark wrote:
This is used by the host capabilities code to construct host CPU definition.
Signed-off-by: Jiri Denemark <jdenemar@redhat.com> Reviewed-by: Ján Tomko <jtomko@redhat.com> --- src/cpu/cpu_x86.c | 22 ++++++++++++++++++++++ 1 file changed, 22 insertions(+)
diff --git a/src/cpu/cpu_x86.c b/src/cpu/cpu_x86.c index a7ec0f7095..5f051950de 100644 --- a/src/cpu/cpu_x86.c +++ b/src/cpu/cpu_x86.c @@ -2773,6 +2773,28 @@ virCPUx86GetHost(virCPUDefPtr cpu, cpuidSet(CPUX86_EXTENDED, cpuData) < 0) goto cleanup;
+ /* Read the IA32_ARCH_CAPABILITIES MSR (0x10a) if supported. + * This is best effort since there might be no way to read the MSR + * when we are not running as root. */ + if (virCPUx86DataCheckFeature(cpuData, "arch-capabilities") == 1) { + uint64_t msr; + unsigned long index = 0x10a; + + if (virHostCPUGetMSR(index, &msr) == 0) { + virCPUx86DataItem item = { + .type = VIR_CPU_X86_DATA_MSR, + .data.msr = { + .index = index, + .eax = msr & 0xffffffff, + .edx = msr >> 32, + }, + }; + + if (virCPUx86DataAdd(cpuData, &item) < 0) + return -1;
Coverity notes that @cpuData will be leaked in this case. John
+ } + } + ret = x86DecodeCPUData(cpu, cpuData, models); cpu->microcodeVersion = virHostCPUGetMicrocodeVersion();
Signed-off-by: Jiri Denemark <jdenemar@redhat.com> Reviewed-by: Ján Tomko <jtomko@redhat.com> --- src/cpu_map/x86_features.xml | 20 +++++++++++++++++++ .../x86_64-cpuid-Core-i7-7600U-enabled.xml | 1 + .../x86_64-cpuid-Core-i7-7600U-json.xml | 1 + ...86_64-cpuid-Xeon-Platinum-8268-enabled.xml | 1 + .../x86_64-cpuid-Xeon-Platinum-8268-guest.xml | 4 ++++ .../x86_64-cpuid-Xeon-Platinum-8268-host.xml | 4 ++++ .../x86_64-cpuid-Xeon-Platinum-8268-json.xml | 3 +++ .../qemu_3.1.0.x86_64.xml | 1 + .../qemu_4.0.0.x86_64.xml | 1 + .../qemu_4.1.0.x86_64.xml | 1 + 10 files changed, 37 insertions(+) diff --git a/src/cpu_map/x86_features.xml b/src/cpu_map/x86_features.xml index 370807f88e..2bed1e0372 100644 --- a/src/cpu_map/x86_features.xml +++ b/src/cpu_map/x86_features.xml @@ -482,4 +482,24 @@ <feature name='amd-no-ssb'> <cpuid eax_in='0x80000008' ebx='0x04000000'/> </feature> + + <!-- IA32_ARCH_CAPABILITIES features --> + <feature name='rdctl-no'> + <msr index='0x10a' edx='0x00000000' eax='0x00000001'/> + </feature> + <feature name='ibrs-all'> + <msr index='0x10a' edx='0x00000000' eax='0x00000002'/> + </feature> + <feature name='rsba'> + <msr index='0x10a' edx='0x00000000' eax='0x00000004'/> + </feature> + <feature name='skip-l1dfl-vmentry'> + <msr index='0x10a' edx='0x00000000' eax='0x00000008'/> + </feature> + <feature name='ssb-no'> + <msr index='0x10a' edx='0x00000000' eax='0x00000010'/> + </feature> + <feature name='mds-no'> + <msr index='0x10a' edx='0x00000000' eax='0x00000020'/> + </feature> </cpus> diff --git a/tests/cputestdata/x86_64-cpuid-Core-i7-7600U-enabled.xml b/tests/cputestdata/x86_64-cpuid-Core-i7-7600U-enabled.xml index b1cdaa802a..58bc84577c 100644 --- a/tests/cputestdata/x86_64-cpuid-Core-i7-7600U-enabled.xml +++ b/tests/cputestdata/x86_64-cpuid-Core-i7-7600U-enabled.xml @@ -5,4 +5,5 @@ <cpuid eax_in='0x00000007' ecx_in='0x00' eax='0x00000000' ebx='0x009c4fbb' ecx='0x00000004' edx='0x84000000'/> <cpuid eax_in='0x0000000d' ecx_in='0x01' eax='0x0000000f' ebx='0x00000000' ecx='0x00000000' edx='0x00000000'/> <cpuid eax_in='0x80000001' ecx_in='0x00' eax='0x00000000' ebx='0x00000000' ecx='0x00000121' edx='0x2c100800'/> + <msr index='0x10a' edx='0x00000000' eax='0x00000008'/> </cpudata> diff --git a/tests/cputestdata/x86_64-cpuid-Core-i7-7600U-json.xml b/tests/cputestdata/x86_64-cpuid-Core-i7-7600U-json.xml index 48089c6003..690081493b 100644 --- a/tests/cputestdata/x86_64-cpuid-Core-i7-7600U-json.xml +++ b/tests/cputestdata/x86_64-cpuid-Core-i7-7600U-json.xml @@ -10,4 +10,5 @@ <feature policy='require' name='ssbd'/> <feature policy='require' name='xsaves'/> <feature policy='require' name='pdpe1gb'/> + <feature policy='require' name='skip-l1dfl-vmentry'/> </cpu> diff --git a/tests/cputestdata/x86_64-cpuid-Xeon-Platinum-8268-enabled.xml b/tests/cputestdata/x86_64-cpuid-Xeon-Platinum-8268-enabled.xml index 434ac1956a..313009b156 100644 --- a/tests/cputestdata/x86_64-cpuid-Xeon-Platinum-8268-enabled.xml +++ b/tests/cputestdata/x86_64-cpuid-Xeon-Platinum-8268-enabled.xml @@ -5,4 +5,5 @@ <cpuid eax_in='0x00000007' ecx_in='0x00' eax='0x00000000' ebx='0xd19f4fbb' ecx='0x0000080c' edx='0x84000000'/> <cpuid eax_in='0x0000000d' ecx_in='0x01' eax='0x0000000f' ebx='0x00000000' ecx='0x00000000' edx='0x00000000'/> <cpuid eax_in='0x80000001' ecx_in='0x00' eax='0x00000000' ebx='0x00000000' ecx='0x00000121' edx='0x2c100800'/> + <msr index='0x10a' edx='0x00000000' eax='0x0000000b'/> </cpudata> diff --git a/tests/cputestdata/x86_64-cpuid-Xeon-Platinum-8268-guest.xml b/tests/cputestdata/x86_64-cpuid-Xeon-Platinum-8268-guest.xml index c7e8a1fccf..988fb1dbdc 100644 --- a/tests/cputestdata/x86_64-cpuid-Xeon-Platinum-8268-guest.xml +++ b/tests/cputestdata/x86_64-cpuid-Xeon-Platinum-8268-guest.xml @@ -30,4 +30,8 @@ <feature policy='require' name='mbm_total'/> <feature policy='require' name='mbm_local'/> <feature policy='require' name='invtsc'/> + <feature policy='require' name='rdctl-no'/> + <feature policy='require' name='ibrs-all'/> + <feature policy='require' name='skip-l1dfl-vmentry'/> + <feature policy='require' name='mds-no'/> </cpu> diff --git a/tests/cputestdata/x86_64-cpuid-Xeon-Platinum-8268-host.xml b/tests/cputestdata/x86_64-cpuid-Xeon-Platinum-8268-host.xml index d7482751b4..fdeafc4870 100644 --- a/tests/cputestdata/x86_64-cpuid-Xeon-Platinum-8268-host.xml +++ b/tests/cputestdata/x86_64-cpuid-Xeon-Platinum-8268-host.xml @@ -31,4 +31,8 @@ <feature name='mbm_total'/> <feature name='mbm_local'/> <feature name='invtsc'/> + <feature name='rdctl-no'/> + <feature name='ibrs-all'/> + <feature name='skip-l1dfl-vmentry'/> + <feature name='mds-no'/> </cpu> diff --git a/tests/cputestdata/x86_64-cpuid-Xeon-Platinum-8268-json.xml b/tests/cputestdata/x86_64-cpuid-Xeon-Platinum-8268-json.xml index b7d12dced7..78863c61d1 100644 --- a/tests/cputestdata/x86_64-cpuid-Xeon-Platinum-8268-json.xml +++ b/tests/cputestdata/x86_64-cpuid-Xeon-Platinum-8268-json.xml @@ -7,4 +7,7 @@ <feature policy='require' name='umip'/> <feature policy='require' name='pku'/> <feature policy='require' name='xsaves'/> + <feature policy='require' name='rdctl-no'/> + <feature policy='require' name='ibrs-all'/> + <feature policy='require' name='skip-l1dfl-vmentry'/> </cpu> diff --git a/tests/domaincapsschemadata/qemu_3.1.0.x86_64.xml b/tests/domaincapsschemadata/qemu_3.1.0.x86_64.xml index ca3baab88c..dfd186afba 100644 --- a/tests/domaincapsschemadata/qemu_3.1.0.x86_64.xml +++ b/tests/domaincapsschemadata/qemu_3.1.0.x86_64.xml @@ -42,6 +42,7 @@ <feature policy='require' name='xsaves'/> <feature policy='require' name='pdpe1gb'/> <feature policy='require' name='invtsc'/> + <feature policy='require' name='skip-l1dfl-vmentry'/> </mode> <mode name='custom' supported='yes'> <model usable='yes'>qemu64</model> diff --git a/tests/domaincapsschemadata/qemu_4.0.0.x86_64.xml b/tests/domaincapsschemadata/qemu_4.0.0.x86_64.xml index cba841d844..36f6f1e94d 100644 --- a/tests/domaincapsschemadata/qemu_4.0.0.x86_64.xml +++ b/tests/domaincapsschemadata/qemu_4.0.0.x86_64.xml @@ -42,6 +42,7 @@ <feature policy='require' name='xsaves'/> <feature policy='require' name='pdpe1gb'/> <feature policy='require' name='invtsc'/> + <feature policy='require' name='skip-l1dfl-vmentry'/> </mode> <mode name='custom' supported='yes'> <model usable='yes'>qemu64</model> diff --git a/tests/domaincapsschemadata/qemu_4.1.0.x86_64.xml b/tests/domaincapsschemadata/qemu_4.1.0.x86_64.xml index 389e641bbb..d3fc1ce9e7 100644 --- a/tests/domaincapsschemadata/qemu_4.1.0.x86_64.xml +++ b/tests/domaincapsschemadata/qemu_4.1.0.x86_64.xml @@ -41,6 +41,7 @@ <feature policy='require' name='arch-capabilities'/> <feature policy='require' name='xsaves'/> <feature policy='require' name='pdpe1gb'/> + <feature policy='require' name='skip-l1dfl-vmentry'/> </mode> <mode name='custom' supported='yes'> <model usable='yes'>qemu64</model> -- 2.22.0
Without "unavailable-features" CPU property we cannot properly detect whether a specific MSR feature we asked for (either explicitly or implicitly via a CPU model) was disabled by QEMU for some reason. Because this could break migration, snapshots, and save/restore operaions, it's better to just forbid any use of MSR features with QEMU which lacks "unavailable-features" CPU property. Signed-off-by: Jiri Denemark <jdenemar@redhat.com> --- src/qemu/qemu_process.c | 29 ++++++++++++++++++++++++++--- 1 file changed, 26 insertions(+), 3 deletions(-) diff --git a/src/qemu/qemu_process.c b/src/qemu/qemu_process.c index fa82adbc1e..45997dad49 100644 --- a/src/qemu/qemu_process.c +++ b/src/qemu/qemu_process.c @@ -59,6 +59,7 @@ #include "qemu_firmware.h" #include "cpu/cpu.h" +#include "cpu/cpu_x86.h" #include "datatypes.h" #include "virlog.h" #include "virerror.h" @@ -5407,9 +5408,31 @@ qemuProcessStartValidate(virQEMUDriverPtr driver, if (qemuProcessStartValidateShmem(vm) < 0) return -1; - if (vm->def->cpu && - virCPUValidateFeatures(vm->def->os.arch, vm->def->cpu) < 0) - return -1; + if (vm->def->cpu) { + if (virCPUValidateFeatures(vm->def->os.arch, vm->def->cpu) < 0) + return -1; + + if (ARCH_IS_X86(vm->def->os.arch) && + !virQEMUCapsGet(qemuCaps, QEMU_CAPS_CPU_UNAVAILABLE_FEATURES)) { + VIR_AUTOSTRINGLIST features = NULL; + int n; + + if ((n = virCPUDefCheckFeatures(vm->def->cpu, + virCPUx86FeatureIsMSR, NULL, + &features)) < 0) + return -1; + + if (n > 0) { + VIR_AUTOFREE(char *) str = NULL; + + str = virStringListJoin((const char **)features, ", "); + virReportError(VIR_ERR_CONFIG_UNSUPPORTED, + _("Some features cannot be reliably used " + "with this QEMU: %s"), str); + return -1; + } + } + } if (qemuProcessStartValidateDisks(vm, qemuCaps) < 0) return -1; -- 2.22.0
On Thu, Jun 20, 2019 at 12:53:41AM +0200, Jiri Denemark wrote:
Without "unavailable-features" CPU property we cannot properly detect whether a specific MSR feature we asked for (either explicitly or implicitly via a CPU model) was disabled by QEMU for some reason. Because this could break migration, snapshots, and save/restore operaions, it's better to just forbid any use of MSR features with QEMU which lacks "unavailable-features" CPU property.
Signed-off-by: Jiri Denemark <jdenemar@redhat.com> --- src/qemu/qemu_process.c | 29 ++++++++++++++++++++++++++--- 1 file changed, 26 insertions(+), 3 deletions(-)
Reviewed-by: Ján Tomko <jtomko@redhat.com> Jano
With QEMU versions which lack "unavailable-features" we use CPUID based detection of features which were enabled or disabled once QEMU starts. Thus using MSR features with host-model would result in all of them being marked as disabled in the active domain definition even though QEMU did not actually disable them. Let's make sure we add MSR features to host-model only when "unavailable-features" property is supported by QEMU. Signed-off-by: Jiri Denemark <jdenemar@redhat.com> --- src/qemu/qemu_capabilities.c | 16 ++++++++++++++++ tests/domaincapsschemadata/qemu_3.1.0.x86_64.xml | 1 - tests/domaincapsschemadata/qemu_4.0.0.x86_64.xml | 1 - 3 files changed, 16 insertions(+), 2 deletions(-) diff --git a/src/qemu/qemu_capabilities.c b/src/qemu/qemu_capabilities.c index 4134f319ac..b7c20f3e3e 100644 --- a/src/qemu/qemu_capabilities.c +++ b/src/qemu/qemu_capabilities.c @@ -3193,6 +3193,22 @@ virQEMUCapsInitHostCPUModel(virQEMUCapsPtr qemuCaps, goto error; } + if (ARCH_IS_X86(qemuCaps->arch) && + !virQEMUCapsGet(qemuCaps, QEMU_CAPS_CPU_UNAVAILABLE_FEATURES)) { + bool selecting = false; + if (cpu && + virCPUDefFilterFeatures(cpu, virCPUx86FeatureIsMSR, &selecting) < 0) + goto error; + + if (migCPU && + virCPUDefFilterFeatures(migCPU, virCPUx86FeatureIsMSR, &selecting) < 0) + goto error; + + if (fullCPU && + virCPUDefFilterFeatures(fullCPU, virCPUx86FeatureIsMSR, &selecting) < 0) + goto error; + } + virQEMUCapsSetHostModel(qemuCaps, type, cpu, migCPU, fullCPU); cleanup: diff --git a/tests/domaincapsschemadata/qemu_3.1.0.x86_64.xml b/tests/domaincapsschemadata/qemu_3.1.0.x86_64.xml index dfd186afba..ca3baab88c 100644 --- a/tests/domaincapsschemadata/qemu_3.1.0.x86_64.xml +++ b/tests/domaincapsschemadata/qemu_3.1.0.x86_64.xml @@ -42,7 +42,6 @@ <feature policy='require' name='xsaves'/> <feature policy='require' name='pdpe1gb'/> <feature policy='require' name='invtsc'/> - <feature policy='require' name='skip-l1dfl-vmentry'/> </mode> <mode name='custom' supported='yes'> <model usable='yes'>qemu64</model> diff --git a/tests/domaincapsschemadata/qemu_4.0.0.x86_64.xml b/tests/domaincapsschemadata/qemu_4.0.0.x86_64.xml index 36f6f1e94d..cba841d844 100644 --- a/tests/domaincapsschemadata/qemu_4.0.0.x86_64.xml +++ b/tests/domaincapsschemadata/qemu_4.0.0.x86_64.xml @@ -42,7 +42,6 @@ <feature policy='require' name='xsaves'/> <feature policy='require' name='pdpe1gb'/> <feature policy='require' name='invtsc'/> - <feature policy='require' name='skip-l1dfl-vmentry'/> </mode> <mode name='custom' supported='yes'> <model usable='yes'>qemu64</model> -- 2.22.0
On Thu, Jun 20, 2019 at 12:53:42AM +0200, Jiri Denemark wrote:
With QEMU versions which lack "unavailable-features" we use CPUID based detection of features which were enabled or disabled once QEMU starts. Thus using MSR features with host-model would result in all of them being marked as disabled in the active domain definition even though QEMU did not actually disable them.
Let's make sure we add MSR features to host-model only when "unavailable-features" property is supported by QEMU.
Signed-off-by: Jiri Denemark <jdenemar@redhat.com> --- src/qemu/qemu_capabilities.c | 16 ++++++++++++++++ tests/domaincapsschemadata/qemu_3.1.0.x86_64.xml | 1 - tests/domaincapsschemadata/qemu_4.0.0.x86_64.xml | 1 - 3 files changed, 16 insertions(+), 2 deletions(-)
diff --git a/src/qemu/qemu_capabilities.c b/src/qemu/qemu_capabilities.c index 4134f319ac..b7c20f3e3e 100644 --- a/src/qemu/qemu_capabilities.c +++ b/src/qemu/qemu_capabilities.c @@ -3193,6 +3193,22 @@ virQEMUCapsInitHostCPUModel(virQEMUCapsPtr qemuCaps, goto error; }
+ if (ARCH_IS_X86(qemuCaps->arch) && + !virQEMUCapsGet(qemuCaps, QEMU_CAPS_CPU_UNAVAILABLE_FEATURES)) { + bool selecting = false;
void *invert = (void *)(intptr_t)0x1; or (if you don't like the pointer fun) bool invert = true;
+ if (cpu && + virCPUDefFilterFeatures(cpu, virCPUx86FeatureIsMSR, &selecting) < 0) + goto error; + + if (migCPU && + virCPUDefFilterFeatures(migCPU, virCPUx86FeatureIsMSR, &selecting) < 0) + goto error; + + if (fullCPU && + virCPUDefFilterFeatures(fullCPU, virCPUx86FeatureIsMSR, &selecting) < 0) + goto error; + } + virQEMUCapsSetHostModel(qemuCaps, type, cpu, migCPU, fullCPU);
Reviewed-by: Ján Tomko <jtomko@redhat.com> Jano
participants (3)
-
Jiri Denemark -
John Ferlan -
Ján Tomko