Re: [PATCH 1/2] i386/cpu: Add the Intel PT capabilities checking before extend the CPUID level

[Eduardo Habkost]

Hi,

Sorry for the long delay in reviewing this.  Now that 5.2 is
about to be released, we can try to merge this.

Comments below:

On Wed, Oct 14, 2020 at 04:04:42PM +0800, Luwei Kang wrote:
> The current implementation will extend the CPUID level to 0x14 if
> Intel PT is enabled in the guest(in x86_cpu_expand_features()) and
> the Intel PT will be disabled if it can't pass the capabilities
> checking later(in x86_cpu_filter_features()). In this case, the
> level of CPUID will be still 0x14 and the CPUID values from leaf
> 0xe to 0x14 are all zero.
> 
> This patch moves the capabilities checking before setting the
> level of the CPUID.

Why is this patch necessary and what problem does it fix?  Is it
a nice to have feature, or a bug fix?

If you still want to change how the x86_cpu_adjust_level() code
behaves, it should apply to all features filtered by
x86_cpu_filter_features(), not just intel-pt, shouldn't it?

> 
> Signed-off-by: Luwei Kang <luwei.kang@intel.com>
> ---
>  target/i386/cpu.c | 63 ++++++++++++++++++++++++-----------------------
>  1 file changed, 32 insertions(+), 31 deletions(-)
> 
> diff --git a/target/i386/cpu.c b/target/i386/cpu.c
> index 9eafbe3690..24644abfd4 100644
> --- a/target/i386/cpu.c
> +++ b/target/i386/cpu.c
> @@ -6401,12 +6401,40 @@ static void x86_cpu_expand_features(X86CPU *cpu, 
> Error **errp)
>  
>          /* Intel Processor Trace requires CPUID[0x14] */
>          if ((env->features[FEAT_7_0_EBX] & CPUID_7_0_EBX_INTEL_PT)) {
> -            if (cpu->intel_pt_auto_level) {
> -                x86_cpu_adjust_level(cpu, &cpu->env.cpuid_min_level, 0x14);
> -            } else if (cpu->env.cpuid_min_level < 0x14) {
> +            uint32_t eax_0, ebx_0, ecx_0, eax_1, ebx_1;
> +
> +            eax_0 = kvm_arch_get_supported_cpuid(kvm_state, 0x14, 0, R_EAX);
> +            ebx_0 = kvm_arch_get_supported_cpuid(kvm_state, 0x14, 0, R_EBX);
> +            ecx_0 = kvm_arch_get_supported_cpuid(kvm_state, 0x14, 0, R_ECX);
> +            eax_1 = kvm_arch_get_supported_cpuid(kvm_state, 0x14, 1, R_EAX);
> +            ebx_1 = kvm_arch_get_supported_cpuid(kvm_state, 0x14, 1, R_EBX);
> +
> +            if (eax_0 &&
> +               ((ebx_0 & INTEL_PT_MINIMAL_EBX) == INTEL_PT_MINIMAL_EBX) &&
> +               ((ecx_0 & INTEL_PT_MINIMAL_ECX) == INTEL_PT_MINIMAL_ECX) &&
> +               ((eax_1 & INTEL_PT_MTC_BITMAP) == INTEL_PT_MTC_BITMAP) &&
> +               ((eax_1 & INTEL_PT_ADDR_RANGES_NUM_MASK) >=
> +                                           INTEL_PT_ADDR_RANGES_NUM) &&
> +               ((ebx_1 & (INTEL_PT_PSB_BITMAP | INTEL_PT_CYCLE_BITMAP)) ==
> +                    (INTEL_PT_PSB_BITMAP | INTEL_PT_CYCLE_BITMAP)) &&
> +               !(ecx_0 & INTEL_PT_IP_LIP)) {
> +                if (cpu->intel_pt_auto_level) {
> +                    x86_cpu_adjust_level(cpu, &cpu->env.cpuid_min_level, 
> 0x14);
> +                } else if (cpu->env.cpuid_min_level < 0x14) {
> +                    mark_unavailable_features(cpu, FEAT_7_0_EBX,
> +                        CPUID_7_0_EBX_INTEL_PT,
> +                        "Intel PT need CPUID leaf 0x14, please set by \"-cpu 
> ...,+intel-pt,min-level=0x14\"");
> +                }
> +            } else {
> +               /*
> +                * Processor Trace capabilities aren't configurable, so if the
> +                * host can't emulate the capabilities we report on
> +                * cpu_x86_cpuid(), intel-pt can't be enabled on the current
> +                * host.
> +                */
>                  mark_unavailable_features(cpu, FEAT_7_0_EBX,
>                      CPUID_7_0_EBX_INTEL_PT,
> -                    "Intel PT need CPUID leaf 0x14, please set by \"-cpu 
> ...,+intel-pt,min-level=0x14\"");
> +                    "host Intel PT features doesn't satisfy the guest 
> request.");
>              }
>          }
>  
> @@ -6466,33 +6494,6 @@ static void x86_cpu_filter_features(X86CPU *cpu, bool 
> verbose)
>          uint64_t unavailable_features = requested_features & ~host_feat;
>          mark_unavailable_features(cpu, w, unavailable_features, prefix);
>      }
> -
> -    if ((env->features[FEAT_7_0_EBX] & CPUID_7_0_EBX_INTEL_PT) &&
> -        kvm_enabled()) {
> -        KVMState *s = CPU(cpu)->kvm_state;
> -        uint32_t eax_0 = kvm_arch_get_supported_cpuid(s, 0x14, 0, R_EAX);
> -        uint32_t ebx_0 = kvm_arch_get_supported_cpuid(s, 0x14, 0, R_EBX);
> -        uint32_t ecx_0 = kvm_arch_get_supported_cpuid(s, 0x14, 0, R_ECX);
> -        uint32_t eax_1 = kvm_arch_get_supported_cpuid(s, 0x14, 1, R_EAX);
> -        uint32_t ebx_1 = kvm_arch_get_supported_cpuid(s, 0x14, 1, R_EBX);
> -
> -        if (!eax_0 ||
> -           ((ebx_0 & INTEL_PT_MINIMAL_EBX) != INTEL_PT_MINIMAL_EBX) ||
> -           ((ecx_0 & INTEL_PT_MINIMAL_ECX) != INTEL_PT_MINIMAL_ECX) ||
> -           ((eax_1 & INTEL_PT_MTC_BITMAP) != INTEL_PT_MTC_BITMAP) ||
> -           ((eax_1 & INTEL_PT_ADDR_RANGES_NUM_MASK) <
> -                                           INTEL_PT_ADDR_RANGES_NUM) ||
> -           ((ebx_1 & (INTEL_PT_PSB_BITMAP | INTEL_PT_CYCLE_BITMAP)) !=
> -                (INTEL_PT_PSB_BITMAP | INTEL_PT_CYCLE_BITMAP)) ||
> -           (ecx_0 & INTEL_PT_IP_LIP)) {
> -            /*
> -             * Processor Trace capabilities aren't configurable, so if the
> -             * host can't emulate the capabilities we report on
> -             * cpu_x86_cpuid(), intel-pt can't be enabled on the current 
> host.
> -             */
> -            mark_unavailable_features(cpu, FEAT_7_0_EBX, 
> CPUID_7_0_EBX_INTEL_PT, prefix);
> -        }
> -    }
>  }
>  
>  static void x86_cpu_realizefn(DeviceState *dev, Error **errp)
> -- 
> 2.18.4
> 

-- 
Eduardo