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 <pbonz...@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

Reply via email to