From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from mail-wr1-f45.google.com (mail-wr1-f45.google.com [209.85.221.45]) by mx.groups.io with SMTP id smtpd.web09.14492.1629895846681827913 for ; Wed, 25 Aug 2021 05:50:47 -0700 Authentication-Results: mx.groups.io; dkim=pass header.i=@nuviainc-com.20150623.gappssmtp.com header.s=20150623 header.b=qAi+o5VN; spf=pass (domain: nuviainc.com, ip: 209.85.221.45, mailfrom: leif@nuviainc.com) Received: by mail-wr1-f45.google.com with SMTP id n5so23915669wro.12 for ; Wed, 25 Aug 2021 05:50:46 -0700 (PDT) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=nuviainc-com.20150623.gappssmtp.com; s=20150623; h=date:from:to:cc:subject:message-id:references:mime-version :content-disposition:in-reply-to; bh=DYf8XY7c2gTTzU96apMh5BHJfWgFA3HeeaWCEZTWZNw=; b=qAi+o5VNe453Xgo896zNUmAO5vmGzsX7PZ20ZF2XrKwqHPFs4vOUq9faNuSMPn2B6u oSstaE53F3qleyBUmKF6ACSte6W00TQjk8JL0zCIyvaA5AFDe7fDFjZWoaXQzXkO3Q7X BMuof1pWNZOEqznYUfrs+w17hFI2wDN1gXundOjUILjLbwNbOxL05pRpJjqF/FKBfAJb RpkbNxU9Djgb9F7fUCOk/3MtoRUgm2sbFT12IDQKsa/4Orie/ancvZHj0RXNsjMwOt9j 7Vj2BBxLJ3UcBtzMbk9YrXb0yTTaA+SNY90zSq3XI0k6oNISxNWDCtsqTV87EebVCRef Zsfw== X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20161025; h=x-gm-message-state:date:from:to:cc:subject:message-id:references :mime-version:content-disposition:in-reply-to; bh=DYf8XY7c2gTTzU96apMh5BHJfWgFA3HeeaWCEZTWZNw=; b=DWZpL8BgaDbskFhPJXtS42WJqnl9FX13AOsgA5xM3BMT5Z/oMoDEvWmavpMvouu+8i 8kq4dtnegj8YQVKwJllLDMqNgLUtTUeDs84uoXAHDXewTUQRL0VSYmrkpOjSW+Tr/+ue ldDchzhqWQKhcCuKNSJbWY8hgsp7CR9HrCQU/b26Qy6LKVpJN1eOg0+1CHfkVctjJtK6 d13QswOsvd1Ynl2HdItMkrv9FYzxJoz1XXS4OYfD38WmDFoxnGeVZVhpbonkMuIV6K68 aF6D2GaL1oR+ygI1TS4S6SN+O0iDXofIpo/7zoPmM6K4SoTdtoSiWP6q0xlp+m6ubjVo 2Vxg== X-Gm-Message-State: AOAM533tMSFDwsnVbTN5zSZfV4L1sMQuPs8TQO1bOGkEDIhc4rZWJPoR MNl67pYj6DCjLRJRAj1tauAJeg== X-Google-Smtp-Source: ABdhPJw1KzxvwBhwBMW9Z/VvZQPlhQPt3v+mOcb+I5TgeKjOWIhsM0RyY/sqg2MzqdTtpx7wxI6eRw== X-Received: by 2002:a5d:470b:: with SMTP id y11mr4322766wrq.213.1629895845238; Wed, 25 Aug 2021 05:50:45 -0700 (PDT) Return-Path: Received: from leviathan (cpc92314-cmbg19-2-0-cust559.5-4.cable.virginm.net. [82.11.186.48]) by smtp.gmail.com with ESMTPSA id f7sm5285899wmh.20.2021.08.25.05.50.44 (version=TLS1_3 cipher=TLS_AES_256_GCM_SHA384 bits=256/256); Wed, 25 Aug 2021 05:50:44 -0700 (PDT) Date: Wed, 25 Aug 2021 13:50:42 +0100 From: "Leif Lindholm" To: Ard Biesheuvel Cc: devel@edk2.groups.io, sami.mujawar@arm.com, gaoliming@byosoft.com.cn, Marc Zyngier Subject: Re: [PATCH] ArmPkg/GicV3Dxe: Don't signal EOI on arbitrary interrupts Message-ID: <20210825125042.ga3ls2th2l2dotnd@leviathan> References: <20210824153132.5379-1-ardb@kernel.org> MIME-Version: 1.0 In-Reply-To: <20210824153132.5379-1-ardb@kernel.org> Content-Type: text/plain; charset=us-ascii Content-Disposition: inline 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 > > > 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 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 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 >