[libvirt] [PATCH] tools: virt-host-validate: fix KVM check on s390

[Daniel P. Berrange]

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 at linux.vnet.ibm.com>
> > Reviewed-by: Boris Fiuczynski <fiuczy at 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 :|