Skip to contentSkip to conversation

kvm_para_available() should check hypervisor bit before accessing hypervisor cpuid leaf.

PATCH Applied Gleb Natapov 3 participants 9 messages Apr 30, 2012 – May 6, 2012

Subsystems (4)
x86 KVM KVM x86 KVM paravirt
lore
Share & tools
lore

Gleb Natapov

author
#0 lore raw

PATCH 1/1 kvm_para_available() should check hypervisor bit before accessing hypervisor cpuid leaf.

This couid range does not exist on real HW and Intel spec says that "Information returned for highest basic information leaf" will be returned. Not very well defined.

Signed-off-by Gleb Natapov <[email protected]>
arch/x86/include/asm/kvm_para.h +10 -8
index 99c4bbe..a7a7a94 100644
@@ -178,14 +178,16 @@ static inline int kvm_para_available(void)
 	unsigned int eax, ebx, ecx, edx;
 	char signature[13];
 
-	cpuid(KVM_CPUID_SIGNATURE, &eax, &ebx, &ecx, &edx);
-	memcpy(signature + 0, &ebx, 4);
-	memcpy(signature + 4, &ecx, 4);
-	memcpy(signature + 8, &edx, 4);
-	signature[12] = 0;
-
-	if (strcmp(signature, "KVMKVMKVM") == 0)
-		return 1;
+	if (cpu_has_hypervisor) {
+		cpuid(KVM_CPUID_SIGNATURE, &eax, &ebx, &ecx, &edx);
+		memcpy(signature + 0, &ebx, 4);
+		memcpy(signature + 4, &ecx, 4);
+		memcpy(signature + 8, &edx, 4);
+		signature[12] = 0;
+
+		if (strcmp(signature, "KVMKVMKVM") == 0)
+			return 1;
+	}
 
 	return 0;
 }

--
Gleb.

#1 lore raw
Quoted 24 lines · Gleb Natapov

This couid range does not exist on real HW and Intel spec says that "Information returned for highest basic information leaf" will be returned. Not very well defined.

Signed-off-by Gleb Natapov <[email protected]>
arch/x86/include/asm/kvm_para.h +3 -8
index 99c4bbe..a7a7a94 100644
@@ -178,14 +178,16 @@ static inline int kvm_para_available(void)
 	unsigned int eax, ebx, ecx, edx;
 	char signature[13];
 
-	cpuid(KVM_CPUID_SIGNATURE, &eax, &ebx, &ecx, &edx);
-	memcpy(signature + 0, &ebx, 4);
-	memcpy(signature + 4, &ecx, 4);
-	memcpy(signature + 8, &edx, 4);
-	signature[12] = 0;
-
-	if (strcmp(signature, "KVMKVMKVM") == 0)
-		return 1;
+	if (cpu_has_hypervisor) {
+		cpuid(KVM_CPUID_SIGNATURE, &eax, &ebx, &ecx, &edx);
+		memcpy(signature + 0, &ebx, 4);

arch/x86/include/asm/kvm_para.h static inline int kvm_para_available(void) · line 193

Quoted 10 lines
arch/x86/include/asm/kvm_para.h +7 -0
+		memcpy(signature + 4, &ecx, 4);
+		memcpy(signature + 8, &edx, 4);
+		signature[12] = 0;
+
+		if (strcmp(signature, "KVMKVMKVM") == 0)
+			return 1;
+	}
 
 	return 0;
 }

Wouldn't this be better?

arch/x86/include/asm/kvm_para.h +3 -0
@@ -170,6 +170,9 @@ static inline int kvm_para_available(void)
        unsigned int eax, ebx, ecx, edx;
        char signature[13];
 
+       if (!cpu_has_hypervisor)
+               return 0;
+
        cpuid(KVM_CPUID_SIGNATURE, &eax, &ebx, &ecx, &edx);
        memcpy(signature + 0, &ebx, 4);
        memcpy(signature + 4, &ecx, 4);
author
#2 lore raw
Quoted 33 lines · Davidlohr Bueso
Quoted 32 lines · Gleb Natapov

This couid range does not exist on real HW and Intel spec says that "Information returned for highest basic information leaf" will be returned. Not very well defined.

Signed-off-by Gleb Natapov <[email protected]>
arch/x86/include/asm/kvm_para.h +10 -8
index 99c4bbe..a7a7a94 100644
@@ -178,14 +178,16 @@ static inline int kvm_para_available(void)
 	unsigned int eax, ebx, ecx, edx;
 	char signature[13];
 
-	cpuid(KVM_CPUID_SIGNATURE, &eax, &ebx, &ecx, &edx);
-	memcpy(signature + 0, &ebx, 4);
-	memcpy(signature + 4, &ecx, 4);
-	memcpy(signature + 8, &edx, 4);
-	signature[12] = 0;
-
-	if (strcmp(signature, "KVMKVMKVM") == 0)
-		return 1;
+	if (cpu_has_hypervisor) {
+		cpuid(KVM_CPUID_SIGNATURE, &eax, &ebx, &ecx, &edx);
+		memcpy(signature + 0, &ebx, 4);
+		memcpy(signature + 4, &ecx, 4);
+		memcpy(signature + 8, &edx, 4);
+		signature[12] = 0;
+
+		if (strcmp(signature, "KVMKVMKVM") == 0)
+			return 1;
+	}
Quoted 5 lines

arch/x86/include/asm/kvm_para.h static inline int kvm_para_available(void) · line 193

Quoted 2 lines
arch/x86/include/asm/kvm_para.h +0 -0
 	return 0;
 }

Wouldn't this be better?

If I thought it would I would have wrote it like that :) I prefer less exit points from a function if possible.

arch/x86/include/asm/kvm_para.h static inline int kvm_para_available(void) · line 178

Quoted 13 lines
arch/x86/include/asm/kvm_para.h +3 -0
@@ -170,6 +170,9 @@ static inline int kvm_para_available(void)
        unsigned int eax, ebx, ecx, edx;
        char signature[13];
 
+       if (!cpu_has_hypervisor)
+               return 0;
+
        cpuid(KVM_CPUID_SIGNATURE, &eax, &ebx, &ecx, &edx);
        memcpy(signature + 0, &ebx, 4);
        memcpy(signature + 4, &ecx, 4);

--
Gleb.

#3 lore raw
In reply to Gleb Natapov
Quoted 4 lines · Gleb Natapov

This couid range does not exist on real HW and Intel spec says that "Information returned for highest basic information leaf" will be returned. Not very well defined.

Correct in principle. But IIRC some old qemus supported the kvm cpuid range without setting the hypervisor bit.

Signature · 1 line
error compiling committee.c: too many arguments to function
author
#4 lore raw
Quoted 9 lines · Avi Kivity
Quoted 4 lines · Gleb Natapov

This couid range does not exist on real HW and Intel spec says that "Information returned for highest basic information leaf" will be returned. Not very well defined.

Correct in principle. But IIRC some old qemus supported the kvm cpuid range without setting the hypervisor bit.

How old?

--
Gleb.

#5 lore raw
Quoted 11 lines · Gleb Natapov
Quoted 9 lines · Avi Kivity
Quoted 4 lines · Gleb Natapov

This couid range does not exist on real HW and Intel spec says that "Information returned for highest basic information leaf" will be returned. Not very well defined.

Correct in principle. But IIRC some old qemus supported the kvm cpuid range without setting the hypervisor bit.

How old?

commit e19967a0e00c7af4efd2f2db12a7d5d7f0b387f7

Author: Avi Kivity <[email protected]>
Date:   Tue Apr 22 11:44:19 2008 +0300

Set "hypervisor present" cpuid bit

This tells Microsoft guests that they are running under a hypervisor.

Signed-off-by Avi Kivity <[email protected]
$ git describe --contains e19967a0e00c
kvm-80rc2~489
Signature · 1 line
error compiling committee.c: too many arguments to function
author
#6 lore raw
Quoted 17 lines · Avi Kivity
Quoted 11 lines · Gleb Natapov
Quoted 9 lines · Avi Kivity
Quoted 4 lines · Gleb Natapov

This couid range does not exist on real HW and Intel spec says that "Information returned for highest basic information leaf" will be returned. Not very well defined.

Correct in principle. But IIRC some old qemus supported the kvm cpuid range without setting the hypervisor bit.

How old?

commit e19967a0e00c7af4efd2f2db12a7d5d7f0b387f7

Author: Avi Kivity <[email protected]>
Date:   Tue Apr 22 11:44:19 2008 +0300
Quoted 9 lines

Set "hypervisor present" cpuid bit

This tells Microsoft guests that they are running under a hypervisor.

Signed-off-by Avi Kivity <[email protected]
$ git describe --contains e19967a0e00c
kvm-80rc2~489

Since latest Windowses will not run on such old version correctly (may use tsc for QPC) may be dropping support for Linuxes past 3.4 is a possibility too?

--
Gleb.

#7 lore raw
Quoted 8 lines · Gleb Natapov
Quoted 3 lines
$ git describe --contains e19967a0e00c
kvm-80rc2~489

Since latest Windowses will not run on such old version correctly (may use tsc for QPC) may be dropping support for Linuxes past 3.4 is a possibility too?

These versions are *way* too old to support.

Signature · 1 line
error compiling committee.c: too many arguments to function
Applied
#8 lore raw
In reply to Gleb Natapov
Quoted 5 lines · Gleb Natapov

This couid range does not exist on real HW and Intel spec says that "Information returned for highest basic information leaf" will be returned. Not very well defined.

Applied, thanks.

Signature · 1 line
error compiling committee.c: too many arguments to function