Re: [Xen-devel] [PATCH 06/13] libx86: Introduce a helper to serialise a cpuid_policy object

["Jan Beulich" <JBeulich@xxxxxxxx>]

>>> On 03.07.18 at 22:55, <andrew.cooper3@xxxxxxxxxx> wrote:
> --- a/xen/common/libx86/cpuid.c
> +++ b/xen/common/libx86/cpuid.c
> @@ -34,6 +34,100 @@ const uint32_t *x86_cpuid_lookup_deep_deps(uint32_t 
> feature)
>  }
>  
>  /*
> + * Copy a single cpuid_leaf into a provided xen_cpuid_leaf_t buffer,
> + * performing boundary checking against the buffer size.
> + */
> +static int copy_leaf_to_buffer(uint32_t leaf, uint32_t subleaf,
> +                               const struct cpuid_leaf *data,
> +                               cpuid_leaf_buffer_t leaves,
> +                               uint32_t *curr_entry, const uint32_t 
> nr_entries)
> +{
> +    const xen_cpuid_leaf_t val = {
> +        leaf, subleaf, data->a, data->b, data->c, data->d,
> +    };
> +
> +    if ( *curr_entry == nr_entries )
> +        return -ENOBUFS;
> +
> +    if ( copy_to_buffer_offset(leaves, *curr_entry, &val, 1) )
> +        return -EFAULT;
> +
> +    ++*curr_entry;

Following on from what Wei has said - you don't mean to have a way
here then to indicate to a higher up caller how many slots would have
been needed?

> +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 )
> +    {
> +        switch ( leaf )
> +        {
> +        case 0x4:
> +            for ( subleaf = 0; subleaf < ARRAY_SIZE(p->cache.raw); ++subleaf 
> )
> +                COPY_LEAF(leaf, subleaf, &p->cache.raw[subleaf]);
> +            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]);
> +            break;
> +
> +        case 0xb:
> +            for ( subleaf = 0; subleaf < ARRAY_SIZE(p->topo.raw); ++subleaf )
> +                COPY_LEAF(leaf, subleaf, &p->topo.raw[subleaf]);
> +            break;
> +
> +        case 0xd:
> +            for ( subleaf = 0; subleaf < ARRAY_SIZE(p->xstate.raw); 
> ++subleaf )
> +                COPY_LEAF(leaf, subleaf, &p->xstate.raw[subleaf]);
> +            break;
> +
> +        default:
> +            COPY_LEAF(leaf, XEN_CPUID_NO_SUBLEAF, &p->basic.raw[leaf]);
> +            break;
> +        }
> +    }
> +
> +    COPY_LEAF(0x40000000, XEN_CPUID_NO_SUBLEAF,
> +              &(struct cpuid_leaf){ p->hv_limit });
> +    COPY_LEAF(0x40000100, XEN_CPUID_NO_SUBLEAF,
> +              &(struct cpuid_leaf){ p->hv2_limit });

Is it a good idea to produce wrong (zero) EBX, ECX, and EDX values here?

> @@ -27,6 +33,17 @@ static inline bool test_bit(unsigned int bit, const void 
> *vaddr)
>      return addr[bit / 8] & (1u << (bit % 8));
>  }
>  
> +/* memcpy(), but with copy_to_guest_offset()'s API */
> +#define copy_to_buffer_offset(dst, index, src, nr) ({   \
> +    const typeof(*(dst)) *s = (src);                    \
> +    unsigned int i;                                     \
> +                                                        \
> +    for ( i = 0; i < (nr); i++ )                        \
> +        (dst)[(index) + i] = s[i];                      \
> +                                                        \
> +    0;                                                  \
> +})

Should you try to avoid multiple evaluation of macro arguments
here?

Jan


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