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

[Daniel Kiper <daniel.kiper@xxxxxxxxxx>]

On Thu, Oct 10, 2013 at 05:35:39PM +0100, David Vrabel wrote:
> On 10/10/13 16:45, Daniel Kiper wrote:
> > On Wed, Oct 09, 2013 at 05:03:22PM +0100, David Vrabel wrote:
> >> On 09/10/13 16:26, Daniel Kiper wrote:
> >>>
> >>> What about setting GPRs to known value (e.g. 0 like in Linux Kernel)
> >>> before jumping into purgatory?
> >>
> >> I have (repeatedly) explained why and you have not provided a sensible
> >
> > What do you mean by that? I have not seen any real explanation.
> > You were saying only that I am defining an ABI. I do not buy it.
> > Even you did not reply to my last question: Could you tell me
> > where do you see an ABI here?
>
> I'm going to comment on your points one final time. I am not going to
> debate with you any further on any of this.

Well, we are here to discuss technical stuff and we are doing that.
Sometimes it is tough but it does not mean that we should break that
discussion. Especially if adversaries obey the rules. I think this
is the case here. Additionally, we agreed some things so this is not
counterproductive. So still I count on cooperation with you.

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

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.

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

We call purgatory. Purgatory is shared code used at least by Linux and Xen.
It was created for Linux by Linux guys so we are the guest here. There is
no other (correct me if I missed something) docs saying anything about
registers states just before jump into purgatory. So we should obey the
rules described in linux/arch/x86/kernel/relocate_kernel_64.S. If we do not
like them we should ask authors of original kexec/kdump about this stuff
and act accordingly to their reply. There is no other way here.

> 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.
Additionally, there are more reliable ways to get 0 for our needs.

> anything other than zero the registers as that would break existing
> users.  Zeroing the register is thus an implicit or defacto ABI (even
> though we specified the register values as undefined).
>
> If the registers are not zeroed then it is highly unlikely that an image
> could make use of their values and thus if we wish to change the
> specification to set some register values we can safely do so without
> breaking existing images.

So how are you going differentiate between old Xen kexec implementation
(current proposal) and new Xen kexec implementation (current proposal +
some changes) in purgatory (if it be needed) using just only registers
if existing proposal does not enforce registers values (they are simply
random)?

> > However, why we do not care about compatiblity with existing implementation?
>
> Xen does diverge from ABI provided by Linux where it makes sense.  i.e.,

ABI should be compatible with exiting implementation because we are
using existing code. Please look above.

> where doing so makes for a better ABI or a better implementation.  For

However, I do not object against better implementation...

> example, 64-bit images are exec'd with page tables that only cover the
> image segments (unlike on Linux were the page tables cover all of RAM
> which has problems as noted by Jan with cachable mapping overlapping
> with uncachable regions).

...like in that case.

> Compatibility with existing Linux tools is a nice bonus but should not
> and does not constrain the Xen ABI or implementation.

Ditto.

> > Are we going to write special purgatory
> > code for Xen if one day original purgatory require 0 or another known value
> > in one or more registers?
>
> If that happens we can always revist the Xen implementation and consider
> changing or (or just fix purgatory).

Why we are assuming that we need fix our implementation just before inclusion
in Xen unstable? Why we cannot fix it now? Why we could not assume that our
implementation should run as long as possible without any changes?

> >>> By the way, you do not need to save and restore %rdi, %rsi and %rbx
> >>> in relocate_pages() in xen/arch/x86/x86_64/kexec_reloc.S.
> >>
> >> This is done so relocate_pages() behaves like a proper function with the
> >> standard calling convention.
> >
> > If you would like to be inline with GCC (and a few others) calling convetion
> > then you should save %rbx in relocate_pages() only. %rcx, %rdi and %rsi 
> > should
> > be saved by caller if needed.
>
> Yes, I got that wrong, but you're really into trivial nit-picking here
> which is quite frankly neither helpful nor productive.

We are here to discuss nitpicks too. It is not counterproductive.

> > Anyway, I do not care about saving registers not
> > used later in relocate_pages() or around it.
>
> This is stupid -- relocate_pages() is called like a function so it
> should behave like one.  Anything else is going to trip up someone else
> looking at or modifying the code in the future.

I do not agree with you but respect your opinion. If you insist on
making relocate_pages() as a real function do that. However, please
be inline with GCC calling convention.

Daniel

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