From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received-SPF: Pass (sender SPF authorized) identity=mailfrom; client-ip=209.132.183.28; helo=mx1.redhat.com; envelope-from=lersek@redhat.com; receiver=edk2-devel@lists.01.org Received: from mx1.redhat.com (mx1.redhat.com [209.132.183.28]) (using TLSv1.2 with cipher ECDHE-RSA-AES256-GCM-SHA384 (256/256 bits)) (No client certificate requested) by ml01.01.org (Postfix) with ESMTPS id 70CB2211CFFE7 for ; Thu, 28 Feb 2019 05:39:05 -0800 (PST) Received: from smtp.corp.redhat.com (int-mx04.intmail.prod.int.phx2.redhat.com [10.5.11.14]) (using TLSv1.2 with cipher AECDH-AES256-SHA (256/256 bits)) (No client certificate requested) by mx1.redhat.com (Postfix) with ESMTPS id A54C1C0CDE28; Thu, 28 Feb 2019 13:39:04 +0000 (UTC) Received: from lacos-laptop-7.usersys.redhat.com (ovpn-120-128.rdu2.redhat.com [10.10.120.128]) by smtp.corp.redhat.com (Postfix) with ESMTP id E813F5D9C9; Thu, 28 Feb 2019 13:39:02 +0000 (UTC) To: Heyi Guo , edk2-devel@lists.01.org Cc: Hao Wu , Julien Grall , wanghaibin.wang@huawei.com, Ard Biesheuvel References: <1551341112-21645-1-git-send-email-guoheyi@huawei.com> From: Laszlo Ersek Message-ID: <668ad400-578a-672a-1df8-0ca72eb2a083@redhat.com> Date: Thu, 28 Feb 2019 14:39:01 +0100 User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:52.0) Gecko/20100101 Thunderbird/52.9.1 MIME-Version: 1.0 In-Reply-To: <1551341112-21645-1-git-send-email-guoheyi@huawei.com> X-Scanned-By: MIMEDefang 2.79 on 10.5.11.14 X-Greylist: Sender IP whitelisted, not delayed by milter-greylist-4.5.16 (mx1.redhat.com [10.5.110.31]); Thu, 28 Feb 2019 13:39:04 +0000 (UTC) Subject: Re: [RFC 0/3] Enable runtime serial debug for ArmVirtQemu X-BeenThere: edk2-devel@lists.01.org X-Mailman-Version: 2.1.29 Precedence: list List-Id: EDK II Development List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , X-List-Received-Date: Thu, 28 Feb 2019 13:39:06 -0000 Content-Type: text/plain; charset=utf-8 Content-Language: en-US Content-Transfer-Encoding: 7bit Hello Heyi, On 02/28/19 09:05, Heyi Guo wrote: > Serial port output is useful when debugging UEFI runtime services in OS runtime. > The patches are trying to provide a handy method to enable runtime serial port > debug for ArmVirtQemu. > > Cc: Jian J Wang > Cc: Hao Wu > Cc: Laszlo Ersek > Cc: Ard Biesheuvel > Cc: Julien Grall > > Heyi Guo (3): > MdeModulePkg/StatusCode: Add PCD to enable runtime serial debug > ArmVirtPkg: add runtime instance of FdtPL011SerialPortLib > ArmVirtQemu: enable runtime debug by build flag > > ArmVirtPkg/ArmVirt.dsc.inc | 6 ++ > ArmVirtPkg/ArmVirtQemu.dsc | 13 +++ > ArmVirtPkg/ArmVirtQemuFvMain.fdf.inc | 5 + > ArmVirtPkg/Library/FdtPL011SerialPortLib/FdtPL011SerialPortLib.c | 29 ++++-- > ArmVirtPkg/Library/FdtPL011SerialPortLib/FdtPL011SerialPortLib.h | 27 +++++ > ArmVirtPkg/Library/FdtPL011SerialPortLib/FdtPL011SerialPortLib.inf | 3 + > ArmVirtPkg/Library/FdtPL011SerialPortLib/FdtPL011SerialPortLibCommon.c | 27 +++++ > ArmVirtPkg/Library/FdtPL011SerialPortLib/FdtPL011SerialPortLibRuntime.c | 104 ++++++++++++++++++++ > ArmVirtPkg/Library/FdtPL011SerialPortLib/FdtPL011SerialPortLibRuntime.inf | 58 +++++++++++ > MdeModulePkg/MdeModulePkg.dec | 6 ++ > MdeModulePkg/MdeModulePkg.uni | 6 ++ > MdeModulePkg/Universal/StatusCodeHandler/RuntimeDxe/StatusCodeHandlerRuntimeDxe.c | 2 +- > MdeModulePkg/Universal/StatusCodeHandler/RuntimeDxe/StatusCodeHandlerRuntimeDxe.inf | 7 +- > 13 files changed, 279 insertions(+), 14 deletions(-) > create mode 100644 ArmVirtPkg/Library/FdtPL011SerialPortLib/FdtPL011SerialPortLib.h > create mode 100644 ArmVirtPkg/Library/FdtPL011SerialPortLib/FdtPL011SerialPortLibCommon.c > create mode 100644 ArmVirtPkg/Library/FdtPL011SerialPortLib/FdtPL011SerialPortLibRuntime.c > create mode 100644 ArmVirtPkg/Library/FdtPL011SerialPortLib/FdtPL011SerialPortLibRuntime.inf > I'm quite swamped, so only a few general comments for now. I'll let the MdeModulePkg maintainers comment on the first patch. (1) The general idea to utilize - MdeModulePkg/Library/PeiDxeDebugLibReportStatusCode - MdePkg/Library/DxeRuntimeDebugLibSerialPort seems mostly OK to me. Although, I think I would actually prefer the reverse implementation (= don't send debug messages to the status code infrastructure; instead, process status codes by logging them with DEBUG). I don't think there is a status code handler already present in edk2 for that however. (2) The use case is too generic. Until very recently, we hadn't enabled status code routing and reporting in ArmVirtPkg at all. We only did that recently, in commit 5c574b222ec2 ("ArmVirtPkg/ArmVirtQemu*: enable minimal Status Code Routing in DXE", 2019-02-25), because there was no other way to implement the boot progress reporting that I had been after. (See commit 1797f32e0a19 ("ArmVirtPkg/PlatformBootManagerLib: display boot option loading/starting", 2019-02-25).) So, in the proposal, the specific use should be described. What runtime module do you want status codes from? And why do you prefer status codes to actual (runtime) debug messages? ... Are you perhaps using this approach only because it lets you print DEBUGs at runtime? I.e., do you use status codes only as a "runtime carrier" for DEBUGs? (3) Patch #2 ("ArmVirtPkg: add runtime instance of FdtPL011SerialPortLib") looks confusing. It introduces a file called "FdtPL011SerialPortLibCommon.c", which doesn't seem common to multiple INF files. It is added only to one file. Also, the patch changes the behavior of the current lib instance with regard to the SerialPortInitialize() function and the library constructor. I don't see why that is necessary just for adding a runtime instance. The runtime instance should not affect the behavior of the existent instance. Sorry if I misunderstood; a more detailed commit message would be nice. (4) What's most worrying is that this change would lead to an unexpected sharing of the PL011 device between the OS and the firmware. I don't think that's a great idea, especially if QEMU's ACPI payload explicitly advertises the port to the OS. (On x86/OVMF, the above problems don't surface. There, DEBUG messages are written to the QEMU debug IO port. The debug IO port is neither used by the OS kernel (so no race), nor affected by page tables (the IO port space is absolute). Hence no runtime specific processing.) Thanks Laszlo