* 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
* 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
* 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-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
* 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
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