[PATCH 1/1] arch/x86/kvm/cpuid.c: cpuid_maxphyaddr "bad" handling.

[PATCH 1/1] arch/x86/kvm/cpuid.c: cpuid_maxphyaddr "bad" handling.

[Raphael S Carvalho]

Well, the below function reports the physical-address width supported
by the processor.
It does its work very well, though I found a detail which it doesn't
handle at all.

PS: The following function is not a patch.
int cpuid_maxphyaddr(struct kvm_vcpu *vcpu)
{
	struct kvm_cpuid_entry2 *best;

	best = kvm_find_cpuid_entry(vcpu, 0x80000000, 0);
	if (!best || best->eax < 0x80000008)
		goto not_found;
	best = kvm_find_cpuid_entry(vcpu, 0x80000008, 0);
	if (best)
		return best->eax & 0xff;
not_found:
	return 36;
}

As I'm seeing, its(above function) first step is to check whether the
CPU provides the CPUID function 80000008H,
if so, it gets the physical-address width from the available CPUID
function, otherwise it implicitly returns 36.

Intel manual says the following:
"For processors that do not support CPUID function 80000008H, the
width is generally 36 if CPUID.01H:EDX.PAE [bit 6] = 1
and 32 otherwise."
According to the above-mentioned statement, we would have to return 32
whether PAE is not supported by the CPU.
So I was wondering if such a function would work efficiently on
processors that do not support PAE extension.

arch/x86/include/asm/processor.h does provide a generic cpuid function
called native_cpuid, however, I added another procedure for
simplicity/efficiency purposes. Besides, I didn't find where the Linux
kernel defines such flags (PAE flag (1 << 6)). For avoiding duplicated
definitions, it would be a pleasure to get my code modified.
I also would like to share that MAXPHYADDR can be at most 52. However,
not sure if such a implementation is even needed.

Signed-off-by: Raphael S.Carvalho <raphael.scarv@gmail.com>

--- cpuid2.c	2012-11-29 22:44:43.881876294 -0200
+++ cpuid.c	2012-11-30 00:02:08.565710892 -0200
@@ -606,6 +606,30 @@
 }
 EXPORT_SYMBOL_GPL(kvm_find_cpuid_entry);

+#ifndef PAE_BIT
+#define PAE_BIT (1ULL << 6)
+#endif
+static inline unsigned cpuid_cpu_pae_support(void)
+{
+	unsigned int __edx;
+	const unsigned int cpu_id_param = 0x01;
+
+	/* According to Intel Manual we can check
+	 * whether the processor does provide PAE by
+	 * using the CPUID instruction.
+	 * Syntax: CPUID.01H:EDX.PAE [bit 6] = 1
+	 */
+	__edx = 0;
+	asm volatile(
+		"cpuid"
+		: "=d"(__edx)
+		: "a"(cpu_id_param)
+		: "ecx","ebx"
+		);
+
+	return (__edx & PAE_BIT);
+}
+
 int cpuid_maxphyaddr(struct kvm_vcpu *vcpu)
 {
 	struct kvm_cpuid_entry2 *best;
@@ -617,7 +641,10 @@
 	if (best)
 		return best->eax & 0xff;
 not_found:
-	return 36;
+	/* Check whether CPU supports PAE, if so the MAXPHYADDR
+	 * is 36, otherwise 32.
+	 */
+	return (cpuid_cpu_pae_support()) ? 36 : 32;
 }

[PATCH 1/1] arch/x86/kvm/cpuid.c: cpuid_maxphyaddr "bad" handling.

[Raphael S Carvalho]

Well, the below function reports the physical-address width supported
by the processor.
It does its work very well, though I found a detail which it doesn't
handle at all.

PS: The following function is not a patch.
int cpuid_maxphyaddr(struct kvm_vcpu *vcpu)
{
        struct kvm_cpuid_entry2 *best;

        best = kvm_find_cpuid_entry(vcpu, 0x80000000, 0);
        if (!best || best->eax < 0x80000008)
                goto not_found;
        best = kvm_find_cpuid_entry(vcpu, 0x80000008, 0);
        if (best)
                return best->eax & 0xff;
not_found:
        return 36;
}

As I'm seeing, its(above function) first step is to check whether the
CPU provides the CPUID function 80000008H,
if so, it gets the physical-address width from the available CPUID
function, otherwise it implicitly returns 36.

Intel manual says the following:
"For processors that do not support CPUID function 80000008H, the
width is generally 36 if CPUID.01H:EDX.PAE [bit 6] = 1
and 32 otherwise."
According to the above-mentioned statement, we would have to return 32
whether PAE is not supported by the CPU.
So I was wondering if such a function would work efficiently on
processors that do not support PAE extension.
I also would like to share that MAXPHYADDR can be at most 52. However,
not sure if such an implementation is even needed.

NOTE:
arch/x86/include/asm/cpufeature.h provides the below macro.
#define cpu_has_pae             boot_cpu_has(X86_FEATURE_PAE)
Does the above macro return 1 whether CPU does support the PAE feature?
If so, we would have to make a single change in the code:

Signed-off-by: Raphael S.Carvalho <raphael.scarv@gmail.com>

--- a/arch/x86/kvm/cpuid.c	2012-11-19 23:44:29.000000000 -0200
+++ b/arch/x86/kvm/cpuid.c	2012-11-30 15:05:51.000000000 -0200
@@ -617,7 +617,10 @@
 	if (best)
 		return best->eax & 0xff;
 not_found:
-	return 36;
+	/* Check whether CPU supports PAE, if so the MAXPHYADDR
+	 * is 36, otherwise 32.
+	 */
+	return (cpu_has_pae) ? 36 : 32;
 }

Follows a different patch otherwise:
arch/x86/include/asm/processor.h does provide a generic cpuid function
called native_cpuid, however, I added another procedure for
simplicity/efficiency purposes.

Signed-off-by: Raphael S.Carvalho <raphael.scarv@gmail.com>

--- a/arch/x86/kvm/cpuid.c	2012-11-19 23:44:29.000000000 -0200
+++ b/arch/x86/kvm/cpuid.c	2012-11-30 14:28:22.000000000 -0200
@@ -606,6 +606,30 @@

+#ifndef PAE_BIT
+#define PAE_BIT (1ULL << 6)
+#endif
+static inline unsigned cpuid_cpu_pae_support(void)
+{
+	unsigned int __edx;
+	const unsigned int cpu_id_param = 0x01;
+
+	/* According to Intel Manual we can check
+	 * whether the processor does provide PAE by
+	 * using the CPUID instruction.
+	 * Syntax: CPUID.01H:EDX.PAE [bit 6] = 1
+	 */
+	__edx = 0;
+	asm volatile(
+		"cpuid"
+		: "=d"(__edx)
+		: "a"(cpu_id_param)
+		: "ecx","ebx"
+		);
+
+	return (__edx & PAE_BIT);
+}
+
 int cpuid_maxphyaddr(struct kvm_vcpu *vcpu)
 {
 	struct kvm_cpuid_entry2 *best;
@@ -617,7 +641,10 @@
 	if (best)
 		return best->eax & 0xff;
 not_found:
-	return 36;
+	/* Check whether CPU supports PAE, if so the MAXPHYADDR
+	 * is 36, otherwise 32.
+	 */
+	return (cpuid_cpu_pae_support()) ? 36 : 32;
 }

Regards,
Raphael S.Carvalho <raphael.scarv@gmail.com>

Re: [PATCH 1/1] arch/x86/kvm/cpuid.c: cpuid_maxphyaddr "bad" handling.

[Gleb Natapov]

On Fri, Nov 30, 2012 at 03:20:27PM -0200, Raphael S Carvalho wrote:
> Well, the below function reports the physical-address width supported
> by the processor.
> It does its work very well, though I found a detail which it doesn't
> handle at all.
> 
> PS: The following function is not a patch.
> int cpuid_maxphyaddr(struct kvm_vcpu *vcpu)
> {
>         struct kvm_cpuid_entry2 *best;
> 
>         best = kvm_find_cpuid_entry(vcpu, 0x80000000, 0);
>         if (!best || best->eax < 0x80000008)
>                 goto not_found;
>         best = kvm_find_cpuid_entry(vcpu, 0x80000008, 0);
>         if (best)
>                 return best->eax & 0xff;
> not_found:
>         return 36;
> }
> 
> As I'm seeing, its(above function) first step is to check whether the
> CPU provides the CPUID function 80000008H,
> if so, it gets the physical-address width from the available CPUID
> function, otherwise it implicitly returns 36.
> 
> Intel manual says the following:
> "For processors that do not support CPUID function 80000008H, the
> width is generally 36 if CPUID.01H:EDX.PAE [bit 6] = 1
> and 32 otherwise."
> According to the above-mentioned statement, we would have to return 32
> whether PAE is not supported by the CPU.
> So I was wondering if such a function would work efficiently on
> processors that do not support PAE extension.
> I also would like to share that MAXPHYADDR can be at most 52. However,
> not sure if such an implementation is even needed.
> 
> NOTE:
> arch/x86/include/asm/cpufeature.h provides the below macro.
> #define cpu_has_pae             boot_cpu_has(X86_FEATURE_PAE)
> Does the above macro return 1 whether CPU does support the PAE feature?
> If so, we would have to make a single change in the code:
> 
> Signed-off-by: Raphael S.Carvalho <raphael.scarv@gmail.com>
> 
> --- a/arch/x86/kvm/cpuid.c	2012-11-19 23:44:29.000000000 -0200
> +++ b/arch/x86/kvm/cpuid.c	2012-11-30 15:05:51.000000000 -0200
> @@ -617,7 +617,10 @@
>  	if (best)
>  		return best->eax & 0xff;
>  not_found:
> -	return 36;
> +	/* Check whether CPU supports PAE, if so the MAXPHYADDR
> +	 * is 36, otherwise 32.
> +	 */
> +	return (cpu_has_pae) ? 36 : 32;
>  }
> 
> Follows a different patch otherwise:
> arch/x86/include/asm/processor.h does provide a generic cpuid function
> called native_cpuid, however, I added another procedure for
> simplicity/efficiency purposes.
> 
> Signed-off-by: Raphael S.Carvalho <raphael.scarv@gmail.com>
> 
> --- a/arch/x86/kvm/cpuid.c	2012-11-19 23:44:29.000000000 -0200
> +++ b/arch/x86/kvm/cpuid.c	2012-11-30 14:28:22.000000000 -0200
> @@ -606,6 +606,30 @@
> 
> +#ifndef PAE_BIT
> +#define PAE_BIT (1ULL << 6)
> +#endif
> +static inline unsigned cpuid_cpu_pae_support(void)
> +{
> +	unsigned int __edx;
> +	const unsigned int cpu_id_param = 0x01;
> +
> +	/* According to Intel Manual we can check
> +	 * whether the processor does provide PAE by
> +	 * using the CPUID instruction.
> +	 * Syntax: CPUID.01H:EDX.PAE [bit 6] = 1
> +	 */
> +	__edx = 0;
> +	asm volatile(
> +		"cpuid"
> +		: "=d"(__edx)
> +		: "a"(cpu_id_param)
> +		: "ecx","ebx"
> +		);
> +
You need to check cpuid as seen by a guest not a host. Something
like this:

struct kvm_cpuid_entry2 *feat = kvm_find_cpuid_entry(vcpu, 1, 0)
if (feat && (feat->edx & PAE_BIT))
    return 36;
else
    return 32;

But currently KVM code assumes that PAE is always present. kvm_set_cr4()
allows PAE to be set without doing the check above.

> +	return (__edx & PAE_BIT);
> +}
> +
>  int cpuid_maxphyaddr(struct kvm_vcpu *vcpu)
>  {
>  	struct kvm_cpuid_entry2 *best;
> @@ -617,7 +641,10 @@
>  	if (best)
>  		return best->eax & 0xff;
>  not_found:
> -	return 36;
> +	/* Check whether CPU supports PAE, if so the MAXPHYADDR
> +	 * is 36, otherwise 32.
> +	 */
> +	return (cpuid_cpu_pae_support()) ? 36 : 32;
>  }
> 
> Regards,
> Raphael S.Carvalho <raphael.scarv@gmail.com>
> --
> To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html
> Please read the FAQ at  http://www.tux.org/lkml/

--
			Gleb.