Re: [Xen-devel] [PATCH] arm: gic-v3: clear GICR active interrupts
> -Original Message- > From: Julien Grall [mailto:julien.gr...@arm.com] > Sent: 2019年1月30日 21:49 > To: Peng Fan ; sstabell...@kernel.org > Cc: xen-devel@lists.xenproject.org; Andre Przywara > > Subject: Re: [PATCH] arm: gic-v3: clear GICR active interrupts > > Hi, > > On 30/01/2019 13:36, Peng Fan wrote: > >>> Each ICACTIVER0 registers hold the active bit for 32 interrupts. > >>> However, this code assumes the register only hold 4 interrupts. So > >>> this will write to unwanted area. > >>> > >>> There are only 16 SGIs, so it fits in one write to ICACTIVER0. As > >>> wrote above, you also need to deactivate the PPIs. So the following > >>> should > >> be enough: > >>> > >>> /* > >>>* The activate state is unknown at boot, so make sure all SGIs > >>> and PPIs > >> are > >>>* de-activated. > >>>*/ > >>> writel_relaxed(0x, GICD_RDIST_SGI_BASE + GICR_ICACTIVER0); > > > > Just have a following up question, according to > > IHI0069D_gic_architecture_specification > > "8.11.3 GICR_ICACTIVER0, Interrupt Clear-Active Register 0" > > This field resets to a value that is architecturally UNKNOWN, Do we > > need to take SPI into consideration as the following in gic_cpu_init? > > for (i = 0; i < nr_lines; i +=32) > > writel_relaxed(0x, GICD_RDIST_SGI_BASE + GICR_ICACTIVER0 + (i > > / 32) * 4); > > > > and move nr_lines out from gic_dist_init to a static global varaiable. > > You can't clear SPIs through GICD_RDIST_SGI_BASE. You have to use the > GICD. Oops. I got wrong. > > Furthermore, gic_cpu_init is called for every CPU so this is not a good place > for clearing shared interrupts. Shared interrupts should be cleared in > gic_dist_init as this is called only once. Yes. Thanks, Peng. > > Cheers, > > -- > Julien Grall ___ Xen-devel mailing list Xen-devel@lists.xenproject.org https://lists.xenproject.org/mailman/listinfo/xen-devel
Re: [Xen-devel] [PATCH] arm: gic-v3: clear GICR active interrupts
Hi Julien
> -Original Message-
> From: Julien Grall [mailto:julien.gr...@arm.com]
> Sent: 2019年1月30日 20:06
> To: Peng Fan ; sstabell...@kernel.org
> Cc: xen-devel@lists.xenproject.org; Andre Przywara
>
> Subject: Re: [PATCH] arm: gic-v3: clear GICR active interrupts
>
> Hi
>
> Replying to this thread again.
>
> On 22/01/2019 10:54, Julien Grall wrote:
> > Hi Peng,
> >
> > The commit title is a bit confusing. It suggests that all interrupts
> > should be deactivated at boot, however you are only deactivating the SGIs.
> >
> > On 1/22/19 2:35 AM, Peng Fan wrote:
> >> On i.MX8, we implemented partition reboot which means Cortex-A reboot
> >> will not impact M4 cores and System control Unit core. However GICv3
> >> is not reset because hardware design.
> >
> > What do you mean by hardware design? Is it a defect?
> >
> >>
> >> The gic-v3 controller is configured with EOImode to 1, so during xen
> >> reboot, GIC_SGI_CALL_FUNCTION interrupt from CPU0 to other CPUs, but
> >> stop_cpu never return, that means other CPUs have no chance to
> >> deactive the interrupt. During xen booting again, CPU0 will issue
> >> GIC_SGI_CALL_FUNCTION to other CPUs. Because
> GIC_SGI_CALL_FUNCTION of
> >> other CPUs are active during the last reboot, interrupts could not be
> >> triggered unless we deactive the interrupt first.
> >
> > From the description here, I think it not very sane to go to sleep
> > with an interrupt activate.
>
> It looks like I was wrong here. There are case where you can't avoid that (see
> my answer to your other patch) and the boot code cannot rely on the activate
> state of interrupt. So they have to be cleaned at boot.
>
> This also have to be done for PPI and SPIs. Peng, would you mind to do that
> patch?
I'll send out v2 patch soon.
>
> >
> > A better solution would be to move the deactivation earlier on in
> > do_sgi (maybe after eoi_irq) so we call stop_cpu() with all interrupts
> disabled.
> >
> >>
> >> So let's deactive the interrupts during GICv3 initialization to fix
> >
> > s/deactivate/
> >
> >> this issue.
> >
> > Similarly to the commit title, you wrote the commit message very
> generically.
> >
> >
> >>
> >> Signed-off-by: Peng Fan
> >> ---
> >> xen/arch/arm/gic-v3.c | 4
> >> 1 file changed, 4 insertions(+)
> >>
> >> diff --git a/xen/arch/arm/gic-v3.c b/xen/arch/arm/gic-v3.c index
> >> 6fbc106757..643d4a33f0 100644
> >> --- a/xen/arch/arm/gic-v3.c
> >> +++ b/xen/arch/arm/gic-v3.c
> >> @@ -824,8 +824,12 @@ static int gicv3_cpu_init(void)
> >> priority = (GIC_PRI_IPI << 24 | GIC_PRI_IPI << 16 | GIC_PRI_IPI
> >> << 8 |
> >> GIC_PRI_IPI);
> >> for (i = 0; i < NR_GIC_SGI; i += 4)
> >> + {
> >> writel_relaxed(priority,
> >> GICD_RDIST_SGI_BASE + GICR_IPRIORITYR0 +
> (i / 4) *
> >> 4);
> >> + writel_relaxed(0x,
> >> + GICD_RDIST_SGI_BASE + GICR_ICACTIVER0 + (i /
> 4) *
> >> +4);
>
> Each ICACTIVER0 registers hold the active bit for 32 interrupts. However, this
> code assumes the register only hold 4 interrupts. So this will write to
> unwanted area.
>
> There are only 16 SGIs, so it fits in one write to ICACTIVER0. As wrote above,
> you also need to deactivate the PPIs. So the following should be enough:
>
> /*
> * The activate state is unknown at boot, so make sure all SGIs and PPIs are
> * de-activated.
> */
> writel_relaxed(0x, GICD_RDIST_SGI_BASE + GICR_ICACTIVER0);
Thanks,
Peng.
>
> Cheers,
>
> --
> Julien Grall
___
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel
Re: [Xen-devel] [PATCH] arm: gic-v3: clear GICR active interrupts
Hi
Replying to this thread again.
On 22/01/2019 10:54, Julien Grall wrote:
Hi Peng,
The commit title is a bit confusing. It suggests that all interrupts should be
deactivated at boot, however you are only deactivating the SGIs.
On 1/22/19 2:35 AM, Peng Fan wrote:
On i.MX8, we implemented partition reboot which means Cortex-A reboot
will not impact M4 cores and System control Unit core. However GICv3
is not reset because hardware design.
What do you mean by hardware design? Is it a defect?
The gic-v3 controller is configured with EOImode to 1, so during xen
reboot, GIC_SGI_CALL_FUNCTION interrupt from CPU0 to other CPUs, but
stop_cpu never return, that means other CPUs have no chance to
deactive the interrupt. During xen booting again, CPU0 will issue
GIC_SGI_CALL_FUNCTION to other CPUs. Because GIC_SGI_CALL_FUNCTION of
other CPUs are active during the last reboot, interrupts could not be
triggered unless we deactive the interrupt first.
From the description here, I think it not very sane to go to sleep with an
interrupt activate.
It looks like I was wrong here. There are case where you can't avoid that (see
my answer to your other patch) and the boot code cannot rely on the activate
state of interrupt. So they have to be cleaned at boot.
This also have to be done for PPI and SPIs. Peng, would you mind to do that
patch?
A better solution would be to move the deactivation earlier on in do_sgi (maybe
after eoi_irq) so we call stop_cpu() with all interrupts disabled.
So let's deactive the interrupts during GICv3 initialization to fix
s/deactivate/
this issue.
Similarly to the commit title, you wrote the commit message very generically.
Signed-off-by: Peng Fan
---
xen/arch/arm/gic-v3.c | 4
1 file changed, 4 insertions(+)
diff --git a/xen/arch/arm/gic-v3.c b/xen/arch/arm/gic-v3.c
index 6fbc106757..643d4a33f0 100644
--- a/xen/arch/arm/gic-v3.c
+++ b/xen/arch/arm/gic-v3.c
@@ -824,8 +824,12 @@ static int gicv3_cpu_init(void)
priority = (GIC_PRI_IPI << 24 | GIC_PRI_IPI << 16 | GIC_PRI_IPI << 8 |
GIC_PRI_IPI);
for (i = 0; i < NR_GIC_SGI; i += 4)
+ {
writel_relaxed(priority,
GICD_RDIST_SGI_BASE + GICR_IPRIORITYR0 + (i / 4) * 4);
+ writel_relaxed(0x,
+ GICD_RDIST_SGI_BASE + GICR_ICACTIVER0 + (i / 4) * 4);
Each ICACTIVER0 registers hold the active bit for 32 interrupts. However, this
code assumes the register only hold 4 interrupts. So this will write to unwanted
area.
There are only 16 SGIs, so it fits in one write to ICACTIVER0. As wrote above,
you also need to deactivate the PPIs. So the following should be enough:
/*
* The activate state is unknown at boot, so make sure all SGIs and PPIs are
* de-activated.
*/
writel_relaxed(0x, GICD_RDIST_SGI_BASE + GICR_ICACTIVER0);
Cheers,
--
Julien Grall
___
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel
Re: [Xen-devel] [PATCH] arm: gic-v3: clear GICR active interrupts
On 24/01/2019 07:22, Peng Fan wrote:
Hi Julien
Hi Peng,
Are you ok with the following patch?
This looks ok. Please resend it formally with the tag for-4.12 and the release
manager (jgr...@suse.com) in CC.
arm: gic: deactivate sgi immediately after eoi
On i.MX8, we implemented partition reboot which means Cortex-A reboot
will not impact M4 cores and System control Unit core. However GICv3
is not reset because we also need to support A72 Cluster reboot without
affecting A53 Cluster.
The gic-v3 controller is configured with EOImode to 1, so during xen
reboot, there is a function call "smp_call_function(halt_this_cpu, NULL,
0);"
,but halt_this_cpu never return, that means other CPUs have no chance to
deactive the SGI interrupt, because the deactivate_irq operation is at
the end of do_sgi. During xen booting again, CPU0 will issue
GIC_SGI_CALL_FUNCTION to other CPUs. Because GIC_SGI_CALL_FUNCTION of
other CPUs are active during the last reboot, interrupts could not be
triggered unless we deactivate the interrupt first.
To fix this issue, let's move the deactivate_irq operation just after
eoi_irq, then the SGI interrupt will be in deactive state when
smp_call_function_interrupt.
Signed-off-by: Peng Fan
diff --git a/xen/arch/arm/gic.c b/xen/arch/arm/gic.c
index 6cc7dec706..300fdbd9ae 100644
--- a/xen/arch/arm/gic.c
+++ b/xen/arch/arm/gic.c
@@ -347,6 +347,8 @@ static void do_sgi(struct cpu_user_regs *regs, enum gic_sgi
sgi)
/* Lower the priority */
gic_hw_ops->eoi_irq(desc);
+/* Deactivate */
+gic_hw_ops->deactivate_irq(desc);
/*
* Ensure any shared data written by the CPU sending
@@ -370,9 +372,6 @@ static void do_sgi(struct cpu_user_regs *regs, enum gic_sgi
sgi)
panic("Unhandled SGI %d on CPU%d\n", sgi, smp_processor_id());
break;
}
-
-/* Deactivate */
-gic_hw_ops->deactivate_irq(desc);
}
Cheers,
--
Julien Grall
___
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel
Re: [Xen-devel] [PATCH] arm: gic-v3: clear GICR active interrupts
Hi Julien
> -Original Message-
> From: Julien Grall [mailto:julien.gr...@arm.com]
> Sent: 2019年1月22日 18:55
> To: Peng Fan ; sstabell...@kernel.org
> Cc: xen-devel@lists.xenproject.org; Andre Przywara
>
> Subject: Re: [PATCH] arm: gic-v3: clear GICR active interrupts
>
> Hi Peng,
>
> The commit title is a bit confusing. It suggests that all interrupts should be
> deactivated at boot, however you are only deactivating the SGIs.
"arm: gic-v3: deactivate SGI interrupts" seems better.
>
> On 1/22/19 2:35 AM, Peng Fan wrote:
> > On i.MX8, we implemented partition reboot which means Cortex-A reboot
> > will not impact M4 cores and System control Unit core. However GICv3
> > is not reset because hardware design.
>
> What do you mean by hardware design? Is it a defect?
Sorry. This is a wrong information. It is not defect, it is required that we
not reset GIC
when implementing partition reboot. We support Cluster reboot without affecting
the other Cluster, so gic is not reset.
>
> >
> > The gic-v3 controller is configured with EOImode to 1, so during xen
> > reboot, GIC_SGI_CALL_FUNCTION interrupt from CPU0 to other CPUs, but
> > stop_cpu never return, that means other CPUs have no chance to
> > deactive the interrupt. During xen booting again, CPU0 will issue
> > GIC_SGI_CALL_FUNCTION to other CPUs. Because
> GIC_SGI_CALL_FUNCTION of
> > other CPUs are active during the last reboot, interrupts could not be
> > triggered unless we deactive the interrupt first.
>
> From the description here, I think it not very sane to go to sleep with an
> interrupt activate.
>
> A better solution would be to move the deactivation earlier on in do_sgi
> (maybe after eoi_irq) so we call stop_cpu() with all interrupts disabled.
Agree.
>
> >
> > So let's deactive the interrupts during GICv3 initialization to fix
>
> s/deactivate/
Fix in V2.
>
> > this issue.
>
> Similarly to the commit title, you wrote the commit message very generically.
Are you ok with the following patch?
arm: gic: deactivate sgi immediately after eoi
On i.MX8, we implemented partition reboot which means Cortex-A reboot
will not impact M4 cores and System control Unit core. However GICv3
is not reset because we also need to support A72 Cluster reboot without
affecting A53 Cluster.
The gic-v3 controller is configured with EOImode to 1, so during xen
reboot, there is a function call "smp_call_function(halt_this_cpu, NULL,
0);"
,but halt_this_cpu never return, that means other CPUs have no chance to
deactive the SGI interrupt, because the deactivate_irq operation is at
the end of do_sgi. During xen booting again, CPU0 will issue
GIC_SGI_CALL_FUNCTION to other CPUs. Because GIC_SGI_CALL_FUNCTION of
other CPUs are active during the last reboot, interrupts could not be
triggered unless we deactivate the interrupt first.
To fix this issue, let's move the deactivate_irq operation just after
eoi_irq, then the SGI interrupt will be in deactive state when
smp_call_function_interrupt.
Signed-off-by: Peng Fan
diff --git a/xen/arch/arm/gic.c b/xen/arch/arm/gic.c
index 6cc7dec706..300fdbd9ae 100644
--- a/xen/arch/arm/gic.c
+++ b/xen/arch/arm/gic.c
@@ -347,6 +347,8 @@ static void do_sgi(struct cpu_user_regs *regs, enum gic_sgi
sgi)
/* Lower the priority */
gic_hw_ops->eoi_irq(desc);
+/* Deactivate */
+gic_hw_ops->deactivate_irq(desc);
/*
* Ensure any shared data written by the CPU sending
@@ -370,9 +372,6 @@ static void do_sgi(struct cpu_user_regs *regs, enum gic_sgi
sgi)
panic("Unhandled SGI %d on CPU%d\n", sgi, smp_processor_id());
break;
}
-
-/* Deactivate */
-gic_hw_ops->deactivate_irq(desc);
}
Thanks,
Peng.
>
>
> >
> > Signed-off-by: Peng Fan
> > ---
> > xen/arch/arm/gic-v3.c | 4
> > 1 file changed, 4 insertions(+)
> >
> > diff --git a/xen/arch/arm/gic-v3.c b/xen/arch/arm/gic-v3.c
> > index 6fbc106757..643d4a33f0 100644
> > --- a/xen/arch/arm/gic-v3.c
> > +++ b/xen/arch/arm/gic-v3.c
> > @@ -824,8 +824,12 @@ static int gicv3_cpu_init(void)
> > priority = (GIC_PRI_IPI << 24 | GIC_PRI_IPI << 16 | GIC_PRI_IPI <<
> 8 |
> > GIC_PRI_IPI);
> > for (i = 0; i < NR_GIC_SGI; i += 4)
> > +{
> > writel_relaxed(priority,
> > GICD_RDIST_SGI_BASE + GICR_IPRIORITYR0 + (i / 4)
> * 4);
> > +writel_relaxed(0x,
> > +GICD_RDIST_SGI_BASE + GICR_ICACTIVER0 + (i / 4) *
> 4);
> > +}
> >
> > priority = (GIC_PRI_IRQ << 24 | GIC_PRI_IRQ << 16 | GIC_PRI_IRQ
> << 8 |
> > GIC_PRI_IRQ);
> >
>
> Cheers,
>
> --
> Julien Grall
___
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel
Re: [Xen-devel] [PATCH] arm: gic-v3: clear GICR active interrupts
Hi Peng,
The commit title is a bit confusing. It suggests that all interrupts
should be deactivated at boot, however you are only deactivating the SGIs.
On 1/22/19 2:35 AM, Peng Fan wrote:
On i.MX8, we implemented partition reboot which means Cortex-A reboot
will not impact M4 cores and System control Unit core. However GICv3
is not reset because hardware design.
What do you mean by hardware design? Is it a defect?
The gic-v3 controller is configured with EOImode to 1, so during xen
reboot, GIC_SGI_CALL_FUNCTION interrupt from CPU0 to other CPUs, but
stop_cpu never return, that means other CPUs have no chance to
deactive the interrupt. During xen booting again, CPU0 will issue
GIC_SGI_CALL_FUNCTION to other CPUs. Because GIC_SGI_CALL_FUNCTION of
other CPUs are active during the last reboot, interrupts could not be
triggered unless we deactive the interrupt first.
From the description here, I think it not very sane to go to sleep with
an interrupt activate.
A better solution would be to move the deactivation earlier on in do_sgi
(maybe after eoi_irq) so we call stop_cpu() with all interrupts disabled.
So let's deactive the interrupts during GICv3 initialization to fix
s/deactivate/
this issue.
Similarly to the commit title, you wrote the commit message very
generically.
Signed-off-by: Peng Fan
---
xen/arch/arm/gic-v3.c | 4
1 file changed, 4 insertions(+)
diff --git a/xen/arch/arm/gic-v3.c b/xen/arch/arm/gic-v3.c
index 6fbc106757..643d4a33f0 100644
--- a/xen/arch/arm/gic-v3.c
+++ b/xen/arch/arm/gic-v3.c
@@ -824,8 +824,12 @@ static int gicv3_cpu_init(void)
priority = (GIC_PRI_IPI << 24 | GIC_PRI_IPI << 16 | GIC_PRI_IPI << 8 |
GIC_PRI_IPI);
for (i = 0; i < NR_GIC_SGI; i += 4)
+{
writel_relaxed(priority,
GICD_RDIST_SGI_BASE + GICR_IPRIORITYR0 + (i / 4) * 4);
+writel_relaxed(0x,
+GICD_RDIST_SGI_BASE + GICR_ICACTIVER0 + (i / 4) * 4);
+}
priority = (GIC_PRI_IRQ << 24 | GIC_PRI_IRQ << 16 | GIC_PRI_IRQ << 8 |
GIC_PRI_IRQ);
Cheers,
--
Julien Grall
___
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel
[Xen-devel] [PATCH] arm: gic-v3: clear GICR active interrupts
On i.MX8, we implemented partition reboot which means Cortex-A reboot
will not impact M4 cores and System control Unit core. However GICv3
is not reset because hardware design.
The gic-v3 controller is configured with EOImode to 1, so during xen
reboot, GIC_SGI_CALL_FUNCTION interrupt from CPU0 to other CPUs, but
stop_cpu never return, that means other CPUs have no chance to
deactive the interrupt. During xen booting again, CPU0 will issue
GIC_SGI_CALL_FUNCTION to other CPUs. Because GIC_SGI_CALL_FUNCTION of
other CPUs are active during the last reboot, interrupts could not be
triggered unless we deactive the interrupt first.
So let's deactive the interrupts during GICv3 initialization to fix
this issue.
Signed-off-by: Peng Fan
---
xen/arch/arm/gic-v3.c | 4
1 file changed, 4 insertions(+)
diff --git a/xen/arch/arm/gic-v3.c b/xen/arch/arm/gic-v3.c
index 6fbc106757..643d4a33f0 100644
--- a/xen/arch/arm/gic-v3.c
+++ b/xen/arch/arm/gic-v3.c
@@ -824,8 +824,12 @@ static int gicv3_cpu_init(void)
priority = (GIC_PRI_IPI << 24 | GIC_PRI_IPI << 16 | GIC_PRI_IPI << 8 |
GIC_PRI_IPI);
for (i = 0; i < NR_GIC_SGI; i += 4)
+{
writel_relaxed(priority,
GICD_RDIST_SGI_BASE + GICR_IPRIORITYR0 + (i / 4) * 4);
+writel_relaxed(0x,
+GICD_RDIST_SGI_BASE + GICR_ICACTIVER0 + (i / 4) * 4);
+}
priority = (GIC_PRI_IRQ << 24 | GIC_PRI_IRQ << 16 | GIC_PRI_IRQ << 8 |
GIC_PRI_IRQ);
--
2.14.1
___
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel
