Re: [PATCH v5] xen: simplify bitmap_to_xenctl_bitmap for little endian

[Stefano Stabellini <sstabellini@xxxxxxxxxx>]

On Tue, 1 Apr 2025, Jan Beulich wrote:
> From: Stefano Stabellini <stefano.stabellini@xxxxxxx>
> 
> The little endian implementation of bitmap_to_xenctl_bitmap leads to
> unnecessary xmallocs and xfrees. Given that Xen only supports little
> endian architectures, it is worth optimizing.
> 
> This patch removes the need for the xmalloc on little endian
> architectures.
> 
> Remove clamp_last_byte as it is only called once and only needs to
> modify one byte. Inline it.
> 
> Signed-off-by: Stefano Stabellini <stefano.stabellini@xxxxxxx>
> Signed-off-by: Jan Beulich <jbeulich@xxxxxxxx>
> ---
> v5: Fix IS_ENABLED() use. Keep more code common. Move comment.
>     Convert LE bitmap_long_to_byte() to just a declaration.

Thanks Jan, I looked at your version carefully and it looks correct to
me. I could give my reviewed-by but it looks weird given that I am also
the first author.

Reviewed-by: Stefano Stabellini <sstabellini@xxxxxxxxxx>


> --- a/xen/common/bitmap.c
> +++ b/xen/common/bitmap.c
> @@ -40,21 +40,6 @@
>   * for the best explanations of this ordering.
>   */
>  
> -/*
> - * If a bitmap has a number of bits which is not a multiple of 8 then
> - * the last few bits of the last byte of the bitmap can be
> - * unexpectedly set which can confuse consumers (e.g. in the tools)
> - * who also round up their loops to 8 bits. Ensure we clear those left
> - * over bits so as to prevent surprises.
> - */
> -static void clamp_last_byte(uint8_t *bp, unsigned int nbits)
> -{
> -     unsigned int remainder = nbits % 8;
> -
> -     if (remainder)
> -             bp[nbits/8] &= (1U << remainder) - 1;
> -}
> -
>  int __bitmap_empty(const unsigned long *bitmap, unsigned int bits)
>  {
>       int k, lim = bits/BITS_PER_LONG;
> @@ -338,7 +323,6 @@ static void bitmap_long_to_byte(uint8_t
>                       nbits -= 8;
>               }
>       }
> -     clamp_last_byte(bp, nbits);
>  }
>  
>  static void bitmap_byte_to_long(unsigned long *lp, const uint8_t *bp,
> @@ -359,12 +343,11 @@ static void bitmap_byte_to_long(unsigned
>  
>  #elif defined(__LITTLE_ENDIAN)
>  
> -static void bitmap_long_to_byte(uint8_t *bp, const unsigned long *lp,
> -                             unsigned int nbits)
> -{
> -     memcpy(bp, lp, DIV_ROUND_UP(nbits, BITS_PER_BYTE));
> -     clamp_last_byte(bp, nbits);
> -}
> +#define LITTLE_ENDIAN 1 /* For IS_ENABLED(). */
> +
> +/* Unused function, but a declaration is needed. */
> +void bitmap_long_to_byte(uint8_t *bp, const unsigned long *lp,
> +                      unsigned int nbits);
>  
>  static void bitmap_byte_to_long(unsigned long *lp, const uint8_t *bp,
>                               unsigned int nbits)
> @@ -384,22 +367,46 @@ int bitmap_to_xenctl_bitmap(struct xenct
>      uint8_t zero = 0;
>      int err = 0;
>      unsigned int xen_bytes = DIV_ROUND_UP(nbits, BITS_PER_BYTE);
> -    uint8_t *bytemap = xmalloc_array(uint8_t, xen_bytes);
> +    const uint8_t *bytemap;
> +    uint8_t last, *buf = NULL;
>  
> -    if ( !bytemap )
> -        return -ENOMEM;
> +    if ( !IS_ENABLED(LITTLE_ENDIAN) )
> +    {
> +        buf = xmalloc_array(uint8_t, xen_bytes);
> +        if ( !buf )
> +            return -ENOMEM;
> +
> +        bitmap_long_to_byte(buf, bitmap, nbits);
> +
> +        bytemap = buf;
> +    }
> +    else
> +        bytemap = (const uint8_t *)bitmap;
>  
>      guest_bytes = DIV_ROUND_UP(xenctl_bitmap->nr_bits, BITS_PER_BYTE);
>      copy_bytes  = min(guest_bytes, xen_bytes);
>  
> -    bitmap_long_to_byte(bytemap, bitmap, nbits);
> +    if ( copy_bytes > 1 &&
> +         copy_to_guest(xenctl_bitmap->bitmap, bytemap, copy_bytes - 1) )
> +        err = -EFAULT;
> +
> +    /*
> +     * If a bitmap has a number of bits which is not a multiple of 8 then the
> +     * last few bits of the last byte of the bitmap can be unexpectedly set,
> +     * which can confuse consumers (e.g. in the tools), who also may round up
> +     * their loops to 8 bits. Ensure we clear those left over bits so as to
> +     * prevent surprises.
> +     */
> +    last = bytemap[nbits / 8];
> +    if ( nbits % 8 )
> +        last &= (1U << (nbits % 8)) - 1;
> +
> +    xfree(buf);
>  
>      if ( copy_bytes &&
> -         copy_to_guest(xenctl_bitmap->bitmap, bytemap, copy_bytes) )
> +         copy_to_guest_offset(xenctl_bitmap->bitmap, copy_bytes - 1, &last, 
> 1) )
>          err = -EFAULT;
>  
> -    xfree(bytemap);
> -
>      for ( i = copy_bytes; !err && i < guest_bytes; i++ )
>          if ( copy_to_guest_offset(xenctl_bitmap->bitmap, i, &zero, 1) )
>              err = -EFAULT;
>