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

[Laszlo Ersek <lersek@redhat.com>]

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.

Thanks,
Laszlo

> +
> +/**
> + Allocate a bounce buffer for SEV DMA.
> +
> +  @param[in]     NumPage  Number of pages.
> +  @param[out]    Buffer   Allocated DMA Buffer pointer
> +
> +**/
> +VOID
> +InternalQemuFwCfgSevDmaAllocateBuffer (
> +  IN     UINT32   NumPages,
> +  OUT    VOID     **Buffer
> +  )
> +{
> +  //
> +  // We should never reach here
> +  //
> +  ASSERT (FALSE);
> +  CpuDeadLoop ();
> +}
> +
> +/**
> + Free the DMA buffer allocated using InternalQemuFwCfgSevDmaAllocateBuffer
> +
> +  @param[in]     NumPage  Number of pages.
> +  @param[in]     Buffer   DMA Buffer pointer
> +
> +**/
> +VOID
> +InternalQemuFwCfgSevDmaFreeBuffer (
> +  IN     VOID     *Buffer,
> +  IN     UINT32   NumPages
> +  )
> +{
> +  //
> +  // We should never reach here
> +  //
> +  ASSERT (FALSE);
> +  CpuDeadLoop ();
> +}
>