Patchwork kvm: x86: Remove useless index and parameter

login
register
mail settings
Submitter Christopherson, Sean J
Date Feb. 6, 2019, 3:55 p.m.
Message ID <20190206155550.GB9575@linux.intel.com>
Download mbox | patch
Permalink /patch/719653/
State New
Headers show

Comments

Christopherson, Sean J - Feb. 6, 2019, 3:55 p.m.
On Fri, Feb 01, 2019 at 07:14:16PM +0800, Jing Liu wrote:
> The kvm_cpuid_param structure is designed for getting cpu level, with ECX input
> is zero. While it is useless and wasteful to use a field in the structure for this.
> Just remove the field and use 0 directly.
> 
> Also, the parameter in function is_centaur_cpu() is also useless.
> 
> Number of a 64 bit build:
> 
>           text    data     bss     dec     hex filename
> before:   7529     112       0    7641    1dd9 ./arch/x86/kvm/cpuid.o
> after:    7428     112       0    7540    1d74 ./arch/x86/kvm/cpuid.o
> 
> Signed-off-by: Jing Liu <jing2.liu@linux.intel.com>
> ---
>  arch/x86/kvm/cpuid.c | 11 +++++------
>  1 file changed, 5 insertions(+), 6 deletions(-)
> 
> diff --git a/arch/x86/kvm/cpuid.c b/arch/x86/kvm/cpuid.c
> index bbffa6c..389aaf7 100644
> --- a/arch/x86/kvm/cpuid.c
> +++ b/arch/x86/kvm/cpuid.c
> @@ -741,12 +741,11 @@ static int do_cpuid_ent(struct kvm_cpuid_entry2 *entry, u32 func,
>  
>  struct kvm_cpuid_param {
>  	u32 func;
> -	u32 idx;
>  	bool has_leaf_count;
> -	bool (*qualifier)(const struct kvm_cpuid_param *param);
> +	bool (*qualifier)(void);
>  };
>  
> -static bool is_centaur_cpu(const struct kvm_cpuid_param *param)
> +static bool is_centaur_cpu(void)
>  {
>  	return boot_cpu_data.x86_vendor == X86_VENDOR_CENTAUR;
>  }
> @@ -811,10 +810,10 @@ int kvm_dev_ioctl_get_cpuid(struct kvm_cpuid2 *cpuid,
>  	for (i = 0; i < ARRAY_SIZE(param); i++) {
>  		const struct kvm_cpuid_param *ent = &param[i];
>  
> -		if (ent->qualifier && !ent->qualifier(ent))
> +		if (ent->qualifier && !ent->qualifier())
>  			continue;
>  
> -		r = do_cpuid_ent(&cpuid_entries[nent], ent->func, ent->idx,
> +		r = do_cpuid_ent(&cpuid_entries[nent], ent->func, 0,
>  				&nent, cpuid->nent, type);
>  
>  		if (r)
> @@ -825,7 +824,7 @@ int kvm_dev_ioctl_get_cpuid(struct kvm_cpuid2 *cpuid,
>  
>  		limit = cpuid_entries[nent - 1].eax;
>  		for (func = ent->func + 1; func <= limit && nent < cpuid->nent && r == 0; ++func)
> -			r = do_cpuid_ent(&cpuid_entries[nent], func, ent->idx,
> +			r = do_cpuid_ent(&cpuid_entries[nent], func, 0,

At this point would it make sense to drop @idx from do_cpuid_ent() and
its children?  __do_cpuid_ent() in particular basically assumes @idx is
zero, e.g. I've had this patch laying around for over a year:

From ecc637181c008d77051200bc24a16ae060d32b29 Mon Sep 17 00:00:00 2001
From: Sean Christopherson <sean.j.christopherson@intel.com>
Date: Tue, 25 Jul 2017 10:23:34 -0700
Subject: [PATCH] kvm: x86: add WARN_ON_ONCE(index!=0) in __do_cpuid_ent

Except for one outlier, function 7, all cases in __do_cpuid_ent and
its children assume that the index passed in is zero.  Furthermore,
the index is fully under KVM's control and all callers pass an index
of zero.  In other words, a non-zero index would indicate either a
bug in the caller or a new case that is expected to be handled.  WARN
and return an error on a non-zero index and remove the now unreachable
code in function 7 for handling a non-zero index.

Signed-off-by: Sean Christopherson <sean.j.christopherson@intel.com>
---
 arch/x86/kvm/cpuid.c | 51 ++++++++++++++++++++++----------------------
 1 file changed, 26 insertions(+), 25 deletions(-)
Jing Liu - Feb. 13, 2019, 7:07 a.m.
Hi Sean,

Thanks for your reviewing.
I agreed with your patch you posted below. See my comments inline.

On 2/6/2019 11:55 PM, Sean Christopherson wrote:
> On Fri, Feb 01, 2019 at 07:14:16PM +0800, Jing Liu wrote:
>> The kvm_cpuid_param structure is designed for getting cpu level, with ECX input
>> is zero. While it is useless and wasteful to use a field in the structure for this.
>> Just remove the field and use 0 directly.
>>
>> Also, the parameter in function is_centaur_cpu() is also useless.
>>
>> Number of a 64 bit build:
>>
>>            text    data     bss     dec     hex filename
>> before:   7529     112       0    7641    1dd9 ./arch/x86/kvm/cpuid.o
>> after:    7428     112       0    7540    1d74 ./arch/x86/kvm/cpuid.o
>>
>> Signed-off-by: Jing Liu <jing2.liu@linux.intel.com>
>> ---
>>   arch/x86/kvm/cpuid.c | 11 +++++------
>>   1 file changed, 5 insertions(+), 6 deletions(-)
>>
>> diff --git a/arch/x86/kvm/cpuid.c b/arch/x86/kvm/cpuid.c
>> index bbffa6c..389aaf7 100644
>> --- a/arch/x86/kvm/cpuid.c
>> +++ b/arch/x86/kvm/cpuid.c
>> @@ -741,12 +741,11 @@ static int do_cpuid_ent(struct kvm_cpuid_entry2 *entry, u32 func,
>>   
>>   struct kvm_cpuid_param {
>>   	u32 func;
>> -	u32 idx;
>>   	bool has_leaf_count;
>> -	bool (*qualifier)(const struct kvm_cpuid_param *param);
>> +	bool (*qualifier)(void);
>>   };
>>   
>> -static bool is_centaur_cpu(const struct kvm_cpuid_param *param)
>> +static bool is_centaur_cpu(void)
>>   {
>>   	return boot_cpu_data.x86_vendor == X86_VENDOR_CENTAUR;
>>   }
>> @@ -811,10 +810,10 @@ int kvm_dev_ioctl_get_cpuid(struct kvm_cpuid2 *cpuid,
>>   	for (i = 0; i < ARRAY_SIZE(param); i++) {
>>   		const struct kvm_cpuid_param *ent = &param[i];
>>   
>> -		if (ent->qualifier && !ent->qualifier(ent))
>> +		if (ent->qualifier && !ent->qualifier())
>>   			continue;
>>   
>> -		r = do_cpuid_ent(&cpuid_entries[nent], ent->func, ent->idx,
>> +		r = do_cpuid_ent(&cpuid_entries[nent], ent->func, 0,
>>   				&nent, cpuid->nent, type);
>>   
>>   		if (r)
>> @@ -825,7 +824,7 @@ int kvm_dev_ioctl_get_cpuid(struct kvm_cpuid2 *cpuid,
>>   
>>   		limit = cpuid_entries[nent - 1].eax;
>>   		for (func = ent->func + 1; func <= limit && nent < cpuid->nent && r == 0; ++func)
>> -			r = do_cpuid_ent(&cpuid_entries[nent], func, ent->idx,
>> +			r = do_cpuid_ent(&cpuid_entries[nent], func, 0,
> 
> At this point would it make sense to drop @idx from do_cpuid_ent() and
> its children?  
I agree.

__do_cpuid_ent() in particular basically assumes @idx is
> zero, e.g. I've had this patch laying around for over a year:
> 
>  From ecc637181c008d77051200bc24a16ae060d32b29 Mon Sep 17 00:00:00 2001
> From: Sean Christopherson <sean.j.christopherson@intel.com>
> Date: Tue, 25 Jul 2017 10:23:34 -0700
> Subject: [PATCH] kvm: x86: add WARN_ON_ONCE(index!=0) in __do_cpuid_ent
> 
> Except for one outlier, function 7, all cases in __do_cpuid_ent and
> its children assume that the index passed in is zero.  Furthermore,
> the index is fully under KVM's control and all callers pass an index
> of zero.  In other words, a non-zero index would indicate either a
> bug in the caller or a new case that is expected to be handled. 

So far, the @idx of do_cpuid_ent is really useless. It doesn't stand for
subleaf, if we see e.g. function 0x14, it is obvious. I even wonder why 
we need this @idx... It is confusing.
For function 7, I agree that don't use @index, but newly defining @times 
as a local
variable, to stand for a subleaf that doesn't support, as spec says. I
post the codes as follows inline your patch.


  WARN
> and return an error on a non-zero index and remove the now unreachable
> code in function 7 for handling a non-zero index.
> 
> Signed-off-by: Sean Christopherson <sean.j.christopherson@intel.com>
> ---
>   arch/x86/kvm/cpuid.c | 51 ++++++++++++++++++++++----------------------
>   1 file changed, 26 insertions(+), 25 deletions(-)
> 
> diff --git a/arch/x86/kvm/cpuid.c b/arch/x86/kvm/cpuid.c
> index 7bcfa61375c0..b643f5605633 100644
> --- a/arch/x86/kvm/cpuid.c
> +++ b/arch/x86/kvm/cpuid.c
> @@ -413,6 +413,14 @@ static inline int __do_cpuid_ent(struct kvm_cpuid_entry2 *entry, u32 function,
>   		F(AVX512_4VNNIW) | F(AVX512_4FMAPS) | F(SPEC_CTRL) |
>   		F(SPEC_CTRL_SSBD) | F(ARCH_CAPABILITIES);
>   
> +	/*
> +	 * The code below assumes index == 0, which simplifies handling leafs
> +	 * with a dynamic number of sub-leafs.  The index is fully under KVM's
> +	 * control, i.e. a non-zero value is a bug.
> +	 */
> +	if (WARN_ON_ONCE(index != 0))
> +		return -EINVAL;
> +
>   	/* all calls to cpuid_count() should be made on the same cpu */
>   	get_cpu();
>   
> @@ -482,35 +490,28 @@ static inline int __do_cpuid_ent(struct kvm_cpuid_entry2 *entry, u32 function,
>   		entry->ecx = 0;
>   		entry->edx = 0;
>   		break;
> -	case 7: {
> +	case 7:
>   		entry->flags |= KVM_CPUID_FLAG_SIGNIFCANT_INDEX;
>   		/* Mask ebx against host capability word 9 */
> -		if (index == 0) {
> -			entry->ebx &= kvm_cpuid_7_0_ebx_x86_features;
> -			cpuid_mask(&entry->ebx, CPUID_7_0_EBX);
> -			// TSC_ADJUST is emulated
> -			entry->ebx |= F(TSC_ADJUST);
> -			entry->ecx &= kvm_cpuid_7_0_ecx_x86_features;
> -			cpuid_mask(&entry->ecx, CPUID_7_ECX);
> -			entry->ecx |= f_umip;
> -			/* PKU is not yet implemented for shadow paging. */
> -			if (!tdp_enabled || !boot_cpu_has(X86_FEATURE_OSPKE))
> -				entry->ecx &= ~F(PKU);
> -			entry->edx &= kvm_cpuid_7_0_edx_x86_features;
> -			cpuid_mask(&entry->edx, CPUID_7_EDX);
> -			/*
> -			 * We emulate ARCH_CAPABILITIES in software even
> -			 * if the host doesn't support it.
> -			 */
> -			entry->edx |= F(ARCH_CAPABILITIES);
> -		} else {
> -			entry->ebx = 0;
> -			entry->ecx = 0;
> -			entry->edx = 0;
> -		}
+               int i, times = entry->eax;


> +		entry->ebx &= kvm_cpuid_7_0_ebx_x86_features;
> +		cpuid_mask(&entry->ebx, CPUID_7_0_EBX);
> +		// TSC_ADJUST is emulated
> +		entry->ebx |= F(TSC_ADJUST);
> +		entry->ecx &= kvm_cpuid_7_0_ecx_x86_features;
> +		cpuid_mask(&entry->ecx, CPUID_7_ECX);
> +		entry->ecx |= f_umip;
> +		/* PKU is not yet implemented for shadow paging. */
> +		if (!tdp_enabled || !boot_cpu_has(X86_FEATURE_OSPKE))
> +			entry->ecx &= ~F(PKU);
> +		entry->edx &= kvm_cpuid_7_0_edx_x86_features;
> +		cpuid_mask(&entry->edx, CPUID_7_EDX);
> +		/*
> +		 * We emulate ARCH_CAPABILITIES in software even
> +		 * if the host doesn't support it.
> +		 */
> +		entry->edx |= F(ARCH_CAPABILITIES);

+               for (i = 1; i <= times; i++) {
+                       if (*nent >= maxnent)
+                               goto out;
+                       do_cpuid_1_ent(&entry[i], function, i);
+                       entry[i].ebx = 0;
+                       entry[i].ecx = 0;
+                       entry[i].edx = 0;
+                       entry[i].flags |= KVM_CPUID_FLAG_SIGNIFCANT_INDEX;
+                       ++*nent;
+               }

>   		entry->eax = 0;
>   		break;
> -	}
>   	case 9:
>   		break;
>   	case 0xa: { /* Architectural Performance Monitoring */
>

Patch

diff --git a/arch/x86/kvm/cpuid.c b/arch/x86/kvm/cpuid.c
index 7bcfa61375c0..b643f5605633 100644
--- a/arch/x86/kvm/cpuid.c
+++ b/arch/x86/kvm/cpuid.c
@@ -413,6 +413,14 @@  static inline int __do_cpuid_ent(struct kvm_cpuid_entry2 *entry, u32 function,
 		F(AVX512_4VNNIW) | F(AVX512_4FMAPS) | F(SPEC_CTRL) |
 		F(SPEC_CTRL_SSBD) | F(ARCH_CAPABILITIES);
 
+	/*
+	 * The code below assumes index == 0, which simplifies handling leafs
+	 * with a dynamic number of sub-leafs.  The index is fully under KVM's
+	 * control, i.e. a non-zero value is a bug.
+	 */
+	if (WARN_ON_ONCE(index != 0))
+		return -EINVAL;
+
 	/* all calls to cpuid_count() should be made on the same cpu */
 	get_cpu();
 
@@ -482,35 +490,28 @@  static inline int __do_cpuid_ent(struct kvm_cpuid_entry2 *entry, u32 function,
 		entry->ecx = 0;
 		entry->edx = 0;
 		break;
-	case 7: {
+	case 7:
 		entry->flags |= KVM_CPUID_FLAG_SIGNIFCANT_INDEX;
 		/* Mask ebx against host capability word 9 */
-		if (index == 0) {
-			entry->ebx &= kvm_cpuid_7_0_ebx_x86_features;
-			cpuid_mask(&entry->ebx, CPUID_7_0_EBX);
-			// TSC_ADJUST is emulated
-			entry->ebx |= F(TSC_ADJUST);
-			entry->ecx &= kvm_cpuid_7_0_ecx_x86_features;
-			cpuid_mask(&entry->ecx, CPUID_7_ECX);
-			entry->ecx |= f_umip;
-			/* PKU is not yet implemented for shadow paging. */
-			if (!tdp_enabled || !boot_cpu_has(X86_FEATURE_OSPKE))
-				entry->ecx &= ~F(PKU);
-			entry->edx &= kvm_cpuid_7_0_edx_x86_features;
-			cpuid_mask(&entry->edx, CPUID_7_EDX);
-			/*
-			 * We emulate ARCH_CAPABILITIES in software even
-			 * if the host doesn't support it.
-			 */
-			entry->edx |= F(ARCH_CAPABILITIES);
-		} else {
-			entry->ebx = 0;
-			entry->ecx = 0;
-			entry->edx = 0;
-		}
+		entry->ebx &= kvm_cpuid_7_0_ebx_x86_features;
+		cpuid_mask(&entry->ebx, CPUID_7_0_EBX);
+		// TSC_ADJUST is emulated
+		entry->ebx |= F(TSC_ADJUST);
+		entry->ecx &= kvm_cpuid_7_0_ecx_x86_features;
+		cpuid_mask(&entry->ecx, CPUID_7_ECX);
+		entry->ecx |= f_umip;
+		/* PKU is not yet implemented for shadow paging. */
+		if (!tdp_enabled || !boot_cpu_has(X86_FEATURE_OSPKE))
+			entry->ecx &= ~F(PKU);
+		entry->edx &= kvm_cpuid_7_0_edx_x86_features;
+		cpuid_mask(&entry->edx, CPUID_7_EDX);
+		/*
+		 * We emulate ARCH_CAPABILITIES in software even
+		 * if the host doesn't support it.
+		 */
+		entry->edx |= F(ARCH_CAPABILITIES);
 		entry->eax = 0;
 		break;
-	}
 	case 9:
 		break;
 	case 0xa: { /* Architectural Performance Monitoring */