Re: [PATCH 2/2] ArmVirtQemu: enable runtime debug by build flag

[Laszlo Ersek <lersek@redhat.com>]

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