From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received-SPF: Pass (sender SPF authorized) identity=mailfrom; client-ip=2a00:1450:400c:c0c::22d; helo=mail-wr0-x22d.google.com; envelope-from=leif.lindholm@linaro.org; receiver=edk2-devel@lists.01.org Received: from mail-wr0-x22d.google.com (mail-wr0-x22d.google.com [IPv6:2a00:1450:400c:c0c::22d]) (using TLSv1.2 with cipher ECDHE-RSA-AES128-GCM-SHA256 (128/128 bits)) (No client certificate requested) by ml01.01.org (Postfix) with ESMTPS id F034820954CC0 for ; Tue, 20 Feb 2018 09:02:20 -0800 (PST) Received: by mail-wr0-x22d.google.com with SMTP id n7so16468527wrn.5 for ; Tue, 20 Feb 2018 09:08:19 -0800 (PST) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=linaro.org; s=google; h=date:from:to:cc:subject:message-id:references:mime-version :content-disposition:in-reply-to:user-agent; bh=T1fsS2cKabes8o+yZa1zXkezVDjQLCst1mlUnHA7qnI=; b=kOupqVm8OFR1lFn8/g6OVuhHraovgnWZEEHkcS+0Nl43inwva/KmzCo+cN9qW4wqmC d3YeVw2dgAQrPFm6QT6VW4OD8wJOjS0WigdYhgv5Lrg0Srgi8xQXlCRzHe10QnHrIjb0 QiuLwwe1RKYLEfBmC0yLqH/3k0a8jWpmBqTew= 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:user-agent; bh=T1fsS2cKabes8o+yZa1zXkezVDjQLCst1mlUnHA7qnI=; b=VazUKmFi+LbFKOF27k/nP0rejkAmCGYKKCkF02h02Vw/3g+g+Nu+MCxk8Dss8veLQT m+1uzY+CywdXZNkOVa1qvIm5sCHe4w1uUAB8Mi7RVN3mpirXEZhfYXl35ICVqxGQkKWU qHf7aQwm6soLssbafTYxd8dYDQRzqr/lRpdcSNmnIdnrlAgQBC/rqZZi24E0B/vUTRva oAC4NGXLiM0YQxfk99h72MWyiRfSEF+MFVpZiNmzz8EoqN9bBCk2Ey4jv7i6oy5tYZtW 4w8pPS3Rv9EmNUe5JTK5tqS0FyK7mgwE9lmzFi3YreX8I6g72ttRPj9OQlEBDuR6kel3 0kbw== X-Gm-Message-State: APf1xPDNsTy+BTCCL0FpC5KdrZtDjH6axcaj1seAtM5dxWIKLj+crRCa 9shYyOU5YnTrY4T3aLwxr/bPPQ== X-Google-Smtp-Source: AH8x2240vtlJxuHOHCKm2eXbkyT8k6zNLQ04RV/wGde0ayNHtV42XDJ7d3q40mpwN212Hutwy8qG3w== X-Received: by 10.223.183.43 with SMTP id l43mr270402wre.265.1519146497877; Tue, 20 Feb 2018 09:08:17 -0800 (PST) Received: from bivouac.eciton.net (bivouac.eciton.net. [2a00:1098:0:86:1000:23:0:2]) by smtp.gmail.com with ESMTPSA id 67sm26877382wmg.13.2018.02.20.09.08.16 (version=TLS1_2 cipher=ECDHE-RSA-CHACHA20-POLY1305 bits=256/256); Tue, 20 Feb 2018 09:08:16 -0800 (PST) Date: Tue, 20 Feb 2018 17:08:15 +0000 From: Leif Lindholm To: Andrew Fish Cc: Ard Biesheuvel , ruiyu.ni@intel.com, edk2-devel@lists.01.org, liming.gao@intel.com, Mike Kinney , lersek@redhat.com, star.zeng@intel.com Message-ID: <20180220170815.q2n52uoystuhvexm@bivouac.eciton.net> References: <20180220110524.9050-1-ard.biesheuvel@linaro.org> <20180220110524.9050-2-ard.biesheuvel@linaro.org> <20180220141641.p47czk4yb74xutyu@bivouac.eciton.net> MIME-Version: 1.0 In-Reply-To: User-Agent: NeoMutt/20170113 (1.7.2) Subject: Re: [PATCH 1/3] MdePkg: introduce DxeRuntimeDebugLibSerialPort X-BeenThere: edk2-devel@lists.01.org X-Mailman-Version: 2.1.23 Precedence: list List-Id: EDK II Development List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , X-List-Received-Date: Tue, 20 Feb 2018 17:02:21 -0000 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline On Tue, Feb 20, 2018 at 08:20:48AM -0800, Andrew Fish wrote: > > On Feb 20, 2018, at 6:16 AM, Leif Lindholm wrote: > > > > On Tue, Feb 20, 2018 at 11:05:22AM +0000, Ard Biesheuvel wrote: > >> +/** > >> + Prints an assert message containing a filename, line number, and description. > >> + This may be followed by a breakpoint or a dead loop. > >> + > >> + Print a message of the form "ASSERT (): \n" > >> + to the debug output device. If DEBUG_PROPERTY_ASSERT_BREAKPOINT_ENABLED bit > >> + of PcdDebugProperyMask is set then CpuBreakpoint() is called. Otherwise, if > >> + DEBUG_PROPERTY_ASSERT_DEADLOOP_ENABLED bit of PcdDebugProperyMask is set then > >> + CpuDeadLoop() is called. If neither of these bits are set, then this function > >> + returns immediately after the message is printed to the debug output device. > >> + DebugAssert() must actively prevent recursion. If DebugAssert() is called > >> + while processing another DebugAssert(), then DebugAssert() must return > >> + immediately. > >> + > >> + If FileName is NULL, then a string of "(NULL) Filename" is printed. > >> + If Description is NULL, then a string of "(NULL) Description" is > >> + printed. > >> + > >> + @param FileName The pointer to the name of the source file that generated > >> + the assert condition. > >> + @param LineNumber The line number in the source file that generated the > >> + assert condition > >> + @param Description The pointer to the description of the assert condition. > >> + > >> +**/ > >> +VOID > >> +EFIAPI > >> +DebugAssert ( > >> + IN CONST CHAR8 *FileName, > >> + IN UINTN LineNumber, > >> + IN CONST CHAR8 *Description > >> + ) > >> +{ > >> + CHAR8 Buffer[MAX_DEBUG_MESSAGE_LENGTH]; > >> + > >> + if (!mEfiAtRuntime) { > >> + // > >> + // Generate the ASSERT() message in Ascii format > >> + // > >> + AsciiSPrint (Buffer, sizeof (Buffer), "ASSERT [%a] %a(%d): %a\n", > >> + gEfiCallerBaseName, FileName, LineNumber, Description); > >> + > >> + // > >> + // Send the print string to the Console Output device > >> + // > >> + SerialPortWrite ((UINT8 *)Buffer, AsciiStrLen (Buffer)); > >> + } > >> + > >> + // > >> + // Generate a Breakpoint, DeadLoop, or NOP based on PCD settings > >> + // > >> + if ((FixedPcdGet8 (PcdDebugPropertyMask) & > >> + DEBUG_PROPERTY_ASSERT_BREAKPOINT_ENABLED) != 0) { > >> + CpuBreakpoint (); > >> + } else if ((FixedPcdGet8 (PcdDebugPropertyMask) & > >> + DEBUG_PROPERTY_ASSERT_DEADLOOP_ENABLED) != 0) { > >> + CpuDeadLoop (); > >> + } > > > > Hmm ... I know this does not fundamentally change the behaviour of the > > existing implementation, but if we're looking to improve runtime > > behaviour, we've just gone from generating a runtime fault to silently > > freezing (if BREAKPOINT_ENABLED or DEADLOOP_ENABLED). > > > > What do breakpoint/deadloop mean in a runtime context anyway - do we > > not need to halt _all_ running cores? > > > > I don't see an obvious "right way" solution here, and this only > > affects DEBUG builds. > > Leif, > > It is not related to DEBUG builds, it is related to PCD configuration. Sorry, I was oversimplifying based on most RELEASE builds tending to have DebugAssertEnabled disabled through PcdDebugPropertyMask. Looking through edk2 platforms, that shows to be incorrect even among most open platform ports, so thanks for pointing this out. > > Possible ways of handling this that I can think > > of include: > > - Don't respect BREAKPOINT/DEADLOOP if at runtime. > > - Respect BREAKPOINT/DEADLOOP and disable all cores. > > - Take ownership back of the system and re-enable 1:1 mapping so > > messages can be printed. > > There is not much risk of losing user data if you "panic" EFI, that > is not true if you are going to "panic" the OS. I've seen more bugs > at runtime from confusion about what is legal to do at runtime, > vs. actual bugs. You can always just return device error on failure, > and if the RT Services hangs you can core dump the OS. Given the OS > provided the virtual mapping it is likely the stuck EFI could would > be in the stack trace of the panic. Oh, indeed. My concern is regarding the fact that this library (with either of those options set) would halt the executing processor (and no others) without any output whatsoever. / Leif