From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from mail02.groups.io (mail02.groups.io [66.175.222.108]) by spool.mail.gandi.net (Postfix) with ESMTPS id 6B2A2740032 for ; Tue, 30 Jan 2024 01:24:36 +0000 (UTC) DKIM-Signature: a=rsa-sha256; bh=0/6oq8mVciMN29SKtUbaTwfExY1oT4o6jEExrp+o8Ao=; c=relaxed/simple; d=groups.io; h=Message-ID:Date:MIME-Version:User-Agent:Subject:To:Cc:References:From:In-Reply-To:Precedence:List-Subscribe:List-Help:Sender:List-Id:Mailing-List:Delivered-To:Reply-To:List-Unsubscribe-Post:List-Unsubscribe:Content-Type; s=20140610; t=1706577875; v=1; b=rIUagLeFwo+bxOwXprQnyCJ2dqhV/ops1rRZ0hGtL02mKyF4JGmwIhXLTdWrzRoBlri+Sgtu pVqhSKZwa6XZi522ThvXEMVZEOKtJx966/4DfRBbXQST8bFr9tBy0tqWrnYyXmP/tfl0qfzlBSL heiTXJcDwaZxpq3tMo5p/AUs= X-Received: by 127.0.0.2 with SMTP id XHrLYY7687511x8X2Reauw3R; Mon, 29 Jan 2024 17:24:35 -0800 X-Received: from mail.loongson.cn (mail.loongson.cn [114.242.206.163]) by mx.groups.io with SMTP id smtpd.web10.8611.1706577873567606596 for ; Mon, 29 Jan 2024 17:24:34 -0800 X-Received: from loongson.cn (unknown [10.40.24.149]) by gateway (Coremail) with SMTP id _____8BxTOnLT7hlfxwIAA--.14964S3; Tue, 30 Jan 2024 09:24:28 +0800 (CST) X-Received: from [10.40.24.149] (unknown [10.40.24.149]) by localhost.localdomain (Coremail) with SMTP id AQAAf8BxXs3GT7hlAxAnAA--.18986S3; Tue, 30 Jan 2024 09:24:22 +0800 (CST) Message-ID: <9bb8c832-7791-40af-89a4-ca5794c026de@loongson.cn> Date: Tue, 30 Jan 2024 09:24:22 +0800 MIME-Version: 1.0 User-Agent: Mozilla Thunderbird Subject: Re: [edk2-devel] [PATCH v8 24/37] ArmVirtPkg: Move two PCD variables into OvmfPkg To: devel@edk2.groups.io, lersek@redhat.com Cc: Ard Biesheuvel , Leif Lindholm , Sami Mujawar , Gerd Hoffmann , Jiewen Yao References: <20240126062715.3099433-1-lichao@loongson.cn> <20240126063017.3102349-1-lichao@loongson.cn> From: "Chao Li" In-Reply-To: X-CM-TRANSID: AQAAf8BxXs3GT7hlAxAnAA--.18986S3 X-CM-SenderInfo: xolfxt3r6o00pqjv00gofq/1tbiAQAMCGW3YeEFUwABsA X-Coremail-Antispam: 1Uk129KBj93XoW3Gr1fZw4UuF43Gr47ZFWrZwc_yoWDGF4UpF s3KrZ5t3s7JFy3tFsxury8G3WF9Fs0kF15Gw4av348WFn5GF1xuF15KayUt34UZFnxCF1f Xrs0vr1DZryqv3gCm3ZEXasCq-sJn29KB7ZKAUJUUUUU529EdanIXcx71UUUUU7KY7ZEXa sCq-sGcSsGvfJ3UbIjqfuFe4nvWSU5nxnvy29KBjDU0xBIdaVrnRJUUUymb4IE77IF4wAF F20E14v26r1j6r4UM7CY07I20VC2zVCF04k26cxKx2IYs7xG6rWj6s0DM7CIcVAFz4kK6r 106r15M28lY4IEw2IIxxk0rwA2F7IY1VAKz4vEj48ve4kI8wA2z4x0Y4vE2Ix0cI8IcVAF wI0_JFI_Gr1l84ACjcxK6xIIjxv20xvEc7CjxVAFwI0_Jr0_Gr1l84ACjcxK6I8E87Iv67 AKxVW8JVWxJwA2z4x0Y4vEx4A2jsIEc7CjxVAFwI0_Gr0_Gr1UM2AIxVAIcxkEcVAq07x2 0xvEncxIr21l57IF6xkI12xvs2x26I8E6xACxx1lYx0E2Ix0cI8IcVAFwI0_Jr0_Jr4lYx 0Ex4A2jsIE14v26r1j6r4UMcvjeVCFs4IE7xkEbVWUJVW8JwACjcxG0xvEwIxGrwCjr7xv wVCIw2I0I7xG6c02F41l42xK82IYc2Ij64vIr41l4I8I3I0E4IkC6x0Yz7v_Jr0_Gr1lx2 IqxVAqx4xG67AKxVWUGVWUWwC20s026x8GjcxK67AKxVWUGVWUWwC2zVAF1VAY17CE14v2 6r126r1DMIIYrxkI7VAKI48JMIIF0xvE2Ix0cI8IcVAFwI0_Jr0_JF4lIxAIcVC0I7IYx2 IY6xkF7I0E14v26r1j6r4UMIIF0xvE42xK8VAvwI8IcIk0rVWUJVWUCwCI42IY6I8E87Iv 67AKxVWUJVW8JwCI42IY6I8E87Iv6xkF7I0E14v26r1j6r4UYxBIdaVFxhVjvjDU0xZFpf 9x07UEFAJUUUUU= Precedence: Bulk List-Subscribe: List-Help: Sender: devel@edk2.groups.io List-Id: Mailing-List: list devel@edk2.groups.io; contact devel+owner@edk2.groups.io Reply-To: devel@edk2.groups.io,lichao@loongson.cn List-Unsubscribe-Post: List-Unsubscribe=One-Click List-Unsubscribe: X-Gm-Message-State: ojbzso9gOfnvwiRuB7qq2SLNx7686176AA= Content-Type: multipart/alternative; boundary="------------GEmwWLTI0jIM69hGnQdkb4IJ" X-GND-Status: LEGIT Authentication-Results: spool.mail.gandi.net; dkim=pass header.d=groups.io header.s=20140610 header.b=rIUagLeF; dmarc=none; spf=pass (spool.mail.gandi.net: domain of bounce@groups.io designates 66.175.222.108 as permitted sender) smtp.mailfrom=bounce@groups.io --------------GEmwWLTI0jIM69hGnQdkb4IJ Content-Type: text/plain; charset=UTF-8; format=flowed Content-Transfer-Encoding: quoted-printable 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=3D4584 >> >> 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|0x0000000= 4 >> >> [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, 0= xDF, 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 A= cpi. >> # Cloud Hypervisor has no other way to pass Rsdp address to the gues= t except use a PCD. >> # >> gArmVirtTokenSpaceGuid.PcdCloudHvAcpiRsdpBaseAddress|0x0|UINT64|0x00= 000005 >> - >> - ## >> - # Whether the EFI memory attributes protocol should be uninstalled be= fore >> - # invoking the OS loader. This may be needed to work around problemat= ic >> - # builds of shim that use the protocol incorrectly. >> - gArmVirtTokenSpaceGuid.PcdUninstallMemAttrProtocol|FALSE|BOOLEAN|0x00= 000006 >> 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) =3D=3D TRUE >> gEfiMdePkgTokenSpaceGuid.PcdDefaultTerminalType|4 >> # Set terminal type to TtyTerm, the value encoded is EFI_TTY_TERM_GU= ID >> - gArmVirtTokenSpaceGuid.PcdTerminalTypeGuidBuffer|{0x80, 0x6d, 0x91, 0= x7d, 0xb1, 0x5b, 0x8c, 0x45, 0xa4, 0x8f, 0xe2, 0x5f, 0xdd, 0x51, 0xef, 0x94= } >> + gUefiOvmfPkgTokenSpaceGuid.PcdTerminalTypeGuidBuffer|{0x80, 0x6d, 0x9= 1, 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/ArmVirtQemuKe= rnel.dsc >> index 6a6ecfc12a..c22a422353 100644 >> --- a/ArmVirtPkg/ArmVirtQemuKernel.dsc >> +++ b/ArmVirtPkg/ArmVirtQemuKernel.dsc >> @@ -147,7 +147,7 @@ >> !if $(TTY_TERMINAL) =3D=3D TRUE >> gEfiMdePkgTokenSpaceGuid.PcdDefaultTerminalType|4 >> # Set terminal type to TtyTerm, the value encoded is EFI_TTY_TERM_GU= ID >> - gArmVirtTokenSpaceGuid.PcdTerminalTypeGuidBuffer|{0x80, 0x6d, 0x91, 0= x7d, 0xb1, 0x5b, 0x8c, 0x45, 0xa4, 0x8f, 0xe2, 0x5f, 0xdd, 0x51, 0xef, 0x94= } >> + gUefiOvmfPkgTokenSpaceGuid.PcdTerminalTypeGuidBuffer|{0x80, 0x6d, 0x9= 1, 0x7d, 0xb1, 0x5b, 0x8c, 0x45, 0xa4, 0x8f, 0xe2, 0x5f, 0xdd, 0x51, 0xef, = 0x94} >> !else >> gEfiMdePkgTokenSpaceGuid.PcdDefaultTerminalType|1 >> !endif >> diff --git a/ArmVirtPkg/Library/PlatformBootManagerLib/PlatformBootManag= erLib.inf b/ArmVirtPkg/Library/PlatformBootManagerLib/PlatformBootManagerLi= b.inf >> index 70e4ebf94a..8e7cd5605f 100644 >> --- a/ArmVirtPkg/Library/PlatformBootManagerLib/PlatformBootManagerLib.i= nf >> +++ b/ArmVirtPkg/Library/PlatformBootManagerLib/PlatformBootManagerLib.i= nf >> @@ -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, 0xA= 6, 0xDF, 0x19, 0xB4, 0xD3, 0x11, 0x9A, 0x2D, 0x00, 0x90, 0x27, 0x3F, 0xC1, = 0x4D}|VOID*|0x66 >> + >> + ## >> + # Whether the EFI memory attributes protocol should be uninstalled be= fore >> + # invoking the OS loader. This may be needed to work around problemat= ic >> + # builds of shim that use the protocol incorrectly. >> + gUefiOvmfPkgTokenSpaceGuid.PcdUninstallMemAttrProtocol|FALSE|BOOLEAN|= 0x67 >> + >> [PcdsFeatureFlag] >> gUefiOvmfPkgTokenSpaceGuid.PcdQemuBootOrderPciTranslation|TRUE|BOOLE= AN|0x1c >> gUefiOvmfPkgTokenSpaceGuid.PcdQemuBootOrderMmioTranslation|FALSE|BOO= LEAN|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 =3D BaseTools/Conf/gitattributes > > [diff "ini"] > xfuncname =3D ^\\[[A-Za-z0-9_., ]+] Next time I will add these two configures in my git config file, it will=20 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, 0xA= 6, 0xDF, 0x19, 0xB4, 0xD3, 0x11, 0x9A, 0x2D, 0x00, 0x90, 0x27, 0x3F, 0xC1, = 0x4D}|VOID*|0x66 >> + >> + ## >> + # Whether the EFI memory attributes protocol should be uninstalled be= fore >> + # invoking the OS loader. This may be needed to work around problemat= ic >> + # builds of shim that use the protocol incorrectly. >> + gUefiOvmfPkgTokenSpaceGuid.PcdUninstallMemAttrProtocol|FALSE|BOOLEAN|= 0x67 >> + >> [PcdsFeatureFlag] >> gUefiOvmfPkgTokenSpaceGuid.PcdQemuBootOrderPciTranslation|TRUE|BOOLE= AN|0x1c >> gUefiOvmfPkgTokenSpaceGuid.PcdQemuBootOrderMmioTranslation|FALSE|BOO= LEAN|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=20 new type and it worked. Do you recommend these two PCDs keep the type=20 with PcdsFixedAtBuild only? If yes, in V9 I would move them under=20 PcdsFixedAtBuild. > > Reviewed-by: Laszlo Ersek > > > >=20 > -=3D-=3D-=3D-=3D-=3D-=3D-=3D-=3D-=3D-=3D-=3D- 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] -=3D-=3D-=3D-=3D-=3D-=3D-=3D-=3D-=3D-=3D-=3D- --------------GEmwWLTI0jIM69hGnQdkb4IJ Content-Type: text/html; charset=UTF-8 Content-Transfer-Encoding: quoted-printable

Hi Laszlo,

My notes added below.


=
Thanks,
Chao
On 2024/1/30 03:49, Laszlo Ersek wrote:<= br> 
On 1/26/24 07:30, Chao Li wrot=
e:

Move the PcdTerminalTypeGuid=
Buffer 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=
=3D4584

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. T=
he
-  # 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}|V=
OID*|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 ex=
cept use a PCD.
   #
   gArmVirtTokenSpaceGuid.PcdCloudHvAcpiRsdpBaseAddress|0x0|UINT64|0x000000=
05
-
-  ##
-  # Whether the EFI memory attributes protocol should be uninstalled befor=
e
-  # invoking the OS loader. This may be needed to work around problematic
-  # builds of shim that use the protocol incorrectly.
-  gArmVirtTokenSpaceGuid.PcdUninstallMemAttrProtocol|FALSE|BOOLEAN|0x00000=
006
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) =3D=3D 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, 0x9=
4}
 !else
   gEfiMdePkgTokenSpaceGuid.PcdDefaultTerminalType|1
 !endif
diff --git a/ArmVirtPkg/ArmVirtQemuKernel.dsc b/ArmVirtPkg/ArmVirtQemuKerne=
l.dsc
index 6a6ecfc12a..c22a422353 100644
--- a/ArmVirtPkg/ArmVirtQemuKernel.dsc
+++ b/ArmVirtPkg/ArmVirtQemuKernel.dsc
@@ -147,7 +147,7 @@
 !if $(TTY_TERMINAL) =3D=3D 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, 0x9=
4}
 !else
   gEfiMdePkgTokenSpaceGuid.PcdDefaultTerminalType|1
 !endif
diff --git a/ArmVirtPkg/Library/PlatformBootManagerLib/PlatformBootManagerL=
ib.inf b/ArmVirtPkg/Library/PlatformBootManagerLib/PlatformBootManagerLib.i=
nf
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|0x6=
f

+  #
+  # Binary representation of the GUID that determines the terminal type. T=
he
+  # 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, 0x4=
D}|VOID*|0x66
+
+  ##
+  # Whether the EFI memory attributes protocol should be uninstalled befor=
e
+  # invoking the OS loader. This may be needed to work around problematic
+  # builds of shim that use the protocol incorrectly.
+  gUefiOvmfPkgTokenSpaceGuid.PcdUninstallMemAttrProtocol|FALSE|BOOLEAN|0x6=
7
+
 [PcdsFeatureFlag]
   gUefiOvmfPkgTokenSpaceGuid.PcdQemuBootOrderPciTranslation|TRUE|BOOLEAN|0=
x1c
   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 =3D BaseTools/Conf/gitattributes

[diff "ini"]
        xfuncname =3D ^\\[[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|0x6=
f

+  #
+  # Binary representation of the GUID that determines the terminal type. T=
he
+  # 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, 0x4=
D}|VOID*|0x66
+
+  ##
+  # Whether the EFI memory attributes protocol should be uninstalled befor=
e
+  # invoking the OS loader. This may be needed to work around problematic
+  # builds of shim that use the protocol incorrectly.
+  gUefiOvmfPkgTokenSpaceGuid.PcdUninstallMemAttrProtocol|FALSE|BOOLEAN|0x6=
7
+
 [PcdsFeatureFlag]
   gUefiOvmfPkgTokenSpaceGuid.PcdQemuBootOrderPciTranslation|TRUE|BOOLEAN|0=
x1c
   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:

=20 You receive all messages sent to this group. =20 =20

View/Reply Online (#114755) | =20 | Mute= This Topic | New Topic
Your Subscriptio= n | Contact Group Owner | Unsubscribe [rebecca@openfw.io]

_._,_._,_
--------------GEmwWLTI0jIM69hGnQdkb4IJ--