Re: [PATCH 2/2] OvmfPkg/QemuFlashFvbServicesRuntimeDxe: Clear C-bit when SEV is active

[Brijesh Singh <brijesh.singh@amd.com>]

On 2/22/18 6:08 AM, Laszlo Ersek wrote:
> On 02/21/18 17:52, Brijesh Singh wrote:
>> Commit:24e4ad7 (OvmfPkg: Add AmdSevDxe driver) added a driver which runs
>> early in PEI phase and clears the C-bit from all MMIO regions (including
> s/PEI/DXE/
>
>
>> Qemu Flash). When SMM is enabled, we build two sets of page tables; first
>> page table is used when executing code in non SMM mode (SMM-less-pgtable)
>> and second page table is used when we are executing code in SMM mode
>> (SMM-pgtable).
>>
>> During boot time, AmdSevDxe driver clears the C-bit from the
>> SMM-less-pgtable. But when SMM is enabled, Qemu Flash services are used
>> from SMM mode.
>>
>> In this patch we explicitly clear the C-bit from Qemu flash MMIO range
>> before we probe the flash. When OVMF is built with SMM_REQUIRE then
>> call to initialize the flash services happen after the SMM-pgtable is
>> created and processor is serving the first SMI. At this time we will
>> have access to the SMM-pgtable.
> The problem statement is good (including the comment in the code).
>
> However, I would prefer if we could reflect the full AmdSevDxe logic to
> the SMM page tables. In other words, when -- or shortly after -- the SMM
> page tables are built, we should clear the C-bit in all those PTEs that
> cover known MMIO and as-yet NonExistent memory ranges. We already have a
> bunch of "mAddressEncMask" usage in PiSmmCpuDxeSmm.
>
> Can we investigate this a bit? If it turns out to be impossible, I guess
> I might be OK with this patch.

I will investigate this a bit. The reason why I didn't replicated full
AmdSevDxe logic is because I thought in SMM world we don't need to do
all those MMIO accesses etc but if its not the case then I agree we
should implement the full logic here.


>
> I have more comments:
>
>
>> Cc: Jordan Justen <jordan.l.justen@intel.com>
>> Cc: Laszlo Ersek <lersek@redhat.com>
>> Cc: Ard Biesheuvel <ard.biesheuvel@linaro.org>
>> Contributed-under: TianoCore Contribution Agreement 1.1
>> Signed-off-by: Brijesh Singh <brijesh.singh@amd.com>
>> ---
>>  OvmfPkg/QemuFlashFvbServicesRuntimeDxe/FvbServicesSmm.inf  |  1 +
>>  OvmfPkg/QemuFlashFvbServicesRuntimeDxe/FwBlockService.h    |  5 +++
>>  OvmfPkg/QemuFlashFvbServicesRuntimeDxe/FwBlockService.c    |  5 +++
>>  OvmfPkg/QemuFlashFvbServicesRuntimeDxe/FwBlockServiceDxe.c | 10 ++++++
>>  OvmfPkg/QemuFlashFvbServicesRuntimeDxe/FwBlockServiceSmm.c | 35 ++++++++++++++++++++
>>  5 files changed, 56 insertions(+)
>>
>> diff --git a/OvmfPkg/QemuFlashFvbServicesRuntimeDxe/FvbServicesSmm.inf b/OvmfPkg/QemuFlashFvbServicesRuntimeDxe/FvbServicesSmm.inf
>> index ba2d3679a46d..d365e27cbe59 100644
>> --- a/OvmfPkg/QemuFlashFvbServicesRuntimeDxe/FvbServicesSmm.inf
>> +++ b/OvmfPkg/QemuFlashFvbServicesRuntimeDxe/FvbServicesSmm.inf
>> @@ -53,6 +53,7 @@ [LibraryClasses]
>>    DevicePathLib
>>    DxeServicesTableLib
>>    MemoryAllocationLib
>> +  MemEncryptSevLib
>>    PcdLib
>>    SmmServicesTableLib
>>    UefiBootServicesTableLib
>> diff --git a/OvmfPkg/QemuFlashFvbServicesRuntimeDxe/FwBlockService.h b/OvmfPkg/QemuFlashFvbServicesRuntimeDxe/FwBlockService.h
>> index 1f9287b08769..704ed477ba14 100644
>> --- a/OvmfPkg/QemuFlashFvbServicesRuntimeDxe/FwBlockService.h
>> +++ b/OvmfPkg/QemuFlashFvbServicesRuntimeDxe/FwBlockService.h
>> @@ -189,4 +189,9 @@ VOID
>>  InstallVirtualAddressChangeHandler (
>>    VOID
>>    );
>> +
>> +VOID
>> +FvbBeforeFlashProbe (
>> +  VOID
>> +  );
>>  #endif
> Please drop the "Fvb" prefix; this function is not an FVB protocol member.

Will do.

>
>
>> diff --git a/OvmfPkg/QemuFlashFvbServicesRuntimeDxe/FwBlockService.c b/OvmfPkg/QemuFlashFvbServicesRuntimeDxe/FwBlockService.c
>> index 558b395dff4a..b7b9bf1fb8d9 100644
>> --- a/OvmfPkg/QemuFlashFvbServicesRuntimeDxe/FwBlockService.c
>> +++ b/OvmfPkg/QemuFlashFvbServicesRuntimeDxe/FwBlockService.c
>> @@ -967,6 +967,11 @@ FvbInitialize (
>>    UINTN                               NumOfBlocks;
>>    RETURN_STATUS                       PcdStatus;
>>  
>> +  //
>> +  // execute platform specific hooks before probing the flash
>> +  //
>> +  FvbBeforeFlashProbe ();
>> +
>>    if (EFI_ERROR (QemuFlashInitialize ())) {
>>      //
>>      // Return an error so image will be unloaded
>> diff --git a/OvmfPkg/QemuFlashFvbServicesRuntimeDxe/FwBlockServiceDxe.c b/OvmfPkg/QemuFlashFvbServicesRuntimeDxe/FwBlockServiceDxe.c
>> index 63b308658e36..7d274c08ad12 100644
>> --- a/OvmfPkg/QemuFlashFvbServicesRuntimeDxe/FwBlockServiceDxe.c
>> +++ b/OvmfPkg/QemuFlashFvbServicesRuntimeDxe/FwBlockServiceDxe.c
>> @@ -155,3 +155,13 @@ InstallVirtualAddressChangeHandler (
>>                    );
>>    ASSERT_EFI_ERROR (Status);
>>  }
>> +
>> +VOID
>> +FvbBeforeFlashProbe (
>> +  VOID
>> +  )
>> +{
>> +  //
>> +  // Do nothing
>> +  //
>> +}
>> diff --git a/OvmfPkg/QemuFlashFvbServicesRuntimeDxe/FwBlockServiceSmm.c b/OvmfPkg/QemuFlashFvbServicesRuntimeDxe/FwBlockServiceSmm.c
>> index e0617f2503a2..d97b13f47bf7 100644
>> --- a/OvmfPkg/QemuFlashFvbServicesRuntimeDxe/FwBlockServiceSmm.c
>> +++ b/OvmfPkg/QemuFlashFvbServicesRuntimeDxe/FwBlockServiceSmm.c
>> @@ -17,6 +17,7 @@
>>  #include <Library/DebugLib.h>
>>  #include <Library/PcdLib.h>
>>  #include <Library/SmmServicesTableLib.h>
>> +#include <Library/MemEncryptSevLib.h>
>>  #include <Protocol/DevicePath.h>
>>  #include <Protocol/SmmFirmwareVolumeBlock.h>
>>  
>> @@ -67,3 +68,37 @@ InstallVirtualAddressChangeHandler (
>>    // Nothing.
>>    //
>>  }
>> +
>> +VOID
>> +FvbBeforeFlashProbe (
>> +  VOID
>> +  )
>> +{
>> +
>> +  ASSERT (FeaturePcdGet (PcdSmmSmramRequire));
>> +
>> +  //
>> +  // When SEV is enabled, AmdSevDxe runs early in PEI phase and clears the C-bit
> s/PEI/DXE/

Will do.
>
>
>> +  // from the MMIO space (including flash ranges) but the driver runs in non SMM
>> +  // context hence it cleared the flash ranges from non SMM page table.
>> +  // When SMM is enabled, the flash services are accessed from the SMM mode
>> +  // hence we explicitly clear the C-bit on flash ranges from SMM page table.
>> +  //
>> +  if (MemEncryptSevIsEnabled ()) {
>> +    EFI_STATUS              Status;
>> +    EFI_PHYSICAL_ADDRESS    BaseAddress;
>> +    UINTN                   FdBlockSize, FdBlockCount;
>> +
>> +    BaseAddress = (EFI_PHYSICAL_ADDRESS) PcdGet32 (PcdOvmfFdBaseAddress);
>> +    FdBlockSize = PcdGet32 (PcdOvmfFirmwareBlockSize);
>> +    FdBlockCount = PcdGet32 (PcdOvmfFirmwareFdSize) / FdBlockSize;
>> +
>> +    Status = MemEncryptSevClearPageEncMask (
>> +               0,
>> +               BaseAddress,
>> +               EFI_SIZE_TO_PAGES (FdBlockSize * FdBlockCount),
>> +               FALSE
>> +              );
>> +    ASSERT_EFI_ERROR (Status);
>> +  }
>> +}
>>
> I think it would be better to hook this logic into
> QemuFlashInitialize(). That function already computes mFlashBase,
> mFdBlockSize and mFdBlockCount. Right before the call to
> QemuFlashDetected(), we could call BeforeFlashProbe(). The latter could
> take the base address, the block size and count as parameters, or just
> use the global variables.
>

Let me see what I can do.

> But, again, my preference would be to mirror the AmdSevDxe logic into
> (or right after) the SMM page table setup. Perhaps that can be done in
> SmmCpuFeaturesInitializeProcessor(), when IsMonarch is TRUE -- this
> function is called from SmmInitHandler(), and at that point, the SMM
> page tables are already in use. (See above the SmmInitHandler() call
> site in "UefiCpuPkg/PiSmmCpuDxeSmm/X64/SmmInit.nasm".)

Ah, I didn't know this one. I got SMM working with very small patch set
hence never looked in UefiCpuPkg for complete understanding of various
SmmFeatureLib routines but now I am looking more into it and I think we
may able to use SmmCpuFeatureInitializeProcessor() routines.

> Thanks,
> Laszlo