Re: [PATCH 06/10] viridian: add ExProcessorMasks variants of the flush hypercalls

[Jan Beulich <jbeulich@xxxxxxxx>]

On 11.11.2020 21:07, Paul Durrant wrote:
> --- a/xen/arch/x86/hvm/viridian/viridian.c
> +++ b/xen/arch/x86/hvm/viridian/viridian.c
> @@ -553,6 +553,83 @@ static unsigned int vpmask_next(struct hypercall_vpmask 
> *vpmask, unsigned int vp
>            (vp) < HVM_MAX_VCPUS; \
>            (vp) = vpmask_next(vpmask, vp))
>  
> +struct hypercall_vpset {
> +        struct hv_vpset set;
> +        uint64_t __bank_contents[64];

gcc documents this to be supported as an extension; did you check
clang supports this, too? (I'd also prefer if the leading
underscores could be dropped, but as you know I'm not the maintainer
of this code.)

> +static unsigned int hv_vpset_nr_banks(struct hv_vpset *vpset)
> +{
> +    uint64_t bank_mask;
> +    unsigned int nr = 0;
> +
> +    for ( bank_mask = vpset->valid_bank_mask; bank_mask; bank_mask >>= 1 )
> +        if ( bank_mask & 1 )
> +            nr++;
> +
> +    return nr;
> +}

Simply use hweight64()?

> +static uint16_t hv_vpset_to_vpmask(struct hv_vpset *set, size_t size,
> +                                   struct hypercall_vpmask *vpmask)
> +{
> +    switch ( set->format )
> +    {
> +    case HV_GENERIC_SET_ALL:
> +        vpmask_fill(vpmask);
> +        return 0;
> +
> +    case HV_GENERIC_SET_SPARSE_4K:
> +    {
> +        uint64_t bank_mask;
> +        unsigned int bank = 0, vp = 0;
> +
> +        vpmask_empty(vpmask);
> +        for ( bank_mask = set->valid_bank_mask; bank_mask; bank_mask >>= 1 )
> +        {
> +            /* Make sure we won't dereference past the end of the array */
> +            if ( (void *)(set->bank_contents + bank) >=
> +                 (void *)set + size )
> +            {
> +                ASSERT_UNREACHABLE();
> +                return -EINVAL;
> +            }

Doesn't this come too late? I.e. don't you want to check instead
(or also) that you won't overrun the space when copying in from
the guest? And for the specific purpose here, doesn't it come too
early, as you won't access any memory when the low bit of bank_mask
isn't set?

Jan