[libvirt] [PATCH v2] host-validate: Improve CPU flags processing
Instead of relying on substring search, tokenize the input and process each CPU flag separately. This ensures CPU flag detection will continue to work correctly even if we start looking for CPU flags whose name might appear as part of other CPU flags' names. The result of processing is stored in a virBitmap, which means we don't have to parse /proc/cpuinfo in its entirety for each single CPU flag we want to check. Moreover, use of the newly-introduced virHostValidateCPUFlag enumeration ensures we don't go looking for random CPU flags which might actually be simple typos. --- Changes in v2: * use virStringSplitCount() and STRPREFIX() instead of strtok_r() and strcmp(), as suggested by Peter tools/virt-host-validate-common.c | 67 ++++++++++++++++++++++++++++++++------- tools/virt-host-validate-common.h | 13 +++++++- tools/virt-host-validate-qemu.c | 12 +++++-- 3 files changed, 77 insertions(+), 15 deletions(-) diff --git a/tools/virt-host-validate-common.c b/tools/virt-host-validate-common.c index 8ebf73e..3305a32 100644 --- a/tools/virt-host-validate-common.c +++ b/tools/virt-host-validate-common.c @@ -31,7 +31,6 @@ #endif /* HAVE_MNTENT_H */ #include <sys/stat.h> -#include "virutil.h" #include "viralloc.h" #include "virfile.h" #include "virt-host-validate-common.h" @@ -39,6 +38,10 @@ #define VIR_FROM_THIS VIR_FROM_NONE +VIR_ENUM_IMPL(virHostValidateCPUFlag, VIR_HOST_VALIDATE_CPU_FLAG_LAST, + "vmx", + "svm"); + static bool quiet; void virHostMsgSetQuiet(bool quietFlag) @@ -188,29 +191,64 @@ int virHostValidateNamespace(const char *hvname, } -bool virHostValidateHasCPUFlag(const char *name) +virBitmapPtr virHostValidateGetCPUFlags(void) { - FILE *fp = fopen("/proc/cpuinfo", "r"); - bool ret = false; + FILE *fp; + virBitmapPtr flags; - if (!fp) - return false; + if (!(fp = fopen("/proc/cpuinfo", "r"))) + return NULL; + + if (!(flags = virBitmapNewQuiet(VIR_HOST_VALIDATE_CPU_FLAG_LAST))) + return NULL; do { char line[1024]; + char *start; + char **tokens; + size_t ntokens; + size_t i; if (!fgets(line, sizeof(line), fp)) break; - if (strstr(line, name)) { - ret = true; - break; + /* The line we're interested in is marked either as "flags" or + * as "Features" depending on the architecture, so check both + * prefixes */ + if (!STRPREFIX(line, "flags") && !STRPREFIX(line, "Features")) + continue; + + /* fgets() includes the trailing newline in the output buffer, + * so we need to clean that up ourselves. We can safely access + * line[strlen(line) - 1] because the checks above would cause + * us to skip empty strings */ + line[strlen(line) - 1] = '\0'; + + /* Skip to the separator */ + if (!(start = strstr(line, ":"))) + continue; + + /* Split the line using " " as a delimiter. The first token + * will always be ":", but that's okay */ + if (!(tokens = virStringSplitCount(start, " ", 0, &ntokens))) + continue; + + /* Go through all flags and check whether one of those we + * might want to check for later on is present; if that's + * the case, set the relevant bit in the bitmap */ + for (i = 0; i < ntokens; i++) { + int value; + + if ((value = virHostValidateCPUFlagTypeFromString(tokens[i])) >= 0) + ignore_value(virBitmapSetBit(flags, value)); } + + virStringFreeListCount(tokens, ntokens); } while (1); VIR_FORCE_FCLOSE(fp); - return ret; + return flags; } @@ -359,15 +397,20 @@ int virHostValidateCGroupController(const char *hvname, int virHostValidateIOMMU(const char *hvname, virHostValidateLevel level) { + virBitmapPtr flags; struct stat sb; const char *bootarg = NULL; bool isAMD = false, isIntel = false; - if (virHostValidateHasCPUFlag("vmx")) + flags = virHostValidateGetCPUFlags(); + + if (flags && virBitmapIsBitSet(flags, VIR_HOST_VALIDATE_CPU_FLAG_VMX)) isIntel = true; - else if (virHostValidateHasCPUFlag("svm")) + else if (flags && virBitmapIsBitSet(flags, VIR_HOST_VALIDATE_CPU_FLAG_SVM)) isAMD = true; + virBitmapFree(flags); + virHostMsgCheck(hvname, "%s", _("for device assignment IOMMU support")); if (isIntel) { diff --git a/tools/virt-host-validate-common.h b/tools/virt-host-validate-common.h index d4c4759..26e006b 100644 --- a/tools/virt-host-validate-common.h +++ b/tools/virt-host-validate-common.h @@ -23,6 +23,8 @@ # define __VIRT_HOST_VALIDATE_COMMON_H__ # include "internal.h" +# include "virutil.h" +# include "virbitmap.h" typedef enum { VIR_HOST_VALIDATE_FAIL, @@ -32,6 +34,15 @@ typedef enum { VIR_HOST_VALIDATE_LAST, } virHostValidateLevel; +typedef enum { + VIR_HOST_VALIDATE_CPU_FLAG_VMX = 0, + VIR_HOST_VALIDATE_CPU_FLAG_SVM, + + VIR_HOST_VALIDATE_CPU_FLAG_LAST, +} virHostValidateCPUFlag; + +VIR_ENUM_DECL(virHostValidateCPUFlag); + extern void virHostMsgSetQuiet(bool quietFlag); extern void virHostMsgCheck(const char *prefix, @@ -53,7 +64,7 @@ extern int virHostValidateDeviceAccessible(const char *hvname, virHostValidateLevel level, const char *hint); -extern bool virHostValidateHasCPUFlag(const char *name); +extern virBitmapPtr virHostValidateGetCPUFlags(void); extern int virHostValidateLinuxKernel(const char *hvname, int version, diff --git a/tools/virt-host-validate-qemu.c b/tools/virt-host-validate-qemu.c index a9f6c1e..b96b020 100644 --- a/tools/virt-host-validate-qemu.c +++ b/tools/virt-host-validate-qemu.c @@ -24,14 +24,20 @@ #include "virt-host-validate-qemu.h" #include "virt-host-validate-common.h" +#include "virbitmap.h" int virHostValidateQEMU(void) { + virBitmapPtr flags; int ret = 0; virHostMsgCheck("QEMU", "%s", ("for hardware virtualization")); - if (virHostValidateHasCPUFlag("svm") || - virHostValidateHasCPUFlag("vmx")) { + + flags = virHostValidateGetCPUFlags(); + + if (flags && + (virBitmapIsBitSet(flags, VIR_HOST_VALIDATE_CPU_FLAG_SVM) || + virBitmapIsBitSet(flags, VIR_HOST_VALIDATE_CPU_FLAG_VMX))) { virHostMsgPass(); if (virHostValidateDeviceExists("QEMU", "/dev/kvm", VIR_HOST_VALIDATE_FAIL, @@ -48,6 +54,8 @@ int virHostValidateQEMU(void) _("Only emulated CPUs are available, performance will be significantly limited")); } + virBitmapFree(flags); + if (virHostValidateDeviceExists("QEMU", "/dev/vhost-net", VIR_HOST_VALIDATE_WARN, _("Load the 'vhost_net' module to improve performance " -- 2.5.5
On Thu, Mar 31, 2016 at 05:26:10PM +0200, Andrea Bolognani wrote:
Instead of relying on substring search, tokenize the input and process each CPU flag separately. This ensures CPU flag detection will continue to work correctly even if we start looking for CPU flags whose name might appear as part of other CPU flags' names.
The result of processing is stored in a virBitmap, which means we don't have to parse /proc/cpuinfo in its entirety for each single CPU flag we want to check.
Moreover, use of the newly-introduced virHostValidateCPUFlag enumeration ensures we don't go looking for random CPU flags which might actually be simple typos.
You could still put the typo in the enum impelmentation :)
--- Changes in v2:
* use virStringSplitCount() and STRPREFIX() instead of strtok_r() and strcmp(), as suggested by Peter
tools/virt-host-validate-common.c | 67 ++++++++++++++++++++++++++++++++------- tools/virt-host-validate-common.h | 13 +++++++- tools/virt-host-validate-qemu.c | 12 +++++-- 3 files changed, 77 insertions(+), 15 deletions(-)
ACK
+ /* Split the line using " " as a delimiter. The first token + * will always be ":", but that's okay */ + if (!(tokens = virStringSplitCount(start, " ", 0, &ntokens))) + continue; + + /* Go through all flags and check whether one of those we + * might want to check for later on is present; if that's + * the case, set the relevant bit in the bitmap */ + for (i = 0; i < ntokens; i++) { + int value; + + if ((value = virHostValidateCPUFlagTypeFromString(tokens[i])) >= 0) + ignore_value(virBitmapSetBit(flags, value)); } + + virStringFreeListCount(tokens, ntokens); } while (1);
We have already found the first 'flags' or 'Features' and parsed all the features. I doubt different processors on the system would have different features so I'd just use while (0) here. Jan
On Tue, 2016-04-05 at 12:35 +0200, Ján Tomko wrote:
+ /* Split the line using " " as a delimiter. The first token + * will always be ":", but that's okay */ + if (!(tokens = virStringSplitCount(start, " ", 0, &ntokens))) + continue; + + /* Go through all flags and check whether one of those we + * might want to check for later on is present; if that's + * the case, set the relevant bit in the bitmap */ + for (i = 0; i < ntokens; i++) { + int value; + + if ((value = virHostValidateCPUFlagTypeFromString(tokens[i])) >= 0) + ignore_value(virBitmapSetBit(flags, value)); } + + virStringFreeListCount(tokens, ntokens); } while (1); We have already found the first 'flags' or 'Features' and parsed all the features. I doubt different processors on the system would have different features so I'd just use while (0) here.
Yeah, if different CPUs had different features we'd probably be in for a word of pain anyway. Turns out that gcc is smart enough to figure out that a 'do {} while (0);' loop is going to be executed only once, and optimizes it accordingly. Side effect: the 'continue' instructions behave the same as the 'break' instructions, and everything stops working :( I've pushed the patch as-is. Cheers. -- Andrea Bolognani Software Engineer - Virtualization Team
Andrea Bolognani <abologna@redhat.com> [2016-03-31, 05:29PM +0200]:
[...] + if (!STRPREFIX(line, "flags") && !STRPREFIX(line, "Features"))
At least on s390x, the flags line begins with a lower-cased "features". I can post a follow-up patch to include the platform specific stuff for s390x.
[...]
participants (3)
-
Andrea Bolognani -
Bjoern Walk -
Ján Tomko