* [PATCH v3 1/3] OvmfPkg/QemuFlashFvbServicesRuntimeDxe: mark Flash memory range as MMIO
2018-07-05 19:12 [PATCH v3 0/3] OvmfPkg: mark flash memory range as MMIO Brijesh Singh
@ 2018-07-05 19:12 ` Brijesh Singh
2018-07-06 11:24 ` Laszlo Ersek
2018-07-05 19:12 ` [PATCH v3 2/3] OvmfPkg/QemuFlashFvbServicesRuntimeDxe: Do not expose MMIO in SMM build Brijesh Singh
` (2 subsequent siblings)
3 siblings, 1 reply; 9+ messages in thread
From: Brijesh Singh @ 2018-07-05 19:12 UTC (permalink / raw)
To: edk2-devel
Cc: Lendacky Thomas, Brijesh Singh, Ard Biesheuvel, Anthony Perard,
Julien Grall, Justen Jordan L, Laszlo Ersek
The flash memory range is an IO address and should be presented as Memory
Mapped IO in EFI Runtime mapping. This information can be used by OS
when mapping the flash memory range.
It is especially helpful in SEV guest case, in which IO addresses should
be mapped as unencrypted. If memory region is not marked as MMIO then OS
maps the range as encrypted.
Cc: Ard Biesheuvel <ard.biesheuvel@linaro.org>
Cc: Anthony Perard <anthony.perard@citrix.com>
Cc: Julien Grall <julien.grall@linaro.org>
Cc: Justen Jordan L <jordan.l.justen@intel.com>
Cc: Laszlo Ersek <lersek@redhat.com>
Contributed-under: TianoCore Contribution Agreement 1.1
Signed-off-by: Brijesh Singh <brijesh.singh@amd.com>
---
.../FwBlockService.c | 28 ++++++++++++++++------
1 file changed, 21 insertions(+), 7 deletions(-)
diff --git a/OvmfPkg/QemuFlashFvbServicesRuntimeDxe/FwBlockService.c b/OvmfPkg/QemuFlashFvbServicesRuntimeDxe/FwBlockService.c
index 558b395dff4a..28499991a43c 100644
--- a/OvmfPkg/QemuFlashFvbServicesRuntimeDxe/FwBlockService.c
+++ b/OvmfPkg/QemuFlashFvbServicesRuntimeDxe/FwBlockService.c
@@ -831,12 +831,13 @@ ValidateFvHeader (
STATIC
EFI_STATUS
-MarkMemoryRangeForRuntimeAccess (
+MarkIoMemoryRangeForRuntimeAccess (
EFI_PHYSICAL_ADDRESS BaseAddress,
UINTN Length
)
{
EFI_STATUS Status;
+ EFI_GCD_MEMORY_SPACE_DESCRIPTOR GcdDescriptor;
//
// Mark flash region as runtime memory
@@ -847,18 +848,31 @@ MarkMemoryRangeForRuntimeAccess (
);
Status = gDS->AddMemorySpace (
- EfiGcdMemoryTypeSystemMemory,
+ EfiGcdMemoryTypeMemoryMappedIo,
BaseAddress,
Length,
EFI_MEMORY_UC | EFI_MEMORY_RUNTIME
);
ASSERT_EFI_ERROR (Status);
- Status = gBS->AllocatePages (
+ Status = gDS->AllocateMemorySpace (
AllocateAddress,
- EfiRuntimeServicesData,
- EFI_SIZE_TO_PAGES (Length),
- &BaseAddress
+ EfiGcdMemoryTypeMemoryMappedIo,
+ 0,
+ Length,
+ &BaseAddress,
+ gImageHandle,
+ NULL
+ );
+ ASSERT_EFI_ERROR (Status);
+
+ Status = gDS->GetMemorySpaceDescriptor (BaseAddress, &GcdDescriptor);
+ ASSERT_EFI_ERROR (Status);
+
+ Status = gDS->SetMemorySpaceAttributes (
+ BaseAddress,
+ Length,
+ GcdDescriptor.Attributes | EFI_MEMORY_RUNTIME
);
ASSERT_EFI_ERROR (Status);
@@ -1091,7 +1105,7 @@ FvbInitialize (
//
InstallProtocolInterfaces (FvbDevice);
- MarkMemoryRangeForRuntimeAccess (BaseAddress, Length);
+ MarkIoMemoryRangeForRuntimeAccess (BaseAddress, Length);
//
// Set several PCD values to point to flash
--
2.7.4
^ permalink raw reply related [flat|nested] 9+ messages in thread* Re: [PATCH v3 1/3] OvmfPkg/QemuFlashFvbServicesRuntimeDxe: mark Flash memory range as MMIO
2018-07-05 19:12 ` [PATCH v3 1/3] OvmfPkg/QemuFlashFvbServicesRuntimeDxe: mark Flash " Brijesh Singh
@ 2018-07-06 11:24 ` Laszlo Ersek
0 siblings, 0 replies; 9+ messages in thread
From: Laszlo Ersek @ 2018-07-06 11:24 UTC (permalink / raw)
To: Brijesh Singh, edk2-devel
Cc: Lendacky Thomas, Ard Biesheuvel, Anthony Perard, Julien Grall,
Justen Jordan L
On 07/05/18 21:12, Brijesh Singh wrote:
> The flash memory range is an IO address and should be presented as Memory
> Mapped IO in EFI Runtime mapping. This information can be used by OS
> when mapping the flash memory range.
>
> It is especially helpful in SEV guest case, in which IO addresses should
> be mapped as unencrypted. If memory region is not marked as MMIO then OS
> maps the range as encrypted.
>
> Cc: Ard Biesheuvel <ard.biesheuvel@linaro.org>
> Cc: Anthony Perard <anthony.perard@citrix.com>
> Cc: Julien Grall <julien.grall@linaro.org>
> Cc: Justen Jordan L <jordan.l.justen@intel.com>
> Cc: Laszlo Ersek <lersek@redhat.com>
> Contributed-under: TianoCore Contribution Agreement 1.1
> Signed-off-by: Brijesh Singh <brijesh.singh@amd.com>
> ---
> .../FwBlockService.c | 28 ++++++++++++++++------
> 1 file changed, 21 insertions(+), 7 deletions(-)
>
> diff --git a/OvmfPkg/QemuFlashFvbServicesRuntimeDxe/FwBlockService.c b/OvmfPkg/QemuFlashFvbServicesRuntimeDxe/FwBlockService.c
> index 558b395dff4a..28499991a43c 100644
> --- a/OvmfPkg/QemuFlashFvbServicesRuntimeDxe/FwBlockService.c
> +++ b/OvmfPkg/QemuFlashFvbServicesRuntimeDxe/FwBlockService.c
> @@ -831,12 +831,13 @@ ValidateFvHeader (
>
> STATIC
> EFI_STATUS
> -MarkMemoryRangeForRuntimeAccess (
> +MarkIoMemoryRangeForRuntimeAccess (
> EFI_PHYSICAL_ADDRESS BaseAddress,
> UINTN Length
> )
> {
> EFI_STATUS Status;
> + EFI_GCD_MEMORY_SPACE_DESCRIPTOR GcdDescriptor;
>
> //
> // Mark flash region as runtime memory
> @@ -847,18 +848,31 @@ MarkMemoryRangeForRuntimeAccess (
> );
>
> Status = gDS->AddMemorySpace (
> - EfiGcdMemoryTypeSystemMemory,
> + EfiGcdMemoryTypeMemoryMappedIo,
> BaseAddress,
> Length,
> EFI_MEMORY_UC | EFI_MEMORY_RUNTIME
> );
> ASSERT_EFI_ERROR (Status);
>
> - Status = gBS->AllocatePages (
> + Status = gDS->AllocateMemorySpace (
> AllocateAddress,
(1) This should be changed to "EfiGcdAllocateAddress". (I didn't notice
this in your previous submission; however, to my excuse, I did suggest
it correctly in
<http://mid.mail-archive.com/fbba354d-1df7-0da5-80a6-40df601b95ea@redhat.com>
:) )
Note that this omission does not invalidate your testing, because
"AllocateAddress" (from "MdePkg/Include/Uefi/UefiSpec.h") has value 2,
and "EfiGcdAllocateAddress" (from "MdePkg/Include/Pi/PiDxeCis.h") has
value 2 as well.
> - EfiRuntimeServicesData,
> - EFI_SIZE_TO_PAGES (Length),
> - &BaseAddress
> + EfiGcdMemoryTypeMemoryMappedIo,
> + 0,
> + Length,
> + &BaseAddress,
> + gImageHandle,
> + NULL
> + );
> + ASSERT_EFI_ERROR (Status);
> +
> + Status = gDS->GetMemorySpaceDescriptor (BaseAddress, &GcdDescriptor);
(2) The extra space before the equal sign (=) looks uncalled for.
> + ASSERT_EFI_ERROR (Status);
> +
> + Status = gDS->SetMemorySpaceAttributes (
> + BaseAddress,
> + Length,
> + GcdDescriptor.Attributes | EFI_MEMORY_RUNTIME
> );
> ASSERT_EFI_ERROR (Status);
>
> @@ -1091,7 +1105,7 @@ FvbInitialize (
> //
> InstallProtocolInterfaces (FvbDevice);
>
> - MarkMemoryRangeForRuntimeAccess (BaseAddress, Length);
> + MarkIoMemoryRangeForRuntimeAccess (BaseAddress, Length);
>
> //
> // Set several PCD values to point to flash
>
Thanks!
Laszlo
^ permalink raw reply [flat|nested] 9+ messages in thread
* [PATCH v3 2/3] OvmfPkg/QemuFlashFvbServicesRuntimeDxe: Do not expose MMIO in SMM build
2018-07-05 19:12 [PATCH v3 0/3] OvmfPkg: mark flash memory range as MMIO Brijesh Singh
2018-07-05 19:12 ` [PATCH v3 1/3] OvmfPkg/QemuFlashFvbServicesRuntimeDxe: mark Flash " Brijesh Singh
@ 2018-07-05 19:12 ` Brijesh Singh
2018-07-06 11:35 ` Laszlo Ersek
2018-07-05 19:12 ` [PATCH v3 3/3] OvmfPkg/QemuFlashFvbServicesRuntimeDxe: Restore C-bit when SEV is active Brijesh Singh
2018-07-06 14:01 ` [PATCH v3 0/3] OvmfPkg: mark flash memory range as MMIO Laszlo Ersek
3 siblings, 1 reply; 9+ messages in thread
From: Brijesh Singh @ 2018-07-05 19:12 UTC (permalink / raw)
To: edk2-devel
Cc: Lendacky Thomas, Brijesh Singh, Ard Biesheuvel, Anthony Perard,
Julien Grall, Justen Jordan L, Laszlo Ersek
In the SMM build, only an SMM driver is using the address range hence we
do not need to expose the flash MMIO range in EFI runtime mapping.
Cc: Ard Biesheuvel <ard.biesheuvel@linaro.org>
Cc: Anthony Perard <anthony.perard@citrix.com>
Cc: Julien Grall <julien.grall@linaro.org>
Cc: Justen Jordan L <jordan.l.justen@intel.com>
Cc: Laszlo Ersek <lersek@redhat.com>
Contributed-under: TianoCore Contribution Agreement 1.1
Signed-off-by: Brijesh Singh <brijesh.singh@amd.com>
---
.../FwBlockService.c | 50 ---------------------
.../FwBlockService.h | 7 +++
.../FwBlockServiceDxe.c | 51 ++++++++++++++++++++++
.../FwBlockServiceSmm.c | 13 ++++++
4 files changed, 71 insertions(+), 50 deletions(-)
diff --git a/OvmfPkg/QemuFlashFvbServicesRuntimeDxe/FwBlockService.c b/OvmfPkg/QemuFlashFvbServicesRuntimeDxe/FwBlockService.c
index 28499991a43c..eec8b1b1ae9d 100644
--- a/OvmfPkg/QemuFlashFvbServicesRuntimeDxe/FwBlockService.c
+++ b/OvmfPkg/QemuFlashFvbServicesRuntimeDxe/FwBlockService.c
@@ -831,56 +831,6 @@ ValidateFvHeader (
STATIC
EFI_STATUS
-MarkIoMemoryRangeForRuntimeAccess (
- EFI_PHYSICAL_ADDRESS BaseAddress,
- UINTN Length
- )
-{
- EFI_STATUS Status;
- EFI_GCD_MEMORY_SPACE_DESCRIPTOR GcdDescriptor;
-
- //
- // Mark flash region as runtime memory
- //
- Status = gDS->RemoveMemorySpace (
- BaseAddress,
- Length
- );
-
- Status = gDS->AddMemorySpace (
- EfiGcdMemoryTypeMemoryMappedIo,
- BaseAddress,
- Length,
- EFI_MEMORY_UC | EFI_MEMORY_RUNTIME
- );
- ASSERT_EFI_ERROR (Status);
-
- Status = gDS->AllocateMemorySpace (
- AllocateAddress,
- EfiGcdMemoryTypeMemoryMappedIo,
- 0,
- Length,
- &BaseAddress,
- gImageHandle,
- NULL
- );
- ASSERT_EFI_ERROR (Status);
-
- Status = gDS->GetMemorySpaceDescriptor (BaseAddress, &GcdDescriptor);
- ASSERT_EFI_ERROR (Status);
-
- Status = gDS->SetMemorySpaceAttributes (
- BaseAddress,
- Length,
- GcdDescriptor.Attributes | EFI_MEMORY_RUNTIME
- );
- ASSERT_EFI_ERROR (Status);
-
- return Status;
-}
-
-STATIC
-EFI_STATUS
InitializeVariableFvHeader (
VOID
)
diff --git a/OvmfPkg/QemuFlashFvbServicesRuntimeDxe/FwBlockService.h b/OvmfPkg/QemuFlashFvbServicesRuntimeDxe/FwBlockService.h
index 1f9287b08769..178f578d49f0 100644
--- a/OvmfPkg/QemuFlashFvbServicesRuntimeDxe/FwBlockService.h
+++ b/OvmfPkg/QemuFlashFvbServicesRuntimeDxe/FwBlockService.h
@@ -189,4 +189,11 @@ VOID
InstallVirtualAddressChangeHandler (
VOID
);
+
+EFI_STATUS
+MarkIoMemoryRangeForRuntimeAccess (
+ IN EFI_PHYSICAL_ADDRESS BaseAddress,
+ IN UINTN Length
+ );
+
#endif
diff --git a/OvmfPkg/QemuFlashFvbServicesRuntimeDxe/FwBlockServiceDxe.c b/OvmfPkg/QemuFlashFvbServicesRuntimeDxe/FwBlockServiceDxe.c
index 63b308658e36..646427bf4e2c 100644
--- a/OvmfPkg/QemuFlashFvbServicesRuntimeDxe/FwBlockServiceDxe.c
+++ b/OvmfPkg/QemuFlashFvbServicesRuntimeDxe/FwBlockServiceDxe.c
@@ -22,6 +22,8 @@
#include <Library/UefiRuntimeLib.h>
#include <Protocol/DevicePath.h>
#include <Protocol/FirmwareVolumeBlock.h>
+#include <Library/DxeServicesTableLib.h>
+#include <Library/MemoryAllocationLib.h>
#include "FwBlockService.h"
#include "QemuFlash.h"
@@ -155,3 +157,52 @@ InstallVirtualAddressChangeHandler (
);
ASSERT_EFI_ERROR (Status);
}
+
+EFI_STATUS
+MarkIoMemoryRangeForRuntimeAccess (
+ EFI_PHYSICAL_ADDRESS BaseAddress,
+ UINTN Length
+ )
+{
+ EFI_STATUS Status;
+ EFI_GCD_MEMORY_SPACE_DESCRIPTOR GcdDescriptor;
+
+ //
+ // Mark flash region as runtime memory
+ //
+ Status = gDS->RemoveMemorySpace (
+ BaseAddress,
+ Length
+ );
+
+ Status = gDS->AddMemorySpace (
+ EfiGcdMemoryTypeMemoryMappedIo,
+ BaseAddress,
+ Length,
+ EFI_MEMORY_UC | EFI_MEMORY_RUNTIME
+ );
+ ASSERT_EFI_ERROR (Status);
+
+ Status = gDS->AllocateMemorySpace (
+ AllocateAddress,
+ EfiGcdMemoryTypeMemoryMappedIo,
+ 0,
+ Length,
+ &BaseAddress,
+ gImageHandle,
+ NULL
+ );
+ ASSERT_EFI_ERROR (Status);
+
+ Status = gDS->GetMemorySpaceDescriptor (BaseAddress, &GcdDescriptor);
+ ASSERT_EFI_ERROR (Status);
+
+ Status = gDS->SetMemorySpaceAttributes (
+ BaseAddress,
+ Length,
+ GcdDescriptor.Attributes | EFI_MEMORY_RUNTIME
+ );
+ ASSERT_EFI_ERROR (Status);
+
+ return Status;
+}
diff --git a/OvmfPkg/QemuFlashFvbServicesRuntimeDxe/FwBlockServiceSmm.c b/OvmfPkg/QemuFlashFvbServicesRuntimeDxe/FwBlockServiceSmm.c
index e0617f2503a2..cdb073348158 100644
--- a/OvmfPkg/QemuFlashFvbServicesRuntimeDxe/FwBlockServiceSmm.c
+++ b/OvmfPkg/QemuFlashFvbServicesRuntimeDxe/FwBlockServiceSmm.c
@@ -67,3 +67,16 @@ InstallVirtualAddressChangeHandler (
// Nothing.
//
}
+
+EFI_STATUS
+MarkIoMemoryRangeForRuntimeAccess (
+ EFI_PHYSICAL_ADDRESS BaseAddress,
+ UINTN Length
+ )
+{
+ //
+ // Nothing
+ //
+
+ return EFI_SUCCESS;
+}
--
2.7.4
^ permalink raw reply related [flat|nested] 9+ messages in thread* Re: [PATCH v3 2/3] OvmfPkg/QemuFlashFvbServicesRuntimeDxe: Do not expose MMIO in SMM build
2018-07-05 19:12 ` [PATCH v3 2/3] OvmfPkg/QemuFlashFvbServicesRuntimeDxe: Do not expose MMIO in SMM build Brijesh Singh
@ 2018-07-06 11:35 ` Laszlo Ersek
2018-07-06 11:48 ` Laszlo Ersek
0 siblings, 1 reply; 9+ messages in thread
From: Laszlo Ersek @ 2018-07-06 11:35 UTC (permalink / raw)
To: Brijesh Singh, edk2-devel
Cc: Lendacky Thomas, Ard Biesheuvel, Anthony Perard, Julien Grall,
Justen Jordan L
On 07/05/18 21:12, Brijesh Singh wrote:
> In the SMM build, only an SMM driver is using the address range hence we
> do not need to expose the flash MMIO range in EFI runtime mapping.
>
> Cc: Ard Biesheuvel <ard.biesheuvel@linaro.org>
> Cc: Anthony Perard <anthony.perard@citrix.com>
> Cc: Julien Grall <julien.grall@linaro.org>
> Cc: Justen Jordan L <jordan.l.justen@intel.com>
> Cc: Laszlo Ersek <lersek@redhat.com>
> Contributed-under: TianoCore Contribution Agreement 1.1
> Signed-off-by: Brijesh Singh <brijesh.singh@amd.com>
> ---
> .../FwBlockService.c | 50 ---------------------
> .../FwBlockService.h | 7 +++
> .../FwBlockServiceDxe.c | 51 ++++++++++++++++++++++
> .../FwBlockServiceSmm.c | 13 ++++++
> 4 files changed, 71 insertions(+), 50 deletions(-)
>
> diff --git a/OvmfPkg/QemuFlashFvbServicesRuntimeDxe/FwBlockService.c b/OvmfPkg/QemuFlashFvbServicesRuntimeDxe/FwBlockService.c
> index 28499991a43c..eec8b1b1ae9d 100644
> --- a/OvmfPkg/QemuFlashFvbServicesRuntimeDxe/FwBlockService.c
> +++ b/OvmfPkg/QemuFlashFvbServicesRuntimeDxe/FwBlockService.c
> @@ -831,56 +831,6 @@ ValidateFvHeader (
>
> STATIC
> EFI_STATUS
> -MarkIoMemoryRangeForRuntimeAccess (
> - EFI_PHYSICAL_ADDRESS BaseAddress,
> - UINTN Length
> - )
> -{
> - EFI_STATUS Status;
> - EFI_GCD_MEMORY_SPACE_DESCRIPTOR GcdDescriptor;
> -
> - //
> - // Mark flash region as runtime memory
> - //
> - Status = gDS->RemoveMemorySpace (
> - BaseAddress,
> - Length
> - );
> -
> - Status = gDS->AddMemorySpace (
> - EfiGcdMemoryTypeMemoryMappedIo,
> - BaseAddress,
> - Length,
> - EFI_MEMORY_UC | EFI_MEMORY_RUNTIME
> - );
> - ASSERT_EFI_ERROR (Status);
> -
> - Status = gDS->AllocateMemorySpace (
> - AllocateAddress,
> - EfiGcdMemoryTypeMemoryMappedIo,
> - 0,
> - Length,
> - &BaseAddress,
> - gImageHandle,
> - NULL
> - );
> - ASSERT_EFI_ERROR (Status);
> -
> - Status = gDS->GetMemorySpaceDescriptor (BaseAddress, &GcdDescriptor);
> - ASSERT_EFI_ERROR (Status);
> -
> - Status = gDS->SetMemorySpaceAttributes (
> - BaseAddress,
> - Length,
> - GcdDescriptor.Attributes | EFI_MEMORY_RUNTIME
> - );
> - ASSERT_EFI_ERROR (Status);
> -
> - return Status;
> -}
> -
> -STATIC
> -EFI_STATUS
> InitializeVariableFvHeader (
> VOID
> )
> diff --git a/OvmfPkg/QemuFlashFvbServicesRuntimeDxe/FwBlockService.h b/OvmfPkg/QemuFlashFvbServicesRuntimeDxe/FwBlockService.h
> index 1f9287b08769..178f578d49f0 100644
> --- a/OvmfPkg/QemuFlashFvbServicesRuntimeDxe/FwBlockService.h
> +++ b/OvmfPkg/QemuFlashFvbServicesRuntimeDxe/FwBlockService.h
> @@ -189,4 +189,11 @@ VOID
> InstallVirtualAddressChangeHandler (
> VOID
> );
> +
> +EFI_STATUS
> +MarkIoMemoryRangeForRuntimeAccess (
> + IN EFI_PHYSICAL_ADDRESS BaseAddress,
> + IN UINTN Length
> + );
> +
> #endif
> diff --git a/OvmfPkg/QemuFlashFvbServicesRuntimeDxe/FwBlockServiceDxe.c b/OvmfPkg/QemuFlashFvbServicesRuntimeDxe/FwBlockServiceDxe.c
> index 63b308658e36..646427bf4e2c 100644
> --- a/OvmfPkg/QemuFlashFvbServicesRuntimeDxe/FwBlockServiceDxe.c
> +++ b/OvmfPkg/QemuFlashFvbServicesRuntimeDxe/FwBlockServiceDxe.c
> @@ -22,6 +22,8 @@
> #include <Library/UefiRuntimeLib.h>
> #include <Protocol/DevicePath.h>
> #include <Protocol/FirmwareVolumeBlock.h>
> +#include <Library/DxeServicesTableLib.h>
> +#include <Library/MemoryAllocationLib.h>
(1) Can you drop "MemoryAllocationLib.h"? I don't think it is needed.
(2) Also, can you add the "DxeServicesTableLib.h" include directive so
that the Library #include list remains sorted?
>
> #include "FwBlockService.h"
> #include "QemuFlash.h"
> @@ -155,3 +157,52 @@ InstallVirtualAddressChangeHandler (
> );
> ASSERT_EFI_ERROR (Status);
> }
> +
> +EFI_STATUS
> +MarkIoMemoryRangeForRuntimeAccess (
> + EFI_PHYSICAL_ADDRESS BaseAddress,
> + UINTN Length
> + )
> +{
> + EFI_STATUS Status;
> + EFI_GCD_MEMORY_SPACE_DESCRIPTOR GcdDescriptor;
> +
> + //
> + // Mark flash region as runtime memory
> + //
> + Status = gDS->RemoveMemorySpace (
> + BaseAddress,
> + Length
> + );
> +
> + Status = gDS->AddMemorySpace (
> + EfiGcdMemoryTypeMemoryMappedIo,
> + BaseAddress,
> + Length,
> + EFI_MEMORY_UC | EFI_MEMORY_RUNTIME
> + );
> + ASSERT_EFI_ERROR (Status);
> +
> + Status = gDS->AllocateMemorySpace (
> + AllocateAddress,
> + EfiGcdMemoryTypeMemoryMappedIo,
> + 0,
> + Length,
> + &BaseAddress,
> + gImageHandle,
> + NULL
> + );
> + ASSERT_EFI_ERROR (Status);
> +
> + Status = gDS->GetMemorySpaceDescriptor (BaseAddress, &GcdDescriptor);
> + ASSERT_EFI_ERROR (Status);
> +
> + Status = gDS->SetMemorySpaceAttributes (
> + BaseAddress,
> + Length,
> + GcdDescriptor.Attributes | EFI_MEMORY_RUNTIME
> + );
> + ASSERT_EFI_ERROR (Status);
> +
> + return Status;
> +}
(3) Please don't forget to update this function (post-move) as I
requested for patch #1.
> diff --git a/OvmfPkg/QemuFlashFvbServicesRuntimeDxe/FwBlockServiceSmm.c b/OvmfPkg/QemuFlashFvbServicesRuntimeDxe/FwBlockServiceSmm.c
> index e0617f2503a2..cdb073348158 100644
> --- a/OvmfPkg/QemuFlashFvbServicesRuntimeDxe/FwBlockServiceSmm.c
> +++ b/OvmfPkg/QemuFlashFvbServicesRuntimeDxe/FwBlockServiceSmm.c
> @@ -67,3 +67,16 @@ InstallVirtualAddressChangeHandler (
> // Nothing.
> //
> }
> +
> +EFI_STATUS
> +MarkIoMemoryRangeForRuntimeAccess (
> + EFI_PHYSICAL_ADDRESS BaseAddress,
> + UINTN Length
> + )
> +{
> + //
> + // Nothing
> + //
> +
> + return EFI_SUCCESS;
> +}
>
Looks OK to me otherwise.
Thanks
Laszlo
^ permalink raw reply [flat|nested] 9+ messages in thread* Re: [PATCH v3 2/3] OvmfPkg/QemuFlashFvbServicesRuntimeDxe: Do not expose MMIO in SMM build
2018-07-06 11:35 ` Laszlo Ersek
@ 2018-07-06 11:48 ` Laszlo Ersek
0 siblings, 0 replies; 9+ messages in thread
From: Laszlo Ersek @ 2018-07-06 11:48 UTC (permalink / raw)
To: Brijesh Singh, edk2-devel
Cc: Lendacky Thomas, Ard Biesheuvel, Anthony Perard, Julien Grall,
Justen Jordan L
On 07/06/18 13:35, Laszlo Ersek wrote:
> On 07/05/18 21:12, Brijesh Singh wrote:
>> In the SMM build, only an SMM driver is using the address range hence we
>> do not need to expose the flash MMIO range in EFI runtime mapping.
>>
>> Cc: Ard Biesheuvel <ard.biesheuvel@linaro.org>
>> Cc: Anthony Perard <anthony.perard@citrix.com>
>> Cc: Julien Grall <julien.grall@linaro.org>
>> Cc: Justen Jordan L <jordan.l.justen@intel.com>
>> Cc: Laszlo Ersek <lersek@redhat.com>
>> Contributed-under: TianoCore Contribution Agreement 1.1
>> Signed-off-by: Brijesh Singh <brijesh.singh@amd.com>
>> ---
>> .../FwBlockService.c | 50 ---------------------
>> .../FwBlockService.h | 7 +++
>> .../FwBlockServiceDxe.c | 51 ++++++++++++++++++++++
>> .../FwBlockServiceSmm.c | 13 ++++++
>> 4 files changed, 71 insertions(+), 50 deletions(-)
>>
>> diff --git a/OvmfPkg/QemuFlashFvbServicesRuntimeDxe/FwBlockService.c b/OvmfPkg/QemuFlashFvbServicesRuntimeDxe/FwBlockService.c
>> index 28499991a43c..eec8b1b1ae9d 100644
>> --- a/OvmfPkg/QemuFlashFvbServicesRuntimeDxe/FwBlockService.c
>> +++ b/OvmfPkg/QemuFlashFvbServicesRuntimeDxe/FwBlockService.c
>> @@ -831,56 +831,6 @@ ValidateFvHeader (
>>
>> STATIC
>> EFI_STATUS
>> -MarkIoMemoryRangeForRuntimeAccess (
>> - EFI_PHYSICAL_ADDRESS BaseAddress,
>> - UINTN Length
>> - )
>> -{
>> - EFI_STATUS Status;
>> - EFI_GCD_MEMORY_SPACE_DESCRIPTOR GcdDescriptor;
>> -
>> - //
>> - // Mark flash region as runtime memory
>> - //
>> - Status = gDS->RemoveMemorySpace (
>> - BaseAddress,
>> - Length
>> - );
>> -
>> - Status = gDS->AddMemorySpace (
>> - EfiGcdMemoryTypeMemoryMappedIo,
>> - BaseAddress,
>> - Length,
>> - EFI_MEMORY_UC | EFI_MEMORY_RUNTIME
>> - );
>> - ASSERT_EFI_ERROR (Status);
>> -
>> - Status = gDS->AllocateMemorySpace (
>> - AllocateAddress,
>> - EfiGcdMemoryTypeMemoryMappedIo,
>> - 0,
>> - Length,
>> - &BaseAddress,
>> - gImageHandle,
>> - NULL
>> - );
>> - ASSERT_EFI_ERROR (Status);
>> -
>> - Status = gDS->GetMemorySpaceDescriptor (BaseAddress, &GcdDescriptor);
>> - ASSERT_EFI_ERROR (Status);
>> -
>> - Status = gDS->SetMemorySpaceAttributes (
>> - BaseAddress,
>> - Length,
>> - GcdDescriptor.Attributes | EFI_MEMORY_RUNTIME
>> - );
>> - ASSERT_EFI_ERROR (Status);
>> -
>> - return Status;
>> -}
>> -
>> -STATIC
>> -EFI_STATUS
>> InitializeVariableFvHeader (
>> VOID
>> )
>> diff --git a/OvmfPkg/QemuFlashFvbServicesRuntimeDxe/FwBlockService.h b/OvmfPkg/QemuFlashFvbServicesRuntimeDxe/FwBlockService.h
>> index 1f9287b08769..178f578d49f0 100644
>> --- a/OvmfPkg/QemuFlashFvbServicesRuntimeDxe/FwBlockService.h
>> +++ b/OvmfPkg/QemuFlashFvbServicesRuntimeDxe/FwBlockService.h
>> @@ -189,4 +189,11 @@ VOID
>> InstallVirtualAddressChangeHandler (
>> VOID
>> );
>> +
>> +EFI_STATUS
>> +MarkIoMemoryRangeForRuntimeAccess (
>> + IN EFI_PHYSICAL_ADDRESS BaseAddress,
>> + IN UINTN Length
>> + );
>> +
>> #endif
>> diff --git a/OvmfPkg/QemuFlashFvbServicesRuntimeDxe/FwBlockServiceDxe.c b/OvmfPkg/QemuFlashFvbServicesRuntimeDxe/FwBlockServiceDxe.c
>> index 63b308658e36..646427bf4e2c 100644
>> --- a/OvmfPkg/QemuFlashFvbServicesRuntimeDxe/FwBlockServiceDxe.c
>> +++ b/OvmfPkg/QemuFlashFvbServicesRuntimeDxe/FwBlockServiceDxe.c
>> @@ -22,6 +22,8 @@
>> #include <Library/UefiRuntimeLib.h>
>> #include <Protocol/DevicePath.h>
>> #include <Protocol/FirmwareVolumeBlock.h>
>> +#include <Library/DxeServicesTableLib.h>
>> +#include <Library/MemoryAllocationLib.h>
>
> (1) Can you drop "MemoryAllocationLib.h"? I don't think it is needed.
>
> (2) Also, can you add the "DxeServicesTableLib.h" include directive so
> that the Library #include list remains sorted?
>
>>
>> #include "FwBlockService.h"
>> #include "QemuFlash.h"
>> @@ -155,3 +157,52 @@ InstallVirtualAddressChangeHandler (
>> );
>> ASSERT_EFI_ERROR (Status);
>> }
>> +
>> +EFI_STATUS
>> +MarkIoMemoryRangeForRuntimeAccess (
>> + EFI_PHYSICAL_ADDRESS BaseAddress,
>> + UINTN Length
>> + )
>> +{
>> + EFI_STATUS Status;
>> + EFI_GCD_MEMORY_SPACE_DESCRIPTOR GcdDescriptor;
>> +
>> + //
>> + // Mark flash region as runtime memory
>> + //
>> + Status = gDS->RemoveMemorySpace (
>> + BaseAddress,
>> + Length
>> + );
>> +
>> + Status = gDS->AddMemorySpace (
>> + EfiGcdMemoryTypeMemoryMappedIo,
>> + BaseAddress,
>> + Length,
>> + EFI_MEMORY_UC | EFI_MEMORY_RUNTIME
>> + );
>> + ASSERT_EFI_ERROR (Status);
>> +
>> + Status = gDS->AllocateMemorySpace (
>> + AllocateAddress,
>> + EfiGcdMemoryTypeMemoryMappedIo,
>> + 0,
>> + Length,
>> + &BaseAddress,
>> + gImageHandle,
>> + NULL
>> + );
>> + ASSERT_EFI_ERROR (Status);
>> +
>> + Status = gDS->GetMemorySpaceDescriptor (BaseAddress, &GcdDescriptor);
>> + ASSERT_EFI_ERROR (Status);
>> +
>> + Status = gDS->SetMemorySpaceAttributes (
>> + BaseAddress,
>> + Length,
>> + GcdDescriptor.Attributes | EFI_MEMORY_RUNTIME
>> + );
>> + ASSERT_EFI_ERROR (Status);
>> +
>> + return Status;
>> +}
>
> (3) Please don't forget to update this function (post-move) as I
> requested for patch #1.
>
>> diff --git a/OvmfPkg/QemuFlashFvbServicesRuntimeDxe/FwBlockServiceSmm.c b/OvmfPkg/QemuFlashFvbServicesRuntimeDxe/FwBlockServiceSmm.c
>> index e0617f2503a2..cdb073348158 100644
>> --- a/OvmfPkg/QemuFlashFvbServicesRuntimeDxe/FwBlockServiceSmm.c
>> +++ b/OvmfPkg/QemuFlashFvbServicesRuntimeDxe/FwBlockServiceSmm.c
>> @@ -67,3 +67,16 @@ InstallVirtualAddressChangeHandler (
>> // Nothing.
>> //
>> }
>> +
>> +EFI_STATUS
>> +MarkIoMemoryRangeForRuntimeAccess (
>> + EFI_PHYSICAL_ADDRESS BaseAddress,
>> + UINTN Length
>> + )
>> +{
>> + //
>> + // Nothing
>> + //
>> +
>> + return EFI_SUCCESS;
>> +}
>>
>
> Looks OK to me otherwise.
Sorry, got another remark:
(4) In "FwBlockService.h", you declare the function prototype -- very
correctly -- with the "IN" parameter decorators. Can you please add the
same "IN" decorators to both *definitions* of the function as well?
Thank you very much!
Laszlo
^ permalink raw reply [flat|nested] 9+ messages in thread
* [PATCH v3 3/3] OvmfPkg/QemuFlashFvbServicesRuntimeDxe: Restore C-bit when SEV is active
2018-07-05 19:12 [PATCH v3 0/3] OvmfPkg: mark flash memory range as MMIO Brijesh Singh
2018-07-05 19:12 ` [PATCH v3 1/3] OvmfPkg/QemuFlashFvbServicesRuntimeDxe: mark Flash " Brijesh Singh
2018-07-05 19:12 ` [PATCH v3 2/3] OvmfPkg/QemuFlashFvbServicesRuntimeDxe: Do not expose MMIO in SMM build Brijesh Singh
@ 2018-07-05 19:12 ` Brijesh Singh
2018-07-06 12:08 ` Laszlo Ersek
2018-07-06 14:01 ` [PATCH v3 0/3] OvmfPkg: mark flash memory range as MMIO Laszlo Ersek
3 siblings, 1 reply; 9+ messages in thread
From: Brijesh Singh @ 2018-07-05 19:12 UTC (permalink / raw)
To: edk2-devel
Cc: Lendacky Thomas, Brijesh Singh, Ard Biesheuvel, Anthony Perard,
Julien Grall, Justen Jordan L, Laszlo Ersek
AmdSevDxe maps the flash memory range with C=0, but
SetMemorySpaceAttributes() unconditionally resets the C-bit to '1'. Lets
restore the mapping back to C=0.
Cc: Ard Biesheuvel <ard.biesheuvel@linaro.org>
Cc: Anthony Perard <anthony.perard@citrix.com>
Cc: Julien Grall <julien.grall@linaro.org>
Cc: Justen Jordan L <jordan.l.justen@intel.com>
Cc: Laszlo Ersek <lersek@redhat.com>
Contributed-under: TianoCore Contribution Agreement 1.1
Signed-off-by: Brijesh Singh <brijesh.singh@amd.com>
---
.../FvbServicesRuntimeDxe.inf | 1 +
.../QemuFlashFvbServicesRuntimeDxe/FwBlockServiceDxe.c | 17 +++++++++++++++++
2 files changed, 18 insertions(+)
diff --git a/OvmfPkg/QemuFlashFvbServicesRuntimeDxe/FvbServicesRuntimeDxe.inf b/OvmfPkg/QemuFlashFvbServicesRuntimeDxe/FvbServicesRuntimeDxe.inf
index d7b4ec06c4e6..6bb5c2093790 100644
--- a/OvmfPkg/QemuFlashFvbServicesRuntimeDxe/FvbServicesRuntimeDxe.inf
+++ b/OvmfPkg/QemuFlashFvbServicesRuntimeDxe/FvbServicesRuntimeDxe.inf
@@ -54,6 +54,7 @@ [LibraryClasses]
DevicePathLib
DxeServicesTableLib
MemoryAllocationLib
+ MemEncryptSevLib
PcdLib
UefiBootServicesTableLib
UefiDriverEntryPoint
diff --git a/OvmfPkg/QemuFlashFvbServicesRuntimeDxe/FwBlockServiceDxe.c b/OvmfPkg/QemuFlashFvbServicesRuntimeDxe/FwBlockServiceDxe.c
index 646427bf4e2c..3add4bbad74c 100644
--- a/OvmfPkg/QemuFlashFvbServicesRuntimeDxe/FwBlockServiceDxe.c
+++ b/OvmfPkg/QemuFlashFvbServicesRuntimeDxe/FwBlockServiceDxe.c
@@ -24,6 +24,7 @@
#include <Protocol/FirmwareVolumeBlock.h>
#include <Library/DxeServicesTableLib.h>
#include <Library/MemoryAllocationLib.h>
+#include <Library/MemEncryptSevLib.h>
#include "FwBlockService.h"
#include "QemuFlash.h"
@@ -204,5 +205,21 @@ MarkIoMemoryRangeForRuntimeAccess (
);
ASSERT_EFI_ERROR (Status);
+ //
+ // When SEV is active, AmdSevDxe should have mapped the BaseAddress with
+ // C=0 but SetMemorySpaceAttribute() remap the range with C=1. Lets restore
+ // the mapping so that both guest and hyervisor can access the flash
+ // memory range.
+ //
+ if (MemEncryptSevIsEnabled()) {
+ Status = MemEncryptSevClearPageEncMask (
+ 0,
+ BaseAddress,
+ EFI_SIZE_TO_PAGES (Length),
+ FALSE
+ );
+ ASSERT_EFI_ERROR (Status);
+ }
+
return Status;
}
--
2.7.4
^ permalink raw reply related [flat|nested] 9+ messages in thread* Re: [PATCH v3 3/3] OvmfPkg/QemuFlashFvbServicesRuntimeDxe: Restore C-bit when SEV is active
2018-07-05 19:12 ` [PATCH v3 3/3] OvmfPkg/QemuFlashFvbServicesRuntimeDxe: Restore C-bit when SEV is active Brijesh Singh
@ 2018-07-06 12:08 ` Laszlo Ersek
0 siblings, 0 replies; 9+ messages in thread
From: Laszlo Ersek @ 2018-07-06 12:08 UTC (permalink / raw)
To: Brijesh Singh, edk2-devel
Cc: Lendacky Thomas, Ard Biesheuvel, Anthony Perard, Julien Grall,
Justen Jordan L
On 07/05/18 21:12, Brijesh Singh wrote:
> AmdSevDxe maps the flash memory range with C=0, but
> SetMemorySpaceAttributes() unconditionally resets the C-bit to '1'. Lets
> restore the mapping back to C=0.
>
> Cc: Ard Biesheuvel <ard.biesheuvel@linaro.org>
> Cc: Anthony Perard <anthony.perard@citrix.com>
> Cc: Julien Grall <julien.grall@linaro.org>
> Cc: Justen Jordan L <jordan.l.justen@intel.com>
> Cc: Laszlo Ersek <lersek@redhat.com>
> Contributed-under: TianoCore Contribution Agreement 1.1
> Signed-off-by: Brijesh Singh <brijesh.singh@amd.com>
> ---
> .../FvbServicesRuntimeDxe.inf | 1 +
> .../QemuFlashFvbServicesRuntimeDxe/FwBlockServiceDxe.c | 17 +++++++++++++++++
> 2 files changed, 18 insertions(+)
>
> diff --git a/OvmfPkg/QemuFlashFvbServicesRuntimeDxe/FvbServicesRuntimeDxe.inf b/OvmfPkg/QemuFlashFvbServicesRuntimeDxe/FvbServicesRuntimeDxe.inf
> index d7b4ec06c4e6..6bb5c2093790 100644
> --- a/OvmfPkg/QemuFlashFvbServicesRuntimeDxe/FvbServicesRuntimeDxe.inf
> +++ b/OvmfPkg/QemuFlashFvbServicesRuntimeDxe/FvbServicesRuntimeDxe.inf
> @@ -54,6 +54,7 @@ [LibraryClasses]
> DevicePathLib
> DxeServicesTableLib
> MemoryAllocationLib
> + MemEncryptSevLib
> PcdLib
> UefiBootServicesTableLib
> UefiDriverEntryPoint
(1) Please keep this list sorted.
(I realize I must have missed this dis-order when we added the
MemEncryptSevLib class to the "FvbServicesSmm.inf" file. If you want,
you can include a patch in v4 for fixing that order, but it's really not
necessary. Let's just not increase the dis-order if we can manage.)
> diff --git a/OvmfPkg/QemuFlashFvbServicesRuntimeDxe/FwBlockServiceDxe.c b/OvmfPkg/QemuFlashFvbServicesRuntimeDxe/FwBlockServiceDxe.c
> index 646427bf4e2c..3add4bbad74c 100644
> --- a/OvmfPkg/QemuFlashFvbServicesRuntimeDxe/FwBlockServiceDxe.c
> +++ b/OvmfPkg/QemuFlashFvbServicesRuntimeDxe/FwBlockServiceDxe.c
> @@ -24,6 +24,7 @@
> #include <Protocol/FirmwareVolumeBlock.h>
> #include <Library/DxeServicesTableLib.h>
> #include <Library/MemoryAllocationLib.h>
> +#include <Library/MemEncryptSevLib.h>
(2) Here too, please keep the Library #include list sorted.
>
> #include "FwBlockService.h"
> #include "QemuFlash.h"
> @@ -204,5 +205,21 @@ MarkIoMemoryRangeForRuntimeAccess (
> );
> ASSERT_EFI_ERROR (Status);
>
> + //
> + // When SEV is active, AmdSevDxe should have mapped the BaseAddress with
> + // C=0 but SetMemorySpaceAttribute() remap the range with C=1. Lets restore
> + // the mapping so that both guest and hyervisor can access the flash
> + // memory range.
> + //
(3) A couple of cleanups, please:
(3a) I suggest s/should have mapped/mapped/, because AmdSevDxe is really
not to blame here,
(3b) s/SetMemorySpaceAttribute/SetMemorySpaceAttributes/ (plural)
(3c) s/remap/remaps/
(3d) s/Lets/Let's/
(3e) Please check that, after the typo fixes, we are still under 80
chars per line.
> + if (MemEncryptSevIsEnabled()) {
(4) missing space before the "()"
> + Status = MemEncryptSevClearPageEncMask (
> + 0,
> + BaseAddress,
> + EFI_SIZE_TO_PAGES (Length),
> + FALSE
> + );
I'm glad that you had documented the "Flush" parameter earlier! :)
"Flush the caches before clearing the bit (mostly TRUE except MMIO
addresses)". So, this looks fine.
> + ASSERT_EFI_ERROR (Status);
> + }
> +
> return Status;
> }
>
I've requested a few changes, but none of those affect functionality in
practice. So, I will go ahead and collect some test results. I'll report
back.
Thanks!
Laszlo
^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [PATCH v3 0/3] OvmfPkg: mark flash memory range as MMIO
2018-07-05 19:12 [PATCH v3 0/3] OvmfPkg: mark flash memory range as MMIO Brijesh Singh
` (2 preceding siblings ...)
2018-07-05 19:12 ` [PATCH v3 3/3] OvmfPkg/QemuFlashFvbServicesRuntimeDxe: Restore C-bit when SEV is active Brijesh Singh
@ 2018-07-06 14:01 ` Laszlo Ersek
3 siblings, 0 replies; 9+ messages in thread
From: Laszlo Ersek @ 2018-07-06 14:01 UTC (permalink / raw)
To: Brijesh Singh, edk2-devel
Cc: Lendacky Thomas, Ard Biesheuvel, Anthony Perard, Julien Grall,
Justen Jordan L
On 07/05/18 21:12, Brijesh Singh wrote:
> The Qemu flash range is marked as 'system ram' in EFI runtime memmap
> but it is actually an IO address. The patch series updates the
> EFI runtime memmap to add this range as Memory Mapped IO address.
>
> Changes since v2:
> - added cover letter and extend CC list to add all OvmfPkg
> maintainers/reviewers.
>
> Changes since v1:
> - split the OvmfPkg single patch into three patches based on Laszlo's feedback.
> a) mark memory as MMIO for non SEV case
> b) do not adding EFI runtime mapping for SMM build
> c) clear C-bit when SEV is active for non SMM builds only
>
> Cc: Ard Biesheuvel <ard.biesheuvel@linaro.org>
> Cc: Anthony Perard <anthony.perard@citrix.com>
> Cc: Julien Grall <julien.grall@linaro.org>
> Cc: Justen Jordan L <jordan.l.justen@intel.com>
> Cc: Laszlo Ersek <lersek@redhat.com>
> Contributed-under: TianoCore Contribution Agreement 1.1
> Signed-off-by: Brijesh Singh <brijesh.singh@amd.com>
>
> Complete tree is available:
> url: https://github.com/codomania/edk2.git
> branch: qemu-flash-mmio
>
> Brijesh Singh (3):
> OvmfPkg/QemuFlashFvbServicesRuntimeDxe: mark Flash memory range as
> MMIO
> OvmfPkg/QemuFlashFvbServicesRuntimeDxe: Do not expose MMIO in SMM
> build
> OvmfPkg/QemuFlashFvbServicesRuntimeDxe: Restore C-bit when SEV is
> active
>
> .../FvbServicesRuntimeDxe.inf | 1 +
> .../FwBlockService.c | 38 +-----------
> .../FwBlockService.h | 7 +++
> .../FwBlockServiceDxe.c | 68 ++++++++++++++++++++++
> .../FwBlockServiceSmm.c | 13 +++++
> 5 files changed, 90 insertions(+), 37 deletions(-)
>
Regression tested -- only without SEV -- on top of commit 9090c8b53301,
using the configs below.
The "rdmsr" and "efibootmgr" checks are explained at
<https://github.com/tianocore/tianocore.github.io/wiki/Testing-SMM-with-QEMU,-KVM-and-libvirt>.
* i440fx, X64, without SMM:
(01) Fedora 28 guest, efibootmgr + rdmsr checks after boot and S3: PASS
(02) RHEL 6 guest, efibootmgr check after boot: PASS
(03) RHEL 7 guest, efibootmgr check after boot and S3: PASS
(04) Windows 7 guest, boot and S3: PASS
* q35, IA32, with SMM:
(05) Fedora 26 guest, efibootmgr check after boot and S3: PASS
* q35, IA32X64, with SMM:
(06) Fedora 28 guest, efibootmgr + rdmsr checks after boot and S3: PASS
(07) RHEL 7 guest, efibootmgr check after boot and S3: PASS
(08) Windows 10 guest, boot and S3: PASS
(09) Windows Server 2016 guest, boot: PASS
(can't test S3 due to signing requirements for the QXL driver)
(10) Windows 8.1 guest, boot and S3: PASS
(11) Windows Server 2012 R2 guest, boot and S3: PASS
(12) Windows 7 guest, boot and S3: PASS
(13) Windows Server 2008 R2 guest, boot and S3: PASS
Once you submit v4 with the superficial updates requested, I will add:
Regression-tested-by: Laszlo Ersek <lersek@redhat.com>
I'll leave the SEV testing (with and without SMM) up to you :)
Thanks!
Laszlo
^ permalink raw reply [flat|nested] 9+ messages in thread