[libvirt] [PATCH] tools: virt-host-validate: fix KVM check on s390
Since kernel version 4.5, s390 has the 'sie' flag to declare its hardware virtualization support. Let's make virt-host-validate aware so this check is passed correctly. Signed-off-by: Bjoern Walk <bwalk@linux.vnet.ibm.com> Reviewed-by: Boris Fiuczynski <fiuczy@linux.vnet.ibm.com> --- tools/virt-host-validate-qemu.c | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/tools/virt-host-validate-qemu.c b/tools/virt-host-validate-qemu.c index a9f6c1e..01fb24b 100644 --- a/tools/virt-host-validate-qemu.c +++ b/tools/virt-host-validate-qemu.c @@ -31,7 +31,8 @@ int virHostValidateQEMU(void) virHostMsgCheck("QEMU", "%s", ("for hardware virtualization")); if (virHostValidateHasCPUFlag("svm") || - virHostValidateHasCPUFlag("vmx")) { + virHostValidateHasCPUFlag("vmx") || + virHostValidateHasCPUFlag("sie")) { virHostMsgPass(); if (virHostValidateDeviceExists("QEMU", "/dev/kvm", VIR_HOST_VALIDATE_FAIL, -- 2.6.5
On 03/21/2016 07:55 AM, Bjoern Walk wrote:
Since kernel version 4.5, s390 has the 'sie' flag to declare its hardware virtualization support. Let's make virt-host-validate aware so this check is passed correctly.
Signed-off-by: Bjoern Walk <bwalk@linux.vnet.ibm.com> Reviewed-by: Boris Fiuczynski <fiuczy@linux.vnet.ibm.com> --- tools/virt-host-validate-qemu.c | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-)
I've never seen the output of /proc/cpuinfo on an S390, but looking at the source code I found it does seem to print with the "features" for a cpuinfo on an s390 machine (as opposed the "flags" for a vmx/svm machine). The virHostValidateHasCPUFlag doesn't really distinguish "flags" or "features" field being specified as a label (although I imagine that could have internationalization impact if we tried to limit that more). Anyway, before I push this - I wanted to see if there were any "other" thoughts regarding letting this be a bit more generic. My one concern is some field name some day has "sie" added (e.g. "easier" or "transient" would match). One change that may help with that would be to check "sie " instead of "sie" (similarly "svm" could be "svm " and "vmx" could be "vmx "). The source code prints the output as "..."%s ", int_hwcap_str[i]..." John
diff --git a/tools/virt-host-validate-qemu.c b/tools/virt-host-validate-qemu.c index a9f6c1e..01fb24b 100644 --- a/tools/virt-host-validate-qemu.c +++ b/tools/virt-host-validate-qemu.c @@ -31,7 +31,8 @@ int virHostValidateQEMU(void)
virHostMsgCheck("QEMU", "%s", ("for hardware virtualization")); if (virHostValidateHasCPUFlag("svm") || - virHostValidateHasCPUFlag("vmx")) { + virHostValidateHasCPUFlag("vmx") || + virHostValidateHasCPUFlag("sie")) { virHostMsgPass(); if (virHostValidateDeviceExists("QEMU", "/dev/kvm", VIR_HOST_VALIDATE_FAIL,
On Mon, Mar 28, 2016 at 02:45:18PM -0400, John Ferlan wrote:
On 03/21/2016 07:55 AM, Bjoern Walk wrote:
Since kernel version 4.5, s390 has the 'sie' flag to declare its hardware virtualization support. Let's make virt-host-validate aware so this check is passed correctly.
Signed-off-by: Bjoern Walk <bwalk@linux.vnet.ibm.com> Reviewed-by: Boris Fiuczynski <fiuczy@linux.vnet.ibm.com> --- tools/virt-host-validate-qemu.c | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-)
I've never seen the output of /proc/cpuinfo on an S390, but looking at the source code I found it does seem to print with the "features" for a cpuinfo on an s390 machine (as opposed the "flags" for a vmx/svm machine).
The virHostValidateHasCPUFlag doesn't really distinguish "flags" or "features" field being specified as a label (although I imagine that could have internationalization impact if we tried to limit that more).
Anyway, before I push this - I wanted to see if there were any "other" thoughts regarding letting this be a bit more generic. My one concern is some field name some day has "sie" added (e.g. "easier" or "transient" would match).
One change that may help with that would be to check "sie " instead of "sie" (similarly "svm" could be "svm " and "vmx" could be "vmx "). The source code prints the output as "..."%s ", int_hwcap_str[i]..."
Yes this is a good point, but also its a pre-existing problem with the virHostValidateHasCPUFlag method, so not a reason to block this patch. It can be fixed separately as desired.
diff --git a/tools/virt-host-validate-qemu.c b/tools/virt-host-validate-qemu.c index a9f6c1e..01fb24b 100644 --- a/tools/virt-host-validate-qemu.c +++ b/tools/virt-host-validate-qemu.c @@ -31,7 +31,8 @@ int virHostValidateQEMU(void)
virHostMsgCheck("QEMU", "%s", ("for hardware virtualization")); if (virHostValidateHasCPUFlag("svm") || - virHostValidateHasCPUFlag("vmx")) { + virHostValidateHasCPUFlag("vmx") || + virHostValidateHasCPUFlag("sie")) { virHostMsgPass(); if (virHostValidateDeviceExists("QEMU", "/dev/kvm", VIR_HOST_VALIDATE_FAIL,
What we should do here though is check the host architecture before checking CPU flags. eg you'll want to make this code do something like this bool hasHWVirt = false; switch (virArchFromHost()) { case VIR_ARCH_I686: case VIR_ARCH_X86_64: if (virHostValidateHasCPUFlag("svm") || virHostValidateHasCPUFlag("vmx")) hasHWVirt = true; break; case VIR_ARCH_S390: case VIR_ARCH_S390X: if (virHostValidateHasCPUFlag("sie")) hasHWVirt = true; } if (hasHWVirt) { ....do the /dev/kvm check... } 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 :|
On Tue, 2016-03-29 at 14:01 +0100, Daniel P. Berrange wrote:
virHostMsgCheck("QEMU", "%s", ("for hardware virtualization")); if (virHostValidateHasCPUFlag("svm") || - virHostValidateHasCPUFlag("vmx")) { + virHostValidateHasCPUFlag("vmx") || + virHostValidateHasCPUFlag("sie")) { virHostMsgPass(); if (virHostValidateDeviceExists("QEMU", "/dev/kvm", VIR_HOST_VALIDATE_FAIL, What we should do here though is check the host architecture before checking CPU flags. eg you'll want to make this code do something like this bool hasHWVirt = false; switch (virArchFromHost()) { case VIR_ARCH_I686: case VIR_ARCH_X86_64: if (virHostValidateHasCPUFlag("svm") || virHostValidateHasCPUFlag("vmx")) hasHWVirt = true; break; case VIR_ARCH_S390: case VIR_ARCH_S390X: if (virHostValidateHasCPUFlag("sie")) hasHWVirt = true; } if (hasHWVirt) { ....do the /dev/kvm check... }
... And I guess I should come up with suitable checks for ppc64 and aarch64 as well :) Cheers. -- Andrea Bolognani Software Engineer - Virtualization Team
participants (4)
-
Andrea Bolognani -
Bjoern Walk -
Daniel P. Berrange -
John Ferlan