Re: [edk2-devel] [PATCH v8 24/37] ArmVirtPkg: Move two PCD variables into OvmfPkg

["Laszlo Ersek" <lersek@redhat.com>]

On 1/26/24 07:30, Chao Li wrote:
> Move the PcdTerminalTypeGuidBuffer and PcdUninstallMemAttrProtocol into
> OvmfPkg so other ARCH can easily use it.
>
> Build-tested only (with "ArmVirtQemu.dsc and OvmfPkgX64.dsc").
>
> BZ: https://bugzilla.tianocore.org/show_bug.cgi?id=4584
>
> Cc: Ard Biesheuvel <ardb+tianocore@kernel.org>
> Cc: Laszlo Ersek <lersek@redhat.com>
> Cc: Leif Lindholm <quic_llindhol@quicinc.com>
> Cc: Sami Mujawar <sami.mujawar@arm.com>
> Cc: Gerd Hoffmann <kraxel@redhat.com>
> Cc: Jiewen Yao <jiewen.yao@intel.com>
> Signed-off-by: Chao Li <lichao@loongson.cn>
> ---
>  ArmVirtPkg/ArmVirtPkg.dec                           | 13 -------------
>  ArmVirtPkg/ArmVirtQemu.dsc                          |  2 +-
>  ArmVirtPkg/ArmVirtQemuKernel.dsc                    |  2 +-
>  .../PlatformBootManagerLib.inf                      |  5 ++---
>  OvmfPkg/OvmfPkg.dec                                 | 13 +++++++++++++
>  5 files changed, 17 insertions(+), 18 deletions(-)
>
> diff --git a/ArmVirtPkg/ArmVirtPkg.dec b/ArmVirtPkg/ArmVirtPkg.dec
> index a658c91031..6aa5ea05f4 100644
> --- a/ArmVirtPkg/ArmVirtPkg.dec
> +++ b/ArmVirtPkg/ArmVirtPkg.dec
> @@ -41,21 +41,8 @@
>    gArmVirtTokenSpaceGuid.PcdTpm2SupportEnabled|FALSE|BOOLEAN|0x00000004
>
>  [PcdsFixedAtBuild, PcdsPatchableInModule]
> -  #
> -  # Binary representation of the GUID that determines the terminal type. The
> -  # size must be exactly 16 bytes. The default value corresponds to
> -  # EFI_VT_100_GUID.
> -  #
> -  gArmVirtTokenSpaceGuid.PcdTerminalTypeGuidBuffer|{0x65, 0x60, 0xA6, 0xDF, 0x19, 0xB4, 0xD3, 0x11, 0x9A, 0x2D, 0x00, 0x90, 0x27, 0x3F, 0xC1, 0x4D}|VOID*|0x00000007
> -
>    ##
>    # This is the physical address of Rsdp which is the core struct of Acpi.
>    # Cloud Hypervisor has no other way to pass Rsdp address to the guest except use a PCD.
>    #
>    gArmVirtTokenSpaceGuid.PcdCloudHvAcpiRsdpBaseAddress|0x0|UINT64|0x00000005
> -
> -  ##
> -  # Whether the EFI memory attributes protocol should be uninstalled before
> -  # invoking the OS loader. This may be needed to work around problematic
> -  # builds of shim that use the protocol incorrectly.
> -  gArmVirtTokenSpaceGuid.PcdUninstallMemAttrProtocol|FALSE|BOOLEAN|0x00000006
> diff --git a/ArmVirtPkg/ArmVirtQemu.dsc b/ArmVirtPkg/ArmVirtQemu.dsc
> index dbd2396c78..147180f645 100644
> --- a/ArmVirtPkg/ArmVirtQemu.dsc
> +++ b/ArmVirtPkg/ArmVirtQemu.dsc
> @@ -182,7 +182,7 @@
>  !if $(TTY_TERMINAL) == TRUE
>    gEfiMdePkgTokenSpaceGuid.PcdDefaultTerminalType|4
>    # Set terminal type to TtyTerm, the value encoded is EFI_TTY_TERM_GUID
> -  gArmVirtTokenSpaceGuid.PcdTerminalTypeGuidBuffer|{0x80, 0x6d, 0x91, 0x7d, 0xb1, 0x5b, 0x8c, 0x45, 0xa4, 0x8f, 0xe2, 0x5f, 0xdd, 0x51, 0xef, 0x94}
> +  gUefiOvmfPkgTokenSpaceGuid.PcdTerminalTypeGuidBuffer|{0x80, 0x6d, 0x91, 0x7d, 0xb1, 0x5b, 0x8c, 0x45, 0xa4, 0x8f, 0xe2, 0x5f, 0xdd, 0x51, 0xef, 0x94}
>  !else
>    gEfiMdePkgTokenSpaceGuid.PcdDefaultTerminalType|1
>  !endif
> diff --git a/ArmVirtPkg/ArmVirtQemuKernel.dsc b/ArmVirtPkg/ArmVirtQemuKernel.dsc
> index 6a6ecfc12a..c22a422353 100644
> --- a/ArmVirtPkg/ArmVirtQemuKernel.dsc
> +++ b/ArmVirtPkg/ArmVirtQemuKernel.dsc
> @@ -147,7 +147,7 @@
>  !if $(TTY_TERMINAL) == TRUE
>    gEfiMdePkgTokenSpaceGuid.PcdDefaultTerminalType|4
>    # Set terminal type to TtyTerm, the value encoded is EFI_TTY_TERM_GUID
> -  gArmVirtTokenSpaceGuid.PcdTerminalTypeGuidBuffer|{0x80, 0x6d, 0x91, 0x7d, 0xb1, 0x5b, 0x8c, 0x45, 0xa4, 0x8f, 0xe2, 0x5f, 0xdd, 0x51, 0xef, 0x94}
> +  gUefiOvmfPkgTokenSpaceGuid.PcdTerminalTypeGuidBuffer|{0x80, 0x6d, 0x91, 0x7d, 0xb1, 0x5b, 0x8c, 0x45, 0xa4, 0x8f, 0xe2, 0x5f, 0xdd, 0x51, 0xef, 0x94}
>  !else
>    gEfiMdePkgTokenSpaceGuid.PcdDefaultTerminalType|1
>  !endif
> diff --git a/ArmVirtPkg/Library/PlatformBootManagerLib/PlatformBootManagerLib.inf b/ArmVirtPkg/Library/PlatformBootManagerLib/PlatformBootManagerLib.inf
> index 70e4ebf94a..8e7cd5605f 100644
> --- a/ArmVirtPkg/Library/PlatformBootManagerLib/PlatformBootManagerLib.inf
> +++ b/ArmVirtPkg/Library/PlatformBootManagerLib/PlatformBootManagerLib.inf
> @@ -29,7 +29,6 @@
>    QemuKernel.c
>
>  [Packages]
> -  ArmVirtPkg/ArmVirtPkg.dec
>    MdeModulePkg/MdeModulePkg.dec
>    MdePkg/MdePkg.dec
>    OvmfPkg/OvmfPkg.dec
> @@ -56,15 +55,15 @@
>    UefiRuntimeServicesTableLib
>
>  [FixedPcd]
> -  gArmVirtTokenSpaceGuid.PcdUninstallMemAttrProtocol
>    gEfiMdePkgTokenSpaceGuid.PcdUartDefaultBaudRate
>    gEfiMdePkgTokenSpaceGuid.PcdUartDefaultDataBits
>    gEfiMdePkgTokenSpaceGuid.PcdUartDefaultParity
>    gEfiMdePkgTokenSpaceGuid.PcdUartDefaultStopBits
> +  gUefiOvmfPkgTokenSpaceGuid.PcdUninstallMemAttrProtocol
>
>  [Pcd]
> -  gArmVirtTokenSpaceGuid.PcdTerminalTypeGuidBuffer
>    gEfiMdePkgTokenSpaceGuid.PcdPlatformBootTimeOut
> +  gUefiOvmfPkgTokenSpaceGuid.PcdTerminalTypeGuidBuffer
>
>  [Guids]
>    gEfiEndOfDxeEventGroupGuid
> diff --git a/OvmfPkg/OvmfPkg.dec b/OvmfPkg/OvmfPkg.dec
> index 13e69e6648..fbc81e4c80 100644
> --- a/OvmfPkg/OvmfPkg.dec
> +++ b/OvmfPkg/OvmfPkg.dec
> @@ -405,6 +405,19 @@
>    #
>    gUefiOvmfPkgTokenSpaceGuid.PcdDeviceTreeAllocationPadding|256|UINT32|0x6f
>
> +  #
> +  # Binary representation of the GUID that determines the terminal type. The
> +  # size must be exactly 16 bytes. The default value corresponds to
> +  # EFI_VT_100_GUID.
> +  #
> +  gUefiOvmfPkgTokenSpaceGuid.PcdTerminalTypeGuidBuffer|{0x65, 0x60, 0xA6, 0xDF, 0x19, 0xB4, 0xD3, 0x11, 0x9A, 0x2D, 0x00, 0x90, 0x27, 0x3F, 0xC1, 0x4D}|VOID*|0x66
> +
> +  ##
> +  # Whether the EFI memory attributes protocol should be uninstalled before
> +  # invoking the OS loader. This may be needed to work around problematic
> +  # builds of shim that use the protocol incorrectly.
> +  gUefiOvmfPkgTokenSpaceGuid.PcdUninstallMemAttrProtocol|FALSE|BOOLEAN|0x67
> +
>  [PcdsFeatureFlag]
>    gUefiOvmfPkgTokenSpaceGuid.PcdQemuBootOrderPciTranslation|TRUE|BOOLEAN|0x1c
>    gUefiOvmfPkgTokenSpaceGuid.PcdQemuBootOrderMmioTranslation|FALSE|BOOLEAN|0x1d

This patch is hard to review; you can make it easier (for next time):

I suggest adding the following to your edk2 git config:

[core]
        attributesFile = BaseTools/Conf/gitattributes

[diff "ini"]
        xfuncname = ^\\[[A-Za-z0-9_., ]+]

Subsequently, the @@ hunk headers will show you the DEC file section
that's being modified; like this:

> diff --git a/OvmfPkg/OvmfPkg.dec b/OvmfPkg/OvmfPkg.dec
> index 13e69e664842..fbc81e4c8070 100644
> --- a/OvmfPkg/OvmfPkg.dec
> +++ b/OvmfPkg/OvmfPkg.dec
> @@ -405,6 +405,19 @@ [PcdsFixedAtBuild, PcdsPatchableInModule]
>    #
>    gUefiOvmfPkgTokenSpaceGuid.PcdDeviceTreeAllocationPadding|256|UINT32|0x6f
> 
> +  #
> +  # Binary representation of the GUID that determines the terminal type. The
> +  # size must be exactly 16 bytes. The default value corresponds to
> +  # EFI_VT_100_GUID.
> +  #
> +  gUefiOvmfPkgTokenSpaceGuid.PcdTerminalTypeGuidBuffer|{0x65, 0x60, 0xA6, 0xDF, 0x19, 0xB4, 0xD3, 0x11, 0x9A, 0x2D, 0x00, 0x90, 0x27, 0x3F, 0xC1, 0x4D}|VOID*|0x66
> +
> +  ##
> +  # Whether the EFI memory attributes protocol should be uninstalled before
> +  # invoking the OS loader. This may be needed to work around problematic
> +  # builds of shim that use the protocol incorrectly.
> +  gUefiOvmfPkgTokenSpaceGuid.PcdUninstallMemAttrProtocol|FALSE|BOOLEAN|0x67
> +
>  [PcdsFeatureFlag]
>    gUefiOvmfPkgTokenSpaceGuid.PcdQemuBootOrderPciTranslation|TRUE|BOOLEAN|0x1c
>    gUefiOvmfPkgTokenSpaceGuid.PcdQemuBootOrderMmioTranslation|FALSE|BOOLEAN|0x1d

Note the "@@ [PcdsFixedAtBuild, PcdsPatchableInModule]" part.

This helps reviewers a lot.

Originally I thought you were moving these PCDs from the original
section

  [PcdsFixedAtBuild, PcdsPatchableInModule]

to the new section

  [PcdsDynamic, PcdsDynamicEx]

which would force a PCD type change.

(It is up to the DSC file to pick a type for the PCDs it assigns default
values to, but if the before-after type lists in the DEC file(s) have an
empty intersection, like it *seems* in this case, then the PCDs in the
DSC files cannot avoid a type change.)

In fact I didn't understand how this patch could successfully build.
After the patch, PcdTerminalTypeGuidBuffer could only be dynamic or
dynamicEx, but in the ArmVirt dsc files, the default values were still
set in section [PcdsFixedAtBuild.common]. So that would certainly cause
a build error.

But then I applied your entire set locally, and showed this patch with
the "smart" hunk headers enabled (as explained above). And that made me
realize this patch was actually correct.

The reason is that patch 18/37 "ArmVirtPkg: Move PCD of FDT base address
and FDT padding to OvmfPkg" introduces a new section to the OVMF DEC
file, [PcdsFixedAtBuild, PcdsPatchableInModule], and this PCD movement
targets the new section. So that's fine, but patch 18 is quite a bit
earlier in the series than this one; you can support reviewers by
including the section names in the hunk headers.

Reviewed-by: Laszlo Ersek <lersek@redhat.com>



-=-=-=-=-=-=-=-=-=-=-=-
Groups.io Links: You receive all messages sent to this group.
View/Reply Online (#114739): https://edk2.groups.io/g/devel/message/114739
Mute This Topic: https://groups.io/mt/103971670/7686176
Group Owner: devel+owner@edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/leave/12367111/7686176/1913456212/xyzzy [rebecca@openfw.io]
-=-=-=-=-=-=-=-=-=-=-=-