public inbox for devel@edk2.groups.io
 help / color / mirror / Atom feed
From: "Laszlo Ersek" <lersek@redhat.com>
To: devel@edk2.groups.io, jon@solid-run.com
Cc: Hao A Wu <hao.a.wu@intel.com>,
	Liming Gao <gaoliming@byosoft.com.cn>,
	"Ard Biesheuvel (TianoCore)" <ardb+tianocore@kernel.org>,
	"Leif Lindholm (Nuvia address)" <leif@nuviainc.com>
Subject: Re: [edk2-devel] Conflicting virtual addresses causing Runtime Services issues
Date: Thu, 11 Mar 2021 23:25:24 +0100	[thread overview]
Message-ID: <4841241f-fc6d-6185-efe6-ed9a536534dd@redhat.com> (raw)
In-Reply-To: <290a35ce-9116-af00-85f4-8df1c5228680@redhat.com>

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

Adding Ard and Leif, comments below:

On 03/11/21 15:50, Laszlo Ersek wrote:
> On 03/11/21 10:48, Jon Nettleton wrote:

[...]

>> And this is where the pointer gets remapped again and into the MMIO
>> space of the nor flash.  If I remove the calls to ConvertPointer for
>> the FvbProtocol I am still seeing those addresses getting remapped
>> but only once and runtime works as expected.
>>
>> I am seeing that in
>> MdeModulePkg/Universal/Variable/RuntimeDxe/VariableDxe.c
>> &mVariableModuleGlobal->FvbInstance->* are all being converted.  It
>> is possible this is a long standing bug and it just so happens that
>> our configuration has caused a conflict and exposed it.
>
> Yes, this is curious, I noticed it too yesterday, trying to see where
> the FVB protocol member function pointers were converted. I found that
> OVMF's flash driver (OvmfPkg/QemuFlashFvbServicesRuntimeDxe) didn't do
> it, but MdeModulePkg/Universal/Variable/RuntimeDxe did. That was
> certainly strange, as the variable driver is a consumer of the
> protocol (not the producer thereof), so I'd say it has no business
> poking new values into the protocol interface structure.

[...]

> ... Strangely, the other flash (FVB) driver in edk2,
> ArmPlatformPkg/Drivers/NorFlashDxe, *does* perform the conversion
> itself! See NorFlashVirtualNotifyEvent().
>
> I don't understand that. Is it possible that, with
> "ArmPlatformPkg/Drivers/NorFlashDxe" too, the conversion happens
> *twice*, but (at least) one of those mappings is "identity"?

Confirmed.

I had to write some elaborate debug patches for determining this,
because in ArmVirtQemu, I cannot produce DEBUG output from the
SetVirtualAddressMap() notification functions. So here's the approach I
took:

(1) Introduce a new GUID-ed HOB structure in MdeModulePkg. The structure
itself lives in reserved memory, but its address is exposed in a GUID-ed
HOB. The structure is named FVB_ADDRESS_LIST, and it has the following
fields:

  - signature ("FVBADRLS" -- FVB Address List)
  - 16 entries of:
    - owner signature [what driver set this entry]
    - address
  - number of entries used (aka next entry to fill)

(2) In PlatformPei, allocate and initialize this structure (in reserved
memory), and expose its address via the GUID-ed HOB. Furthermore,
produce a log message with the allocation address.

(3) In NorFlashDxe, look up the structure via the GUID-ed HOB, in the
entry point function; remember the address in a global variable. In the
SetVirtualAddressMap() handler function, treat the conversion of the
"GetPhysicalAddress" FVB member function specially: via the global
variable pointer to FVB_ADDRESS_LIST in reserved memory, save both the
physical (original) and the virtual (converted) address of the
"GetPhysicalAddress" FVB member function, in new entries. As owner
signature in both entries, use "NORFLASH".

(4) In the runtime DXE variable driver, do the exact same thing, just
use a different "owner signature" -- "VARIABLE".

(5) Once the guest is up and running, run "efibootmgr --delete-timeout"
at a root prompt in the guest, deleting the existent "Timeout" UEFI
non-volatile variable, for verifying that the runtime variable (write)
service is functional.

(6) Using the log message from point (2):

> PlatformPeim: FvbAddressList @ 13FEC9000

hexdump the guest memory containing the FVB_ADDRESS_LIST, as follows:

> $ virsh qemu-monitor-command aavmf.rhel7.registered --hmp xp  /268cb 0x13FEC9000

Ccomments to the right of the hexdump:

> 000000013fec9000: 'F' 'V' 'B' 'A' 'D' 'R' 'L' 'S'                         <- structure signature: FVBADRLS
> 000000013fec9008: 'N' 'O' 'R' 'F' 'L' 'A' 'S' 'H'                         <- entry[0], signature: NORFLASH
> 000000013fec9010: 'T' ' ' '\xc6' ';' '\x01' '\x00' '\x00' '\x00'          <- entry[0], GetPhysicalAddress *physical*: 0x000000013bc62054
> 000000013fec9018: 'N' 'O' 'R' 'F' 'L' 'A' 'S' 'H'                         <- entry[1], signature: NORFLASH
> 000000013fec9020: 'T' ' ' 'N' '$' '\x00' '\x00' '\x00' '\x00'             <- entry[1], GetPhysicalAddress *virtual*: 0x00000000244e2054
> 000000013fec9028: 'V' 'A' 'R' 'I' 'A' 'B' 'L' 'E'                         <- entry[2], signature: VARIABLE
> 000000013fec9030: 'T' ' ' 'N' '$' '\x00' '\x00' '\x00' '\x00'             <- entry[2], GetPhysicalAddress *physical*: 0x00000000244e2054
> 000000013fec9038: 'V' 'A' 'R' 'I' 'A' 'B' 'L' 'E'                         <- entry[3], signature: VARIABLE
> 000000013fec9040: 'T' ' ' 'N' '$' '\x00' '\x00' '\x00' '\x00'             <- entry[3], GetPhysicalAddress *virtual*: 0x00000000244e2054
> 000000013fec9048: '\x00' '\x00' '\x00' '\x00' '\x00' '\x00' '\x00' '\x00'
> 000000013fec9050: '\x00' '\x00' '\x00' '\x00' '\x00' '\x00' '\x00' '\x00'
> 000000013fec9058: '\x00' '\x00' '\x00' '\x00' '\x00' '\x00' '\x00' '\x00'
> 000000013fec9060: '\x00' '\x00' '\x00' '\x00' '\x00' '\x00' '\x00' '\x00'
> 000000013fec9068: '\x00' '\x00' '\x00' '\x00' '\x00' '\x00' '\x00' '\x00'
> 000000013fec9070: '\x00' '\x00' '\x00' '\x00' '\x00' '\x00' '\x00' '\x00'
> 000000013fec9078: '\x00' '\x00' '\x00' '\x00' '\x00' '\x00' '\x00' '\x00'
> 000000013fec9080: '\x00' '\x00' '\x00' '\x00' '\x00' '\x00' '\x00' '\x00'
> 000000013fec9088: '\x00' '\x00' '\x00' '\x00' '\x00' '\x00' '\x00' '\x00'
> 000000013fec9090: '\x00' '\x00' '\x00' '\x00' '\x00' '\x00' '\x00' '\x00'
> 000000013fec9098: '\x00' '\x00' '\x00' '\x00' '\x00' '\x00' '\x00' '\x00'
> 000000013fec90a0: '\x00' '\x00' '\x00' '\x00' '\x00' '\x00' '\x00' '\x00'
> 000000013fec90a8: '\x00' '\x00' '\x00' '\x00' '\x00' '\x00' '\x00' '\x00'
> 000000013fec90b0: '\x00' '\x00' '\x00' '\x00' '\x00' '\x00' '\x00' '\x00'
> 000000013fec90b8: '\x00' '\x00' '\x00' '\x00' '\x00' '\x00' '\x00' '\x00'
> 000000013fec90c0: '\x00' '\x00' '\x00' '\x00' '\x00' '\x00' '\x00' '\x00'
> 000000013fec90c8: '\x00' '\x00' '\x00' '\x00' '\x00' '\x00' '\x00' '\x00'
> 000000013fec90d0: '\x00' '\x00' '\x00' '\x00' '\x00' '\x00' '\x00' '\x00'
> 000000013fec90d8: '\x00' '\x00' '\x00' '\x00' '\x00' '\x00' '\x00' '\x00'
> 000000013fec90e0: '\x00' '\x00' '\x00' '\x00' '\x00' '\x00' '\x00' '\x00'
> 000000013fec90e8: '\x00' '\x00' '\x00' '\x00' '\x00' '\x00' '\x00' '\x00'
> 000000013fec90f0: '\x00' '\x00' '\x00' '\x00' '\x00' '\x00' '\x00' '\x00'
> 000000013fec90f8: '\x00' '\x00' '\x00' '\x00' '\x00' '\x00' '\x00' '\x00'
> 000000013fec9100: '\x00' '\x00' '\x00' '\x00' '\x00' '\x00' '\x00' '\x00'
> 000000013fec9108: '\x04' '\x00' '\x00' '\x00'                             <- number of entries used: 4

This shows the following:

- both NorFlashDxe and the runtime DXE variable driver converted the
  FVB.GetPhysicalAddress member function,

- the NorFlashDxe driver acted first, the runtime DXE variable driver
  acted second,

- when the runtime DXE variable driver "converted" the "physical"
  address to virtual address, there was no change (and no crash!),
  because the virtual address map passed in by the Linux kernel
  apparently identity maps this area -- just as I guessed.

So we definitely have a bug (only Linux's page tables save us from the
crash); now the question is:

Which driver is wrong to even attempt the conversion of the FVB member
functions?

The answer must be documented somewhere highly visible.

Debug patches attached, for the record (based on commit edd46cd407ea).

Thanks
Laszlo

[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #2: 0001-MdeModulePkg-Guid-introduce-FVB_ADDRESS_LIST.patch --]
[-- Type: text/x-patch; name="0001-MdeModulePkg-Guid-introduce-FVB_ADDRESS_LIST.patch", Size: 3391 bytes --]

From 6670b74f94dca9dcdcfdcabc370527c735bb152d Mon Sep 17 00:00:00 2001
From: Laszlo Ersek <lersek@redhat.com>
Date: Thu, 11 Mar 2021 21:08:09 +0100
Subject: [PATCH 1/4] MdeModulePkg/Guid: introduce FVB_ADDRESS_LIST

Introduce a new GUIDed HOB. The GUIDed HOB contains a 64-bit physical
address (FVB_ADDRESS_LIST_PTR), pointing to FVB_ADDRESS_LIST.
FVB_ADDRESS_LIST itself lives in reserved memory, and drivers can populate
it with up to 16 FVB-related addresses (64-bit words).

This will be used to stash the original (physical) and converted (virtual)
addresses of some FVB member functions, for guest memory dumping and
debugging purposes.

Signed-off-by: Laszlo Ersek <lersek@redhat.com>
---
 MdeModulePkg/MdeModulePkg.dec              |  3 ++
 MdeModulePkg/Include/Guid/FvbAddressList.h | 45 ++++++++++++++++++++
 2 files changed, 48 insertions(+)

diff --git a/MdeModulePkg/MdeModulePkg.dec b/MdeModulePkg/MdeModulePkg.dec
index f1144144be14..a67f4e8eb92c 100644
--- a/MdeModulePkg/MdeModulePkg.dec
+++ b/MdeModulePkg/MdeModulePkg.dec
@@ -400,14 +400,17 @@ [Guids]
 
   ## GUID indicates the capsule is to store Capsule On Disk file names.
   gEdkiiCapsuleOnDiskNameGuid = { 0x98c80a4f, 0xe16b, 0x4d11, { 0x93, 0x9a, 0xab, 0xe5, 0x61, 0x26, 0x3, 0x30 } }
 
   ## Include/Guid/MigratedFvInfo.h
   gEdkiiMigratedFvInfoGuid = { 0xc1ab12f7, 0x74aa, 0x408d, { 0xa2, 0xf4, 0xc6, 0xce, 0xfd, 0x17, 0x98, 0x71 } }
 
+  ## Include/Guid/FvbAddressList.h
+  gEdkiiFvbAddressListGuid = { 0xaa87bf00, 0x7f76, 0x4e32, { 0xb3, 0x92, 0x86, 0xa1, 0x3e, 0x37, 0x33, 0x11 } }
+
 [Ppis]
   ## Include/Ppi/AtaController.h
   gPeiAtaControllerPpiGuid       = { 0xa45e60d1, 0xc719, 0x44aa, { 0xb0, 0x7a, 0xaa, 0x77, 0x7f, 0x85, 0x90, 0x6d }}
 
   ## Include/Ppi/UsbHostController.h
   gPeiUsbHostControllerPpiGuid   = { 0x652B38A9, 0x77F4, 0x453F, { 0x89, 0xD5, 0xE7, 0xBD, 0xC3, 0x52, 0xFC, 0x53 }}
 
diff --git a/MdeModulePkg/Include/Guid/FvbAddressList.h b/MdeModulePkg/Include/Guid/FvbAddressList.h
new file mode 100644
index 000000000000..92aa781383f5
--- /dev/null
+++ b/MdeModulePkg/Include/Guid/FvbAddressList.h
@@ -0,0 +1,45 @@
+/** @file
+  Stash FVB-related addresses in a reserved memory structure, pointed-to by a
+  pointer living in a GUID-ed HOB.
+
+  Copyright (C) 2021 Red Hat, Inc.
+
+  SPDX-License-Identifier: BSD-2-Clause-Patent
+**/
+
+#ifndef FVB_ADDRESS_LIST_H_
+#define FVB_ADDRESS_LIST_H_
+
+#define EDKII_FVB_ADDRESS_LIST_GUID                    \
+  {                                                    \
+    0xaa87bf00,                                        \
+    0x7f76,                                            \
+    0x4e32,                                            \
+    { 0xb3, 0x92, 0x86, 0xa1, 0x3e, 0x37, 0x33, 0x11 } \
+  }
+
+#define FVB_ADDRESS_LIST_SIGNATURE \
+  SIGNATURE_64 ('F', 'V', 'B', 'A', 'D', 'R', 'L', 'S')
+
+typedef struct {
+  UINT64 OwnerSignature;
+  UINT64 Address;
+} FVB_ADDRESS_LIST_ENTRY;
+
+//
+// lives in reserved memory
+//
+typedef struct {
+  UINT64                 Signature;
+  FVB_ADDRESS_LIST_ENTRY Entry[16];
+  UINT32                 Next;
+} FVB_ADDRESS_LIST;
+
+//
+// HOB contents: a pointer to FVB_ADDRESS_LIST
+//
+typedef UINT64 FVB_ADDRESS_LIST_PTR;
+
+extern EFI_GUID gEdkiiFvbAddressListGuid;
+
+#endif // FVB_ADDRESS_LIST_H_
-- 
2.19.1.3.g30247aa5d201


[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #3: 0002-ArmVirtPkg-PlatformPeiLib-create-FVB_ADDRESS_LIST.patch --]
[-- Type: text/x-patch; name="0002-ArmVirtPkg-PlatformPeiLib-create-FVB_ADDRESS_LIST.patch", Size: 4986 bytes --]

From d61bf620beb379097eca2705e0cf135f941f13bd Mon Sep 17 00:00:00 2001
From: Laszlo Ersek <lersek@redhat.com>
Date: Thu, 11 Mar 2021 21:35:55 +0100
Subject: [PATCH 2/4] ArmVirtPkg/PlatformPeiLib: create FVB_ADDRESS_LIST

Allocate and initialize FVB_ADDRESS_LIST in a new reserved memory page,
and expose the address in the EDKII_FVB_ADDRESS_LIST_GUID HOB.

Signed-off-by: Laszlo Ersek <lersek@redhat.com>
---
 ArmVirtPkg/Library/PlatformPeiLib/PlatformPeiLib.inf |  2 +
 ArmVirtPkg/Library/PlatformPeiLib/PlatformPeiLib.c   | 58 +++++++++++++-------
 2 files changed, 41 insertions(+), 19 deletions(-)

diff --git a/ArmVirtPkg/Library/PlatformPeiLib/PlatformPeiLib.inf b/ArmVirtPkg/Library/PlatformPeiLib/PlatformPeiLib.inf
index 3f97ef080520..78eef962fdac 100644
--- a/ArmVirtPkg/Library/PlatformPeiLib/PlatformPeiLib.inf
+++ b/ArmVirtPkg/Library/PlatformPeiLib/PlatformPeiLib.inf
@@ -27,14 +27,15 @@ [Packages]
   OvmfPkg/OvmfPkg.dec
   SecurityPkg/SecurityPkg.dec
 
 [FeaturePcd]
   gArmVirtTokenSpaceGuid.PcdTpm2SupportEnabled
 
 [LibraryClasses]
+  BaseMemoryLib
   DebugLib
   HobLib
   FdtLib
   PcdLib
   PeiServicesLib
 
 [FixedPcd]
@@ -48,11 +49,12 @@ [Pcd]
 
 [Ppis]
   gOvmfTpmDiscoveredPpiGuid                               ## SOMETIMES_PRODUCES
   gPeiTpmInitializationDonePpiGuid                        ## SOMETIMES_PRODUCES
 
 [Guids]
   gEarlyPL011BaseAddressGuid
+  gEdkiiFvbAddressListGuid
   gFdtHobGuid
 
 [Depex]
   gEfiPeiMemoryDiscoveredPpiGuid
diff --git a/ArmVirtPkg/Library/PlatformPeiLib/PlatformPeiLib.c b/ArmVirtPkg/Library/PlatformPeiLib/PlatformPeiLib.c
index 6c4028e17995..079ecfad336d 100644
--- a/ArmVirtPkg/Library/PlatformPeiLib/PlatformPeiLib.c
+++ b/ArmVirtPkg/Library/PlatformPeiLib/PlatformPeiLib.c
@@ -5,14 +5,16 @@
 *
 *  SPDX-License-Identifier: BSD-2-Clause-Patent
 *
 **/
 
 #include <PiPei.h>
 
+#include <Guid/FvbAddressList.h>
+#include <Library/BaseMemoryLib.h>
 #include <Library/MemoryAllocationLib.h>
 #include <Library/DebugLib.h>
 #include <Library/HobLib.h>
 #include <Library/PcdLib.h>
 #include <Library/PeiServicesLib.h>
 #include <libfdt.h>
 
@@ -33,33 +35,35 @@ STATIC CONST EFI_PEI_PPI_DESCRIPTOR mTpm2InitializationDonePpi = {
 
 EFI_STATUS
 EFIAPI
 PlatformPeim (
   VOID
   )
 {
-  VOID               *Base;
-  VOID               *NewBase;
-  UINTN              FdtSize;
-  UINTN              FdtPages;
-  UINT64             *FdtHobData;
-  UINT64             *UartHobData;
-  INT32              Node, Prev;
-  INT32              Parent, Depth;
-  CONST CHAR8        *Compatible;
-  CONST CHAR8        *CompItem;
-  CONST CHAR8        *NodeStatus;
-  INT32              Len;
-  INT32              RangesLen;
-  INT32              StatusLen;
-  CONST UINT64       *RegProp;
-  CONST UINT32       *RangesProp;
-  UINT64             UartBase;
-  UINT64             TpmBase;
-  EFI_STATUS         Status;
+  VOID                 *Base;
+  VOID                 *NewBase;
+  UINTN                FdtSize;
+  UINTN                FdtPages;
+  UINT64               *FdtHobData;
+  UINT64               *UartHobData;
+  INT32                Node, Prev;
+  INT32                Parent, Depth;
+  CONST CHAR8          *Compatible;
+  CONST CHAR8          *CompItem;
+  CONST CHAR8          *NodeStatus;
+  INT32                Len;
+  INT32                RangesLen;
+  INT32                StatusLen;
+  CONST UINT64         *RegProp;
+  CONST UINT32         *RangesProp;
+  UINT64               UartBase;
+  UINT64               TpmBase;
+  EFI_STATUS           Status;
+  FVB_ADDRESS_LIST     *FvbAddressList;
+  FVB_ADDRESS_LIST_PTR *FvbAddressListPtrHobData;
 
   Base = (VOID*)(UINTN)PcdGet64 (PcdDeviceTreeInitialBaseAddress);
   ASSERT (Base != NULL);
   ASSERT (fdt_check_header (Base) == 0);
 
   FdtSize = fdt_totalsize (Base) + PcdGet32 (PcdDeviceTreeAllocationPadding);
   FdtPages = EFI_SIZE_TO_PAGES (FdtSize);
@@ -181,9 +185,25 @@ PlatformPeim (
       Status = PeiServicesInstallPpi (&mTpm2InitializationDonePpi);
     }
     ASSERT_EFI_ERROR (Status);
   }
 
   BuildFvHob (PcdGet64 (PcdFvBaseAddress), PcdGet32 (PcdFvSize));
 
+  //
+  // Allocate, initialize, and expose FVB_ADDRESS_LIST.
+  //
+  FvbAddressList = AllocateReservedPages (
+                     EFI_SIZE_TO_PAGES (sizeof *FvbAddressList));
+  ASSERT (FvbAddressList != NULL);
+  DEBUG ((DEBUG_VERBOSE, "%a: FvbAddressList @ %p\n", __FUNCTION__,
+    (VOID *)FvbAddressList));
+  FvbAddressList->Signature = FVB_ADDRESS_LIST_SIGNATURE;
+  ZeroMem (FvbAddressList->Entry, sizeof FvbAddressList->Entry);
+  FvbAddressList->Next = 0;
+  FvbAddressListPtrHobData = BuildGuidHob (&gEdkiiFvbAddressListGuid,
+                               sizeof *FvbAddressListPtrHobData);
+  ASSERT (FvbAddressListPtrHobData != NULL);
+  *FvbAddressListPtrHobData = (UINTN)FvbAddressList;
+
   return EFI_SUCCESS;
 }
-- 
2.19.1.3.g30247aa5d201


[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #4: 0003-ArmPlatformPkg-NorFlashDxe-populate-FVB_ADDRESS_LIST.patch --]
[-- Type: text/x-patch; name="0003-ArmPlatformPkg-NorFlashDxe-populate-FVB_ADDRESS_LIST.patch", Size: 7153 bytes --]

From a656046171cd98600d4f6810f1d9f856aba19b6e Mon Sep 17 00:00:00 2001
From: Laszlo Ersek <lersek@redhat.com>
Date: Thu, 11 Mar 2021 22:16:41 +0100
Subject: [PATCH 3/4] ArmPlatformPkg/NorFlashDxe: populate FVB_ADDRESS_LIST

When the SetVirtualAddressMap() handler runs, and we convert (among other
things) the GetPhysicalAddress() FVB member function pointer, stash both
the physical and the virtual addresses of this member function, in
FVB_ADDRESS_LIST.

Signed-off-by: Laszlo Ersek <lersek@redhat.com>
---
 ArmPlatformPkg/Drivers/NorFlashDxe/NorFlashDxe.inf |  1 +
 ArmPlatformPkg/Drivers/NorFlashDxe/NorFlash.c      | 27 ++++++++++++++++++--
 ArmPlatformPkg/Drivers/NorFlashDxe/NorFlashDxe.c   | 11 ++++++++
 3 files changed, 37 insertions(+), 2 deletions(-)

diff --git a/ArmPlatformPkg/Drivers/NorFlashDxe/NorFlashDxe.inf b/ArmPlatformPkg/Drivers/NorFlashDxe/NorFlashDxe.inf
index f8d4c2703143..1eba5f869c55 100644
--- a/ArmPlatformPkg/Drivers/NorFlashDxe/NorFlashDxe.inf
+++ b/ArmPlatformPkg/Drivers/NorFlashDxe/NorFlashDxe.inf
@@ -43,14 +43,15 @@ [LibraryClasses]
 
 [Guids]
   gEfiSystemNvDataFvGuid
   gEfiVariableGuid
   gEfiAuthenticatedVariableGuid
   gEfiEventVirtualAddressChangeGuid
   gEdkiiNvVarStoreFormattedGuid     ## PRODUCES ## PROTOCOL
+  gEdkiiFvbAddressListGuid
 
 [Protocols]
   gEfiBlockIoProtocolGuid
   gEfiDevicePathProtocolGuid
   gEfiFirmwareVolumeBlockProtocolGuid
   gEfiDiskIoProtocolGuid
 
diff --git a/ArmPlatformPkg/Drivers/NorFlashDxe/NorFlash.c b/ArmPlatformPkg/Drivers/NorFlashDxe/NorFlash.c
index a9e23db4461b..73faca549bda 100644
--- a/ArmPlatformPkg/Drivers/NorFlashDxe/NorFlash.c
+++ b/ArmPlatformPkg/Drivers/NorFlashDxe/NorFlash.c
@@ -3,23 +3,25 @@
   Copyright (c) 2011 - 2020, Arm Limited. All rights reserved.<BR>
   Copyright (c) 2020, Linaro, Ltd. All rights reserved.<BR>
 
   SPDX-License-Identifier: BSD-2-Clause-Patent
 
 **/
 
+#include <Guid/FvbAddressList.h>
 #include <Library/BaseMemoryLib.h>
 
 #include "NorFlash.h"
 
 //
 // Global variable declarations
 //
 extern NOR_FLASH_INSTANCE **mNorFlashInstances;
 extern UINT32               mNorFlashDeviceCount;
+extern FVB_ADDRESS_LIST     *mFvbAddressList;
 
 UINT32
 NorFlashReadStatusRegister (
   IN NOR_FLASH_INSTANCE     *Instance,
   IN UINTN                  SR_Address
   )
 {
@@ -939,35 +941,56 @@ NorFlashReset (
 VOID
 EFIAPI
 NorFlashVirtualNotifyEvent (
   IN EFI_EVENT        Event,
   IN VOID             *Context
   )
 {
-  UINTN Index;
+  UINT64                 OwnerSignature;
+  FVB_ADDRESS_LIST_ENTRY *Entry;
+  UINTN                  Index;
+
+  OwnerSignature = SIGNATURE_64 ('N', 'O', 'R', 'F', 'L', 'A', 'S', 'H');
+  Entry          = mFvbAddressList->Entry + mFvbAddressList->Next;
 
   for (Index = 0; Index < mNorFlashDeviceCount; Index++) {
+    VOID *Pointer;
+
     EfiConvertPointer (0x0, (VOID**)&mNorFlashInstances[Index]->DeviceBaseAddress);
     EfiConvertPointer (0x0, (VOID**)&mNorFlashInstances[Index]->RegionBaseAddress);
 
     // Convert BlockIo protocol
     EfiConvertPointer (0x0, (VOID**)&mNorFlashInstances[Index]->BlockIoProtocol.FlushBlocks);
     EfiConvertPointer (0x0, (VOID**)&mNorFlashInstances[Index]->BlockIoProtocol.ReadBlocks);
     EfiConvertPointer (0x0, (VOID**)&mNorFlashInstances[Index]->BlockIoProtocol.Reset);
     EfiConvertPointer (0x0, (VOID**)&mNorFlashInstances[Index]->BlockIoProtocol.WriteBlocks);
 
     // Convert Fvb
     EfiConvertPointer (0x0, (VOID**)&mNorFlashInstances[Index]->FvbProtocol.EraseBlocks);
     EfiConvertPointer (0x0, (VOID**)&mNorFlashInstances[Index]->FvbProtocol.GetAttributes);
     EfiConvertPointer (0x0, (VOID**)&mNorFlashInstances[Index]->FvbProtocol.GetBlockSize);
-    EfiConvertPointer (0x0, (VOID**)&mNorFlashInstances[Index]->FvbProtocol.GetPhysicalAddress);
+
+    Pointer = (VOID *)(UINTN)mNorFlashInstances[Index]->FvbProtocol.GetPhysicalAddress;
+    Entry->OwnerSignature = OwnerSignature;
+    Entry->Address        = (UINTN)Pointer;
+    ++Entry;
+
+    EfiConvertPointer (0x0, &Pointer);
+    mNorFlashInstances[Index]->FvbProtocol.GetPhysicalAddress =
+      (EFI_FVB_GET_PHYSICAL_ADDRESS)(UINTN)Pointer;
+
+    Entry->OwnerSignature = OwnerSignature;
+    Entry->Address        = (UINTN)Pointer;
+    ++Entry;
+
     EfiConvertPointer (0x0, (VOID**)&mNorFlashInstances[Index]->FvbProtocol.Read);
     EfiConvertPointer (0x0, (VOID**)&mNorFlashInstances[Index]->FvbProtocol.SetAttributes);
     EfiConvertPointer (0x0, (VOID**)&mNorFlashInstances[Index]->FvbProtocol.Write);
 
     if (mNorFlashInstances[Index]->ShadowBuffer != NULL) {
       EfiConvertPointer (0x0, (VOID**)&mNorFlashInstances[Index]->ShadowBuffer);
     }
   }
 
+  mFvbAddressList->Next = Entry - mFvbAddressList->Entry;
   return;
 }
diff --git a/ArmPlatformPkg/Drivers/NorFlashDxe/NorFlashDxe.c b/ArmPlatformPkg/Drivers/NorFlashDxe/NorFlashDxe.c
index f412731200cf..fad0b8c39723 100644
--- a/ArmPlatformPkg/Drivers/NorFlashDxe/NorFlashDxe.c
+++ b/ArmPlatformPkg/Drivers/NorFlashDxe/NorFlashDxe.c
@@ -2,14 +2,15 @@
 
   Copyright (c) 2011 - 2021, Arm Limited. All rights reserved.<BR>
 
   SPDX-License-Identifier: BSD-2-Clause-Patent
 
 **/
 
+#include <Guid/FvbAddressList.h>
 #include <Library/UefiLib.h>
 #include <Library/BaseMemoryLib.h>
 #include <Library/MemoryAllocationLib.h>
 #include <Library/UefiBootServicesTableLib.h>
 #include <Library/PcdLib.h>
 #include <Library/HobLib.h>
 #include <Library/DxeServicesTableLib.h>
@@ -21,14 +22,16 @@ STATIC EFI_EVENT mNorFlashVirtualAddrChangeEvent;
 //
 // Global variable declarations
 //
 NOR_FLASH_INSTANCE **mNorFlashInstances;
 UINT32               mNorFlashDeviceCount;
 UINTN                mFlashNvStorageVariableBase;
 EFI_EVENT            mFvbVirtualAddrChangeEvent;
+FVB_ADDRESS_LIST     *mFvbAddressList;
+
 
 NOR_FLASH_INSTANCE  mNorFlashInstanceTemplate = {
   NOR_FLASH_SIGNATURE, // Signature
   NULL, // Handle ... NEED TO BE FILLED
 
   0, // DeviceBaseAddress ... NEED TO BE FILLED
   0, // RegionBaseAddress ... NEED TO BE FILLED
@@ -318,19 +321,27 @@ NorFlashWriteFullBlock (
 EFI_STATUS
 EFIAPI
 NorFlashInitialise (
   IN EFI_HANDLE         ImageHandle,
   IN EFI_SYSTEM_TABLE   *SystemTable
   )
 {
+  VOID                    *Hob;
+  FVB_ADDRESS_LIST_PTR    *FvbAddressListPtrHobData;
   EFI_STATUS              Status;
   UINT32                  Index;
   NOR_FLASH_DESCRIPTION*  NorFlashDevices;
   BOOLEAN                 ContainVariableStorage;
 
+  Hob = GetFirstGuidHob (&gEdkiiFvbAddressListGuid);
+  ASSERT (Hob != NULL);
+  FvbAddressListPtrHobData = GET_GUID_HOB_DATA (Hob);
+  mFvbAddressList = (VOID*)(UINTN)*FvbAddressListPtrHobData;
+  ASSERT (mFvbAddressList->Signature == FVB_ADDRESS_LIST_SIGNATURE);
+
   Status = NorFlashPlatformInitialization ();
   if (EFI_ERROR(Status)) {
     DEBUG((EFI_D_ERROR,"NorFlashInitialise: Fail to initialize Nor Flash devices\n"));
     return Status;
   }
 
   Status = NorFlashPlatformGetDevices (&NorFlashDevices, &mNorFlashDeviceCount);
-- 
2.19.1.3.g30247aa5d201


[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #5: 0004-MdeModulePkg-VariableRuntimeDxe-populate-FVB_ADDRESS_LIST.patch --]
[-- Type: text/x-patch; name="0004-MdeModulePkg-VariableRuntimeDxe-populate-FVB_ADDRESS_LIST.patch", Size: 8524 bytes --]

From f424470f69ce8811766b1118fc801238f2242596 Mon Sep 17 00:00:00 2001
From: Laszlo Ersek <lersek@redhat.com>
Date: Thu, 11 Mar 2021 22:16:41 +0100
Subject: [PATCH 4/4] MdeModulePkg/VariableRuntimeDxe: populate
 FVB_ADDRESS_LIST

When the SetVirtualAddressMap() handler runs, and we convert (among other
things) the GetPhysicalAddress() FVB member function pointer, stash both
the physical and the virtual addresses of this member function, in
FVB_ADDRESS_LIST.

Signed-off-by: Laszlo Ersek <lersek@redhat.com>
---
 MdeModulePkg/Universal/Variable/RuntimeDxe/VariableRuntimeDxe.inf |  2 ++
 MdeModulePkg/Universal/Variable/RuntimeDxe/Variable.h             |  2 ++
 MdeModulePkg/Universal/Variable/RuntimeDxe/VariableDxe.c          | 36 ++++++++++++++++++--
 3 files changed, 38 insertions(+), 2 deletions(-)

diff --git a/MdeModulePkg/Universal/Variable/RuntimeDxe/VariableRuntimeDxe.inf b/MdeModulePkg/Universal/Variable/RuntimeDxe/VariableRuntimeDxe.inf
index c9434df631ee..e4cc49692f2d 100644
--- a/MdeModulePkg/Universal/Variable/RuntimeDxe/VariableRuntimeDxe.inf
+++ b/MdeModulePkg/Universal/Variable/RuntimeDxe/VariableRuntimeDxe.inf
@@ -120,14 +120,16 @@ [Guids]
   gEdkiiVarErrorFlagGuid
 
   ## SOMETIMES_CONSUMES   ## Variable:L"db"
   ## SOMETIMES_CONSUMES   ## Variable:L"dbx"
   ## SOMETIMES_CONSUMES   ## Variable:L"dbt"
   gEfiImageSecurityDatabaseGuid
 
+  gEdkiiFvbAddressListGuid
+
 [Pcd]
   gEfiMdeModulePkgTokenSpaceGuid.PcdFlashNvStorageVariableSize      ## CONSUMES
   gEfiMdeModulePkgTokenSpaceGuid.PcdFlashNvStorageVariableBase      ## SOMETIMES_CONSUMES
   gEfiMdeModulePkgTokenSpaceGuid.PcdFlashNvStorageVariableBase64    ## CONSUMES
   gEfiMdeModulePkgTokenSpaceGuid.PcdMaxVariableSize                 ## CONSUMES
   gEfiMdeModulePkgTokenSpaceGuid.PcdMaxAuthVariableSize             ## CONSUMES
   gEfiMdeModulePkgTokenSpaceGuid.PcdMaxVolatileVariableSize         ## CONSUMES
diff --git a/MdeModulePkg/Universal/Variable/RuntimeDxe/Variable.h b/MdeModulePkg/Universal/Variable/RuntimeDxe/Variable.h
index 0b2bb6ae6648..77c390586d35 100644
--- a/MdeModulePkg/Universal/Variable/RuntimeDxe/Variable.h
+++ b/MdeModulePkg/Universal/Variable/RuntimeDxe/Variable.h
@@ -33,14 +33,15 @@ SPDX-License-Identifier: BSD-2-Clause-Patent
 #include <Library/VarCheckLib.h>
 #include <Guid/GlobalVariable.h>
 #include <Guid/EventGroup.h>
 #include <Guid/VariableFormat.h>
 #include <Guid/SystemNvDataGuid.h>
 #include <Guid/FaultTolerantWrite.h>
 #include <Guid/VarErrorFlag.h>
+#include <Guid/FvbAddressList.h>
 
 #include "PrivilegePolymorphic.h"
 
 #define NV_STORAGE_VARIABLE_BASE (EFI_PHYSICAL_ADDRESS) \
                                    (PcdGet64 (PcdFlashNvStorageVariableBase64) != 0 ? \
                                     PcdGet64 (PcdFlashNvStorageVariableBase64) : \
                                     PcdGet32 (PcdFlashNvStorageVariableBase))
@@ -693,14 +694,15 @@ extern VARIABLE_MODULE_GLOBAL       *mVariableModuleGlobal;
 extern EFI_FIRMWARE_VOLUME_HEADER   *mNvFvHeaderCache;
 extern VARIABLE_STORE_HEADER        *mNvVariableCache;
 extern VARIABLE_INFO_ENTRY          *gVariableInfo;
 extern BOOLEAN                      mEndOfDxe;
 extern VAR_CHECK_REQUEST_SOURCE     mRequestSource;
 
 extern AUTH_VAR_LIB_CONTEXT_OUT     mAuthContextOut;
+extern FVB_ADDRESS_LIST             *mFvbAddressList;
 
 /**
   Finds variable in storage blocks of volatile and non-volatile storage areas.
 
   This code finds variable in storage blocks of volatile and non-volatile storage areas.
   If VariableName is an empty string, then we just return the first
   qualified variable without comparing VariableName and VendorGuid.
diff --git a/MdeModulePkg/Universal/Variable/RuntimeDxe/VariableDxe.c b/MdeModulePkg/Universal/Variable/RuntimeDxe/VariableDxe.c
index 0fca0bb2a9b5..b7a33e3a348d 100644
--- a/MdeModulePkg/Universal/Variable/RuntimeDxe/VariableDxe.c
+++ b/MdeModulePkg/Universal/Variable/RuntimeDxe/VariableDxe.c
@@ -32,14 +32,15 @@ EDKII_VARIABLE_POLICY_PROTOCOL      mVariablePolicyProtocol    = { EDKII_VARIABL
                                                                     ProtocolIsVariablePolicyEnabled,
                                                                     RegisterVariablePolicy,
                                                                     DumpVariablePolicy,
                                                                     LockVariablePolicy };
 EDKII_VAR_CHECK_PROTOCOL            mVarCheck                  = { VarCheckRegisterSetVariableCheckHandler,
                                                                     VarCheckVariablePropertySet,
                                                                     VarCheckVariablePropertyGet };
+FVB_ADDRESS_LIST *mFvbAddressList;
 
 /**
   Some Secure Boot Policy Variable may update following other variable changes(SecureBoot follows PK change, etc).
   Record their initial State when variable write service is ready.
 
 **/
 VOID
@@ -240,26 +241,49 @@ GetFvbCountAndBuffer (
 VOID
 EFIAPI
 VariableClassAddressChangeEvent (
   IN EFI_EVENT                            Event,
   IN VOID                                 *Context
   )
 {
-  UINTN          Index;
+  UINT64                 OwnerSignature;
+  FVB_ADDRESS_LIST_ENTRY *Entry;
+  UINTN                  Index;
+
+  OwnerSignature = SIGNATURE_64 ('V', 'A', 'R', 'I', 'A', 'B', 'L', 'E');
+  Entry          = mFvbAddressList->Entry + mFvbAddressList->Next;
 
   if (mVariableModuleGlobal->FvbInstance != NULL) {
+    VOID *Pointer;
+
     EfiConvertPointer (0x0, (VOID **) &mVariableModuleGlobal->FvbInstance->GetBlockSize);
-    EfiConvertPointer (0x0, (VOID **) &mVariableModuleGlobal->FvbInstance->GetPhysicalAddress);
+
+    Pointer = (VOID *)(UINTN)mVariableModuleGlobal->FvbInstance->GetPhysicalAddress;
+    Entry->OwnerSignature = OwnerSignature;
+    Entry->Address        = (UINTN)Pointer;
+    ++Entry;
+
+    EfiConvertPointer (0x0, &Pointer);
+    mVariableModuleGlobal->FvbInstance->GetPhysicalAddress =
+      (EFI_FVB_GET_PHYSICAL_ADDRESS)(UINTN)Pointer;
+
+    Entry->OwnerSignature = OwnerSignature;
+    Entry->Address        = (UINTN)Pointer;
+    ++Entry;
+
     EfiConvertPointer (0x0, (VOID **) &mVariableModuleGlobal->FvbInstance->GetAttributes);
     EfiConvertPointer (0x0, (VOID **) &mVariableModuleGlobal->FvbInstance->SetAttributes);
     EfiConvertPointer (0x0, (VOID **) &mVariableModuleGlobal->FvbInstance->Read);
     EfiConvertPointer (0x0, (VOID **) &mVariableModuleGlobal->FvbInstance->Write);
     EfiConvertPointer (0x0, (VOID **) &mVariableModuleGlobal->FvbInstance->EraseBlocks);
     EfiConvertPointer (0x0, (VOID **) &mVariableModuleGlobal->FvbInstance);
   }
+
+  mFvbAddressList->Next = Entry - mFvbAddressList->Entry;
+
   EfiConvertPointer (0x0, (VOID **) &mVariableModuleGlobal->PlatformLangCodes);
   EfiConvertPointer (0x0, (VOID **) &mVariableModuleGlobal->LangCodes);
   EfiConvertPointer (0x0, (VOID **) &mVariableModuleGlobal->PlatformLang);
   EfiConvertPointer (0x0, (VOID **) &mVariableModuleGlobal->VariableGlobal.NonVolatileVariableBase);
   EfiConvertPointer (0x0, (VOID **) &mVariableModuleGlobal->VariableGlobal.VolatileVariableBase);
   EfiConvertPointer (0x0, (VOID **) &mVariableModuleGlobal->VariableGlobal.HobVariableBase);
   EfiConvertPointer (0x0, (VOID **) &mVariableModuleGlobal);
@@ -528,18 +552,26 @@ ProtocolIsVariablePolicyEnabled (
 EFI_STATUS
 EFIAPI
 VariableServiceInitialize (
   IN EFI_HANDLE                         ImageHandle,
   IN EFI_SYSTEM_TABLE                   *SystemTable
   )
 {
+  VOID                                  *Hob;
+  FVB_ADDRESS_LIST_PTR                  *FvbAddressListPtrHobData;
   EFI_STATUS                            Status;
   EFI_EVENT                             ReadyToBootEvent;
   EFI_EVENT                             EndOfDxeEvent;
 
+  Hob = GetFirstGuidHob (&gEdkiiFvbAddressListGuid);
+  ASSERT (Hob != NULL);
+  FvbAddressListPtrHobData = GET_GUID_HOB_DATA (Hob);
+  mFvbAddressList = (VOID*)(UINTN)*FvbAddressListPtrHobData;
+  ASSERT (mFvbAddressList->Signature == FVB_ADDRESS_LIST_SIGNATURE);
+
   Status = VariableCommonInitialize ();
   ASSERT_EFI_ERROR (Status);
 
   Status = gBS->InstallMultipleProtocolInterfaces (
                   &mHandle,
                   &gEdkiiVariableLockProtocolGuid,
                   &mVariableLock,
-- 
2.19.1.3.g30247aa5d201


  parent reply	other threads:[~2021-03-11 22:25 UTC|newest]

Thread overview: 13+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2021-03-10  8:04 Conflicting virtual addresses causing Runtime Services issues Jon Nettleton
2021-03-10 14:52 ` [edk2-devel] " Laszlo Ersek
2021-03-11  6:53   ` Jon Nettleton
     [not found]   ` <166B374585A9D8FC.18699@groups.io>
2021-03-11  9:48     ` Jon Nettleton
2021-03-11 14:50       ` Laszlo Ersek
2021-03-11 15:49         ` Jon Nettleton
2021-03-11 22:25         ` Laszlo Ersek [this message]
2021-03-11 22:39           ` Ard Biesheuvel
2021-03-12  3:01             ` Jon Nettleton
     [not found]             ` <166B792D1514133B.31346@groups.io>
2021-03-12  5:59               ` Jon Nettleton
2021-03-12 19:30                 ` Laszlo Ersek
2021-03-12 19:52                 ` Ard Biesheuvel
2021-03-12 19:17             ` Laszlo Ersek

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-list from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=4841241f-fc6d-6185-efe6-ed9a536534dd@redhat.com \
    --to=devel@edk2.groups.io \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox