Re: [Fastboot] [PATCH] kexec: Avoid overwriting the current pgd (i386)

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

On 5/2/06, Eric W. Biederman <ebiederm@xmission.com> wrote:
> "Magnus Damm" <magnus.damm@gmail.com> writes:
>
> > On 5/2/06, Eric W. Biederman <ebiederm@xmission.com> wrote:
> >>
> >> Well global variables don't quite work in the normal case.
> >>
> >> However it probably makes most sense to maintain the needed variables
> >> in a structure on the control page.  Which will keep them out of harms way,
> >> and won't require patches to the generic code.
> >
> > I agree with both of you that the #ifdefs in struct kimage should be
> > avoided, but I wonder if adding variables in a structure on the
> > control page is the easiest and cleanest way.
> >
> > I think that defining a structure for each architecture in
> > include/asm/kexec.h that is included in struct kimage is the best way
> > to go. Then each architecture can have whatever data it wants there,
> > and we both avoid #ifdefs in struct kimage _and_ we stay away from
> > overly complicated code that just allocates, frees and parses
> > architecture-dependent data.
>
> Well I think it would be fairly simple to have a structure:
> struct control_page {
>        type     variabe;
>        ...
>        code[0];
> };
>
> Or something like that we can work with.
>
> The big reason for doing this is that I believe control pages
> have additional protection that struct kimage does, being allocated
> in areas where the kernel never sets up DMA transfers.  Possibly
> that needs to be fixed, but this is something we need to be very
> careful with.

I suppose you mean that control pages have additional protection that
struct kimage does _not_ have. Protection provided by
kimage_alloc_control_pages(), right?

I agree with you that this protection is good. But I do not see how
that applies to my patch, because the page_table_a[] pages pointed out
by struct kimage are read out by machine_kexec() and passed as
arguments to the assembly code. So the assembly code itself never
tries to access struct kimage. All data accessed by the assembly code
is allocated with kimage_alloc_control_pages(), isn't that good
enough? Or maybe I'm misunderstanding?

On top of that I fear that my patch likes the fact that the assembly
code in the control page is page aligned. But it is probably no biggie
to change. I'm just lazy. =)

> For a page table all we need to store is the physical address of the
> first page.  Storing and working with a struct page entry is the wrong
> thing to do.  I would prefer to stomp the kernel data structures than
> to add an extra dependencies on the original panic'd kernel.  At first
> glance I am afraid that you current code introduces extra
> dependencies.

I used struct page *[] because the control page was a struct page *. I
never use the contents of what the struct page points to, I only use
them to convert to physical/virtual addresses or pfns. So I would say
that no extra dependencies are introduced at all actually. But I may
be wrong.

I would be happy to change the struct page *[] to unsigned long [] or
something else, but I must say I like the typing that struct page
provides.

Regarding storing the just root page or all pages - I stored all pages
because I need to pass them to the Xen hypervisor which will fill in
new values in page_table_a. page_table_b OTOH never gets modified by
the hypervisor, which is why page_table_a is an array of pointers and
page_table_b is just a root pointer.

> You don't need two x86_64 page tables as you can easily map
> all of the kernel virtual address, and the identity mapped physical
> address until the x86_64 kernel stops using an 8TB/8TB split.

Sure. I just thought that one page table with a mix of 4K pages and
huge pages would result in difficult code. The current page_table_a
code is actually more or less the same on x86 and x86_64, with the
exception of some macro magic.

Thanks for the detailed reply!

/ magnus