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

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

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

like this? If two bits is too few for "leaf"?

static const struct ipt_cap_desc {
    const char    *name;
    unsigned char leaf:2;
    unsigned char reg:2;
    unsinged int mask;
}

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


Get it. Will test it.

Thanks,
Luwei Kang

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