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

[Laszlo Ersek <lersek@redhat.com>]

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