Re: [Xen-devel] [PATCH v2 06/10] x86: Introduce a new function to get capability of Intel PT

["Kang, Luwei" <luwei.kang@xxxxxxxxx>]

> > --- a/xen/arch/x86/cpu/ipt.c
> > +++ b/xen/arch/x86/cpu/ipt.c
> > @@ -25,11 +25,74 @@
> >  #include <asm/ipt.h>
> >  #include <asm/msr.h>
> >
> > +#define EAX 0
> > +#define ECX 1
> > +#define EDX 2
> > +#define EBX 3
> > +#define CPUID_REGS_NUM   4 /* number of regsters (eax, ebx, ecx, edx) */
> > +
> > +#define BIT(nr)                 (1UL << (nr))
> 
> I don't particularly like any such pretty generic things to be added to 
> individual files, but I also have nothing better to suggest. But
> please add the missing i to the comment.
> 
> > +#define IPT_CAP(_n, _l, _r, _m)                               \
> > +    [IPT_CAP_ ## _n] = { .name = __stringify(_n), .leaf = _l, \
> > +        .reg = _r, .mask = _m }
> > +
> > +static struct ipt_cap_desc {
> > +    const char    *name;
> > +    unsigned int  leaf;
> > +    unsigned char reg;
> 
> I don't think leaf needs to be full 32 bits wide? Once shrunk by at least two 
> bits, the size of the overall structure could go down
> from 24 to 16 bytes.

OK, will change it from " unsigned int  " to "unsinged char".

> 
> > +    unsigned int  mask;
> > +} ipt_caps[] = {
> > +    IPT_CAP(max_subleaf,            0, EAX, 0xffffffff),
> > +    IPT_CAP(cr3_filter,             0, EBX, BIT(0)),
> > +    IPT_CAP(psb_cyc,                0, EBX, BIT(1)),
> > +    IPT_CAP(ip_filter,              0, EBX, BIT(2)),
> > +    IPT_CAP(mtc,                    0, EBX, BIT(3)),
> > +    IPT_CAP(ptwrite,                0, EBX, BIT(4)),
> > +    IPT_CAP(power_event,            0, EBX, BIT(5)),
> > +    IPT_CAP(topa_output,            0, ECX, BIT(0)),
> > +    IPT_CAP(topa_multi_entry,       0, ECX, BIT(1)),
> > +    IPT_CAP(single_range_output,    0, ECX, BIT(2)),
> > +    IPT_CAP(output_subsys,          0, ECX, BIT(3)),
> > +    IPT_CAP(payloads_lip,           0, ECX, BIT(31)),
> > +    IPT_CAP(addr_range,             1, EAX, 0x7),
> > +    IPT_CAP(mtc_period,             1, EAX, 0xffff0000),
> > +    IPT_CAP(cycle_threshold,        1, EBX, 0xffff),
> > +    IPT_CAP(psb_freq,               1, EBX, 0xffff0000),
> > +};
> 
> const?

Oh, will add it.

> 
> > +static unsigned int ipt_cap(const struct cpuid_leaf *cpuid_ipt, enum
> > +ipt_cap cap) {
> > +    const struct ipt_cap_desc *cd = &ipt_caps[cap];
> > +    unsigned int shift = ffs(cd->mask) - 1;
> 
> Do you really need this?
> 
> > +    unsigned int val = 0;
> > +
> > +    cpuid_ipt += cd->leaf;
> > +
> > +    switch ( cd->reg )
> > +    {
> > +    case EAX:
> > +        val = cpuid_ipt->a;
> > +        break;
> > +    case EBX:
> > +        val = cpuid_ipt->b;
> > +        break;
> > +    case ECX:
> > +        val = cpuid_ipt->c;
> > +        break;
> > +    case EDX:
> > +        val = cpuid_ipt->d;
> > +        break;
> > +    }
> > +
> > +    return (val & cd->mask) >> shift;
> 
> If all masks are indeed contiguous series of set bits, MASK_EXTR() can be 
> used here afaict.

Yes, it is a good define to me. Will fix it.

> 
> > +}
> > +
> >  static int __init parse_ipt_params(const char *str)  {
> >      if ( !strcmp("guest", str) )
> 
> So this is the end of the changes to this file, and the function you 
> introduce is static. I'm pretty sure compilers will warn about the
> unused static, and hence the build will fail at this point of the series (due 
> to -Werror). I think you want to introduce the function
> together with its first user.

I can't reproduce this issue by:
#./configure
# make build-xen         (-Werror has been included during build)
Could you tell me how to?

Thanks,
Luwei Kang


_______________________________________________
Xen-devel mailing list
Xen-devel@xxxxxxxxxxxxxxxxxxxx
https://lists.xenproject.org/mailman/listinfo/xen-devel