[libvirt] [PATCH 0/4] Add more host validation checks
This extends the virt-host-validate command todo a bunch more checks. Probably the most interesting is the last one which checks for Intel IOMMU needed todo PCI assignment with KVM. It is pretty crude, so further suggestions welcome. I don't want to grep dmesg as suggested in [1] since dmesg can be purged by the time we look at it. What kmods should we look for for VT-D too ? [1] http://www.linux-kvm.org/page/How_to_assign_devices_with_VT-d_in_KVM Daniel P. Berrange (4): virt-host-validate.c: check for kernel namespaces virt-host-validate: distinguish exists vs accessible for devices virt-host-validate: check for required cgroups virt-host-validate: check for intel_iommu=on cmdline flag tools/virt-host-validate-common.c | 209 +++++++++++++++++++++++++++++++++++--- tools/virt-host-validate-common.h | 31 +++++- tools/virt-host-validate-lxc.c | 60 +++++++++++ tools/virt-host-validate-qemu.c | 66 ++++++++++-- 4 files changed, 338 insertions(+), 28 deletions(-) -- 2.4.3
The LXC driver requires the uts, mnt, pid & ipc namespaces, while net & user namespaces are optional. Validate all these are present. Signed-off-by: Daniel P. Berrange <berrange@redhat.com> --- tools/virt-host-validate-common.c | 20 ++++++++++++++++++++ tools/virt-host-validate-common.h | 5 +++++ tools/virt-host-validate-lxc.c | 30 ++++++++++++++++++++++++++++++ 3 files changed, 55 insertions(+) diff --git a/tools/virt-host-validate-common.c b/tools/virt-host-validate-common.c index 92a19c5..12a98f4 100644 --- a/tools/virt-host-validate-common.c +++ b/tools/virt-host-validate-common.c @@ -132,6 +132,26 @@ int virHostValidateDevice(const char *hvname, } +int virHostValidateNamespace(const char *hvname, + const char *ns_name, + virHostValidateLevel level, + const char *hint) +{ + virHostMsgCheck(hvname, "for namespace %s", ns_name); + char nspath[100]; + + snprintf(nspath, sizeof(nspath), "/proc/self/ns/%s", ns_name); + + if (access(nspath, F_OK) < 0) { + virHostMsgFail(level, hint); + return -1; + } + + virHostMsgPass(); + return 0; +} + + bool virHostValidateHasCPUFlag(const char *name) { FILE *fp = fopen("/proc/cpuinfo", "r"); diff --git a/tools/virt-host-validate-common.h b/tools/virt-host-validate-common.h index 25644dc..9d8bcea 100644 --- a/tools/virt-host-validate-common.h +++ b/tools/virt-host-validate-common.h @@ -54,4 +54,9 @@ extern int virHostValidateLinuxKernel(const char *hvname, virHostValidateLevel level, const char *hint); +extern int virHostValidateNamespace(const char *hvname, + const char *ns_name, + virHostValidateLevel level, + const char *hint); + #endif /* __VIRT_HOST_VALIDATE_COMMON_H__ */ diff --git a/tools/virt-host-validate-lxc.c b/tools/virt-host-validate-lxc.c index e0d2df4..43c3f5f 100644 --- a/tools/virt-host-validate-lxc.c +++ b/tools/virt-host-validate-lxc.c @@ -33,5 +33,35 @@ int virHostValidateLXC(void) _("Upgrade to a kernel supporting namespaces")) < 0) ret = -1; + if (virHostValidateNamespace("LXC", "ipc", + VIR_HOST_VALIDATE_FAIL, + _("IPC namespace support is required")) < 0) + ret = -1; + + if (virHostValidateNamespace("LXC", "mnt", + VIR_HOST_VALIDATE_FAIL, + _("Mount namespace support is required")) < 0) + ret = -1; + + if (virHostValidateNamespace("LXC", "pid", + VIR_HOST_VALIDATE_FAIL, + _("PID namespace support is required")) < 0) + ret = -1; + + if (virHostValidateNamespace("LXC", "uts", + VIR_HOST_VALIDATE_FAIL, + _("UTS namespace support is required")) < 0) + ret = -1; + + if (virHostValidateNamespace("LXC", "net", + VIR_HOST_VALIDATE_WARN, + _("Network namespace support is recommended")) < 0) + ret = -1; + + if (virHostValidateNamespace("LXC", "user", + VIR_HOST_VALIDATE_FAIL, + _("User namespace support is recommended")) < 0) + ret = -1; + return ret; } -- 2.4.3
Currently we just check that various devices are accessible. This leads to inaccurate errors reported for /dev/kvm and /dev/vhost-net if they exist but an unprivileged user lacks access. Switch existing checks to look for file existance, and add a separate check for accessibility of /dev/kvm since some distros don't grant users access by default. Signed-off-by: Daniel P. Berrange <berrange@redhat.com> --- tools/virt-host-validate-common.c | 27 ++++++++++++++++++++++----- tools/virt-host-validate-common.h | 13 +++++++++---- tools/virt-host-validate-qemu.c | 28 ++++++++++++++++------------ 3 files changed, 47 insertions(+), 21 deletions(-) diff --git a/tools/virt-host-validate-common.c b/tools/virt-host-validate-common.c index 12a98f4..d646a79 100644 --- a/tools/virt-host-validate-common.c +++ b/tools/virt-host-validate-common.c @@ -115,12 +115,29 @@ void virHostMsgFail(virHostValidateLevel level, } -int virHostValidateDevice(const char *hvname, - const char *dev_name, - virHostValidateLevel level, - const char *hint) +int virHostValidateDeviceExists(const char *hvname, + const char *dev_name, + virHostValidateLevel level, + const char *hint) { - virHostMsgCheck(hvname, "for device %s", dev_name); + virHostMsgCheck(hvname, "if device %s exists", dev_name); + + if (access(dev_name, F_OK) < 0) { + virHostMsgFail(level, hint); + return -1; + } + + virHostMsgPass(); + return 0; +} + + +int virHostValidateDeviceAccessible(const char *hvname, + const char *dev_name, + virHostValidateLevel level, + const char *hint) +{ + virHostMsgCheck(hvname, "if device %s is accessible", dev_name); if (access(dev_name, R_OK|W_OK) < 0) { virHostMsgFail(level, hint); diff --git a/tools/virt-host-validate-common.h b/tools/virt-host-validate-common.h index 9d8bcea..c25e69c 100644 --- a/tools/virt-host-validate-common.h +++ b/tools/virt-host-validate-common.h @@ -42,10 +42,15 @@ extern void virHostMsgPass(void); extern void virHostMsgFail(virHostValidateLevel level, const char *hint); -extern int virHostValidateDevice(const char *hvname, - const char *dev_name, - virHostValidateLevel level, - const char *hint); +extern int virHostValidateDeviceExists(const char *hvname, + const char *dev_name, + virHostValidateLevel level, + const char *hint); + +extern int virHostValidateDeviceAccessible(const char *hvname, + const char *dev_name, + virHostValidateLevel level, + const char *hint); extern bool virHostValidateHasCPUFlag(const char *name); diff --git a/tools/virt-host-validate-qemu.c b/tools/virt-host-validate-qemu.c index 33a5738..9486064 100644 --- a/tools/virt-host-validate-qemu.c +++ b/tools/virt-host-validate-qemu.c @@ -20,7 +20,6 @@ */ #include <config.h> - #include "virt-host-validate-qemu.h" #include "virt-host-validate-common.h" @@ -32,25 +31,30 @@ int virHostValidateQEMU(void) if (virHostValidateHasCPUFlag("svm") || virHostValidateHasCPUFlag("vmx")) { virHostMsgPass(); - if (virHostValidateDevice("QEMU", "/dev/kvm", - VIR_HOST_VALIDATE_FAIL, - _("Check that the 'kvm-intel' or 'kvm-amd' modules are " - "loaded & the BIOS has enabled virtualization")) < 0) + if (virHostValidateDeviceExists("QEMU", "/dev/kvm", + VIR_HOST_VALIDATE_FAIL, + _("Check that the 'kvm-intel' or 'kvm-amd' modules are " + "loaded & the BIOS has enabled virtualization")) < 0) + ret = -1; + else if (virHostValidateDeviceAccessible("QEMU", "/dev/kvm", + VIR_HOST_VALIDATE_FAIL, + _("Check /dev/kvm is world writable or you are in " + "a group that is allowed to access it")) < 0) ret = -1; } else { virHostMsgFail(VIR_HOST_VALIDATE_WARN, _("Only emulated CPUs are available, performance will be significantly limited")); } - if (virHostValidateDevice("QEMU", "/dev/vhost-net", - VIR_HOST_VALIDATE_WARN, - _("Load the 'vhost_net' module to improve performance " - "of virtio networking")) < 0) + if (virHostValidateDeviceExists("QEMU", "/dev/vhost-net", + VIR_HOST_VALIDATE_WARN, + _("Load the 'vhost_net' module to improve performance " + "of virtio networking")) < 0) ret = -1; - if (virHostValidateDevice("QEMU", "/dev/net/tun", - VIR_HOST_VALIDATE_FAIL, - _("Load the 'tun' module to enable networking for QEMU guests")) < 0) + if (virHostValidateDeviceExists("QEMU", "/dev/net/tun", + VIR_HOST_VALIDATE_FAIL, + _("Load the 'tun' module to enable networking for QEMU guests")) < 0) ret = -1; return ret; -- 2.4.3
On 10/07/2015 12:50 PM, Daniel P. Berrange wrote:
Currently we just check that various devices are accessible. This leads to inaccurate errors reported for /dev/kvm and /dev/vhost-net if they exist but an unprivileged user lacks access. Switch existing checks to look for file existance, and add a separate check for accessibility of /dev/kvm since some distros don't grant users access by default.
One problem with this is that the people with those distros probably won't be running virt-host-validate under the same uid as used by libvirt when running qemu, so the results won't necessarily tell you what you need - if you run it as root it will say that /dev/kvm is accessible, even though it may not be for the case of the "qemu user", and if you run it as some unprivileged user, if may say that /dev/kvm *isn't* accessible, even though it is in the case of the qemu user. (This came to mind because someone in #virt recently was trying to figure out why kvm wasn't working for them, showed that they had /dev/kvm with mode 700, and someone else pointed out that this is how it's shipped for [some other distro, I forget which].) ACK other than that though.
Signed-off-by: Daniel P. Berrange <berrange@redhat.com> --- tools/virt-host-validate-common.c | 27 ++++++++++++++++++++++----- tools/virt-host-validate-common.h | 13 +++++++++---- tools/virt-host-validate-qemu.c | 28 ++++++++++++++++------------ 3 files changed, 47 insertions(+), 21 deletions(-)
diff --git a/tools/virt-host-validate-common.c b/tools/virt-host-validate-common.c index 12a98f4..d646a79 100644 --- a/tools/virt-host-validate-common.c +++ b/tools/virt-host-validate-common.c @@ -115,12 +115,29 @@ void virHostMsgFail(virHostValidateLevel level, }
-int virHostValidateDevice(const char *hvname, - const char *dev_name, - virHostValidateLevel level, - const char *hint) +int virHostValidateDeviceExists(const char *hvname, + const char *dev_name, + virHostValidateLevel level, + const char *hint) { - virHostMsgCheck(hvname, "for device %s", dev_name); + virHostMsgCheck(hvname, "if device %s exists", dev_name); + + if (access(dev_name, F_OK) < 0) { + virHostMsgFail(level, hint); + return -1; + } + + virHostMsgPass(); + return 0; +} + + +int virHostValidateDeviceAccessible(const char *hvname, + const char *dev_name, + virHostValidateLevel level, + const char *hint) +{ + virHostMsgCheck(hvname, "if device %s is accessible", dev_name);
if (access(dev_name, R_OK|W_OK) < 0) { virHostMsgFail(level, hint); diff --git a/tools/virt-host-validate-common.h b/tools/virt-host-validate-common.h index 9d8bcea..c25e69c 100644 --- a/tools/virt-host-validate-common.h +++ b/tools/virt-host-validate-common.h @@ -42,10 +42,15 @@ extern void virHostMsgPass(void); extern void virHostMsgFail(virHostValidateLevel level, const char *hint);
-extern int virHostValidateDevice(const char *hvname, - const char *dev_name, - virHostValidateLevel level, - const char *hint); +extern int virHostValidateDeviceExists(const char *hvname, + const char *dev_name, + virHostValidateLevel level, + const char *hint); + +extern int virHostValidateDeviceAccessible(const char *hvname, + const char *dev_name, + virHostValidateLevel level, + const char *hint);
extern bool virHostValidateHasCPUFlag(const char *name);
diff --git a/tools/virt-host-validate-qemu.c b/tools/virt-host-validate-qemu.c index 33a5738..9486064 100644 --- a/tools/virt-host-validate-qemu.c +++ b/tools/virt-host-validate-qemu.c @@ -20,7 +20,6 @@ */
#include <config.h> - #include "virt-host-validate-qemu.h" #include "virt-host-validate-common.h"
@@ -32,25 +31,30 @@ int virHostValidateQEMU(void) if (virHostValidateHasCPUFlag("svm") || virHostValidateHasCPUFlag("vmx")) { virHostMsgPass(); - if (virHostValidateDevice("QEMU", "/dev/kvm", - VIR_HOST_VALIDATE_FAIL, - _("Check that the 'kvm-intel' or 'kvm-amd' modules are " - "loaded & the BIOS has enabled virtualization")) < 0) + if (virHostValidateDeviceExists("QEMU", "/dev/kvm", + VIR_HOST_VALIDATE_FAIL, + _("Check that the 'kvm-intel' or 'kvm-amd' modules are " + "loaded & the BIOS has enabled virtualization")) < 0) + ret = -1; + else if (virHostValidateDeviceAccessible("QEMU", "/dev/kvm", + VIR_HOST_VALIDATE_FAIL, + _("Check /dev/kvm is world writable or you are in " + "a group that is allowed to access it")) < 0) ret = -1; } else { virHostMsgFail(VIR_HOST_VALIDATE_WARN, _("Only emulated CPUs are available, performance will be significantly limited")); }
- if (virHostValidateDevice("QEMU", "/dev/vhost-net", - VIR_HOST_VALIDATE_WARN, - _("Load the 'vhost_net' module to improve performance " - "of virtio networking")) < 0) + if (virHostValidateDeviceExists("QEMU", "/dev/vhost-net", + VIR_HOST_VALIDATE_WARN, + _("Load the 'vhost_net' module to improve performance " + "of virtio networking")) < 0) ret = -1;
- if (virHostValidateDevice("QEMU", "/dev/net/tun", - VIR_HOST_VALIDATE_FAIL, - _("Load the 'tun' module to enable networking for QEMU guests")) < 0) + if (virHostValidateDeviceExists("QEMU", "/dev/net/tun", + VIR_HOST_VALIDATE_FAIL, + _("Load the 'tun' module to enable networking for QEMU guests")) < 0) ret = -1;
return ret;
On Thu, Oct 08, 2015 at 05:06:29PM -0400, Laine Stump wrote:
On 10/07/2015 12:50 PM, Daniel P. Berrange wrote:
Currently we just check that various devices are accessible. This leads to inaccurate errors reported for /dev/kvm and /dev/vhost-net if they exist but an unprivileged user lacks access. Switch existing checks to look for file existance, and add a separate check for accessibility of /dev/kvm since some distros don't grant users access by default.
One problem with this is that the people with those distros probably won't be running virt-host-validate under the same uid as used by libvirt when running qemu, so the results won't necessarily tell you what you need - if you run it as root it will say that /dev/kvm is accessible, even though it may not be for the case of the "qemu user", and if you run it as some unprivileged user, if may say that /dev/kvm *isn't* accessible, even though it is in the case of the qemu user.
Yep, this is not an exact science. Generally I was working on the assumption that virt-host-validate be run as the same user as libvirtd itself runs as. eg if you run it unprivileged, then its results should refect what an instance of qemu:///session is able todo. If you run it as root, then it should check what qemu:///system can do. Of course in some cases that would not work as we'd be checking for root, rather than qemu. We could parse the qemu.conf to find the user we should use as root, but I figure that can be a later enhancement. Regards, Daniel -- |: http://berrange.com -o- http://www.flickr.com/photos/dberrange/ :| |: http://libvirt.org -o- http://virt-manager.org :| |: http://autobuild.org -o- http://search.cpan.org/~danberr/ :| |: http://entangle-photo.org -o- http://live.gnome.org/gtk-vnc :|
Extend the virt-host-validate checks to see if the required cgroups are compiled into the kernel and that they are mounted on the system. The cgroups are all optional except for 3 that LXC mandates Signed-off-by: Daniel P. Berrange <berrange@redhat.com> --- tools/virt-host-validate-common.c | 130 +++++++++++++++++++++++++++++++++++--- tools/virt-host-validate-common.h | 8 ++- tools/virt-host-validate-lxc.c | 30 +++++++++ tools/virt-host-validate-qemu.c | 30 +++++++++ 4 files changed, 188 insertions(+), 10 deletions(-) diff --git a/tools/virt-host-validate-common.c b/tools/virt-host-validate-common.c index d646a79..7e8d619 100644 --- a/tools/virt-host-validate-common.c +++ b/tools/virt-host-validate-common.c @@ -26,6 +26,7 @@ #include <stdio.h> #include <unistd.h> #include <sys/utsname.h> +#include <mntent.h> #include "virutil.h" #include "viralloc.h" @@ -104,14 +105,29 @@ static const char *failEscapeCodes[] = { verify(ARRAY_CARDINALITY(failEscapeCodes) == VIR_HOST_VALIDATE_LAST); void virHostMsgFail(virHostValidateLevel level, - const char *hint) + const char *format, + ...) { + va_list args; + char *msg; + + if (quiet) + return; + + va_start(args, format); + if (virVasprintf(&msg, format, args) < 0) { + perror("malloc"); + abort(); + } + va_end(args); + if (virHostMsgWantEscape()) fprintf(stdout, "%s%s\033[0m (%s)\n", - failEscapeCodes[level], _(failMessages[level]), hint); + failEscapeCodes[level], _(failMessages[level]), msg); else fprintf(stdout, "%s (%s)\n", - _(failMessages[level]), hint); + _(failMessages[level]), msg); + free(msg); } @@ -123,7 +139,7 @@ int virHostValidateDeviceExists(const char *hvname, virHostMsgCheck(hvname, "if device %s exists", dev_name); if (access(dev_name, F_OK) < 0) { - virHostMsgFail(level, hint); + virHostMsgFail(level, "%s", hint); return -1; } @@ -140,7 +156,7 @@ int virHostValidateDeviceAccessible(const char *hvname, virHostMsgCheck(hvname, "if device %s is accessible", dev_name); if (access(dev_name, R_OK|W_OK) < 0) { - virHostMsgFail(level, hint); + virHostMsgFail(level, "%s", hint); return -1; } @@ -160,7 +176,7 @@ int virHostValidateNamespace(const char *hvname, snprintf(nspath, sizeof(nspath), "/proc/self/ns/%s", ns_name); if (access(nspath, F_OK) < 0) { - virHostMsgFail(level, hint); + virHostMsgFail(level, "%s", hint); return -1; } @@ -211,20 +227,116 @@ int virHostValidateLinuxKernel(const char *hvname, (version & 0xff)); if (STRNEQ(uts.sysname, "Linux")) { - virHostMsgFail(level, hint); + virHostMsgFail(level, "%s", hint); return -1; } if (virParseVersionString(uts.release, &thisversion, true) < 0) { - virHostMsgFail(level, hint); + virHostMsgFail(level, "%s", hint); return -1; } if (thisversion < version) { - virHostMsgFail(level, hint); + virHostMsgFail(level, "%s", hint); return -1; } else { virHostMsgPass(); return 0; } } + + +static int virHostValidateCGroupSupport(const char *hvname, + const char *cg_name, + virHostValidateLevel level, + const char *config_name) +{ + virHostMsgCheck(hvname, "for cgroup '%s' controller support", cg_name); + FILE *fp = fopen("/proc/self/cgroup", "r"); + size_t len = 0; + char *line = NULL; + ssize_t ret; + bool matched = false; + + if (!fp) + goto error; + + while ((ret = getline(&line, &len, fp)) >= 0 && !matched) { + char *tmp = strstr(line, cg_name); + if (!tmp) + continue; + + tmp += strlen(cg_name); + if (*tmp != ',' && + *tmp != ':') + continue; + + matched = true; + } + free(line); + fclose(fp); + if (!matched) + goto error; + + virHostMsgPass(); + return 0; + + error: + free(line); + virHostMsgFail(level, "Enable CONFIG_%s in kernel Kconfig file", config_name); + return -1; +} + +static int virHostValidateCGroupMount(const char *hvname, + const char *cg_name, + virHostValidateLevel level) +{ + virHostMsgCheck(hvname, "for cgroup '%s' controller mount-point", cg_name); + FILE *fp = setmntent("/proc/mounts", "r"); + struct mntent *ent; + bool matched = false; + + if (!fp) + goto error; + + while ((ent = getmntent(fp)) && !matched) { + char *tmp = strstr(ent->mnt_opts, cg_name); + if (!tmp) + continue; + + tmp += strlen(cg_name); + if (*tmp != ',' && + *tmp != '\0') + continue; + + matched = true; + } + endmntent(fp); + if (!matched) + goto error; + + virHostMsgPass(); + return 0; + + error: + virHostMsgFail(level, "Mount '%s' cgroup controller (suggested at /sys/fs/cgroup/%s)", + cg_name, cg_name); + return -1; +} + +extern int virHostValidateCGroupController(const char *hvname, + const char *cg_name, + virHostValidateLevel level, + const char *config_name) +{ + if (virHostValidateCGroupSupport(hvname, + cg_name, + level, + config_name) < 0) + return -1; + if (virHostValidateCGroupMount(hvname, + cg_name, + level) < 0) + return -1; + return 0; +} diff --git a/tools/virt-host-validate-common.h b/tools/virt-host-validate-common.h index c25e69c..1547e22 100644 --- a/tools/virt-host-validate-common.h +++ b/tools/virt-host-validate-common.h @@ -40,7 +40,8 @@ extern void virHostMsgCheck(const char *prefix, extern void virHostMsgPass(void); extern void virHostMsgFail(virHostValidateLevel level, - const char *hint); + const char *format, + ...) ATTRIBUTE_FMT_PRINTF(2, 3); extern int virHostValidateDeviceExists(const char *hvname, const char *dev_name, @@ -64,4 +65,9 @@ extern int virHostValidateNamespace(const char *hvname, virHostValidateLevel level, const char *hint); +extern int virHostValidateCGroupController(const char *hvname, + const char *cg_name, + virHostValidateLevel level, + const char *config_name); + #endif /* __VIRT_HOST_VALIDATE_COMMON_H__ */ diff --git a/tools/virt-host-validate-lxc.c b/tools/virt-host-validate-lxc.c index 43c3f5f..48323da 100644 --- a/tools/virt-host-validate-lxc.c +++ b/tools/virt-host-validate-lxc.c @@ -63,5 +63,35 @@ int virHostValidateLXC(void) _("User namespace support is recommended")) < 0) ret = -1; + if (virHostValidateCGroupController("LXC", "memory", + VIR_HOST_VALIDATE_FAIL, + "MEMCG") < 0) + ret = -1; + + if (virHostValidateCGroupController("LXC", "cpu", + VIR_HOST_VALIDATE_FAIL, + "CGROUP_SCHED") < 0) + ret = -1; + + if (virHostValidateCGroupController("LXC", "cpuacct", + VIR_HOST_VALIDATE_FAIL, + "CGROUP_CPUACCT") < 0) + ret = -1; + + if (virHostValidateCGroupController("LXC", "devices", + VIR_HOST_VALIDATE_FAIL, + "CGROUP_DEVICE") < 0) + ret = -1; + + if (virHostValidateCGroupController("LXC", "net_cls", + VIR_HOST_VALIDATE_WARN, + "NET_CLS_CGROUP") < 0) + ret = -1; + + if (virHostValidateCGroupController("LXC", "freezer", + VIR_HOST_VALIDATE_WARN, + "CGROUP_FREEZER") < 0) + ret = -1; + return ret; } diff --git a/tools/virt-host-validate-qemu.c b/tools/virt-host-validate-qemu.c index 9486064..b0ae293 100644 --- a/tools/virt-host-validate-qemu.c +++ b/tools/virt-host-validate-qemu.c @@ -57,5 +57,35 @@ int virHostValidateQEMU(void) _("Load the 'tun' module to enable networking for QEMU guests")) < 0) ret = -1; + if (virHostValidateCGroupController("QEMU", "memory", + VIR_HOST_VALIDATE_WARN, + "MEMCG") < 0) + ret = -1; + + if (virHostValidateCGroupController("QEMU", "cpu", + VIR_HOST_VALIDATE_WARN, + "CGROUP_CPU") < 0) + ret = -1; + + if (virHostValidateCGroupController("QEMU", "cpuacct", + VIR_HOST_VALIDATE_WARN, + "CGROUP_CPUACCT") < 0) + ret = -1; + + if (virHostValidateCGroupController("QEMU", "devices", + VIR_HOST_VALIDATE_WARN, + "CGROUP_DEVICES") < 0) + ret = -1; + + if (virHostValidateCGroupController("QEMU", "net_cls", + VIR_HOST_VALIDATE_WARN, + "NET_CLS_CGROUP") < 0) + ret = -1; + + if (virHostValidateCGroupController("QEMU", "blkio", + VIR_HOST_VALIDATE_WARN, + "BLK_CGROUP") < 0) + ret = -1; + return ret; } -- 2.4.3
On 10/07/2015 12:50 PM, Daniel P. Berrange wrote:
Extend the virt-host-validate checks to see if the required cgroups are compiled into the kernel and that they are mounted on the system. The cgroups are all optional except for 3 that LXC mandates
Signed-off-by: Daniel P. Berrange <berrange@redhat.com> --- tools/virt-host-validate-common.c | 130 +++++++++++++++++++++++++++++++++++--- tools/virt-host-validate-common.h | 8 ++- tools/virt-host-validate-lxc.c | 30 +++++++++ tools/virt-host-validate-qemu.c | 30 +++++++++ 4 files changed, 188 insertions(+), 10 deletions(-)
diff --git a/tools/virt-host-validate-common.c b/tools/virt-host-validate-common.c index d646a79..7e8d619 100644 --- a/tools/virt-host-validate-common.c +++ b/tools/virt-host-validate-common.c @@ -26,6 +26,7 @@ #include <stdio.h> #include <unistd.h> #include <sys/utsname.h> +#include <mntent.h>
#include "virutil.h" #include "viralloc.h" @@ -104,14 +105,29 @@ static const char *failEscapeCodes[] = { verify(ARRAY_CARDINALITY(failEscapeCodes) == VIR_HOST_VALIDATE_LAST);
void virHostMsgFail(virHostValidateLevel level, - const char *hint) + const char *format, + ...) { + va_list args; + char *msg; + + if (quiet) + return; + + va_start(args, format); + if (virVasprintf(&msg, format, args) < 0) { + perror("malloc"); + abort(); + } + va_end(args); + if (virHostMsgWantEscape()) fprintf(stdout, "%s%s\033[0m (%s)\n", - failEscapeCodes[level], _(failMessages[level]), hint); + failEscapeCodes[level], _(failMessages[level]), msg); else fprintf(stdout, "%s (%s)\n", - _(failMessages[level]), hint); + _(failMessages[level]), msg); + free(msg);
make syntax-check doesn't complain, but should we be consistently using VIR_FREE()? (actually it looks like there are a lot of these already existing, although VIR_FREE() has at least one use in virt-host-validate-common.c dating all the way back to the original commit)
}
@@ -123,7 +139,7 @@ int virHostValidateDeviceExists(const char *hvname, virHostMsgCheck(hvname, "if device %s exists", dev_name);
if (access(dev_name, F_OK) < 0) { - virHostMsgFail(level, hint); + virHostMsgFail(level, "%s", hint); return -1; }
@@ -140,7 +156,7 @@ int virHostValidateDeviceAccessible(const char *hvname, virHostMsgCheck(hvname, "if device %s is accessible", dev_name);
if (access(dev_name, R_OK|W_OK) < 0) { - virHostMsgFail(level, hint); + virHostMsgFail(level, "%s", hint); return -1; }
@@ -160,7 +176,7 @@ int virHostValidateNamespace(const char *hvname, snprintf(nspath, sizeof(nspath), "/proc/self/ns/%s", ns_name);
if (access(nspath, F_OK) < 0) { - virHostMsgFail(level, hint); + virHostMsgFail(level, "%s", hint); return -1; }
@@ -211,20 +227,116 @@ int virHostValidateLinuxKernel(const char *hvname, (version & 0xff));
if (STRNEQ(uts.sysname, "Linux")) { - virHostMsgFail(level, hint); + virHostMsgFail(level, "%s", hint); return -1; }
if (virParseVersionString(uts.release, &thisversion, true) < 0) { - virHostMsgFail(level, hint); + virHostMsgFail(level, "%s", hint); return -1; }
if (thisversion < version) { - virHostMsgFail(level, hint); + virHostMsgFail(level, "%s", hint); return -1; } else { virHostMsgPass(); return 0; } } + + +static int virHostValidateCGroupSupport(const char *hvname, + const char *cg_name, + virHostValidateLevel level, + const char *config_name) +{ + virHostMsgCheck(hvname, "for cgroup '%s' controller support", cg_name); + FILE *fp = fopen("/proc/self/cgroup", "r"); + size_t len = 0; + char *line = NULL; + ssize_t ret; + bool matched = false; + + if (!fp) + goto error; + + while ((ret = getline(&line, &len, fp)) >= 0 && !matched) { + char *tmp = strstr(line, cg_name); + if (!tmp) + continue; + + tmp += strlen(cg_name); + if (*tmp != ',' && + *tmp != ':') + continue; + + matched = true; + } + free(line); + fclose(fp);
This one does fail make syntax-check - wants it to be VIR_FCLOSE(fp). ACK, aside from replacing the libc functions with VIR_* equivalents.
On 10/08/2015 02:52 PM, Laine Stump wrote:
On 10/07/2015 12:50 PM, Daniel P. Berrange wrote:
else fprintf(stdout, "%s (%s)\n", - _(failMessages[level]), hint); + _(failMessages[level]), msg); + free(msg);
make syntax-check doesn't complain, but should we be consistently using VIR_FREE()? (actually it looks like there are a lot of these already existing, although VIR_FREE() has at least one use in virt-host-validate-common.c dating all the way back to the original commit)
I spoke too soon - when I fixed the fclose() occurences, *then* syntax-check complaining about free() (and I also saw that all of the "already existing" occurrences were actually newly added in this same patch.
On 10/07/2015 12:50 PM, Daniel P. Berrange wrote:
Extend the virt-host-validate checks to see if the required cgroups are compiled into the kernel and that they are mounted on the system. The cgroups are all optional except for 3 that LXC mandates
Signed-off-by: Daniel P. Berrange <berrange@redhat.com> --- tools/virt-host-validate-common.c | 130 +++++++++++++++++++++++++++++++++++--- tools/virt-host-validate-common.h | 8 ++- tools/virt-host-validate-lxc.c | 30 +++++++++ tools/virt-host-validate-qemu.c | 30 +++++++++ 4 files changed, 188 insertions(+), 10 deletions(-)
diff --git a/tools/virt-host-validate-common.c b/tools/virt-host-validate-common.c index d646a79..7e8d619 100644 --- a/tools/virt-host-validate-common.c +++ b/tools/virt-host-validate-common.c @@ -26,6 +26,7 @@ #include <stdio.h> #include <unistd.h> #include <sys/utsname.h> +#include <mntent.h>
#include "virutil.h" #include "viralloc.h" @@ -104,14 +105,29 @@ static const char *failEscapeCodes[] = { verify(ARRAY_CARDINALITY(failEscapeCodes) == VIR_HOST_VALIDATE_LAST);
void virHostMsgFail(virHostValidateLevel level, - const char *hint) + const char *format, + ...) { + va_list args; + char *msg; + + if (quiet) + return; + + va_start(args, format); + if (virVasprintf(&msg, format, args) < 0) { + perror("malloc"); + abort(); + } + va_end(args); + if (virHostMsgWantEscape()) fprintf(stdout, "%s%s\033[0m (%s)\n", - failEscapeCodes[level], _(failMessages[level]), hint); + failEscapeCodes[level], _(failMessages[level]), msg); else fprintf(stdout, "%s (%s)\n", - _(failMessages[level]), hint); + _(failMessages[level]), msg); + free(msg); }
@@ -123,7 +139,7 @@ int virHostValidateDeviceExists(const char *hvname, virHostMsgCheck(hvname, "if device %s exists", dev_name);
if (access(dev_name, F_OK) < 0) { - virHostMsgFail(level, hint); + virHostMsgFail(level, "%s", hint); return -1; }
@@ -140,7 +156,7 @@ int virHostValidateDeviceAccessible(const char *hvname, virHostMsgCheck(hvname, "if device %s is accessible", dev_name);
if (access(dev_name, R_OK|W_OK) < 0) { - virHostMsgFail(level, hint); + virHostMsgFail(level, "%s", hint); return -1; }
@@ -160,7 +176,7 @@ int virHostValidateNamespace(const char *hvname, snprintf(nspath, sizeof(nspath), "/proc/self/ns/%s", ns_name);
if (access(nspath, F_OK) < 0) { - virHostMsgFail(level, hint); + virHostMsgFail(level, "%s", hint); return -1; }
@@ -211,20 +227,116 @@ int virHostValidateLinuxKernel(const char *hvname, (version & 0xff));
if (STRNEQ(uts.sysname, "Linux")) { - virHostMsgFail(level, hint); + virHostMsgFail(level, "%s", hint); return -1; }
if (virParseVersionString(uts.release, &thisversion, true) < 0) { - virHostMsgFail(level, hint); + virHostMsgFail(level, "%s", hint); return -1; }
if (thisversion < version) { - virHostMsgFail(level, hint); + virHostMsgFail(level, "%s", hint); return -1; } else { virHostMsgPass(); return 0; } } + + +static int virHostValidateCGroupSupport(const char *hvname, + const char *cg_name, + virHostValidateLevel level, + const char *config_name) +{ + virHostMsgCheck(hvname, "for cgroup '%s' controller support", cg_name); + FILE *fp = fopen("/proc/self/cgroup", "r"); + size_t len = 0; + char *line = NULL; + ssize_t ret; + bool matched = false; + + if (!fp) + goto error; + + while ((ret = getline(&line, &len, fp)) >= 0 && !matched) { + char *tmp = strstr(line, cg_name); + if (!tmp) + continue; + + tmp += strlen(cg_name); + if (*tmp != ',' && + *tmp != ':') + continue; + + matched = true; + } + free(line); + fclose(fp); + if (!matched) + goto error; + + virHostMsgPass(); + return 0; + + error: + free(line); + virHostMsgFail(level, "Enable CONFIG_%s in kernel Kconfig file", config_name); + return -1; +} + +static int virHostValidateCGroupMount(const char *hvname, + const char *cg_name, + virHostValidateLevel level) +{ + virHostMsgCheck(hvname, "for cgroup '%s' controller mount-point", cg_name); + FILE *fp = setmntent("/proc/mounts", "r"); + struct mntent *ent; + bool matched = false; + + if (!fp) + goto error; + + while ((ent = getmntent(fp)) && !matched) {
Oops, another make syntax-check fail: tools/virt-host-validate-common.c:302: while ((ent = getmntent(fp)) && !matched) { maint.mk: use getmntent_r, not getmntent
This looks for existance of /sys/firmware/acpi/tables/DMAR as a reasonable hint that the BIOS support an Intel IOMMU. This file is only present if enabled in the BIOS, so the check only does something if the BIOS is enabled but the kernel has it disabled. TBD figure out a check for AMD too, which doesn't involve grepping dmesg which might have already been purged. Signed-off-by: Daniel P. Berrange <berrange@redhat.com> --- tools/virt-host-validate-common.c | 36 ++++++++++++++++++++++++++++++++++++ tools/virt-host-validate-common.h | 5 +++++ tools/virt-host-validate-qemu.c | 10 ++++++++++ 3 files changed, 51 insertions(+) diff --git a/tools/virt-host-validate-common.c b/tools/virt-host-validate-common.c index 7e8d619..093acf6 100644 --- a/tools/virt-host-validate-common.c +++ b/tools/virt-host-validate-common.c @@ -340,3 +340,39 @@ extern int virHostValidateCGroupController(const char *hvname, return -1; return 0; } + +extern int virHostValidateKernelCmdline(const char *hvname, + const char *arg, + virHostValidateLevel level, + const char *hint) +{ + FILE *fp = fopen("/proc/cmdline", "r"); + char *line = NULL; + size_t len = 0; + char *tmp; + + virHostMsgCheck(hvname, "/proc/cmdline for '%s'", arg); + + if (!fp) + goto error; + + if (getline(&line, &len, fp) < 0) + goto error; + + tmp = strstr(line, arg); + if (!tmp) + goto error; + tmp += strlen(arg); + if (*tmp != '\0' && + *tmp != ' ') + goto error; + + fclose(fp); + virHostMsgPass(); + return 0; + + error: + fclose(fp); + virHostMsgFail(level, "%s", hint); + return -1; +} diff --git a/tools/virt-host-validate-common.h b/tools/virt-host-validate-common.h index 1547e22..a01e47b 100644 --- a/tools/virt-host-validate-common.h +++ b/tools/virt-host-validate-common.h @@ -60,6 +60,11 @@ extern int virHostValidateLinuxKernel(const char *hvname, virHostValidateLevel level, const char *hint); +extern int virHostValidateKernelCmdline(const char *hvname, + const char *arg, + virHostValidateLevel level, + const char *hint); + extern int virHostValidateNamespace(const char *hvname, const char *ns_name, virHostValidateLevel level, diff --git a/tools/virt-host-validate-qemu.c b/tools/virt-host-validate-qemu.c index b0ae293..1917962 100644 --- a/tools/virt-host-validate-qemu.c +++ b/tools/virt-host-validate-qemu.c @@ -20,6 +20,8 @@ */ #include <config.h> +#include <unistd.h> + #include "virt-host-validate-qemu.h" #include "virt-host-validate-common.h" @@ -87,5 +89,13 @@ int virHostValidateQEMU(void) "BLK_CGROUP") < 0) ret = -1; + if (access("/sys/firmware/acpi/tables/DMAR", F_OK) == 0 && + virHostValidateHasCPUFlag("vmx") && + virHostValidateKernelCmdline("QEMU", "intel_iommu=on", + VIR_HOST_VALIDATE_WARN, + "IOMMU present in system but disabled in kernel. " + "Add intel_iommu=on to kernel cmdline arguments") < 0) + ret = -1; + return ret; } -- 2.4.3
On Wed, 2015-10-07 at 17:50 +0100, Daniel P. Berrange wrote:
This looks for existance of /sys/firmware/acpi/tables/DMAR as a reasonable hint that the BIOS support an Intel IOMMU.
This file is only present if enabled in the BIOS, so the check only does something if the BIOS is enabled but the kernel has it disabled.
TBD figure out a check for AMD too, which doesn't involve grepping dmesg which might have already been purged.
Signed-off-by: Daniel P. Berrange <berrange@redhat.com> --- tools/virt-host-validate-common.c | 36 ++++++++++++++++++++++++++++++++++++ tools/virt-host-validate-common.h | 5 +++++ tools/virt-host-validate-qemu.c | 10 ++++++++++ 3 files changed, 51 insertions(+)
diff --git a/tools/virt-host-validate-common.c b/tools/virt-host-validate-common.c index 7e8d619..093acf6 100644 --- a/tools/virt-host-validate-common.c +++ b/tools/virt-host-validate-common.c @@ -340,3 +340,39 @@ extern int virHostValidateCGroupController(const char *hvname, return -1; return 0; } + +extern int virHostValidateKernelCmdline(const char *hvname, + const char *arg, + virHostValidateLevel level, + const char *hint) +{ + FILE *fp = fopen("/proc/cmdline", "r"); + char *line = NULL; + size_t len = 0; + char *tmp; + + virHostMsgCheck(hvname, "/proc/cmdline for '%s'", arg); + + if (!fp) + goto error; + + if (getline(&line, &len, fp) < 0) + goto error; + + tmp = strstr(line, arg); + if (!tmp) + goto error; + tmp += strlen(arg); + if (*tmp != '\0' && + *tmp != ' ') + goto error; + + fclose(fp); + virHostMsgPass(); + return 0; + + error: + fclose(fp); + virHostMsgFail(level, "%s", hint); + return -1; +} diff --git a/tools/virt-host-validate-common.h b/tools/virt-host-validate-common.h index 1547e22..a01e47b 100644 --- a/tools/virt-host-validate-common.h +++ b/tools/virt-host-validate-common.h @@ -60,6 +60,11 @@ extern int virHostValidateLinuxKernel(const char *hvname, virHostValidateLevel level, const char *hint);
+extern int virHostValidateKernelCmdline(const char *hvname, + const char *arg, + virHostValidateLevel level, + const char *hint); + extern int virHostValidateNamespace(const char *hvname, const char *ns_name, virHostValidateLevel level, diff --git a/tools/virt-host-validate-qemu.c b/tools/virt-host-validate-qemu.c index b0ae293..1917962 100644 --- a/tools/virt-host-validate-qemu.c +++ b/tools/virt-host-validate-qemu.c @@ -20,6 +20,8 @@ */
#include <config.h> +#include <unistd.h> + #include "virt-host-validate-qemu.h" #include "virt-host-validate-common.h"
@@ -87,5 +89,13 @@ int virHostValidateQEMU(void) "BLK_CGROUP") < 0) ret = -1;
+ if (access("/sys/firmware/acpi/tables/DMAR", F_OK) == 0 && + virHostValidateHasCPUFlag("vmx") && + virHostValidateKernelCmdline("QEMU", "intel_iommu=on", + VIR_HOST_VALIDATE_WARN, + "IOMMU present in system but disabled in kernel. " + "Add intel_iommu=on to kernel cmdline arguments") < 0) + ret = -1; + return ret; }
The IVRS table is the equivalent that you'd look for on AMD. amd_iommu=on is more often enabled by default, so the test probably still applies (s/vmx/svm/), but it usually seems to be less often a problem that it's simply not enabled. Intel can also be configured on by default so you might get some false hits if the firmware tables or hardware are just too broken to be initialized, but it at least still points users to a kernel problem rather than a libvirt issue. Thanks, Alex
On 10/07/2015 12:50 PM, Daniel P. Berrange wrote:
This looks for existance of /sys/firmware/acpi/tables/DMAR as a reasonable hint that the BIOS support an Intel IOMMU.
This file is only present if enabled in the BIOS, so the check only does something if the BIOS is enabled but the kernel has it disabled.
TBD figure out a check for AMD too, which doesn't involve grepping dmesg which might have already been purged.
I have a couple AMD machines, if you want anything tested. ACK with what's here though.
Signed-off-by: Daniel P. Berrange <berrange@redhat.com> --- tools/virt-host-validate-common.c | 36 ++++++++++++++++++++++++++++++++++++ tools/virt-host-validate-common.h | 5 +++++ tools/virt-host-validate-qemu.c | 10 ++++++++++ 3 files changed, 51 insertions(+)
diff --git a/tools/virt-host-validate-common.c b/tools/virt-host-validate-common.c index 7e8d619..093acf6 100644 --- a/tools/virt-host-validate-common.c +++ b/tools/virt-host-validate-common.c @@ -340,3 +340,39 @@ extern int virHostValidateCGroupController(const char *hvname, return -1; return 0; } + +extern int virHostValidateKernelCmdline(const char *hvname, + const char *arg, + virHostValidateLevel level, + const char *hint) +{ + FILE *fp = fopen("/proc/cmdline", "r"); + char *line = NULL; + size_t len = 0; + char *tmp; + + virHostMsgCheck(hvname, "/proc/cmdline for '%s'", arg); + + if (!fp) + goto error; + + if (getline(&line, &len, fp) < 0) + goto error; + + tmp = strstr(line, arg); + if (!tmp) + goto error; + tmp += strlen(arg); + if (*tmp != '\0' && + *tmp != ' ') + goto error; + + fclose(fp); + virHostMsgPass(); + return 0; + + error: + fclose(fp); + virHostMsgFail(level, "%s", hint); + return -1; +} diff --git a/tools/virt-host-validate-common.h b/tools/virt-host-validate-common.h index 1547e22..a01e47b 100644 --- a/tools/virt-host-validate-common.h +++ b/tools/virt-host-validate-common.h @@ -60,6 +60,11 @@ extern int virHostValidateLinuxKernel(const char *hvname, virHostValidateLevel level, const char *hint);
+extern int virHostValidateKernelCmdline(const char *hvname, + const char *arg, + virHostValidateLevel level, + const char *hint); + extern int virHostValidateNamespace(const char *hvname, const char *ns_name, virHostValidateLevel level, diff --git a/tools/virt-host-validate-qemu.c b/tools/virt-host-validate-qemu.c index b0ae293..1917962 100644 --- a/tools/virt-host-validate-qemu.c +++ b/tools/virt-host-validate-qemu.c @@ -20,6 +20,8 @@ */
#include <config.h> +#include <unistd.h> + #include "virt-host-validate-qemu.h" #include "virt-host-validate-common.h"
@@ -87,5 +89,13 @@ int virHostValidateQEMU(void) "BLK_CGROUP") < 0) ret = -1;
+ if (access("/sys/firmware/acpi/tables/DMAR", F_OK) == 0 && + virHostValidateHasCPUFlag("vmx") && + virHostValidateKernelCmdline("QEMU", "intel_iommu=on", + VIR_HOST_VALIDATE_WARN, + "IOMMU present in system but disabled in kernel. " + "Add intel_iommu=on to kernel cmdline arguments") < 0) + ret = -1; + return ret; }
On Thu, Oct 08, 2015 at 03:09:02PM -0400, Laine Stump wrote:
On 10/07/2015 12:50 PM, Daniel P. Berrange wrote:
This looks for existance of /sys/firmware/acpi/tables/DMAR as a reasonable hint that the BIOS support an Intel IOMMU.
This file is only present if enabled in the BIOS, so the check only does something if the BIOS is enabled but the kernel has it disabled.
TBD figure out a check for AMD too, which doesn't involve grepping dmesg which might have already been purged.
I have a couple AMD machines, if you want anything tested.
I'll knock something up for AMD per Alex's recommendation and then post another patch for that.
ACK with what's here though.
Regards, Daniel -- |: http://berrange.com -o- http://www.flickr.com/photos/dberrange/ :| |: http://libvirt.org -o- http://virt-manager.org :| |: http://autobuild.org -o- http://search.cpan.org/~danberr/ :| |: http://entangle-photo.org -o- http://live.gnome.org/gtk-vnc :|
participants (3)
-
Alex Williamson -
Daniel P. Berrange -
Laine Stump