* Conflicting virtual addresses causing Runtime Services issues @ 2021-03-10 8:04 Jon Nettleton 2021-03-10 14:52 ` [edk2-devel] " Laszlo Ersek 0 siblings, 1 reply; 13+ messages in thread From: Jon Nettleton @ 2021-03-10 8:04 UTC (permalink / raw) To: devel I am debugging a failure that I am seeing while using the HoneyComb's spi-nor flash for runtime variable storage. I am hoping someone on the list can give me some insight as what may be the problem. The problem showed up when we switched the MMIO region for the fspi flash device to be marked as non executable. reading variables is fine, however writes began throwing an error. [ 556.709828] Unable to handle kernel execute from non-executable memory at virtual address 00000000206a3968 I have patched the kernel and removed the X86 requirement and enabled the sysfs runtime mappings kernel config so I can get an easy view of the mappings the kernel carries for runtime services. I then track that virtual address to the MMIO region of nor flash, which makes sense that region is marked as non executable. The question is why is code being executed from this address range attribute :::::::::::::: 0x8000000000000001 :::::::::::::: num_pages :::::::::::::: 0x40 :::::::::::::: phys_addr :::::::::::::: 0x20500000 :::::::::::::: type :::::::::::::: 0xb :::::::::::::: virt_addr :::::::::::::: 0x20680000 So then I patched the PL011 serial driver to be able to log to the console in runtime and I track down the access to Status = Fvb->GetPhysicalAddress(Fvb, &FvVolHdr); in UpdateVariableStore(). What I don't understand is why EfiConvertPointer is mapping that pointer into the Virtual address space occupied by the runtime mmio of the flash. The pointer is being properly remapped. Here are the pointer addresses in EFI and Kernel Runtime EFI: UpdateVariableStore:156 ECE33968 FvbGetPhysicalAddress(BaseAddress=0x20000000) KERNEL: UpdateVariableStore:156 206A3968 [ 556.709828] Unable to handle kernel execute from non-executable memory at virtual address 00000000206a3968 Any insight that anyone could provide would be much appreciated. Thanks, Jon ^ permalink raw reply [flat|nested] 13+ messages in thread
* Re: [edk2-devel] Conflicting virtual addresses causing Runtime Services issues 2021-03-10 8:04 Conflicting virtual addresses causing Runtime Services issues Jon Nettleton @ 2021-03-10 14:52 ` Laszlo Ersek 2021-03-11 6:53 ` Jon Nettleton [not found] ` <166B374585A9D8FC.18699@groups.io> 0 siblings, 2 replies; 13+ messages in thread From: Laszlo Ersek @ 2021-03-10 14:52 UTC (permalink / raw) To: devel, jon On 03/10/21 09:04, Jon Nettleton wrote: > I am debugging a failure that I am seeing while using the HoneyComb's > spi-nor flash for runtime variable storage. I am hoping someone on > the list can give me some insight as what may be the problem. > > The problem showed up when we switched the MMIO region for the fspi > flash device to be marked as non executable. reading variables is > fine, however writes began throwing an error. > > [ 556.709828] Unable to handle kernel execute from non-executable > memory at virtual address 00000000206a3968 > > I have patched the kernel and removed the X86 requirement and enabled > the sysfs runtime mappings kernel config so I can get an easy view of > the mappings the kernel carries for runtime services. I then track > that virtual address to the MMIO region of nor flash, which makes > sense that region is marked as non executable. The question is why is > code being executed from this address range > > attribute > :::::::::::::: > 0x8000000000000001 > :::::::::::::: > num_pages > :::::::::::::: > 0x40 > :::::::::::::: > phys_addr > :::::::::::::: > 0x20500000 > :::::::::::::: > type > :::::::::::::: > 0xb > :::::::::::::: > virt_addr > :::::::::::::: > 0x20680000 > > So then I patched the PL011 serial driver to be able to log to the > console in runtime and I track down the access to Status = > Fvb->GetPhysicalAddress(Fvb, &FvVolHdr); in UpdateVariableStore(). > What I don't understand is why EfiConvertPointer is mapping that > pointer into the Virtual address space occupied by the runtime mmio of > the flash. The pointer is being properly remapped. Here are the > pointer addresses in EFI and Kernel Runtime > > EFI: > UpdateVariableStore:156 ECE33968 > FvbGetPhysicalAddress(BaseAddress=0x20000000) > > KERNEL: > UpdateVariableStore:156 206A3968 > [ 556.709828] Unable to handle kernel execute from non-executable > memory at virtual address 00000000206a3968 > > Any insight that anyone could provide would be much appreciated. Your platform appears misconfigured -- the flash MMIO range appears to overlap runtime services code even before SetVirtualAddressMap. The virtual address conflict is likely the result of the original physical address conflict. After the virtual address updates, the EfiMemoryMappedIO (0xb) type range is mapped at virtual address range [0x20680000, 0x206C0000). The GetPhysicalAddress function seems to be located at offset 0x23968 in that range (with 0x1C698 bytes to go in the range). That's inexplicable. What is the physical base address of the flash? What are the PCDs used in "ArmPlatformPkg/Drivers/NorFlashDxe/NorFlashFvb.c"? Laszlo ^ permalink raw reply [flat|nested] 13+ messages in thread
* Re: [edk2-devel] Conflicting virtual addresses causing Runtime Services issues 2021-03-10 14:52 ` [edk2-devel] " Laszlo Ersek @ 2021-03-11 6:53 ` Jon Nettleton [not found] ` <166B374585A9D8FC.18699@groups.io> 1 sibling, 0 replies; 13+ messages in thread From: Jon Nettleton @ 2021-03-11 6:53 UTC (permalink / raw) To: Laszlo Ersek; +Cc: devel On Wed, Mar 10, 2021 at 3:52 PM Laszlo Ersek <lersek@redhat.com> wrote: > > On 03/10/21 09:04, Jon Nettleton wrote: > > I am debugging a failure that I am seeing while using the HoneyComb's > > spi-nor flash for runtime variable storage. I am hoping someone on > > the list can give me some insight as what may be the problem. > > > > The problem showed up when we switched the MMIO region for the fspi > > flash device to be marked as non executable. reading variables is > > fine, however writes began throwing an error. > > > > [ 556.709828] Unable to handle kernel execute from non-executable > > memory at virtual address 00000000206a3968 > > > > I have patched the kernel and removed the X86 requirement and enabled > > the sysfs runtime mappings kernel config so I can get an easy view of > > the mappings the kernel carries for runtime services. I then track > > that virtual address to the MMIO region of nor flash, which makes > > sense that region is marked as non executable. The question is why is > > code being executed from this address range > > > > attribute > > :::::::::::::: > > 0x8000000000000001 > > :::::::::::::: > > num_pages > > :::::::::::::: > > 0x40 > > :::::::::::::: > > phys_addr > > :::::::::::::: > > 0x20500000 > > :::::::::::::: > > type > > :::::::::::::: > > 0xb > > :::::::::::::: > > virt_addr > > :::::::::::::: > > 0x20680000 > > > > So then I patched the PL011 serial driver to be able to log to the > > console in runtime and I track down the access to Status = > > Fvb->GetPhysicalAddress(Fvb, &FvVolHdr); in UpdateVariableStore(). > > What I don't understand is why EfiConvertPointer is mapping that > > pointer into the Virtual address space occupied by the runtime mmio of > > the flash. The pointer is being properly remapped. Here are the > > pointer addresses in EFI and Kernel Runtime > > > > EFI: > > UpdateVariableStore:156 ECE33968 > > FvbGetPhysicalAddress(BaseAddress=0x20000000) > > > > KERNEL: > > UpdateVariableStore:156 206A3968 > > [ 556.709828] Unable to handle kernel execute from non-executable > > memory at virtual address 00000000206a3968 > > > > Any insight that anyone could provide would be much appreciated. > > Your platform appears misconfigured -- the flash MMIO range appears to > overlap runtime services code even before SetVirtualAddressMap. The > virtual address conflict is likely the result of the original physical > address conflict. There is no physical address conflict. Running in physical mode the pointer for GetPhysicalAddress is at 0xECE33968 and the MMIO physical region is 0x20000000 - 0x2FFFFFFF. Shown as the BaseAddress above. Obviously I don't use that full MMIO region because our flash is not 256MB. > > After the virtual address updates, the EfiMemoryMappedIO (0xb) type > range is mapped at virtual address range [0x20680000, 0x206C0000). The > GetPhysicalAddress function seems to be located at offset 0x23968 in > that range (with 0x1C698 bytes to go in the range). That's inexplicable. Yes, exactly why I am reaching out. > > What is the physical base address of the flash? What are the PCDs used > in "ArmPlatformPkg/Drivers/NorFlashDxe/NorFlashFvb.c"? We don't use the ArmPlatformPkg The code is available here. https://github.com/SolidRun/edk2-platforms/tree/LX2160_UEFI_ACPI_EAR3-lx2160cex7/Silicon/NXP/Drivers/SpiNorFlashDxe Thanks > > Laszlo > ^ permalink raw reply [flat|nested] 13+ messages in thread
[parent not found: <166B374585A9D8FC.18699@groups.io>]
* Re: [edk2-devel] Conflicting virtual addresses causing Runtime Services issues [not found] ` <166B374585A9D8FC.18699@groups.io> @ 2021-03-11 9:48 ` Jon Nettleton 2021-03-11 14:50 ` Laszlo Ersek 0 siblings, 1 reply; 13+ messages in thread From: Jon Nettleton @ 2021-03-11 9:48 UTC (permalink / raw) To: devel, Jon Nettleton; +Cc: Laszlo Ersek On Thu, Mar 11, 2021 at 7:54 AM Jon Nettleton via groups.io <jon=solid-run.com@groups.io> wrote: > > On Wed, Mar 10, 2021 at 3:52 PM Laszlo Ersek <lersek@redhat.com> wrote: > > > > On 03/10/21 09:04, Jon Nettleton wrote: > > > I am debugging a failure that I am seeing while using the HoneyComb's > > > spi-nor flash for runtime variable storage. I am hoping someone on > > > the list can give me some insight as what may be the problem. > > > > > > The problem showed up when we switched the MMIO region for the fspi > > > flash device to be marked as non executable. reading variables is > > > fine, however writes began throwing an error. > > > > > > [ 556.709828] Unable to handle kernel execute from non-executable > > > memory at virtual address 00000000206a3968 > > > > > > I have patched the kernel and removed the X86 requirement and enabled > > > the sysfs runtime mappings kernel config so I can get an easy view of > > > the mappings the kernel carries for runtime services. I then track > > > that virtual address to the MMIO region of nor flash, which makes > > > sense that region is marked as non executable. The question is why is > > > code being executed from this address range > > > > > > attribute > > > :::::::::::::: > > > 0x8000000000000001 > > > :::::::::::::: > > > num_pages > > > :::::::::::::: > > > 0x40 > > > :::::::::::::: > > > phys_addr > > > :::::::::::::: > > > 0x20500000 > > > :::::::::::::: > > > type > > > :::::::::::::: > > > 0xb > > > :::::::::::::: > > > virt_addr > > > :::::::::::::: > > > 0x20680000 > > > > > > So then I patched the PL011 serial driver to be able to log to the > > > console in runtime and I track down the access to Status = > > > Fvb->GetPhysicalAddress(Fvb, &FvVolHdr); in UpdateVariableStore(). > > > What I don't understand is why EfiConvertPointer is mapping that > > > pointer into the Virtual address space occupied by the runtime mmio of > > > the flash. The pointer is being properly remapped. Here are the > > > pointer addresses in EFI and Kernel Runtime > > > > > > EFI: > > > UpdateVariableStore:156 ECE33968 > > > FvbGetPhysicalAddress(BaseAddress=0x20000000) > > > > > > KERNEL: > > > UpdateVariableStore:156 206A3968 > > > [ 556.709828] Unable to handle kernel execute from non-executable > > > memory at virtual address 00000000206a3968 > > > > > > Any insight that anyone could provide would be much appreciated. > > > > Your platform appears misconfigured -- the flash MMIO range appears to > > overlap runtime services code even before SetVirtualAddressMap. The > > virtual address conflict is likely the result of the original physical > > address conflict. > > There is no physical address conflict. Running in physical mode the pointer > for GetPhysicalAddress is at 0xECE33968 and the MMIO physical > region is 0x20000000 - 0x2FFFFFFF. Shown as the BaseAddress above. > Obviously I don't use that full MMIO region because our flash is not > 256MB. > > > > > After the virtual address updates, the EfiMemoryMappedIO (0xb) type > > range is mapped at virtual address range [0x20680000, 0x206C0000). The > > GetPhysicalAddress function seems to be located at offset 0x23968 in > > that range (with 0x1C698 bytes to go in the range). That's inexplicable. > > Yes, exactly why I am reaching out. > > > > > What is the physical base address of the flash? What are the PCDs used > > in "ArmPlatformPkg/Drivers/NorFlashDxe/NorFlashFvb.c"? > > We don't use the ArmPlatformPkg > > The code is available here. > https://github.com/SolidRun/edk2-platforms/tree/LX2160_UEFI_ACPI_EAR3-lx2160cex7/Silicon/NXP/Drivers/SpiNorFlashDxe > > Thanks > > Found the root of my problem and fixed it. As used from other devices they are relocating the individual pointer for each function in FvbProtocol of the DXE. This is done in ArmPlatformPkg/Drivers/NorFlashDxe/NorFlashFvb.c, as well as lots of vendor drivers. I was able to dump the addresses being used in RuntimeDriverConvertPointer and could locate the pointer to GetPhysicalAddress. Convert 0xEC8C1648:0xEC8B0000:0x203D0000 New virtual address is then 203E1648 which is fine. However then later down in the remapping I found Convert 0x203E1648:0x20000000:0x20800000 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. -Jon ^ permalink raw reply [flat|nested] 13+ messages in thread
* Re: [edk2-devel] Conflicting virtual addresses causing Runtime Services issues 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 0 siblings, 2 replies; 13+ messages in thread From: Laszlo Ersek @ 2021-03-11 14:50 UTC (permalink / raw) To: devel, jon; +Cc: Hao A Wu, Liming Gao On 03/11/21 10:48, Jon Nettleton wrote: > On Thu, Mar 11, 2021 at 7:54 AM Jon Nettleton via groups.io > <jon=solid-run.com@groups.io> wrote: >> >> On Wed, Mar 10, 2021 at 3:52 PM Laszlo Ersek <lersek@redhat.com> wrote: >>> >>> On 03/10/21 09:04, Jon Nettleton wrote: >>>> I am debugging a failure that I am seeing while using the HoneyComb's >>>> spi-nor flash for runtime variable storage. I am hoping someone on >>>> the list can give me some insight as what may be the problem. >>>> >>>> The problem showed up when we switched the MMIO region for the fspi >>>> flash device to be marked as non executable. reading variables is >>>> fine, however writes began throwing an error. >>>> >>>> [ 556.709828] Unable to handle kernel execute from non-executable >>>> memory at virtual address 00000000206a3968 >>>> >>>> I have patched the kernel and removed the X86 requirement and enabled >>>> the sysfs runtime mappings kernel config so I can get an easy view of >>>> the mappings the kernel carries for runtime services. I then track >>>> that virtual address to the MMIO region of nor flash, which makes >>>> sense that region is marked as non executable. The question is why is >>>> code being executed from this address range >>>> >>>> attribute >>>> :::::::::::::: >>>> 0x8000000000000001 >>>> :::::::::::::: >>>> num_pages >>>> :::::::::::::: >>>> 0x40 >>>> :::::::::::::: >>>> phys_addr >>>> :::::::::::::: >>>> 0x20500000 >>>> :::::::::::::: >>>> type >>>> :::::::::::::: >>>> 0xb >>>> :::::::::::::: >>>> virt_addr >>>> :::::::::::::: >>>> 0x20680000 >>>> >>>> So then I patched the PL011 serial driver to be able to log to the >>>> console in runtime and I track down the access to Status = >>>> Fvb->GetPhysicalAddress(Fvb, &FvVolHdr); in UpdateVariableStore(). >>>> What I don't understand is why EfiConvertPointer is mapping that >>>> pointer into the Virtual address space occupied by the runtime mmio of >>>> the flash. The pointer is being properly remapped. Here are the >>>> pointer addresses in EFI and Kernel Runtime >>>> >>>> EFI: >>>> UpdateVariableStore:156 ECE33968 >>>> FvbGetPhysicalAddress(BaseAddress=0x20000000) >>>> >>>> KERNEL: >>>> UpdateVariableStore:156 206A3968 >>>> [ 556.709828] Unable to handle kernel execute from non-executable >>>> memory at virtual address 00000000206a3968 >>>> >>>> Any insight that anyone could provide would be much appreciated. >>> >>> Your platform appears misconfigured -- the flash MMIO range appears to >>> overlap runtime services code even before SetVirtualAddressMap. The >>> virtual address conflict is likely the result of the original physical >>> address conflict. >> >> There is no physical address conflict. Running in physical mode the pointer >> for GetPhysicalAddress is at 0xECE33968 and the MMIO physical >> region is 0x20000000 - 0x2FFFFFFF. Shown as the BaseAddress above. >> Obviously I don't use that full MMIO region because our flash is not >> 256MB. >> >>> >>> After the virtual address updates, the EfiMemoryMappedIO (0xb) type >>> range is mapped at virtual address range [0x20680000, 0x206C0000). The >>> GetPhysicalAddress function seems to be located at offset 0x23968 in >>> that range (with 0x1C698 bytes to go in the range). That's inexplicable. >> >> Yes, exactly why I am reaching out. >> >>> >>> What is the physical base address of the flash? What are the PCDs used >>> in "ArmPlatformPkg/Drivers/NorFlashDxe/NorFlashFvb.c"? >> >> We don't use the ArmPlatformPkg >> >> The code is available here. >> https://github.com/SolidRun/edk2-platforms/tree/LX2160_UEFI_ACPI_EAR3-lx2160cex7/Silicon/NXP/Drivers/SpiNorFlashDxe >> >> Thanks >>> > > Found the root of my problem and fixed it. As used from other devices > they are relocating the individual pointer for each function in FvbProtocol > of the DXE. This is done in ArmPlatformPkg/Drivers/NorFlashDxe/NorFlashFvb.c, > as well as lots of vendor drivers. I was able to dump the addresses being used > in RuntimeDriverConvertPointer and could locate the pointer to > GetPhysicalAddress. > > Convert 0xEC8C1648:0xEC8B0000:0x203D0000 > New virtual address is then 203E1648 which is fine. > > However then later down in the remapping I found > Convert 0x203E1648:0x20000000:0x20800000 > > 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. So the issue (IIUC) on your platform is that the function pointers are doubly-converted, once by your flash driver (which is arguably the right thing), and another time by the variable driver. I recommend filing a bug about this, at <https://bugzilla.tianocore.org/>, against "VariableDxe.c". I suggest CC'ing the "UEFI Variable modules" reviewers on the BZ as well, from "Maintainers.txt" -- FWIW, I'm CC'ing Hao and Liming on this email, now. ... It feels as if the Platform Init specification should speak to the FVB[2] protocol's runtime aspects... But (as of v1.7), I don't see anything runtime-related in that section. In fact I've checked the whole PI v1.7 spec for "runtime", and the closest matches are under EFI_VARIABLE_WRITE_ARCH_PROTOCOL. FVB2 is not spelled out as a (potential) runtime dependency of EFI_VARIABLE_WRITE_ARCH_PROTOCOL. The only explicitly runtime protocol (unrelated to runtime services) is "Status Code Runtime Protocol". So what we're seeing here is an edk2-ism, I believe. I tried seeing if one of the "tour beyond BIOS" edk2 whitepapers said anything on FVB -- but the only reference I found (in "Implementing UEFI Authenticated Variables in SMM with EDKII", version 2) mentions only *SMM* FVB. https://github.com/tianocore/tianocore.github.io/wiki/EDK-II-white-papers I think we should have a TianoCore BZ about this, even if we don' change the code as a result -- which driver is responsible for updating the runtime FVB protocol member functions should be spelled out somewhere "searchable". ... 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"? Thanks Laszlo ^ permalink raw reply [flat|nested] 13+ messages in thread
* Re: [edk2-devel] Conflicting virtual addresses causing Runtime Services issues 2021-03-11 14:50 ` Laszlo Ersek @ 2021-03-11 15:49 ` Jon Nettleton 2021-03-11 22:25 ` Laszlo Ersek 1 sibling, 0 replies; 13+ messages in thread From: Jon Nettleton @ 2021-03-11 15:49 UTC (permalink / raw) To: devel, Laszlo Ersek; +Cc: Hao A Wu, Liming Gao On Thu, Mar 11, 2021 at 3:51 PM Laszlo Ersek <lersek@redhat.com> wrote: > > On 03/11/21 10:48, Jon Nettleton wrote: > > On Thu, Mar 11, 2021 at 7:54 AM Jon Nettleton via groups.io > > <jon=solid-run.com@groups.io> wrote: > >> > >> On Wed, Mar 10, 2021 at 3:52 PM Laszlo Ersek <lersek@redhat.com> wrote: > >>> > >>> On 03/10/21 09:04, Jon Nettleton wrote: > >>>> I am debugging a failure that I am seeing while using the HoneyComb's > >>>> spi-nor flash for runtime variable storage. I am hoping someone on > >>>> the list can give me some insight as what may be the problem. > >>>> > >>>> The problem showed up when we switched the MMIO region for the fspi > >>>> flash device to be marked as non executable. reading variables is > >>>> fine, however writes began throwing an error. > >>>> > >>>> [ 556.709828] Unable to handle kernel execute from non-executable > >>>> memory at virtual address 00000000206a3968 > >>>> > >>>> I have patched the kernel and removed the X86 requirement and enabled > >>>> the sysfs runtime mappings kernel config so I can get an easy view of > >>>> the mappings the kernel carries for runtime services. I then track > >>>> that virtual address to the MMIO region of nor flash, which makes > >>>> sense that region is marked as non executable. The question is why is > >>>> code being executed from this address range > >>>> > >>>> attribute > >>>> :::::::::::::: > >>>> 0x8000000000000001 > >>>> :::::::::::::: > >>>> num_pages > >>>> :::::::::::::: > >>>> 0x40 > >>>> :::::::::::::: > >>>> phys_addr > >>>> :::::::::::::: > >>>> 0x20500000 > >>>> :::::::::::::: > >>>> type > >>>> :::::::::::::: > >>>> 0xb > >>>> :::::::::::::: > >>>> virt_addr > >>>> :::::::::::::: > >>>> 0x20680000 > >>>> > >>>> So then I patched the PL011 serial driver to be able to log to the > >>>> console in runtime and I track down the access to Status = > >>>> Fvb->GetPhysicalAddress(Fvb, &FvVolHdr); in UpdateVariableStore(). > >>>> What I don't understand is why EfiConvertPointer is mapping that > >>>> pointer into the Virtual address space occupied by the runtime mmio of > >>>> the flash. The pointer is being properly remapped. Here are the > >>>> pointer addresses in EFI and Kernel Runtime > >>>> > >>>> EFI: > >>>> UpdateVariableStore:156 ECE33968 > >>>> FvbGetPhysicalAddress(BaseAddress=0x20000000) > >>>> > >>>> KERNEL: > >>>> UpdateVariableStore:156 206A3968 > >>>> [ 556.709828] Unable to handle kernel execute from non-executable > >>>> memory at virtual address 00000000206a3968 > >>>> > >>>> Any insight that anyone could provide would be much appreciated. > >>> > >>> Your platform appears misconfigured -- the flash MMIO range appears to > >>> overlap runtime services code even before SetVirtualAddressMap. The > >>> virtual address conflict is likely the result of the original physical > >>> address conflict. > >> > >> There is no physical address conflict. Running in physical mode the pointer > >> for GetPhysicalAddress is at 0xECE33968 and the MMIO physical > >> region is 0x20000000 - 0x2FFFFFFF. Shown as the BaseAddress above. > >> Obviously I don't use that full MMIO region because our flash is not > >> 256MB. > >> > >>> > >>> After the virtual address updates, the EfiMemoryMappedIO (0xb) type > >>> range is mapped at virtual address range [0x20680000, 0x206C0000). The > >>> GetPhysicalAddress function seems to be located at offset 0x23968 in > >>> that range (with 0x1C698 bytes to go in the range). That's inexplicable. > >> > >> Yes, exactly why I am reaching out. > >> > >>> > >>> What is the physical base address of the flash? What are the PCDs used > >>> in "ArmPlatformPkg/Drivers/NorFlashDxe/NorFlashFvb.c"? > >> > >> We don't use the ArmPlatformPkg > >> > >> The code is available here. > >> https://github.com/SolidRun/edk2-platforms/tree/LX2160_UEFI_ACPI_EAR3-lx2160cex7/Silicon/NXP/Drivers/SpiNorFlashDxe > >> > >> Thanks > >>> > > > > Found the root of my problem and fixed it. As used from other devices > > they are relocating the individual pointer for each function in FvbProtocol > > of the DXE. This is done in ArmPlatformPkg/Drivers/NorFlashDxe/NorFlashFvb.c, > > as well as lots of vendor drivers. I was able to dump the addresses being used > > in RuntimeDriverConvertPointer and could locate the pointer to > > GetPhysicalAddress. > > > > Convert 0xEC8C1648:0xEC8B0000:0x203D0000 > > New virtual address is then 203E1648 which is fine. > > > > However then later down in the remapping I found > > Convert 0x203E1648:0x20000000:0x20800000 > > > > 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. > > So the issue (IIUC) on your platform is that the function pointers are > doubly-converted, once by your flash driver (which is arguably the right > thing), and another time by the variable driver. Correct, that seems to be what is happening > > I recommend filing a bug about this, at > <https://bugzilla.tianocore.org/>, against "VariableDxe.c". Sure > > I suggest CC'ing the "UEFI Variable modules" reviewers on the BZ as > well, from "Maintainers.txt" -- FWIW, I'm CC'ing Hao and Liming on this > email, now. > > ... It feels as if the Platform Init specification should speak to the > FVB[2] protocol's runtime aspects... But (as of v1.7), I don't see > anything runtime-related in that section. In fact I've checked the whole > PI v1.7 spec for "runtime", and the closest matches are under > EFI_VARIABLE_WRITE_ARCH_PROTOCOL. FVB2 is not spelled out as a > (potential) runtime dependency of EFI_VARIABLE_WRITE_ARCH_PROTOCOL. The > only explicitly runtime protocol (unrelated to runtime services) is > "Status Code Runtime Protocol". > > So what we're seeing here is an edk2-ism, I believe. I checked the commits and the code that does that conversion is quite old. > > I tried seeing if one of the "tour beyond BIOS" edk2 whitepapers said > anything on FVB -- but the only reference I found (in "Implementing > UEFI Authenticated Variables in SMM with EDKII", version 2) mentions > only *SMM* FVB. > > https://github.com/tianocore/tianocore.github.io/wiki/EDK-II-white-papers > > I think we should have a TianoCore BZ about this, even if we don' change > the code as a result -- which driver is responsible for updating the > runtime FVB protocol member functions should be spelled out somewhere > "searchable". > > ... 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"? I believe all the drivers that are doing the conversion are getting double mappings. It just so happens our second mapping got poked into an area that we had marked non-executable which started the debugging and led to the issue. > > Thanks > Laszlo > > > > > > ^ permalink raw reply [flat|nested] 13+ messages in thread
* Re: [edk2-devel] Conflicting virtual addresses causing Runtime Services issues 2021-03-11 14:50 ` Laszlo Ersek 2021-03-11 15:49 ` Jon Nettleton @ 2021-03-11 22:25 ` Laszlo Ersek 2021-03-11 22:39 ` Ard Biesheuvel 1 sibling, 1 reply; 13+ messages in thread From: Laszlo Ersek @ 2021-03-11 22:25 UTC (permalink / raw) To: devel, jon Cc: Hao A Wu, Liming Gao, Ard Biesheuvel (TianoCore), Leif Lindholm (Nuvia address) [-- 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 ^ permalink raw reply related [flat|nested] 13+ messages in thread
* Re: [edk2-devel] Conflicting virtual addresses causing Runtime Services issues 2021-03-11 22:25 ` Laszlo Ersek @ 2021-03-11 22:39 ` Ard Biesheuvel 2021-03-12 3:01 ` Jon Nettleton ` (2 more replies) 0 siblings, 3 replies; 13+ messages in thread From: Ard Biesheuvel @ 2021-03-11 22:39 UTC (permalink / raw) To: Laszlo Ersek Cc: devel, jon, Hao A Wu, Liming Gao, Ard Biesheuvel (TianoCore), Leif Lindholm (Nuvia address) On Thu, 11 Mar 2021 at 23:25, Laszlo Ersek <lersek@redhat.com> wrote: > > 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 for inviting me to this party! So the tl;dr here is that some points get converted twice, which usually is not a problem because the virtual address resulting from the conversion is rarely mistaken for a physical address living in a EFI_MEMORY_RUNTIME region. So I agree with Laszlo's assertion that the consumer of a protocol has no business updating its protocol pointers, so this should definitely be fixed in the core VariableRuntime driver. However, given the typical nature of the variable stack, i.e., a platform specfic NOR flash driver combined with the generic FTW and variable drivers, doing so would likely break many out of tree platforms where the NOR flash driver does not bother to update its pointers at all. ^ permalink raw reply [flat|nested] 13+ messages in thread
* Re: [edk2-devel] Conflicting virtual addresses causing Runtime Services issues 2021-03-11 22:39 ` Ard Biesheuvel @ 2021-03-12 3:01 ` Jon Nettleton [not found] ` <166B792D1514133B.31346@groups.io> 2021-03-12 19:17 ` Laszlo Ersek 2 siblings, 0 replies; 13+ messages in thread From: Jon Nettleton @ 2021-03-12 3:01 UTC (permalink / raw) To: Ard Biesheuvel Cc: Laszlo Ersek, devel, Hao A Wu, Liming Gao, Ard Biesheuvel (TianoCore), Leif Lindholm (Nuvia address) On Thu, Mar 11, 2021 at 11:39 PM Ard Biesheuvel <ardb@kernel.org> wrote: > > On Thu, 11 Mar 2021 at 23:25, Laszlo Ersek <lersek@redhat.com> wrote: > > > > 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 for inviting me to this party! > > So the tl;dr here is that some points get converted twice, which > usually is not a problem because the virtual address resulting from > the conversion is rarely mistaken for a physical address living in a > EFI_MEMORY_RUNTIME region. > > So I agree with Laszlo's assertion that the consumer of a protocol has > no business updating its protocol pointers, so this should definitely > be fixed in the core VariableRuntime driver. However, given the > typical nature of the variable stack, i.e., a platform specfic NOR > flash driver combined with the generic FTW and variable drivers, doing > so would likely break many out of tree platforms where the NOR flash > driver does not bother to update its pointers at all. Not ideal, but we could add a flag to Runtime Services and let the platform specify if it is remapping the FVB. This would allow us to not break legacy drivers, but still easily let properly designed platforms bypass this behaviour. As time progressed this could be used to for a deprecation warning, until it became the default handling of FVB pointer conversion. ^ permalink raw reply [flat|nested] 13+ messages in thread
[parent not found: <166B792D1514133B.31346@groups.io>]
* Re: [edk2-devel] Conflicting virtual addresses causing Runtime Services issues [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 0 siblings, 2 replies; 13+ messages in thread From: Jon Nettleton @ 2021-03-12 5:59 UTC (permalink / raw) To: devel, Jon Nettleton Cc: Ard Biesheuvel, Laszlo Ersek, Hao A Wu, Liming Gao, Ard Biesheuvel (TianoCore), Leif Lindholm (Nuvia address) [-- Attachment #1: Type: text/plain, Size: 9627 bytes --] On Fri, Mar 12, 2021 at 4:02 AM Jon Nettleton via groups.io <jon=solid-run.com@groups.io> wrote: > > On Thu, Mar 11, 2021 at 11:39 PM Ard Biesheuvel <ardb@kernel.org> wrote: > > > > On Thu, 11 Mar 2021 at 23:25, Laszlo Ersek <lersek@redhat.com> wrote: > > > > > > 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 for inviting me to this party! > > > > So the tl;dr here is that some points get converted twice, which > > usually is not a problem because the virtual address resulting from > > the conversion is rarely mistaken for a physical address living in a > > EFI_MEMORY_RUNTIME region. > > > > So I agree with Laszlo's assertion that the consumer of a protocol has > > no business updating its protocol pointers, so this should definitely > > be fixed in the core VariableRuntime driver. However, given the > > typical nature of the variable stack, i.e., a platform specfic NOR > > flash driver combined with the generic FTW and variable drivers, doing > > so would likely break many out of tree platforms where the NOR flash > > driver does not bother to update its pointers at all. > > Not ideal, but we could add a flag to Runtime Services and let the platform > specify if it is remapping the FVB. This would allow us to not break legacy > drivers, but still easily let properly designed platforms bypass this > behaviour. As time progressed this could be used to for a deprecation > warning, until it became the default handling of FVB pointer conversion. > Something like the attached patch possibly? [-- Attachment #2: 0001-MdeModulePkg-Variable-VariableRuntimeDxe-Add-Bool-Pc.patch --] [-- Type: text/x-patch, Size: 4931 bytes --] From 126bd8bebf24e0269696f22a282a4b0340d9923b Mon Sep 17 00:00:00 2001 From: Jon Nettleton <jon@solid-run.com> Date: Fri, 12 Mar 2021 06:52:56 +0100 Subject: [PATCH] MdeModulePkg/Variable/VariableRuntimeDxe: Add Bool PcdDriverConvertsFvbFuncPointers VariableRuntimeDxe is unconditionally converting the pointers to the Fvb protocol functions even if the platform drivers are already converting the pointers themselves. This leads to a double pointer conversion and can cause unexpected runtime behaviour depending on the memory layout of the platform. Since we don't want to break legacy and out of tree platforms we add a Bool flag that defaults to the existing behaviour but allows platforms to only use the pointer conversion being done in their driver implementation that produces the FVB Protocol. Signed-off-by: Jon Nettleton <jon@solid-run.com> --- MdeModulePkg/MdeModulePkg.dec | 6 ++++++ .../Universal/Variable/RuntimeDxe/VariableDxe.c | 16 +++++++++------- .../Variable/RuntimeDxe/VariableRuntimeDxe.inf | 1 + 3 files changed, 16 insertions(+), 7 deletions(-) diff --git a/MdeModulePkg/MdeModulePkg.dec b/MdeModulePkg/MdeModulePkg.dec index 1483955110..07b84b1837 100644 --- a/MdeModulePkg/MdeModulePkg.dec +++ b/MdeModulePkg/MdeModulePkg.dec @@ -1130,6 +1130,12 @@ # @Prompt Reclaim variable space at EndOfDxe. gEfiMdeModulePkgTokenSpaceGuid.PcdReclaimVariableSpaceAtEndOfDxe|FALSE|BOOLEAN|0x30000008 + ## Driver converts FVB function pointers.<BR><BR> + # The value is FALSE as default to retain the current behaviour and retain compatibility with out of tree platorms.<BR> + # If the value is set to TRUE, variable driver does not convert the FVB function pointers.<BR> + # @Prompt Platform driver converts FVB pointers. + gEfiMdeModulePkgTokenSpaceGuid.PcdDriverConvertsFvbFuncPointers|FALSE|BOOLEAN|0x3000000b + ## The size of volatile buffer. This buffer is used to store VOLATILE attribute variables. # @Prompt Variable storage size. gEfiMdeModulePkgTokenSpaceGuid.PcdVariableStoreSize|0x10000|UINT32|0x30000005 diff --git a/MdeModulePkg/Universal/Variable/RuntimeDxe/VariableDxe.c b/MdeModulePkg/Universal/Variable/RuntimeDxe/VariableDxe.c index 0fca0bb2a9..ede2a66682 100644 --- a/MdeModulePkg/Universal/Variable/RuntimeDxe/VariableDxe.c +++ b/MdeModulePkg/Universal/Variable/RuntimeDxe/VariableDxe.c @@ -247,13 +247,15 @@ VariableClassAddressChangeEvent ( UINTN Index; if (mVariableModuleGlobal->FvbInstance != NULL) { - EfiConvertPointer (0x0, (VOID **) &mVariableModuleGlobal->FvbInstance->GetBlockSize); - EfiConvertPointer (0x0, (VOID **) &mVariableModuleGlobal->FvbInstance->GetPhysicalAddress); - 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); + if (!PcdGetBool (PcdDriverConvertsFvbFuncPointers)) { + EfiConvertPointer (0x0, (VOID **) &mVariableModuleGlobal->FvbInstance->GetBlockSize); + EfiConvertPointer (0x0, (VOID **) &mVariableModuleGlobal->FvbInstance->GetPhysicalAddress); + 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); } EfiConvertPointer (0x0, (VOID **) &mVariableModuleGlobal->PlatformLangCodes); diff --git a/MdeModulePkg/Universal/Variable/RuntimeDxe/VariableRuntimeDxe.inf b/MdeModulePkg/Universal/Variable/RuntimeDxe/VariableRuntimeDxe.inf index c9434df631..f3373f8137 100644 --- a/MdeModulePkg/Universal/Variable/RuntimeDxe/VariableRuntimeDxe.inf +++ b/MdeModulePkg/Universal/Variable/RuntimeDxe/VariableRuntimeDxe.inf @@ -137,6 +137,7 @@ gEfiMdeModulePkgTokenSpaceGuid.PcdMaxUserNvVariableSpaceSize ## CONSUMES gEfiMdeModulePkgTokenSpaceGuid.PcdBoottimeReservedNvVariableSpaceSize ## CONSUMES gEfiMdeModulePkgTokenSpaceGuid.PcdReclaimVariableSpaceAtEndOfDxe ## CONSUMES + gEfiMdeModulePkgTokenSpaceGuid.PcdDriverConvertsFvbFuncPointers ## CONSUMES gEfiMdeModulePkgTokenSpaceGuid.PcdEmuVariableNvModeEnable ## SOMETIMES_CONSUMES gEfiMdeModulePkgTokenSpaceGuid.PcdEmuVariableNvStoreReserved ## SOMETIMES_CONSUMES -- 2.27.0 ^ permalink raw reply related [flat|nested] 13+ messages in thread
* Re: [edk2-devel] Conflicting virtual addresses causing Runtime Services issues 2021-03-12 5:59 ` Jon Nettleton @ 2021-03-12 19:30 ` Laszlo Ersek 2021-03-12 19:52 ` Ard Biesheuvel 1 sibling, 0 replies; 13+ messages in thread From: Laszlo Ersek @ 2021-03-12 19:30 UTC (permalink / raw) To: Jon Nettleton, devel Cc: Ard Biesheuvel, Hao A Wu, Liming Gao, Ard Biesheuvel (TianoCore), Leif Lindholm (Nuvia address) On 03/12/21 06:59, Jon Nettleton wrote: > On Fri, Mar 12, 2021 at 4:02 AM Jon Nettleton via groups.io > <jon=solid-run.com@groups.io> wrote: >> On Thu, Mar 11, 2021 at 11:39 PM Ard Biesheuvel <ardb@kernel.org> wrote: >>> On Thu, 11 Mar 2021 at 23:25, Laszlo Ersek <lersek@redhat.com> wrote: >>>> 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 for inviting me to this party! >>> >>> So the tl;dr here is that some points get converted twice, which >>> usually is not a problem because the virtual address resulting from >>> the conversion is rarely mistaken for a physical address living in a >>> EFI_MEMORY_RUNTIME region. >>> >>> So I agree with Laszlo's assertion that the consumer of a protocol has >>> no business updating its protocol pointers, so this should definitely >>> be fixed in the core VariableRuntime driver. However, given the >>> typical nature of the variable stack, i.e., a platform specfic NOR >>> flash driver combined with the generic FTW and variable drivers, doing >>> so would likely break many out of tree platforms where the NOR flash >>> driver does not bother to update its pointers at all. >> Not ideal, but we could add a flag to Runtime Services and let the platform >> specify if it is remapping the FVB. This would allow us to not break legacy >> drivers, but still easily let properly designed platforms bypass this >> behaviour. As time progressed this could be used to for a deprecation >> warning, until it became the default handling of FVB pointer conversion. >> > Something like the attached patch possibly? > > > 0001-MdeModulePkg-Variable-VariableRuntimeDxe-Add-Bool-Pc.patch > > > From 126bd8bebf24e0269696f22a282a4b0340d9923b Mon Sep 17 00:00:00 2001 > From: Jon Nettleton <jon@solid-run.com> > Date: Fri, 12 Mar 2021 06:52:56 +0100 > Subject: [PATCH] MdeModulePkg/Variable/VariableRuntimeDxe: Add Bool > PcdDriverConvertsFvbFuncPointers > > VariableRuntimeDxe is unconditionally converting the pointers to the Fvb protocol > functions even if the platform drivers are already converting the pointers > themselves. This leads to a double pointer conversion and can cause unexpected > runtime behaviour depending on the memory layout of the platform. > > Since we don't want to break legacy and out of tree platforms we add a Bool flag > that defaults to the existing behaviour but allows platforms to only use the > pointer conversion being done in their driver implementation that produces the > FVB Protocol. > > Signed-off-by: Jon Nettleton <jon@solid-run.com> > --- > MdeModulePkg/MdeModulePkg.dec | 6 ++++++ > .../Universal/Variable/RuntimeDxe/VariableDxe.c | 16 +++++++++------- > .../Variable/RuntimeDxe/VariableRuntimeDxe.inf | 1 + > 3 files changed, 16 insertions(+), 7 deletions(-) > > diff --git a/MdeModulePkg/MdeModulePkg.dec b/MdeModulePkg/MdeModulePkg.dec > index 1483955110..07b84b1837 100644 > --- a/MdeModulePkg/MdeModulePkg.dec > +++ b/MdeModulePkg/MdeModulePkg.dec > @@ -1130,6 +1130,12 @@ > # @Prompt Reclaim variable space at EndOfDxe. > gEfiMdeModulePkgTokenSpaceGuid.PcdReclaimVariableSpaceAtEndOfDxe|FALSE|BOOLEAN|0x30000008 > > + ## Driver converts FVB function pointers.<BR><BR> > + # The value is FALSE as default to retain the current behaviour and retain compatibility with out of tree platorms.<BR> > + # If the value is set to TRUE, variable driver does not convert the FVB function pointers.<BR> > + # @Prompt Platform driver converts FVB pointers. > + gEfiMdeModulePkgTokenSpaceGuid.PcdDriverConvertsFvbFuncPointers|FALSE|BOOLEAN|0x3000000b > + > ## The size of volatile buffer. This buffer is used to store VOLATILE attribute variables. > # @Prompt Variable storage size. > gEfiMdeModulePkgTokenSpaceGuid.PcdVariableStoreSize|0x10000|UINT32|0x30000005 > diff --git a/MdeModulePkg/Universal/Variable/RuntimeDxe/VariableDxe.c b/MdeModulePkg/Universal/Variable/RuntimeDxe/VariableDxe.c > index 0fca0bb2a9..ede2a66682 100644 > --- a/MdeModulePkg/Universal/Variable/RuntimeDxe/VariableDxe.c > +++ b/MdeModulePkg/Universal/Variable/RuntimeDxe/VariableDxe.c > @@ -247,13 +247,15 @@ VariableClassAddressChangeEvent ( > UINTN Index; > > if (mVariableModuleGlobal->FvbInstance != NULL) { > - EfiConvertPointer (0x0, (VOID **) &mVariableModuleGlobal->FvbInstance->GetBlockSize); > - EfiConvertPointer (0x0, (VOID **) &mVariableModuleGlobal->FvbInstance->GetPhysicalAddress); > - 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); > + if (!PcdGetBool (PcdDriverConvertsFvbFuncPointers)) { > + EfiConvertPointer (0x0, (VOID **) &mVariableModuleGlobal->FvbInstance->GetBlockSize); > + EfiConvertPointer (0x0, (VOID **) &mVariableModuleGlobal->FvbInstance->GetPhysicalAddress); > + 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); > } > EfiConvertPointer (0x0, (VOID **) &mVariableModuleGlobal->PlatformLangCodes); > diff --git a/MdeModulePkg/Universal/Variable/RuntimeDxe/VariableRuntimeDxe.inf b/MdeModulePkg/Universal/Variable/RuntimeDxe/VariableRuntimeDxe.inf > index c9434df631..f3373f8137 100644 > --- a/MdeModulePkg/Universal/Variable/RuntimeDxe/VariableRuntimeDxe.inf > +++ b/MdeModulePkg/Universal/Variable/RuntimeDxe/VariableRuntimeDxe.inf > @@ -137,6 +137,7 @@ > gEfiMdeModulePkgTokenSpaceGuid.PcdMaxUserNvVariableSpaceSize ## CONSUMES > gEfiMdeModulePkgTokenSpaceGuid.PcdBoottimeReservedNvVariableSpaceSize ## CONSUMES > gEfiMdeModulePkgTokenSpaceGuid.PcdReclaimVariableSpaceAtEndOfDxe ## CONSUMES > + gEfiMdeModulePkgTokenSpaceGuid.PcdDriverConvertsFvbFuncPointers ## CONSUMES > gEfiMdeModulePkgTokenSpaceGuid.PcdEmuVariableNvModeEnable ## SOMETIMES_CONSUMES > gEfiMdeModulePkgTokenSpaceGuid.PcdEmuVariableNvStoreReserved ## SOMETIMES_CONSUMES > > -- 2.27.0 > (1) I suggest filing a TianoCore BZ for this, and then submitting the patch to the mailing list with git-send-email. Please don't forget to CC the Variable Driver reviewers / maintainers. official: https://github.com/tianocore/tianocore.github.io/wiki/EDK-II-Development-Process unofficial hints: https://github.com/tianocore/tianocore.github.io/wiki/Laszlo%27s-unkempt-git-guide-for-edk2-contributors-and-maintainers (2) I don't think removing the original condition "mVariableModuleGlobal->FvbInstance" is valid. That condition comes from commit 7cd69959463a ("MdeModulePkg Variable: Add emulated variable NV mode support", 2019-01-24). The condition should likely be *restricted* with the new condition. (3) Rather than a PCD, I wonder if we should add a new FVB attribute bit instead, to EFI_FVB_ATTRIBUTES_2, returned by FVB2.GetAttributes(). This would raise the topic to the PI spec's level. For an example, we have EFI_FVB2_WEAK_ALIGNMENT (0x80000000) -- from commit 3837e91c58d4 ("MdeModulePkg: Add support for weakly aligned FVs.", 2013-09-16) --, associated with <https://mantis.uefi.org/mantis/view.php?id=839>. Just an idea; it really needs to be commented upon by the maintainers. I don't "insist" that it be explained in the PI spec, just saying that it might help with the confusion. Thanks Laszlo ^ permalink raw reply [flat|nested] 13+ messages in thread
* Re: [edk2-devel] Conflicting virtual addresses causing Runtime Services issues 2021-03-12 5:59 ` Jon Nettleton 2021-03-12 19:30 ` Laszlo Ersek @ 2021-03-12 19:52 ` Ard Biesheuvel 1 sibling, 0 replies; 13+ messages in thread From: Ard Biesheuvel @ 2021-03-12 19:52 UTC (permalink / raw) To: Jon Nettleton Cc: devel, Laszlo Ersek, Hao A Wu, Liming Gao, Ard Biesheuvel (TianoCore), Leif Lindholm (Nuvia address) On Fri, 12 Mar 2021 at 07:00, Jon Nettleton <jon@solid-run.com> wrote: > > On Fri, Mar 12, 2021 at 4:02 AM Jon Nettleton via groups.io > <jon=solid-run.com@groups.io> wrote: > > > > On Thu, Mar 11, 2021 at 11:39 PM Ard Biesheuvel <ardb@kernel.org> wrote: > > > > > > On Thu, 11 Mar 2021 at 23:25, Laszlo Ersek <lersek@redhat.com> wrote: > > > > > > > > Adding Ard and Leif, comments below: > > > > ... > > > Thanks for inviting me to this party! > > > > > > So the tl;dr here is that some points get converted twice, which > > > usually is not a problem because the virtual address resulting from > > > the conversion is rarely mistaken for a physical address living in a > > > EFI_MEMORY_RUNTIME region. > > > > > > So I agree with Laszlo's assertion that the consumer of a protocol has > > > no business updating its protocol pointers, so this should definitely > > > be fixed in the core VariableRuntime driver. However, given the > > > typical nature of the variable stack, i.e., a platform specfic NOR > > > flash driver combined with the generic FTW and variable drivers, doing > > > so would likely break many out of tree platforms where the NOR flash > > > driver does not bother to update its pointers at all. > > > > Not ideal, but we could add a flag to Runtime Services and let the platform > > specify if it is remapping the FVB. This would allow us to not break legacy > > drivers, but still easily let properly designed platforms bypass this > > behaviour. As time progressed this could be used to for a deprecation > > warning, until it became the default handling of FVB pointer conversion. > > > Something like the attached patch possibly? Frankly, I don't think adding a PCD that does one or the other is the right way to go here. Instead, what we might do is: - record the original values of the pointers at ExitBootServices() time - don't touch the original values but the copies during SetVirtualAddressMap() - in each exposed RuntimeService(), do a runtime check whether the addresses in the protocol struct are equal to the converted copies, and if not, update them. This functionality should be enabled by default, but can be opted out of by a platform if we dedicate a PCD to it that defaults to enabled. At some point, we could add some kind of deprecation warning if the PCD is enabled, and have a path forward to actually retiring it completely. ^ permalink raw reply [flat|nested] 13+ messages in thread
* Re: [edk2-devel] Conflicting virtual addresses causing Runtime Services issues 2021-03-11 22:39 ` Ard Biesheuvel 2021-03-12 3:01 ` Jon Nettleton [not found] ` <166B792D1514133B.31346@groups.io> @ 2021-03-12 19:17 ` Laszlo Ersek 2 siblings, 0 replies; 13+ messages in thread From: Laszlo Ersek @ 2021-03-12 19:17 UTC (permalink / raw) To: Ard Biesheuvel Cc: devel, jon, Hao A Wu, Liming Gao, Ard Biesheuvel (TianoCore), Leif Lindholm (Nuvia address) On 03/11/21 23:39, Ard Biesheuvel wrote: > On Thu, 11 Mar 2021 at 23:25, Laszlo Ersek <lersek@redhat.com> wrote: >> >> 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 for inviting me to this party! > > So the tl;dr here is that some points get converted twice, which > usually is not a problem because the virtual address resulting from > the conversion is rarely mistaken for a physical address living in a > EFI_MEMORY_RUNTIME region. Ah, good point! Where I assumed that an identity mapping must have existed, from the OS's mappings, there's a much simpler explanation indeed: If the "physical address" that's being converted simply doesn't fall into a domain that's supposed to be runtime-mapped (per the "VirtualMap" parameter of SetVirtualAddressMap()), the ConvertPointer() call simply fails with EFI_NOT_FOUND, and the pointer is left intact. > So I agree with Laszlo's assertion that the consumer of a protocol has > no business updating its protocol pointers, so this should definitely > be fixed in the core VariableRuntime driver. However, given the > typical nature of the variable stack, i.e., a platform specfic NOR > flash driver combined with the generic FTW and variable drivers, doing > so would likely break many out of tree platforms where the NOR flash > driver does not bother to update its pointers at all. Yes, this is indeed the compatibility argument. Where I see a gray area though is the PI spec. I checked PI v1.7 yesterday (all occurrences of "runtime"), and FVB drivers / protocols are not required to be runtime drivers / protocol -- not even the *possibility* is raised. The variable write arch protocol / driver must be runtime, but how that may (or may not) translate to FVB is not mentioned, as far as I recall. FWIW, the variable driver bug goes back to historical commit 8a9e0b7274c69, dated 2009-03-09. The commit message is... obscure. Hmmm... look at related commit 00f3851372eb ("retire FvbServiceLib class in MdeModulePkg [...]", 2009-03-09). It looks like the ConvertPointer() stuff was originally there? "Firmeware Volume BLock Service Library". FvbServiceLib seems to have been a helper library for FVB drivers, and so it was in its right to offer pointer conversion services... FvbLibInitialize() was the constructor, and it registered FvbVirtualAddressChangeNotifyEvent(). FvbServiceLib was originally added in commit 677472aae492, dated 2008-10-25. I don't know why commit 8a9e0b7274c69 merged the pointer conversion into the variable driver; that seems to have been wrong. But... it's been with us for 12 years now :/ Thanks Laszlo ^ permalink raw reply [flat|nested] 13+ messages in thread
end of thread, other threads:[~2021-03-12 19:52 UTC | newest]
Thread overview: 13+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
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
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
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox