Re: [Xen-devel] [PATCHv9 0/9] Xen: extend kexec hypercall for use with pv-ops kernels

[Daniel Kiper <daniel.kiper@xxxxxxxxxx>]

On Fri, Oct 11, 2013 at 07:49:21AM +0100, Jan Beulich wrote:
> >>> On 10.10.13 at 23:24, Daniel Kiper <daniel.kiper@xxxxxxxxxx> wrote:
> > On Thu, Oct 10, 2013 at 05:35:39PM +0100, David Vrabel wrote:
> >> The register state on executing the image is undefined (this is the
> >> specified ABI), so there is no need to set the registers to any
> >> particular value.
> >
> > So let's look into the docs.
>
> "docs"?
>
> > I have found at least two interesting source
> > files for us. linux/arch/x86/kernel/relocate_kernel_64.S (let's focus
> > on 64-bit; 32-bit is quite similar) contains something like:
> >
> >         /*
> >          * set all of the registers to known values
> >          * leave %rsp alone
> >          */
> >
> >         testq   %r11, %r11
> >         jnz 1f
> >         xorl    %eax, %eax
> >         xorl    %ebx, %ebx
> >         xorl    %ecx, %ecx
> >         xorl    %edx, %edx
> >         xorl    %esi, %esi
> >         xorl    %edi, %edi
> >         xorl    %ebp, %ebp
> >         xorl    %r8d, %r8d
> >         xorl    %r9d, %r9d
> >         xorl    %r10d, %r10d
> >         xorl    %r11d, %r11d
> >         xorl    %r12d, %r12d
> >         xorl    %r13d, %r13d
> >         xorl    %r14d, %r14d
> >         xorl    %r15d, %r15d
> >
> >         ret
>
> This is all source code, not documentation.

OK, maybe I should write "docs". Sadly we do not have real docs in that case
(correct me if I am wrong). So we are forced to use sources as a reference.
We did it writing our kexec implementation. However, we missed that part.

> > Comment clearly states: set all of the registers to known values; leave %rsp
> > alone.
> > So registers states, just before jump into purgatory, are clearly defined.
>
> The implementer of this code wanted them to be, but none of the
> above in any way says that there's a requirement. One could as
> well load ~0 into all of them, or some random or garbage pattern.

I agree that it could be anything here. However, we have real piece
of code and no other documentation. So we should read what is written.
No more no less. And here guys clear all registers. However, there is
no explanation why they are doing that (I think that this is the problem
here). But this code as I can see is actively maintained and I suppose
that there is some reason to leave it as is. I will ask them why they do that.

> > Now look into kexec-tools/purgatory/arch/x86_64/setup-x86_64.S. There is
> > no any single word about registers states. Even any instruction assumes
> > their
> > states. Excluding %rsp which should point to jump_back_entry. In our case
> > %rsp should point to 0UL stored at the stack (we have missed that and it
> > should be fixed by us).
>
> So then where's your problem with David leaving the register
> values alone?

We could not state that registers have undefined state on entry of purgatory
just reading kexec-tools/purgatory/arch/x86_64/setup-x86_64.S. In theory they
may have it but there is the caller side in 
linux/arch/x86/kernel/relocate_kernel_64.S
and we should take it into account too.

> >> If the implementation did zero the registers then an image could rely on
> >> this.  It would then be impossible to change the implementation to do
> >
> > This is bogus. Any sane developer or maintainer do not assume any register
> > state if it is not clearly described by existing docs. And it is not. Such
> > brain dead code would not be accepted at least at kexec list. Guys doing
> > that are crazy and I do not care about them. They are just asking for
> > problems.
>
> So you seem to contradict yourself: First you demand the registers
> to be zeroed, and then you explain why there's no need to? What's
> your position then after all?

As I correctly understand (correct me if I am wrong), David said that
some guys may assume that registers are zeroed on entry of purgatory
just because we do zero them in relocate_kernel_64.S. I do not agree.
There is no any statement in kexec-tools/purgatory/arch/x86_64/setup-x86_64.S
which allows them to do that. Even registers are zeroed (or set to other value)
in relocate_kernel_64.S.

Daniel

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