RE: [PATCH v2 1/2] i386: Add Intel Processor Trace feature support

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 



> > From: Chao Peng <chao.p.peng@xxxxxxxxxxxxxxx>
> >
> > Expose Intel Processor Trace feature to guest.
> >
> > In order to make this feature migration-safe, new feature word
> > information "FEAT_INTEL_PT_EBX" and "FEAT_INTEL_PT_ECX" be added.
> > Some constant value initialized in CPUID[0x14].0x01 to guarantee get
> > same result in diffrent hardware when this feature is enabled.
> >
> > Signed-off-by: Chao Peng <chao.p.peng@xxxxxxxxxxxxxxx>
> > Signed-off-by: Luwei Kang <luwei.kang@xxxxxxxxx>
> > ---
> > v1->v2:
> >  - In order to make this feature migration-safe, new feature word
> >    information "FEAT_INTEL_PT_EBX" and "FEAT_INTEL_PT_ECX" be added.
> >    Some constant value initialized in CPUID[0x14].0x01 to guarantee
> >    get same result in diffrent hardware when this feature is enabled.
> >
> > ---
> >  target/i386/cpu.c | 68
> > +++++++++++++++++++++++++++++++++++++++++++++++++++++--
> >  target/i386/cpu.h |  6 +++++
> >  target/i386/kvm.c | 23 +++++++++++++++++++
> >  3 files changed, 95 insertions(+), 2 deletions(-)
> >
> > diff --git a/target/i386/cpu.c b/target/i386/cpu.c index
> > a49d222..ee8c6e6 100644
> > --- a/target/i386/cpu.c
> > +++ b/target/i386/cpu.c
> > @@ -172,7 +172,11 @@
> >  #define L2_ITLB_4K_ASSOC       4
> >  #define L2_ITLB_4K_ENTRIES   512
> >
> > -
> > +/* CPUID Leaf 0x14 constants: */
> > +#define INTLE_PT_ADDR_RANGES_NUM 2
> > +#define INTEL_PT_MTC_BITMAP      (0x0249 << 16) /* Support ART(0,3,6,9) */
> > +#define INTEL_PT_CYCLE_BITMAP    0x3fff         /* Support 0,2^(0~12) */
> > +#define INTEL_PT_PSB_BITMAP      (0x003f << 16) /* Support 2K,4K,8K,16K,32K,64K */
> >
> >  static void x86_cpu_vendor_words2str(char *dst, uint32_t vendor1,
> >                                       uint32_t vendor2, uint32_t
> > vendor3) @@ -427,7 +431,7 @@ static FeatureWordInfo feature_word_info[FEATURE_WORDS] = {
> >              NULL, NULL, "mpx", NULL,
> >              "avx512f", "avx512dq", "rdseed", "adx",
> >              "smap", "avx512ifma", "pcommit", "clflushopt",
> > -            "clwb", NULL, "avx512pf", "avx512er",
> > +            "clwb", "intel-pt", "avx512pf", "avx512er",
> >              "avx512cd", "sha-ni", "avx512bw", "avx512vl",
> >          },
> >          .cpuid_eax = 7,
> > @@ -545,6 +549,38 @@ static FeatureWordInfo feature_word_info[FEATURE_WORDS] = {
> >          .cpuid_reg = R_EDX,
> >          .tcg_features = ~0U,
> >      },
> > +    [FEAT_INTEL_PT_EBX] = {
> > +        .feat_names = {
> > +            "cr3-filter", "psb-cycle", "filtering", "mtc-cofi",
> > +            "ptwrite", "power-evt", NULL, NULL,
> 
> Note that this will make all those bits configurable on the command-line.  You don't really need to do this in the first version, if you
> don't really want to.
> 
> Also, if we want to make this configurable, we will need to choose carefully the property names, because changing them later would
> be more difficult.
> 
> Note that the feature names live in a single QOM property namespace, so "filtering", "any-entries" and "transport" are probably too
> generic.  Maybe we could prefix all names here with "intel-pt-" or "pt-"?
> 
> If you don't want to make all bits configurable on the first version, you can fill the CPUID[14h] bits with constant default values just
> like you did with the INTEL_PT_* constants below.
> 

Agree, I think it is too complex to make all Intel PT sub-feature configurable in command line. We also need make some short and clear enough property names for each sub-feature.

> 
> > +            NULL, NULL, NULL, NULL,
> > +            NULL, NULL, NULL, NULL,
> > +            NULL, NULL, NULL, NULL,
> > +            NULL, NULL, NULL, NULL,
> > +            NULL, NULL, NULL, NULL,
> > +            NULL, NULL, NULL, NULL,
> > +        },
> > +        .cpuid_eax = 0x14,
> > +        .cpuid_needs_ecx = true, .cpuid_ecx = 0,
> > +        .cpuid_reg = R_EBX,
> > +        .tcg_features = ~0U,
> 
> TCG doesn't implement this feature, so ~0U doesn't look right.
> 
> 
> > +    },
> > +    [FEAT_INTEL_PT_ECX] = {
> > +        .feat_names = {
> > +            "topa-output", "any-entries", "single-range", "transport",
> > +            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,
> > +        },
> > +        .cpuid_eax = 0x14,
> > +        .cpuid_needs_ecx = true, .cpuid_ecx = 0,
> > +        .cpuid_reg = R_ECX,
> > +        .tcg_features = ~0U,
> 
> Same as above.
> 
> > +    },
> >  };
> >
> >  typedef struct X86RegisterInfo32 {
> > @@ -3452,6 +3488,34 @@ void cpu_x86_cpuid(CPUX86State *env, uint32_t index, uint32_t count,
> >          }
> >          break;
> >      }
> > +    case 0x14: {
> > +	/* Intel Processor Trace Enumeration */
> > +        *eax = 0;
> > +        *ebx = 0;
> > +        *ecx = 0;
> > +        *edx = 0;
> > +        if (!(env->features[FEAT_7_0_EBX] & CPUID_7_0_EBX_INTEL_PT) ||
> > +             !kvm_enabled()) {
> > +            break;
> > +        }
> > +
> > +        if (count == 0) {
> > +            *eax = 1;
> > +            *ebx = env->features[FEAT_INTEL_PT_EBX];
> > +            *ecx = env->features[FEAT_INTEL_PT_ECX];
> > +        } else if (count == 1) {
> > +            *eax = INTLE_PT_ADDR_RANGES_NUM;
> > +            if (env->features[FEAT_INTEL_PT_EBX] &
> > +                    CPUID_INTEL_PT_EBX_MTC_COFI) {
> > +                *eax |= INTEL_PT_MTC_BITMAP;
> > +            }
> > +            if (env->features[FEAT_INTEL_PT_EBX] &
> > +                    CPUID_INTEL_PT_EBX_PSB_CYCLE) {
> > +                *ebx = INTEL_PT_PSB_BITMAP | INTEL_PT_CYCLE_BITMAP;
> > +            }
> 
> We still need to validate the bitmaps and number of ranges against the host capabilities (reported on GET_SUPPORTED_CPUID),
> don't we?

Yes, we need to validate the bitmaps and number of ranges against the host capabilities. For example,  MSR_IA32_RTIT_CTL.MTCFreq only support the value defined in bitmap or will cause #GP fault.

> 
> If you are going to set CPUID bits that are not already present on env->features[], you will want x86_cpu_filter_features() to
> manually validate the constants against x86_cpu_get_supported_feature_word(), to ensure we won't try to enable unsupported
> bits.
> 
> (If doing that, we need to make sure CPUID_7_0_EBX_INTEL_PT will be set on xc->filtered_features[FEAT_7_0_EBX] if something is
> unsupported, to tell the calling code that intel-pt can't be enabled on the current host)
> 

So, Can I make all the value in CPUID[14] as constant and make the CPUID information get from IceLake hardware as default(minimal) value.
CPUID[14] available only when Intel PT is supported and enabled.
We also need to check the host value by kvm_arch_get_supported_cpuid().  If something is unsupported in minimal value Intel PT can't be enabled.  I didn't use x86_cpu_get_supported_feature_word() because the value of CPUID[14] will all be constant hence sub-leaf FEAT_INTEL_PT_EBX/ FEAT_INTEL_PT_ECX are unnecessary(will remove in next version).
Please help correct me if anything wrong.

Thanks,
Luwei Kang

> In the future we could extend FeatureWordInfo to make it easier to handle counter/bitmap fields like those, then we won't need
> special cases inside x86_cpu_filter_features().
> 
> 




[Index of Archives]     [KVM ARM]     [KVM ia64]     [KVM ppc]     [Virtualization Tools]     [Spice Development]     [Libvirt]     [Libvirt Users]     [Linux USB Devel]     [Linux Audio Users]     [Yosemite Questions]     [Linux Kernel]     [Linux SCSI]     [XFree86]

  Powered by Linux