From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from mail.kernel.org (mail.kernel.org [198.145.29.99]) by mx.groups.io with SMTP id smtpd.web08.7615.1630068872734614130 for ; Fri, 27 Aug 2021 05:54:32 -0700 Authentication-Results: mx.groups.io; dkim=pass header.i=@kernel.org header.s=k20201202 header.b=nL48BiX4; spf=pass (domain: kernel.org, ip: 198.145.29.99, mailfrom: ardb@kernel.org) Received: by mail.kernel.org (Postfix) with ESMTPSA id E9DC060FE6 for ; Fri, 27 Aug 2021 12:54:31 +0000 (UTC) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/simple; d=kernel.org; s=k20201202; t=1630068871; bh=/D8vMk/8S71cc8OSJ7RrKna1p7YbLigLUvVRLkqLru4=; h=References:In-Reply-To:From:Date:Subject:To:Cc:From; b=nL48BiX4WzlRuyv8ynq9Jf3vWDQSteSI5/Gg68i2dVrYOCFlL31sL0mp8+zL9hs/f FyPX+mztWGFnHPFjhT2t+YToPNNykI/l3Ouevz7P48F76hwy0B/VNxlGU2h2Dm1XfV oWeyoWnjaJGkcaT5+CfZUQRn9NStO/fi2pAFZbbftFJUkvy1lzPJcYNtCkCDygdZAR f0lAz/uxSScnOKvu3fzZfnmHo8xTXaGwti7/vk4rqnkCR48fadW75gmYwHD7w2h8RG mxp9AN+g5l9UeojastNnw7j+RsaAaTw2BNXit+D8d+aglHnYoiVjJbE5k/ZClEnf+R p98j+FvQL1Tgg== Received: by mail-oo1-f42.google.com with SMTP id t1-20020a4a54010000b02902638ef0f883so1950127ooa.11 for ; Fri, 27 Aug 2021 05:54:31 -0700 (PDT) X-Gm-Message-State: AOAM530bzz6RC2rYnYV7EF7k2NaXJFNrjBeuF92yunmBDYOwkhghNuC5 5LtX/bwR3woJ8r7Q3KlLOJhh/p3P+4AREbKF4M0= X-Google-Smtp-Source: ABdhPJwKW9BUYMQMlx3fNycBmqopy5L4RWr1KXdcAG5dNrjEQFETX4LalgP+KdE2IUaP4iHEoQ6KwUb1yJZXn81JE2c= X-Received: by 2002:a4a:d752:: with SMTP id h18mr7409469oot.13.1630068871213; Fri, 27 Aug 2021 05:54:31 -0700 (PDT) MIME-Version: 1.0 References: <20210824153132.5379-1-ardb@kernel.org> <20210825125042.ga3ls2th2l2dotnd@leviathan> <042101d79a24$ab39c8b0$01ad5a10$@byosoft.com.cn> In-Reply-To: <042101d79a24$ab39c8b0$01ad5a10$@byosoft.com.cn> From: "Ard Biesheuvel" Date: Fri, 27 Aug 2021 14:54:19 +0200 X-Gmail-Original-Message-ID: Message-ID: Subject: Re: [edk2-devel] [PATCH] ArmPkg/GicV3Dxe: Don't signal EOI on arbitrary interrupts To: edk2-devel-groups-io , "Liming Gao (Byosoft address)" Cc: Leif Lindholm , Sami Mujawar , Marc Zyngier Content-Type: text/plain; charset="UTF-8" Content-Transfer-Encoding: quoted-printable Merged as #1920 Thanks all, On Thu, 26 Aug 2021 at 04:47, gaoliming wrote: > > I agree with Leif. > > Thanks > Liming > > -----=E9=82=AE=E4=BB=B6=E5=8E=9F=E4=BB=B6----- > > =E5=8F=91=E4=BB=B6=E4=BA=BA: devel@edk2.groups.io =E4=BB=A3=E8=A1=A8 Leif Lindholm > > =E5=8F=91=E9=80=81=E6=97=B6=E9=97=B4: 2021=E5=B9=B48=E6=9C=8825=E6=97= =A5 20:51 > > =E6=94=B6=E4=BB=B6=E4=BA=BA: Ard Biesheuvel > > =E6=8A=84=E9=80=81: devel@edk2.groups.io; sami.mujawar@arm.com; > > gaoliming@byosoft.com.cn; Marc Zyngier > > =E4=B8=BB=E9=A2=98: Re: [edk2-devel] [PATCH] ArmPkg/GicV3Dxe: Don't sig= nal 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 t= he > > > interrupt controller. This appears to have been carried over from the > > > GICv2 version, but has been turned into something that violates the G= IC > > > 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 corre= ct > > > nesting, breaks the state machine and leads to unpredictable result= s > > > that affects *all* interrupts in the system (most likely, the prior= ity > > > 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 d= o > > > 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 interrupt= s > > > at ExitBootServices() time in the first place, so let's just drop thi= s > > > code. This fixes an issue reported by Marc where an SError is trigger= ed > > > by this code, bringing down the system. > > > > > > Reported-by: Marc Zyngier > > > Signed-off-by: Ard Biesheuvel > > > > Yikes. > > Pretty sure that wasn't the right thing to do on gicv2 either. > > > > Reviewed-by: Leif Lindholm > > > > > --- > > > This is a clear bugfix, but given how late we are in the cycle, I wil= l > > > leave it up to Liming to decide whether we can still take this for th= e > > > 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 =3D 0; Index < mGicNumInterrupts; Index++) { > > > - GicV3EndOfInterrupt (&gHardwareInterruptV3Protocol, Index); > > > - } > > > - > > > // Disable Gic Interface > > > ArmGicV3DisableInterruptInterface (); > > > > > > -- > > > 2.30.2 > > > > > > > > > > > > > > > > >=20 > >