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
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 ();
+}
+
+/**
+ 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 ();
+}
--
2.7.4
_______________________________________________
edk2-devel mailing list
edk2-devel@lists.01.org
https://lists.01.org/mailman/listinfo/edk2-devel
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 ();
> +}
>
_______________________________________________
edk2-devel mailing list
edk2-devel@lists.01.org
https://lists.01.org/mailman/listinfo/edk2-devel
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
_______________________________________________
edk2-devel mailing list
edk2-devel@lists.01.org
https://lists.01.org/mailman/listinfo/edk2-devel
© 2016 - 2024 Red Hat, Inc.