On 15/01/2021 15:37, Jan Beulich wrote:
> On 12.01.2021 20:48, Andrew Cooper wrote:
>> @@ -446,6 +430,31 @@ int compat_memory_op(unsigned int cmd,
>> XEN_GUEST_HANDLE_PARAM(void) compat)
>>
>> #undef XLAT_mem_acquire_resource_HNDL_frame_list
>>
>> + if ( xen_frame_list && cmp.mar.nr_frames )
>> + {
>> + /*
>> + * frame_list is an input for translated guests, and an
>> output
>> + * for untranslated guests. Only copy in for translated
>> guests.
>> + */
>> + if ( paging_mode_translate(currd) )
>> + {
>> + compat_pfn_t *compat_frame_list = (void
>> *)xen_frame_list;
>> +
>> + if ( !compat_handle_okay(cmp.mar.frame_list,
>> + cmp.mar.nr_frames) ||
>> + __copy_from_compat_offset(
>> + compat_frame_list, cmp.mar.frame_list,
>> + 0, cmp.mar.nr_frames) )
>> + return -EFAULT;
>> +
>> + /*
>> + * Iterate backwards over compat_frame_list[] expanding
>> + * compat_pfn_t to xen_pfn_t in place.
>> + */
>> + for ( int x = cmp.mar.nr_frames - 1; x >= 0; --x )
>> + xen_frame_list[x] = compat_frame_list[x];
> Just as a nit, without requiring you to adjust (but with the
> request to consider adjusting) - x getting used as array index
> would generally suggest it wants to be an unsigned type (despite
> me guessing the compiler ought to be able to avoid an explicit
> sign-extension for the actual memory accesses):
>
> for ( unsigned int x = cmp.mar.nr_frames; x--; )
> xen_frame_list[x] = compat_frame_list[x];
Signed numbers are not inherently evil. The range of x is between 0 and
1020 so there is no issue with failing to enter the loop.
It is the compilers job to make this optimisation. It is a very poor
use of a developers time to write logic which takes extra effort to
figure out whether it is correct or not.
You know what my attitude will be towards a compiler which is incapable
of making the optimisation, and you've got to go back a decade to find a
processor old enough to not have identical performance between the
unoptimised signed and unsigned forms.
Both signs of numbers have their uses, and a rigid policy of using
unsigned numbers does more harm than good (in this case, concerning the
simplicity of the code).
~Andrew
