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

[Vivek Goyal <vgoyal@in.ibm.com>]

On Mon, May 01, 2006 at 06:49:16PM +0900, Magnus Damm wrote:
> kexec: Avoid overwriting the current pgd (i386)
> 
> This patch upgrades the i386-specific kexec code to avoid overwriting the
> current pgd. Overwriting the current pgd is bad when CONFIG_CRASH_DUMP is used
> to start a secondary kernel that dumps the memory of the previous kernel.
> 
> The code introduces a new set of page tables called "page_table_a". These
> tables are used to provide an executable identity mapping without overwriting
> the current pgd.

True, current pgd is overwritten but that effects only user space mappings
and currently "crash" supports only backtracing kernel space code. But at
the same time probably it is not a bad idea to maintain a separate page
table and switch to that instead of overwriting the existing pgd. This
shall help if in future user space backtracing is also supported.
  
[..]
>  
> +static int allocate_page_table_a(struct kimage *image)
> +{
> +	struct page *page;
> +	int k = sizeof(image->page_table_a) / sizeof(image->page_table_a[0]);
> +
> +	for (; k > 0; k--) {
> +		page = kimage_alloc_control_pages(image, 0);
> +		if (!page)
> +			return -ENOMEM;
> +
> +		clear_page(page_address(page));
> +		image->page_table_a[k - 1] = page;

I think you also need to write the logic to free those pages if somebody
chooses to unload the pre-loaded kernel.

[..]
> --- 0001/include/linux/kexec.h
> +++ work/include/linux/kexec.h	2006-05-01 11:13:14.000000000 +0900
> @@ -69,6 +69,17 @@ struct kimage {
>  	unsigned long start;
>  	struct page *control_code_page;
>  
> +	/* page_table_a[] holds enough pages to create a new page table
> +	 * that maps the control page twice..
> +	 */
> +
> +#if defined(CONFIG_X86_32) && !defined(CONFIG_X86_PAE)
> +	struct page *page_table_a[3]; /* (2 * pte) + pgd */
> +#endif
> +#if defined(CONFIG_X86_32) && defined(CONFIG_X86_PAE)
> +	struct page *page_table_a[5]; /* (2 * pte) + (2 * pmd) + pgd */
> +#endif
> +

Would it make a cleaner interface if these array of pointers are maintained
in arch dependent code as global variables instead of putting in arch
independent code. Existing code does something simlar. This becomes further
ugly when another array comes into the picture for x86_64 in next patch.
(page_table_b)

Thanks
Vivek