[PATCH] ArmPkg/GicV3Dxe: Don't signal EOI on arbitrary interrupts

[PATCH] ArmPkg/GicV3Dxe: Don't signal EOI on arbitrary interrupts

[Ard Biesheuvel]

Currently, at ExitBootServices() time, the GICv3 driver signals
End-Of-Interrupt (EOI) on all interrupt lines that are supported by the
interrupt controller. This appears to have been carried over from the
GICv2 version, but has been turned into something that violates the GIC
spec, and may trigger SError exceptions on some implementations.

Marc puts it as follows:

  The GIC interrupt state machine is pretty strict. An interrupt can
  only be deactivated (with or without prior priority drop) if it has
  been acknowledged first. In GIC speak, this means that only the
  following sequences are valid:

  With EOImode==0:
	  x = ICC_IAR{0,1}_EL1;
	  ICC_EOIR{0,1}_EL1 = x;

  With EOImode==1:
	  x = ICC_IAR{0,1}_EL1;
	  ICC_EOIR{0,1}_EL1 = x;
	  ICC_DIR_EL1 = x;

  Any write to ICC_EOIR{0,1}_EL1 that isn't the direct consequence of
  the same value being read from ICC_IAR{0,1}_EL1, and with the correct
  nesting, breaks the state machine and leads to unpredictable results
  that affects *all* interrupts in the system (most likely, the priority
  system is dead). See Figure 4-3 ("Interrupt handling state machine")
  in Arm IHI 0069F for a description of the acceptable transitions.

  Additionally, on implementations that have ICC_CTLR_EL1.SEIS==1, a
  SError may be generated to signal the error. See the various

  <quote>
	  IMPLEMENTATION_DEFINED "SError ....";
  </quote>

  that are all over the pseudocode contained in the same architecture
  spec. Needless to say, this is pretty final for any SW that would do
  silly things on such implementations (which do exist).

Given that in our implementation, every signalled interrupt is acked,
handled and EOId in sequence, there is no reason to EOI all interrupts
at ExitBootServices() time in the first place, so let's just drop this
code. This fixes an issue reported by Marc where an SError is triggered
by this code, bringing down the system.

Reported-by: Marc Zyngier <maz@kernel.org>
Signed-off-by: Ard Biesheuvel <ardb@kernel.org>
---
This is a clear bugfix, but given how late we are in the cycle, I will
leave it up to Liming to decide whether we can still take this for the
upcoming stable tag.

 ArmPkg/Drivers/ArmGic/GicV3/ArmGicV3Dxe.c | 4 ----
 1 file changed, 4 deletions(-)

diff --git a/ArmPkg/Drivers/ArmGic/GicV3/ArmGicV3Dxe.c b/ArmPkg/Drivers/ArmGic/GicV3/ArmGicV3Dxe.c
index 85ee4c87b6d1..fa515d1a01ba 100644
--- a/ArmPkg/Drivers/ArmGic/GicV3/ArmGicV3Dxe.c
+++ b/ArmPkg/Drivers/ArmGic/GicV3/ArmGicV3Dxe.c
@@ -344,10 +344,6 @@ GicV3ExitBootServicesEvent (
     GicV3DisableInterruptSource (&gHardwareInterruptV3Protocol, Index);
   }
 
-  for (Index = 0; Index < mGicNumInterrupts; Index++) {
-    GicV3EndOfInterrupt (&gHardwareInterruptV3Protocol, Index);
-  }
-
   // Disable Gic Interface
   ArmGicV3DisableInterruptInterface ();
 
-- 
2.30.2

Re: [PATCH] ArmPkg/GicV3Dxe: Don't signal EOI on arbitrary interrupts

[Leif Lindholm]

On Tue, Aug 24, 2021 at 17:31:32 +0200, Ard Biesheuvel wrote:
> Currently, at ExitBootServices() time, the GICv3 driver signals
> End-Of-Interrupt (EOI) on all interrupt lines that are supported by the
> interrupt controller. This appears to have been carried over from the
> GICv2 version, but has been turned into something that violates the GIC
> spec, and may trigger SError exceptions on some implementations.
> 
> Marc puts it as follows:
> 
>   The GIC interrupt state machine is pretty strict. An interrupt can
>   only be deactivated (with or without prior priority drop) if it has
>   been acknowledged first. In GIC speak, this means that only the
>   following sequences are valid:
> 
>   With EOImode==0:
> 	  x = ICC_IAR{0,1}_EL1;
> 	  ICC_EOIR{0,1}_EL1 = x;
> 
>   With EOImode==1:
> 	  x = ICC_IAR{0,1}_EL1;
> 	  ICC_EOIR{0,1}_EL1 = x;
> 	  ICC_DIR_EL1 = x;
> 
>   Any write to ICC_EOIR{0,1}_EL1 that isn't the direct consequence of
>   the same value being read from ICC_IAR{0,1}_EL1, and with the correct
>   nesting, breaks the state machine and leads to unpredictable results
>   that affects *all* interrupts in the system (most likely, the priority
>   system is dead). See Figure 4-3 ("Interrupt handling state machine")
>   in Arm IHI 0069F for a description of the acceptable transitions.
> 
>   Additionally, on implementations that have ICC_CTLR_EL1.SEIS==1, a
>   SError may be generated to signal the error. See the various
> 
>   <quote>
> 	  IMPLEMENTATION_DEFINED "SError ....";
>   </quote>
> 
>   that are all over the pseudocode contained in the same architecture
>   spec. Needless to say, this is pretty final for any SW that would do
>   silly things on such implementations (which do exist).
> 
> Given that in our implementation, every signalled interrupt is acked,
> handled and EOId in sequence, there is no reason to EOI all interrupts
> at ExitBootServices() time in the first place, so let's just drop this
> code. This fixes an issue reported by Marc where an SError is triggered
> by this code, bringing down the system.
> 
> Reported-by: Marc Zyngier <maz@kernel.org>
> Signed-off-by: Ard Biesheuvel <ardb@kernel.org>

Yikes.
Pretty sure that wasn't the right thing to do on gicv2 either.

Reviewed-by: Leif Lindholm <leif@nuviainc.com>

> ---
> This is a clear bugfix, but given how late we are in the cycle, I will
> leave it up to Liming to decide whether we can still take this for the
> upcoming stable tag.

I can see hypothetical situations where this could break existing
(broken) firmware ... so I would prefer to leave it until after the
stable tag unless there is substantial benefit to including it here.

/
    Leif

>  ArmPkg/Drivers/ArmGic/GicV3/ArmGicV3Dxe.c | 4 ----
>  1 file changed, 4 deletions(-)
> 
> diff --git a/ArmPkg/Drivers/ArmGic/GicV3/ArmGicV3Dxe.c b/ArmPkg/Drivers/ArmGic/GicV3/ArmGicV3Dxe.c
> index 85ee4c87b6d1..fa515d1a01ba 100644
> --- a/ArmPkg/Drivers/ArmGic/GicV3/ArmGicV3Dxe.c
> +++ b/ArmPkg/Drivers/ArmGic/GicV3/ArmGicV3Dxe.c
> @@ -344,10 +344,6 @@ GicV3ExitBootServicesEvent (
>      GicV3DisableInterruptSource (&gHardwareInterruptV3Protocol, Index);
>    }
>  
> -  for (Index = 0; Index < mGicNumInterrupts; Index++) {
> -    GicV3EndOfInterrupt (&gHardwareInterruptV3Protocol, Index);
> -  }
> -
>    // Disable Gic Interface
>    ArmGicV3DisableInterruptInterface ();
>  
> -- 
> 2.30.2
> 

回复: [edk2-devel] [PATCH] ArmPkg/GicV3Dxe: Don't signal EOI on arbitrary interrupts

[gaoliming]

I agree with Leif.

Thanks
Liming
> -----邮件原件-----
> 发件人: devel@edk2.groups.io <devel@edk2.groups.io> 代表 Leif Lindholm
> 发送时间: 2021年8月25日 20:51
> 收件人: Ard Biesheuvel <ardb@kernel.org>
> 抄送: devel@edk2.groups.io; sami.mujawar@arm.com;
> gaoliming@byosoft.com.cn; Marc Zyngier <maz@kernel.org>
> 主题: Re: [edk2-devel] [PATCH] ArmPkg/GicV3Dxe: Don't signal EOI on
> arbitrary interrupts
> 
> On Tue, Aug 24, 2021 at 17:31:32 +0200, Ard Biesheuvel wrote:
> > Currently, at ExitBootServices() time, the GICv3 driver signals
> > End-Of-Interrupt (EOI) on all interrupt lines that are supported by the
> > interrupt controller. This appears to have been carried over from the
> > GICv2 version, but has been turned into something that violates the GIC
> > spec, and may trigger SError exceptions on some implementations.
> >
> > Marc puts it as follows:
> >
> >   The GIC interrupt state machine is pretty strict. An interrupt can
> >   only be deactivated (with or without prior priority drop) if it has
> >   been acknowledged first. In GIC speak, this means that only the
> >   following sequences are valid:
> >
> >   With EOImode==0:
> > 	  x = ICC_IAR{0,1}_EL1;
> > 	  ICC_EOIR{0,1}_EL1 = x;
> >
> >   With EOImode==1:
> > 	  x = ICC_IAR{0,1}_EL1;
> > 	  ICC_EOIR{0,1}_EL1 = x;
> > 	  ICC_DIR_EL1 = x;
> >
> >   Any write to ICC_EOIR{0,1}_EL1 that isn't the direct consequence of
> >   the same value being read from ICC_IAR{0,1}_EL1, and with the correct
> >   nesting, breaks the state machine and leads to unpredictable results
> >   that affects *all* interrupts in the system (most likely, the priority
> >   system is dead). See Figure 4-3 ("Interrupt handling state machine")
> >   in Arm IHI 0069F for a description of the acceptable transitions.
> >
> >   Additionally, on implementations that have ICC_CTLR_EL1.SEIS==1, a
> >   SError may be generated to signal the error. See the various
> >
> >   <quote>
> > 	  IMPLEMENTATION_DEFINED "SError ....";
> >   </quote>
> >
> >   that are all over the pseudocode contained in the same architecture
> >   spec. Needless to say, this is pretty final for any SW that would do
> >   silly things on such implementations (which do exist).
> >
> > Given that in our implementation, every signalled interrupt is acked,
> > handled and EOId in sequence, there is no reason to EOI all interrupts
> > at ExitBootServices() time in the first place, so let's just drop this
> > code. This fixes an issue reported by Marc where an SError is triggered
> > by this code, bringing down the system.
> >
> > Reported-by: Marc Zyngier <maz@kernel.org>
> > Signed-off-by: Ard Biesheuvel <ardb@kernel.org>
> 
> Yikes.
> Pretty sure that wasn't the right thing to do on gicv2 either.
> 
> Reviewed-by: Leif Lindholm <leif@nuviainc.com>
> 
> > ---
> > This is a clear bugfix, but given how late we are in the cycle, I will
> > leave it up to Liming to decide whether we can still take this for the
> > upcoming stable tag.
> 
> I can see hypothetical situations where this could break existing
> (broken) firmware ... so I would prefer to leave it until after the
> stable tag unless there is substantial benefit to including it here.
> 
> /
>     Leif
> 
> >  ArmPkg/Drivers/ArmGic/GicV3/ArmGicV3Dxe.c | 4 ----
> >  1 file changed, 4 deletions(-)
> >
> > diff --git a/ArmPkg/Drivers/ArmGic/GicV3/ArmGicV3Dxe.c
> b/ArmPkg/Drivers/ArmGic/GicV3/ArmGicV3Dxe.c
> > index 85ee4c87b6d1..fa515d1a01ba 100644
> > --- a/ArmPkg/Drivers/ArmGic/GicV3/ArmGicV3Dxe.c
> > +++ b/ArmPkg/Drivers/ArmGic/GicV3/ArmGicV3Dxe.c
> > @@ -344,10 +344,6 @@ GicV3ExitBootServicesEvent (
> >      GicV3DisableInterruptSource (&gHardwareInterruptV3Protocol,
> Index);
> >    }
> >
> > -  for (Index = 0; Index < mGicNumInterrupts; Index++) {
> > -    GicV3EndOfInterrupt (&gHardwareInterruptV3Protocol, Index);
> > -  }
> > -
> >    // Disable Gic Interface
> >    ArmGicV3DisableInterruptInterface ();
> >
> > --
> > 2.30.2
> >
> 
> 
> 
> 

Re: [edk2-devel] [PATCH] ArmPkg/GicV3Dxe: Don't signal EOI on arbitrary interrupts

[Ard Biesheuvel]

Merged as #1920

Thanks all,


On Thu, 26 Aug 2021 at 04:47, gaoliming <gaoliming@byosoft.com.cn> wrote:
>
> I agree with Leif.
>
> Thanks
> Liming
> > -----邮件原件-----
> > 发件人: devel@edk2.groups.io <devel@edk2.groups.io> 代表 Leif Lindholm
> > 发送时间: 2021年8月25日 20:51
> > 收件人: Ard Biesheuvel <ardb@kernel.org>
> > 抄送: devel@edk2.groups.io; sami.mujawar@arm.com;
> > gaoliming@byosoft.com.cn; Marc Zyngier <maz@kernel.org>
> > 主题: Re: [edk2-devel] [PATCH] ArmPkg/GicV3Dxe: Don't signal EOI on
> > arbitrary interrupts
> >
> > On Tue, Aug 24, 2021 at 17:31:32 +0200, Ard Biesheuvel wrote:
> > > Currently, at ExitBootServices() time, the GICv3 driver signals
> > > End-Of-Interrupt (EOI) on all interrupt lines that are supported by the
> > > interrupt controller. This appears to have been carried over from the
> > > GICv2 version, but has been turned into something that violates the GIC
> > > spec, and may trigger SError exceptions on some implementations.
> > >
> > > Marc puts it as follows:
> > >
> > >   The GIC interrupt state machine is pretty strict. An interrupt can
> > >   only be deactivated (with or without prior priority drop) if it has
> > >   been acknowledged first. In GIC speak, this means that only the
> > >   following sequences are valid:
> > >
> > >   With EOImode==0:
> > >       x = ICC_IAR{0,1}_EL1;
> > >       ICC_EOIR{0,1}_EL1 = x;
> > >
> > >   With EOImode==1:
> > >       x = ICC_IAR{0,1}_EL1;
> > >       ICC_EOIR{0,1}_EL1 = x;
> > >       ICC_DIR_EL1 = x;
> > >
> > >   Any write to ICC_EOIR{0,1}_EL1 that isn't the direct consequence of
> > >   the same value being read from ICC_IAR{0,1}_EL1, and with the correct
> > >   nesting, breaks the state machine and leads to unpredictable results
> > >   that affects *all* interrupts in the system (most likely, the priority
> > >   system is dead). See Figure 4-3 ("Interrupt handling state machine")
> > >   in Arm IHI 0069F for a description of the acceptable transitions.
> > >
> > >   Additionally, on implementations that have ICC_CTLR_EL1.SEIS==1, a
> > >   SError may be generated to signal the error. See the various
> > >
> > >   <quote>
> > >       IMPLEMENTATION_DEFINED "SError ....";
> > >   </quote>
> > >
> > >   that are all over the pseudocode contained in the same architecture
> > >   spec. Needless to say, this is pretty final for any SW that would do
> > >   silly things on such implementations (which do exist).
> > >
> > > Given that in our implementation, every signalled interrupt is acked,
> > > handled and EOId in sequence, there is no reason to EOI all interrupts
> > > at ExitBootServices() time in the first place, so let's just drop this
> > > code. This fixes an issue reported by Marc where an SError is triggered
> > > by this code, bringing down the system.
> > >
> > > Reported-by: Marc Zyngier <maz@kernel.org>
> > > Signed-off-by: Ard Biesheuvel <ardb@kernel.org>
> >
> > Yikes.
> > Pretty sure that wasn't the right thing to do on gicv2 either.
> >
> > Reviewed-by: Leif Lindholm <leif@nuviainc.com>
> >
> > > ---
> > > This is a clear bugfix, but given how late we are in the cycle, I will
> > > leave it up to Liming to decide whether we can still take this for the
> > > upcoming stable tag.
> >
> > I can see hypothetical situations where this could break existing
> > (broken) firmware ... so I would prefer to leave it until after the
> > stable tag unless there is substantial benefit to including it here.
> >
> > /
> >     Leif
> >
> > >  ArmPkg/Drivers/ArmGic/GicV3/ArmGicV3Dxe.c | 4 ----
> > >  1 file changed, 4 deletions(-)
> > >
> > > diff --git a/ArmPkg/Drivers/ArmGic/GicV3/ArmGicV3Dxe.c
> > b/ArmPkg/Drivers/ArmGic/GicV3/ArmGicV3Dxe.c
> > > index 85ee4c87b6d1..fa515d1a01ba 100644
> > > --- a/ArmPkg/Drivers/ArmGic/GicV3/ArmGicV3Dxe.c
> > > +++ b/ArmPkg/Drivers/ArmGic/GicV3/ArmGicV3Dxe.c
> > > @@ -344,10 +344,6 @@ GicV3ExitBootServicesEvent (
> > >      GicV3DisableInterruptSource (&gHardwareInterruptV3Protocol,
> > Index);
> > >    }
> > >
> > > -  for (Index = 0; Index < mGicNumInterrupts; Index++) {
> > > -    GicV3EndOfInterrupt (&gHardwareInterruptV3Protocol, Index);
> > > -  }
> > > -
> > >    // Disable Gic Interface
> > >    ArmGicV3DisableInterruptInterface ();
> > >
> > > --
> > > 2.30.2
> > >
> >
> >
> >
> >
>
>
>
>
>
> 
>
>