Re: [Fastboot] [PATCH 00/03] kexec: Avoid overwriting the current pgd (V2)

["Magnus Damm" <magnus.damm@gmail.com>]

On 5/25/06, Eric W. Biederman <ebiederm@xmission.com> wrote:
> Magnus Damm <magnus@valinux.co.jp> writes:
>
> > Hi Eric,
> >
> > On Wed, 2006-05-24 at 20:56 -0600, Eric W. Biederman wrote:
> >>
> >> C code is much more accessible to other programmers than arch specific
> >> assembly.  The code on the control page was almost written in C, and
> >> I'm still not quite convinced that it would be wrong to do that.
> >
> > I agree with you that it is of course better to implement something in C
> > if possible compared to writing it in architecture-specific assembly.
> >
> > But I do not agree that wrapping architecture-specific assembly code in
> > C functions makes the code more understandable. I'd really like to meet
> > the kernel hacker that is aware of how x86 segmentation works but is
> > unable to read x86 assembly.
>
> For some young programmers it may be a matter of reading ability.
> For older programmers it is more likely to be a matter of reading
> speed.
>
> Regardless that is how the code is now, and how it came out of the series
> of code reviews I had to go through when I wrote it.  I had requests
> to do more in C and I never had a request to do more in assembly.
> Proving there was no sane way to write the control code page in
> C was actually difficult.

Consider this a "more assembly" request then. What about these reasons:

- C code requires a stack
You must access one page only and therefore you need to setup the
stackpointer somewhere. Requires assembly.

- We switch to a new page table, twice on x86_64
Requires assembly.

- Need to setup segment registeres and cr*
Requires assembly.

- You need to setup/clear registers before passing control
Requires assembly.

> If there is a legitimate reason to change the code that is fine.  But
> as it looked as simply a change without a good reason that is not
> fine.

We would have to duplicate the segment handling code in our xen port
otherwise. It's no biggie, it's just a matter of a few lines of code.

Also, I feel that my approach with a valid idt and gdt is more robust.

> The big problem was you did several things with a single patch,
> and that made the review much more difficult than it had to be.
>
> Having to check if you correctly modified the page tables, while also
> having to check for segmentation, and the interrupt descriptor
> transformations was distracting.

Let me know which parts you think are good and we will implement and
review them bit by bit instead then.

Thanks,

/ magnus