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 4A58CAC12B0 for ; Mon, 29 Jan 2024 19:49:42 +0000 (UTC) DKIM-Signature: a=rsa-sha256; bh=ee6OVb1PFD2QQevdjB9eidbDSIwYC1+EBr93WPohcaI=; c=relaxed/simple; d=groups.io; h=Message-ID:Date:MIME-Version: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-Language:Content-Type:Content-Transfer-Encoding; s=20140610; t=1706557780; v=1; b=YObpC+CjV2cHjpjwWGR4x7069keXcjT704wN5gm/20Y7SOO8Xgjh6w+O/KRxLERkIx7qc1QA 9SEU1/Mnb0M8R6yA/6sZvAyMbaa+HqghdyN/GmMkSIx7Dy/SttxIGmCqAPnKlXh40zLKNppfsBz f5/J7UgRIAullBXGuHsL5BVc= X-Received: by 127.0.0.2 with SMTP id 96jBYY7687511xNsJ3gr8uk6; Mon, 29 Jan 2024 11:49:40 -0800 X-Received: from us-smtp-delivery-124.mimecast.com (us-smtp-delivery-124.mimecast.com [170.10.133.124]) by mx.groups.io with SMTP id smtpd.web11.296.1706557780108389865 for ; Mon, 29 Jan 2024 11:49:40 -0800 X-Received: from mimecast-mx02.redhat.com (mimecast-mx02.redhat.com [66.187.233.88]) by relay.mimecast.com with ESMTP with STARTTLS (version=TLSv1.3, cipher=TLS_AES_256_GCM_SHA384) id us-mta-460-FvEYYiuQOcq27ToztbXdYg-1; Mon, 29 Jan 2024 14:49:35 -0500 X-MC-Unique: FvEYYiuQOcq27ToztbXdYg-1 X-Received: from smtp.corp.redhat.com (int-mx07.intmail.prod.int.rdu2.redhat.com [10.11.54.7]) (using TLSv1.3 with cipher TLS_AES_256_GCM_SHA384 (256/256 bits) key-exchange X25519 server-signature RSA-PSS (2048 bits) server-digest SHA256) (No client certificate requested) by mimecast-mx02.redhat.com (Postfix) with ESMTPS id 6DD34845DC5; Mon, 29 Jan 2024 19:49:35 +0000 (UTC) X-Received: from [10.39.192.97] (unknown [10.39.192.97]) by smtp.corp.redhat.com (Postfix) with ESMTPS id 291431C060B1; Mon, 29 Jan 2024 19:49:33 +0000 (UTC) Message-ID: Date: Mon, 29 Jan 2024 20:49:32 +0100 MIME-Version: 1.0 Subject: Re: [edk2-devel] [PATCH v8 24/37] ArmVirtPkg: Move two PCD variables into OvmfPkg To: Chao Li , devel@edk2.groups.io 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: "Laszlo Ersek" In-Reply-To: <20240126063017.3102349-1-lichao@loongson.cn> X-Scanned-By: MIMEDefang 3.4.1 on 10.11.54.7 X-Mimecast-Spam-Score: 0 X-Mimecast-Originator: redhat.com 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,lersek@redhat.com List-Unsubscribe-Post: List-Unsubscribe=One-Click List-Unsubscribe: X-Gm-Message-State: mdhKkeLWbLv1A62041fn7lFUx7686176AA= Content-Language: en-US Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: quoted-printable X-GND-Status: LEGIT Authentication-Results: spool.mail.gandi.net; dkim=pass header.d=groups.io header.s=20140610 header.b=YObpC+Cj; dmarc=fail reason="SPF not aligned (relaxed), DKIM not aligned (relaxed)" header.from=redhat.com (policy=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 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|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, 0x= DF, 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 Acp= i. > # Cloud Hypervisor has no other way to pass Rsdp address to the guest = except use a PCD. > # > gArmVirtTokenSpaceGuid.PcdCloudHvAcpiRsdpBaseAddress|0x0|UINT64|0x0000= 0005 > - > - ## > - # Whether the EFI memory attributes protocol should be uninstalled bef= ore > - # invoking the OS loader. This may be needed to work around problemati= c > - # builds of shim that use the protocol incorrectly. > - gArmVirtTokenSpaceGuid.PcdUninstallMemAttrProtocol|FALSE|BOOLEAN|0x000= 00006 > 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, 0x= 7d, 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, 0= x94} > !else > gEfiMdePkgTokenSpaceGuid.PcdDefaultTerminalType|1 > !endif > diff --git a/ArmVirtPkg/ArmVirtQemuKernel.dsc b/ArmVirtPkg/ArmVirtQemuKer= nel.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, 0x= 7d, 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, 0= x94} > !else > gEfiMdePkgTokenSpaceGuid.PcdDefaultTerminalType|1 > !endif > diff --git a/ArmVirtPkg/Library/PlatformBootManagerLib/PlatformBootManage= rLib.inf b/ArmVirtPkg/Library/PlatformBootManagerLib/PlatformBootManagerLib= .inf > index 70e4ebf94a..8e7cd5605f 100644 > --- a/ArmVirtPkg/Library/PlatformBootManagerLib/PlatformBootManagerLib.in= f > +++ b/ArmVirtPkg/Library/PlatformBootManagerLib/PlatformBootManagerLib.in= f > @@ -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|0= x6f > > + # > + # 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, 0= x4D}|VOID*|0x66 > + > + ## > + # Whether the EFI memory attributes protocol should be uninstalled bef= ore > + # invoking the OS loader. This may be needed to work around problemati= c > + # builds of shim that use the protocol incorrectly. > + gUefiOvmfPkgTokenSpaceGuid.PcdUninstallMemAttrProtocol|FALSE|BOOLEAN|0= x67 > + > [PcdsFeatureFlag] > gUefiOvmfPkgTokenSpaceGuid.PcdQemuBootOrderPciTranslation|TRUE|BOOLEAN= |0x1c > gUefiOvmfPkgTokenSpaceGuid.PcdQemuBootOrderMmioTranslation|FALSE|BOOLE= AN|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_., ]+] 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|0= x6f >=20 > + # > + # 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, 0= x4D}|VOID*|0x66 > + > + ## > + # Whether the EFI memory attributes protocol should be uninstalled bef= ore > + # invoking the OS loader. This may be needed to work around problemati= c > + # builds of shim that use the protocol incorrectly. > + gUefiOvmfPkgTokenSpaceGuid.PcdUninstallMemAttrProtocol|FALSE|BOOLEAN|0= x67 > + > [PcdsFeatureFlag] > gUefiOvmfPkgTokenSpaceGuid.PcdQemuBootOrderPciTranslation|TRUE|BOOLEAN= |0x1c > gUefiOvmfPkgTokenSpaceGuid.PcdQemuBootOrderMmioTranslation|FALSE|BOOLE= AN|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 -=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 (#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/19134562= 12/xyzzy [rebecca@openfw.io] -=3D-=3D-=3D-=3D-=3D-=3D-=3D-=3D-=3D-=3D-=3D-