pSeries_mach_cpu_die() question

pSeries_mach_cpu_die() question

[Benjamin Herrenschmidt]

While doing some autumn cleaning of the irq stuff in general and xics
specifically, I found out that
the low level pSeriesLP_cppr_info() is exported because
pSeries_mach_cpu_die() calls it:

static void pSeries_mach_cpu_die(void)
{
	local_irq_disable();
	idle_task_exit();
	/* Some hardware requires clearing the CPPR, while other hardware does
not
	 * it is safe either way
	 */
	pSeriesLP_cppr_info(0, 0);
	rtas_stop_self();
	/* Should never get here... */
	BUG();
	for(;;);
}

This leads to a few questions:

 - We always pass "0" as the CPU. Is that right ? I seems not, but maybe
pHyp doesn't care and always assume the calling CPU ...

 - xics has a xics_teardown_cpu() now, used by kexec, that does
something very similar except that it passes the proper CPU number, and
for secondary CPUs also does an EOI of any pending IPI (just in case). I
think that could be used instead of the direct call to the low level
pSeriesLP_* funciton (which I itend to unexport and rename anyway as
part of my rework). Can whoever knows that code confirm ?

 - There is a comment about "some hardware....", what does it mean ? Is
it ok to do it unconditionally ? I suppose so but heh...

Cheers,
Ben.

Re: pSeries_mach_cpu_die() question

[Nathan Lynch]

Benjamin Herrenschmidt wrote:
> While doing some autumn cleaning of the irq stuff in general and xics
> specifically, I found out that
> the low level pSeriesLP_cppr_info() is exported because
> pSeries_mach_cpu_die() calls it:
> 
> static void pSeries_mach_cpu_die(void)
> {
> 	local_irq_disable();
> 	idle_task_exit();
> 	/* Some hardware requires clearing the CPPR, while other hardware does
> not
> 	 * it is safe either way
> 	 */
> 	pSeriesLP_cppr_info(0, 0);
> 	rtas_stop_self();
> 	/* Should never get here... */
> 	BUG();
> 	for(;;);
> }
> 
> This leads to a few questions:
> 
>  - We always pass "0" as the CPU. Is that right ? I seems not, but maybe
> pHyp doesn't care and always assume the calling CPU ...

The cpu parameter is actually unused by in the lpar case:

void pSeriesLP_cppr_info(int n_cpu, u8 value)
{
	unsigned long lpar_rc;

	lpar_rc = plpar_cppr(value);
	if (lpar_rc != H_SUCCESS)
		panic("bad return code cppr - rc = %lx\n", lpar_rc);
}


>  - xics has a xics_teardown_cpu() now, used by kexec, that does
> something very similar except that it passes the proper CPU number, and
> for secondary CPUs also does an EOI of any pending IPI (just in case). I
> think that could be used instead of the direct call to the low level
> pSeriesLP_* funciton (which I itend to unexport and rename anyway as
> part of my rework). Can whoever knows that code confirm ?

Sounds okay to me.


>  - There is a comment about "some hardware....", what does it mean ? Is
> it ok to do it unconditionally ? I suppose so but heh...

The comment should be changed or removed really.  We got away without
doing plpar_cppr() on the Power4 hypervisor but we found out it was
necessary when testing Power5.  I think it's required by the
architecture regardless, and yes, it's safe on both platforms.

Re: pSeries_mach_cpu_die() question

[Benjamin Herrenschmidt]

On Fri, 2006-06-02 at 01:19 -0500, Nathan Lynch wrote:

> The cpu parameter is actually unused by in the lpar case:

Ok, missed that :)

> >  - xics has a xics_teardown_cpu() now, used by kexec, that does
> > something very similar except that it passes the proper CPU number, and
> > for secondary CPUs also does an EOI of any pending IPI (just in case). I
> > think that could be used instead of the direct call to the low level
> > pSeriesLP_* funciton (which I itend to unexport and rename anyway as
> > part of my rework). Can whoever knows that code confirm ?
> 
> Sounds okay to me.

Ok. I'll call it with 0 for the "secondary" argument so it doesn't do
the additional EOI of the IPI in order to not change behaviour from the
current code. We can do differently in the future if we want.

> The comment should be changed or removed really.  We got away without
> doing plpar_cppr() on the Power4 hypervisor but we found out it was
> necessary when testing Power5.  I think it's required by the
> architecture regardless, and yes, it's safe on both platforms.

I'll remove the comment.

Thanks !

Cheers,
Ben.