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

[Laszlo Ersek <lersek@redhat.com>]

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