From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from us-smtp-delivery-124.mimecast.com (us-smtp-delivery-124.mimecast.com [216.205.24.124]) by mx.groups.io with SMTP id smtpd.web09.36334.1610380471714500996 for ; Mon, 11 Jan 2021 07:54:31 -0800 Authentication-Results: mx.groups.io; dkim=pass header.i=@redhat.com header.s=mimecast20190719 header.b=Qj0h6jtT; spf=pass (domain: redhat.com, ip: 216.205.24.124, mailfrom: lersek@redhat.com) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=redhat.com; s=mimecast20190719; t=1610380470; h=from:from:reply-to:subject:subject:date:date:message-id:message-id: to:to:cc:cc:mime-version:mime-version:content-type:content-type: content-transfer-encoding:content-transfer-encoding: in-reply-to:in-reply-to:references:references; bh=Y7mBIBjL+4/XLY52WwAc8JJG6nR/ij5JYEgP8tOhyD0=; b=Qj0h6jtTOmXLRDMTcBVc8VsiFFZwSmYdtE7Fg/AeLeES8vhND2+a2z+SiVk5GB2iPWNNoF NwC8uwPgmho4OAgOOA9zgjXCxbIqOlCDkhO/ABlUTvmWAywyqwSKbih6b2DerzLTH/1oBx L37q20ll+dGCJ3NgMbj1d/zoHTTyukA= Received: from mimecast-mx01.redhat.com (mimecast-mx01.redhat.com [209.132.183.4]) (Using TLS) by relay.mimecast.com with ESMTP id us-mta-277-DZfpxVspOjSytNmV75Q-TA-1; Mon, 11 Jan 2021 10:54:27 -0500 X-MC-Unique: DZfpxVspOjSytNmV75Q-TA-1 Received: from smtp.corp.redhat.com (int-mx08.intmail.prod.int.phx2.redhat.com [10.5.11.23]) (using TLSv1.2 with cipher AECDH-AES256-SHA (256/256 bits)) (No client certificate requested) by mimecast-mx01.redhat.com (Postfix) with ESMTPS id AF1BF180A0B0; Mon, 11 Jan 2021 15:54:25 +0000 (UTC) Received: from lacos-laptop-7.usersys.redhat.com (ovpn-112-114.ams2.redhat.com [10.36.112.114]) by smtp.corp.redhat.com (Postfix) with ESMTP id 0969319C59; Mon, 11 Jan 2021 15:54:23 +0000 (UTC) Subject: Re: [edk2-devel] [PATCH] ArmPlatformPkg/NorFlashDxe: use correct PCD accessors To: devel@edk2.groups.io, ard.biesheuvel@arm.com Cc: leif@nuviainc.com, Vijayenthiran Subramaniam , Masahisa Kojima , Sami Mujawar References: <20210111105719.18700-1-ard.biesheuvel@arm.com> From: "Laszlo Ersek" Message-ID: Date: Mon, 11 Jan 2021 16:54:23 +0100 MIME-Version: 1.0 In-Reply-To: <20210111105719.18700-1-ard.biesheuvel@arm.com> X-Scanned-By: MIMEDefang 2.84 on 10.5.11.23 Authentication-Results: relay.mimecast.com; auth=pass smtp.auth=CUSA124A263 smtp.mailfrom=lersek@redhat.com X-Mimecast-Spam-Score: 0 X-Mimecast-Originator: redhat.com Content-Type: text/plain; charset=utf-8 Content-Language: en-US Content-Transfer-Encoding: 7bit On 01/11/21 11:57, Ard Biesheuvel wrote: > Commit 8015f3f6d4005d83 ("ArmPlatformPkg: Enable support for flash in > 64-bit address space") updated the NorFlash DXE and StMM drivers to > take alternate PCDs into account when discovering the base of the > NOR flash regions. > > This introduced a disparity between the declarations of the PCD references > in the .INF files, which permits the use of dynamic PCDs, and the code > itself, which now uses FixedPcdGet() accessors. On platforms that actually > use dynamic PCDs, this results in a build error. > > So let's clean this up: > - for the DXE version, use the generic PcdGet() accessors, so dynamic PCDs > are permitted > - for the standalone MM version, redeclare the PCDs as [FixedPcd] in the > .INF description, and switch to the FixedPcdGet() accessors. > > Cc: Vijayenthiran Subramaniam > Cc: Masahisa Kojima > Cc: Sami Mujawar > Signed-off-by: Ard Biesheuvel > --- > ArmPlatformPkg/Drivers/NorFlashDxe/NorFlashStandaloneMm.inf | 3 ++- > ArmPlatformPkg/Drivers/NorFlashDxe/NorFlashDxe.c | 4 ++-- > ArmPlatformPkg/Drivers/NorFlashDxe/NorFlashStandaloneMm.c | 10 +++++----- > 3 files changed, 9 insertions(+), 8 deletions(-) Looks justified to me. Acked-by: Laszlo Ersek Thanks, Laszlo > diff --git a/ArmPlatformPkg/Drivers/NorFlashDxe/NorFlashStandaloneMm.inf b/ArmPlatformPkg/Drivers/NorFlashDxe/NorFlashStandaloneMm.inf > index b2f72fb4de20..56347a8bc7c1 100644 > --- a/ArmPlatformPkg/Drivers/NorFlashDxe/NorFlashStandaloneMm.inf > +++ b/ArmPlatformPkg/Drivers/NorFlashDxe/NorFlashStandaloneMm.inf > @@ -49,7 +49,7 @@ [Guids] > [Protocols] > gEfiSmmFirmwareVolumeBlockProtocolGuid > > -[Pcd.common] > +[FixedPcd] > gEfiMdeModulePkgTokenSpaceGuid.PcdFlashNvStorageVariableBase64 > gEfiMdeModulePkgTokenSpaceGuid.PcdFlashNvStorageVariableBase > gEfiMdeModulePkgTokenSpaceGuid.PcdFlashNvStorageVariableSize > @@ -60,6 +60,7 @@ [Pcd.common] > gEfiMdeModulePkgTokenSpaceGuid.PcdFlashNvStorageFtwSpareBase > gEfiMdeModulePkgTokenSpaceGuid.PcdFlashNvStorageFtwSpareSize > > +[FeaturePcd] > gArmPlatformTokenSpaceGuid.PcdNorFlashCheckBlockLocked > > [Depex] > diff --git a/ArmPlatformPkg/Drivers/NorFlashDxe/NorFlashDxe.c b/ArmPlatformPkg/Drivers/NorFlashDxe/NorFlashDxe.c > index 28dc8e125c78..f412731200cf 100644 > --- a/ArmPlatformPkg/Drivers/NorFlashDxe/NorFlashDxe.c > +++ b/ArmPlatformPkg/Drivers/NorFlashDxe/NorFlashDxe.c > @@ -422,8 +422,8 @@ NorFlashFvbInitialize ( > EFI_MEMORY_UC | EFI_MEMORY_RUNTIME); > ASSERT_EFI_ERROR (Status); > > - mFlashNvStorageVariableBase = (FixedPcdGet64 (PcdFlashNvStorageVariableBase64) != 0) ? > - FixedPcdGet64 (PcdFlashNvStorageVariableBase64) : FixedPcdGet32 (PcdFlashNvStorageVariableBase); > + mFlashNvStorageVariableBase = (PcdGet64 (PcdFlashNvStorageVariableBase64) != 0) ? > + PcdGet64 (PcdFlashNvStorageVariableBase64) : PcdGet32 (PcdFlashNvStorageVariableBase); > > // Set the index of the first LBA for the FVB > Instance->StartLba = (mFlashNvStorageVariableBase - Instance->RegionBaseAddress) / Instance->Media.BlockSize; > diff --git a/ArmPlatformPkg/Drivers/NorFlashDxe/NorFlashStandaloneMm.c b/ArmPlatformPkg/Drivers/NorFlashDxe/NorFlashStandaloneMm.c > index 8a4fb395d286..4ebbc06e1de3 100644 > --- a/ArmPlatformPkg/Drivers/NorFlashDxe/NorFlashStandaloneMm.c > +++ b/ArmPlatformPkg/Drivers/NorFlashDxe/NorFlashStandaloneMm.c > @@ -299,15 +299,15 @@ NorFlashInitialise ( > for (Index = 0; Index < mNorFlashDeviceCount; Index++) { > // Check if this NOR Flash device contain the variable storage region > > - if (PcdGet64 (PcdFlashNvStorageVariableBase64) != 0) { > + if (FixedPcdGet64 (PcdFlashNvStorageVariableBase64) != 0) { > ContainVariableStorage = > - (NorFlashDevices[Index].RegionBaseAddress <= PcdGet64 (PcdFlashNvStorageVariableBase64)) && > - (PcdGet64 (PcdFlashNvStorageVariableBase64) + PcdGet32 (PcdFlashNvStorageVariableSize) <= > + (NorFlashDevices[Index].RegionBaseAddress <= FixedPcdGet64 (PcdFlashNvStorageVariableBase64)) && > + (FixedPcdGet64 (PcdFlashNvStorageVariableBase64) + FixedPcdGet32 (PcdFlashNvStorageVariableSize) <= > NorFlashDevices[Index].RegionBaseAddress + NorFlashDevices[Index].Size); > } else { > ContainVariableStorage = > - (NorFlashDevices[Index].RegionBaseAddress <= PcdGet32 (PcdFlashNvStorageVariableBase)) && > - (PcdGet32 (PcdFlashNvStorageVariableBase) + PcdGet32 (PcdFlashNvStorageVariableSize) <= > + (NorFlashDevices[Index].RegionBaseAddress <= FixedPcdGet32 (PcdFlashNvStorageVariableBase)) && > + (FixedPcdGet32 (PcdFlashNvStorageVariableBase) + FixedPcdGet32 (PcdFlashNvStorageVariableSize) <= > NorFlashDevices[Index].RegionBaseAddress + NorFlashDevices[Index].Size); > } > >