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.35; helo=huawei.com; envelope-from=guoheyi@huawei.com; receiver=edk2-devel@lists.01.org Received: from huawei.com (szxga07-in.huawei.com [45.249.212.35]) (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 288902194EB7C for ; Mon, 1 Apr 2019 18:45:27 -0700 (PDT) Received: from DGGEMS407-HUB.china.huawei.com (unknown [10.3.19.207]) by Forcepoint Email with ESMTP id C654024166B92E94386F; Tue, 2 Apr 2019 09:45:24 +0800 (CST) Received: from [127.0.0.1] (10.177.31.55) by DGGEMS407-HUB.china.huawei.com (10.3.19.207) with Microsoft SMTP Server id 14.3.408.0; Tue, 2 Apr 2019 09:45:15 +0800 To: Laszlo Ersek , References: <1554109590-16131-1-git-send-email-guoheyi@huawei.com> <1554109590-16131-2-git-send-email-guoheyi@huawei.com> <41295e12-8f53-7c81-0fd4-bf1f99aea3c8@redhat.com> CC: , Ard Biesheuvel , Julien Grall , Heyi Guo From: Heyi Guo Message-ID: <0b2b52c9-00ea-b7c6-9972-840ffb0f93c8@huawei.com> Date: Tue, 2 Apr 2019 09:45:14 +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: <41295e12-8f53-7c81-0fd4-bf1f99aea3c8@redhat.com> X-Originating-IP: [10.177.31.55] Subject: Re: [PATCH 1/2] ArmVirtPkg: add runtime instance of FdtPL011SerialPortLib 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: Tue, 02 Apr 2019 01:45:29 -0000 Content-Type: text/plain; charset="utf-8"; format=flowed Content-Transfer-Encoding: 7bit Hi Laszlo, Thanks for your time of reviewing and sorry for code style issues. I accept most of comments, with some questions for below ones: (7) coding style -- please use empty // lines before and after the comment line that you have right now. I remember Ard said this rule conflicted some other rule; not sure if it is decided to use empty // lines around comments. > + > + gInternalRT = SystemTable->RuntimeServices; > + > + Status = InternalGetSystemConfigurationTable ( > + SystemTable, > + &gEfiDxeServicesTableGuid, > + (VOID **) &InternalDS > + ); (13) coding style -- indentation is broken It is true that I'm not clear about the indentation rule for multiple-line function call; shall I use 2 spaces indentation following the function name, just like below? + Status = InternalGetSystemConfigurationTable ( + SystemTable, + &gEfiDxeServicesTableGuid, + (VOID **) &InternalDS + ); Shall we follow the coding style in this document: https://github.com/tianocore-docs/Docs/raw/master/Specifications/CCS_2_1_Draft.pdf? Thanks, Heyi On 2019/4/2 0:14, Laszlo Ersek wrote: > On 04/01/19 11:06, Heyi Guo wrote: >> Add a runtime instance of FdtPL011SerialPortLib to support runtime >> serial port debug for UEFI runtime services. >> >> The framework is based on below discussion: >> https://lists.01.org/pipermail/edk2-devel/2019-March/037986.html >> >> We have decided to use an individual firmware UART for UEFI runtime >> debug, however this depends on QEMU to provide this virtual device, so >> we still use the OS visible system UART at the moment, with the >> potential *risk* of conflicting OS serial port read/write. > (1) I'll defer to Ard on whether this is a good idea after all, even > just for debugging. I realize the feature is disabled by default, so > personally I wouldn't mind, I think. > >> Once QEMU implements individual firmware UART, we need rewrite >> PlatformGetRtSerialBase() to get the real runtime serial port base >> address. > (2) Assuming Ard is OK with this, please file a feature request in the > upstream QEMU bug tracker , about > the new UART. > > In turn, this launchpad bug URL should be mentioned (a) in the FIXME > comment in patch #1 (below), (b) more importantly, near the default > RT_DEBUG setting (of FALSE) in the DSC file, in patch #2. > > People looking at the build flags should realize that RT_DEBUG=TRUE is > basically broken, and find a pointer to the missing QEMU feature. > >> Cc: Laszlo Ersek >> Cc: Ard Biesheuvel >> Cc: Julien Grall >> Contributed-under: TianoCore Contribution Agreement 1.1 >> Signed-off-by: Heyi Guo >> Signed-off-by: Heyi Guo > (3) This pair of signoffs looks strange, but maybe it is justified. Was > it your intent? (Applies to patch #2 as well.) > >> --- >> ArmVirtPkg/Library/FdtPL011SerialPortLib/FdtPL011SerialPortLib.c | 6 +- >> ArmVirtPkg/Library/FdtPL011SerialPortLib/FdtPL011SerialPortLib.h | 32 ++++ >> ArmVirtPkg/Library/FdtPL011SerialPortLib/FdtPL011SerialPortLibRuntime.c | 187 ++++++++++++++++++++ >> ArmVirtPkg/Library/FdtPL011SerialPortLib/FdtPL011SerialPortLibRuntime.inf | 59 ++++++ >> 4 files changed, 282 insertions(+), 2 deletions(-) >> >> diff --git a/ArmVirtPkg/Library/FdtPL011SerialPortLib/FdtPL011SerialPortLib.c b/ArmVirtPkg/Library/FdtPL011SerialPortLib/FdtPL011SerialPortLib.c >> index 2f10fb7..017ca30 100644 >> --- a/ArmVirtPkg/Library/FdtPL011SerialPortLib/FdtPL011SerialPortLib.c >> +++ b/ArmVirtPkg/Library/FdtPL011SerialPortLib/FdtPL011SerialPortLib.c >> @@ -3,9 +3,10 @@ >> >> Copyright (c) 2008 - 2010, Apple Inc. All rights reserved.
>> Copyright (c) 2012 - 2013, ARM Ltd. All rights reserved.
>> - Copyright (c) 2014, Linaro Ltd. All rights reserved.
>> + Copyright (c) 2014 - 2019, Linaro Ltd. All rights reserved.
>> Copyright (c) 2014, Red Hat, Inc.
>> Copyright (c) 2015, Intel Corporation. All rights reserved.
>> + Copyright (c) 2019, Huawei Technologies Co., Ltd. All rights reserved.
>> >> This program and the accompanying materials >> are licensed and made available under the terms and conditions of the BSD License >> @@ -28,8 +29,9 @@ >> #include >> #include >> #include >> +#include "FdtPL011SerialPortLib.h" >> >> -STATIC UINTN mSerialBaseAddress; >> +UINTN mSerialBaseAddress; >> >> RETURN_STATUS >> EFIAPI >> diff --git a/ArmVirtPkg/Library/FdtPL011SerialPortLib/FdtPL011SerialPortLib.h b/ArmVirtPkg/Library/FdtPL011SerialPortLib/FdtPL011SerialPortLib.h >> new file mode 100644 >> index 0000000..32c0b9b >> --- /dev/null >> +++ b/ArmVirtPkg/Library/FdtPL011SerialPortLib/FdtPL011SerialPortLib.h >> @@ -0,0 +1,32 @@ >> +/** @file >> + Serial I/O Port library internal header >> + >> + Copyright (c) 2008 - 2010, Apple Inc. All rights reserved.
>> + Copyright (c) 2012 - 2013, ARM Ltd. All rights reserved.
>> + Copyright (c) 2014 - 2019, Linaro Ltd. All rights reserved.
>> + Copyright (c) 2014, Red Hat, Inc.
>> + Copyright (c) 2015, Intel Corporation. All rights reserved.
>> + Copyright (c) 2019, Huawei Technologies Co., Ltd. All rights reserved.
>> + >> + This program and the accompanying materials >> + are licensed and made available under the terms and conditions of the BSD License >> + which accompanies this distribution. The full text of the license may be found at >> + http://opensource.org/licenses/bsd-license.php >> + >> + THE PROGRAM IS DISTRIBUTED UNDER THE BSD LICENSE ON AN "AS IS" BASIS, >> + WITHOUT WARRANTIES OR REPRESENTATIONS OF ANY KIND, EITHER EXPRESS OR IMPLIED. >> + >> +**/ > (4) From April 2nd through April 9th, the edk2 tree will be locked from > file additions / removals, due to a patch review period for > . > > I think you will have to repost after that period, and then the license > blocks in all the new files should follow the new format. > >> + >> +#ifndef _FDT_PL011_SERIAL_PORT_LIB_H_ >> +#define _FDT_PL011_SERIAL_PORT_LIB_H_ >> + >> +extern UINTN mSerialBaseAddress; >> + >> +RETURN_STATUS >> +EFIAPI >> +FdtPL011SerialPortLibInitialize ( >> + VOID >> + ); >> + >> +#endif > (5) We should list *.h files in INF files, as sources. I.e., please > update "FdtPL011SerialPortLib.inf" too. > > Also, please split these changes to a separate patch. The API export > patch is not big, bit splitting it off simplifies the diffstat for the > feature addition. > >> diff --git a/ArmVirtPkg/Library/FdtPL011SerialPortLib/FdtPL011SerialPortLibRuntime.c b/ArmVirtPkg/Library/FdtPL011SerialPortLib/FdtPL011SerialPortLibRuntime.c >> new file mode 100644 >> index 0000000..4a7b0b5 >> --- /dev/null >> +++ b/ArmVirtPkg/Library/FdtPL011SerialPortLib/FdtPL011SerialPortLibRuntime.c >> @@ -0,0 +1,187 @@ >> +/** @file >> + Initialization for runtime serial I/O port library >> + >> + Copyright (c) 2008 - 2010, Apple Inc. All rights reserved.
>> + Copyright (c) 2012 - 2013, ARM Ltd. All rights reserved.
>> + Copyright (c) 2014 - 2019, Linaro Ltd. All rights reserved.
>> + Copyright (c) 2014, Red Hat, Inc.
>> + Copyright (c) 2015, Intel Corporation. All rights reserved.
>> + Copyright (c) 2019, Huawei Technologies Co., Ltd. All rights reserved.
>> + >> + This program and the accompanying materials >> + are licensed and made available under the terms and conditions of the BSD License >> + which accompanies this distribution. The full text of the license may be found at >> + http://opensource.org/licenses/bsd-license.php >> + >> + THE PROGRAM IS DISTRIBUTED UNDER THE BSD LICENSE ON AN "AS IS" BASIS, >> + WITHOUT WARRANTIES OR REPRESENTATIONS OF ANY KIND, EITHER EXPRESS OR IMPLIED. >> + >> +**/ >> + >> +// >> +// UEFI runtime serial port debug framework: >> +// - Use BaseDebugLibSerialPort for DebugLib >> +// - Boot time debug message of runtime modules will be directed to the same >> +// serial port of other modules >> +// - Runtime debug message should be directed to an individual firmware serial >> +// port to avoid conflict with OS serial port access >> +// >> + >> +#include >> +#include >> +#include >> +#include "FdtPL011SerialPortLib.h" >> + >> +STATIC UINTN mRtSerialBaseAddress; >> +STATIC EFI_EVENT mSerialVirtualAddressChangeEvent = NULL; >> +// We can't use gRT directly due to library dependency. >> +STATIC EFI_RUNTIME_SERVICES *gInternalRT = NULL; > (6) I think it would be justified to use an "m" prefix here, rather than > "g". > > (7) coding style -- please use empty // lines before and after the > comment line that you have right now. > >> + >> +VOID >> +EFIAPI >> +SerialVirtualAddressChangeCallBack ( >> + IN EFI_EVENT Event, >> + IN VOID *Context >> + ) >> +{ >> + gInternalRT->ConvertPointer (0, (VOID **) &mRtSerialBaseAddress); >> + mSerialBaseAddress = mRtSerialBaseAddress; >> +} >> + >> +// >> +// To avoid library constructor looped dependence, we copy >> +// EfiGetSystemConfigurationTable() here instead of using UefiLib. >> +// >> +STATIC >> +RETURN_STATUS >> +InternalGetSystemConfigurationTable ( >> + IN EFI_SYSTEM_TABLE *SystemTable, >> + IN EFI_GUID *TableGuid, >> + OUT VOID **Table > (8) indentation / alignment looks broken > >> + ) >> +{ >> + UINTN Index; >> + >> + *Table = NULL; >> + for (Index = 0; Index < SystemTable->NumberOfTableEntries; Index++) { >> + if (CompareGuid (TableGuid, &(SystemTable->ConfigurationTable[Index].VendorGuid))) { >> + *Table = SystemTable->ConfigurationTable[Index].VendorTable; >> + return RETURN_SUCCESS; >> + } >> + } >> + >> + return RETURN_NOT_FOUND; >> +} >> + >> +STATIC >> +RETURN_STATUS >> +PlatformGetRtSerialBase ( >> + IN EFI_HANDLE ImageHandle, >> + IN EFI_SYSTEM_TABLE *SystemTable, > (9) These parameters are currently unused, and this is a function that > we can declare whichever way we want. Can you drop the parameters? > >> + IN OUT UINTN *SerialBase >> + ) >> +{ >> + // >> + // FIXME: Using system serial port will probably cause conflict with OS serial >> + // port access, so this code can ONLY be used for debug purpose and may cause >> + // unexpected system behaviour! >> + // We need change to the individual firmware serial port as soon as QEMU >> + // implements that. >> + // >> + *SerialBase = mSerialBaseAddress; >> + return RETURN_SUCCESS; >> +} >> + >> +RETURN_STATUS >> +EFIAPI >> +SerialPortLibConstructorRuntime ( >> + IN EFI_HANDLE ImageHandle, >> + IN EFI_SYSTEM_TABLE *SystemTable >> + ) >> +{ >> + RETURN_STATUS Status; >> + UINT64 Length = SIZE_4KB; >> + EFI_GCD_MEMORY_SPACE_DESCRIPTOR Desc = {0}; >> + EFI_DXE_SERVICES *InternalDS = NULL; > (10) Coding style -- edk2 doesn't allow initialization of variables with > auto storage duration. > >> + >> + Status = FdtPL011SerialPortLibInitialize(); > (11) Coding style -- please insert a space character after the function > name. > >> + if (RETURN_ERROR (Status)) { >> + return Status; >> + } >> + >> + Status = PlatformGetRtSerialBase (ImageHandle, SystemTable, &mRtSerialBaseAddress); >> + if (RETURN_ERROR (Status)) { >> + return Status; >> + } > (12) Please add a comment here about the next steps -- namely that we're > going to set up the memory space map for the runtime serial port > register block. > >> + >> + gInternalRT = SystemTable->RuntimeServices; >> + >> + Status = InternalGetSystemConfigurationTable ( >> + SystemTable, >> + &gEfiDxeServicesTableGuid, >> + (VOID **) &InternalDS >> + ); > (13) coding style -- indentation is broken > >> + if (RETURN_ERROR (Status)) { >> + return Status; >> + } >> + >> + // >> + // For the serial port register space length is only 4KB, we don't need to >> + // check if the descriptor is large enough to cover the space. >> + // > (14) Please replace the "Length" variable with a macro then, or even > open-code SIZE_4KB below. > >> + Status = InternalDS->GetMemorySpaceDescriptor(mRtSerialBaseAddress, &Desc); > (15) coding style -- missing space > >> + if (RETURN_ERROR (Status) || >> + Desc.GcdMemoryType == EfiGcdMemoryTypeNonExistent) { >> + Status = InternalDS->AddMemorySpace ( >> + EfiGcdMemoryTypeMemoryMappedIo, >> + mRtSerialBaseAddress, >> + Length, >> + EFI_MEMORY_UC | EFI_MEMORY_RUNTIME >> + ); >> + if (RETURN_ERROR (Status)) { >> + return Status; >> + } >> + Desc.Attributes = EFI_MEMORY_UC; >> + } >> + >> + if ((Desc.Attributes & EFI_MEMORY_RUNTIME) == 0) { >> + Desc.Attributes |= EFI_MEMORY_RUNTIME; >> + Status = InternalDS->SetMemorySpaceAttributes ( >> + mRtSerialBaseAddress, >> + Length, >> + Desc.Attributes >> + ); >> + if(RETURN_ERROR (Status)){ >> + return Status; >> + } >> + } > (16) The indentation is wrong, but more importantly, > > (17) I don't like how this logic tries to be halfway-generic. Normally, > I would suggest making it stricter: > > - If GetMemorySpaceDescriptor() fails, just return an error (it should > never happen). > > - If the descriptor reports "nonexistent", then add the range, and set > the attributes as well. > > - If the descriptor reports "MMIO", check the attributes. If there is > any mismatch in the attributes, return an error. > > - If the descriptor reports some type other than nonexistent and MMIO, > return an error. > > - The idea is that we expect to "own" this range. > > However, given the current messy state, namely that the MMIO area is > *not* owned by us (because we're just piggy-backing the normal UART), we > get to fiddle with *just* the runtime attribute. > > This is super ugly. So I suggest extracting this logic too, to a > separate function, and making a comment (with LP# reference again) that > once the QEMU feature is implemented, the function will have to be > rewritten. Separating this mess to a dedicated helper will keep the > constructor function cleaner, structurally. > >> + >> + Status = SystemTable->BootServices->CreateEventEx ( >> + EVT_NOTIFY_SIGNAL, >> + TPL_NOTIFY, >> + SerialVirtualAddressChangeCallBack, >> + NULL, >> + &gEfiEventVirtualAddressChangeGuid, >> + &mSerialVirtualAddressChangeEvent >> + ); > (18) The indentation is wrong. > >> + if (RETURN_ERROR (Status)) { >> + mSerialVirtualAddressChangeEvent = NULL; >> + } >> + >> + return Status; >> +} >> + >> +RETURN_STATUS >> +EFIAPI >> +SerialPortLibDestructor ( >> + IN EFI_HANDLE ImageHandle, >> + IN EFI_SYSTEM_TABLE *SystemTable >> + ) >> +{ >> + >> + if (!mSerialVirtualAddressChangeEvent) { >> + return RETURN_SUCCESS; >> + } >> + >> + return SystemTable->BootServices->CloseEvent (mSerialVirtualAddressChangeEvent); >> +} > (19) This implementation has a subtle logic bug, which is -- of course > -- hidden by the fact that we don't have two UARTs presently. > > The logic bug is that the system can be *between* ExitBootServices() and > SetVirtualAddressMap(). In that state, the boot loader can perfectly > well call runtime services, such as variable services, and you would > want to see debug messages on the *runtime* UART (because, we have > exited the UEFI boot services). However, the virtual address map has not > been set yet, and so the code above will not flip the drivers to the > runtime UART. > > Therefore, the code in SerialVirtualAddressChangeCallBack() should be > split, to an EBS callback, and to a VAC callback. In the EBS callback, > we should only do "mSerialBaseAddress = mRtSerialBaseAddress". And in > the VAC callback, we should ignore "mRtSerialBaseAddress"; we should > convert "mSerialBaseAddress" in-place, instead. > >> diff --git a/ArmVirtPkg/Library/FdtPL011SerialPortLib/FdtPL011SerialPortLibRuntime.inf b/ArmVirtPkg/Library/FdtPL011SerialPortLib/FdtPL011SerialPortLibRuntime.inf >> new file mode 100644 >> index 0000000..9403030 >> --- /dev/null >> +++ b/ArmVirtPkg/Library/FdtPL011SerialPortLib/FdtPL011SerialPortLibRuntime.inf >> @@ -0,0 +1,59 @@ >> +#/** @file >> +# >> +# Component description file for FdtPL011SerialPortLibRuntime module > (20) This comment should be replaced with something non-generic. Please > explain the core idea behind the library instance in one or two sentences. > >> +# >> +# Copyright (c) 2011-2015, ARM Ltd. All rights reserved.
>> +# Copyright (c) 2019, Linaro Ltd. All rights reserved.
>> +# Copyright (c) 2019, Huawei Technologies Co., Ltd. All rights reserved.
>> +# >> +# This program and the accompanying materials >> +# are licensed and made available under the terms and conditions of the BSD License >> +# which accompanies this distribution. The full text of the license may be found at >> +# http://opensource.org/licenses/bsd-license.php >> +# >> +# THE PROGRAM IS DISTRIBUTED UNDER THE BSD LICENSE ON AN "AS IS" BASIS, >> +# WITHOUT WARRANTIES OR REPRESENTATIONS OF ANY KIND, EITHER EXPRESS OR IMPLIED. >> +# >> +#**/ >> + >> +[Defines] >> + INF_VERSION = 0x00010005 > (21) We should use "1.28" here -- > . > >> + BASE_NAME = FdtPL011SerialPortLibRuntime >> + FILE_GUID = 83afbb90-38c6-11e9-aeab-203db202c950 >> + MODULE_TYPE = DXE_DRIVER >> + VERSION_STRING = 1.0 >> + LIBRARY_CLASS = SerialPortLib|DXE_RUNTIME_DRIVER >> + CONSTRUCTOR = SerialPortLibConstructorRuntime >> + DESTRUCTOR = SerialPortLibDestructor >> + >> +[Sources.common] >> + FdtPL011SerialPortLib.c >> + FdtPL011SerialPortLibRuntime.c > (22) Please list the header file too. > >> + >> +[LibraryClasses] >> + BaseMemoryLib >> + HobLib >> + PL011UartLib >> + >> +[Packages] >> + EmbeddedPkg/EmbeddedPkg.dec >> + MdePkg/MdePkg.dec >> + MdeModulePkg/MdeModulePkg.dec >> + ArmPlatformPkg/ArmPlatformPkg.dec >> + ArmVirtPkg/ArmVirtPkg.dec >> + ArmPkg/ArmPkg.dec >> + >> +[FixedPcd] >> + gEfiMdePkgTokenSpaceGuid.PcdUartDefaultBaudRate >> + gEfiMdePkgTokenSpaceGuid.PcdUartDefaultDataBits >> + gEfiMdePkgTokenSpaceGuid.PcdUartDefaultParity >> + gEfiMdePkgTokenSpaceGuid.PcdUartDefaultStopBits >> + gArmPlatformTokenSpaceGuid.PL011UartClkInHz >> + >> +[Guids] >> + gEarlyPL011BaseAddressGuid >> + gEfiDxeServicesTableGuid >> + gEfiEventVirtualAddressChangeGuid >> + >> +[Depex] >> + gEfiCpuArchProtocolGuid >> > This review ate half of my day, and now I'm seriously doubting the > series gives us enough bang for the buck, with the underlying QEMU > feature missing -- which will require a significant rework of this code > anyway. > > I don't mind if you post a v2 before upstreaming the QEMU feature, but > please try to make *really sure* that you get the edk2 coding style bits > right. Wrong indentation and such is very distracting when I should > think about ordering of events and so on. > > Thanks > Laszlo > > . >