Re: [Xen-devel] [PATCH 03/18] x86/boot: use %ecx instead of %eax
On Tue, Feb 03, 2015 at 10:02:09AM +, Jan Beulich wrote: On 30.01.15 at 18:54, daniel.ki...@oracle.com wrote: /* Save the Multiboot info struct (after relocation) for later use. */ mov $sym_phys(cpu0_stack)+1024,%esp -push%ebx -callreloc +mov %ecx,%eax +push%ebx/* Multiboot information address */ +callreloc /* %eax contains trampoline address */ This last part looks completely unrelated to the change made here (and contrary to the description, as here you clobber %eax while the description says reloc() needs it unclobbered); afaict it belongs in whatever patch add the consumption of this value in reloc(). Yep, this is confusing. I should change reloc.c:_start() in this patch too. That said - passing parameters to reloc() by two different means looks very odd too. I'm clearly of the opinion that parameter passing should follow an existing convention unless entirely unfeasible. Which then raises the question whether this patch is So, I think that we should add another patch which fixes this issue and put all arguments on the stack according to the cdecl calling convention on x86. really needed: Rather than fiddling with a lot of code, can't you just copy the incoming %eax into some other register, making this a single line change that can again simply be done in the patch where you actually consume the new information? If we do thing(s) mentioned above then this issue will disappear too. Additionally, I think that we should not use another register if it is not really required. Daniel ___ Xen-devel mailing list Xen-devel@lists.xen.org http://lists.xen.org/xen-devel
Re: [Xen-devel] [PATCH 03/18] x86/boot: use %ecx instead of %eax
On 30.01.15 at 18:54, daniel.ki...@oracle.com wrote: /* Save the Multiboot info struct (after relocation) for later use. */ mov $sym_phys(cpu0_stack)+1024,%esp -push%ebx -callreloc +mov %ecx,%eax +push%ebx/* Multiboot information address */ +callreloc /* %eax contains trampoline address */ This last part looks completely unrelated to the change made here (and contrary to the description, as here you clobber %eax while the description says reloc() needs it unclobbered); afaict it belongs in whatever patch add the consumption of this value in reloc(). That said - passing parameters to reloc() by two different means looks very odd too. I'm clearly of the opinion that parameter passing should follow an existing convention unless entirely unfeasible. Which then raises the question whether this patch is really needed: Rather than fiddling with a lot of code, can't you just copy the incoming %eax into some other register, making this a single line change that can again simply be done in the patch where you actually consume the new information? Jan ___ Xen-devel mailing list Xen-devel@lists.xen.org http://lists.xen.org/xen-devel
[Xen-devel] [PATCH 03/18] x86/boot: use %ecx instead of %eax
Use %ecx instead of %eax to store low memory upper limit from EBDA. This way we do not wipe multiboot protocol identifier. It is needed in reloc() to differentiate between multiboot (v1) and multiboot2 protocol. Signed-off-by: Daniel Kiper daniel.ki...@oracle.com Reviewed-by: Andrew Cooper andrew.coop...@citrix.com --- xen/arch/x86/boot/head.S | 27 ++- 1 file changed, 14 insertions(+), 13 deletions(-) diff --git a/xen/arch/x86/boot/head.S b/xen/arch/x86/boot/head.S index c99f739..6180783 100644 --- a/xen/arch/x86/boot/head.S +++ b/xen/arch/x86/boot/head.S @@ -86,14 +86,14 @@ __start: jne not_multiboot /* Set up trampoline segment 64k below EBDA */ -movzwl 0x40e,%eax /* EBDA segment */ -cmp $0xa000,%eax/* sanity check (high) */ +movzwl 0x40e,%ecx /* EBDA segment */ +cmp $0xa000,%ecx/* sanity check (high) */ jae 0f -cmp $0x4000,%eax/* sanity check (low) */ +cmp $0x4000,%ecx/* sanity check (low) */ jae 1f 0: -movzwl 0x413,%eax /* use base memory size on failure */ -shl $10-4,%eax +movzwl 0x413,%ecx /* use base memory size on failure */ +shl $10-4,%ecx 1: /* * Compare the value in the BDA with the information from the @@ -105,21 +105,22 @@ __start: cmp $0x100,%edx /* is the multiboot value too small? */ jb 2f /* if so, do not use it */ shl $10-4,%edx -cmp %eax,%edx /* compare with BDA value */ -cmovb %edx,%eax /* and use the smaller */ +cmp %ecx,%edx /* compare with BDA value */ +cmovb %edx,%ecx /* and use the smaller */ 2: /* Reserve 64kb for the trampoline */ -sub $0x1000,%eax +sub $0x1000,%ecx /* From arch/x86/smpboot.c: start_eip had better be page-aligned! */ -xor %al, %al -shl $4, %eax -mov %eax,sym_phys(trampoline_phys) +xor %cl, %cl +shl $4, %ecx +mov %ecx,sym_phys(trampoline_phys) /* Save the Multiboot info struct (after relocation) for later use. */ mov $sym_phys(cpu0_stack)+1024,%esp -push%ebx -callreloc +mov %ecx,%eax +push%ebx/* Multiboot information address */ +callreloc /* %eax contains trampoline address */ mov %eax,sym_phys(multiboot_ptr) /* Initialize BSS (no nasty surprises!) */ -- 1.7.10.4 ___ Xen-devel mailing list Xen-devel@lists.xen.org http://lists.xen.org/xen-devel
