Re: [Xen-devel] [PATCH v2 2/3] xen/x86: use trampoline e820 buffer for BIOS interface only

["Jan Beulich" <JBeulich@xxxxxxxx>]

>>> On 23.03.17 at 16:40, <andrew.cooper3@xxxxxxxxxx> wrote:
> On 23/03/17 15:35, Jan Beulich wrote:
>>>>> On 23.03.17 at 07:25, <jgross@xxxxxxxx> wrote:
>>>>>
>>> +        movl    bootsym(e820nr), %ecx
>>> +        cmpl    %ecx, %eax
>>> +        cmova   %ecx, %eax                      # number of entries to move
>>> +        movl    %eax, %ecx
>>> +        shll    $2, %ecx
>>> +        jz      .Lcopyexit                      # early exit: nothing to 
> move
>>> +        addl    %eax, %ecx                      # number of 4-byte moves
>>> +        cld                                     # (5 times of entries)
>> I don't think you need this - the function is being called from C
>> code, which assume EFLAGS.DF to be always clear. And to make
>> the "5 times" more obvious, how about
>>
>>         cmova   %ecx, %eax                      # number of entries to move
>>         imul    $5, %eax, %ecx
>>         jrcxz   .Lcopyexit                      # early exit: nothing to 
> move
>>
>> And the branch isn't even needed - REP MOVS does the right
>> thing when %rcx is zero.
>>
>>> +        rep movsd                               # do the move
>>> +.Lcopyexit:
>>> +        retq
>> Please be consistent: Either suffix all insns, or only those where the
>> suffix is really needed.
> 
> None of these instructions need suffixes, so far as I can tell.  I would
> certainly prefer if we opted for only using suffixes when necessary,
> because the resulting code is neater.

Same here - I had actually meant to add a "(here: none)" remark,
but then forgot.

Jan


_______________________________________________
Xen-devel mailing list
Xen-devel@xxxxxxxxxxxxx
https://lists.xen.org/xen-devel