[RFC PATCH v2] Add IPI entry for CPU UP

[RFC PATCH v2] Add IPI entry for CPU UP

[Zhaoyang Huang]

In some ARM SOCs, IPI interrupt is used for hotplug in one cpu, that is,
sending a IPI to the core in WFI and powerdown status. So Add a IPI
entry for handle this kind of cpu up interrupt
Launching the IPI can be done within PSCI, while there will be one unknown
type of IPI as the dest core come up to the kernel world which will bring a
warning so far.So add such type of IPI to handle the interrupt.

Signed-off-by: Zhaoyang Huang <zhaoyang.huang@spreadtrum.com>
---
 arch/arm64/include/asm/hardirq.h |    2 +-
 arch/arm64/kernel/smp.c          |   10 ++++++++++
 2 files changed, 11 insertions(+), 1 deletion(-)

diff --git a/arch/arm64/include/asm/hardirq.h b/arch/arm64/include/asm/hardirq.h
index a57601f..bb4edb7 100644
--- a/arch/arm64/include/asm/hardirq.h
+++ b/arch/arm64/include/asm/hardirq.h
@@ -20,7 +20,7 @@
 #include <linux/threads.h>
 #include <asm/irq.h>
 
-#define NR_IPI	5
+#define NR_IPI   6
 
 typedef struct {
 	unsigned int __softirq_pending;
diff --git a/arch/arm64/kernel/smp.c b/arch/arm64/kernel/smp.c
index b1adc51..20e63c9 100644
--- a/arch/arm64/kernel/smp.c
+++ b/arch/arm64/kernel/smp.c
@@ -70,6 +70,7 @@ enum ipi_msg_type {
 	IPI_CPU_STOP,
 	IPI_TIMER,
 	IPI_IRQ_WORK,
+	IPI_CPU_UP,
 };
 
 /*
@@ -627,6 +628,7 @@ static const char *ipi_types[NR_IPI] __tracepoint_string = {
 	S(IPI_CPU_STOP, "CPU stop interrupts"),
 	S(IPI_TIMER, "Timer broadcast interrupts"),
 	S(IPI_IRQ_WORK, "IRQ work interrupts"),
+	S(IPI_CPU_UP, "Hotplug cpu up by ipi"),
 };
 
 static void smp_cross_call(const struct cpumask *target, unsigned int ipinr)
@@ -746,6 +748,8 @@ void handle_IPI(int ipinr, struct pt_regs *regs)
 		irq_exit();
 		break;
 #endif
+       case IPI_CPU_UP:
+               break;
 
 	default:
 		pr_crit("CPU%u: Unknown IPI message 0x%x\n", cpu, ipinr);
@@ -798,3 +802,9 @@ int setup_profiling_timer(unsigned int multiplier)
 {
 	return -EINVAL;
 }
+
+void smp_send_cpuup(int cpu)
+{
+       smp_cross_call(cpumask_of(cpu), IPI_CPU_UP);
+}
+
-- 
1.7.9.5

Re: [RFC PATCH v2] Add IPI entry for CPU UP

[Zhaoyang Huang]

On 12 January 2016 at 10:05, Zhaoyang Huang <zhaoyang.huang@linaro.org> wrote:
> In some ARM SOCs, IPI interrupt is used for hotplug in one cpu, that is,
> sending a IPI to the core in WFI and powerdown status. So Add a IPI
> entry for handle this kind of cpu up interrupt
> Launching the IPI can be done within PSCI, while there will be one unknown
> type of IPI as the dest core come up to the kernel world which will bring a
> warning so far.So add such type of IPI to handle the interrupt.
>
> Signed-off-by: Zhaoyang Huang <zhaoyang.huang@spreadtrum.com>
> ---
>  arch/arm64/include/asm/hardirq.h |    2 +-
>  arch/arm64/kernel/smp.c          |   10 ++++++++++
>  2 files changed, 11 insertions(+), 1 deletion(-)
>
> diff --git a/arch/arm64/include/asm/hardirq.h b/arch/arm64/include/asm/hardirq.h
> index a57601f..bb4edb7 100644
> --- a/arch/arm64/include/asm/hardirq.h
> +++ b/arch/arm64/include/asm/hardirq.h
> @@ -20,7 +20,7 @@
>  #include <linux/threads.h>
>  #include <asm/irq.h>
>
> -#define NR_IPI 5
> +#define NR_IPI   6
>
>  typedef struct {
>         unsigned int __softirq_pending;
> diff --git a/arch/arm64/kernel/smp.c b/arch/arm64/kernel/smp.c
> index b1adc51..20e63c9 100644
> --- a/arch/arm64/kernel/smp.c
> +++ b/arch/arm64/kernel/smp.c
> @@ -70,6 +70,7 @@ enum ipi_msg_type {
>         IPI_CPU_STOP,
>         IPI_TIMER,
>         IPI_IRQ_WORK,
> +       IPI_CPU_UP,
>  };
>
>  /*
> @@ -627,6 +628,7 @@ static const char *ipi_types[NR_IPI] __tracepoint_string = {
>         S(IPI_CPU_STOP, "CPU stop interrupts"),
>         S(IPI_TIMER, "Timer broadcast interrupts"),
>         S(IPI_IRQ_WORK, "IRQ work interrupts"),
> +       S(IPI_CPU_UP, "Hotplug cpu up by ipi"),
>  };
>
>  static void smp_cross_call(const struct cpumask *target, unsigned int ipinr)
> @@ -746,6 +748,8 @@ void handle_IPI(int ipinr, struct pt_regs *regs)
>                 irq_exit();
>                 break;
>  #endif
> +       case IPI_CPU_UP:
> +               break;
>
>         default:
>                 pr_crit("CPU%u: Unknown IPI message 0x%x\n", cpu, ipinr);
> @@ -798,3 +802,9 @@ int setup_profiling_timer(unsigned int multiplier)
>  {
>         return -EINVAL;
>  }
> +
> +void smp_send_cpuup(int cpu)
> +{
> +       smp_cross_call(cpumask_of(cpu), IPI_CPU_UP);
> +}
> +
> --
> 1.7.9.5
>
As the added commit message saying, the IPI can be launched in either
of kernel or PSCI. But there will be a unknown type of IPI when dest
core come to the kernel.

Re: [RFC PATCH v2] Add IPI entry for CPU UP

[Lorenzo Pieralisi]

On Tue, Jan 12, 2016 at 10:17:42AM +0800, Zhaoyang Huang wrote:
> On 12 January 2016 at 10:05, Zhaoyang Huang <zhaoyang.huang@linaro.org> wrote:
> > In some ARM SOCs, IPI interrupt is used for hotplug in one cpu, that is,
> > sending a IPI to the core in WFI and powerdown status. So Add a IPI
> > entry for handle this kind of cpu up interrupt
> > Launching the IPI can be done within PSCI, while there will be one unknown
> > type of IPI as the dest core come up to the kernel world which will bring a
> > warning so far.So add such type of IPI to handle the interrupt.

You missed CC'ing ALKML for the second time and you were warned.

You are adding a call to *send* an IPI in the kernel so the commit
above is misleading.

Acknowledge and clear the IRQ in FW so that the mechanism is completely
implemented in FW (ie PSCI), that the CPU coming out of reset will run
before getting to the kernel, this patch is not needed and we already
explained to you why.

Lorenzo

> > Signed-off-by: Zhaoyang Huang <zhaoyang.huang@spreadtrum.com>
> > ---
> >  arch/arm64/include/asm/hardirq.h |    2 +-
> >  arch/arm64/kernel/smp.c          |   10 ++++++++++
> >  2 files changed, 11 insertions(+), 1 deletion(-)
> >
> > diff --git a/arch/arm64/include/asm/hardirq.h b/arch/arm64/include/asm/hardirq.h
> > index a57601f..bb4edb7 100644
> > --- a/arch/arm64/include/asm/hardirq.h
> > +++ b/arch/arm64/include/asm/hardirq.h
> > @@ -20,7 +20,7 @@
> >  #include <linux/threads.h>
> >  #include <asm/irq.h>
> >
> > -#define NR_IPI 5
> > +#define NR_IPI   6
> >
> >  typedef struct {
> >         unsigned int __softirq_pending;
> > diff --git a/arch/arm64/kernel/smp.c b/arch/arm64/kernel/smp.c
> > index b1adc51..20e63c9 100644
> > --- a/arch/arm64/kernel/smp.c
> > +++ b/arch/arm64/kernel/smp.c
> > @@ -70,6 +70,7 @@ enum ipi_msg_type {
> >         IPI_CPU_STOP,
> >         IPI_TIMER,
> >         IPI_IRQ_WORK,
> > +       IPI_CPU_UP,
> >  };
> >
> >  /*
> > @@ -627,6 +628,7 @@ static const char *ipi_types[NR_IPI] __tracepoint_string = {
> >         S(IPI_CPU_STOP, "CPU stop interrupts"),
> >         S(IPI_TIMER, "Timer broadcast interrupts"),
> >         S(IPI_IRQ_WORK, "IRQ work interrupts"),
> > +       S(IPI_CPU_UP, "Hotplug cpu up by ipi"),
> >  };
> >
> >  static void smp_cross_call(const struct cpumask *target, unsigned int ipinr)
> > @@ -746,6 +748,8 @@ void handle_IPI(int ipinr, struct pt_regs *regs)
> >                 irq_exit();
> >                 break;
> >  #endif
> > +       case IPI_CPU_UP:
> > +               break;
> >
> >         default:
> >                 pr_crit("CPU%u: Unknown IPI message 0x%x\n", cpu, ipinr);
> > @@ -798,3 +802,9 @@ int setup_profiling_timer(unsigned int multiplier)
> >  {
> >         return -EINVAL;
> >  }
> > +
> > +void smp_send_cpuup(int cpu)
> > +{
> > +       smp_cross_call(cpumask_of(cpu), IPI_CPU_UP);
> > +}
> > +
> > --
> > 1.7.9.5
> >
> As the added commit message saying, the IPI can be launched in either
> of kernel or PSCI. But there will be a unknown type of IPI when dest
> core come to the kernel.
> 

Re: [RFC PATCH v2] Add IPI entry for CPU UP

[Zhaoyang Huang]

On 12 January 2016 at 17:38, Lorenzo Pieralisi
<lorenzo.pieralisi@arm.com> wrote:
> On Tue, Jan 12, 2016 at 10:17:42AM +0800, Zhaoyang Huang wrote:
>> On 12 January 2016 at 10:05, Zhaoyang Huang <zhaoyang.huang@linaro.org> wrote:
>> > In some ARM SOCs, IPI interrupt is used for hotplug in one cpu, that is,
>> > sending a IPI to the core in WFI and powerdown status. So Add a IPI
>> > entry for handle this kind of cpu up interrupt
>> > Launching the IPI can be done within PSCI, while there will be one unknown
>> > type of IPI as the dest core come up to the kernel world which will bring a
>> > warning so far.So add such type of IPI to handle the interrupt.
>
> You missed CC'ing ALKML for the second time and you were warned.
>
> You are adding a call to *send* an IPI in the kernel so the commit
> above is misleading.
>
> Acknowledge and clear the IRQ in FW so that the mechanism is completely
> implemented in FW (ie PSCI), that the CPU coming out of reset will run
> before getting to the kernel, this patch is not needed and we already
> explained to you why.
>
> Lorenzo
>
Thanks for clarification. Whereas, I got a kernel warning for an
unknown type of IPI
from the dest core. It seems that the irq is not acked and cleared by
the fw before
kernel.

hint:There is only bl31 for the testing SOC

>> > Signed-off-by: Zhaoyang Huang <zhaoyang.huang@spreadtrum.com>
>> > ---
>> >  arch/arm64/include/asm/hardirq.h |    2 +-
>> >  arch/arm64/kernel/smp.c          |   10 ++++++++++
>> >  2 files changed, 11 insertions(+), 1 deletion(-)
>> >
>> > diff --git a/arch/arm64/include/asm/hardirq.h b/arch/arm64/include/asm/hardirq.h
>> > index a57601f..bb4edb7 100644
>> > --- a/arch/arm64/include/asm/hardirq.h
>> > +++ b/arch/arm64/include/asm/hardirq.h
>> > @@ -20,7 +20,7 @@
>> >  #include <linux/threads.h>
>> >  #include <asm/irq.h>
>> >
>> > -#define NR_IPI 5
>> > +#define NR_IPI   6
>> >
>> >  typedef struct {
>> >         unsigned int __softirq_pending;
>> > diff --git a/arch/arm64/kernel/smp.c b/arch/arm64/kernel/smp.c
>> > index b1adc51..20e63c9 100644
>> > --- a/arch/arm64/kernel/smp.c
>> > +++ b/arch/arm64/kernel/smp.c
>> > @@ -70,6 +70,7 @@ enum ipi_msg_type {
>> >         IPI_CPU_STOP,
>> >         IPI_TIMER,
>> >         IPI_IRQ_WORK,
>> > +       IPI_CPU_UP,
>> >  };
>> >
>> >  /*
>> > @@ -627,6 +628,7 @@ static const char *ipi_types[NR_IPI] __tracepoint_string = {
>> >         S(IPI_CPU_STOP, "CPU stop interrupts"),
>> >         S(IPI_TIMER, "Timer broadcast interrupts"),
>> >         S(IPI_IRQ_WORK, "IRQ work interrupts"),
>> > +       S(IPI_CPU_UP, "Hotplug cpu up by ipi"),
>> >  };
>> >
>> >  static void smp_cross_call(const struct cpumask *target, unsigned int ipinr)
>> > @@ -746,6 +748,8 @@ void handle_IPI(int ipinr, struct pt_regs *regs)
>> >                 irq_exit();
>> >                 break;
>> >  #endif
>> > +       case IPI_CPU_UP:
>> > +               break;
>> >
>> >         default:
>> >                 pr_crit("CPU%u: Unknown IPI message 0x%x\n", cpu, ipinr);
>> > @@ -798,3 +802,9 @@ int setup_profiling_timer(unsigned int multiplier)
>> >  {
>> >         return -EINVAL;
>> >  }
>> > +
>> > +void smp_send_cpuup(int cpu)
>> > +{
>> > +       smp_cross_call(cpumask_of(cpu), IPI_CPU_UP);
>> > +}
>> > +
>> > --
>> > 1.7.9.5
>> >
>> As the added commit message saying, the IPI can be launched in either
>> of kernel or PSCI. But there will be a unknown type of IPI when dest
>> core come to the kernel.
>>

Re: [RFC PATCH v2] Add IPI entry for CPU UP

[Mark Rutland]

On Tue, Jan 12, 2016 at 09:38:20AM +0000, Lorenzo Pieralisi wrote:
> On Tue, Jan 12, 2016 at 10:17:42AM +0800, Zhaoyang Huang wrote:
> > On 12 January 2016 at 10:05, Zhaoyang Huang <zhaoyang.huang@linaro.org> wrote:
> > > In some ARM SOCs, IPI interrupt is used for hotplug in one cpu, that is,
> > > sending a IPI to the core in WFI and powerdown status. So Add a IPI
> > > entry for handle this kind of cpu up interrupt
> > > Launching the IPI can be done within PSCI, while there will be one unknown
> > > type of IPI as the dest core come up to the kernel world which will bring a
> > > warning so far.So add such type of IPI to handle the interrupt.
> 
> You missed CC'ing ALKML for the second time and you were warned.
> 
> You are adding a call to *send* an IPI in the kernel so the commit
> above is misleading.
> 
> Acknowledge and clear the IRQ in FW so that the mechanism is completely
> implemented in FW (ie PSCI), that the CPU coming out of reset will run
> before getting to the kernel, this patch is not needed and we already
> explained to you why.
> 
> Lorenzo

I would also suggest that FW used the set of SGIs reserved for secure
usage (i.e. ID8 - ID15), as these will not conflict with those the
kernel uses.

Thanks,
Mark.

Re: [RFC PATCH v2] Add IPI entry for CPU UP

[Zhaoyang Huang]

Hi Mark,
Do you have any suggestion on how to sync the GIC operation from
kernel and psci parallelly? Thanks!

On 12 January 2016 at 19:51, Mark Rutland <mark.rutland@arm.com> wrote:
> On Tue, Jan 12, 2016 at 09:38:20AM +0000, Lorenzo Pieralisi wrote:
>> On Tue, Jan 12, 2016 at 10:17:42AM +0800, Zhaoyang Huang wrote:
>> > On 12 January 2016 at 10:05, Zhaoyang Huang <zhaoyang.huang@linaro.org> wrote:
>> > > In some ARM SOCs, IPI interrupt is used for hotplug in one cpu, that is,
>> > > sending a IPI to the core in WFI and powerdown status. So Add a IPI
>> > > entry for handle this kind of cpu up interrupt
>> > > Launching the IPI can be done within PSCI, while there will be one unknown
>> > > type of IPI as the dest core come up to the kernel world which will bring a
>> > > warning so far.So add such type of IPI to handle the interrupt.
>>
>> You missed CC'ing ALKML for the second time and you were warned.
>>
>> You are adding a call to *send* an IPI in the kernel so the commit
>> above is misleading.
>>
>> Acknowledge and clear the IRQ in FW so that the mechanism is completely
>> implemented in FW (ie PSCI), that the CPU coming out of reset will run
>> before getting to the kernel, this patch is not needed and we already
>> explained to you why.
>>
>> Lorenzo
>
> I would also suggest that FW used the set of SGIs reserved for secure
> usage (i.e. ID8 - ID15), as these will not conflict with those the
> kernel uses.
>
> Thanks,
> Mark.

Re: [RFC PATCH v2] Add IPI entry for CPU UP

[Mark Rutland]

On Thu, Jan 21, 2016 at 04:48:57PM +0800, Zhaoyang Huang wrote:
> Hi Mark,

Hi,

> Do you have any suggestion on how to sync the GIC operation from
> kernel and psci parallelly? Thanks!

I'm not sure what you mean. 

What problem are you having with synchronising GIC accesses?

As far as I can see, the CPU sending the IPI can simply poke the
relevant register in the distributor without requiring any
synchronisation. The CPU receiving the IPI is the only CPU with access
to its CPU interface.

Could you describe your problem in more detail?

Thanks,
Mark.

> On 12 January 2016 at 19:51, Mark Rutland <mark.rutland@arm.com> wrote:
> > On Tue, Jan 12, 2016 at 09:38:20AM +0000, Lorenzo Pieralisi wrote:
> >> On Tue, Jan 12, 2016 at 10:17:42AM +0800, Zhaoyang Huang wrote:
> >> > On 12 January 2016 at 10:05, Zhaoyang Huang <zhaoyang.huang@linaro.org> wrote:
> >> > > In some ARM SOCs, IPI interrupt is used for hotplug in one cpu, that is,
> >> > > sending a IPI to the core in WFI and powerdown status. So Add a IPI
> >> > > entry for handle this kind of cpu up interrupt
> >> > > Launching the IPI can be done within PSCI, while there will be one unknown
> >> > > type of IPI as the dest core come up to the kernel world which will bring a
> >> > > warning so far.So add such type of IPI to handle the interrupt.
> >>
> >> You missed CC'ing ALKML for the second time and you were warned.
> >>
> >> You are adding a call to *send* an IPI in the kernel so the commit
> >> above is misleading.
> >>
> >> Acknowledge and clear the IRQ in FW so that the mechanism is completely
> >> implemented in FW (ie PSCI), that the CPU coming out of reset will run
> >> before getting to the kernel, this patch is not needed and we already
> >> explained to you why.
> >>
> >> Lorenzo
> >
> > I would also suggest that FW used the set of SGIs reserved for secure
> > usage (i.e. ID8 - ID15), as these will not conflict with those the
> > kernel uses.
> >
> > Thanks,
> > Mark.
> 

Re: [RFC PATCH v2] Add IPI entry for CPU UP

[Zhaoyang Huang]

On 21 January 2016 at 18:51, Mark Rutland <mark.rutland@arm.com> wrote:
> On Thu, Jan 21, 2016 at 04:48:57PM +0800, Zhaoyang Huang wrote:
>> Hi Mark,
>
> Hi,
>
>> Do you have any suggestion on how to sync the GIC operation from
>> kernel and psci parallelly? Thanks!
>
> I'm not sure what you mean.
>
> What problem are you having with synchronising GIC accesses?
>
> As far as I can see, the CPU sending the IPI can simply poke the
> relevant register in the distributor without requiring any
> synchronisation. The CPU receiving the IPI is the only CPU with access
> to its CPU interface.
>
> Could you describe your problem in more detail?
>
> Thanks,
> Mark.
>
Hi Mark,
Sorry for making confusions. I mean mutex between kernel and trustzone
when accessing
GIC registers. It is possible for they two issuing an accessing to the
same register at the
same time. How should I handle such kind of race conditions?

>> On 12 January 2016 at 19:51, Mark Rutland <mark.rutland@arm.com> wrote:
>> > On Tue, Jan 12, 2016 at 09:38:20AM +0000, Lorenzo Pieralisi wrote:
>> >> On Tue, Jan 12, 2016 at 10:17:42AM +0800, Zhaoyang Huang wrote:
>> >> > On 12 January 2016 at 10:05, Zhaoyang Huang <zhaoyang.huang@linaro.org> wrote:
>> >> > > In some ARM SOCs, IPI interrupt is used for hotplug in one cpu, that is,
>> >> > > sending a IPI to the core in WFI and powerdown status. So Add a IPI
>> >> > > entry for handle this kind of cpu up interrupt
>> >> > > Launching the IPI can be done within PSCI, while there will be one unknown
>> >> > > type of IPI as the dest core come up to the kernel world which will bring a
>> >> > > warning so far.So add such type of IPI to handle the interrupt.
>> >>
>> >> You missed CC'ing ALKML for the second time and you were warned.
>> >>
>> >> You are adding a call to *send* an IPI in the kernel so the commit
>> >> above is misleading.
>> >>
>> >> Acknowledge and clear the IRQ in FW so that the mechanism is completely
>> >> implemented in FW (ie PSCI), that the CPU coming out of reset will run
>> >> before getting to the kernel, this patch is not needed and we already
>> >> explained to you why.
>> >>
>> >> Lorenzo
>> >
>> > I would also suggest that FW used the set of SGIs reserved for secure
>> > usage (i.e. ID8 - ID15), as these will not conflict with those the
>> > kernel uses.
>> >
>> > Thanks,
>> > Mark.
>>

Re: [RFC PATCH v2] Add IPI entry for CPU UP

[Marc Zyngier]

On Fri, 22 Jan 2016 10:01:24 +0800
Zhaoyang Huang <zhaoyang.huang@linaro.org> wrote:

> On 21 January 2016 at 18:51, Mark Rutland <mark.rutland@arm.com>
> wrote:
> > On Thu, Jan 21, 2016 at 04:48:57PM +0800, Zhaoyang Huang wrote:
> >> Hi Mark,
> >
> > Hi,
> >
> >> Do you have any suggestion on how to sync the GIC operation from
> >> kernel and psci parallelly? Thanks!
> >
> > I'm not sure what you mean.
> >
> > What problem are you having with synchronising GIC accesses?
> >
> > As far as I can see, the CPU sending the IPI can simply poke the
> > relevant register in the distributor without requiring any
> > synchronisation. The CPU receiving the IPI is the only CPU with
> > access to its CPU interface.
> >
> > Could you describe your problem in more detail?
> >
> > Thanks,
> > Mark.
> >
> Hi Mark,
> Sorry for making confusions. I mean mutex between kernel and trustzone
> when accessing
> GIC registers. It is possible for they two issuing an accessing to the
> same register at the
> same time. How should I handle such kind of race conditions?

The GIC programming interface is designed to allow this kind of access
without locking:

- CPU interface: the CPU cannot be in secure and non-secure at the same
  time, so there is no race for the access. Furthermore, the fact that
  secure interrupts have a higher priority than non-secure ones ensure
  that a secure interrupt will preempt a non-secure one, making the
  whole thing race free.

- Distributor: Writing to the GICD_SGIR register is atomic, and the GIC
  will ensure simultaneous access.

In a nutshell, there is no need to worry about these things, because
the GIC architecture has been designed from the ground up to support
this.

See for example this:
http://git.denx.de/?p=u-boot.git;a=blob_plain;f=arch/arm/cpu/armv7/sunxi/psci_sun7i.S;hb=HEAD

which uses IPIs to implement PSCI on an existing ARMv7 system.

Thanks,

	M.
-- 
Without deviation from the norm, progress is not possible.