* [PATCH 0/2] Enable runtime serial port debug for ArmVirtQemu @ 2019-04-01 9:06 Heyi Guo 2019-04-01 9:06 ` [PATCH 1/2] ArmVirtPkg: add runtime instance of FdtPL011SerialPortLib Heyi Guo 2019-04-01 9:06 ` [PATCH 2/2] ArmVirtQemu: enable runtime debug by build flag Heyi Guo 0 siblings, 2 replies; 7+ messages in thread From: Heyi Guo @ 2019-04-01 9:06 UTC (permalink / raw) To: edk2-devel Cc: wanghaibin.wang, Heyi Guo, Laszlo Ersek, Ard Biesheuvel, Julien Grall These patches are 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. Once QEMU implements individual firmware UART, we need rewrite PlatformGetRtSerialBase() to get the real runtime serial port base address. Cc: Laszlo Ersek <lersek@redhat.com> Cc: Ard Biesheuvel <ard.biesheuvel@linaro.org> Cc: Julien Grall <julien.grall@arm.com> Heyi Guo (2): ArmVirtPkg: add runtime instance of FdtPL011SerialPortLib ArmVirtQemu: enable runtime debug by build flag ArmVirtPkg/ArmVirt.dsc.inc | 4 + ArmVirtPkg/ArmVirtQemu.dsc | 1 + ArmVirtPkg/Library/FdtPL011SerialPortLib/FdtPL011SerialPortLib.c | 6 +- ArmVirtPkg/Library/FdtPL011SerialPortLib/FdtPL011SerialPortLib.h | 32 ++++ ArmVirtPkg/Library/FdtPL011SerialPortLib/FdtPL011SerialPortLibRuntime.c | 187 ++++++++++++++++++++ ArmVirtPkg/Library/FdtPL011SerialPortLib/FdtPL011SerialPortLibRuntime.inf | 59 ++++++ 6 files changed, 287 insertions(+), 2 deletions(-) create mode 100644 ArmVirtPkg/Library/FdtPL011SerialPortLib/FdtPL011SerialPortLib.h create mode 100644 ArmVirtPkg/Library/FdtPL011SerialPortLib/FdtPL011SerialPortLibRuntime.c create mode 100644 ArmVirtPkg/Library/FdtPL011SerialPortLib/FdtPL011SerialPortLibRuntime.inf -- 1.8.3.1 ^ permalink raw reply [flat|nested] 7+ messages in thread
* [PATCH 1/2] ArmVirtPkg: add runtime instance of FdtPL011SerialPortLib 2019-04-01 9:06 [PATCH 0/2] Enable runtime serial port debug for ArmVirtQemu Heyi Guo @ 2019-04-01 9:06 ` Heyi Guo 2019-04-01 16:14 ` Laszlo Ersek 2019-04-01 9:06 ` [PATCH 2/2] ArmVirtQemu: enable runtime debug by build flag Heyi Guo 1 sibling, 1 reply; 7+ messages in thread From: Heyi Guo @ 2019-04-01 9:06 UTC (permalink / raw) To: edk2-devel Cc: wanghaibin.wang, Heyi Guo, Laszlo Ersek, Ard Biesheuvel, Julien Grall, Heyi Guo 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. Once QEMU implements individual firmware UART, we need rewrite PlatformGetRtSerialBase() to get the real runtime serial port base address. Cc: Laszlo Ersek <lersek@redhat.com> Cc: Ard Biesheuvel <ard.biesheuvel@linaro.org> Cc: Julien Grall <julien.grall@arm.com> Contributed-under: TianoCore Contribution Agreement 1.1 Signed-off-by: Heyi Guo <guoheyi@huawei.com> Signed-off-by: Heyi Guo <heyi.guo@linaro.org> --- 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.<BR> Copyright (c) 2012 - 2013, ARM Ltd. All rights reserved.<BR> - Copyright (c) 2014, Linaro Ltd. All rights reserved.<BR> + Copyright (c) 2014 - 2019, Linaro Ltd. All rights reserved.<BR> Copyright (c) 2014, Red Hat, Inc.<BR> Copyright (c) 2015, Intel Corporation. All rights reserved.<BR> + Copyright (c) 2019, Huawei Technologies Co., Ltd. All rights reserved.<BR> 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 <Pi/PiHob.h> #include <Library/HobLib.h> #include <Guid/EarlyPL011BaseAddress.h> +#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.<BR> + Copyright (c) 2012 - 2013, ARM Ltd. All rights reserved.<BR> + Copyright (c) 2014 - 2019, Linaro Ltd. All rights reserved.<BR> + Copyright (c) 2014, Red Hat, Inc.<BR> + Copyright (c) 2015, Intel Corporation. All rights reserved.<BR> + Copyright (c) 2019, Huawei Technologies Co., Ltd. All rights reserved.<BR> + + 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. + +**/ + +#ifndef _FDT_PL011_SERIAL_PORT_LIB_H_ +#define _FDT_PL011_SERIAL_PORT_LIB_H_ + +extern UINTN mSerialBaseAddress; + +RETURN_STATUS +EFIAPI +FdtPL011SerialPortLibInitialize ( + VOID + ); + +#endif 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.<BR> + Copyright (c) 2012 - 2013, ARM Ltd. All rights reserved.<BR> + Copyright (c) 2014 - 2019, Linaro Ltd. All rights reserved.<BR> + Copyright (c) 2014, Red Hat, Inc.<BR> + Copyright (c) 2015, Intel Corporation. All rights reserved.<BR> + Copyright (c) 2019, Huawei Technologies Co., Ltd. All rights reserved.<BR> + + 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 <Uefi.h> +#include <Guid/EventGroup.h> +#include <Library/BaseMemoryLib.h> +#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; + +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 + ) +{ + 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, + 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; + + Status = FdtPL011SerialPortLibInitialize(); + if (RETURN_ERROR (Status)) { + return Status; + } + + Status = PlatformGetRtSerialBase (ImageHandle, SystemTable, &mRtSerialBaseAddress); + if (RETURN_ERROR (Status)) { + return Status; + } + + gInternalRT = SystemTable->RuntimeServices; + + Status = InternalGetSystemConfigurationTable ( + SystemTable, + &gEfiDxeServicesTableGuid, + (VOID **) &InternalDS + ); + 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. + // + Status = InternalDS->GetMemorySpaceDescriptor(mRtSerialBaseAddress, &Desc); + 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; + } + } + + Status = SystemTable->BootServices->CreateEventEx ( + EVT_NOTIFY_SIGNAL, + TPL_NOTIFY, + SerialVirtualAddressChangeCallBack, + NULL, + &gEfiEventVirtualAddressChangeGuid, + &mSerialVirtualAddressChangeEvent + ); + 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); +} 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 +# +# Copyright (c) 2011-2015, ARM Ltd. All rights reserved.<BR> +# Copyright (c) 2019, Linaro Ltd. All rights reserved.<BR> +# Copyright (c) 2019, Huawei Technologies Co., Ltd. All rights reserved.<BR> +# +# 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 + 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 + +[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 -- 1.8.3.1 ^ permalink raw reply related [flat|nested] 7+ messages in thread
* Re: [PATCH 1/2] ArmVirtPkg: add runtime instance of FdtPL011SerialPortLib 2019-04-01 9:06 ` [PATCH 1/2] ArmVirtPkg: add runtime instance of FdtPL011SerialPortLib Heyi Guo @ 2019-04-01 16:14 ` Laszlo Ersek 2019-04-02 1:45 ` Heyi Guo 0 siblings, 1 reply; 7+ messages in thread From: Laszlo Ersek @ 2019-04-01 16:14 UTC (permalink / raw) To: Heyi Guo, edk2-devel Cc: wanghaibin.wang, Ard Biesheuvel, Julien Grall, Heyi Guo 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 <https://bugs.launchpad.net/qemu/+bugs>, 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 <lersek@redhat.com> > Cc: Ard Biesheuvel <ard.biesheuvel@linaro.org> > Cc: Julien Grall <julien.grall@arm.com> > Contributed-under: TianoCore Contribution Agreement 1.1 > Signed-off-by: Heyi Guo <guoheyi@huawei.com> > Signed-off-by: Heyi Guo <heyi.guo@linaro.org> (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.<BR> > Copyright (c) 2012 - 2013, ARM Ltd. All rights reserved.<BR> > - Copyright (c) 2014, Linaro Ltd. All rights reserved.<BR> > + Copyright (c) 2014 - 2019, Linaro Ltd. All rights reserved.<BR> > Copyright (c) 2014, Red Hat, Inc.<BR> > Copyright (c) 2015, Intel Corporation. All rights reserved.<BR> > + Copyright (c) 2019, Huawei Technologies Co., Ltd. All rights reserved.<BR> > > 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 <Pi/PiHob.h> > #include <Library/HobLib.h> > #include <Guid/EarlyPL011BaseAddress.h> > +#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.<BR> > + Copyright (c) 2012 - 2013, ARM Ltd. All rights reserved.<BR> > + Copyright (c) 2014 - 2019, Linaro Ltd. All rights reserved.<BR> > + Copyright (c) 2014, Red Hat, Inc.<BR> > + Copyright (c) 2015, Intel Corporation. All rights reserved.<BR> > + Copyright (c) 2019, Huawei Technologies Co., Ltd. All rights reserved.<BR> > + > + 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 <https://bugzilla.tianocore.org/show_bug.cgi?id=1373>. 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.<BR> > + Copyright (c) 2012 - 2013, ARM Ltd. All rights reserved.<BR> > + Copyright (c) 2014 - 2019, Linaro Ltd. All rights reserved.<BR> > + Copyright (c) 2014, Red Hat, Inc.<BR> > + Copyright (c) 2015, Intel Corporation. All rights reserved.<BR> > + Copyright (c) 2019, Huawei Technologies Co., Ltd. All rights reserved.<BR> > + > + 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 <Uefi.h> > +#include <Guid/EventGroup.h> > +#include <Library/BaseMemoryLib.h> > +#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.<BR> > +# Copyright (c) 2019, Linaro Ltd. All rights reserved.<BR> > +# Copyright (c) 2019, Huawei Technologies Co., Ltd. All rights reserved.<BR> > +# > +# 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 -- <https://edk2-docs.gitbooks.io/edk-ii-inf-specification/content/#edk-ii-module-information-inf-file-specification>. > + 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 ^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [PATCH 1/2] ArmVirtPkg: add runtime instance of FdtPL011SerialPortLib 2019-04-01 16:14 ` Laszlo Ersek @ 2019-04-02 1:45 ` Heyi Guo 2019-04-02 7:38 ` Laszlo Ersek 0 siblings, 1 reply; 7+ messages in thread From: Heyi Guo @ 2019-04-02 1:45 UTC (permalink / raw) To: Laszlo Ersek, edk2-devel Cc: wanghaibin.wang, Ard Biesheuvel, Julien Grall, Heyi Guo 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 <https://bugs.launchpad.net/qemu/+bugs>, 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 <lersek@redhat.com> >> Cc: Ard Biesheuvel <ard.biesheuvel@linaro.org> >> Cc: Julien Grall <julien.grall@arm.com> >> Contributed-under: TianoCore Contribution Agreement 1.1 >> Signed-off-by: Heyi Guo <guoheyi@huawei.com> >> Signed-off-by: Heyi Guo <heyi.guo@linaro.org> > (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.<BR> >> Copyright (c) 2012 - 2013, ARM Ltd. All rights reserved.<BR> >> - Copyright (c) 2014, Linaro Ltd. All rights reserved.<BR> >> + Copyright (c) 2014 - 2019, Linaro Ltd. All rights reserved.<BR> >> Copyright (c) 2014, Red Hat, Inc.<BR> >> Copyright (c) 2015, Intel Corporation. All rights reserved.<BR> >> + Copyright (c) 2019, Huawei Technologies Co., Ltd. All rights reserved.<BR> >> >> 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 <Pi/PiHob.h> >> #include <Library/HobLib.h> >> #include <Guid/EarlyPL011BaseAddress.h> >> +#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.<BR> >> + Copyright (c) 2012 - 2013, ARM Ltd. All rights reserved.<BR> >> + Copyright (c) 2014 - 2019, Linaro Ltd. All rights reserved.<BR> >> + Copyright (c) 2014, Red Hat, Inc.<BR> >> + Copyright (c) 2015, Intel Corporation. All rights reserved.<BR> >> + Copyright (c) 2019, Huawei Technologies Co., Ltd. All rights reserved.<BR> >> + >> + 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 > <https://bugzilla.tianocore.org/show_bug.cgi?id=1373>. > > 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.<BR> >> + Copyright (c) 2012 - 2013, ARM Ltd. All rights reserved.<BR> >> + Copyright (c) 2014 - 2019, Linaro Ltd. All rights reserved.<BR> >> + Copyright (c) 2014, Red Hat, Inc.<BR> >> + Copyright (c) 2015, Intel Corporation. All rights reserved.<BR> >> + Copyright (c) 2019, Huawei Technologies Co., Ltd. All rights reserved.<BR> >> + >> + 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 <Uefi.h> >> +#include <Guid/EventGroup.h> >> +#include <Library/BaseMemoryLib.h> >> +#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.<BR> >> +# Copyright (c) 2019, Linaro Ltd. All rights reserved.<BR> >> +# Copyright (c) 2019, Huawei Technologies Co., Ltd. All rights reserved.<BR> >> +# >> +# 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 -- > <https://edk2-docs.gitbooks.io/edk-ii-inf-specification/content/#edk-ii-module-information-inf-file-specification>. > >> + 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 > > . > ^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [PATCH 1/2] ArmVirtPkg: add runtime instance of FdtPL011SerialPortLib 2019-04-02 1:45 ` Heyi Guo @ 2019-04-02 7:38 ` Laszlo Ersek 0 siblings, 0 replies; 7+ messages in thread From: Laszlo Ersek @ 2019-04-02 7:38 UTC (permalink / raw) To: Heyi Guo, edk2-devel Cc: wanghaibin.wang, Ard Biesheuvel, Julien Grall, Heyi Guo On 04/02/19 03:45, Heyi Guo wrote: > 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. Interesting -- I remember seeing other "naked" // comments under Arm*Pkg, but I'm unaware of any rule conflicts regarding comment lines. Ard, can you clarify please? > >> + >> + 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 > + ); In ArmVirtPkg we accept two indentation styles for multi-line function calls. One is the official edk2 style, which you show above -- two spaces relative to the last word in the function designator, each argument on a new line (including the first argument), closing paren on a new line, closing paren indented similarly to the function call arguments. Status = PciIo->Pci.Read ( PciIo, EfiPciIoWidthUint32, 0, // Offset sizeof Pci / sizeof (UINT32), &Pci ); The other style could be called "condensed" -- it uses the same indentation, the only difference is that you need to insert a newline character only when you must, in order not to exceed 80 characters per line: Status = PciIo->Pci.Read (PciIo, EfiPciIoWidthUint32, 0 /* Offset */, sizeof Pci / sizeof (UINT32), &Pci); Obviously it's preferred not to break individual arguments in the middle, in this style. > Shall we follow the coding style in this document: > https://github.com/tianocore-docs/Docs/raw/master/Specifications/CCS_2_1_Draft.pdf? The latest version seems to be 2.2: https://edk2-docs.gitbooks.io/edk-ii-c-coding-standards-specification/content/ which includes the fix for <https://bugzilla.tianocore.org/show_bug.cgi?id=425>; but yes, that's the one. (Funnily enough, TianoCore#425 was exactly about cleaning up the multi-line function call rules in the official edk2 style. But, again, in ArmVirtPkg and OvmfPkg, we accept the condensed style in addition to the official edk2 style. You are free to pick whichever you like, and you can even mix them in a single file -- for example, use the condensed style as a basis, but switch to the more verbose official style if you have several comments on the arguments of a large function call.) Thanks, Laszlo > > 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 <https://bugs.launchpad.net/qemu/+bugs>, 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 <lersek@redhat.com> >>> Cc: Ard Biesheuvel <ard.biesheuvel@linaro.org> >>> Cc: Julien Grall <julien.grall@arm.com> >>> Contributed-under: TianoCore Contribution Agreement 1.1 >>> Signed-off-by: Heyi Guo <guoheyi@huawei.com> >>> Signed-off-by: Heyi Guo <heyi.guo@linaro.org> >> (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.<BR> >>> Copyright (c) 2012 - 2013, ARM Ltd. All rights reserved.<BR> >>> - Copyright (c) 2014, Linaro Ltd. All rights reserved.<BR> >>> + Copyright (c) 2014 - 2019, Linaro Ltd. All rights reserved.<BR> >>> Copyright (c) 2014, Red Hat, Inc.<BR> >>> Copyright (c) 2015, Intel Corporation. All rights reserved.<BR> >>> + Copyright (c) 2019, Huawei Technologies Co., Ltd. All rights >>> reserved.<BR> >>> 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 <Pi/PiHob.h> >>> #include <Library/HobLib.h> >>> #include <Guid/EarlyPL011BaseAddress.h> >>> +#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.<BR> >>> + Copyright (c) 2012 - 2013, ARM Ltd. All rights reserved.<BR> >>> + Copyright (c) 2014 - 2019, Linaro Ltd. All rights reserved.<BR> >>> + Copyright (c) 2014, Red Hat, Inc.<BR> >>> + Copyright (c) 2015, Intel Corporation. All rights reserved.<BR> >>> + Copyright (c) 2019, Huawei Technologies Co., Ltd. All rights >>> reserved.<BR> >>> + >>> + 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 >> <https://bugzilla.tianocore.org/show_bug.cgi?id=1373>. >> >> 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.<BR> >>> + Copyright (c) 2012 - 2013, ARM Ltd. All rights reserved.<BR> >>> + Copyright (c) 2014 - 2019, Linaro Ltd. All rights reserved.<BR> >>> + Copyright (c) 2014, Red Hat, Inc.<BR> >>> + Copyright (c) 2015, Intel Corporation. All rights reserved.<BR> >>> + Copyright (c) 2019, Huawei Technologies Co., Ltd. All rights >>> reserved.<BR> >>> + >>> + 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 <Uefi.h> >>> +#include <Guid/EventGroup.h> >>> +#include <Library/BaseMemoryLib.h> >>> +#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.<BR> >>> +# Copyright (c) 2019, Linaro Ltd. All rights reserved.<BR> >>> +# Copyright (c) 2019, Huawei Technologies Co., Ltd. All rights >>> reserved.<BR> >>> +# >>> +# 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 -- >> <https://edk2-docs.gitbooks.io/edk-ii-inf-specification/content/#edk-ii-module-information-inf-file-specification>. >> >> >>> + 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 >> >> . >> > > ^ permalink raw reply [flat|nested] 7+ messages in thread
* [PATCH 2/2] ArmVirtQemu: enable runtime debug by build flag 2019-04-01 9:06 [PATCH 0/2] Enable runtime serial port debug for ArmVirtQemu Heyi Guo 2019-04-01 9:06 ` [PATCH 1/2] ArmVirtPkg: add runtime instance of FdtPL011SerialPortLib Heyi Guo @ 2019-04-01 9:06 ` Heyi Guo 2019-04-01 16:14 ` Laszlo Ersek 1 sibling, 1 reply; 7+ messages in thread From: Heyi Guo @ 2019-04-01 9:06 UTC (permalink / raw) To: edk2-devel Cc: wanghaibin.wang, Heyi Guo, Laszlo Ersek, Ard Biesheuvel, Julien Grall, Heyi Guo Introduce a build flag "RT_DEBUG" to enable runtime debug, so that we can have a handy method for UEFI runtime services debug. This build flag only applies to DEBUG build; the reason for not enabling it for DEBUG build by default is that we are still using the same UART of OS and it may cause potential conflict. User needs to specify "-D RT_DEBUG=TRUE"; "-D RT_DEBUG" is not enough for we use !if $(RT_DEBUG)==TRUE in the code. Cc: Laszlo Ersek <lersek@redhat.com> Cc: Ard Biesheuvel <ard.biesheuvel@linaro.org> Cc: Julien Grall <julien.grall@arm.com> Contributed-under: TianoCore Contribution Agreement 1.1 Signed-off-by: Heyi Guo <guoheyi@huawei.com> Signed-off-by: Heyi Guo <heyi.guo@linaro.org> --- ArmVirtPkg/ArmVirt.dsc.inc | 4 ++++ ArmVirtPkg/ArmVirtQemu.dsc | 1 + 2 files changed, 5 insertions(+) diff --git a/ArmVirtPkg/ArmVirt.dsc.inc b/ArmVirtPkg/ArmVirt.dsc.inc index d172a08..5ace31b 100644 --- a/ArmVirtPkg/ArmVirt.dsc.inc +++ b/ArmVirtPkg/ArmVirt.dsc.inc @@ -240,8 +240,12 @@ MemoryAllocationLib|MdePkg/Library/UefiMemoryAllocationLib/UefiMemoryAllocationLib.inf CapsuleLib|MdeModulePkg/Library/DxeCapsuleLibNull/DxeCapsuleLibNull.inf !if $(TARGET) != RELEASE +!if $(RT_DEBUG) == TRUE + SerialPortLib|ArmVirtPkg/Library/FdtPL011SerialPortLib/FdtPL011SerialPortLibRuntime.inf +!else DebugLib|MdePkg/Library/DxeRuntimeDebugLibSerialPort/DxeRuntimeDebugLibSerialPort.inf !endif +!endif !if $(SECURE_BOOT_ENABLE) == TRUE BaseCryptLib|CryptoPkg/Library/BaseCryptLib/RuntimeCryptLib.inf diff --git a/ArmVirtPkg/ArmVirtQemu.dsc b/ArmVirtPkg/ArmVirtQemu.dsc index d703317..b279d7d 100644 --- a/ArmVirtPkg/ArmVirtQemu.dsc +++ b/ArmVirtPkg/ArmVirtQemu.dsc @@ -37,6 +37,7 @@ DEFINE SECURE_BOOT_ENABLE = FALSE DEFINE NETWORK_IP6_ENABLE = FALSE DEFINE HTTP_BOOT_ENABLE = FALSE + DEFINE RT_DEBUG = FALSE !include ArmVirtPkg/ArmVirt.dsc.inc -- 1.8.3.1 ^ permalink raw reply related [flat|nested] 7+ messages in thread
* Re: [PATCH 2/2] ArmVirtQemu: enable runtime debug by build flag 2019-04-01 9:06 ` [PATCH 2/2] ArmVirtQemu: enable runtime debug by build flag Heyi Guo @ 2019-04-01 16:14 ` Laszlo Ersek 0 siblings, 0 replies; 7+ messages in thread From: Laszlo Ersek @ 2019-04-01 16:14 UTC (permalink / raw) To: Heyi Guo, edk2-devel Cc: wanghaibin.wang, Ard Biesheuvel, Julien Grall, Heyi Guo On 04/01/19 11:06, Heyi Guo wrote: > Introduce a build flag "RT_DEBUG" to enable runtime debug, so that we > can have a handy method for UEFI runtime services debug. > > This build flag only applies to DEBUG build; the reason for not > enabling it for DEBUG build by default is that we are still using > the same UART of OS and it may cause potential conflict. > > User needs to specify "-D RT_DEBUG=TRUE"; "-D RT_DEBUG" is not enough > for we use !if $(RT_DEBUG)==TRUE in the code. (1) This looks like a bug / regression in basetools, or somewhere else. We use a bunch of other similar flags, and "-D FLAG" works just fine. See "HTTP_BOOT_ENABLE" for example -- set to FALSE by default in the individual DSC files, and depended upon in the dsc.inc file as well. > Cc: Laszlo Ersek <lersek@redhat.com> > Cc: Ard Biesheuvel <ard.biesheuvel@linaro.org> > Cc: Julien Grall <julien.grall@arm.com> > Contributed-under: TianoCore Contribution Agreement 1.1 > Signed-off-by: Heyi Guo <guoheyi@huawei.com> > Signed-off-by: Heyi Guo <heyi.guo@linaro.org> > --- > ArmVirtPkg/ArmVirt.dsc.inc | 4 ++++ > ArmVirtPkg/ArmVirtQemu.dsc | 1 + > 2 files changed, 5 insertions(+) (2) I think whenever you modify ArmVirtQemu.dsc, you almost always have to reflect it to ArmVirtQemuKernel.dsc as well. > diff --git a/ArmVirtPkg/ArmVirt.dsc.inc b/ArmVirtPkg/ArmVirt.dsc.inc > index d172a08..5ace31b 100644 > --- a/ArmVirtPkg/ArmVirt.dsc.inc > +++ b/ArmVirtPkg/ArmVirt.dsc.inc > @@ -240,8 +240,12 @@ > MemoryAllocationLib|MdePkg/Library/UefiMemoryAllocationLib/UefiMemoryAllocationLib.inf > CapsuleLib|MdeModulePkg/Library/DxeCapsuleLibNull/DxeCapsuleLibNull.inf > !if $(TARGET) != RELEASE > +!if $(RT_DEBUG) == TRUE > + SerialPortLib|ArmVirtPkg/Library/FdtPL011SerialPortLib/FdtPL011SerialPortLibRuntime.inf > +!else > DebugLib|MdePkg/Library/DxeRuntimeDebugLibSerialPort/DxeRuntimeDebugLibSerialPort.inf > !endif > +!endif (3) Unless you intend to bring this feature to Xen at some point, adding the lib resolution to "ArmVirt.dsc.inc" seems wrong. IIRC, in such cases we usually move (triplicate) the default lib resolution to the individual DSC files, and then update only the QEMU ones separately (and leave the Xen one alone). > !if $(SECURE_BOOT_ENABLE) == TRUE > BaseCryptLib|CryptoPkg/Library/BaseCryptLib/RuntimeCryptLib.inf > diff --git a/ArmVirtPkg/ArmVirtQemu.dsc b/ArmVirtPkg/ArmVirtQemu.dsc > index d703317..b279d7d 100644 > --- a/ArmVirtPkg/ArmVirtQemu.dsc > +++ b/ArmVirtPkg/ArmVirtQemu.dsc > @@ -37,6 +37,7 @@ > DEFINE SECURE_BOOT_ENABLE = FALSE > DEFINE NETWORK_IP6_ENABLE = FALSE > DEFINE HTTP_BOOT_ENABLE = FALSE > + DEFINE RT_DEBUG = FALSE > > !include ArmVirtPkg/ArmVirt.dsc.inc > > Thanks Laszlo ^ permalink raw reply [flat|nested] 7+ messages in thread
end of thread, other threads:[~2019-04-02 7:38 UTC | newest] Thread overview: 7+ messages (download: mbox.gz follow: Atom feed -- links below jump to the message on this page -- 2019-04-01 9:06 [PATCH 0/2] Enable runtime serial port debug for ArmVirtQemu Heyi Guo 2019-04-01 9:06 ` [PATCH 1/2] ArmVirtPkg: add runtime instance of FdtPL011SerialPortLib Heyi Guo 2019-04-01 16:14 ` Laszlo Ersek 2019-04-02 1:45 ` Heyi Guo 2019-04-02 7:38 ` Laszlo Ersek 2019-04-01 9:06 ` [PATCH 2/2] ArmVirtQemu: enable runtime debug by build flag Heyi Guo 2019-04-01 16:14 ` Laszlo Ersek
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox