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 07:25, <jgross@xxxxxxxx> wrote:
> --- a/xen/arch/x86/boot/mem.S
> +++ b/xen/arch/x86/boot/mem.S
> @@ -67,10 +67,32 @@ get_memory_map:
>  
>          ret
>  
> +/*
> + * Copy E820 map obtained from BIOS to a buffer allocated by Xen.
> + * Input: %rdi: target address of e820 entry array
> + *        %rsi: maximum number of entries to copy

%esi (and also needs to be used that way below)

> + * Output: %rax: number of entries copied
> + */
> +        .code64
> +ENTRY(e820map_copy)
> +        mov     %rsi, %rax
> +        movq    $bootsym(e820map), %rsi

%rip-relative leaq? Even more - is bootsym() usable here at all? The
comment next to its definition says otherwise. Otoh I'm sure you've
tested this, so it must be working despite me not seeing how it would
do so.

> +        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. And in no case should you use an Intel
mnemonic (movsd) in AT&T syntax code (should be movsl).

> @@ -782,14 +782,16 @@ void __init noreturn __start_xen(unsigned long mbi_p)
>      }
>      else if ( efi_enabled(EFI_BOOT) )
>          memmap_type = "EFI";
> -    else if ( e820_raw_nr != 0 )
> +    else if ( (nr_e820 = copy_bios_e820(e820_raw.map, E820MAX)) != 0 )

ARRAY_SIZE()?

Jan


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