Re: [v2 2/5] arm64: kdump: implement machine_crash_shutdown()

2015-08-06 Thread AKASHI Takahiro

Marc,

On 08/07/2015 12:51 AM, Marc Zyngier wrote:

Hi,

On 06/08/15 08:09, AKASHI Takahiro wrote:

Marc, Mark

Sorry for not revisiting your comment below for a while.


Wow. It took me a few minutes to page the context back in.


Please don't purge the page from your cache for a while :)


On 04/24/2015 07:43 PM, Marc Zyngier wrote:

On 24/04/15 11:39, Mark Rutland wrote:

On Fri, Apr 24, 2015 at 08:53:05AM +0100, AKASHI Takahiro wrote:

kdump calls machine_crash_shutdown() to shut down non-boot cpus and
save per-cpu general-purpose registers before restarting the crash dump
kernel. See kernel_kexec().
ipi_cpu_stop() is used and a bit modified to support this behavior.

Signed-off-by: AKASHI Takahiro 
---
   arch/arm64/include/asm/kexec.h|   34 ++-
   arch/arm64/kernel/machine_kexec.c |   55 
-
   arch/arm64/kernel/smp.c   |   12 ++--
   3 files changed, 97 insertions(+), 4 deletions(-)

diff --git a/arch/arm64/include/asm/kexec.h b/arch/arm64/include/asm/kexec.h
index 3530ff5..eaf3fcb 100644
--- a/arch/arm64/include/asm/kexec.h
+++ b/arch/arm64/include/asm/kexec.h
@@ -30,6 +30,8 @@

   #if !defined(__ASSEMBLY__)

+extern bool in_crash_kexec;
+
   /**
* crash_setup_regs() - save registers for the panic kernel
*
@@ -40,7 +42,37 @@
   static inline void crash_setup_regs(struct pt_regs *newregs,
struct pt_regs *oldregs)
   {
-   /* Empty routine needed to avoid build errors. */
+   if (oldregs) {
+   memcpy(newregs, oldregs, sizeof(*newregs));
+   } else {
+   __asm__ __volatile__ (
+   "stpx0,   x1, [%3]\n\t"


Why the tabs?

Please use #16 * N as the offset for consistency with entry.S, with 0
for the first N.

[...]


+static void machine_kexec_mask_interrupts(void)
+{
+   unsigned int i;
+   struct irq_desc *desc;
+
+   for_each_irq_desc(i, desc) {
+   struct irq_chip *chip;
+
+   chip = irq_desc_get_chip(desc);
+   if (!chip)
+   continue;
+
+   if (chip->irq_eoi && irqd_irq_inprogress(>irq_data))
+   chip->irq_eoi(>irq_data);
+
+   if (chip->irq_mask)
+   chip->irq_mask(>irq_data);
+
+   if (chip->irq_disable && !irqd_irq_disabled(>irq_data))
+   chip->irq_disable(>irq_data);
+   }
+}


I'm surprised that this isn't left to the irqchip driver init code in
the crash kernel. For all we know this state could be corrupt anyway.


Indeed, parsing the irqdesc list is a recipe for disaster. Who knows
which locks have been taken or simply corrupted, pointers nuked...


Is there any reason we can't get the GIC driver to nuke all of this at
probe time?


Is it just enough to remove kexec_mask_interrupts() and add gic_eoi_irq()
at the beginning of gic_cpu_init() in irq-gic.c and irq-gic-v3.c?


No, doing an EOI is definitely the wrong thing to do. If you do it in
the wrong order, you just screw up the GIC state machine. Plus, you have
no idea what to write there...

The only real solution is to zero the "active" registers.


This feels like the better option. I can cook a patch or two for that.


If you do, that will be much better :)


OK, I'll prepare something that we can merge at the same time kexec
comes back from the dead (if it ever does - I'm not holding my breath).


Thank you.
Please note that the same function, machine_kexec_mask_interrupts(),
is already there on arm(/kernel/machine_kexec.c).

Well, kexec/kdump stuff is not dead.
Hopefully I and Geoff will submit a full series of patchset in a few weeks
although the main logic will be the same.



BTW, in arm-gic-v3.h, GICD_CTRL_ARE_NS is defined as
  (1U << 4)
but should it be 5?
(I'm referring to the page 8-415 in IHI0069A.)


No, look at the definition ARE_NS has when the access is non-secure or
on a system supporting a single security state. The definition you're
referring to is for a secure access (firmware).


Aha, I should remember that arm has "multiple personalities."

-Takahiro AKASHI


Thanks,

M.


--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/


Re: [v2 2/5] arm64: kdump: implement machine_crash_shutdown()

2015-08-06 Thread Marc Zyngier
Hi,

On 06/08/15 08:09, AKASHI Takahiro wrote:
> Marc, Mark
> 
> Sorry for not revisiting your comment below for a while.

Wow. It took me a few minutes to page the context back in.

> On 04/24/2015 07:43 PM, Marc Zyngier wrote:
>> On 24/04/15 11:39, Mark Rutland wrote:
>>> On Fri, Apr 24, 2015 at 08:53:05AM +0100, AKASHI Takahiro wrote:
 kdump calls machine_crash_shutdown() to shut down non-boot cpus and
 save per-cpu general-purpose registers before restarting the crash dump
 kernel. See kernel_kexec().
 ipi_cpu_stop() is used and a bit modified to support this behavior.

 Signed-off-by: AKASHI Takahiro 
 ---
   arch/arm64/include/asm/kexec.h|   34 ++-
   arch/arm64/kernel/machine_kexec.c |   55 
 -
   arch/arm64/kernel/smp.c   |   12 ++--
   3 files changed, 97 insertions(+), 4 deletions(-)

 diff --git a/arch/arm64/include/asm/kexec.h 
 b/arch/arm64/include/asm/kexec.h
 index 3530ff5..eaf3fcb 100644
 --- a/arch/arm64/include/asm/kexec.h
 +++ b/arch/arm64/include/asm/kexec.h
 @@ -30,6 +30,8 @@

   #if !defined(__ASSEMBLY__)

 +extern bool in_crash_kexec;
 +
   /**
* crash_setup_regs() - save registers for the panic kernel
*
 @@ -40,7 +42,37 @@
   static inline void crash_setup_regs(struct pt_regs *newregs,
struct pt_regs *oldregs)
   {
 -  /* Empty routine needed to avoid build errors. */
 +  if (oldregs) {
 +  memcpy(newregs, oldregs, sizeof(*newregs));
 +  } else {
 +  __asm__ __volatile__ (
 +  "stp x0,   x1, [%3]\n\t"
>>>
>>> Why the tabs?
>>>
>>> Please use #16 * N as the offset for consistency with entry.S, with 0
>>> for the first N.
>>>
>>> [...]
>>>
 +static void machine_kexec_mask_interrupts(void)
 +{
 +  unsigned int i;
 +  struct irq_desc *desc;
 +
 +  for_each_irq_desc(i, desc) {
 +  struct irq_chip *chip;
 +
 +  chip = irq_desc_get_chip(desc);
 +  if (!chip)
 +  continue;
 +
 +  if (chip->irq_eoi && irqd_irq_inprogress(>irq_data))
 +  chip->irq_eoi(>irq_data);
 +
 +  if (chip->irq_mask)
 +  chip->irq_mask(>irq_data);
 +
 +  if (chip->irq_disable && !irqd_irq_disabled(>irq_data))
 +  chip->irq_disable(>irq_data);
 +  }
 +}
>>>
>>> I'm surprised that this isn't left to the irqchip driver init code in
>>> the crash kernel. For all we know this state could be corrupt anyway.
>>
>> Indeed, parsing the irqdesc list is a recipe for disaster. Who knows
>> which locks have been taken or simply corrupted, pointers nuked...
>>
>>> Is there any reason we can't get the GIC driver to nuke all of this at
>>> probe time?
> 
> Is it just enough to remove kexec_mask_interrupts() and add gic_eoi_irq()
> at the beginning of gic_cpu_init() in irq-gic.c and irq-gic-v3.c?

No, doing an EOI is definitely the wrong thing to do. If you do it in
the wrong order, you just screw up the GIC state machine. Plus, you have
no idea what to write there...

The only real solution is to zero the "active" registers.

>> This feels like the better option. I can cook a patch or two for that.
> 
> If you do, that will be much better :)

OK, I'll prepare something that we can merge at the same time kexec
comes back from the dead (if it ever does - I'm not holding my breath).

> 
> BTW, in arm-gic-v3.h, GICD_CTRL_ARE_NS is defined as
>  (1U << 4)
> but should it be 5?
> (I'm referring to the page 8-415 in IHI0069A.)

No, look at the definition ARE_NS has when the access is non-secure or
on a system supporting a single security state. The definition you're
referring to is for a secure access (firmware).

Thanks,

M.
-- 
Jazz is not dead. It just smells funny...
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/


Re: [v2 2/5] arm64: kdump: implement machine_crash_shutdown()

2015-08-06 Thread AKASHI Takahiro

Marc, Mark

Sorry for not revisiting your comment below for a while.

On 04/24/2015 07:43 PM, Marc Zyngier wrote:

On 24/04/15 11:39, Mark Rutland wrote:

On Fri, Apr 24, 2015 at 08:53:05AM +0100, AKASHI Takahiro wrote:

kdump calls machine_crash_shutdown() to shut down non-boot cpus and
save per-cpu general-purpose registers before restarting the crash dump
kernel. See kernel_kexec().
ipi_cpu_stop() is used and a bit modified to support this behavior.

Signed-off-by: AKASHI Takahiro 
---
  arch/arm64/include/asm/kexec.h|   34 ++-
  arch/arm64/kernel/machine_kexec.c |   55 -
  arch/arm64/kernel/smp.c   |   12 ++--
  3 files changed, 97 insertions(+), 4 deletions(-)

diff --git a/arch/arm64/include/asm/kexec.h b/arch/arm64/include/asm/kexec.h
index 3530ff5..eaf3fcb 100644
--- a/arch/arm64/include/asm/kexec.h
+++ b/arch/arm64/include/asm/kexec.h
@@ -30,6 +30,8 @@

  #if !defined(__ASSEMBLY__)

+extern bool in_crash_kexec;
+
  /**
   * crash_setup_regs() - save registers for the panic kernel
   *
@@ -40,7 +42,37 @@
  static inline void crash_setup_regs(struct pt_regs *newregs,
struct pt_regs *oldregs)
  {
-   /* Empty routine needed to avoid build errors. */
+   if (oldregs) {
+   memcpy(newregs, oldregs, sizeof(*newregs));
+   } else {
+   __asm__ __volatile__ (
+   "stpx0,   x1, [%3]\n\t"


Why the tabs?

Please use #16 * N as the offset for consistency with entry.S, with 0
for the first N.

[...]


+static void machine_kexec_mask_interrupts(void)
+{
+   unsigned int i;
+   struct irq_desc *desc;
+
+   for_each_irq_desc(i, desc) {
+   struct irq_chip *chip;
+
+   chip = irq_desc_get_chip(desc);
+   if (!chip)
+   continue;
+
+   if (chip->irq_eoi && irqd_irq_inprogress(>irq_data))
+   chip->irq_eoi(>irq_data);
+
+   if (chip->irq_mask)
+   chip->irq_mask(>irq_data);
+
+   if (chip->irq_disable && !irqd_irq_disabled(>irq_data))
+   chip->irq_disable(>irq_data);
+   }
+}


I'm surprised that this isn't left to the irqchip driver init code in
the crash kernel. For all we know this state could be corrupt anyway.


Indeed, parsing the irqdesc list is a recipe for disaster. Who knows
which locks have been taken or simply corrupted, pointers nuked...


Is there any reason we can't get the GIC driver to nuke all of this at
probe time?


Is it just enough to remove kexec_mask_interrupts() and add gic_eoi_irq()
at the beginning of gic_cpu_init() in irq-gic.c and irq-gic-v3.c?


This feels like the better option. I can cook a patch or two for that.


If you do, that will be much better :)

BTW, in arm-gic-v3.h, GICD_CTRL_ARE_NS is defined as
(1U << 4)
but should it be 5?
(I'm referring to the page 8-415 in IHI0069A.)

Thanks,
-Takahiro AKASHI


Thanks,

M.


--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/


Re: [v2 2/5] arm64: kdump: implement machine_crash_shutdown()

2015-08-06 Thread AKASHI Takahiro

Marc,

On 08/07/2015 12:51 AM, Marc Zyngier wrote:

Hi,

On 06/08/15 08:09, AKASHI Takahiro wrote:

Marc, Mark

Sorry for not revisiting your comment below for a while.


Wow. It took me a few minutes to page the context back in.


Please don't purge the page from your cache for a while :)


On 04/24/2015 07:43 PM, Marc Zyngier wrote:

On 24/04/15 11:39, Mark Rutland wrote:

On Fri, Apr 24, 2015 at 08:53:05AM +0100, AKASHI Takahiro wrote:

kdump calls machine_crash_shutdown() to shut down non-boot cpus and
save per-cpu general-purpose registers before restarting the crash dump
kernel. See kernel_kexec().
ipi_cpu_stop() is used and a bit modified to support this behavior.

Signed-off-by: AKASHI Takahiro takahiro.aka...@linaro.org
---
   arch/arm64/include/asm/kexec.h|   34 ++-
   arch/arm64/kernel/machine_kexec.c |   55 
-
   arch/arm64/kernel/smp.c   |   12 ++--
   3 files changed, 97 insertions(+), 4 deletions(-)

diff --git a/arch/arm64/include/asm/kexec.h b/arch/arm64/include/asm/kexec.h
index 3530ff5..eaf3fcb 100644
--- a/arch/arm64/include/asm/kexec.h
+++ b/arch/arm64/include/asm/kexec.h
@@ -30,6 +30,8 @@

   #if !defined(__ASSEMBLY__)

+extern bool in_crash_kexec;
+
   /**
* crash_setup_regs() - save registers for the panic kernel
*
@@ -40,7 +42,37 @@
   static inline void crash_setup_regs(struct pt_regs *newregs,
struct pt_regs *oldregs)
   {
-   /* Empty routine needed to avoid build errors. */
+   if (oldregs) {
+   memcpy(newregs, oldregs, sizeof(*newregs));
+   } else {
+   __asm__ __volatile__ (
+   stpx0,   x1, [%3]\n\t


Why the tabs?

Please use #16 * N as the offset for consistency with entry.S, with 0
for the first N.

[...]


+static void machine_kexec_mask_interrupts(void)
+{
+   unsigned int i;
+   struct irq_desc *desc;
+
+   for_each_irq_desc(i, desc) {
+   struct irq_chip *chip;
+
+   chip = irq_desc_get_chip(desc);
+   if (!chip)
+   continue;
+
+   if (chip-irq_eoi  irqd_irq_inprogress(desc-irq_data))
+   chip-irq_eoi(desc-irq_data);
+
+   if (chip-irq_mask)
+   chip-irq_mask(desc-irq_data);
+
+   if (chip-irq_disable  !irqd_irq_disabled(desc-irq_data))
+   chip-irq_disable(desc-irq_data);
+   }
+}


I'm surprised that this isn't left to the irqchip driver init code in
the crash kernel. For all we know this state could be corrupt anyway.


Indeed, parsing the irqdesc list is a recipe for disaster. Who knows
which locks have been taken or simply corrupted, pointers nuked...


Is there any reason we can't get the GIC driver to nuke all of this at
probe time?


Is it just enough to remove kexec_mask_interrupts() and add gic_eoi_irq()
at the beginning of gic_cpu_init() in irq-gic.c and irq-gic-v3.c?


No, doing an EOI is definitely the wrong thing to do. If you do it in
the wrong order, you just screw up the GIC state machine. Plus, you have
no idea what to write there...

The only real solution is to zero the active registers.


This feels like the better option. I can cook a patch or two for that.


If you do, that will be much better :)


OK, I'll prepare something that we can merge at the same time kexec
comes back from the dead (if it ever does - I'm not holding my breath).


Thank you.
Please note that the same function, machine_kexec_mask_interrupts(),
is already there on arm(/kernel/machine_kexec.c).

Well, kexec/kdump stuff is not dead.
Hopefully I and Geoff will submit a full series of patchset in a few weeks
although the main logic will be the same.



BTW, in arm-gic-v3.h, GICD_CTRL_ARE_NS is defined as
  (1U  4)
but should it be 5?
(I'm referring to the page 8-415 in IHI0069A.)


No, look at the definition ARE_NS has when the access is non-secure or
on a system supporting a single security state. The definition you're
referring to is for a secure access (firmware).


Aha, I should remember that arm has multiple personalities.

-Takahiro AKASHI


Thanks,

M.


--
To unsubscribe from this list: send the line unsubscribe linux-kernel in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/


Re: [v2 2/5] arm64: kdump: implement machine_crash_shutdown()

2015-08-06 Thread AKASHI Takahiro

Marc, Mark

Sorry for not revisiting your comment below for a while.

On 04/24/2015 07:43 PM, Marc Zyngier wrote:

On 24/04/15 11:39, Mark Rutland wrote:

On Fri, Apr 24, 2015 at 08:53:05AM +0100, AKASHI Takahiro wrote:

kdump calls machine_crash_shutdown() to shut down non-boot cpus and
save per-cpu general-purpose registers before restarting the crash dump
kernel. See kernel_kexec().
ipi_cpu_stop() is used and a bit modified to support this behavior.

Signed-off-by: AKASHI Takahiro takahiro.aka...@linaro.org
---
  arch/arm64/include/asm/kexec.h|   34 ++-
  arch/arm64/kernel/machine_kexec.c |   55 -
  arch/arm64/kernel/smp.c   |   12 ++--
  3 files changed, 97 insertions(+), 4 deletions(-)

diff --git a/arch/arm64/include/asm/kexec.h b/arch/arm64/include/asm/kexec.h
index 3530ff5..eaf3fcb 100644
--- a/arch/arm64/include/asm/kexec.h
+++ b/arch/arm64/include/asm/kexec.h
@@ -30,6 +30,8 @@

  #if !defined(__ASSEMBLY__)

+extern bool in_crash_kexec;
+
  /**
   * crash_setup_regs() - save registers for the panic kernel
   *
@@ -40,7 +42,37 @@
  static inline void crash_setup_regs(struct pt_regs *newregs,
struct pt_regs *oldregs)
  {
-   /* Empty routine needed to avoid build errors. */
+   if (oldregs) {
+   memcpy(newregs, oldregs, sizeof(*newregs));
+   } else {
+   __asm__ __volatile__ (
+   stpx0,   x1, [%3]\n\t


Why the tabs?

Please use #16 * N as the offset for consistency with entry.S, with 0
for the first N.

[...]


+static void machine_kexec_mask_interrupts(void)
+{
+   unsigned int i;
+   struct irq_desc *desc;
+
+   for_each_irq_desc(i, desc) {
+   struct irq_chip *chip;
+
+   chip = irq_desc_get_chip(desc);
+   if (!chip)
+   continue;
+
+   if (chip-irq_eoi  irqd_irq_inprogress(desc-irq_data))
+   chip-irq_eoi(desc-irq_data);
+
+   if (chip-irq_mask)
+   chip-irq_mask(desc-irq_data);
+
+   if (chip-irq_disable  !irqd_irq_disabled(desc-irq_data))
+   chip-irq_disable(desc-irq_data);
+   }
+}


I'm surprised that this isn't left to the irqchip driver init code in
the crash kernel. For all we know this state could be corrupt anyway.


Indeed, parsing the irqdesc list is a recipe for disaster. Who knows
which locks have been taken or simply corrupted, pointers nuked...


Is there any reason we can't get the GIC driver to nuke all of this at
probe time?


Is it just enough to remove kexec_mask_interrupts() and add gic_eoi_irq()
at the beginning of gic_cpu_init() in irq-gic.c and irq-gic-v3.c?


This feels like the better option. I can cook a patch or two for that.


If you do, that will be much better :)

BTW, in arm-gic-v3.h, GICD_CTRL_ARE_NS is defined as
(1U  4)
but should it be 5?
(I'm referring to the page 8-415 in IHI0069A.)

Thanks,
-Takahiro AKASHI


Thanks,

M.


--
To unsubscribe from this list: send the line unsubscribe linux-kernel in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/


Re: [v2 2/5] arm64: kdump: implement machine_crash_shutdown()

2015-08-06 Thread Marc Zyngier
Hi,

On 06/08/15 08:09, AKASHI Takahiro wrote:
 Marc, Mark
 
 Sorry for not revisiting your comment below for a while.

Wow. It took me a few minutes to page the context back in.

 On 04/24/2015 07:43 PM, Marc Zyngier wrote:
 On 24/04/15 11:39, Mark Rutland wrote:
 On Fri, Apr 24, 2015 at 08:53:05AM +0100, AKASHI Takahiro wrote:
 kdump calls machine_crash_shutdown() to shut down non-boot cpus and
 save per-cpu general-purpose registers before restarting the crash dump
 kernel. See kernel_kexec().
 ipi_cpu_stop() is used and a bit modified to support this behavior.

 Signed-off-by: AKASHI Takahiro takahiro.aka...@linaro.org
 ---
   arch/arm64/include/asm/kexec.h|   34 ++-
   arch/arm64/kernel/machine_kexec.c |   55 
 -
   arch/arm64/kernel/smp.c   |   12 ++--
   3 files changed, 97 insertions(+), 4 deletions(-)

 diff --git a/arch/arm64/include/asm/kexec.h 
 b/arch/arm64/include/asm/kexec.h
 index 3530ff5..eaf3fcb 100644
 --- a/arch/arm64/include/asm/kexec.h
 +++ b/arch/arm64/include/asm/kexec.h
 @@ -30,6 +30,8 @@

   #if !defined(__ASSEMBLY__)

 +extern bool in_crash_kexec;
 +
   /**
* crash_setup_regs() - save registers for the panic kernel
*
 @@ -40,7 +42,37 @@
   static inline void crash_setup_regs(struct pt_regs *newregs,
struct pt_regs *oldregs)
   {
 -  /* Empty routine needed to avoid build errors. */
 +  if (oldregs) {
 +  memcpy(newregs, oldregs, sizeof(*newregs));
 +  } else {
 +  __asm__ __volatile__ (
 +  stp x0,   x1, [%3]\n\t

 Why the tabs?

 Please use #16 * N as the offset for consistency with entry.S, with 0
 for the first N.

 [...]

 +static void machine_kexec_mask_interrupts(void)
 +{
 +  unsigned int i;
 +  struct irq_desc *desc;
 +
 +  for_each_irq_desc(i, desc) {
 +  struct irq_chip *chip;
 +
 +  chip = irq_desc_get_chip(desc);
 +  if (!chip)
 +  continue;
 +
 +  if (chip-irq_eoi  irqd_irq_inprogress(desc-irq_data))
 +  chip-irq_eoi(desc-irq_data);
 +
 +  if (chip-irq_mask)
 +  chip-irq_mask(desc-irq_data);
 +
 +  if (chip-irq_disable  !irqd_irq_disabled(desc-irq_data))
 +  chip-irq_disable(desc-irq_data);
 +  }
 +}

 I'm surprised that this isn't left to the irqchip driver init code in
 the crash kernel. For all we know this state could be corrupt anyway.

 Indeed, parsing the irqdesc list is a recipe for disaster. Who knows
 which locks have been taken or simply corrupted, pointers nuked...

 Is there any reason we can't get the GIC driver to nuke all of this at
 probe time?
 
 Is it just enough to remove kexec_mask_interrupts() and add gic_eoi_irq()
 at the beginning of gic_cpu_init() in irq-gic.c and irq-gic-v3.c?

No, doing an EOI is definitely the wrong thing to do. If you do it in
the wrong order, you just screw up the GIC state machine. Plus, you have
no idea what to write there...

The only real solution is to zero the active registers.

 This feels like the better option. I can cook a patch or two for that.
 
 If you do, that will be much better :)

OK, I'll prepare something that we can merge at the same time kexec
comes back from the dead (if it ever does - I'm not holding my breath).

 
 BTW, in arm-gic-v3.h, GICD_CTRL_ARE_NS is defined as
  (1U  4)
 but should it be 5?
 (I'm referring to the page 8-415 in IHI0069A.)

No, look at the definition ARE_NS has when the access is non-secure or
on a system supporting a single security state. The definition you're
referring to is for a secure access (firmware).

Thanks,

M.
-- 
Jazz is not dead. It just smells funny...
--
To unsubscribe from this list: send the line unsubscribe linux-kernel in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/


Re: [v2 2/5] arm64: kdump: implement machine_crash_shutdown()

2015-05-21 Thread AKASHI Takahiro

Mark,

On 05/11/2015 04:10 PM, AKASHI Takahiro wrote:

On 04/24/2015 07:39 PM, Mark Rutland wrote:

On Fri, Apr 24, 2015 at 08:53:05AM +0100, AKASHI Takahiro wrote:

kdump calls machine_crash_shutdown() to shut down non-boot cpus and
save per-cpu general-purpose registers before restarting the crash dump
kernel. See kernel_kexec().
ipi_cpu_stop() is used and a bit modified to support this behavior.

Signed-off-by: AKASHI Takahiro 
---
  arch/arm64/include/asm/kexec.h|   34 ++-
  arch/arm64/kernel/machine_kexec.c |   55 -
  arch/arm64/kernel/smp.c   |   12 ++--
  3 files changed, 97 insertions(+), 4 deletions(-)

diff --git a/arch/arm64/include/asm/kexec.h b/arch/arm64/include/asm/kexec.h
index 3530ff5..eaf3fcb 100644
--- a/arch/arm64/include/asm/kexec.h
+++ b/arch/arm64/include/asm/kexec.h
@@ -30,6 +30,8 @@

  #if !defined(__ASSEMBLY__)

+extern bool in_crash_kexec;
+
  /**
   * crash_setup_regs() - save registers for the panic kernel
   *
@@ -40,7 +42,37 @@
  static inline void crash_setup_regs(struct pt_regs *newregs,
  struct pt_regs *oldregs)
  {
-/* Empty routine needed to avoid build errors. */
+if (oldregs) {
+memcpy(newregs, oldregs, sizeof(*newregs));
+} else {
+__asm__ __volatile__ (
+"stp x0,   x1, [%3]\n\t"


Why the tabs?


Don't know. Will remove them.


Please use #16 * N as the offset for consistency with entry.S, with 0
for the first N.


OK.


[...]


+static void machine_kexec_mask_interrupts(void)
+{
+unsigned int i;
+struct irq_desc *desc;
+
+for_each_irq_desc(i, desc) {
+struct irq_chip *chip;
+
+chip = irq_desc_get_chip(desc);
+if (!chip)
+continue;
+
+if (chip->irq_eoi && irqd_irq_inprogress(>irq_data))
+chip->irq_eoi(>irq_data);
+
+if (chip->irq_mask)
+chip->irq_mask(>irq_data);
+
+if (chip->irq_disable && !irqd_irq_disabled(>irq_data))
+chip->irq_disable(>irq_data);
+}
+}


I'm surprised that this isn't left to the irqchip driver init code in
the crash kernel. For all we know this state could be corrupt anyway.

Is there any reason we can't get the GIC driver to nuke all of this at
probe time?


I don't get the point. You mean that the code be put in probe() of GIC driver?
To be honest, this function was copied from arm's implementation.


Could you please elaborate a bit more details so that I can address the issue?

Thanks,
-Takahiro AKASHI


[...]


@@ -542,7 +543,7 @@ static DEFINE_RAW_SPINLOCK(stop_lock);
  /*
   * ipi_cpu_stop - handle IPI from smp_send_stop()
   */
-static void ipi_cpu_stop(unsigned int cpu)
+static void ipi_cpu_stop(unsigned int cpu, struct pt_regs *regs)
  {
  if (system_state == SYSTEM_BOOTING ||
  system_state == SYSTEM_RUNNING) {
@@ -556,6 +557,13 @@ static void ipi_cpu_stop(unsigned int cpu)

  local_irq_disable();

+#ifdef CONFIG_KEXEC
+if (in_crash_kexec) {
+crash_save_cpu(regs, cpu);
+flush_cache_all();


Any cache maintenance will need to be by VA; flush_cache_all doesn't do
what the name implies, though may appear to work by chance.

Is kdump implemented for ARM? I don't see equivalent for in the arch/arm
ipi_cpu_stop.


Arm has a dedicated function in arch/arm/kernel/machine_kexec.c:
machine_crash_shutdown()
   => machine_crash_nonpanic_core()

Thanks,
-Takahiro AKASHI


Mark.


--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/


Re: [v2 2/5] arm64: kdump: implement machine_crash_shutdown()

2015-05-21 Thread AKASHI Takahiro

Mark,

On 05/11/2015 04:10 PM, AKASHI Takahiro wrote:

On 04/24/2015 07:39 PM, Mark Rutland wrote:

On Fri, Apr 24, 2015 at 08:53:05AM +0100, AKASHI Takahiro wrote:

kdump calls machine_crash_shutdown() to shut down non-boot cpus and
save per-cpu general-purpose registers before restarting the crash dump
kernel. See kernel_kexec().
ipi_cpu_stop() is used and a bit modified to support this behavior.

Signed-off-by: AKASHI Takahiro takahiro.aka...@linaro.org
---
  arch/arm64/include/asm/kexec.h|   34 ++-
  arch/arm64/kernel/machine_kexec.c |   55 -
  arch/arm64/kernel/smp.c   |   12 ++--
  3 files changed, 97 insertions(+), 4 deletions(-)

diff --git a/arch/arm64/include/asm/kexec.h b/arch/arm64/include/asm/kexec.h
index 3530ff5..eaf3fcb 100644
--- a/arch/arm64/include/asm/kexec.h
+++ b/arch/arm64/include/asm/kexec.h
@@ -30,6 +30,8 @@

  #if !defined(__ASSEMBLY__)

+extern bool in_crash_kexec;
+
  /**
   * crash_setup_regs() - save registers for the panic kernel
   *
@@ -40,7 +42,37 @@
  static inline void crash_setup_regs(struct pt_regs *newregs,
  struct pt_regs *oldregs)
  {
-/* Empty routine needed to avoid build errors. */
+if (oldregs) {
+memcpy(newregs, oldregs, sizeof(*newregs));
+} else {
+__asm__ __volatile__ (
+stp x0,   x1, [%3]\n\t


Why the tabs?


Don't know. Will remove them.


Please use #16 * N as the offset for consistency with entry.S, with 0
for the first N.


OK.


[...]


+static void machine_kexec_mask_interrupts(void)
+{
+unsigned int i;
+struct irq_desc *desc;
+
+for_each_irq_desc(i, desc) {
+struct irq_chip *chip;
+
+chip = irq_desc_get_chip(desc);
+if (!chip)
+continue;
+
+if (chip-irq_eoi  irqd_irq_inprogress(desc-irq_data))
+chip-irq_eoi(desc-irq_data);
+
+if (chip-irq_mask)
+chip-irq_mask(desc-irq_data);
+
+if (chip-irq_disable  !irqd_irq_disabled(desc-irq_data))
+chip-irq_disable(desc-irq_data);
+}
+}


I'm surprised that this isn't left to the irqchip driver init code in
the crash kernel. For all we know this state could be corrupt anyway.

Is there any reason we can't get the GIC driver to nuke all of this at
probe time?


I don't get the point. You mean that the code be put in probe() of GIC driver?
To be honest, this function was copied from arm's implementation.


Could you please elaborate a bit more details so that I can address the issue?

Thanks,
-Takahiro AKASHI


[...]


@@ -542,7 +543,7 @@ static DEFINE_RAW_SPINLOCK(stop_lock);
  /*
   * ipi_cpu_stop - handle IPI from smp_send_stop()
   */
-static void ipi_cpu_stop(unsigned int cpu)
+static void ipi_cpu_stop(unsigned int cpu, struct pt_regs *regs)
  {
  if (system_state == SYSTEM_BOOTING ||
  system_state == SYSTEM_RUNNING) {
@@ -556,6 +557,13 @@ static void ipi_cpu_stop(unsigned int cpu)

  local_irq_disable();

+#ifdef CONFIG_KEXEC
+if (in_crash_kexec) {
+crash_save_cpu(regs, cpu);
+flush_cache_all();


Any cache maintenance will need to be by VA; flush_cache_all doesn't do
what the name implies, though may appear to work by chance.

Is kdump implemented for ARM? I don't see equivalent for in the arch/arm
ipi_cpu_stop.


Arm has a dedicated function in arch/arm/kernel/machine_kexec.c:
machine_crash_shutdown()
   = machine_crash_nonpanic_core()

Thanks,
-Takahiro AKASHI


Mark.


--
To unsubscribe from this list: send the line unsubscribe linux-kernel in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/


Re: [v2 2/5] arm64: kdump: implement machine_crash_shutdown()

2015-05-11 Thread AKASHI Takahiro

On 04/24/2015 07:39 PM, Mark Rutland wrote:

On Fri, Apr 24, 2015 at 08:53:05AM +0100, AKASHI Takahiro wrote:

kdump calls machine_crash_shutdown() to shut down non-boot cpus and
save per-cpu general-purpose registers before restarting the crash dump
kernel. See kernel_kexec().
ipi_cpu_stop() is used and a bit modified to support this behavior.

Signed-off-by: AKASHI Takahiro 
---
  arch/arm64/include/asm/kexec.h|   34 ++-
  arch/arm64/kernel/machine_kexec.c |   55 -
  arch/arm64/kernel/smp.c   |   12 ++--
  3 files changed, 97 insertions(+), 4 deletions(-)

diff --git a/arch/arm64/include/asm/kexec.h b/arch/arm64/include/asm/kexec.h
index 3530ff5..eaf3fcb 100644
--- a/arch/arm64/include/asm/kexec.h
+++ b/arch/arm64/include/asm/kexec.h
@@ -30,6 +30,8 @@

  #if !defined(__ASSEMBLY__)

+extern bool in_crash_kexec;
+
  /**
   * crash_setup_regs() - save registers for the panic kernel
   *
@@ -40,7 +42,37 @@
  static inline void crash_setup_regs(struct pt_regs *newregs,
struct pt_regs *oldregs)
  {
-   /* Empty routine needed to avoid build errors. */
+   if (oldregs) {
+   memcpy(newregs, oldregs, sizeof(*newregs));
+   } else {
+   __asm__ __volatile__ (
+   "stpx0,   x1, [%3]\n\t"


Why the tabs?


Don't know. Will remove them.


Please use #16 * N as the offset for consistency with entry.S, with 0
for the first N.


OK.


[...]


+static void machine_kexec_mask_interrupts(void)
+{
+   unsigned int i;
+   struct irq_desc *desc;
+
+   for_each_irq_desc(i, desc) {
+   struct irq_chip *chip;
+
+   chip = irq_desc_get_chip(desc);
+   if (!chip)
+   continue;
+
+   if (chip->irq_eoi && irqd_irq_inprogress(>irq_data))
+   chip->irq_eoi(>irq_data);
+
+   if (chip->irq_mask)
+   chip->irq_mask(>irq_data);
+
+   if (chip->irq_disable && !irqd_irq_disabled(>irq_data))
+   chip->irq_disable(>irq_data);
+   }
+}


I'm surprised that this isn't left to the irqchip driver init code in
the crash kernel. For all we know this state could be corrupt anyway.

Is there any reason we can't get the GIC driver to nuke all of this at
probe time?


I don't get the point. You mean that the code be put in probe() of GIC driver?
To be honest, this function was copied from arm's implementation.


[...]


@@ -542,7 +543,7 @@ static DEFINE_RAW_SPINLOCK(stop_lock);
  /*
   * ipi_cpu_stop - handle IPI from smp_send_stop()
   */
-static void ipi_cpu_stop(unsigned int cpu)
+static void ipi_cpu_stop(unsigned int cpu, struct pt_regs *regs)
  {
if (system_state == SYSTEM_BOOTING ||
system_state == SYSTEM_RUNNING) {
@@ -556,6 +557,13 @@ static void ipi_cpu_stop(unsigned int cpu)

local_irq_disable();

+#ifdef CONFIG_KEXEC
+   if (in_crash_kexec) {
+   crash_save_cpu(regs, cpu);
+   flush_cache_all();


Any cache maintenance will need to be by VA; flush_cache_all doesn't do
what the name implies, though may appear to work by chance.

Is kdump implemented for ARM? I don't see equivalent for in the arch/arm
ipi_cpu_stop.


Arm has a dedicated function in arch/arm/kernel/machine_kexec.c:
   machine_crash_shutdown()
  => machine_crash_nonpanic_core()

Thanks,
-Takahiro AKASHI


Mark.


--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/


Re: [v2 2/5] arm64: kdump: implement machine_crash_shutdown()

2015-05-11 Thread AKASHI Takahiro

On 04/24/2015 07:39 PM, Mark Rutland wrote:

On Fri, Apr 24, 2015 at 08:53:05AM +0100, AKASHI Takahiro wrote:

kdump calls machine_crash_shutdown() to shut down non-boot cpus and
save per-cpu general-purpose registers before restarting the crash dump
kernel. See kernel_kexec().
ipi_cpu_stop() is used and a bit modified to support this behavior.

Signed-off-by: AKASHI Takahiro takahiro.aka...@linaro.org
---
  arch/arm64/include/asm/kexec.h|   34 ++-
  arch/arm64/kernel/machine_kexec.c |   55 -
  arch/arm64/kernel/smp.c   |   12 ++--
  3 files changed, 97 insertions(+), 4 deletions(-)

diff --git a/arch/arm64/include/asm/kexec.h b/arch/arm64/include/asm/kexec.h
index 3530ff5..eaf3fcb 100644
--- a/arch/arm64/include/asm/kexec.h
+++ b/arch/arm64/include/asm/kexec.h
@@ -30,6 +30,8 @@

  #if !defined(__ASSEMBLY__)

+extern bool in_crash_kexec;
+
  /**
   * crash_setup_regs() - save registers for the panic kernel
   *
@@ -40,7 +42,37 @@
  static inline void crash_setup_regs(struct pt_regs *newregs,
struct pt_regs *oldregs)
  {
-   /* Empty routine needed to avoid build errors. */
+   if (oldregs) {
+   memcpy(newregs, oldregs, sizeof(*newregs));
+   } else {
+   __asm__ __volatile__ (
+   stpx0,   x1, [%3]\n\t


Why the tabs?


Don't know. Will remove them.


Please use #16 * N as the offset for consistency with entry.S, with 0
for the first N.


OK.


[...]


+static void machine_kexec_mask_interrupts(void)
+{
+   unsigned int i;
+   struct irq_desc *desc;
+
+   for_each_irq_desc(i, desc) {
+   struct irq_chip *chip;
+
+   chip = irq_desc_get_chip(desc);
+   if (!chip)
+   continue;
+
+   if (chip-irq_eoi  irqd_irq_inprogress(desc-irq_data))
+   chip-irq_eoi(desc-irq_data);
+
+   if (chip-irq_mask)
+   chip-irq_mask(desc-irq_data);
+
+   if (chip-irq_disable  !irqd_irq_disabled(desc-irq_data))
+   chip-irq_disable(desc-irq_data);
+   }
+}


I'm surprised that this isn't left to the irqchip driver init code in
the crash kernel. For all we know this state could be corrupt anyway.

Is there any reason we can't get the GIC driver to nuke all of this at
probe time?


I don't get the point. You mean that the code be put in probe() of GIC driver?
To be honest, this function was copied from arm's implementation.


[...]


@@ -542,7 +543,7 @@ static DEFINE_RAW_SPINLOCK(stop_lock);
  /*
   * ipi_cpu_stop - handle IPI from smp_send_stop()
   */
-static void ipi_cpu_stop(unsigned int cpu)
+static void ipi_cpu_stop(unsigned int cpu, struct pt_regs *regs)
  {
if (system_state == SYSTEM_BOOTING ||
system_state == SYSTEM_RUNNING) {
@@ -556,6 +557,13 @@ static void ipi_cpu_stop(unsigned int cpu)

local_irq_disable();

+#ifdef CONFIG_KEXEC
+   if (in_crash_kexec) {
+   crash_save_cpu(regs, cpu);
+   flush_cache_all();


Any cache maintenance will need to be by VA; flush_cache_all doesn't do
what the name implies, though may appear to work by chance.

Is kdump implemented for ARM? I don't see equivalent for in the arch/arm
ipi_cpu_stop.


Arm has a dedicated function in arch/arm/kernel/machine_kexec.c:
   machine_crash_shutdown()
  = machine_crash_nonpanic_core()

Thanks,
-Takahiro AKASHI


Mark.


--
To unsubscribe from this list: send the line unsubscribe linux-kernel in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/


Re: [v2 2/5] arm64: kdump: implement machine_crash_shutdown()

2015-04-24 Thread Marc Zyngier
On 24/04/15 11:39, Mark Rutland wrote:
> On Fri, Apr 24, 2015 at 08:53:05AM +0100, AKASHI Takahiro wrote:
>> kdump calls machine_crash_shutdown() to shut down non-boot cpus and
>> save per-cpu general-purpose registers before restarting the crash dump
>> kernel. See kernel_kexec().
>> ipi_cpu_stop() is used and a bit modified to support this behavior.
>>
>> Signed-off-by: AKASHI Takahiro 
>> ---
>>  arch/arm64/include/asm/kexec.h|   34 ++-
>>  arch/arm64/kernel/machine_kexec.c |   55 
>> -
>>  arch/arm64/kernel/smp.c   |   12 ++--
>>  3 files changed, 97 insertions(+), 4 deletions(-)
>>
>> diff --git a/arch/arm64/include/asm/kexec.h b/arch/arm64/include/asm/kexec.h
>> index 3530ff5..eaf3fcb 100644
>> --- a/arch/arm64/include/asm/kexec.h
>> +++ b/arch/arm64/include/asm/kexec.h
>> @@ -30,6 +30,8 @@
>>  
>>  #if !defined(__ASSEMBLY__)
>>  
>> +extern bool in_crash_kexec;
>> +
>>  /**
>>   * crash_setup_regs() - save registers for the panic kernel
>>   *
>> @@ -40,7 +42,37 @@
>>  static inline void crash_setup_regs(struct pt_regs *newregs,
>>  struct pt_regs *oldregs)
>>  {
>> -/* Empty routine needed to avoid build errors. */
>> +if (oldregs) {
>> +memcpy(newregs, oldregs, sizeof(*newregs));
>> +} else {
>> +__asm__ __volatile__ (
>> +"stp x0,   x1, [%3]\n\t"
> 
> Why the tabs?
> 
> Please use #16 * N as the offset for consistency with entry.S, with 0
> for the first N.
> 
> [...]
> 
>> +static void machine_kexec_mask_interrupts(void)
>> +{
>> +unsigned int i;
>> +struct irq_desc *desc;
>> +
>> +for_each_irq_desc(i, desc) {
>> +struct irq_chip *chip;
>> +
>> +chip = irq_desc_get_chip(desc);
>> +if (!chip)
>> +continue;
>> +
>> +if (chip->irq_eoi && irqd_irq_inprogress(>irq_data))
>> +chip->irq_eoi(>irq_data);
>> +
>> +if (chip->irq_mask)
>> +chip->irq_mask(>irq_data);
>> +
>> +if (chip->irq_disable && !irqd_irq_disabled(>irq_data))
>> +chip->irq_disable(>irq_data);
>> +}
>> +}
> 
> I'm surprised that this isn't left to the irqchip driver init code in
> the crash kernel. For all we know this state could be corrupt anyway.

Indeed, parsing the irqdesc list is a recipe for disaster. Who knows
which locks have been taken or simply corrupted, pointers nuked...

> Is there any reason we can't get the GIC driver to nuke all of this at
> probe time?

This feels like the better option. I can cook a patch or two for that.

Thanks,

M.
-- 
Jazz is not dead. It just smells funny...
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/


Re: [v2 2/5] arm64: kdump: implement machine_crash_shutdown()

2015-04-24 Thread Mark Rutland
On Fri, Apr 24, 2015 at 08:53:05AM +0100, AKASHI Takahiro wrote:
> kdump calls machine_crash_shutdown() to shut down non-boot cpus and
> save per-cpu general-purpose registers before restarting the crash dump
> kernel. See kernel_kexec().
> ipi_cpu_stop() is used and a bit modified to support this behavior.
> 
> Signed-off-by: AKASHI Takahiro 
> ---
>  arch/arm64/include/asm/kexec.h|   34 ++-
>  arch/arm64/kernel/machine_kexec.c |   55 
> -
>  arch/arm64/kernel/smp.c   |   12 ++--
>  3 files changed, 97 insertions(+), 4 deletions(-)
> 
> diff --git a/arch/arm64/include/asm/kexec.h b/arch/arm64/include/asm/kexec.h
> index 3530ff5..eaf3fcb 100644
> --- a/arch/arm64/include/asm/kexec.h
> +++ b/arch/arm64/include/asm/kexec.h
> @@ -30,6 +30,8 @@
>  
>  #if !defined(__ASSEMBLY__)
>  
> +extern bool in_crash_kexec;
> +
>  /**
>   * crash_setup_regs() - save registers for the panic kernel
>   *
> @@ -40,7 +42,37 @@
>  static inline void crash_setup_regs(struct pt_regs *newregs,
>   struct pt_regs *oldregs)
>  {
> - /* Empty routine needed to avoid build errors. */
> + if (oldregs) {
> + memcpy(newregs, oldregs, sizeof(*newregs));
> + } else {
> + __asm__ __volatile__ (
> + "stp x0,   x1, [%3]\n\t"

Why the tabs?

Please use #16 * N as the offset for consistency with entry.S, with 0
for the first N.

[...]

> +static void machine_kexec_mask_interrupts(void)
> +{
> + unsigned int i;
> + struct irq_desc *desc;
> +
> + for_each_irq_desc(i, desc) {
> + struct irq_chip *chip;
> +
> + chip = irq_desc_get_chip(desc);
> + if (!chip)
> + continue;
> +
> + if (chip->irq_eoi && irqd_irq_inprogress(>irq_data))
> + chip->irq_eoi(>irq_data);
> +
> + if (chip->irq_mask)
> + chip->irq_mask(>irq_data);
> +
> + if (chip->irq_disable && !irqd_irq_disabled(>irq_data))
> + chip->irq_disable(>irq_data);
> + }
> +}

I'm surprised that this isn't left to the irqchip driver init code in
the crash kernel. For all we know this state could be corrupt anyway.

Is there any reason we can't get the GIC driver to nuke all of this at
probe time?

[...]

> @@ -542,7 +543,7 @@ static DEFINE_RAW_SPINLOCK(stop_lock);
>  /*
>   * ipi_cpu_stop - handle IPI from smp_send_stop()
>   */
> -static void ipi_cpu_stop(unsigned int cpu)
> +static void ipi_cpu_stop(unsigned int cpu, struct pt_regs *regs)
>  {
>   if (system_state == SYSTEM_BOOTING ||
>   system_state == SYSTEM_RUNNING) {
> @@ -556,6 +557,13 @@ static void ipi_cpu_stop(unsigned int cpu)
>  
>   local_irq_disable();
>  
> +#ifdef CONFIG_KEXEC
> + if (in_crash_kexec) {
> + crash_save_cpu(regs, cpu);
> + flush_cache_all();

Any cache maintenance will need to be by VA; flush_cache_all doesn't do
what the name implies, though may appear to work by chance.

Is kdump implemented for ARM? I don't see equivalent for in the arch/arm
ipi_cpu_stop.

Mark.
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/


Re: [v2 2/5] arm64: kdump: implement machine_crash_shutdown()

2015-04-24 Thread Mark Rutland
On Fri, Apr 24, 2015 at 08:53:05AM +0100, AKASHI Takahiro wrote:
 kdump calls machine_crash_shutdown() to shut down non-boot cpus and
 save per-cpu general-purpose registers before restarting the crash dump
 kernel. See kernel_kexec().
 ipi_cpu_stop() is used and a bit modified to support this behavior.
 
 Signed-off-by: AKASHI Takahiro takahiro.aka...@linaro.org
 ---
  arch/arm64/include/asm/kexec.h|   34 ++-
  arch/arm64/kernel/machine_kexec.c |   55 
 -
  arch/arm64/kernel/smp.c   |   12 ++--
  3 files changed, 97 insertions(+), 4 deletions(-)
 
 diff --git a/arch/arm64/include/asm/kexec.h b/arch/arm64/include/asm/kexec.h
 index 3530ff5..eaf3fcb 100644
 --- a/arch/arm64/include/asm/kexec.h
 +++ b/arch/arm64/include/asm/kexec.h
 @@ -30,6 +30,8 @@
  
  #if !defined(__ASSEMBLY__)
  
 +extern bool in_crash_kexec;
 +
  /**
   * crash_setup_regs() - save registers for the panic kernel
   *
 @@ -40,7 +42,37 @@
  static inline void crash_setup_regs(struct pt_regs *newregs,
   struct pt_regs *oldregs)
  {
 - /* Empty routine needed to avoid build errors. */
 + if (oldregs) {
 + memcpy(newregs, oldregs, sizeof(*newregs));
 + } else {
 + __asm__ __volatile__ (
 + stp x0,   x1, [%3]\n\t

Why the tabs?

Please use #16 * N as the offset for consistency with entry.S, with 0
for the first N.

[...]

 +static void machine_kexec_mask_interrupts(void)
 +{
 + unsigned int i;
 + struct irq_desc *desc;
 +
 + for_each_irq_desc(i, desc) {
 + struct irq_chip *chip;
 +
 + chip = irq_desc_get_chip(desc);
 + if (!chip)
 + continue;
 +
 + if (chip-irq_eoi  irqd_irq_inprogress(desc-irq_data))
 + chip-irq_eoi(desc-irq_data);
 +
 + if (chip-irq_mask)
 + chip-irq_mask(desc-irq_data);
 +
 + if (chip-irq_disable  !irqd_irq_disabled(desc-irq_data))
 + chip-irq_disable(desc-irq_data);
 + }
 +}

I'm surprised that this isn't left to the irqchip driver init code in
the crash kernel. For all we know this state could be corrupt anyway.

Is there any reason we can't get the GIC driver to nuke all of this at
probe time?

[...]

 @@ -542,7 +543,7 @@ static DEFINE_RAW_SPINLOCK(stop_lock);
  /*
   * ipi_cpu_stop - handle IPI from smp_send_stop()
   */
 -static void ipi_cpu_stop(unsigned int cpu)
 +static void ipi_cpu_stop(unsigned int cpu, struct pt_regs *regs)
  {
   if (system_state == SYSTEM_BOOTING ||
   system_state == SYSTEM_RUNNING) {
 @@ -556,6 +557,13 @@ static void ipi_cpu_stop(unsigned int cpu)
  
   local_irq_disable();
  
 +#ifdef CONFIG_KEXEC
 + if (in_crash_kexec) {
 + crash_save_cpu(regs, cpu);
 + flush_cache_all();

Any cache maintenance will need to be by VA; flush_cache_all doesn't do
what the name implies, though may appear to work by chance.

Is kdump implemented for ARM? I don't see equivalent for in the arch/arm
ipi_cpu_stop.

Mark.
--
To unsubscribe from this list: send the line unsubscribe linux-kernel in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/


Re: [v2 2/5] arm64: kdump: implement machine_crash_shutdown()

2015-04-24 Thread Marc Zyngier
On 24/04/15 11:39, Mark Rutland wrote:
 On Fri, Apr 24, 2015 at 08:53:05AM +0100, AKASHI Takahiro wrote:
 kdump calls machine_crash_shutdown() to shut down non-boot cpus and
 save per-cpu general-purpose registers before restarting the crash dump
 kernel. See kernel_kexec().
 ipi_cpu_stop() is used and a bit modified to support this behavior.

 Signed-off-by: AKASHI Takahiro takahiro.aka...@linaro.org
 ---
  arch/arm64/include/asm/kexec.h|   34 ++-
  arch/arm64/kernel/machine_kexec.c |   55 
 -
  arch/arm64/kernel/smp.c   |   12 ++--
  3 files changed, 97 insertions(+), 4 deletions(-)

 diff --git a/arch/arm64/include/asm/kexec.h b/arch/arm64/include/asm/kexec.h
 index 3530ff5..eaf3fcb 100644
 --- a/arch/arm64/include/asm/kexec.h
 +++ b/arch/arm64/include/asm/kexec.h
 @@ -30,6 +30,8 @@
  
  #if !defined(__ASSEMBLY__)
  
 +extern bool in_crash_kexec;
 +
  /**
   * crash_setup_regs() - save registers for the panic kernel
   *
 @@ -40,7 +42,37 @@
  static inline void crash_setup_regs(struct pt_regs *newregs,
  struct pt_regs *oldregs)
  {
 -/* Empty routine needed to avoid build errors. */
 +if (oldregs) {
 +memcpy(newregs, oldregs, sizeof(*newregs));
 +} else {
 +__asm__ __volatile__ (
 +stp x0,   x1, [%3]\n\t
 
 Why the tabs?
 
 Please use #16 * N as the offset for consistency with entry.S, with 0
 for the first N.
 
 [...]
 
 +static void machine_kexec_mask_interrupts(void)
 +{
 +unsigned int i;
 +struct irq_desc *desc;
 +
 +for_each_irq_desc(i, desc) {
 +struct irq_chip *chip;
 +
 +chip = irq_desc_get_chip(desc);
 +if (!chip)
 +continue;
 +
 +if (chip-irq_eoi  irqd_irq_inprogress(desc-irq_data))
 +chip-irq_eoi(desc-irq_data);
 +
 +if (chip-irq_mask)
 +chip-irq_mask(desc-irq_data);
 +
 +if (chip-irq_disable  !irqd_irq_disabled(desc-irq_data))
 +chip-irq_disable(desc-irq_data);
 +}
 +}
 
 I'm surprised that this isn't left to the irqchip driver init code in
 the crash kernel. For all we know this state could be corrupt anyway.

Indeed, parsing the irqdesc list is a recipe for disaster. Who knows
which locks have been taken or simply corrupted, pointers nuked...

 Is there any reason we can't get the GIC driver to nuke all of this at
 probe time?

This feels like the better option. I can cook a patch or two for that.

Thanks,

M.
-- 
Jazz is not dead. It just smells funny...
--
To unsubscribe from this list: send the line unsubscribe linux-kernel in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/