Re: [Xen-devel] [PATCH v2 07/13] libx86: Introduce a helper to serialise cpuid_policy objects

[Andrew Cooper <andrew.cooper3@xxxxxxxxxx>]

On 16/07/18 11:45, Jan Beulich wrote:
>>>> On 13.07.18 at 22:03, <andrew.cooper3@xxxxxxxxxx> wrote:
>> +int x86_cpuid_copy_to_buffer(const struct cpuid_policy *p,
>> +                             cpuid_leaf_buffer_t leaves,
>> +                             uint32_t *nr_entries_p)
>> +{
>> +    const uint32_t nr_entries = *nr_entries_p;
>> +    uint32_t curr_entry = 0, leaf, subleaf;
>> +
>> +#define COPY_LEAF(l, s, data)                                       \
>> +    ({  int ret;                                                    \
>> +        if ( (ret = copy_leaf_to_buffer(                            \
>> +                  l, s, data, leaves, &curr_entry, nr_entries)) )   \
>> +            return ret;                                             \
>> +    })
>> +
>> +    /* Basic leaves. */
>> +    for ( leaf = 0; leaf <= MIN(p->basic.max_leaf,
>> +                                ARRAY_SIZE(p->basic.raw) - 1); ++leaf )
> Here and ...
>
>> +    {
>> +        switch ( leaf )
>> +        {
>> +        case 0x4:
>> +            for ( subleaf = 0; subleaf < ARRAY_SIZE(p->cache.raw); 
>> ++subleaf )
>> +                COPY_LEAF(leaf, subleaf, &p->cache.raw[subleaf]);
> ... here ...
>
>> +            break;
>> +
>> +        case 0x7:
>> +            for ( subleaf = 0;
>> +                  subleaf <= MIN(p->feat.max_subleaf,
>> +                                 ARRAY_SIZE(p->feat.raw) - 1); ++subleaf )
>> +                COPY_LEAF(leaf, subleaf, &p->feat.raw[subleaf]);
> ... but even more importantly here I wonder whether some form(s) of
> for_each_...() wouldn't be helpful to introduce: Such constructs are a
> prime source of future copy-and-past mistakes, perhaps just missing
> a single of the distinguishing field names. If there was exactly one
> instance of those field names, that risk would imo be much reduced.
>
> For example (completely untested)
>
> #define for_each_subleaf(which, limit) \
>     for ( subleaf = 0; subleaf <= MIN(limit, ARRAY_SIZE(p->which.raw) - 1); 
> ++subleaf )
>         COPY_LEAF(leaf, subleaf, p->which.raw[subleaf]);
>
> albeit I realize that the specification of "limit" would then still require
> an open-coded use of "which", and I have no good idea how to
> avoid it.

This pattern shows up in several locations, but in addition to the
problems you've found here, such a construct would be even harder for
p->extd.max_leaf which has to account for truncating the top bits out of
the limit.

I already tried, and failed, to come up with a reasonable way to
encapsulate this.  The CPUID leaves aren't actually as consistent as
they appear at a first glance.

~Andrew

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