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 82874211E011B for ; Mon, 1 Apr 2019 09:14:20 -0700 (PDT) Received: from smtp.corp.redhat.com (int-mx06.intmail.prod.int.phx2.redhat.com [10.5.11.16]) (using TLSv1.2 with cipher AECDH-AES256-SHA (256/256 bits)) (No client certificate requested) by mx1.redhat.com (Postfix) with ESMTPS id 5784F88AA6; Mon, 1 Apr 2019 16:14:19 +0000 (UTC) Received: from lacos-laptop-7.usersys.redhat.com (ovpn-120-149.rdu2.redhat.com [10.10.120.149]) by smtp.corp.redhat.com (Postfix) with ESMTP id 698125C206; Mon, 1 Apr 2019 16:14:17 +0000 (UTC) To: Heyi Guo , edk2-devel@lists.01.org Cc: wanghaibin.wang@huawei.com, Ard Biesheuvel , Julien Grall , Heyi Guo References: <1554109590-16131-1-git-send-email-guoheyi@huawei.com> <1554109590-16131-2-git-send-email-guoheyi@huawei.com> From: Laszlo Ersek Message-ID: <41295e12-8f53-7c81-0fd4-bf1f99aea3c8@redhat.com> Date: Mon, 1 Apr 2019 18:14:15 +0200 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: <1554109590-16131-2-git-send-email-guoheyi@huawei.com> X-Scanned-By: MIMEDefang 2.79 on 10.5.11.16 X-Greylist: Sender IP whitelisted, not delayed by milter-greylist-4.5.16 (mx1.redhat.com [10.5.110.26]); Mon, 01 Apr 2019 16:14:19 +0000 (UTC) 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: Mon, 01 Apr 2019 16:14:20 -0000 Content-Type: text/plain; charset=utf-8 Content-Language: en-US Content-Transfer-Encoding: 7bit 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