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

[Jan Beulich <jbeulich@xxxxxxxx>]

On 30.03.2025 11:29, Julien Grall wrote:
> On 27/03/2025 23:34, Stefano Stabellini wrote:
>> 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.
>>
>> Signed-off-by: Stefano Stabellini <stefano.stabellini@xxxxxxx>
>> ---
>> Changes in v3:
>> - code style
>> - copy_bytes > 1 checks
>> - copy_bytes > 0 check for copy_to_guest_offset
>> ---
>>   xen/common/bitmap.c | 42 ++++++++++++++++++++++++++++++------------
>>   1 file changed, 30 insertions(+), 12 deletions(-)
>>
>> diff --git a/xen/common/bitmap.c b/xen/common/bitmap.c
>> index 3da63a32a6..d3f9347e62 100644
>> --- a/xen/common/bitmap.c
>> +++ b/xen/common/bitmap.c
>> @@ -52,7 +52,7 @@ static void clamp_last_byte(uint8_t *bp, unsigned int 
>> nbits)
>>      unsigned int remainder = nbits % 8;
>>   
>>      if (remainder)
>> -            bp[nbits/8] &= (1U << remainder) - 1;
>> +            *bp &= (1U << remainder) - 1;
> 
> This is changing the behavior of clamp_last_byte(). Yet, it doesn't seem 
> the comment on top of is changed. Is this intended? Also, I would 
> consider rename 'bp' so it is clearer this is meant to point to the last 
> byte of the bitmap rather than the start.

+1

Perhaps also drop "last" from its name? (It's not clear to me what 'b' actually
is meant to stand for in the parameter name. It may have been meant to say
"base"; it could now be meant to say "byte". Nevertheless just "p" as parameter
name is likely sufficient and then no longer ambiguous.)

Jan