Re: [PATCH 1/2] ArmVirtPkg: add runtime instance of FdtPL011SerialPortLib

[Heyi Guo <guoheyi@huawei.com>]

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
>
> .
>