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

[ebiederm@xmission.com (Eric W. Biederman)]

"Magnus Damm" <magnus.damm@gmail.com> writes:

> The code is not broken. The code does exactly what you are talking
> about. Maybe I was a bit unclear.

Ack.  I got a little confused.  I was still thinking of what
the identity mapping was trying to achieve, and you don't achieve
the same thing.

> "page_table_a" contains two mappings. One which is the same as where
> the page is mapped in the kernel (the virtual address where
> relocate_new_kernel is located), and one identity mapped. So when the
> switch occurs the cpu will continue to run on the same virtual
> address.

In the general case which you want to cover page_table_a is still
slightly wrong.  In particular you can theoretically get a conflict
between the virtual address of your page table switching function and
the physical address of the control code page.

However while I agree that you need to do this in assembly for
control I disagree that this code should be part of the
relocate_new_kernel function.

Please move the code that uses page_table_a to a separate function,
that when it is done jumps to the control_code page.  Then you can
map this page both virtually and physically with a statically
allocated page table built a compile time.

This is a little simpler as you don't need to build this first
page table dynamically and a little clearer as you aren't trying to
get the control code page to serve two different functions.

If this function was more than 3 lines of assembly it could even
be written in C for clarity with a special section so that
the linker would not map it twice.  Of course that would still need
to address the stack usage problem.


>> So you need at least one additional entry in your page table.
>>
>> The fact that this bug did not jump out is a clear sign you were
>> changing too many things at once, and did not have an adequate
>> explanation in your change log.
>
> I will ignore that last part for now. I've tested the code on i386,
> i386/pae, x86_64 (both opteron and athlon64) and all the combinations
> with and without xen, and all configurations except x86_64 with both
> kexec and kdump.

Testing is important here.  But given that 99% of the bug hunting must
be done via code review and thinking about the problem testing is not
sufficient.

Eric