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

["Jan Beulich" <JBeulich@xxxxxxxx>]

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

Other than the hope for some improvement there (and if nothing half
way reasonable turns up, then that'll too be fine for now) there's only
the previously given comment on copy_to_buffer_offset() here.

Jan



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