[patch 08/18] PS3: Kexec support

Fixup the core platform parts needed for kexec to work on the PS3.
 - Setup ps3_hpte_clear correctly.
 - Mask interrupts on irq removal.
 - Release all hypervisor resources.

Signed-off-by: Geoff Levand <redacted>
---
 arch/powerpc/platforms/ps3/htab.c      |   14 +-
 arch/powerpc/platforms/ps3/interrupt.c |  199 ++++++++++++++++++++-------------
 arch/powerpc/platforms/ps3/setup.c     |   29 ++--
 3 files changed, 147 insertions(+), 95 deletions(-)

--- a/arch/powerpc/platforms/ps3/htab.c
+++ b/arch/powerpc/platforms/ps3/htab.c

@@ -234,10 +234,18 @@ static void ps3_hpte_invalidate(unsigned
 
 static void ps3_hpte_clear(void)
 {
-	/* Make sure to clean up the frame buffer device first */
-	ps3fb_cleanup();
+	int result;
 
-	lv1_unmap_htab(htab_addr);
+	DBG(" -> %s:%d\n", __func__, __LINE__);
+
+	result = lv1_unmap_htab(htab_addr);
+	BUG_ON(result);
+
+	ps3_mm_shutdown();
+
+	ps3_mm_vas_destroy();
+
+	DBG(" <- %s:%d\n", __func__, __LINE__);
 }
 
 void __init ps3_hpte_init(unsigned long htab_size)
--- a/arch/powerpc/platforms/ps3/interrupt.c
+++ b/arch/powerpc/platforms/ps3/interrupt.c

@@ -91,6 +91,92 @@ struct ps3_private {
 static DEFINE_PER_CPU(struct ps3_private, ps3_private);
 
 /**
+ * ps3_chip_mask - Set an interrupt mask bit in ps3_bmp.
+ * @virq: The assigned Linux virq.
+ *
+ * Sets ps3_bmp.mask and calls lv1_did_update_interrupt_mask().
+ */
+
+static void ps3_chip_mask(unsigned int virq)
+{
+	struct ps3_private *pd = get_irq_chip_data(virq);
+	u64 bit = 0x8000000000000000UL >> virq;
+	u64 *p = &pd->bmp.mask;
+	u64 old;
+	unsigned long flags;
+
+	pr_debug("%s:%d: cpu %u, virq %d\n", __func__, __LINE__, pd->cpu, virq);
+
+	local_irq_save(flags);
+	asm volatile(
+		     "1:	ldarx %0,0,%3\n"
+		     "andc	%0,%0,%2\n"
+		     "stdcx.	%0,0,%3\n"
+		     "bne-	1b"
+		     : "=&r" (old), "+m" (*p)
+		     : "r" (bit), "r" (p)
+		     : "cc" );
+
+	lv1_did_update_interrupt_mask(pd->node, pd->cpu);
+	local_irq_restore(flags);
+}
+
+/**
+ * ps3_chip_unmask - Clear an interrupt mask bit in ps3_bmp.
+ * @virq: The assigned Linux virq.
+ *
+ * Clears ps3_bmp.mask and calls lv1_did_update_interrupt_mask().
+ */
+
+static void ps3_chip_unmask(unsigned int virq)
+{
+	struct ps3_private *pd = get_irq_chip_data(virq);
+	u64 bit = 0x8000000000000000UL >> virq;
+	u64 *p = &pd->bmp.mask;
+	u64 old;
+	unsigned long flags;
+
+	pr_debug("%s:%d: cpu %u, virq %d\n", __func__, __LINE__, pd->cpu, virq);
+
+	local_irq_save(flags);
+	asm volatile(
+		     "1:	ldarx %0,0,%3\n"
+		     "or	%0,%0,%2\n"
+		     "stdcx.	%0,0,%3\n"
+		     "bne-	1b"
+		     : "=&r" (old), "+m" (*p)
+		     : "r" (bit), "r" (p)
+		     : "cc" );
+
+	lv1_did_update_interrupt_mask(pd->node, pd->cpu);
+	local_irq_restore(flags);
+}
+
+/**
+ * ps3_chip_eoi - HV end-of-interrupt.
+ * @virq: The assigned Linux virq.
+ *
+ * Calls lv1_end_of_interrupt_ext().
+ */
+
+static void ps3_chip_eoi(unsigned int virq)
+{
+	const struct ps3_private *pd = get_irq_chip_data(virq);
+	lv1_end_of_interrupt_ext(pd->node, pd->cpu, virq);
+}
+
+/**
+ * ps3_irq_chip - Represents the ps3_bmp as a Linux struct irq_chip.
+ */
+
+static struct irq_chip ps3_irq_chip = {
+	.typename = "ps3",
+	.mask = ps3_chip_mask,
+	.unmask = ps3_chip_unmask,
+	.eoi = ps3_chip_eoi,
+};
+
+/**
  * ps3_virq_setup - virq related setup.
  * @cpu: enum ps3_cpu_binding indicating the cpu the interrupt should be
  * serviced on.

@@ -134,6 +220,8 @@ int ps3_virq_setup(enum ps3_cpu_binding 
 		goto fail_set;
 	}
 
+	ps3_chip_mask(*virq);
+
 	return result;
 
 fail_set:

@@ -225,6 +313,8 @@ int ps3_irq_plug_destroy(unsigned int vi
 	pr_debug("%s:%d: node %lu, cpu %d, virq %u\n", __func__, __LINE__,
 		pd->node, pd->cpu, virq);
 
+	ps3_chip_mask(virq);
+
 	result = lv1_disconnect_irq_plug_ext(pd->node, pd->cpu, virq);
 
 	if (result)

@@ -282,7 +372,9 @@ int ps3_event_receive_port_destroy(unsig
 {
 	int result;
 
-	pr_debug(" -> %s:%d virq: %u\n", __func__, __LINE__, virq);
+	pr_debug(" -> %s:%d virq %u\n", __func__, __LINE__, virq);
+
+	ps3_chip_mask(virq);
 
 	result = lv1_destruct_event_receive_port(virq_to_hw(virq));
 

@@ -290,17 +382,13 @@ int ps3_event_receive_port_destroy(unsig
 		pr_debug("%s:%d: lv1_destruct_event_receive_port failed: %s\n",
 			__func__, __LINE__, ps3_result(result));
 
-	/* lv1_destruct_event_receive_port() destroys the IRQ plug,
-	 * so don't call ps3_irq_plug_destroy() here.
+	/* Can't call ps3_virq_destroy() here since ps3_smp_cleanup_cpu()
+	 * calls from interrupt context (smp_call_function).
 	 */
 
-	result = ps3_virq_destroy(virq);
-	BUG_ON(result);
-
 	pr_debug(" <- %s:%d\n", __func__, __LINE__);
 	return result;
 }
-EXPORT_SYMBOL_GPL(ps3_event_receive_port_destroy);
 
 int ps3_send_event_locally(unsigned int virq)
 {

@@ -372,6 +460,13 @@ int ps3_sb_event_receive_port_destroy(co
 	result = ps3_event_receive_port_destroy(virq);
 	BUG_ON(result);
 
+	/* ps3_event_receive_port_destroy() destroys the IRQ plug,
+	 * so don't call ps3_irq_plug_destroy() here.
+	 */
+
+	result = ps3_virq_destroy(virq);
+	BUG_ON(result);
+
 	pr_debug(" <- %s:%d\n", __func__, __LINE__);
 	return result;
 }

@@ -412,16 +507,23 @@ EXPORT_SYMBOL_GPL(ps3_io_irq_setup);
 int ps3_io_irq_destroy(unsigned int virq)
 {
 	int result;
+	unsigned long outlet = virq_to_hw(virq);
 
-	result = lv1_destruct_io_irq_outlet(virq_to_hw(virq));
+	ps3_chip_mask(virq);
 
-	if (result)
-		pr_debug("%s:%d: lv1_destruct_io_irq_outlet failed: %s\n",
-			__func__, __LINE__, ps3_result(result));
+	/* lv1_destruct_io_irq_outlet() will destroy the IRQ plug,
+	 * so call ps3_irq_plug_destroy() first.
+	 */
 
 	result = ps3_irq_plug_destroy(virq);
 	BUG_ON(result);
 
+	result = lv1_destruct_io_irq_outlet(outlet);
+
+	if (result)
+		pr_debug("%s:%d: lv1_destruct_io_irq_outlet failed: %s\n",
+			__func__, __LINE__, ps3_result(result));
+
 	return result;
 }
 EXPORT_SYMBOL_GPL(ps3_io_irq_destroy);

@@ -466,6 +568,7 @@ int ps3_vuart_irq_destroy(unsigned int v
 {
 	int result;
 
+	ps3_chip_mask(virq);
 	result = lv1_deconfigure_virtual_uart_irq();
 
 	if (result) {

@@ -514,9 +617,14 @@ int ps3_spe_irq_setup(enum ps3_cpu_bindi
 
 int ps3_spe_irq_destroy(unsigned int virq)
 {
-	int result = ps3_irq_plug_destroy(virq);
+	int result;
+
+	ps3_chip_mask(virq);
+
+	result = ps3_irq_plug_destroy(virq);
 	BUG_ON(result);
-	return 0;
+
+	return result;
 }
 
 

@@ -565,67 +673,6 @@ static void __maybe_unused _dump_mask(st
 static void dump_bmp(struct ps3_private* pd) {};
 #endif /* defined(DEBUG) */
 
-static void ps3_chip_mask(unsigned int virq)
-{
-	struct ps3_private *pd = get_irq_chip_data(virq);
-	u64 bit = 0x8000000000000000UL >> virq;
-	u64 *p = &pd->bmp.mask;
-	u64 old;
-	unsigned long flags;
-
-	pr_debug("%s:%d: cpu %u, virq %d\n", __func__, __LINE__, pd->cpu, virq);
-
-	local_irq_save(flags);
-	asm volatile(
-		     "1:	ldarx %0,0,%3\n"
-		     "andc	%0,%0,%2\n"
-		     "stdcx.	%0,0,%3\n"
-		     "bne-	1b"
-		     : "=&r" (old), "+m" (*p)
-		     : "r" (bit), "r" (p)
-		     : "cc" );
-
-	lv1_did_update_interrupt_mask(pd->node, pd->cpu);
-	local_irq_restore(flags);
-}
-
-static void ps3_chip_unmask(unsigned int virq)
-{
-	struct ps3_private *pd = get_irq_chip_data(virq);
-	u64 bit = 0x8000000000000000UL >> virq;
-	u64 *p = &pd->bmp.mask;
-	u64 old;
-	unsigned long flags;
-
-	pr_debug("%s:%d: cpu %u, virq %d\n", __func__, __LINE__, pd->cpu, virq);
-
-	local_irq_save(flags);
-	asm volatile(
-		     "1:	ldarx %0,0,%3\n"
-		     "or	%0,%0,%2\n"
-		     "stdcx.	%0,0,%3\n"
-		     "bne-	1b"
-		     : "=&r" (old), "+m" (*p)
-		     : "r" (bit), "r" (p)
-		     : "cc" );
-
-	lv1_did_update_interrupt_mask(pd->node, pd->cpu);
-	local_irq_restore(flags);
-}
-
-static void ps3_chip_eoi(unsigned int virq)
-{
-	const struct ps3_private *pd = get_irq_chip_data(virq);
-	lv1_end_of_interrupt_ext(pd->node, pd->cpu, virq);
-}
-
-static struct irq_chip irq_chip = {
-	.typename = "ps3",
-	.mask = ps3_chip_mask,
-	.unmask = ps3_chip_unmask,
-	.eoi = ps3_chip_eoi,
-};
-
 static void ps3_host_unmap(struct irq_host *h, unsigned int virq)
 {
 	set_irq_chip_data(virq, NULL);

@@ -637,7 +684,7 @@ static int ps3_host_map(struct irq_host 
 	pr_debug("%s:%d: hwirq %lu, virq %u\n", __func__, __LINE__, hwirq,
 		virq);
 
-	set_irq_chip_and_handler(virq, &irq_chip, handle_fasteoi_irq);
+	set_irq_chip_and_handler(virq, &ps3_irq_chip, handle_fasteoi_irq);
 
 	return 0;
 }

@@ -657,7 +704,7 @@ void __init ps3_register_ipi_debug_brk(u
 		cpu, virq, pd->bmp.ipi_debug_brk_mask);
 }
 
-unsigned int ps3_get_irq(void)
+static unsigned int ps3_get_irq(void)
 {
 	struct ps3_private *pd = &__get_cpu_var(ps3_private);
 	u64 x = (pd->bmp.status & pd->bmp.mask);
--- a/arch/powerpc/platforms/ps3/setup.c
+++ b/arch/powerpc/platforms/ps3/setup.c

@@ -209,31 +209,28 @@ static int __init ps3_probe(void)
 #if defined(CONFIG_KEXEC)
 static void ps3_kexec_cpu_down(int crash_shutdown, int secondary)
 {
-	DBG(" -> %s:%d\n", __func__, __LINE__);
+	int result;
+	u64 ppe_id;
+	u64 thread_id = secondary ? 1 : 0;
+
+	DBG(" -> %s:%d: (%d)\n", __func__, __LINE__, secondary);
+	ps3_smp_cleanup_cpu(thread_id);
+
+	lv1_get_logical_ppe_id(&ppe_id);
+	result = lv1_configure_irq_state_bitmap(ppe_id, secondary ? 0 : 1, 0);
 
-	if (secondary) {
-		int cpu;
-		for_each_online_cpu(cpu)
-			if (cpu)
-				ps3_smp_cleanup_cpu(cpu);
-	} else
-		ps3_smp_cleanup_cpu(0);
+	/* seems to fail on second call */
+	DBG("%s:%d: lv1_configure_irq_state_bitmap (%d) %s\n", __func__,
+		__LINE__, secondary, ps3_result(result));
 
 	DBG(" <- %s:%d\n", __func__, __LINE__);
 }
 
 static void ps3_machine_kexec(struct kimage *image)
 {
-	unsigned long ppe_id;
-
 	DBG(" -> %s:%d\n", __func__, __LINE__);
 
-	lv1_get_logical_ppe_id(&ppe_id);
-	lv1_configure_irq_state_bitmap(ppe_id, 0, 0);
-	ps3_mm_shutdown();
-	ps3_mm_vas_destroy();
-
-	default_machine_kexec(image);
+	default_machine_kexec(image); // needs ipi, never returns.
 
 	DBG(" <- %s:%d\n", __func__, __LINE__);
 }

-- 

On Tue, 2007-06-05 at 20:00 -0700, Geoff Levand wrote:

Fixup the core platform parts needed for kexec to work on the PS3.
 - Setup ps3_hpte_clear correctly.
 - Mask interrupts on irq removal.
 - Release all hypervisor resources.

The irq changes might be kexec related, but it's a mess to review. You
seem to moving a bunch of code around in the patch as well.

Do you really want to be calling DBG() here? Hmm, it looks like it
doesn't actually do anything?

How is this different from set_bit() ?  (asm-powerpc/bitops.h)

ps. now that I see you're just moving this code around someone's
probably already asked that question.

Just get rid of ps3_machine_kexec() and hook default_machine_kexec()
directly into your ppc_md.

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

Michael Ellerman wrote:

On Tue, 2007-06-05 at 20:00 -0700, Geoff Levand wrote:

quoted

Fixup the core platform parts needed for kexec to work on the PS3.
 - Setup ps3_hpte_clear correctly.
 - Mask interrupts on irq removal.
 - Release all hypervisor resources.

The irq changes might be kexec related, but it's a mess to review. You
seem to moving a bunch of code around in the patch as well.

Yes, I need to move the static chip_mask routines up so they would be
defined before the irq setup/destroy routines.

quoted

Signed-off-by: Geoff Levand <redacted>
---
 arch/powerpc/platforms/ps3/htab.c      |   14 +-
 arch/powerpc/platforms/ps3/interrupt.c |  199 ++++++++++++++++++++-------------
 arch/powerpc/platforms/ps3/setup.c     |   29 ++--
 3 files changed, 147 insertions(+), 95 deletions(-)

--- a/arch/powerpc/platforms/ps3/htab.c
+++ b/arch/powerpc/platforms/ps3/htab.c

@@ -234,10 +234,18 @@ static void ps3_hpte_invalidate(unsigned
 
 static void ps3_hpte_clear(void)
 {
-	/* Make sure to clean up the frame buffer device first */
-	ps3fb_cleanup();
+	int result;
 
-	lv1_unmap_htab(htab_addr);
+	DBG(" -> %s:%d\n", __func__, __LINE__);
+
+	result = lv1_unmap_htab(htab_addr);
+	BUG_ON(result);
+
+	ps3_mm_shutdown();
+
+	ps3_mm_vas_destroy();
+
+	DBG(" <- %s:%d\n", __func__, __LINE__);
 }

Do you really want to be calling DBG() here? Hmm, it looks like it
doesn't actually do anything?

Sure, it uses udbg_printf, and works OK.

quoted

+static void ps3_chip_mask(unsigned int virq)
+{
+	struct ps3_private *pd = get_irq_chip_data(virq);
+	u64 bit = 0x8000000000000000UL >> virq;
+	u64 *p = &pd->bmp.mask;
+	u64 old;
+	unsigned long flags;
+
+	pr_debug("%s:%d: cpu %u, virq %d\n", __func__, __LINE__, pd->cpu, virq);
+
+	local_irq_save(flags);
+	asm volatile(
+		     "1:	ldarx %0,0,%3\n"
+		     "andc	%0,%0,%2\n"
+		     "stdcx.	%0,0,%3\n"
+		     "bne-	1b"
+		     : "=&r" (old), "+m" (*p)
+		     : "r" (bit), "r" (p)
+		     : "cc" );
+
+	lv1_did_update_interrupt_mask(pd->node, pd->cpu);
+	local_irq_restore(flags);

How is this different from set_bit() ?  (asm-powerpc/bitops.h)

ps. now that I see you're just moving this code around someone's
probably already asked that question.

This was contributed by Ben H as the fastest way.  I think the
reason was that we could minimize the time between local_irq_save
and local_irq_restore?

quoted

 static void ps3_machine_kexec(struct kimage *image)
 {
-	unsigned long ppe_id;
-
 	DBG(" -> %s:%d\n", __func__, __LINE__);
 
-	lv1_get_logical_ppe_id(&ppe_id);
-	lv1_configure_irq_state_bitmap(ppe_id, 0, 0);
-	ps3_mm_shutdown();
-	ps3_mm_vas_destroy();
-
-	default_machine_kexec(image);
+	default_machine_kexec(image); // needs ipi, never returns.

Just get rid of ps3_machine_kexec() and hook default_machine_kexec()
directly into your ppc_md.

Right, just a debugging leftover...


-Geoff

Hi Geoff,

On Wed, 06 Jun 2007 14:55:00 -0700 Geoff Levand [off-list ref] wrote:

Michael Ellerman wrote:

quoted

On Tue, 2007-06-05 at 20:00 -0700, Geoff Levand wrote:

quoted

Fixup the core platform parts needed for kexec to work on the PS3.
 - Setup ps3_hpte_clear correctly.
 - Mask interrupts on irq removal.
 - Release all hypervisor resources.

The irq changes might be kexec related, but it's a mess to review. You
seem to moving a bunch of code around in the patch as well.

Yes, I need to move the static chip_mask routines up so they would be
defined before the irq setup/destroy routines.

I think Michael's point then is that if you submitted a separate
preceding patch that just moves stuff around without any other changes
(if at all possible) then reviewing the changes in this would be much
easier (and our confidence would be higher).

-- 
Cheers,
Stephen Rothwell                    sfr@canb.auug.org.au
http://www.canb.auug.org.au/~sfr/

Stephen Rothwell wrote:

Hi Geoff,

On Wed, 06 Jun 2007 14:55:00 -0700 Geoff Levand [off-list ref] wrote:

quoted

Michael Ellerman wrote:

quoted

On Tue, 2007-06-05 at 20:00 -0700, Geoff Levand wrote:

quoted

Fixup the core platform parts needed for kexec to work on the PS3.
 - Setup ps3_hpte_clear correctly.
 - Mask interrupts on irq removal.
 - Release all hypervisor resources.

The irq changes might be kexec related, but it's a mess to review. You
seem to moving a bunch of code around in the patch as well.

Yes, I need to move the static chip_mask routines up so they would be
defined before the irq setup/destroy routines.

I think Michael's point then is that if you submitted a separate
preceding patch that just moves stuff around without any other changes
(if at all possible) then reviewing the changes in this would be much
easier (and our confidence would be higher).

Yes, sorry, I understood.  I've already split it up for the next round.

-Geoff

On Wed, 2007-06-06 at 14:55 -0700, Geoff Levand wrote:

Michael Ellerman wrote:

quoted

quoted

--- a/arch/powerpc/platforms/ps3/htab.c
+++ b/arch/powerpc/platforms/ps3/htab.c

@@ -234,10 +234,18 @@ static void ps3_hpte_invalidate(unsigned
 
 static void ps3_hpte_clear(void)
 {
-	/* Make sure to clean up the frame buffer device first */
-	ps3fb_cleanup();
+	int result;
 
-	lv1_unmap_htab(htab_addr);
+	DBG(" -> %s:%d\n", __func__, __LINE__);
+
+	result = lv1_unmap_htab(htab_addr);
+	BUG_ON(result);
+
+	ps3_mm_shutdown();
+
+	ps3_mm_vas_destroy();
+
+	DBG(" <- %s:%d\n", __func__, __LINE__);
 }

Do you really want to be calling DBG() here? Hmm, it looks like it
doesn't actually do anything?

Sure, it uses udbg_printf, and works OK.

Which is hooked up to what?

quoted

quoted

+static void ps3_chip_mask(unsigned int virq)
+{
+	struct ps3_private *pd = get_irq_chip_data(virq);
+	u64 bit = 0x8000000000000000UL >> virq;
+	u64 *p = &pd->bmp.mask;
+	u64 old;
+	unsigned long flags;
+
+	pr_debug("%s:%d: cpu %u, virq %d\n", __func__, __LINE__, pd->cpu, virq);
+
+	local_irq_save(flags);
+	asm volatile(
+		     "1:	ldarx %0,0,%3\n"
+		     "andc	%0,%0,%2\n"
+		     "stdcx.	%0,0,%3\n"
+		     "bne-	1b"
+		     : "=&r" (old), "+m" (*p)
+		     : "r" (bit), "r" (p)
+		     : "cc" );
+
+	lv1_did_update_interrupt_mask(pd->node, pd->cpu);
+	local_irq_restore(flags);

How is this different from set_bit() ?  (asm-powerpc/bitops.h)

ps. now that I see you're just moving this code around someone's
probably already asked that question.

This was contributed by Ben H as the fastest way.  I think the
reason was that we could minimize the time between local_irq_save
and local_irq_restore?

To save ~10 instructions? Followed by a HV call? I guess Ben has some
really good benchmark data to support it ;)

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

On Wed, 06 Jun 2007 18:33:05 -0700 Geoff Levand [off-list ref] wrote:

Yes, sorry, I understood.  I've already split it up for the next round.

Excellent, thanks.

-- 
Cheers,
Stephen Rothwell                    sfr@canb.auug.org.au
http://www.canb.auug.org.au/~sfr/

On Thu, 2007-06-07 at 12:31 +1000, Michael Ellerman wrote:

quoted

quoted

quoted

+  pr_debug("%s:%d: cpu %u, virq %d\n", __func__, __LINE__,

pd->cpu, virq);

quoted

quoted

quoted

+
+  local_irq_save(flags);
+  asm volatile(
+               "1:        ldarx %0,0,%3\n"
+               "andc      %0,%0,%2\n"
+               "stdcx.    %0,0,%3\n"
+               "bne-      1b"
+               : "=&r" (old), "+m" (*p)
+               : "r" (bit), "r" (p)
+               : "cc" );
+
+  lv1_did_update_interrupt_mask(pd->node, pd->cpu);
+  local_irq_restore(flags);

How is this different from set_bit() ?  (asm-powerpc/bitops.h)

ps. now that I see you're just moving this code around someone's
probably already asked that question. 

Actually the above is clear_bit :-)

That would thus be something around the lines of:

 clear_bit(&pd->bmp.mask, 63 - virq);

Geoff, feel free to replace it with the clear_bit if you think it's
nicer that way :-) (and double check I didn't smoke crack when doing
the above conversion).

The other one in unmask would be set_bit() of course.

Ben.

On Wed Jun 6 13:00:15 EST 2007, Geoff Levand wrote:

I'm glad to see this go.  Which patch added the call to the driver?

+       int result;

-       lv1_unmap_htab(htab_addr);
+       DBG(" -> %s:%d\n", __func__, __LINE__);
+
+       result = lv1_unmap_htab(htab_addr);
+       BUG_ON(result);
+
+       ps3_mm_shutdown();
+
+       ps3_mm_vas_destroy();

I tried to look at these to check that nothing dynamically allocated 
was being touched.   I didn't find anything if the memory had been 
hot-unplugged, but it also looked like they skipped the last one.

+
+       DBG(" <- %s:%d\n", __func__, __LINE__);
 }

 void __init ps3_hpte_init(unsigned long htab_size)

[skipped interrupt.c changes]

This is wrong.   This is not what secondary means.  To get the 
thread_id you must use smp_processor_id for logical or 
hard_smp_processor_id() for the hardware thread id.

+
+       DBG(" -> %s:%d: (%d)\n", __func__, __LINE__, secondary);
+       ps3_smp_cleanup_cpu(thread_id);
+
+       lv1_get_logical_ppe_id(&ppe_id);
+       result = lv1_configure_irq_state_bitmap(ppe_id, secondary ? 0 
: 1, 0);

As the second argument is thread id, again this is wrong.

-       if (secondary) {
-               int cpu;
-               for_each_online_cpu(cpu)
-                       if (cpu)
-                               ps3_smp_cleanup_cpu(cpu);
-       } else
-               ps3_smp_cleanup_cpu(0);
+       /* seems to fail on second call */
+       DBG("%s:%d: lv1_configure_irq_state_bitmap (%d) %s\n", 
__func__,
+               __LINE__, secondary, ps3_result(result));

        DBG(" <- %s:%d\n", __func__, __LINE__);
 }

Once linux is running, all processors are identical.  That is the S in 
SMP.   However, during kernel boot, we need one cpu to be running and 
the others to wait until the path is prepared.  Since kexec effectively 
leads to a boot, one cpu becomes known as the boot cpu and the rest 
become secondary cpus.

There are two paths to enter the kexec code: the panic code, and the 
shutdown/reboot syscall.  For normal kexec, whatever cpu thread is 
running the user process when it makes the reboot system call will be 
the master.  For crash kexec, its whichever thread called panic.

The secondary flag to cpu_down exists because the secondary cpus will 
call it in ipi context but will not return to the irq layer to eoi the 
ipi.  The call to cpu_down is made from kexec_smp_down initiated via 
the smp_call_function ipi context but instead of returning, 
kexec_smp_down calls kexec_smp_wait which will mark the paca, switch to 
real mode and spin with the hardware thread in r3 until the master 
tells them its done copying the kernel, when it will jump to address 
0x60.

The code in default_machine_kexec calls kexec_prepare_cpus which uses 
smp_call_function to ipi the other cpus and have them call 
kexec_cpu_down.  After the secondaries have marked their paca, cpu_down 
will be called on the master with the secondary arg 0.  During this 
call all other cpus are spinning.  After this call, the cpu will switch 
to a statically allocated stack and copy the new image pages into 
place, destroying any dynamically allocated and per-cpu data.  It then 
calls switches to real mode and calls the htab_clear hook to tear down 
the page tables, leaving a clean state for the new kernel.  When 
finished it copies 256 bytes from the entry point to address 0 and 
tells any slaves to branch to 0x60.  It then branches to the entry 
point (not address 0) with r3 containing its hardware cpu id, r4 
containing the entry address, and r5 containing 0.

When using kexec-tools, the entry point in v2wrap.S stores the master 
cpu id, calls the generic C code to checksum the image, then stores the 
master cpu id as the boot cpu in the device tree header, loads r3 with 
the device tree, and enters the new kernel.  (This adjusts for the 
difference between leaving the kernel, where cpu id is in r3, and 
entering the kernel, which expects a pointer to the device tree.   The 
kexec_load syscall just supplies memory contents and the entry point; 
the design is that any registers needed by the new code are to be set 
by a trampoline added to the list of image segments by user space.  The 
master cpu is not known until kexec is initiated and therefore is 
passed in the r3 (the very existence of the device-tree structure is 
only known to user space, not passed to the system call); the 
specification of r4 and r5 for the master thread is for convenience)

Since there is no handoff to say the slave noticed that the master was 
done copying the image, I have submitted a kernel patch to release the 
slaves to the new kernel's wait code entry point at 0x60 before calling 
the htab_clear routine, giving them the time that the htab_clear 
function executes in addition to the time for the code in purgatory.  
The patch \to copy the payload kernel's spin loop instead of creating 
another loop and sync gate is in kexec-testing.

Note that the order describe above is for the 64 bit PowerPC port; most 
architectures switch to real mode, flash invalidate the mmu and copy 
the new kernel in real mode using an relocatable assembly routine 
running at a location chosen by the kernel (a page that is neither an 
image source or destination page).   The LPAR real mode limitations 
deem this impractical; instead we reserve the kernel text, data, and 
bss space, the mmu hash table (in non-lpar mode), and any tce tables.  
If the execed image was a kernel, it will copy itself to its linked 
location as it must when started from open firmware.

 static void ps3_machine_kexec(struct kimage *image)
 {
-       unsigned long ppe_id;
-
        DBG(" -> %s:%d\n", __func__, __LINE__);

-       lv1_get_logical_ppe_id(&ppe_id);
-       lv1_configure_irq_state_bitmap(ppe_id, 0, 0);
-       ps3_mm_shutdown();
-       ps3_mm_vas_destroy();
-
-       default_machine_kexec(image);
+       default_machine_kexec(image); // needs ipi, never returns.

        DBG(" <- %s:%d\n", __func__, __LINE__);
 }

Others noted this now passthough function can be eliminated.

milton

Hi Milton.

Milton Miller wrote:

On Wed Jun 6 13:00:15 EST 2007, Geoff Levand wrote:

quoted

Fixup the core platform parts needed for kexec to work on the PS3.
 - Setup ps3_hpte_clear correctly.
 - Mask interrupts on irq removal.
 - Release all hypervisor resources.

quoted

 static void ps3_hpte_clear(void)
 {
-       /* Make sure to clean up the frame buffer device first */
-       ps3fb_cleanup();

I'm glad to see this go.  Which patch added the call to the driver?

I don't know the exact history, but I am pretty sure that is a left
over from before the framebuffer driver used the dma support now
provided by the ps3_system_bus.  The old fb code managed its own
IOPTE's, and I think this call cleaned those.  But as Geert was
re-writing the fb driver ps3fb_cleanup became a place to put general
fb shutdown code.  At the time we were working to just get it to boot
and run, and had no concern what happened at shutdown let alone
kexec.  It wasn't like someone consciously made a single change to
do driver shutdown here, it just was a result of the churn.

quoted

+       int result;

-       lv1_unmap_htab(htab_addr);
+       DBG(" -> %s:%d\n", __func__, __LINE__);
+
+       result = lv1_unmap_htab(htab_addr);
+       BUG_ON(result);
+
+       ps3_mm_shutdown();
+
+       ps3_mm_vas_destroy();

I tried to look at these to check that nothing dynamically allocated 
was being touched.   I didn't find anything if the memory had been 
hot-unplugged, but it also looked like they skipped the last one.

By 'last one' I guess you mean the rm region (map.rm).  That is
the 'real mode' boot mem region.  It is allocated by the hypervisor
for the life of the lpar.  It's not hot-pluggable.

quoted

@@ -209,31 +209,28 @@ static int __init ps3_probe(void)
 #if defined(CONFIG_KEXEC)
 static void ps3_kexec_cpu_down(int crash_shutdown, int secondary)
 {
-       DBG(" -> %s:%d\n", __func__, __LINE__);
+       int result;
+       u64 ppe_id;
+       u64 thread_id = secondary ? 1 : 0;

This is wrong.   This is not what secondary means.  To get the 
thread_id you must use smp_processor_id for logical or 
hard_smp_processor_id() for the hardware thread id.

quoted

+       DBG(" -> %s:%d: (%d)\n", __func__, __LINE__, secondary);
+       ps3_smp_cleanup_cpu(thread_id);
+
+       lv1_get_logical_ppe_id(&ppe_id);
+       result = lv1_configure_irq_state_bitmap(ppe_id, secondary ? 0 
: 1, 0);
+       /* seems to fail on second call */
+       DBG("%s:%d: lv1_configure_irq_state_bitmap (%d) %s\n", 

As the second argument is thread id, again this is wrong.

OK, I setup a new routine ps3_shutdown_IRQ() to mirror ps3_init_IRQ().
ps3_shutdown_IRQ() uses the hard processor id.

Once linux is running, all processors are identical.  That is the S in 

...

location as it must when started from open firmware.

Thanks for the explanation.  I wish I had that before starting this
work, but it still explained a few points I still wasn't clear on.
I think we should put it somewhere formal like the kernel source
Documentntation directory.  What do you think?

-Geoff

Benjamin Herrenschmidt wrote:

On Thu, 2007-06-07 at 12:31 +1000, Michael Ellerman wrote:

quoted

quoted

quoted

quoted

+  pr_debug("%s:%d: cpu %u, virq %d\n", __func__, __LINE__,

pd->cpu, virq);

quoted

quoted

quoted

+
+  local_irq_save(flags);
+  asm volatile(
+               "1:        ldarx %0,0,%3\n"
+               "andc      %0,%0,%2\n"
+               "stdcx.    %0,0,%3\n"
+               "bne-      1b"
+               : "=&r" (old), "+m" (*p)
+               : "r" (bit), "r" (p)
+               : "cc" );
+
+  lv1_did_update_interrupt_mask(pd->node, pd->cpu);
+  local_irq_restore(flags);

How is this different from set_bit() ?  (asm-powerpc/bitops.h)

ps. now that I see you're just moving this code around someone's
probably already asked that question. 

Actually the above is clear_bit :-)

That would thus be something around the lines of:

 clear_bit(&pd->bmp.mask, 63 - virq);

Geoff, feel free to replace it with the clear_bit if you think it's
nicer that way :-) (and double check I didn't smoke crack when doing
the above conversion).

The other one in unmask would be set_bit() of course.

Almost right.  Needed clear_bit(63 - virq, &pd->bmp.mask).
I added it in.

-Geoff