Re: [PATCH] Support for relocatable kdump kernel

[Michael Ellerman <michael@ellerman.id.au>]

[-- Attachment #1: Type: text/plain, Size: 4568 bytes --]

On Mon, 2008-10-20 at 15:04 +0530, Mohan Kumar M wrote:
> Michael Ellerman wrote:
> >>>  #ifdef CONFIG_CRASH_DUMP
> >>> +#ifdef CONFIG_RELOCATABLE
> >>> +
> >>> +static inline void reserve_kdump_trampoline(void) { ; }
> >>> +static inline void setup_kdump_trampoline(void) { ; }
> >>> +
> >>> +#else
> >>>  
> >>>  extern void reserve_kdump_trampoline(void);
> >>>  extern void setup_kdump_trampoline(void);
> >>>  
> >>> +#endif /* CONFIG_RELOCATABLE */
> > 
> > You've disabled the else case with your Kconfig changes, so you should
> > just rip all that code out.
> 
> I made Kconfig changes only to the 64 bit powerpc path and still the 32 
> bit powerpc code uses the legacy kdump code. So we need to retain some 
> of legacy kdump code.

Does it? I see CONFIG_CRASH_DUMP depending on PPC64, so there is no
32-bit kdump possible. Or is someone working on it out-of-tree?

> >>> diff --git a/arch/powerpc/kernel/head_64.S b/arch/powerpc/kernel/head_64.S
> >>> index e409338..5b12b10 100644
> >>> --- a/arch/powerpc/kernel/head_64.S
> >>> +++ b/arch/powerpc/kernel/head_64.S
> >>> @@ -97,6 +97,14 @@ __secondary_hold_spinloop:
> >>>  __secondary_hold_acknowledge:
> >>>  	.llong	0x0
> >>>  
> >>> +	/* This flag is set only for kdump kernels so that */
> >>> +	/* it will be relocatable. Purgatory code user space kexec-tools */
> >>> +	/* sets this flag. Do not move this variable as purgatory code */
> >>> +	/* relies on the position of this variables */
> >>> +	.globl	__kdump_flag
> >>> +__kdump_flag:
> >>> +	.llong	0x0
> > 
> > I guess the __ matches the other flags here, it's not the prettiest
> > though. For client code (like in iommu.c) it'd be nice to have static
> > inline, perhaps is_kdump_kernel() that hides this.
> > 
> Do you expect a function to do the checking in iommu.c?

You'd use the function in iommu.c, but it should be defined in some
header.

> >>>  #ifdef CONFIG_PPC_ISERIES
> >>>  	/*
> >>>  	 * At offset 0x20, there is a pointer to iSeries LPAR data.
> >>> @@ -1384,8 +1392,13 @@ _STATIC(__after_prom_start)
> >>>  	/* process relocations for the final address of the kernel */
> >>>  	lis	r25,PAGE_OFFSET@highest	/* compute virtual base of kernel */
> >>>  	sldi	r25,r25,32
> >>> +#ifdef CONFIG_CRASH_DUMP
> >>> +	ld	r7,__kdump_flag-_stext(r26)
> >>> +	cmpldi	cr0,r7,1	/* relocatable kernel ? */
> > 
> > You don't use the signature here?
> 
> kexec-tools check the signature and based on the signature it sets 
> __kdump_flag to 1 (or 0). So kernel code just checks whether its set or not.

OK. Does old purgatory ensure that the register is 0? Otherwise I think
it's possible that a new kernel could get confused by cruft left in that
register by an old purgatory - causing the 2nd kernel to think it's a
kdump kernel when it shouldn't be.

> >>> @@ -1401,9 +1414,21 @@ _STATIC(__after_prom_start)
> >>>  	beq	9f			/* have already put us at zero */
> >>>  	li	r6,0x100		/* Start offset, the first 0x100 */
> >>>  					/* bytes were copied earlier.	 */
> >>> -#ifdef CONFIG_RELOCATABLE
> >>> +
> >>> +#ifdef CONFIG_CRASH_DUMP
> >>> +/*
> >>> + * Check if the kernel has to be running as relocatable kernel based on the
> >>> + * variable __kdump_flag, if it is set the kernel is treated as relocatble
> >>> + * kernel, otherwise it will be moved to PHYSICAL_START
> >>> + */
> >>> +	ld	r7,__kdump_flag-_stext(r26)
> >>> +	cmpldi	cr0,r7,1
> >>> +	bne	regular
> >>> +
> >>>  	li	r5,__end_interrupts - _stext	/* just copy interrupts */
> >>> -#else
> >>> +	b	5f
> >>> +regular:
> >>> +#endif
> >>>  	lis	r5,(copy_to_here - _stext)@ha
> >>>  	addi	r5,r5,(copy_to_here - _stext)@l /* # bytes of memory to copy */
> > 
> > I'm jet lagged to hell, so I'm not sure I can trust my parsing of this.
> > But I think this definitely breaks CONFIG_RELOCATABLE without
> > CRASH_DUMP, and I'm not sure it's right otherwise.
> >
> Hmmm, I compiled and tried the kernel with 3 config option combinations: 
> 1. CONFIG_RELOCATABLE and CONFIG_CRASH_DUMP 2. CONFIG_RELOCATABLE 3. 
> Without CONFIG_RELOCATABLE (without CONFIG_CRASH_DUMP)
> 
> All of the above 3 combinations worked. This patch relies on Pauls' 
> patch5 in the relocatable kernel patcheset.

OK if you've tested.

cheers

-- 
Michael Ellerman
OzLabs, IBM Australia Development Lab

wwweb: http://michael.ellerman.id.au
phone: +61 2 6212 1183 (tie line 70 21183)

We do not inherit the earth from our ancestors,
we borrow it from our children. - S.M.A.R.T Person

[-- Attachment #2: This is a digitally signed message part --]
[-- Type: application/pgp-signature, Size: 189 bytes --]