Re: [RFC v4 09/13] OvmfPkg/QemuFwCfgLib: Implement SEV internal function for SEC phase

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

On 05/11/2017 11:24 AM, Laszlo Ersek wrote:
> On 05/11/17 00:09, Brijesh Singh wrote:
>> Cc: Jordan Justen <jordan.l.justen@intel.com>
>> Cc: Laszlo Ersek <lersek@redhat.com>
>> Contributed-under: TianoCore Contribution Agreement 1.0
>> Signed-off-by: Brijesh Singh <brijesh.singh@amd.com>
>> ---
>>  OvmfPkg/Library/QemuFwCfgLib/QemuFwCfgSecLib.inf |  1 +
>>  OvmfPkg/Library/QemuFwCfgLib/QemuFwCfgSec.c      | 57 ++++++++++++++++++++
>>  2 files changed, 58 insertions(+)
>>
>> diff --git a/OvmfPkg/Library/QemuFwCfgLib/QemuFwCfgSecLib.inf b/OvmfPkg/Library/QemuFwCfgLib/QemuFwCfgSecLib.inf
>> index 7a96575d1851..b782ac6c0aa2 100644
>> --- a/OvmfPkg/Library/QemuFwCfgLib/QemuFwCfgSecLib.inf
>> +++ b/OvmfPkg/Library/QemuFwCfgLib/QemuFwCfgSecLib.inf
>> @@ -45,4 +45,5 @@ [LibraryClasses]
>>    DebugLib
>>    IoLib
>>    MemoryAllocationLib
>> +  MemEncryptSevLib
>
> This will not compile. More below.
>
>>
>> diff --git a/OvmfPkg/Library/QemuFwCfgLib/QemuFwCfgSec.c b/OvmfPkg/Library/QemuFwCfgLib/QemuFwCfgSec.c
>> index 465ccbe90dad..cd04cc814063 100644
>> --- a/OvmfPkg/Library/QemuFwCfgLib/QemuFwCfgSec.c
>> +++ b/OvmfPkg/Library/QemuFwCfgLib/QemuFwCfgSec.c
>> @@ -6,6 +6,7 @@
>>
>>    Copyright (C) 2013, Red Hat, Inc.
>>    Copyright (c) 2011 - 2013, Intel Corporation. All rights reserved.<BR>
>> +  Copyright (c) 2017, Advanced Micro Devices. All rights reserved.<BR>
>>
>>    This program and the accompanying materials are licensed and made available
>>    under the terms and conditions of the BSD License which accompanies this
>> @@ -18,6 +19,7 @@
>>
>>  #include <Library/DebugLib.h>
>>  #include <Library/QemuFwCfgLib.h>
>> +#include <Library/MemEncryptSevLib.h>
>>
>>  #include "QemuFwCfgLibInternal.h"
>>
>> @@ -94,3 +96,58 @@ InternalQemuFwCfgDmaIsAvailable (
>>  {
>>    return FALSE;
>>  }
>> +
>> +/**
>> +
>> + Returns a boolean indicating whether SEV is enabled
>> +
>> + @retval    TRUE    SEV is enabled
>> + @retval    FALSE   SEV is disabled
>> +**/
>> +BOOLEAN
>> +InternalQemuFwCfgSevIsEnabled (
>> +  VOID
>> +  )
>> +{
>> +  return MemEncryptSevIsEnabled ();
>> +}
>
> So, this is not right. We have one instance of MemEncryptSevLib, namely
> BaseMemEncryptSevLib. It uses / needs writeable static variables,
> therefore it is not usable in SEC phase modules. QemuFwCfgSecLib.inf is
> restricted to SEC type client modules, so the above function call would
> not work.
>
> Thankfully, this patch won't even compile (showcasing that the edk2
> build system works fine). Namely, corresponding to the writeable static
> variable requirement in BaseMemEncryptSevLib, we restricted that library
> instance to the following client module types:
>
> PEIM DXE_DRIVER DXE_RUNTIME_DRIVER DXE_SMM_DRIVER UEFI_DRIVER
>
> Whereas, QemuFwCfgSecLib.inf itself is restricted to SEC.
>
> The result is, if you try to build QemuFwCfgSecLib.inf into a SEC
> module, the client module type restrictions inherited from
> BaseMemEncryptSevLib will cause a build conflict. Otherwise, if you try
> to build QemuFwCfgSecLib.inf into, say, a PEIM, then
> QemuFwCfgSecLib.inf's own SEC restriction will cause a build conflict.
>
> So this patch cannot compile.
>
> You didn't find this in your testing because OVMF currently has no SEC
> phase module that uses fw_cfg -- QemuFwCfgSecLib.inf is never built. You
> can manually build it like this, for example (note the "-m" option):
>
>   build -a X64 -p OvmfPkg/OvmfPkgX64.dsc -t GCC48 -b DEBUG \
>     -m OvmfPkg/Library/QemuFwCfgLib/QemuFwCfgSecLib.inf
>
> (a) One solution is what I wrote in
> <http://mid.mail-archive.com/a07a25d1-0aec-4176-312f-198bf10e29d1@redhat.com>:
>
>> (2) Implement InternalQemuFwCfgSevIsEnabled() in "QemuFwCfgSec.c"
>> without using global variables (i.e., with a CPUID on each call, on
>> AMD processors, and return constant FALSE on Intel processors).
>
> So basically you should open-code MemEncryptSevIsEnabled() here, without
> using any global variables.
>
> (b) Now, a "by the book" solution would be to introduce a SEC instance
> of MemEncryptSevLib as well, which would execute CPUID on every
> MemEncryptSevIsEnabled() call, and return RETURN_UNSUPPORTED from both
> MemEncryptSevClearPageEncMask() and MemEncryptSevSetPageEncMask(),
> regardless of architecture (IA32 vs X64). Then this patch would compile
> as-is.
>
> (c) But, I don't want to contribute to the proliferation, or growth, of
> library instances that are never used in reality. So I dislike both (a)
> and (b) above. The only reason we care about QemuFwCfgLib, with regard
> to SEV, is because QemuFwCfgLib *sometimes* uses DMA, and SEV has
> consequences for DMA.
>
> Notice though that the SEC instance of InternalQemuFwCfgDmaIsAvailable()
> returns constant FALSE. This is why it is fine to put ASSERT(FALSE) /
> CpuDeadLoop() in the bounce buffer alloc / dealloc routines: they will
> never be called.
>
> With the same argument (i.e., we'll never use DMA fw_cfg in SEC), it is
> entirely irrelevant for this lib instance whether SEV is present or not.
> So I suggest to simply return FALSE from InternalQemuFwCfgSevIsEnabled()
> above, and to remark there, in a comment, that
> InternalQemuFwCfgDmaIsAvailable() returns constant FALSE, hence SEV
> availability is irrelevant.
>


Agreed, lets not add a code which is never used in reality. I will update the
InternalQemuFwCfgSevIsEnabled() to return FALSE.

-Briejsh