Hi Laszlo, My notes added below. Thanks, Chao On 2024/1/30 03:49, Laszlo Ersek wrote: > 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 >> Cc: Laszlo Ersek >> Cc: Leif Lindholm >> Cc: Sami Mujawar >> Cc: Gerd Hoffmann >> Cc: Jiewen Yao >> Signed-off-by: Chao Li >> --- >> 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_., ]+] Next time I will add these two configures in my git config file, it will be reflected in V9 if it needed to commit. > > 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. Ha, yes, patch 18 is added a new type, and I just add them under this new type and it worked. Do you recommend these two PCDs keep the type with PcdsFixedAtBuild only? If yes, in V9 I would move them under PcdsFixedAtBuild. > > Reviewed-by: Laszlo Ersek > > > > > -=-=-=-=-=-=-=-=-=-=-=- Groups.io Links: You receive all messages sent to this group. View/Reply Online (#114755): https://edk2.groups.io/g/devel/message/114755 Mute This Topic: https://groups.io/mt/103971670/7686176 Group Owner: devel+owner@edk2.groups.io Unsubscribe: https://edk2.groups.io/g/devel/unsub [rebecca@openfw.io] -=-=-=-=-=-=-=-=-=-=-=-