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

[Juergen Gross <jgross@suse.com>]

On 23/03/17 16:35, Jan Beulich wrote:
>>>> On 23.03.17 at 07:25, <jgross@suse.com> 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)

Okay.

> 
>> + * 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.

Hmm, let me double ckeck. I'm sure to have tested it: the system came up
and the memory map looked as usual.

Oh wait - don't know whether I should cry or laugh: the system is really
fault tolerant! Seems as if e820map_copy returned zero and Xen used the
Multiboot-e820 map!

Thank you very much for questioning using bootsym!

>> +        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.

Okay, I'll change it.

> 
>> +        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).

No suffixes then in general and movsl here.

> 
>> @@ -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()?

Of course!


Juergen


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