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

[Andrew Cooper <andrew.cooper3@xxxxxxxxxx>]

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.

~Andrew

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