Re: [PATCH v2 4/7] bitmap: move to/from xenctl_bitmap conversion helpers

[Jan Beulich <jbeulich@xxxxxxxx>]

On 07.08.2020 19:50, Andrew Cooper wrote:
> On 07/08/2020 12:33, Jan Beulich wrote:
>> --- a/xen/common/bitmap.c
>> +++ b/xen/common/bitmap.c
>> @@ -384,3 +386,87 @@ void bitmap_byte_to_long(unsigned long *
>>  }
>>  
>>  #endif
>> +
>> +int bitmap_to_xenctl_bitmap(struct xenctl_bitmap *xenctl_bitmap,
>> +                            const unsigned long *bitmap, unsigned int nbits)
>> +{
>> +    unsigned int guest_bytes, copy_bytes, i;
>> +    uint8_t zero = 0;
>> +    int err = 0;
>> +    uint8_t *bytemap = xmalloc_array(uint8_t, (nbits + 7) / 8);
>> +
>> +    if ( !bytemap )
>> +        return -ENOMEM;
>> +
>> +    guest_bytes = (xenctl_bitmap->nr_bits + 7) / 8;
>> +    copy_bytes  = min_t(unsigned int, guest_bytes, (nbits + 7) / 8);
>> +
>> +    bitmap_long_to_byte(bytemap, bitmap, nbits);
>> +
>> +    if ( copy_bytes != 0 )
>> +        if ( copy_to_guest(xenctl_bitmap->bitmap, bytemap, copy_bytes) )
>> +            err = -EFAULT;
>> +
>> +    for ( i = copy_bytes; !err && i < guest_bytes; i++ )
>> +        if ( copy_to_guest_offset(xenctl_bitmap->bitmap, i, &zero, 1) )
>> +            err = -EFAULT;
>> +
>> +    xfree(bytemap);
>> +
>> +    return err;
>> +}
>> +
>> +int xenctl_bitmap_to_bitmap(unsigned long *bitmap,
>> +                            const struct xenctl_bitmap *xenctl_bitmap,
>> +                            unsigned int nbits)
>> +{
>> +    unsigned int guest_bytes, copy_bytes;
>> +    int err = 0;
>> +    uint8_t *bytemap = xzalloc_array(uint8_t, (nbits + 7) / 8);
>> +
>> +    if ( !bytemap )
>> +        return -ENOMEM;
>> +
>> +    guest_bytes = (xenctl_bitmap->nr_bits + 7) / 8;
>> +    copy_bytes  = min_t(unsigned int, guest_bytes, (nbits + 7) / 8);
>> +
>> +    if ( copy_bytes != 0 )
>> +    {
>> +        if ( copy_from_guest(bytemap, xenctl_bitmap->bitmap, copy_bytes) )
>> +            err = -EFAULT;
>> +        if ( (xenctl_bitmap->nr_bits & 7) && (guest_bytes == copy_bytes) )
>> +            bytemap[guest_bytes-1] &= ~(0xff << (xenctl_bitmap->nr_bits & 
>> 7));
>> +    }
>> +
>> +    if ( !err )
>> +        bitmap_byte_to_long(bitmap, bytemap, nbits);
>> +
>> +    xfree(bytemap);
>> +
>> +    return err;
>> +}
>> +
>> +int cpumask_to_xenctl_bitmap(struct xenctl_bitmap *xenctl_cpumap,
>> +                             const cpumask_t *cpumask)
>> +{
>> +    return bitmap_to_xenctl_bitmap(xenctl_cpumap, cpumask_bits(cpumask),
>> +                                   nr_cpu_ids);
>> +}
>> +
>> +int xenctl_bitmap_to_cpumask(cpumask_var_t *cpumask,
>> +                             const struct xenctl_bitmap *xenctl_cpumap)
>> +{
>> +    int err = 0;
>> +
>> +    if ( alloc_cpumask_var(cpumask) ) {
> 
> At a minimum, please fix this style during the move.

Oh, I should have noticed this. I've also spotted a 2nd style issue.

> However, the more I look at this code, the more concerned I get.
> 
> There is absolutely no need to allocate(/doubly allocate) memory or
> double/triple bounce buffer the data.  All that is necessary is some
> careful handling of the copy length, and trailing zeroing.
> 
> The cpumask variants should be trivial static inline wrapper.  The fact
> that they're not suggests an API error.
> 
> In reality, these are just data-shuffling helpers, and would probably
> live better in a guest-access.c if we had a suitable one to hand, but I
> guess bitmap.c will have to do for now.

But changing the implementation is certainly way beyond the purpose
here. (I also can't spot any pointless double allocation - the one
in xenctl_bitmap_to_cpumask() allocates an output of the function.)

Jan