RE: [PATCH 2/2] i386/cpu: Make the Intel PT LIP feature configurable

[Kang, Luwei]

> > The current implementation will disable the guest Intel PT feature if
> > the Intel PT LIP feature is supported on the host, but the LIP feature
> > is comming soon(e.g. SnowRidge and later).
> >
> > This patch will make the guest LIP feature configurable and Intel PT
> > feature can be enabled in guest when the guest LIP status same with
> > the host.
> >
> > Signed-off-by: Luwei Kang <luwei.kang@intel.com>
> 
> The patch looks good, but I would like to confirm if the feature works as
> expected if patch 1/2 is not applied.
> 
> I have manually fixed it up to make patch 1/1 not required, but I would like 
> to
> confirm it really works on both Intel hosts with
> LIP=1 and hosts with LIP=0.  Could you please test the following version of 
> the
> patch?

Hi Eduardo,
    Thanks for your reply. I sent out a new version(V2) which adds a dependency 
flag based on your modification. And all the test cases and results include HW 
w/ and w/o LIP list in version 2.

Thanks,
Luwei Kang

> 
> -------
> From 8a2c9fdbe50f7ffd9900c67e72f53c8dc85ff3fd Mon Sep 17 00:00:00 2001
> From: Luwei Kang <luwei.kang@intel.com>
> Date: Wed, 14 Oct 2020 16:04:43 +0800
> Subject: [PATCH] i386/cpu: Make the Intel PT LIP feature configurable
> 
> The current implementation will disable the guest Intel PT feature if the 
> Intel
> PT LIP feature is supported on the host, but the LIP feature is comming
> soon(e.g. SnowRidge and later).
> 
> This patch will make the guest LIP feature configurable and Intel PT feature 
> can
> be enabled in guest when the guest LIP status same with the host.
> 
> Signed-off-by: Luwei Kang <luwei.kang@intel.com>
> Message-Id: <20201014080443.23751-2-luwei.kang@intel.com>
> Signed-off-by: Eduardo Habkost <ehabkost@redhat.com>
> ---
>  target/i386/cpu.h |  4 ++++
>  target/i386/cpu.c | 27 ++++++++++++++++++++++++++-
>  2 files changed, 30 insertions(+), 1 deletion(-)
> 
> diff --git a/target/i386/cpu.h b/target/i386/cpu.h index 
> 88e8586f8f..c4a49c06a8
> 100644
> --- a/target/i386/cpu.h
> +++ b/target/i386/cpu.h
> @@ -541,6 +541,7 @@ typedef enum FeatureWord {
>      FEAT_VMX_EPT_VPID_CAPS,
>      FEAT_VMX_BASIC,
>      FEAT_VMX_VMFUNC,
> +    FEAT_14_0_ECX,
>      FEATURE_WORDS,
>  } FeatureWord;
> 
> @@ -797,6 +798,9 @@ typedef uint64_t FeatureWordArray[FEATURE_WORDS];
>  /* AVX512 BFloat16 Instruction */
>  #define CPUID_7_1_EAX_AVX512_BF16       (1U << 5)
> 
> +/* Packets which contain IP payload have LIP values */
> +#define CPUID_14_0_ECX_LIP              (1U << 31)
> +
>  /* CLZERO instruction */
>  #define CPUID_8000_0008_EBX_CLZERO      (1U << 0)
>  /* Always save/restore FP error pointers */ diff --git a/target/i386/cpu.c
> b/target/i386/cpu.c index 5a8c96072e..db9e48033d 100644
> --- a/target/i386/cpu.c
> +++ b/target/i386/cpu.c
> @@ -672,6 +672,7 @@ static void x86_cpu_vendor_words2str(char *dst,
> uint32_t vendor1,  #define TCG_XSAVE_FEATURES (CPUID_XSAVE_XSAVEOPT
> | CPUID_XSAVE_XGETBV1)
>            /* missing:
>            CPUID_XSAVE_XSAVEC, CPUID_XSAVE_XSAVES */
> +#define TCG_14_0_ECX_FEATURES 0
> 
>  typedef enum FeatureWordType {
>     CPUID_FEATURE_WORD,
> @@ -1301,6 +1302,26 @@ static FeatureWordInfo
> feature_word_info[FEATURE_WORDS] = {
>          }
>      },
> 
> +    [FEAT_14_0_ECX] = {
> +        .type = CPUID_FEATURE_WORD,
> +        .feat_names = {
> +            NULL, NULL, NULL, NULL,
> +            NULL, NULL, NULL, NULL,
> +            NULL, NULL, NULL, NULL,
> +            NULL, NULL, NULL, NULL,
> +            NULL, NULL, NULL, NULL,
> +            NULL, NULL, NULL, NULL,
> +            NULL, NULL, NULL, NULL,
> +            NULL, NULL, NULL, "intel-pt-lip",
> +        },
> +        .cpuid = {
> +            .eax = 0x14,
> +            .needs_ecx = true, .ecx = 0,
> +            .reg = R_ECX,
> +        },
> +        .tcg_features = TCG_14_0_ECX_FEATURES,
> +    },
> +
>  };
> 
>  typedef struct FeatureMask {
> @@ -5752,6 +5773,9 @@ void cpu_x86_cpuid(CPUX86State *env, uint32_t
> index, uint32_t count,
>              *eax = INTEL_PT_MAX_SUBLEAF;
>              *ebx = INTEL_PT_MINIMAL_EBX;
>              *ecx = INTEL_PT_MINIMAL_ECX;
> +            if (env->features[FEAT_14_0_ECX] & CPUID_14_0_ECX_LIP) {
> +                *ecx |= CPUID_14_0_ECX_LIP;
> +            }
>          } else if (count == 1) {
>              *eax = INTEL_PT_MTC_BITMAP | INTEL_PT_ADDR_RANGES_NUM;
>              *ebx = INTEL_PT_PSB_BITMAP | INTEL_PT_CYCLE_BITMAP; @@ -6498,7
> +6522,8 @@ static void x86_cpu_filter_features(X86CPU *cpu, bool verbose)
>                                             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)) {
> +           ((ecx_0 & CPUID_14_0_ECX_LIP) !=
> +                (env->features[FEAT_14_0_ECX] & CPUID_14_0_ECX_LIP))) {
>              /*
>               * Processor Trace capabilities aren't configurable, so if the
>               * host can't emulate the capabilities we report on
> --
> 2.28.0
> 
> --
> Eduardo