[PATCH 1/2] kvm/x86: fix XSAVE bit scanning

[PATCH 1/2] kvm/x86: fix XSAVE bit scanning

[Andre Przywara]

When KVM scans the 0xD CPUID leaf for propagating the XSAVE save area
leaves, it assumes that the leaves are contigious and stops at the
first zero one. On AMD hardware there is a gap, though, as LWP uses
leaf 62 to announce it's state save area.
So lets iterate through all 64 possible leaves and simply skip zero
ones to also cover later features.

CC: <stable@kernel.org> [2.6.38]
Signed-off-by: Andre Przywara <andre.przywara@amd.com>
---
 arch/x86/kvm/x86.c |    6 +++---
 1 files changed, 3 insertions(+), 3 deletions(-)

diff --git a/arch/x86/kvm/x86.c b/arch/x86/kvm/x86.c
index bfd7763..6e86cec 100644
--- a/arch/x86/kvm/x86.c
+++ b/arch/x86/kvm/x86.c
@@ -2395,9 +2395,9 @@ static void do_cpuid_ent(struct kvm_cpuid_entry2 *entry, u32 function,
 		int i;
 
 		entry->flags |= KVM_CPUID_FLAG_SIGNIFCANT_INDEX;
-		for (i = 1; *nent < maxnent; ++i) {
-			if (entry[i - 1].eax == 0 && i != 2)
-				break;
+		for (i = 1; *nent < maxnent && i < 64; ++i) {
+			if (entry[i].eax == 0)
+				continue;
 			do_cpuid_1_ent(&entry[i], function, i);
 			entry[i].flags |=
 			       KVM_CPUID_FLAG_SIGNIFCANT_INDEX;
-- 
1.6.4

[PATCH 2/2] kvm/x86: remove unneeded substitute search for missing CPUID entries

[Andre Przywara]

If KVM cannot find an exact match for a requested CPUID leaf, the
code will try to find the closest match instead of simply confessing
it's failure. The heuristic is on one hand wrong nowadays,
since it does not take the KVM CPUID leaves (0x400000xx) into
account. On the other hand the callers of this function can all deal
with the no-match situation. So lets remove this code, as it serves
no purpose.
This fixes a crash of newer Linux kernels as KVM guests on
AMD Bulldozer CPUs, where bogus values were returned in response to
a CPUID intercept.

CC: <stable@kernel.org> [2.6.38]
Signed-off-by: Andre Przywara <andre.przywara@amd.com>
---
 arch/x86/kvm/x86.c |    6 ------
 1 files changed, 0 insertions(+), 6 deletions(-)

diff --git a/arch/x86/kvm/x86.c b/arch/x86/kvm/x86.c
index 6e86cec..625143f 100644
--- a/arch/x86/kvm/x86.c
+++ b/arch/x86/kvm/x86.c
@@ -4959,12 +4959,6 @@ struct kvm_cpuid_entry2 *kvm_find_cpuid_entry(struct kvm_vcpu *vcpu,
 			best = e;
 			break;
 		}
-		/*
-		 * Both basic or both extended?
-		 */
-		if (((e->function ^ function) & 0x80000000) == 0)
-			if (!best || e->function > best->function)
-				best = e;
 	}
 	return best;
 }
-- 
1.6.4

Re: [PATCH 2/2] kvm/x86: remove unneeded substitute search for missing CPUID entries

[Avi Kivity]

On 03/30/2011 03:01 PM, Andre Przywara wrote:
> If KVM cannot find an exact match for a requested CPUID leaf, the
> code will try to find the closest match instead of simply confessing
> it's failure. The heuristic is on one hand wrong nowadays,
> since it does not take the KVM CPUID leaves (0x400000xx) into
> account. On the other hand the callers of this function can all deal
> with the no-match situation. So lets remove this code, as it serves
> no purpose.
> This fixes a crash of newer Linux kernels as KVM guests on
> AMD Bulldozer CPUs, where bogus values were returned in response to
> a CPUID intercept.
>
>
> @@ -4959,12 +4959,6 @@ struct kvm_cpuid_entry2 *kvm_find_cpuid_entry(struct kvm_vcpu *vcpu,
>   			best = e;
>   			break;
>   		}
> -		/*
> -		 * Both basic or both extended?
> -		 */
> -		if (((e->function ^ function)&  0x80000000) == 0)
> -			if (!best || e->function>  best->function)
> -				best = e;
>   	}
>   	return best;
>   }


This behaviour is mandated by the spec (looking at the Intel one), 
though it is implemented incorrectly - should always return largest 
basic leaf, and ignore the kvm leaves.

I think the correct behaviour is:

    if (e->function < 10000 && (!best || e->function > best->function))
         best = e;

We probably need a find_exact_cpuid_entry() that returns NULL if it 
doesn't find a match, for internal use.

-- 
error compiling committee.c: too many arguments to function

Re: [PATCH 2/2] kvm/x86: remove unneeded substitute search for missing CPUID entries

[Avi Kivity]

On 03/30/2011 03:26 PM, Avi Kivity wrote:
>
> This behaviour is mandated by the spec (looking at the Intel one), 
> though it is implemented incorrectly - should always return largest 
> basic leaf, and ignore the kvm leaves.
>
> I think the correct behaviour is:
>
>    if (e->function < 10000 && (!best || e->function > best->function))
>         best = e;
>

Oh, and it should honor ecx.. what a great interface.

-- 
error compiling committee.c: too many arguments to function

Re: [PATCH 2/2] kvm/x86: remove unneeded substitute search for missing CPUID entries

[Andre Przywara]

Avi Kivity wrote:
> On 03/30/2011 03:01 PM, Andre Przywara wrote:
>> If KVM cannot find an exact match for a requested CPUID leaf, the
>> code will try to find the closest match instead of simply confessing
>> it's failure. The heuristic is on one hand wrong nowadays,
>> since it does not take the KVM CPUID leaves (0x400000xx) into
>> account. On the other hand the callers of this function can all deal
>> with the no-match situation. So lets remove this code, as it serves
>> no purpose.
>> This fixes a crash of newer Linux kernels as KVM guests on
>> AMD Bulldozer CPUs, where bogus values were returned in response to
>> a CPUID intercept.
>>
>>
>> @@ -4959,12 +4959,6 @@ struct kvm_cpuid_entry2 *kvm_find_cpuid_entry(struct kvm_vcpu *vcpu,
>>   			best = e;
>>   			break;
>>   		}
>> -		/*
>> -		 * Both basic or both extended?
>> -		 */
>> -		if (((e->function ^ function)&  0x80000000) == 0)
>> -			if (!best || e->function>  best->function)
>> -				best = e;
>>   	}
>>   	return best;
>>   }
> 
> 
> This behaviour is mandated by the spec (looking at the Intel one), 
> though it is implemented incorrectly - should always return largest 
> basic leaf, and ignore the kvm leaves.

But the spec says that this applies only if EAX is higher than the 
largest supported leaf. The code as is checks whether KVM has an entry 
in the cpuid "cache" for it, which is not the same. Especially this case 
that hit me was a missing index entry, which should return 0.
The check for too large leaf numbers should be moved into 
kvm_emulate_cpuid(). There is already some code in QEMU (cpu_x86_cpuid) 
to handle this, but that path does not apply to KVM.

I will make a new version of this patch which replaces the old check 
with a sane version in kvm_emulate_cpuid().

Thanks for pointing this out.

> 
> I think the correct behaviour is:
> 
>     if (e->function < 10000 && (!best || e->function > best->function))
>          best = e;
> 
> We probably need a find_exact_cpuid_entry() that returns NULL if it 
> doesn't find a match, for internal use.

As mentioned, this behavior only applies to the actual intercept case, 
not to all users of kvm_find_cpuid_entry(). So I'd like to make this 
check in the intercept code path and not in this function.

Regards,
Andre.

-- 
Andre Przywara
AMD-Operating System Research Center (OSRC), Dresden, Germany

Re: [PATCH 2/2] kvm/x86: remove unneeded substitute search for missing CPUID entries

[Avi Kivity]

On 03/31/2011 12:12 PM, Andre Przywara wrote:
> Avi Kivity wrote:
>> On 03/30/2011 03:01 PM, Andre Przywara wrote:
>>> If KVM cannot find an exact match for a requested CPUID leaf, the
>>> code will try to find the closest match instead of simply confessing
>>> it's failure. The heuristic is on one hand wrong nowadays,
>>> since it does not take the KVM CPUID leaves (0x400000xx) into
>>> account. On the other hand the callers of this function can all deal
>>> with the no-match situation. So lets remove this code, as it serves
>>> no purpose.
>>> This fixes a crash of newer Linux kernels as KVM guests on
>>> AMD Bulldozer CPUs, where bogus values were returned in response to
>>> a CPUID intercept.
>>>
>>>
>>> @@ -4959,12 +4959,6 @@ struct kvm_cpuid_entry2 
>>> *kvm_find_cpuid_entry(struct kvm_vcpu *vcpu,
>>>               best = e;
>>>               break;
>>>           }
>>> -        /*
>>> -         * Both basic or both extended?
>>> -         */
>>> -        if (((e->function ^ function)&  0x80000000) == 0)
>>> -            if (!best || e->function>  best->function)
>>> -                best = e;
>>>       }
>>>       return best;
>>>   }
>>
>>
>> This behaviour is mandated by the spec (looking at the Intel one), 
>> though it is implemented incorrectly - should always return largest 
>> basic leaf, and ignore the kvm leaves.
>
> But the spec says that this applies only if EAX is higher than the 
> largest supported leaf. The code as is checks whether KVM has an entry 
> in the cpuid "cache" for it, which is not the same. Especially this 
> case that hit me was a missing index entry, which should return 0.

Ah, I see.

> The check for too large leaf numbers should be moved into 
> kvm_emulate_cpuid(). There is already some code in QEMU 
> (cpu_x86_cpuid) to handle this, but that path does not apply to KVM.
>
> I will make a new version of this patch which replaces the old check 
> with a sane version in kvm_emulate_cpuid().
>
> Thanks for pointing this out.
>
>>
>> I think the correct behaviour is:
>>
>>     if (e->function < 10000 && (!best || e->function > best->function))
>>          best = e;
>>
>> We probably need a find_exact_cpuid_entry() that returns NULL if it 
>> doesn't find a match, for internal use.
>
> As mentioned, this behavior only applies to the actual intercept case, 
> not to all users of kvm_find_cpuid_entry(). So I'd like to make this 
> check in the intercept code path and not in this function.
>

Right.

-- 
error compiling committee.c: too many arguments to function

[PATCH 2/2] kvm/x86: move and fix substitue search for missing CPUID entries

[Andre Przywara]

If KVM cannot find an exact match for a requested CPUID leaf, the
code will try to find the closest match instead of simply confessing
it's failure.
The implementation was meant to satisfy the CPUID specification, but
did not properly check for extended and standard leaves and also
didn't account for the index subleaf.
Beside that this rule only applies to CPUID intercepts, which is not
the only user of the kvm_find_cpuid_entry() function.

So fix this algorithm and move it into kvm_emulate_cpuid().
This fixes a crash of newer Linux kernels as KVM guests on
AMD Bulldozer CPUs, where bogus values were returned in response to
a CPUID intercept.

CC: <stable@kernel.org> [2.6.38]
Signed-off-by: Andre Przywara <andre.przywara@amd.com>
---
 arch/x86/kvm/x86.c |   19 +++++++++++++------
 1 files changed, 13 insertions(+), 6 deletions(-)

diff --git a/arch/x86/kvm/x86.c b/arch/x86/kvm/x86.c
index 6e86cec..552b8f8 100644
--- a/arch/x86/kvm/x86.c
+++ b/arch/x86/kvm/x86.c
@@ -4959,12 +4959,6 @@ struct kvm_cpuid_entry2 *kvm_find_cpuid_entry(struct kvm_vcpu *vcpu,
 			best = e;
 			break;
 		}
-		/*
-		 * Both basic or both extended?
-		 */
-		if (((e->function ^ function) & 0x80000000) == 0)
-			if (!best || e->function > best->function)
-				best = e;
 	}
 	return best;
 }
@@ -4996,6 +4990,19 @@ void kvm_emulate_cpuid(struct kvm_vcpu *vcpu)
 	kvm_register_write(vcpu, VCPU_REGS_RCX, 0);
 	kvm_register_write(vcpu, VCPU_REGS_RDX, 0);
 	best = kvm_find_cpuid_entry(vcpu, function, index);
+
+	/* if no match is found, check whether we exceed the vCPU's limit
+	 * and return the content of the highest valid standard leaf instead.
+	 * This is to satisfy the CPUID specification.
+	 */
+	if (!best) {
+		best = kvm_find_cpuid_entry(vcpu, function & 0x80000000, 0);
+		if (best && best->eax < function)
+			best = kvm_find_cpuid_entry(vcpu, best->eax, index);
+		else
+			best = NULL;
+	}
+
 	if (best) {
 		kvm_register_write(vcpu, VCPU_REGS_RAX, best->eax);
 		kvm_register_write(vcpu, VCPU_REGS_RBX, best->ebx);
-- 
1.6.4

Re: [PATCH 2/2] kvm/x86: move and fix substitue search for missing CPUID entries

[Avi Kivity]

On 03/31/2011 03:13 PM, Andre Przywara wrote:
> If KVM cannot find an exact match for a requested CPUID leaf, the
> code will try to find the closest match instead of simply confessing
> it's failure.
> The implementation was meant to satisfy the CPUID specification, but
> did not properly check for extended and standard leaves and also
> didn't account for the index subleaf.
> Beside that this rule only applies to CPUID intercepts, which is not
> the only user of the kvm_find_cpuid_entry() function.
>
> So fix this algorithm and move it into kvm_emulate_cpuid().
> This fixes a crash of newer Linux kernels as KVM guests on
> AMD Bulldozer CPUs, where bogus values were returned in response to
> a CPUID intercept.
>
> @@ -4996,6 +4990,19 @@ void kvm_emulate_cpuid(struct kvm_vcpu *vcpu)
>   	kvm_register_write(vcpu, VCPU_REGS_RCX, 0);
>   	kvm_register_write(vcpu, VCPU_REGS_RDX, 0);
>   	best = kvm_find_cpuid_entry(vcpu, function, index);
> +
> +	/* if no match is found, check whether we exceed the vCPU's limit
> +	 * and return the content of the highest valid standard leaf instead.
> +	 * This is to satisfy the CPUID specification.
> +	 */
> +	if (!best) {
> +		best = kvm_find_cpuid_entry(vcpu, function&  0x80000000, 0);

"highest valid standard leaf" means the second argument should be zero, no?

> +		if (best&&  best->eax<  function)
> +			best = kvm_find_cpuid_entry(vcpu, best->eax, index);
> +		else
> +			best = NULL;
> +	}
> +
>   	if (best) {
>   		kvm_register_write(vcpu, VCPU_REGS_RAX, best->eax);
>   		kvm_register_write(vcpu, VCPU_REGS_RBX, best->ebx);


-- 
error compiling committee.c: too many arguments to function

_______________________________________________
stable mailing list
stable@linux.kernel.org
http://linux.kernel.org/mailman/listinfo/stable

Re: [PATCH 2/2] kvm/x86: move and fix substitue search for missing CPUID entries

[Andre Przywara]

Avi Kivity wrote:
> On 03/31/2011 03:13 PM, Andre Przywara wrote:
>> If KVM cannot find an exact match for a requested CPUID leaf, the
>> code will try to find the closest match instead of simply confessing
>> it's failure.
>> The implementation was meant to satisfy the CPUID specification, but
>> did not properly check for extended and standard leaves and also
>> didn't account for the index subleaf.
>> Beside that this rule only applies to CPUID intercepts, which is not
>> the only user of the kvm_find_cpuid_entry() function.
>>
>> So fix this algorithm and move it into kvm_emulate_cpuid().
>> This fixes a crash of newer Linux kernels as KVM guests on
>> AMD Bulldozer CPUs, where bogus values were returned in response to
>> a CPUID intercept.
>>
>> @@ -4996,6 +4990,19 @@ void kvm_emulate_cpuid(struct kvm_vcpu *vcpu)
>>   	kvm_register_write(vcpu, VCPU_REGS_RCX, 0);
>>   	kvm_register_write(vcpu, VCPU_REGS_RDX, 0);
>>   	best = kvm_find_cpuid_entry(vcpu, function, index);
>> +
>> +	/* if no match is found, check whether we exceed the vCPU's limit
>> +	 * and return the content of the highest valid standard leaf instead.
>> +	 * This is to satisfy the CPUID specification.
>> +	 */
>> +	if (!best) {
>> +		best = kvm_find_cpuid_entry(vcpu, function&  0x80000000, 0);
> 
> "highest valid standard leaf" means the second argument should be zero, no?

Weird, but somehow true. I fixed this is in a another version (following).

Thanks for spotting this.

Andre.

> 
>> +		if (best&&  best->eax<  function)
>> +			best = kvm_find_cpuid_entry(vcpu, best->eax, index);
>> +		else
>> +			best = NULL;
>> +	}
>> +
>>   	if (best) {
>>   		kvm_register_write(vcpu, VCPU_REGS_RAX, best->eax);
>>   		kvm_register_write(vcpu, VCPU_REGS_RBX, best->ebx);
> 
> 

-- 
Andre Przywara
AMD-Operating System Research Center (OSRC), Dresden, Germany

[PATCH 2/2 v3] kvm/x86: move and fix substitue search for missing CPUID entries

[Andre Przywara]

If KVM cannot find an exact match for a requested CPUID leaf, the
code will try to find the closest match instead of simply confessing
it's failure.
The implementation was meant to satisfy the CPUID specification, but
did not properly check for extended and standard leaves and also
didn't account for the index subleaf.
Beside that this rule only applies to CPUID intercepts, which is not
the only user of the kvm_find_cpuid_entry() function.

So fix this algorithm and call it from kvm_emulate_cpuid().
This fixes a crash of newer Linux kernels as KVM guests on
AMD Bulldozer CPUs, where bogus values were returned in response to
a CPUID intercept.

CC: <stable@kernel.org> [2.6.38]
Signed-off-by: Andre Przywara <andre.przywara@amd.com>
---
 arch/x86/kvm/x86.c |   31 +++++++++++++++++++++++++------
 1 files changed, 25 insertions(+), 6 deletions(-)

diff --git a/arch/x86/kvm/x86.c b/arch/x86/kvm/x86.c
index 6e86cec..a38fb9b 100644
--- a/arch/x86/kvm/x86.c
+++ b/arch/x86/kvm/x86.c
@@ -4959,12 +4959,6 @@ struct kvm_cpuid_entry2 *kvm_find_cpuid_entry(struct kvm_vcpu *vcpu,
 			best = e;
 			break;
 		}
-		/*
-		 * Both basic or both extended?
-		 */
-		if (((e->function ^ function) & 0x80000000) == 0)
-			if (!best || e->function > best->function)
-				best = e;
 	}
 	return best;
 }
@@ -4984,6 +4978,27 @@ not_found:
 	return 36;
 }
 
+/*
+ * If no match is found, check whether we exceed the vCPU's limit
+ * and return the content of the highest valid _standard_ leaf instead.
+ * This is to satisfy the CPUID specification.
+ */
+static struct kvm_cpuid_entry2* check_cpuid_limit(struct kvm_vcpu *vcpu,
+                                                  u32 function, u32 index)
+{
+	struct kvm_cpuid_entry2 *maxlevel;
+
+	maxlevel = kvm_find_cpuid_entry(vcpu, function & 0x80000000, 0);
+	if (!maxlevel || maxlevel->eax >= function)
+		return NULL;
+	if (function & 0x80000000) {
+		maxlevel = kvm_find_cpuid_entry(vcpu, 0, 0);
+		if (!maxlevel)
+			return NULL;
+	}
+	return kvm_find_cpuid_entry(vcpu, maxlevel->eax, index);
+}
+
 void kvm_emulate_cpuid(struct kvm_vcpu *vcpu)
 {
 	u32 function, index;
@@ -4996,6 +5011,10 @@ void kvm_emulate_cpuid(struct kvm_vcpu *vcpu)
 	kvm_register_write(vcpu, VCPU_REGS_RCX, 0);
 	kvm_register_write(vcpu, VCPU_REGS_RDX, 0);
 	best = kvm_find_cpuid_entry(vcpu, function, index);
+
+	if (!best)
+		best = check_cpuid_limit(vcpu, function, index);
+
 	if (best) {
 		kvm_register_write(vcpu, VCPU_REGS_RAX, best->eax);
 		kvm_register_write(vcpu, VCPU_REGS_RBX, best->ebx);
-- 
1.6.4

Re: [PATCH 2/2 v3] kvm/x86: move and fix substitue search for missing CPUID entries

[Avi Kivity]

On 03/31/2011 04:58 PM, Andre Przywara wrote:
> If KVM cannot find an exact match for a requested CPUID leaf, the
> code will try to find the closest match instead of simply confessing
> it's failure.
> The implementation was meant to satisfy the CPUID specification, but
> did not properly check for extended and standard leaves and also
> didn't account for the index subleaf.
> Beside that this rule only applies to CPUID intercepts, which is not
> the only user of the kvm_find_cpuid_entry() function.
>
> So fix this algorithm and call it from kvm_emulate_cpuid().
> This fixes a crash of newer Linux kernels as KVM guests on
> AMD Bulldozer CPUs, where bogus values were returned in response to
> a CPUID intercept.
>

Thanks, applied and queued both for 2.6.39.

-- 
error compiling committee.c: too many arguments to function