From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from mail02.groups.io (mail02.groups.io [66.175.222.108]) by spool.mail.gandi.net (Postfix) with ESMTPS id 14D8E74003E for ; Wed, 2 Aug 2023 16:26:09 +0000 (UTC) DKIM-Signature: a=rsa-sha256; bh=tH15tjZbdmBFCwMLHxwR/XA5vlbVKNqYMtpKCr8g+YA=; c=relaxed/simple; d=groups.io; h=MIME-Version:References:In-Reply-To:From:Date:Message-ID:Subject:To:Cc:Precedence:List-Subscribe:List-Help:Sender:List-Id:Mailing-List:Delivered-To:Reply-To:List-Unsubscribe-Post:List-Unsubscribe:Content-Type; s=20140610; t=1690993568; v=1; b=WmV7vbpkYOXG1mERmmSMYjLFGe/xIpcTLyzouCx+JdwVni9GHYWcrYfyy6fXxjETef2Zttsi vftV6kA3G3Qw5q69pPOyXkrs/LoxwoE9b3+zD726O2/Sr3vidVy/+zPLlmx9FJkq0ur8EcwNwnH GHJI6MpPlDNl6F3N6HAb9JzI= X-Received: by 127.0.0.2 with SMTP id NqBsYY7687511xrefTVN0cDT; Wed, 02 Aug 2023 09:26:08 -0700 X-Received: from dfw.source.kernel.org (dfw.source.kernel.org [139.178.84.217]) by mx.groups.io with SMTP id smtpd.web11.798.1690993568102540197 for ; Wed, 02 Aug 2023 09:26:08 -0700 X-Received: from smtp.kernel.org (relay.kernel.org [52.25.139.140]) (using TLSv1.3 with cipher TLS_AES_256_GCM_SHA384 (256/256 bits) key-exchange X25519 server-signature RSA-PSS (2048 bits)) (No client certificate requested) by dfw.source.kernel.org (Postfix) with ESMTPS id 726AD61A33 for ; Wed, 2 Aug 2023 16:26:07 +0000 (UTC) X-Received: by smtp.kernel.org (Postfix) with ESMTPSA id D3C75C433CA for ; Wed, 2 Aug 2023 16:26:06 +0000 (UTC) X-Received: by mail-lf1-f44.google.com with SMTP id 2adb3069b0e04-4fe4f5290daso16490e87.1 for ; Wed, 02 Aug 2023 09:26:06 -0700 (PDT) X-Gm-Message-State: hFnA1UlJeZqovpYJx7gQXrKax7686176AA= X-Google-Smtp-Source: APBJJlHsS6oSO8AgeDWLaxkKsllftJRXm2QBFDhgcHYp+Jj5WxyFwUcIhblUOZy8ig3r9YMYPujdGrRsBhxPf/4XOKE= X-Received: by 2002:a05:6512:2e8:b0:4fd:ba8d:d4ed with SMTP id m8-20020a05651202e800b004fdba8dd4edmr4444407lfq.24.1690993564845; Wed, 02 Aug 2023 09:26:04 -0700 (PDT) MIME-Version: 1.0 References: <20230731220428.23002-1-osde@linux.microsoft.com> <787bd2e6-c50e-a2a6-735c-ff0d9146685d@linux.microsoft.com> In-Reply-To: <787bd2e6-c50e-a2a6-735c-ff0d9146685d@linux.microsoft.com> From: "Ard Biesheuvel" Date: Wed, 2 Aug 2023 18:25:53 +0200 X-Gmail-Original-Message-ID: Message-ID: Subject: Re: [edk2-devel] [PATCH v1 1/1] ArmPkg: DefaultExceptionHandlerLib: Do Not Allocate Memory To: Oliver Smith-Denny Cc: devel@edk2.groups.io, Leif Lindholm , Ard Biesheuvel , Sami Mujawar , Gerd Hoffmann Precedence: Bulk List-Subscribe: List-Help: Sender: devel@edk2.groups.io List-Id: Mailing-List: list devel@edk2.groups.io; contact devel+owner@edk2.groups.io Reply-To: devel@edk2.groups.io,ardb@kernel.org List-Unsubscribe-Post: List-Unsubscribe=One-Click List-Unsubscribe: Content-Type: text/plain; charset="UTF-8" X-GND-Status: LEGIT Authentication-Results: spool.mail.gandi.net; dkim=pass header.d=groups.io header.s=20140610 header.b=WmV7vbpk; dmarc=fail reason="SPF not aligned (relaxed), DKIM not aligned (relaxed)" header.from=kernel.org (policy=none); spf=pass (spool.mail.gandi.net: domain of bounce@groups.io designates 66.175.222.108 as permitted sender) smtp.mailfrom=bounce@groups.io On Wed, 2 Aug 2023 at 18:21, Oliver Smith-Denny wrote: > > On 8/2/2023 9:06 AM, Ard Biesheuvel wrote: > > On Tue, 1 Aug 2023 at 00:04, Oliver Smith-Denny > > wrote: > >> > >> If gST->ConOut is available when Arm's DefaultExceptionHandler > >> is running, AsciiPrint will get called to attempt to print to > >> ConOut, in addition to the serial output. > >> > >> AsciiPrint calls AsciiInternalPrint in UefiLibPrint.c which > >> in turn calls AllocatePool to allocate a buffer to convert > >> the Ascii input string to a Unicode string to pass to > >> ConOut->OutputString. > >> > >> Per the comment on DefaultExceptionHandler, we should not be > >> allocating memory in the exception handler, as this can cause > >> the exception handler to fail if we had a memory exception or > >> the system state is such that we cannot allocate memory. > >> > >> It has been observed on ArmVirtQemu that exceptions generated > >> in the memory handling code will fail to output the stack dump > >> and CPU state that is critical to debugging because the > >> AllocatePool will fail. > >> > >> This patch fixes the Arm and AARCH64 DefaultExceptionHandlers to > >> not allocate memory when ConOut is available and instead use > >> stack memory to convert the Ascii string needed for SerialPortWrite > >> to the Unicode string needed for ConOut->OutputString. Correspondingly, > >> ArmVirtQemu can now output the stack dump and CPU state when hitting > >> an exception in memory code. > >> > >> GitHub PR: https://github.com/tianocore/edk2/pull/4703 > >> > >> Cc: Leif Lindholm > >> Cc: Ard Biesheuvel > >> Cc: Sami Mujawar > >> Cc: Gerd Hoffmann > >> > >> Signed-off-by: Oliver Smith-Denny > > > > Thanks a lot for fixing this. > > > > Is calling into gST->ConOut guaranteed to be safe in this regard? Or > > is it still best effort? > > > > > > Yeah, this is something I worried about when fixing this. It is very > much best effort, because there are no guarantees that OutputString will > not allocate memory (or some other such unsafe operation in an exception > handler). With the ability to BYO ConOut stack and the complications > with graphics that can be involved here, I personally would be happy to > drop the ConOut call entirely and rely on the serial output. > > In my testing, this did work on ArmVirtQemu and I can envision a case > where having the ConOut output would be nice, but I tend towards lets > keep the exception handler as simple as possible and make sure we get > the useful information dumped out. > > Another option would be to keep this change but move the ConOut calls > to after all of the serial output, so that if we do get a recursive > exception in ConOut, at least we got our serial output. > > Let me know what you think, happy to spin up a v2. > Yeah, that seems like a nice approach - it will still be best effort, but it won't derail the more reliable serial console if it fails. That probably also means we shouldn't bother printing the 'recursive exception' to ConOut at all, or perhap have a special 'recursive exception in conout' flag on top of the existing one but that might be overkill. -=-=-=-=-=-=-=-=-=-=-=- Groups.io Links: You receive all messages sent to this group. View/Reply Online (#107461): https://edk2.groups.io/g/devel/message/107461 Mute This Topic: https://groups.io/mt/100472023/7686176 Group Owner: devel+owner@edk2.groups.io Unsubscribe: https://edk2.groups.io/g/devel/unsub [rebecca@openfw.io] -=-=-=-=-=-=-=-=-=-=-=-