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

["Jan Beulich" <JBeulich@xxxxxxxx>]

>>> On 17.07.18 at 12:02, <andrew.cooper3@xxxxxxxxxx> wrote:
> 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.

Dropping the top bits could be done universally, e.g. by AND-ing the
leaf with 0xffff.

Jan



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