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

["Jan Beulich" <JBeulich@xxxxxxxx>]

>>> On 30.05.18 at 15:28, <luwei.kang@xxxxxxxxx> wrote:
> --- 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.

> +    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?

> +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.

> +}
> +
>  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.

> diff --git a/xen/include/asm-x86/ipt.h b/xen/include/asm-x86/ipt.h
> index a69f049..422f46a 100644
> --- a/xen/include/asm-x86/ipt.h
> +++ b/xen/include/asm-x86/ipt.h
> @@ -31,6 +31,25 @@
>  
>  extern unsigned int ipt_mode;
>  
> +enum ipt_cap {
> +    IPT_CAP_max_subleaf = 0,

Pointless value specification.

Jan



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