From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from mail.byosoft.com.cn (mail.byosoft.com.cn [58.240.74.242]) by mx.groups.io with SMTP id smtpd.web11.25413.1629946037291643563 for ; Wed, 25 Aug 2021 19:47:18 -0700 Authentication-Results: mx.groups.io; dkim=missing; spf=none, err=permanent DNS error (domain: byosoft.com.cn, ip: 58.240.74.242, mailfrom: gaoliming@byosoft.com.cn) Received: from DESKTOPS6D0PVI ([58.246.60.130]) (envelope-sender ) by 192.168.6.13 with ESMTP for ; Thu, 26 Aug 2021 10:47:09 +0800 X-WM-Sender: gaoliming@byosoft.com.cn X-Originating-IP: 58.246.60.130 X-WM-AuthFlag: YES X-WM-AuthUser: gaoliming@byosoft.com.cn From: "gaoliming" To: , , "'Ard Biesheuvel'" Cc: , "'Marc Zyngier'" References: <20210824153132.5379-1-ardb@kernel.org> <20210825125042.ga3ls2th2l2dotnd@leviathan> In-Reply-To: <20210825125042.ga3ls2th2l2dotnd@leviathan> Subject: =?UTF-8?B?5Zue5aSNOiBbZWRrMi1kZXZlbF0gW1BBVENIXSBBcm1Qa2cvR2ljVjNEeGU6IERvbid0IHNpZ25hbCBFT0kgb24gYXJiaXRyYXJ5IGludGVycnVwdHM=?= Date: Thu, 26 Aug 2021 10:47:11 +0800 Message-ID: <042101d79a24$ab39c8b0$01ad5a10$@byosoft.com.cn> MIME-Version: 1.0 X-Mailer: Microsoft Outlook 16.0 Thread-Index: AQDdnsD4CNtASN06k5ibxpov29yPEQGdSemYrWwhEsA= Content-Type: text/plain; charset="gb2312" Content-Transfer-Encoding: quoted-printable Content-Language: zh-cn I agree with Leif. Thanks Liming > -----=D3=CA=BC=FE=D4=AD=BC=FE----- > =B7=A2=BC=FE=C8=CB: devel@edk2.groups.io =B4=FA=B1= =ED Leif Lindholm > =B7=A2=CB=CD=CA=B1=BC=E4: 2021=C4=EA8=D4=C225=C8=D5 20:51 > =CA=D5=BC=FE=C8=CB: Ard Biesheuvel > =B3=AD=CB=CD: devel@edk2.groups.io; sami.mujawar@arm.com; > gaoliming@byosoft.com.cn; Marc Zyngier > =D6=F7=CC=E2: Re: [edk2-devel] [PATCH] ArmPkg/GicV3Dxe: Don't signal EOI = on > arbitrary interrupts >=20 > 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=3D=3D0: > > x =3D ICC_IAR{0,1}_EL1; > > ICC_EOIR{0,1}_EL1 =3D x; > > > > With EOImode=3D=3D1: > > x =3D ICC_IAR{0,1}_EL1; > > ICC_EOIR{0,1}_EL1 =3D x; > > ICC_DIR_EL1 =3D 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 priorit= y > > 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=3D=3D1, = a > > SError may be generated to signal the error. See the various > > > > > > IMPLEMENTATION_DEFINED "SError ...."; > > > > > > 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 > > Signed-off-by: Ard Biesheuvel >=20 > Yikes. > Pretty sure that wasn't the right thing to do on gicv2 either. >=20 > Reviewed-by: Leif Lindholm >=20 > > --- > > 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. >=20 > 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. >=20 > / > Leif >=20 > > 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 =3D 0; Index < mGicNumInterrupts; Index++) { > > - GicV3EndOfInterrupt (&gHardwareInterruptV3Protocol, Index); > > - } > > - > > // Disable Gic Interface > > ArmGicV3DisableInterruptInterface (); > > > > -- > > 2.30.2 > > >=20 >=20 >=20 >=20