[PATCH for-4.12] arm: gic: deactivate sgi immediately after eoi

[PATCH for-4.12] arm: gic: deactivate sgi immediately after eoi

[Peng Fan]

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 <peng.fan@nxp.com>
---
 xen/arch/arm/gic.c | 5 ++---
 1 file changed, 2 insertions(+), 3 deletions(-)

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);
 }
 
 /* Accept an interrupt from the GIC and dispatch its handler */
-- 
2.14.1


_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel

Re: [PATCH for-4.12] arm: gic: deactivate sgi immediately after eoi

[Juergen Gross]

On 24/01/2019 13:43, 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 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 <peng.fan@nxp.com>

Release-acked-by: Juergen Gross <jgross@suse.com>


Juergen

_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel

Re: [PATCH for-4.12] arm: gic: deactivate sgi immediately after eoi

[Julien Grall]

(+ Andre)

Hi Peng,

On 24/01/2019 12:43, 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 we also need to support A72 Cluster reboot without
> affecting A53 Cluster.

How about the other interrupts? For instance, it would be theoretically possible 
to have a PPI/SPI active and receive the order to turn it off. Is this going to 
be an issue?

> 
> 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

s/,/, /

> deactive the SGI interrupt, because the deactivate_irq operation is at

s/deactive/deactivate/	

> the end of do_sgi. During xen booting again, CPU0 will issue

s/xen/Xen/

Also:

"During the next boot of Xen,"

> 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

s/of other/on the others/

> triggered unless we deactivate the interrupt first.

I think it would be clearer if you say: "As the Active state for SGI is left 
untouched during the reboot, the GIC_SGI_CALL_FUNCTION will still be active on 
the non-boot CPUs. This means the interrupt cannot be triggered again until it 
get deactivated."

> 
> 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

s/deactive/deactivate/

> smp_call_function_interrupt.

I would add: "This is fine because the interrupts are masked while handling the 
SGI.".

> 
> Signed-off-by: Peng Fan <peng.fan@nxp.com>

I think the patch makes sense in itself. This is a saner behavior than turning 
off the CPU with active interrupts. I want to double-check a few things before 
committing it. But I will definitely commit it by the end of the week.

No need to resend the patch if there are no changes in the code. I can take care 
for the commit message.

Cheers,

-- 
Julien Grall

_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel

Re: [PATCH for-4.12] arm: gic: deactivate sgi immediately after eoi

[Peng Fan]

Hi Julien

> -----Original Message-----
> From: Julien Grall [mailto:julien.grall@arm.com]
> Sent: 2019年1月29日 23:57
> To: Peng Fan <peng.fan@nxp.com>; sstabellini@kernel.org; jgross@suse.com
> Cc: xen-devel@lists.xenproject.org; Andre Przywara
> <andre.przywara@arm.com>
> Subject: Re: [PATCH for-4.12] arm: gic: deactivate sgi immediately after eoi
> 
> (+ Andre)
> 
> Hi Peng,
> 
> On 24/01/2019 12:43, 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 we also need to support A72 Cluster reboot
> > without affecting A53 Cluster.
> 
> How about the other interrupts? For instance, it would be theoretically
> possible to have a PPI/SPI active and receive the order to turn it off. Is this
> going to be an issue?

Currently we only met this issue when doing Xen reboot, because halt_this_cpu never return.
For other interrupts, I think they are deactivated properly.

> 
> >
> > 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
> 
> s/,/, /
> 
> > deactive the SGI interrupt, because the deactivate_irq operation is at
> 
> s/deactive/deactivate/
> 
> > the end of do_sgi. During xen booting again, CPU0 will issue
> 
> s/xen/Xen/
> 
> Also:
> 
> "During the next boot of Xen,"
> 
> > 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
> 
> s/of other/on the others/
> 
> > triggered unless we deactivate the interrupt first.
> 
> I think it would be clearer if you say: "As the Active state for SGI is left
> untouched during the reboot, the GIC_SGI_CALL_FUNCTION will still be
> active on the non-boot CPUs. This means the interrupt cannot be triggered
> again until it get deactivated."
> 
> >
> > 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
> 
> s/deactive/deactivate/
> 
> > smp_call_function_interrupt.
> 
> I would add: "This is fine because the interrupts are masked while handling
> the
> SGI.".
> 
> >
> > Signed-off-by: Peng Fan <peng.fan@nxp.com>
> 
> I think the patch makes sense in itself. This is a saner behavior than turning
> off the CPU with active interrupts. I want to double-check a few things before
> committing it. But I will definitely commit it by the end of the week.
> 
> No need to resend the patch if there are no changes in the code. I can take
> care
> for the commit message.

Thanks,
Peng.

> 
> Cheers,
> 
> --
> Julien Grall
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel

Re: [PATCH for-4.12] arm: gic: deactivate sgi immediately after eoi

[Julien Grall]

On 30/01/2019 01:21, Peng Fan wrote:
> Hi Julien

Hi Peng,

>> -----Original Message-----
>> From: Julien Grall [mailto:julien.grall@arm.com]
>> Sent: 2019年1月29日 23:57
>> To: Peng Fan <peng.fan@nxp.com>; sstabellini@kernel.org; jgross@suse.com
>> Cc: xen-devel@lists.xenproject.org; Andre Przywara
>> <andre.przywara@arm.com>
>> Subject: Re: [PATCH for-4.12] arm: gic: deactivate sgi immediately after eoi
>>
>> (+ Andre)
>>
>> Hi Peng,
>>
>> On 24/01/2019 12:43, 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 we also need to support A72 Cluster reboot
>>> without affecting A53 Cluster.
>>
>> How about the other interrupts? For instance, it would be theoretically
>> possible to have a PPI/SPI active and receive the order to turn it off. Is this
>> going to be an issue?
> 
> Currently we only met this issue when doing Xen reboot, because halt_this_cpu never return.
> For other interrupts, I think they are deactivated properly.

I don't think so, IPIs has an higher priority than all the other interrupt. So 
you may receive the IPI while handling another interrupts. This will leave the 
interrupts activated while turning off the processor.

I spoke with Marc Z. (GIC expert) today regarding rebooting/kexecing with 
interrupts activated. He said this is a valid case and the boot code should take 
care of de-activating all interrupts. So your first patch might be more suitable 
here. Sorry for the inconvenience. I will comment back on the first one again.

Cheers,

-- 
Julien Grall

_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel