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

["Jan Beulich" <JBeulich@xxxxxxxx>]

>>> On 03.07.18 at 12:18, <luwei.kang@xxxxxxxxx> wrote:
>> > +#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".

I'd prefer if you used bit fields, as was meant to be implied by my
reply.

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

There is certainly the possibility that gcc versions differ in this regard.
But I'm sure it's clear to you that the code should build fine with all
supported versions. There's also the possibility that I'm overlooking
something.

Jan



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