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

["Chao Li" <lichao@loongson.cn>]

[-- Attachment #1: Type: text/plain, Size: 10301 bytes --]

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<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_., ]+]
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<lersek@redhat.com>
>
>
>
> 
>


-=-=-=-=-=-=-=-=-=-=-=-
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]
-=-=-=-=-=-=-=-=-=-=-=-



[-- Attachment #2: Type: text/html, Size: 12246 bytes --]