[PATCH 0/5] KVM: cpuid: cleanups, simplify multi-index CPUID leaves

[PATCH 0/5] KVM: cpuid: cleanups, simplify multi-index CPUID leaves

[Paolo Bonzini]

While reviewing the AVX512_BF16, Jing Liu and I noted that the handling of
CPUID leaf 7 is quite messy, and in general the handling of multi-index
CPUID leaves is confusing.  These patches clean the code to prepare
for adding CPUID leaf 7 subleaf 1.

Paolo Bonzini (5):
  KVM: cpuid: do_cpuid_ent works on a whole CPUID function
  KVM: cpuid: extract do_cpuid_7_mask and support multiple subleafs
  KVM: cpuid: set struct kvm_cpuid_entry2 flags in do_cpuid_1_ent
  KVM: cpuid: rename do_cpuid_1_ent
  KVM: cpuid: remove has_leaf_count from struct kvm_cpuid_param

 arch/x86/kvm/cpuid.c | 222 ++++++++++++++++++++++++-------------------
 1 file changed, 122 insertions(+), 100 deletions(-)

-- 
2.21.0

[PATCH 1/5] KVM: cpuid: do_cpuid_ent works on a whole CPUID function

[Paolo Bonzini]

Rename it as well as __do_cpuid_ent and __do_cpuid_ent_emulated to have
"func" in its name, and drop the index parameter which is always 0.

Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
---
 arch/x86/kvm/cpuid.c | 89 +++++++++++++++++++++-----------------------
 1 file changed, 42 insertions(+), 47 deletions(-)

diff --git a/arch/x86/kvm/cpuid.c b/arch/x86/kvm/cpuid.c
index 004cbd84c351..ddffc56c39b4 100644
--- a/arch/x86/kvm/cpuid.c
+++ b/arch/x86/kvm/cpuid.c
@@ -294,14 +294,19 @@ static void do_cpuid_1_ent(struct kvm_cpuid_entry2 *entry, u32 function,
 {
 	entry->function = function;
 	entry->index = index;
+	entry->flags = 0;
+
 	cpuid_count(entry->function, entry->index,
 		    &entry->eax, &entry->ebx, &entry->ecx, &entry->edx);
-	entry->flags = 0;
 }
 
-static int __do_cpuid_ent_emulated(struct kvm_cpuid_entry2 *entry,
-				   u32 func, u32 index, int *nent, int maxnent)
+static int __do_cpuid_func_emulated(struct kvm_cpuid_entry2 *entry,
+				    u32 func, int *nent, int maxnent)
 {
+	entry->function = func;
+	entry->index = 0;
+	entry->flags = 0;
+
 	switch (func) {
 	case 0:
 		entry->eax = 7;
@@ -313,21 +318,18 @@ static int __do_cpuid_ent_emulated(struct kvm_cpuid_entry2 *entry,
 		break;
 	case 7:
 		entry->flags |= KVM_CPUID_FLAG_SIGNIFCANT_INDEX;
-		if (index == 0)
-			entry->ecx = F(RDPID);
+		entry->eax = 0;
+		entry->ecx = F(RDPID);
 		++*nent;
 	default:
 		break;
 	}
 
-	entry->function = func;
-	entry->index = index;
-
 	return 0;
 }
 
-static inline int __do_cpuid_ent(struct kvm_cpuid_entry2 *entry, u32 function,
-				 u32 index, int *nent, int maxnent)
+static inline int __do_cpuid_func(struct kvm_cpuid_entry2 *entry, u32 function,
+				  int *nent, int maxnent)
 {
 	int r;
 	unsigned f_nx = is_efer_nx() ? F(NX) : 0;
@@ -431,7 +433,7 @@ static inline int __do_cpuid_ent(struct kvm_cpuid_entry2 *entry, u32 function,
 	if (*nent >= maxnent)
 		goto out;
 
-	do_cpuid_1_ent(entry, function, index);
+	do_cpuid_1_ent(entry, function, 0);
 	++*nent;
 
 	switch (function) {
@@ -496,34 +498,28 @@ static inline int __do_cpuid_ent(struct kvm_cpuid_entry2 *entry, u32 function,
 		break;
 	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;
-			f_la57 = entry->ecx & F(LA57);
-			cpuid_mask(&entry->ecx, CPUID_7_ECX);
-			/* Set LA57 based on hardware capability. */
-			entry->ecx |= f_la57;
-			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->eax = 0;
+		/* Mask ebx against host capability word 9 */
+		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;
+		f_la57 = entry->ecx & F(LA57);
+		cpuid_mask(&entry->ecx, CPUID_7_ECX);
+		/* Set LA57 based on hardware capability. */
+		entry->ecx |= f_la57;
+		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);
 		break;
 	}
 	case 9:
@@ -750,20 +746,19 @@ static inline int __do_cpuid_ent(struct kvm_cpuid_entry2 *entry, u32 function,
 	return r;
 }
 
-static int do_cpuid_ent(struct kvm_cpuid_entry2 *entry, u32 func,
-			u32 idx, int *nent, int maxnent, unsigned int type)
+static int do_cpuid_func(struct kvm_cpuid_entry2 *entry, u32 func,
+			 int *nent, int maxnent, unsigned int type)
 {
 	if (type == KVM_GET_EMULATED_CPUID)
-		return __do_cpuid_ent_emulated(entry, func, idx, nent, maxnent);
+		return __do_cpuid_func_emulated(entry, func, nent, maxnent);
 
-	return __do_cpuid_ent(entry, func, idx, nent, maxnent);
+	return __do_cpuid_func(entry, func, nent, maxnent);
 }
 
 #undef F
 
 struct kvm_cpuid_param {
 	u32 func;
-	u32 idx;
 	bool has_leaf_count;
 	bool (*qualifier)(const struct kvm_cpuid_param *param);
 };
@@ -836,8 +831,8 @@ int kvm_dev_ioctl_get_cpuid(struct kvm_cpuid2 *cpuid,
 		if (ent->qualifier && !ent->qualifier(ent))
 			continue;
 
-		r = do_cpuid_ent(&cpuid_entries[nent], ent->func, ent->idx,
-				&nent, cpuid->nent, type);
+		r = do_cpuid_func(&cpuid_entries[nent], ent->func,
+				  &nent, cpuid->nent, type);
 
 		if (r)
 			goto out_free;
@@ -847,8 +842,8 @@ 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,
-				     &nent, cpuid->nent, type);
+			r = do_cpuid_func(&cpuid_entries[nent], func,
+				          &nent, cpuid->nent, type);
 
 		if (r)
 			goto out_free;
-- 
2.21.0

Re: [PATCH 1/5] KVM: cpuid: do_cpuid_ent works on a whole CPUID function

[Jing Liu]

Hi Paolo,

On 7/4/2019 10:07 PM, Paolo Bonzini wrote:
> Rename it as well as __do_cpuid_ent and __do_cpuid_ent_emulated to have
> "func" in its name, and drop the index parameter which is always 0.
> 
> Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
> ---
>   arch/x86/kvm/cpuid.c | 89 +++++++++++++++++++++-----------------------
>   1 file changed, 42 insertions(+), 47 deletions(-)
> 
> diff --git a/arch/x86/kvm/cpuid.c b/arch/x86/kvm/cpuid.c
> index 004cbd84c351..ddffc56c39b4 100644
> --- a/arch/x86/kvm/cpuid.c
> +++ b/arch/x86/kvm/cpuid.c
> @@ -294,14 +294,19 @@ static void do_cpuid_1_ent(struct kvm_cpuid_entry2 *entry, u32 function,
>   {
>   	entry->function = function;
>   	entry->index = index;
> +	entry->flags = 0;
> +

I'm wondering if we need set entry->flags = 0 here?
entry->flags was initialized as zero when vzalloc.

>   	cpuid_count(entry->function, entry->index,
>   		    &entry->eax, &entry->ebx, &entry->ecx, &entry->edx);
> -	entry->flags = 0;
>   }
>   
> -static int __do_cpuid_ent_emulated(struct kvm_cpuid_entry2 *entry,
> -				   u32 func, u32 index, int *nent, int maxnent)
> +static int __do_cpuid_func_emulated(struct kvm_cpuid_entry2 *entry,
> +				    u32 func, int *nent, int maxnent)
>   {
> +	entry->function = func;
> +	entry->index = 0;
> +	entry->flags = 0;
> +

The same question for flags and index, because entry is allocated
by vzalloc.

>   	switch (func) {
>   	case 0:
>   		entry->eax = 7;
> @@ -313,21 +318,18 @@ static int __do_cpuid_ent_emulated(struct kvm_cpuid_entry2 *entry,
>   		break;
>   	case 7:
>   		entry->flags |= KVM_CPUID_FLAG_SIGNIFCANT_INDEX;
> -		if (index == 0)
> -			entry->ecx = F(RDPID);
> +		entry->eax = 0;
> +		entry->ecx = F(RDPID);
>   		++*nent;
>   	default:
>   		break;
>   	}
>   
> -	entry->function = func;
> -	entry->index = index;
> -
>   	return 0;
>   }
>   


Thanks,
Jing

[PATCH 2/5] KVM: cpuid: extract do_cpuid_7_mask and support multiple subleafs

[Paolo Bonzini]

CPUID function 7 has multiple subleafs.  Instead of having nested
switch statements, move the logic to filter supported features to
a separate function, and call it for each subleaf.

Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
---
	Here you would have something like entry->eax = min(entry->eax, 1)
	when adding subleaf 1.

 arch/x86/kvm/cpuid.c | 128 +++++++++++++++++++++++++++----------------
 1 file changed, 81 insertions(+), 47 deletions(-)

diff --git a/arch/x86/kvm/cpuid.c b/arch/x86/kvm/cpuid.c
index ddffc56c39b4..1c6b9a4a74de 100644
--- a/arch/x86/kvm/cpuid.c
+++ b/arch/x86/kvm/cpuid.c
@@ -328,6 +328,71 @@ static int __do_cpuid_func_emulated(struct kvm_cpuid_entry2 *entry,
 	return 0;
 }
 
+static inline void do_cpuid_7_mask(struct kvm_cpuid_entry2 *entry, int index)
+{
+	unsigned f_invpcid = kvm_x86_ops->invpcid_supported() ? F(INVPCID) : 0;
+	unsigned f_mpx = kvm_mpx_supported() ? F(MPX) : 0;
+	unsigned f_umip = kvm_x86_ops->umip_emulated() ? F(UMIP) : 0;
+	unsigned f_intel_pt = kvm_x86_ops->pt_supported() ? F(INTEL_PT) : 0;
+	unsigned f_la57;
+
+	/* cpuid 7.0.ebx */
+	const u32 kvm_cpuid_7_0_ebx_x86_features =
+		F(FSGSBASE) | F(BMI1) | F(HLE) | F(AVX2) | F(SMEP) |
+		F(BMI2) | F(ERMS) | f_invpcid | F(RTM) | f_mpx | F(RDSEED) |
+		F(ADX) | F(SMAP) | F(AVX512IFMA) | F(AVX512F) | F(AVX512PF) |
+		F(AVX512ER) | F(AVX512CD) | F(CLFLUSHOPT) | F(CLWB) | F(AVX512DQ) |
+		F(SHA_NI) | F(AVX512BW) | F(AVX512VL) | f_intel_pt;
+
+	/* cpuid 7.0.ecx*/
+	const u32 kvm_cpuid_7_0_ecx_x86_features =
+		F(AVX512VBMI) | F(LA57) | F(PKU) | 0 /*OSPKE*/ |
+		F(AVX512_VPOPCNTDQ) | F(UMIP) | F(AVX512_VBMI2) | F(GFNI) |
+		F(VAES) | F(VPCLMULQDQ) | F(AVX512_VNNI) | F(AVX512_BITALG) |
+		F(CLDEMOTE) | F(MOVDIRI) | F(MOVDIR64B);
+
+	/* cpuid 7.0.edx*/
+	const u32 kvm_cpuid_7_0_edx_x86_features =
+		F(AVX512_4VNNIW) | F(AVX512_4FMAPS) | F(SPEC_CTRL) |
+		F(SPEC_CTRL_SSBD) | F(ARCH_CAPABILITIES) | F(INTEL_STIBP) |
+		F(MD_CLEAR);
+
+	switch (index) {
+	case 0:
+		entry->eax = 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;
+		f_la57 = entry->ecx & F(LA57);
+		cpuid_mask(&entry->ecx, CPUID_7_ECX);
+		/* Set LA57 based on hardware capability. */
+		entry->ecx |= f_la57;
+		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);
+		break;
+	default:
+		WARN_ON_ONCE(1);
+		entry->eax = 0;
+		entry->ebx = 0;
+		entry->ecx = 0;
+		entry->edx = 0;
+		break;
+	}
+}
+
 static inline int __do_cpuid_func(struct kvm_cpuid_entry2 *entry, u32 function,
 				  int *nent, int maxnent)
 {
@@ -342,12 +407,8 @@ static inline int __do_cpuid_func(struct kvm_cpuid_entry2 *entry, u32 function,
 	unsigned f_lm = 0;
 #endif
 	unsigned f_rdtscp = kvm_x86_ops->rdtscp_supported() ? F(RDTSCP) : 0;
-	unsigned f_invpcid = kvm_x86_ops->invpcid_supported() ? F(INVPCID) : 0;
-	unsigned f_mpx = kvm_mpx_supported() ? F(MPX) : 0;
 	unsigned f_xsaves = kvm_x86_ops->xsaves_supported() ? F(XSAVES) : 0;
-	unsigned f_umip = kvm_x86_ops->umip_emulated() ? F(UMIP) : 0;
 	unsigned f_intel_pt = kvm_x86_ops->pt_supported() ? F(INTEL_PT) : 0;
-	unsigned f_la57 = 0;
 
 	/* cpuid 1.edx */
 	const u32 kvm_cpuid_1_edx_x86_features =
@@ -400,31 +461,10 @@ static inline int __do_cpuid_func(struct kvm_cpuid_entry2 *entry, u32 function,
 		F(ACE2) | F(ACE2_EN) | F(PHE) | F(PHE_EN) |
 		F(PMM) | F(PMM_EN);
 
-	/* cpuid 7.0.ebx */
-	const u32 kvm_cpuid_7_0_ebx_x86_features =
-		F(FSGSBASE) | F(BMI1) | F(HLE) | F(AVX2) | F(SMEP) |
-		F(BMI2) | F(ERMS) | f_invpcid | F(RTM) | f_mpx | F(RDSEED) |
-		F(ADX) | F(SMAP) | F(AVX512IFMA) | F(AVX512F) | F(AVX512PF) |
-		F(AVX512ER) | F(AVX512CD) | F(CLFLUSHOPT) | F(CLWB) | F(AVX512DQ) |
-		F(SHA_NI) | F(AVX512BW) | F(AVX512VL) | f_intel_pt;
-
 	/* cpuid 0xD.1.eax */
 	const u32 kvm_cpuid_D_1_eax_x86_features =
 		F(XSAVEOPT) | F(XSAVEC) | F(XGETBV1) | f_xsaves;
 
-	/* cpuid 7.0.ecx*/
-	const u32 kvm_cpuid_7_0_ecx_x86_features =
-		F(AVX512VBMI) | F(LA57) | F(PKU) | 0 /*OSPKE*/ |
-		F(AVX512_VPOPCNTDQ) | F(UMIP) | F(AVX512_VBMI2) | F(GFNI) |
-		F(VAES) | F(VPCLMULQDQ) | F(AVX512_VNNI) | F(AVX512_BITALG) |
-		F(CLDEMOTE) | F(MOVDIRI) | F(MOVDIR64B);
-
-	/* cpuid 7.0.edx*/
-	const u32 kvm_cpuid_7_0_edx_x86_features =
-		F(AVX512_4VNNIW) | F(AVX512_4FMAPS) | F(SPEC_CTRL) |
-		F(SPEC_CTRL_SSBD) | F(ARCH_CAPABILITIES) | F(INTEL_STIBP) |
-		F(MD_CLEAR);
-
 	/* all calls to cpuid_count() should be made on the same cpu */
 	get_cpu();
 
@@ -496,30 +536,24 @@ static inline int __do_cpuid_func(struct kvm_cpuid_entry2 *entry, u32 function,
 		entry->ecx = 0;
 		entry->edx = 0;
 		break;
+	/* function 7 has additional index. */
 	case 7: {
+		int i;
+
 		entry->flags |= KVM_CPUID_FLAG_SIGNIFCANT_INDEX;
-		entry->eax = 0;
-		/* Mask ebx against host capability word 9 */
-		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;
-		f_la57 = entry->ecx & F(LA57);
-		cpuid_mask(&entry->ecx, CPUID_7_ECX);
-		/* Set LA57 based on hardware capability. */
-		entry->ecx |= f_la57;
-		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 = 0; ; ) {
+			do_cpuid_7_mask(&entry[i], i);
+			if (i == entry->eax)
+				break;
+			if (*nent >= maxnent)
+				goto out;
+
+			++i;
+			do_cpuid_1_ent(&entry[i], function, i);
+			entry[i].flags |=
+			       KVM_CPUID_FLAG_SIGNIFCANT_INDEX;
+			++*nent;
+		}
 		break;
 	}
 	case 9:
-- 
2.21.0

Re: [PATCH 2/5] KVM: cpuid: extract do_cpuid_7_mask and support multiple subleafs

[Jing Liu]

Hi Paolo,

Thank you for refining the cpuid codes especially for case 7! It looks
much clear now!

On 7/4/2019 10:07 PM, Paolo Bonzini wrote:
> CPUID function 7 has multiple subleafs.  Instead of having nested
> switch statements, move the logic to filter supported features to
> a separate function, and call it for each subleaf.
> 
> Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
> ---
> 	Here you would have something like entry->eax = min(entry->eax, 1)
> 	when adding subleaf 1.
> 
>   arch/x86/kvm/cpuid.c | 128 +++++++++++++++++++++++++++----------------
>   1 file changed, 81 insertions(+), 47 deletions(-)
> 
[...]
> +
> +	switch (index) {
> +	case 0:
> +		entry->eax = 0;

Here, mark: when adding subleaf 1, change to
entry->eax = min(entry->eax, 1).

> +		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;
> +		f_la57 = entry->ecx & F(LA57);
> +		cpuid_mask(&entry->ecx, CPUID_7_ECX);
> +		/* Set LA57 based on hardware capability. */
> +		entry->ecx |= f_la57;
> +		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);
> +		break;
And when adding subleaf 1, plan to add codes,

case 1:
	entry->eax |= kvm_cpuid_7_1_eax_x86_features;
	entry->ebx = entry->ecx = entry->edx =0;
	break;

What do you think?

> +	default:
> +		WARN_ON_ONCE(1);
> +		entry->eax = 0;
> +		entry->ebx = 0;
> +		entry->ecx = 0;
> +		entry->edx = 0;
> +		break;
> +	}
> +}
> +
>   static inline int __do_cpuid_func(struct kvm_cpuid_entry2 *entry, u32 function,
>   				  int *nent, int maxnent)
[...]
> +	/* function 7 has additional index. */
>   	case 7: {
> +		int i;
> +
>   		entry->flags |= KVM_CPUID_FLAG_SIGNIFCANT_INDEX;
> -		entry->eax = 0;
> -		/* Mask ebx against host capability word 9 */
> -		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;
> -		f_la57 = entry->ecx & F(LA57);
> -		cpuid_mask(&entry->ecx, CPUID_7_ECX);
> -		/* Set LA57 based on hardware capability. */
> -		entry->ecx |= f_la57;
> -		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 = 0; ; ) {
> +			do_cpuid_7_mask(&entry[i], i);
> +			if (i == entry->eax)
> +				break;
> +			if (*nent >= maxnent)
> +				goto out;
> +
> +			++i;
> +			do_cpuid_1_ent(&entry[i], function, i);
> +			entry[i].flags |=
> +			       KVM_CPUID_FLAG_SIGNIFCANT_INDEX;
> +			++*nent;
> +		}

The new logic is great and adding subleaf support would be much easier!

>   		break;
>   	}
>   	case 9:
> 

Thanks,
Jing

Re: [PATCH 2/5] KVM: cpuid: extract do_cpuid_7_mask and support multiple subleafs

[Paolo Bonzini]

On 08/07/19 09:07, Jing Liu wrote:
>>
> And when adding subleaf 1, plan to add codes,
> 
> case 1:
>     entry->eax |= kvm_cpuid_7_1_eax_x86_features;
>     entry->ebx = entry->ecx = entry->edx =0;
>     break;
> 
> What do you think?

This should be "&=", not "|=".  Otherwise yes, that's the idea.

Paolo

Re: [PATCH 2/5] KVM: cpuid: extract do_cpuid_7_mask and support multiple subleafs

[Jing Liu]

On 7/10/2019 2:30 PM, Paolo Bonzini wrote:
> On 08/07/19 09:07, Jing Liu wrote:
>>>
>> And when adding subleaf 1, plan to add codes,
>>
>> case 1:
>>      entry->eax |= kvm_cpuid_7_1_eax_x86_features;
>>      entry->ebx = entry->ecx = entry->edx =0;
>>      break;
>>
>> What do you think?
> 
> This should be "&=", not "|=".  Otherwise yes, that's the idea.
> 

Yes! So let me send out the BFloat16 patch based on your patch set now
or you have merge plan soon?

Thanks,
Jing

> Paolo
> 

Re: [PATCH 2/5] KVM: cpuid: extract do_cpuid_7_mask and support multiple subleafs

[Paolo Bonzini]

On 10/07/19 09:32, Jing Liu wrote:
> 
> 
> On 7/10/2019 2:30 PM, Paolo Bonzini wrote:
>> On 08/07/19 09:07, Jing Liu wrote:
>>>>
>>> And when adding subleaf 1, plan to add codes,
>>>
>>> case 1:
>>>      entry->eax |= kvm_cpuid_7_1_eax_x86_features;
>>>      entry->ebx = entry->ecx = entry->edx =0;
>>>      break;
>>>
>>> What do you think?
>>
>> This should be "&=", not "|=".  Otherwise yes, that's the idea.
>>
> 
> Yes! So let me send out the BFloat16 patch based on your patch set now
> or you have merge plan soon?

Just send it . :)

Paolo

[PATCH 3/5] KVM: cpuid: set struct kvm_cpuid_entry2 flags in do_cpuid_1_ent

[Paolo Bonzini]

do_cpuid_1_ent is typically called in two places by __do_cpuid_func
for CPUID functions that have subleafs.  Both places have to set
the KVM_CPUID_FLAG_SIGNIFCANT_INDEX.  Set that flag, and
KVM_CPUID_FLAG_STATEFUL_FUNC as well, directly in do_cpuid_1_ent.

Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
---
 arch/x86/kvm/cpuid.c | 30 ++++++++++++++----------------
 1 file changed, 14 insertions(+), 16 deletions(-)

diff --git a/arch/x86/kvm/cpuid.c b/arch/x86/kvm/cpuid.c
index 1c6b9a4a74de..9ebc5ae7fa0e 100644
--- a/arch/x86/kvm/cpuid.c
+++ b/arch/x86/kvm/cpuid.c
@@ -298,6 +298,20 @@ static void do_cpuid_1_ent(struct kvm_cpuid_entry2 *entry, u32 function,
 
 	cpuid_count(entry->function, entry->index,
 		    &entry->eax, &entry->ebx, &entry->ecx, &entry->edx);
+
+	switch (function) {
+	case 2:
+		entry->flags |= KVM_CPUID_FLAG_STATEFUL_FUNC;
+		break;
+	case 4:
+	case 7:
+	case 0xb:
+	case 0xd:
+	case 0x14:
+	case 0x8000001d:
+		entry->flags |= KVM_CPUID_FLAG_SIGNIFCANT_INDEX;
+		break;
+	}
 }
 
 static int __do_cpuid_func_emulated(struct kvm_cpuid_entry2 *entry,
@@ -497,14 +511,12 @@ static inline int __do_cpuid_func(struct kvm_cpuid_entry2 *entry, u32 function,
 	case 2: {
 		int t, times = entry->eax & 0xff;
 
-		entry->flags |= KVM_CPUID_FLAG_STATEFUL_FUNC;
 		entry->flags |= KVM_CPUID_FLAG_STATE_READ_NEXT;
 		for (t = 1; t < times; ++t) {
 			if (*nent >= maxnent)
 				goto out;
 
 			do_cpuid_1_ent(&entry[t], function, 0);
-			entry[t].flags |= KVM_CPUID_FLAG_STATEFUL_FUNC;
 			++*nent;
 		}
 		break;
@@ -514,7 +526,6 @@ static inline int __do_cpuid_func(struct kvm_cpuid_entry2 *entry, u32 function,
 	case 0x8000001d: {
 		int i, cache_type;
 
-		entry->flags |= KVM_CPUID_FLAG_SIGNIFCANT_INDEX;
 		/* read more entries until cache_type is zero */
 		for (i = 1; ; ++i) {
 			if (*nent >= maxnent)
@@ -524,8 +535,6 @@ static inline int __do_cpuid_func(struct kvm_cpuid_entry2 *entry, u32 function,
 			if (!cache_type)
 				break;
 			do_cpuid_1_ent(&entry[i], function, i);
-			entry[i].flags |=
-			       KVM_CPUID_FLAG_SIGNIFCANT_INDEX;
 			++*nent;
 		}
 		break;
@@ -540,7 +549,6 @@ static inline int __do_cpuid_func(struct kvm_cpuid_entry2 *entry, u32 function,
 	case 7: {
 		int i;
 
-		entry->flags |= KVM_CPUID_FLAG_SIGNIFCANT_INDEX;
 		for (i = 0; ; ) {
 			do_cpuid_7_mask(&entry[i], i);
 			if (i == entry->eax)
@@ -550,8 +558,6 @@ static inline int __do_cpuid_func(struct kvm_cpuid_entry2 *entry, u32 function,
 
 			++i;
 			do_cpuid_1_ent(&entry[i], function, i);
-			entry[i].flags |=
-			       KVM_CPUID_FLAG_SIGNIFCANT_INDEX;
 			++*nent;
 		}
 		break;
@@ -595,7 +601,6 @@ static inline int __do_cpuid_func(struct kvm_cpuid_entry2 *entry, u32 function,
 	case 0xb: {
 		int i, level_type;
 
-		entry->flags |= KVM_CPUID_FLAG_SIGNIFCANT_INDEX;
 		/* read more entries until level_type is zero */
 		for (i = 1; ; ++i) {
 			if (*nent >= maxnent)
@@ -605,8 +610,6 @@ static inline int __do_cpuid_func(struct kvm_cpuid_entry2 *entry, u32 function,
 			if (!level_type)
 				break;
 			do_cpuid_1_ent(&entry[i], function, i);
-			entry[i].flags |=
-			       KVM_CPUID_FLAG_SIGNIFCANT_INDEX;
 			++*nent;
 		}
 		break;
@@ -619,7 +622,6 @@ static inline int __do_cpuid_func(struct kvm_cpuid_entry2 *entry, u32 function,
 		entry->ebx = xstate_required_size(supported, false);
 		entry->ecx = entry->ebx;
 		entry->edx &= supported >> 32;
-		entry->flags |= KVM_CPUID_FLAG_SIGNIFCANT_INDEX;
 		if (!supported)
 			break;
 
@@ -645,8 +647,6 @@ static inline int __do_cpuid_func(struct kvm_cpuid_entry2 *entry, u32 function,
 			}
 			entry[i].ecx = 0;
 			entry[i].edx = 0;
-			entry[i].flags |=
-			       KVM_CPUID_FLAG_SIGNIFCANT_INDEX;
 			++*nent;
 			++i;
 		}
@@ -659,12 +659,10 @@ static inline int __do_cpuid_func(struct kvm_cpuid_entry2 *entry, u32 function,
 		if (!f_intel_pt)
 			break;
 
-		entry->flags |= KVM_CPUID_FLAG_SIGNIFCANT_INDEX;
 		for (t = 1; t <= times; ++t) {
 			if (*nent >= maxnent)
 				goto out;
 			do_cpuid_1_ent(&entry[t], function, t);
-			entry[t].flags |= KVM_CPUID_FLAG_SIGNIFCANT_INDEX;
 			++*nent;
 		}
 		break;
-- 
2.21.0

[PATCH 4/5] KVM: cpuid: rename do_cpuid_1_ent

[Paolo Bonzini]

do_cpuid_1_ent does not do the entire processing for a CPUID entry, it
only retrieves the host's values.  Rename it to match reality.

Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
---
 arch/x86/kvm/cpuid.c | 16 ++++++++--------
 1 file changed, 8 insertions(+), 8 deletions(-)

diff --git a/arch/x86/kvm/cpuid.c b/arch/x86/kvm/cpuid.c
index 9ebc5ae7fa0e..d403695f2f3b 100644
--- a/arch/x86/kvm/cpuid.c
+++ b/arch/x86/kvm/cpuid.c
@@ -289,7 +289,7 @@ static void cpuid_mask(u32 *word, int wordnum)
 	*word &= boot_cpu_data.x86_capability[wordnum];
 }
 
-static void do_cpuid_1_ent(struct kvm_cpuid_entry2 *entry, u32 function,
+static void do_host_cpuid(struct kvm_cpuid_entry2 *entry, u32 function,
 			   u32 index)
 {
 	entry->function = function;
@@ -487,7 +487,7 @@ static inline int __do_cpuid_func(struct kvm_cpuid_entry2 *entry, u32 function,
 	if (*nent >= maxnent)
 		goto out;
 
-	do_cpuid_1_ent(entry, function, 0);
+	do_host_cpuid(entry, function, 0);
 	++*nent;
 
 	switch (function) {
@@ -516,7 +516,7 @@ static inline int __do_cpuid_func(struct kvm_cpuid_entry2 *entry, u32 function,
 			if (*nent >= maxnent)
 				goto out;
 
-			do_cpuid_1_ent(&entry[t], function, 0);
+			do_host_cpuid(&entry[t], function, 0);
 			++*nent;
 		}
 		break;
@@ -534,7 +534,7 @@ static inline int __do_cpuid_func(struct kvm_cpuid_entry2 *entry, u32 function,
 			cache_type = entry[i - 1].eax & 0x1f;
 			if (!cache_type)
 				break;
-			do_cpuid_1_ent(&entry[i], function, i);
+			do_host_cpuid(&entry[i], function, i);
 			++*nent;
 		}
 		break;
@@ -557,7 +557,7 @@ static inline int __do_cpuid_func(struct kvm_cpuid_entry2 *entry, u32 function,
 				goto out;
 
 			++i;
-			do_cpuid_1_ent(&entry[i], function, i);
+			do_host_cpuid(&entry[i], function, i);
 			++*nent;
 		}
 		break;
@@ -609,7 +609,7 @@ static inline int __do_cpuid_func(struct kvm_cpuid_entry2 *entry, u32 function,
 			level_type = entry[i - 1].ecx & 0xff00;
 			if (!level_type)
 				break;
-			do_cpuid_1_ent(&entry[i], function, i);
+			do_host_cpuid(&entry[i], function, i);
 			++*nent;
 		}
 		break;
@@ -630,7 +630,7 @@ static inline int __do_cpuid_func(struct kvm_cpuid_entry2 *entry, u32 function,
 			if (*nent >= maxnent)
 				goto out;
 
-			do_cpuid_1_ent(&entry[i], function, idx);
+			do_host_cpuid(&entry[i], function, idx);
 			if (idx == 1) {
 				entry[i].eax &= kvm_cpuid_D_1_eax_x86_features;
 				cpuid_mask(&entry[i].eax, CPUID_D_1_EAX);
@@ -662,7 +662,7 @@ static inline int __do_cpuid_func(struct kvm_cpuid_entry2 *entry, u32 function,
 		for (t = 1; t <= times; ++t) {
 			if (*nent >= maxnent)
 				goto out;
-			do_cpuid_1_ent(&entry[t], function, t);
+			do_host_cpuid(&entry[t], function, t);
 			++*nent;
 		}
 		break;
-- 
2.21.0

[PATCH 5/5] KVM: cpuid: remove has_leaf_count from struct kvm_cpuid_param

[Paolo Bonzini]

The has_leaf_count member was originally added for KVM's paravirtualization
CPUID leaves.  However, since then the leaf count _has_ been added to those
leaves as well, so we can drop that special case.

Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
---
 arch/x86/kvm/cpuid.c | 11 +++--------
 1 file changed, 3 insertions(+), 8 deletions(-)

diff --git a/arch/x86/kvm/cpuid.c b/arch/x86/kvm/cpuid.c
index d403695f2f3b..243613bf5978 100644
--- a/arch/x86/kvm/cpuid.c
+++ b/arch/x86/kvm/cpuid.c
@@ -791,7 +791,6 @@ static int do_cpuid_func(struct kvm_cpuid_entry2 *entry, u32 func,
 
 struct kvm_cpuid_param {
 	u32 func;
-	bool has_leaf_count;
 	bool (*qualifier)(const struct kvm_cpuid_param *param);
 };
 
@@ -835,11 +834,10 @@ int kvm_dev_ioctl_get_cpuid(struct kvm_cpuid2 *cpuid,
 	int limit, nent = 0, r = -E2BIG, i;
 	u32 func;
 	static const struct kvm_cpuid_param param[] = {
-		{ .func = 0, .has_leaf_count = true },
-		{ .func = 0x80000000, .has_leaf_count = true },
-		{ .func = 0xC0000000, .qualifier = is_centaur_cpu, .has_leaf_count = true },
+		{ .func = 0 },
+		{ .func = 0x80000000 },
+		{ .func = 0xC0000000, .qualifier = is_centaur_cpu },
 		{ .func = KVM_CPUID_SIGNATURE },
-		{ .func = KVM_CPUID_FEATURES },
 	};
 
 	if (cpuid->nent < 1)
@@ -869,9 +867,6 @@ int kvm_dev_ioctl_get_cpuid(struct kvm_cpuid2 *cpuid,
 		if (r)
 			goto out_free;
 
-		if (!ent->has_leaf_count)
-			continue;
-
 		limit = cpuid_entries[nent - 1].eax;
 		for (func = ent->func + 1; func <= limit && nent < cpuid->nent && r == 0; ++func)
 			r = do_cpuid_func(&cpuid_entries[nent], func,
-- 
2.21.0

Re: [PATCH 5/5] KVM: cpuid: remove has_leaf_count from struct kvm_cpuid_param

[Jing Liu]

Hi Paolo,

On 7/4/2019 10:07 PM, Paolo Bonzini wrote:
> The has_leaf_count member was originally added for KVM's paravirtualization
> CPUID leaves.  However, since then the leaf count _has_ been added to those
> leaves as well, so we can drop that special case.
> 
> Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
[...]
> @@ -835,11 +834,10 @@ int kvm_dev_ioctl_get_cpuid(struct kvm_cpuid2 *cpuid,
>   	int limit, nent = 0, r = -E2BIG, i;
>   	u32 func;
>   	static const struct kvm_cpuid_param param[] = {
> -		{ .func = 0, .has_leaf_count = true },
> -		{ .func = 0x80000000, .has_leaf_count = true },
> -		{ .func = 0xC0000000, .qualifier = is_centaur_cpu, .has_leaf_count = true },
> +		{ .func = 0 },
> +		{ .func = 0x80000000 },
> +		{ .func = 0xC0000000, .qualifier = is_centaur_cpu },

>   		{ .func = KVM_CPUID_SIGNATURE },
> -		{ .func = KVM_CPUID_FEATURES },

It seems the two func are introduced by 2b5e97e, as paravirtual cpuid.
But when searching KVM_CPUID_SIGNATURE, there seems no caller requesting
this cpuid. Meanwhile, I felt curious if KVM_CPUID_FEATURES is still in 
use but it seems kvm_update_cpuid() uses that. Not sure which spec 
introduces the latest pv cpuid.

Thanks,
Jing

[...]

Re: [PATCH 5/5] KVM: cpuid: remove has_leaf_count from struct kvm_cpuid_param

[Paolo Bonzini]

On 08/07/19 09:09, Jing Liu wrote:
> It seems the two func are introduced by 2b5e97e, as paravirtual cpuid.
> But when searching KVM_CPUID_SIGNATURE, there seems no caller requesting
> this cpuid. Meanwhile, I felt curious if KVM_CPUID_FEATURES is still in
> use but it seems kvm_update_cpuid() uses that. Not sure which spec
> introduces the latest pv cpuid.

Yes, KVM_CPUID_SIGNATURE is generally not very interesting for
userspace.  But KVM_CPUID_FEATURES is called here:

        for (w = 0; w < FEATURE_WORDS; w++) {
            /* Override only features that weren't set explicitly
             * by the user.
             */
            env->features[w] |=
                x86_cpu_get_supported_feature_word(w, cpu->migratable) &
                ~env->user_features[w] & \
                ~feature_word_info[w].no_autoenable_flags;
        }

Paolo