On 16/07/18 11:45, Jan Beulich wrote:
>>>> On 13.07.18 at 22:03, <andrew.coop...@citrix.com> 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@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel
