From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received-SPF: Pass (sender SPF authorized) identity=mailfrom; client-ip=45.249.212.190; helo=huawei.com; envelope-from=guoheyi@huawei.com; receiver=edk2-devel@lists.01.org Received: from huawei.com (szxga04-in.huawei.com [45.249.212.190]) (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 8C94F208AE36C for ; Fri, 1 Mar 2019 04:24:28 -0800 (PST) Received: from DGGEMS404-HUB.china.huawei.com (unknown [172.30.72.58]) by Forcepoint Email with ESMTP id 6AC6AE73F21EE7209962; Fri, 1 Mar 2019 20:24:25 +0800 (CST) Received: from [127.0.0.1] (10.177.31.55) by DGGEMS404-HUB.china.huawei.com (10.3.19.204) with Microsoft SMTP Server id 14.3.408.0; Fri, 1 Mar 2019 20:24:17 +0800 To: Laszlo Ersek , References: <1551341112-21645-1-git-send-email-guoheyi@huawei.com> <668ad400-578a-672a-1df8-0ca72eb2a083@redhat.com> CC: Hao Wu , Julien Grall , , Ard Biesheuvel From: Heyi Guo Message-ID: Date: Fri, 1 Mar 2019 20:24:15 +0800 User-Agent: Mozilla/5.0 (Windows NT 6.1; WOW64; rv:45.0) Gecko/20100101 Thunderbird/45.7.1 MIME-Version: 1.0 In-Reply-To: <668ad400-578a-672a-1df8-0ca72eb2a083@redhat.com> X-Originating-IP: [10.177.31.55] X-CFilter-Loop: Reflected 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: Fri, 01 Mar 2019 12:24:28 -0000 Content-Type: text/plain; charset="utf-8"; format=flowed Content-Transfer-Encoding: 7bit On 2019/2/28 21:39, Laszlo Ersek wrote: > 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? That's true:) StatusCode is only a carrier. We can see lots of X86 platforms use StatusCode driver for DEBUG serial print. > > > (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. Yes, the file name is not good. It only contains a null hook to build a non-runtime instance. I didn't get a proper name for that. Maybe RuntimeDummy or NoRuntime? > > 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. Ah, this is a mistake. In current implementation we can still enable the constructor. The long story is: at first I proposed to still use BaseSerialPortDebugLib, so I added RuntimePrepare() function into the constructor directly, but the builder complained about looped constructors, for RuntimePrepare() invokes gBS and some RunTime libraries. Then I tried disabling the constructor and moved RuntimePrepare() into SerialPortInitialize() which could complete the build, but still couldn't guarantee the calling order of these constructors, for BaseSerialPortDebugLib has a constructor to call SerialPortInitialize(). Finally I switched to the current implementation, but forgot to revert previous changes... > > > (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. That's true, so I propose to disable the feature by default. It is only used for UEFI runtime code debug. It is always painful when we don't a handy debug tool for runtime services code. > > (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.) We have not real port IO on ARM, so I think address conversion is always needed. Also I think this can be a feasible template for physical ARM platforms, for it does not require additional hardware components. Thanks, Heyi > > Thanks > Laszlo > > . >