[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index] Re: [Xen-devel] [PATCH v2 2/3] xen/x86: use trampoline e820 buffer for BIOS interface only
On 23/03/17 16:35, Jan Beulich wrote: >>>> 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) 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@xxxxxxxxxxxxx https://lists.xen.org/xen-devel
|
Lists.xenproject.org is hosted with RackSpace, monitoring our |