On 06/27/18 19:49, Brijesh Singh wrote:
>
>
> On 06/27/2018 11:59 AM, Laszlo Ersek wrote:
>> On 06/27/18 18:34, Brijesh Singh wrote:
>>> On 06/27/2018 07:54 AM, Laszlo Ersek wrote:
>>>> On 06/26/18 21:46, Brijesh Singh wrote:
>>
>>>>> After that, any access
>>>>> to the flash will end up going through the encryption engine. I did
>>>>> try
>>>>> hacking EDK2 to restore the C-bit
>>>>
>>>> (I continue to be annoyed that the memory encryption bit is not exposed
>>>> in the GCD memory space attributes explicitly.)
>>>>
>>>>> but that was not sufficient because UEFI
>>>>> runtime services are mapped as "encrypted" in OS page table
>>>>
>>>> What do you mean here? Runtime services *code* or runtime services
>>>> *data*? Code must obviously be remain encrypted (otherwise we cannot
>>>> execute it in SEV). Runtime Services Data should also be mapped as
>>>> encrypted (it is normal RAM that is not used for guest<->hypervisor
>>>> exchange).
>>>
>>> Sorry, I was meaning to say both the "code" and "data" are mapped as
>>> encrypted by the OS.
>>>
>>>>> hence we end up accessing the flash as encrypted when OS requests to
>>>>> update the variables.
>>>>
>>>> I don't understand the "hence" here; I don't see how the implication
>>>> follows. runtime services code and data should be encrypted. Runtime
>>>> MMIO should be un-encrypted.
>>>>
>>>> Ohh, wait, in MarkMemoryRangeForRuntimeAccess(), we use
>>>> "EfiGcdMemoryTypeSystemMemory". I don't have a clue why that is a good
>>>> idea. That should have been EfiGcdMemoryTypeMemoryMappedIo.
>>>
>>> Right, the memory is marked as 'system ram' and not 'mmio'.
>>> Just to experiment, I did try changing it to 'mmio' to see if OS will
>>> map this region as "unencrypted" but ovmf fails with below error
>>> message after changing it from systemRAM->mmio
>>>
>>> ConvertPages: failed to find range FFC00000 - FFFFFFFF
>>> ASSERT_EFI_ERROR (Status = Not Found)
>>> ASSERT [FvbServicesRuntimeDxe]
>>> /home/amd/workdir/upstream/edk2/OvmfPkg/QemuFlashFvbServicesRuntimeDxe/FwBlockServie.c(864):
>>>
>>> !EFI_ERROR (Status)
>>
>> This error occurs because (I think) you modified only the AddMemorySpace
>> call. If you change the GCD type on that, then please update the
>> subsequent AllocatePages as well, from EfiRuntimeServicesData to
>> EfiMemoryMappedIO.
>>
>
> Here is what I have.
>
> --- a/OvmfPkg/QemuFlashFvbServicesRuntimeDxe/FwBlockService.c
> +++ b/OvmfPkg/QemuFlashFvbServicesRuntimeDxe/FwBlockService.c
> @@ -847,7 +847,7 @@ MarkMemoryRangeForRuntimeAccess (
> );
>
> Status = gDS->AddMemorySpace (
> - EfiGcdMemoryTypeSystemMemory,
> + EfiGcdMemoryTypeMemoryMappedIo,
> BaseAddress,
> Length,
> EFI_MEMORY_UC | EFI_MEMORY_RUNTIME
> @@ -856,7 +856,7 @@ MarkMemoryRangeForRuntimeAccess (
>
> Status = gBS->AllocatePages (
> AllocateAddress,
> - EfiRuntimeServicesData,
> + EfiMemoryMappedIO,
> EFI_SIZE_TO_PAGES (Length),
> &BaseAddress
> );
>
> I am still getting the error assertion failure. I can debug to see what
> is going on.
Hmmm. Indeed, memory space added to GCD need not immediately show up in
the UEFI memory map, for the UEFI memory allocation services to allocate
from. IIRC, the PI spec says that *system memory* added like this *may*
show up immediately:
"""
If the memory range specified by BaseAddress and Length is of type
EfiGcdMemoryTypeSystemMemory or EfiGcdMemoryTypeMoreReliable, then the
memory range may be automatically allocated for use by the UEFI memory
services.
"""
and, indeed in edk2, it happens at once; but it doesn't apply to MMIO at
all.
So, can you replace the AllocatePages call with the following:
Status = gDS->AllocateMemorySpace (
EfiGcdAllocateAddress,
EfiGcdMemoryTypeMemoryMappedIo,
0, // Alignment
EFI_SIZE_TO_PAGES (Length),
&BaseAddress,
gImageHandle,
NULL // DeviceHandle
);
>> The spec says about the latter enum constant, "Used by system firmware
>> to request that a memory-mapped IO region be mapped by the OS to a
>> virtual address so it can be accessed by EFI runtime services." It seems
>> appropriate (and I'm a bit confused why we haven't used the MMIO GCD and
>> UEFI enum values for the memory type, all this time.)
>>
>>> Since this efi runtime data is mapped as C=1 by the OS, hence when OS
>>> asks efi to update the runtime variable we end up accessing the memory
>>> region with C=1 (runtime services are executed using OS pagetable).
>>
>> Indeed.
>>
>> (And, this is only a problem when SMM is not used, i.e. when the full
>> variable driver stack is non-SMM, just DXE. In the SMM case, the SMM
>> page tables are used, and the OS cannot interfere with that.)
>>
>
> Good point, I will try it and let you know. As you say since SMM uses
> UEFI page table
More correctly: SMM drivers use, at runtime as well, the page table in
SMRAM that was created by the firmware.
> hence after fixing FtwNotificationEvent(..) we should be
> good.
No, that's not precise -- FtwNotificationEvent() is not used in SMM *at
all*.
I checked that before I sent my previous email; namely,
FtwNotificationEvent() is in
"MdeModulePkg/Universal/Variable/RuntimeDxe/VariableDxe.c", which is not
built into the SMM variant of the variable driver.
Please see the variable driver stacks in OVMF:
- non-SMM:
OvmfPkg/QemuFlashFvbServicesRuntimeDxe/FvbServicesRuntimeDxe.inf
OvmfPkg/EmuVariableFvbRuntimeDxe/Fvb.inf
MdeModulePkg/Universal/FaultTolerantWriteDxe/FaultTolerantWriteDxe.inf
MdeModulePkg/Universal/Variable/RuntimeDxe/VariableRuntimeDxe.inf
In the non-SMM case, we have a normal runtime driver,
VariableRuntimeDxe.inf, that implements the variable service. It relies
on the FTW driver (another runtime driver) for the FTW protocol.
Finally, both the variable driver and the FTW driver rely on the FVB
(firmware volume block) protocol, which may come from the QEMU flash
driver, or from the emulation driver. Again, all of these are runtime
drivers. "VariableRuntimeDxe.inf" includes the "VariableDxe.c" file,
hence FtwNotificationEvent() is relevant.
- SMM:
OvmfPkg/QemuFlashFvbServicesRuntimeDxe/FvbServicesSmm.inf
MdeModulePkg/Universal/FaultTolerantWriteDxe/FaultTolerantWriteSmm.inf
MdeModulePkg/Universal/Variable/RuntimeDxe/VariableSmm.inf
MdeModulePkg/Universal/Variable/RuntimeDxe/VariableSmmRuntimeDxe.inf
In the SMM case, we have a *split* variable driver,
"VariableSmmRuntimeDxe.inf" (which is a normal runtime driver) and
"VariableSmm.inf" (a "traditional SMM" driver). The runtime half is
unprivileged and only formats a message for the privileged SMM half,
submits the message, and then parses the response. The rest of the
variable stack, namely the privileged half of the variable driver, and
the FTW driver, and the QEMU flash (FVB) driver, all stay within SMM --
they are traditional SMM drivers too.
"VariableSmmRuntimeDxe.inf" does not include "VariableDxe.c". Instead,
"VariableSmm.inf" (the privileged half) includes "VariableSmm.c", and
that source file waits for the *SMM* FVB protocol, in the
SmmFtwNotificationEvent() function. But, this notification function,
unlike the DXE counterpart FtwNotificationEvent(), does not care about
the GCD entry attributes -- because in SMM, separate (protected) page
tables are used anyway.
If you put FtwNotificationEvent() and SmmFtwNotificationEvent() side by
side, you'll see that they perform almost identical steps:
- "Ensure [SMM] FTW protocol is installed"
- "Find the proper [SMM] FVB protocol for variable"
- "Mark the variable storage region of the FLASH as RUNTIME" -- this is
DXE only!
- VariableWriteServiceInitialize
- "Install the Variable Write Architectural protocol"
(I skipped RecordSecureBootPolicyVarData(), because that always happens
in the DXE half. Anyway it's irrelevant here.)
Thus, my expectation is that the current issue manifests itself only if
you build OVMF without -D SMM_REQUIRE.
>> Anyway, in the pure DXE / runtime driver case, do you think a guest
>> kernel patch will be necessary too? Perhaps if you change the UEFI
>> memmap entry type (see AllocatePages above) to MMIO, then the guest
>> kernel could technically honor that.
>>
>
>
> Theoretically speaking, if we are able to make this memory region as
> mmio then OS should be able to map it with C=0.
Thanks
Laszlo
_______________________________________________
edk2-devel mailing list
edk2-devel@lists.01.org
https://lists.01.org/mailman/listinfo/edk2-devel
