Re: [Xen-devel] [PATCH 03/18] x86/boot: use %ecx instead of %eax

[Daniel Kiper <daniel.kiper@xxxxxxxxxx>]

On Tue, Feb 03, 2015 at 10:02:09AM +0000, Jan Beulich wrote:
> >>> On 30.01.15 at 18:54, <daniel.kiper@xxxxxxxxxx> wrote:
> >          /* Save the Multiboot info struct (after relocation) for later 
> > use. */
> >          mov     $sym_phys(cpu0_stack)+1024,%esp
> > -        push    %ebx
> > -        call    reloc
> > +        mov     %ecx,%eax
> > +        push    %ebx                /* Multiboot information address */
> > +        call    reloc               /* %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@xxxxxxxxxxxxx
http://lists.xen.org/xen-devel