* [PATCH 00/12] OvmfPkg/IoMmuDxe: cleanups and fixes
@ 2017-08-02 21:24 Laszlo Ersek
2017-08-02 21:24 ` [PATCH 01/12] OvmfPkg/IoMmuDxe: rewrap source code to 79 characters Laszlo Ersek
` (13 more replies)
0 siblings, 14 replies; 22+ messages in thread
From: Laszlo Ersek @ 2017-08-02 21:24 UTC (permalink / raw)
To: edk2-devel-01; +Cc: Ard Biesheuvel, Brijesh Singh, Jordan Justen, Tom Lendacky
This series is proposed as a replacement (or a replacement "basis") for
patches #1 through #3 of Brijesh's series
[PATCH v1 0/4] OvmfPkg : IoMmuDxe: BusMasterCommonBuffer support when
SEV is active
http://mid.mail-archive.com/1501529474-20550-1-git-send-email-brijesh.singh@amd.com
Patch #4 of the same series ("OvmfPkg : QemuFwCfgLib: Map DMA buffer
with CommonBuffer when SEV is enable") is required on top of this
series; otherwise QemuFwCfgLib will break on SEV.
In the present series, patches #1 through #7 are lightweight
improvements for OvmfPkg/IoMmuDxe, concerning line width, MAP_INFO field
names, conversion specifiers for DEBUG(), coding style, error
propagation, and library class listings.
Patch #8 ("zero out pages before releasing them") fixes the "information
leak" issue pointed out in:
http://mid.mail-archive.com/e1137140-f5d5-7eb8-0162-952694b24f96@redhat.com
Patch #9 ('rework setup of "MapInfo->PlainTextAddress" in Map()') fixes
as-yet undiscussed issues, and lays the groundwork for patch #10, by
reworking the calculation of the plaintext buffer address.
Patch #10 ("implement in-place decryption/encryption for Map/Unmap")
fixes the issues around BusMasterCommonBuffer[64] operations that were
discussed in the following messages:
http://mid.mail-archive.com/4071596d-32c9-e6d9-8c93-0d43d28e9b5a@redhat.com
http://mid.mail-archive.com/e1137140-f5d5-7eb8-0162-952694b24f96@redhat.com
http://mid.mail-archive.com/84c3c5db-623e-181b-c472-7fd7ae1c1670@amd.com
http://mid.mail-archive.com/89e1553a-1630-87a5-cffd-99174a380d41@redhat.com
Patch #11 ("abort harder on memory encryption mask failures") settles
the error handling for MemEncryptSevClearPageEncMask() and
MemEncryptSevSetPageEncMask(), discussed in:
http://mid.mail-archive.com/89e1553a-1630-87a5-cffd-99174a380d41@redhat.com
Patch #12 ("Unmap(): recycle MAP_INFO after BusMasterCommonBuffer[64]")
implements the "free list" proposed in:
http://mid.mail-archive.com/e1137140-f5d5-7eb8-0162-952694b24f96@redhat.com
The series has been formatted with "--function-context", for easier
review.
Repo: https://github.com/lersek/edk2.git
Branch: amdsev_iommu_cleanups_fixes
Cc: Ard Biesheuvel <ard.biesheuvel@linaro.org>
Cc: Brijesh Singh <brijesh.singh@amd.com>
Cc: Jordan Justen <jordan.l.justen@intel.com>
Cc: Tom Lendacky <thomas.lendacky@amd.com>
Thanks
Laszlo
Laszlo Ersek (12):
OvmfPkg/IoMmuDxe: rewrap source code to 79 characters
OvmfPkg/IoMmuDxe: rename DeviceAddress to PlainTextAddress in MAP_INFO
OvmfPkg/IoMmuDxe: rename HostAddress to CryptedAddress in MAP_INFO
OvmfPkg/IoMmuDxe: convert UINTN arguments to UINT64 for the %Lx fmt
spec
OvmfPkg/IoMmuDxe: don't initialize local variables
OvmfPkg/IoMmuDxe: propagate errors from AmdSevInstallIoMmuProtocol()
OvmfPkg/IoMmuDxe: clean up used library classes
OvmfPkg/IoMmuDxe: zero out pages before releasing them
OvmfPkg/IoMmuDxe: rework setup of "MapInfo->PlainTextAddress" in Map()
OvmfPkg/IoMmuDxe: implement in-place decryption/encryption for
Map/Unmap
OvmfPkg/IoMmuDxe: abort harder on memory encryption mask failures
OvmfPkg/IoMmuDxe: Unmap(): recycle MAP_INFO after
BusMasterCommonBuffer[64]
OvmfPkg/IoMmuDxe/IoMmuDxe.inf | 19 +-
OvmfPkg/IoMmuDxe/AmdSevIoMmu.h | 14 +-
OvmfPkg/IoMmuDxe/AmdSevIoMmu.c | 594 ++++++++++++++------
OvmfPkg/IoMmuDxe/IoMmuDxe.c | 25 +-
4 files changed, 447 insertions(+), 205 deletions(-)
--
2.13.1.3.g8be5a757fa67
^ permalink raw reply [flat|nested] 22+ messages in thread
* [PATCH 01/12] OvmfPkg/IoMmuDxe: rewrap source code to 79 characters
2017-08-02 21:24 [PATCH 00/12] OvmfPkg/IoMmuDxe: cleanups and fixes Laszlo Ersek
@ 2017-08-02 21:24 ` Laszlo Ersek
2017-08-02 21:24 ` [PATCH 02/12] OvmfPkg/IoMmuDxe: rename DeviceAddress to PlainTextAddress in MAP_INFO Laszlo Ersek
` (12 subsequent siblings)
13 siblings, 0 replies; 22+ messages in thread
From: Laszlo Ersek @ 2017-08-02 21:24 UTC (permalink / raw)
To: edk2-devel-01; +Cc: Ard Biesheuvel, Brijesh Singh, Jordan Justen, Tom Lendacky
No functional changes.
Cc: Ard Biesheuvel <ard.biesheuvel@linaro.org>
Cc: Brijesh Singh <brijesh.singh@amd.com>
Cc: Jordan Justen <jordan.l.justen@intel.com>
Cc: Tom Lendacky <thomas.lendacky@amd.com>
Contributed-under: TianoCore Contribution Agreement 1.0
Signed-off-by: Laszlo Ersek <lersek@redhat.com>
---
OvmfPkg/IoMmuDxe/IoMmuDxe.inf | 11 +-
OvmfPkg/IoMmuDxe/AmdSevIoMmu.h | 8 +-
OvmfPkg/IoMmuDxe/AmdSevIoMmu.c | 173 +++++++++++++-------
OvmfPkg/IoMmuDxe/IoMmuDxe.c | 8 +-
4 files changed, 130 insertions(+), 70 deletions(-)
diff --git a/OvmfPkg/IoMmuDxe/IoMmuDxe.inf b/OvmfPkg/IoMmuDxe/IoMmuDxe.inf
index b90dc80dfd37..21dc39b9233a 100644
--- a/OvmfPkg/IoMmuDxe/IoMmuDxe.inf
+++ b/OvmfPkg/IoMmuDxe/IoMmuDxe.inf
@@ -1,17 +1,18 @@
#/** @file
#
# Driver provides the IOMMU protcol support for PciHostBridgeIo and others
# drivers.
#
# Copyright (c) 2017, AMD Inc. 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 distribution. The full text of the license may
-# be found at http://opensource.org/licenses/bsd-license.php
+# This program and the accompanying materials are licensed and made available
+# under the terms and conditions of the BSD License which accompanies this
+# distribution. The full text of the license may be found at
+# http://opensource.org/licenses/bsd-license.php
#
# THE PROGRAM IS DISTRIBUTED UNDER THE BSD LICENSE ON AN "AS IS" BASIS,
-# WITHOUT WARRANTIES OR REPRESENTATIONS OF ANY KIND, EITHER EXPRESS OR IMPLIED.
+# WITHOUT WARRANTIES OR REPRESENTATIONS OF ANY KIND, EITHER EXPRESS OR
+# IMPLIED.
#
#**/
diff --git a/OvmfPkg/IoMmuDxe/AmdSevIoMmu.h b/OvmfPkg/IoMmuDxe/AmdSevIoMmu.h
index 8b3962a8c395..88dabfc2c435 100644
--- a/OvmfPkg/IoMmuDxe/AmdSevIoMmu.h
+++ b/OvmfPkg/IoMmuDxe/AmdSevIoMmu.h
@@ -1,36 +1,36 @@
/** @file
- The protocol provides support to allocate, free, map and umap a DMA buffer for
- bus master (e.g PciHostBridge). When SEV is enabled, the DMA operations must
- be performed on unencrypted buffer hence protocol clear the encryption bit
- from the DMA buffer.
+ The protocol provides support to allocate, free, map and umap a DMA buffer
+ for bus master (e.g PciHostBridge). When SEV is enabled, the DMA operations
+ must be performed on unencrypted buffer hence protocol clear the encryption
+ bit from the DMA buffer.
Copyright (c) 2017, Intel Corporation. All rights reserved.<BR>
Copyright (c) 2017, AMD Inc. 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
distribution. The full text of the license may be found at
http://opensource.org/licenses/bsd-license.php
THE PROGRAM IS DISTRIBUTED UNDER THE BSD LICENSE ON AN "AS IS" BASIS,
WITHOUT WARRANTIES OR REPRESENTATIONS OF ANY KIND, EITHER EXPRESS OR IMPLIED.
**/
#ifndef __AMD_SEV_IOMMU_H_
#define __AMD_SEV_IOMMU_H
#include <Protocol/IoMmu.h>
#include <Library/BaseLib.h>
#include <Library/DebugLib.h>
#include <Library/BaseMemoryLib.h>
#include <Library/MemoryAllocationLib.h>
#include <Library/UefiBootServicesTableLib.h>
#include <Library/MemEncryptSevLib.h>
/**
Install IOMMU protocol to provide the DMA support for PciHostBridge and
MemEncryptSevLib.
**/
diff --git a/OvmfPkg/IoMmuDxe/AmdSevIoMmu.c b/OvmfPkg/IoMmuDxe/AmdSevIoMmu.c
index 9e78058b7242..edef0f41eecc 100644
--- a/OvmfPkg/IoMmuDxe/AmdSevIoMmu.c
+++ b/OvmfPkg/IoMmuDxe/AmdSevIoMmu.c
@@ -1,21 +1,21 @@
/** @file
- The protocol provides support to allocate, free, map and umap a DMA buffer for
- bus master (e.g PciHostBridge). When SEV is enabled, the DMA operations must
- be performed on unencrypted buffer hence we use a bounce buffer to map the guest
- buffer into an unencrypted DMA buffer.
+ The protocol provides support to allocate, free, map and umap a DMA buffer
+ for bus master (e.g PciHostBridge). When SEV is enabled, the DMA operations
+ must be performed on unencrypted buffer hence we use a bounce buffer to map
+ the guest buffer into an unencrypted DMA buffer.
Copyright (c) 2017, AMD Inc. All rights reserved.<BR>
Copyright (c) 2017, Intel Corporation. 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
distribution. The full text of the license may be found at
http://opensource.org/licenses/bsd-license.php
THE PROGRAM IS DISTRIBUTED UNDER THE BSD LICENSE ON AN "AS IS" BASIS,
WITHOUT WARRANTIES OR REPRESENTATIONS OF ANY KIND, EITHER EXPRESS OR IMPLIED.
**/
#include "AmdSevIoMmu.h"
@@ -23,37 +23,41 @@
typedef struct {
EDKII_IOMMU_OPERATION Operation;
UINTN NumberOfBytes;
UINTN NumberOfPages;
EFI_PHYSICAL_ADDRESS HostAddress;
EFI_PHYSICAL_ADDRESS DeviceAddress;
} MAP_INFO;
#define NO_MAPPING (VOID *) (UINTN) -1
/**
- Provides the controller-specific addresses required to access system memory from a
- DMA bus master. On SEV guest, the DMA operations must be performed on shared
- buffer hence we allocate a bounce buffer to map the HostAddress to a DeviceAddress.
- The Encryption attribute is removed from the DeviceAddress buffer.
+ Provides the controller-specific addresses required to access system memory
+ from a DMA bus master. On SEV guest, the DMA operations must be performed on
+ shared buffer hence we allocate a bounce buffer to map the HostAddress to a
+ DeviceAddress. The Encryption attribute is removed from the DeviceAddress
+ buffer.
@param This The protocol instance pointer.
@param Operation Indicates if the bus master is going to read or
write to system memory.
- @param HostAddress The system memory address to map to the PCI controller.
+ @param HostAddress The system memory address to map to the PCI
+ controller.
@param NumberOfBytes On input the number of bytes to map. On output
- the number of bytes
- that were mapped.
- @param DeviceAddress The resulting map address for the bus master PCI
- controller to use to
- access the hosts HostAddress.
+ the number of bytes that were mapped.
+ @param DeviceAddress The resulting map address for the bus master
+ PCI controller to use to access the hosts
+ HostAddress.
@param Mapping A resulting value to pass to Unmap().
- @retval EFI_SUCCESS The range was mapped for the returned NumberOfBytes.
- @retval EFI_UNSUPPORTED The HostAddress cannot be mapped as a common buffer.
+ @retval EFI_SUCCESS The range was mapped for the returned
+ NumberOfBytes.
+ @retval EFI_UNSUPPORTED The HostAddress cannot be mapped as a common
+ buffer.
@retval EFI_INVALID_PARAMETER One or more parameters are invalid.
- @retval EFI_OUT_OF_RESOURCES The request could not be completed due to a lack
- of resources.
- @retval EFI_DEVICE_ERROR The system hardware could not map the requested address.
+ @retval EFI_OUT_OF_RESOURCES The request could not be completed due to a
+ lack of resources.
+ @retval EFI_DEVICE_ERROR The system hardware could not map the requested
+ address.
**/
EFI_STATUS
@@ -61,223 +65,249 @@ EFIAPI
IoMmuMap (
IN EDKII_IOMMU_PROTOCOL *This,
IN EDKII_IOMMU_OPERATION Operation,
IN VOID *HostAddress,
IN OUT UINTN *NumberOfBytes,
OUT EFI_PHYSICAL_ADDRESS *DeviceAddress,
OUT VOID **Mapping
)
{
EFI_STATUS Status;
EFI_PHYSICAL_ADDRESS PhysicalAddress;
MAP_INFO *MapInfo;
EFI_PHYSICAL_ADDRESS DmaMemoryTop;
EFI_ALLOCATE_TYPE AllocateType;
if (HostAddress == NULL || NumberOfBytes == NULL || DeviceAddress == NULL ||
Mapping == NULL) {
return EFI_INVALID_PARAMETER;
}
//
// Make sure that Operation is valid
//
if ((UINT32) Operation >= EdkiiIoMmuOperationMaximum) {
return EFI_INVALID_PARAMETER;
}
PhysicalAddress = (EFI_PHYSICAL_ADDRESS) (UINTN) HostAddress;
DmaMemoryTop = (UINTN)-1;
AllocateType = AllocateAnyPages;
if (((Operation != EdkiiIoMmuOperationBusMasterRead64 &&
Operation != EdkiiIoMmuOperationBusMasterWrite64 &&
Operation != EdkiiIoMmuOperationBusMasterCommonBuffer64)) &&
((PhysicalAddress + *NumberOfBytes) > SIZE_4GB)) {
//
// If the root bridge or the device cannot handle performing DMA above
// 4GB but any part of the DMA transfer being mapped is above 4GB, then
// map the DMA transfer to a buffer below 4GB.
//
DmaMemoryTop = SIZE_4GB - 1;
AllocateType = AllocateMaxAddress;
if (Operation == EdkiiIoMmuOperationBusMasterCommonBuffer ||
Operation == EdkiiIoMmuOperationBusMasterCommonBuffer64) {
//
// Common Buffer operations can not be remapped. If the common buffer
- // if above 4GB, then it is not possible to generate a mapping, so return
- // an error.
+ // if above 4GB, then it is not possible to generate a mapping, so
+ // return an error.
//
return EFI_UNSUPPORTED;
}
}
//
// CommandBuffer was allocated by us (AllocateBuffer) and is already in
// unencryted buffer so no need to create bounce buffer
//
if (Operation == EdkiiIoMmuOperationBusMasterCommonBuffer ||
Operation == EdkiiIoMmuOperationBusMasterCommonBuffer64) {
*Mapping = NO_MAPPING;
*DeviceAddress = PhysicalAddress;
return EFI_SUCCESS;
}
//
// Allocate a MAP_INFO structure to remember the mapping when Unmap() is
// called later.
//
MapInfo = AllocatePool (sizeof (MAP_INFO));
if (MapInfo == NULL) {
*NumberOfBytes = 0;
return EFI_OUT_OF_RESOURCES;
}
//
// Initialize the MAP_INFO structure
//
MapInfo->Operation = Operation;
MapInfo->NumberOfBytes = *NumberOfBytes;
MapInfo->NumberOfPages = EFI_SIZE_TO_PAGES (MapInfo->NumberOfBytes);
MapInfo->HostAddress = PhysicalAddress;
MapInfo->DeviceAddress = DmaMemoryTop;
//
// Allocate a buffer to map the transfer to.
//
Status = gBS->AllocatePages (
AllocateType,
EfiBootServicesData,
MapInfo->NumberOfPages,
&MapInfo->DeviceAddress
);
if (EFI_ERROR (Status)) {
FreePool (MapInfo);
*NumberOfBytes = 0;
return Status;
}
//
// Clear the memory encryption mask from the device buffer
//
- Status = MemEncryptSevClearPageEncMask (0, MapInfo->DeviceAddress, MapInfo->NumberOfPages, TRUE);
+ Status = MemEncryptSevClearPageEncMask (
+ 0,
+ MapInfo->DeviceAddress,
+ MapInfo->NumberOfPages,
+ TRUE
+ );
ASSERT_EFI_ERROR(Status);
//
// If this is a read operation from the Bus Master's point of view,
// then copy the contents of the real buffer into the mapped buffer
// so the Bus Master can read the contents of the real buffer.
//
if (Operation == EdkiiIoMmuOperationBusMasterRead ||
Operation == EdkiiIoMmuOperationBusMasterRead64) {
CopyMem (
(VOID *) (UINTN) MapInfo->DeviceAddress,
(VOID *) (UINTN) MapInfo->HostAddress,
MapInfo->NumberOfBytes
);
}
//
// The DeviceAddress is the address of the maped buffer below 4GB
//
*DeviceAddress = MapInfo->DeviceAddress;
//
// Return a pointer to the MAP_INFO structure in Mapping
//
*Mapping = MapInfo;
- DEBUG ((DEBUG_VERBOSE, "%a Device 0x%Lx Host 0x%Lx Pages 0x%Lx Bytes 0x%Lx\n",
- __FUNCTION__, MapInfo->DeviceAddress, MapInfo->HostAddress,
- MapInfo->NumberOfPages, MapInfo->NumberOfBytes));
+ DEBUG ((
+ DEBUG_VERBOSE,
+ "%a Device 0x%Lx Host 0x%Lx Pages 0x%Lx Bytes 0x%Lx\n",
+ __FUNCTION__,
+ MapInfo->DeviceAddress,
+ MapInfo->HostAddress,
+ MapInfo->NumberOfPages,
+ MapInfo->NumberOfBytes
+ ));
return EFI_SUCCESS;
}
/**
Completes the Map() operation and releases any corresponding resources.
@param This The protocol instance pointer.
@param Mapping The mapping value returned from Map().
@retval EFI_SUCCESS The range was unmapped.
- @retval EFI_INVALID_PARAMETER Mapping is not a value that was returned by Map().
- @retval EFI_DEVICE_ERROR The data was not committed to the target system memory.
+ @retval EFI_INVALID_PARAMETER Mapping is not a value that was returned by
+ Map().
+ @retval EFI_DEVICE_ERROR The data was not committed to the target system
+ memory.
**/
EFI_STATUS
EFIAPI
IoMmuUnmap (
IN EDKII_IOMMU_PROTOCOL *This,
IN VOID *Mapping
)
{
MAP_INFO *MapInfo;
EFI_STATUS Status;
if (Mapping == NULL) {
return EFI_INVALID_PARAMETER;
}
//
// See if the Map() operation associated with this Unmap() required a mapping
// buffer. If a mapping buffer was not required, then this function simply
// buffer. If a mapping buffer was not required, then this function simply
//
if (Mapping == NO_MAPPING) {
return EFI_SUCCESS;
}
MapInfo = (MAP_INFO *)Mapping;
//
// If this is a write operation from the Bus Master's point of view,
// then copy the contents of the mapped buffer into the real buffer
// so the processor can read the contents of the real buffer.
//
if (MapInfo->Operation == EdkiiIoMmuOperationBusMasterWrite ||
MapInfo->Operation == EdkiiIoMmuOperationBusMasterWrite64) {
CopyMem (
(VOID *) (UINTN) MapInfo->HostAddress,
(VOID *) (UINTN) MapInfo->DeviceAddress,
MapInfo->NumberOfBytes
);
}
- DEBUG ((DEBUG_VERBOSE, "%a Device 0x%Lx Host 0x%Lx Pages 0x%Lx Bytes 0x%Lx\n",
- __FUNCTION__, MapInfo->DeviceAddress, MapInfo->HostAddress,
- MapInfo->NumberOfPages, MapInfo->NumberOfBytes));
+ DEBUG ((
+ DEBUG_VERBOSE,
+ "%a Device 0x%Lx Host 0x%Lx Pages 0x%Lx Bytes 0x%Lx\n",
+ __FUNCTION__,
+ MapInfo->DeviceAddress,
+ MapInfo->HostAddress,
+ MapInfo->NumberOfPages,
+ MapInfo->NumberOfBytes
+ ));
//
// Restore the memory encryption mask
//
- Status = MemEncryptSevSetPageEncMask (0, MapInfo->DeviceAddress, MapInfo->NumberOfPages, TRUE);
+ Status = MemEncryptSevSetPageEncMask (
+ 0,
+ MapInfo->DeviceAddress,
+ MapInfo->NumberOfPages,
+ TRUE
+ );
ASSERT_EFI_ERROR(Status);
//
// Free the mapped buffer and the MAP_INFO structure.
//
gBS->FreePages (MapInfo->DeviceAddress, MapInfo->NumberOfPages);
FreePool (Mapping);
return EFI_SUCCESS;
}
/**
Allocates pages that are suitable for an OperationBusMasterCommonBuffer or
OperationBusMasterCommonBuffer64 mapping.
@param This The protocol instance pointer.
@param Type This parameter is not used and must be ignored.
- @param MemoryType The type of memory to allocate, EfiBootServicesData
- or EfiRuntimeServicesData.
+ @param MemoryType The type of memory to allocate,
+ EfiBootServicesData or EfiRuntimeServicesData.
@param Pages The number of pages to allocate.
- @param HostAddress A pointer to store the base system memory address
- of the allocated range.
- @param Attributes The requested bit mask of attributes for the allocated range.
+ @param HostAddress A pointer to store the base system memory
+ address of the allocated range.
+ @param Attributes The requested bit mask of attributes for the
+ allocated range.
@retval EFI_SUCCESS The requested memory pages were allocated.
- @retval EFI_UNSUPPORTED Attributes is unsupported. The only legal attribute
- bits are MEMORY_WRITE_COMBINE and MEMORY_CACHED.
+ @retval EFI_UNSUPPORTED Attributes is unsupported. The only legal
+ attribute bits are MEMORY_WRITE_COMBINE and
+ MEMORY_CACHED.
@retval EFI_INVALID_PARAMETER One or more parameters are invalid.
@retval EFI_OUT_OF_RESOURCES The memory pages could not be allocated.
**/
@@ -286,75 +316,82 @@ EFIAPI
IoMmuAllocateBuffer (
IN EDKII_IOMMU_PROTOCOL *This,
IN EFI_ALLOCATE_TYPE Type,
IN EFI_MEMORY_TYPE MemoryType,
IN UINTN Pages,
IN OUT VOID **HostAddress,
IN UINT64 Attributes
)
{
EFI_STATUS Status;
EFI_PHYSICAL_ADDRESS PhysicalAddress;
//
// Validate Attributes
//
if ((Attributes & EDKII_IOMMU_ATTRIBUTE_INVALID_FOR_ALLOCATE_BUFFER) != 0) {
return EFI_UNSUPPORTED;
}
//
// Check for invalid inputs
//
if (HostAddress == NULL) {
return EFI_INVALID_PARAMETER;
}
//
// The only valid memory types are EfiBootServicesData and
// EfiRuntimeServicesData
//
if (MemoryType != EfiBootServicesData &&
MemoryType != EfiRuntimeServicesData) {
return EFI_INVALID_PARAMETER;
}
PhysicalAddress = (UINTN)-1;
if ((Attributes & EDKII_IOMMU_ATTRIBUTE_DUAL_ADDRESS_CYCLE) == 0) {
//
// Limit allocations to memory below 4GB
//
PhysicalAddress = SIZE_4GB - 1;
}
Status = gBS->AllocatePages (
AllocateMaxAddress,
MemoryType,
Pages,
&PhysicalAddress
);
if (!EFI_ERROR (Status)) {
*HostAddress = (VOID *) (UINTN) PhysicalAddress;
//
// Clear memory encryption mask
//
Status = MemEncryptSevClearPageEncMask (0, PhysicalAddress, Pages, TRUE);
ASSERT_EFI_ERROR(Status);
}
- DEBUG ((DEBUG_VERBOSE, "%a Address 0x%Lx Pages 0x%Lx\n", __FUNCTION__, PhysicalAddress, Pages));
+ DEBUG ((
+ DEBUG_VERBOSE,
+ "%a Address 0x%Lx Pages 0x%Lx\n",
+ __FUNCTION__,
+ PhysicalAddress,
+ Pages
+ ));
return Status;
}
/**
Frees memory that was allocated with AllocateBuffer().
@param This The protocol instance pointer.
@param Pages The number of pages to free.
- @param HostAddress The base system memory address of the allocated range.
+ @param HostAddress The base system memory address of the allocated
+ range.
@retval EFI_SUCCESS The requested memory pages were freed.
- @retval EFI_INVALID_PARAMETER The memory range specified by HostAddress and Pages
- was not allocated with AllocateBuffer().
+ @retval EFI_INVALID_PARAMETER The memory range specified by HostAddress and
+ Pages was not allocated with AllocateBuffer().
**/
EFI_STATUS
@@ -362,57 +399,79 @@ EFIAPI
IoMmuFreeBuffer (
IN EDKII_IOMMU_PROTOCOL *This,
IN UINTN Pages,
IN VOID *HostAddress
)
{
EFI_STATUS Status;
//
// Set memory encryption mask
//
- Status = MemEncryptSevSetPageEncMask (0, (EFI_PHYSICAL_ADDRESS)(UINTN)HostAddress, Pages, TRUE);
+ Status = MemEncryptSevSetPageEncMask (
+ 0,
+ (EFI_PHYSICAL_ADDRESS)(UINTN)HostAddress,
+ Pages,
+ TRUE
+ );
ASSERT_EFI_ERROR(Status);
- DEBUG ((DEBUG_VERBOSE, "%a Address 0x%Lx Pages 0x%Lx\n", __FUNCTION__, (UINTN)HostAddress, Pages));
+ DEBUG ((
+ DEBUG_VERBOSE,
+ "%a Address 0x%Lx Pages 0x%Lx\n",
+ __FUNCTION__,
+ (UINTN)HostAddress,
+ Pages
+ ));
return gBS->FreePages ((EFI_PHYSICAL_ADDRESS) (UINTN) HostAddress, Pages);
}
/**
Set IOMMU attribute for a system memory.
If the IOMMU protocol exists, the system memory cannot be used
for DMA by default.
When a device requests a DMA access for a system memory,
the device driver need use SetAttribute() to update the IOMMU
attribute to request DMA access (read and/or write).
The DeviceHandle is used to identify which device submits the request.
- The IOMMU implementation need translate the device path to an IOMMU device ID,
- and set IOMMU hardware register accordingly.
+ The IOMMU implementation need translate the device path to an IOMMU device
+ ID, and set IOMMU hardware register accordingly.
1) DeviceHandle can be a standard PCI device.
The memory for BusMasterRead need set EDKII_IOMMU_ACCESS_READ.
The memory for BusMasterWrite need set EDKII_IOMMU_ACCESS_WRITE.
- The memory for BusMasterCommonBuffer need set EDKII_IOMMU_ACCESS_READ|EDKII_IOMMU_ACCESS_WRITE.
- After the memory is used, the memory need set 0 to keep it being protected.
+ The memory for BusMasterCommonBuffer need set
+ EDKII_IOMMU_ACCESS_READ|EDKII_IOMMU_ACCESS_WRITE.
+ After the memory is used, the memory need set 0 to keep it being
+ protected.
2) DeviceHandle can be an ACPI device (ISA, I2C, SPI, etc).
- The memory for DMA access need set EDKII_IOMMU_ACCESS_READ and/or EDKII_IOMMU_ACCESS_WRITE.
+ The memory for DMA access need set EDKII_IOMMU_ACCESS_READ and/or
+ EDKII_IOMMU_ACCESS_WRITE.
@param[in] This The protocol instance pointer.
- @param[in] DeviceHandle The device who initiates the DMA access request.
+ @param[in] DeviceHandle The device who initiates the DMA access
+ request.
@param[in] Mapping The mapping value returned from Map().
@param[in] IoMmuAccess The IOMMU access.
- @retval EFI_SUCCESS The IoMmuAccess is set for the memory range specified by DeviceAddress and Length.
+ @retval EFI_SUCCESS The IoMmuAccess is set for the memory range
+ specified by DeviceAddress and Length.
@retval EFI_INVALID_PARAMETER DeviceHandle is an invalid handle.
- @retval EFI_INVALID_PARAMETER Mapping is not a value that was returned by Map().
- @retval EFI_INVALID_PARAMETER IoMmuAccess specified an illegal combination of access.
+ @retval EFI_INVALID_PARAMETER Mapping is not a value that was returned by
+ Map().
+ @retval EFI_INVALID_PARAMETER IoMmuAccess specified an illegal combination
+ of access.
@retval EFI_UNSUPPORTED DeviceHandle is unknown by the IOMMU.
- @retval EFI_UNSUPPORTED The bit mask of IoMmuAccess is not supported by the IOMMU.
- @retval EFI_UNSUPPORTED The IOMMU does not support the memory range specified by Mapping.
- @retval EFI_OUT_OF_RESOURCES There are not enough resources available to modify the IOMMU access.
- @retval EFI_DEVICE_ERROR The IOMMU device reported an error while attempting the operation.
+ @retval EFI_UNSUPPORTED The bit mask of IoMmuAccess is not supported
+ by the IOMMU.
+ @retval EFI_UNSUPPORTED The IOMMU does not support the memory range
+ specified by Mapping.
+ @retval EFI_OUT_OF_RESOURCES There are not enough resources available to
+ modify the IOMMU access.
+ @retval EFI_DEVICE_ERROR The IOMMU device reported an error while
+ attempting the operation.
**/
EFI_STATUS
diff --git a/OvmfPkg/IoMmuDxe/IoMmuDxe.c b/OvmfPkg/IoMmuDxe/IoMmuDxe.c
index 101157e228b3..5809afc44196 100644
--- a/OvmfPkg/IoMmuDxe/IoMmuDxe.c
+++ b/OvmfPkg/IoMmuDxe/IoMmuDxe.c
@@ -1,27 +1,27 @@
/** @file
IoMmuDxe driver installs EDKII_IOMMU_PROTOCOL to provide the support for DMA
operations when SEV is enabled.
Copyright (c) 2017, AMD Inc. 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 distribution. The full text of the license may
- be found at http://opensource.org/licenses/bsd-license.php
+ This program and the accompanying materials are licensed and made available
+ under the terms and conditions of the BSD License which accompanies this
+ distribution. The full text of the license may be found at
+ http://opensource.org/licenses/bsd-license.php
THE PROGRAM IS DISTRIBUTED UNDER THE BSD LICENSE ON AN "AS IS" BASIS,
WITHOUT WARRANTIES OR REPRESENTATIONS OF ANY KIND, EITHER EXPRESS OR IMPLIED.
**/
#include <PiDxe.h>
#include <Library/BaseLib.h>
#include <Library/DebugLib.h>
#include <Library/BaseMemoryLib.h>
#include <Library/MemoryAllocationLib.h>
#include <Library/UefiBootServicesTableLib.h>
#include <Library/MemEncryptSevLib.h>
#include "AmdSevIoMmu.h"
--
2.13.1.3.g8be5a757fa67
^ permalink raw reply related [flat|nested] 22+ messages in thread
* [PATCH 02/12] OvmfPkg/IoMmuDxe: rename DeviceAddress to PlainTextAddress in MAP_INFO
2017-08-02 21:24 [PATCH 00/12] OvmfPkg/IoMmuDxe: cleanups and fixes Laszlo Ersek
2017-08-02 21:24 ` [PATCH 01/12] OvmfPkg/IoMmuDxe: rewrap source code to 79 characters Laszlo Ersek
@ 2017-08-02 21:24 ` Laszlo Ersek
2017-08-02 21:24 ` [PATCH 03/12] OvmfPkg/IoMmuDxe: rename HostAddress to CryptedAddress " Laszlo Ersek
` (11 subsequent siblings)
13 siblings, 0 replies; 22+ messages in thread
From: Laszlo Ersek @ 2017-08-02 21:24 UTC (permalink / raw)
To: edk2-devel-01; +Cc: Ard Biesheuvel, Brijesh Singh, Jordan Justen, Tom Lendacky
In this particular IOMMU driver, "DeviceAddress" is just as accessible to
the CPU as "HostAddress", the difference is that the area pointed-to by
the former is plain-text and accessible to the hypervisor. Rename
"DeviceAddress" to "PlainTextAddress" in MAP_INFO.
Cc: Ard Biesheuvel <ard.biesheuvel@linaro.org>
Cc: Brijesh Singh <brijesh.singh@amd.com>
Cc: Jordan Justen <jordan.l.justen@intel.com>
Cc: Tom Lendacky <thomas.lendacky@amd.com>
Suggested-by: Ard Biesheuvel <ard.biesheuvel@linaro.org>
Contributed-under: TianoCore Contribution Agreement 1.0
Signed-off-by: Laszlo Ersek <lersek@redhat.com>
---
OvmfPkg/IoMmuDxe/AmdSevIoMmu.c | 26 ++++++++++----------
1 file changed, 13 insertions(+), 13 deletions(-)
diff --git a/OvmfPkg/IoMmuDxe/AmdSevIoMmu.c b/OvmfPkg/IoMmuDxe/AmdSevIoMmu.c
index edef0f41eecc..fcb7bcfaecc2 100644
--- a/OvmfPkg/IoMmuDxe/AmdSevIoMmu.c
+++ b/OvmfPkg/IoMmuDxe/AmdSevIoMmu.c
@@ -23,40 +23,40 @@
typedef struct {
EDKII_IOMMU_OPERATION Operation;
UINTN NumberOfBytes;
UINTN NumberOfPages;
EFI_PHYSICAL_ADDRESS HostAddress;
- EFI_PHYSICAL_ADDRESS DeviceAddress;
+ EFI_PHYSICAL_ADDRESS PlainTextAddress;
} MAP_INFO;
#define NO_MAPPING (VOID *) (UINTN) -1
/**
Provides the controller-specific addresses required to access system memory
from a DMA bus master. On SEV guest, the DMA operations must be performed on
shared buffer hence we allocate a bounce buffer to map the HostAddress to a
DeviceAddress. The Encryption attribute is removed from the DeviceAddress
buffer.
@param This The protocol instance pointer.
@param Operation Indicates if the bus master is going to read or
write to system memory.
@param HostAddress The system memory address to map to the PCI
controller.
@param NumberOfBytes On input the number of bytes to map. On output
the number of bytes that were mapped.
@param DeviceAddress The resulting map address for the bus master
PCI controller to use to access the hosts
HostAddress.
@param Mapping A resulting value to pass to Unmap().
@retval EFI_SUCCESS The range was mapped for the returned
NumberOfBytes.
@retval EFI_UNSUPPORTED The HostAddress cannot be mapped as a common
buffer.
@retval EFI_INVALID_PARAMETER One or more parameters are invalid.
@retval EFI_OUT_OF_RESOURCES The request could not be completed due to a
lack of resources.
@retval EFI_DEVICE_ERROR The system hardware could not map the requested
address.
**/
@@ -65,160 +65,160 @@ EFIAPI
IoMmuMap (
IN EDKII_IOMMU_PROTOCOL *This,
IN EDKII_IOMMU_OPERATION Operation,
IN VOID *HostAddress,
IN OUT UINTN *NumberOfBytes,
OUT EFI_PHYSICAL_ADDRESS *DeviceAddress,
OUT VOID **Mapping
)
{
EFI_STATUS Status;
EFI_PHYSICAL_ADDRESS PhysicalAddress;
MAP_INFO *MapInfo;
EFI_PHYSICAL_ADDRESS DmaMemoryTop;
EFI_ALLOCATE_TYPE AllocateType;
if (HostAddress == NULL || NumberOfBytes == NULL || DeviceAddress == NULL ||
Mapping == NULL) {
return EFI_INVALID_PARAMETER;
}
//
// Make sure that Operation is valid
//
if ((UINT32) Operation >= EdkiiIoMmuOperationMaximum) {
return EFI_INVALID_PARAMETER;
}
PhysicalAddress = (EFI_PHYSICAL_ADDRESS) (UINTN) HostAddress;
DmaMemoryTop = (UINTN)-1;
AllocateType = AllocateAnyPages;
if (((Operation != EdkiiIoMmuOperationBusMasterRead64 &&
Operation != EdkiiIoMmuOperationBusMasterWrite64 &&
Operation != EdkiiIoMmuOperationBusMasterCommonBuffer64)) &&
((PhysicalAddress + *NumberOfBytes) > SIZE_4GB)) {
//
// If the root bridge or the device cannot handle performing DMA above
// 4GB but any part of the DMA transfer being mapped is above 4GB, then
// map the DMA transfer to a buffer below 4GB.
//
DmaMemoryTop = SIZE_4GB - 1;
AllocateType = AllocateMaxAddress;
if (Operation == EdkiiIoMmuOperationBusMasterCommonBuffer ||
Operation == EdkiiIoMmuOperationBusMasterCommonBuffer64) {
//
// Common Buffer operations can not be remapped. If the common buffer
// if above 4GB, then it is not possible to generate a mapping, so
// return an error.
//
return EFI_UNSUPPORTED;
}
}
//
// CommandBuffer was allocated by us (AllocateBuffer) and is already in
// unencryted buffer so no need to create bounce buffer
//
if (Operation == EdkiiIoMmuOperationBusMasterCommonBuffer ||
Operation == EdkiiIoMmuOperationBusMasterCommonBuffer64) {
*Mapping = NO_MAPPING;
*DeviceAddress = PhysicalAddress;
return EFI_SUCCESS;
}
//
// Allocate a MAP_INFO structure to remember the mapping when Unmap() is
// called later.
//
MapInfo = AllocatePool (sizeof (MAP_INFO));
if (MapInfo == NULL) {
*NumberOfBytes = 0;
return EFI_OUT_OF_RESOURCES;
}
//
// Initialize the MAP_INFO structure
//
MapInfo->Operation = Operation;
MapInfo->NumberOfBytes = *NumberOfBytes;
MapInfo->NumberOfPages = EFI_SIZE_TO_PAGES (MapInfo->NumberOfBytes);
MapInfo->HostAddress = PhysicalAddress;
- MapInfo->DeviceAddress = DmaMemoryTop;
+ MapInfo->PlainTextAddress = DmaMemoryTop;
//
// Allocate a buffer to map the transfer to.
//
Status = gBS->AllocatePages (
AllocateType,
EfiBootServicesData,
MapInfo->NumberOfPages,
- &MapInfo->DeviceAddress
+ &MapInfo->PlainTextAddress
);
if (EFI_ERROR (Status)) {
FreePool (MapInfo);
*NumberOfBytes = 0;
return Status;
}
//
// Clear the memory encryption mask from the device buffer
//
Status = MemEncryptSevClearPageEncMask (
0,
- MapInfo->DeviceAddress,
+ MapInfo->PlainTextAddress,
MapInfo->NumberOfPages,
TRUE
);
ASSERT_EFI_ERROR(Status);
//
// If this is a read operation from the Bus Master's point of view,
// then copy the contents of the real buffer into the mapped buffer
// so the Bus Master can read the contents of the real buffer.
//
if (Operation == EdkiiIoMmuOperationBusMasterRead ||
Operation == EdkiiIoMmuOperationBusMasterRead64) {
CopyMem (
- (VOID *) (UINTN) MapInfo->DeviceAddress,
+ (VOID *) (UINTN) MapInfo->PlainTextAddress,
(VOID *) (UINTN) MapInfo->HostAddress,
MapInfo->NumberOfBytes
);
}
//
// The DeviceAddress is the address of the maped buffer below 4GB
//
- *DeviceAddress = MapInfo->DeviceAddress;
+ *DeviceAddress = MapInfo->PlainTextAddress;
//
// Return a pointer to the MAP_INFO structure in Mapping
//
*Mapping = MapInfo;
DEBUG ((
DEBUG_VERBOSE,
- "%a Device 0x%Lx Host 0x%Lx Pages 0x%Lx Bytes 0x%Lx\n",
+ "%a PlainText 0x%Lx Host 0x%Lx Pages 0x%Lx Bytes 0x%Lx\n",
__FUNCTION__,
- MapInfo->DeviceAddress,
+ MapInfo->PlainTextAddress,
MapInfo->HostAddress,
MapInfo->NumberOfPages,
MapInfo->NumberOfBytes
));
return EFI_SUCCESS;
}
/**
Completes the Map() operation and releases any corresponding resources.
@param This The protocol instance pointer.
@param Mapping The mapping value returned from Map().
@retval EFI_SUCCESS The range was unmapped.
@retval EFI_INVALID_PARAMETER Mapping is not a value that was returned by
Map().
@retval EFI_DEVICE_ERROR The data was not committed to the target system
memory.
**/
@@ -227,87 +227,87 @@ EFIAPI
IoMmuUnmap (
IN EDKII_IOMMU_PROTOCOL *This,
IN VOID *Mapping
)
{
MAP_INFO *MapInfo;
EFI_STATUS Status;
if (Mapping == NULL) {
return EFI_INVALID_PARAMETER;
}
//
// See if the Map() operation associated with this Unmap() required a mapping
// buffer. If a mapping buffer was not required, then this function simply
// buffer. If a mapping buffer was not required, then this function simply
//
if (Mapping == NO_MAPPING) {
return EFI_SUCCESS;
}
MapInfo = (MAP_INFO *)Mapping;
//
// If this is a write operation from the Bus Master's point of view,
// then copy the contents of the mapped buffer into the real buffer
// so the processor can read the contents of the real buffer.
//
if (MapInfo->Operation == EdkiiIoMmuOperationBusMasterWrite ||
MapInfo->Operation == EdkiiIoMmuOperationBusMasterWrite64) {
CopyMem (
(VOID *) (UINTN) MapInfo->HostAddress,
- (VOID *) (UINTN) MapInfo->DeviceAddress,
+ (VOID *) (UINTN) MapInfo->PlainTextAddress,
MapInfo->NumberOfBytes
);
}
DEBUG ((
DEBUG_VERBOSE,
- "%a Device 0x%Lx Host 0x%Lx Pages 0x%Lx Bytes 0x%Lx\n",
+ "%a PlainText 0x%Lx Host 0x%Lx Pages 0x%Lx Bytes 0x%Lx\n",
__FUNCTION__,
- MapInfo->DeviceAddress,
+ MapInfo->PlainTextAddress,
MapInfo->HostAddress,
MapInfo->NumberOfPages,
MapInfo->NumberOfBytes
));
//
// Restore the memory encryption mask
//
Status = MemEncryptSevSetPageEncMask (
0,
- MapInfo->DeviceAddress,
+ MapInfo->PlainTextAddress,
MapInfo->NumberOfPages,
TRUE
);
ASSERT_EFI_ERROR(Status);
//
// Free the mapped buffer and the MAP_INFO structure.
//
- gBS->FreePages (MapInfo->DeviceAddress, MapInfo->NumberOfPages);
+ gBS->FreePages (MapInfo->PlainTextAddress, MapInfo->NumberOfPages);
FreePool (Mapping);
return EFI_SUCCESS;
}
/**
Allocates pages that are suitable for an OperationBusMasterCommonBuffer or
OperationBusMasterCommonBuffer64 mapping.
@param This The protocol instance pointer.
@param Type This parameter is not used and must be ignored.
@param MemoryType The type of memory to allocate,
EfiBootServicesData or EfiRuntimeServicesData.
@param Pages The number of pages to allocate.
@param HostAddress A pointer to store the base system memory
address of the allocated range.
@param Attributes The requested bit mask of attributes for the
allocated range.
@retval EFI_SUCCESS The requested memory pages were allocated.
@retval EFI_UNSUPPORTED Attributes is unsupported. The only legal
attribute bits are MEMORY_WRITE_COMBINE and
MEMORY_CACHED.
@retval EFI_INVALID_PARAMETER One or more parameters are invalid.
@retval EFI_OUT_OF_RESOURCES The memory pages could not be allocated.
**/
--
2.13.1.3.g8be5a757fa67
^ permalink raw reply related [flat|nested] 22+ messages in thread
* [PATCH 03/12] OvmfPkg/IoMmuDxe: rename HostAddress to CryptedAddress in MAP_INFO
2017-08-02 21:24 [PATCH 00/12] OvmfPkg/IoMmuDxe: cleanups and fixes Laszlo Ersek
2017-08-02 21:24 ` [PATCH 01/12] OvmfPkg/IoMmuDxe: rewrap source code to 79 characters Laszlo Ersek
2017-08-02 21:24 ` [PATCH 02/12] OvmfPkg/IoMmuDxe: rename DeviceAddress to PlainTextAddress in MAP_INFO Laszlo Ersek
@ 2017-08-02 21:24 ` Laszlo Ersek
2017-08-02 21:24 ` [PATCH 04/12] OvmfPkg/IoMmuDxe: convert UINTN arguments to UINT64 for the %Lx fmt spec Laszlo Ersek
` (10 subsequent siblings)
13 siblings, 0 replies; 22+ messages in thread
From: Laszlo Ersek @ 2017-08-02 21:24 UTC (permalink / raw)
To: edk2-devel-01; +Cc: Ard Biesheuvel, Brijesh Singh, Jordan Justen, Tom Lendacky
As a continuation of the last patch, clarify that the area pointed-to by
"HostAddress" is encrypted and hidden from the hypervisor.
Cc: Ard Biesheuvel <ard.biesheuvel@linaro.org>
Cc: Brijesh Singh <brijesh.singh@amd.com>
Cc: Jordan Justen <jordan.l.justen@intel.com>
Cc: Tom Lendacky <thomas.lendacky@amd.com>
Contributed-under: TianoCore Contribution Agreement 1.0
Signed-off-by: Laszlo Ersek <lersek@redhat.com>
---
OvmfPkg/IoMmuDxe/AmdSevIoMmu.c | 16 ++++++++--------
1 file changed, 8 insertions(+), 8 deletions(-)
diff --git a/OvmfPkg/IoMmuDxe/AmdSevIoMmu.c b/OvmfPkg/IoMmuDxe/AmdSevIoMmu.c
index fcb7bcfaecc2..dfad2cbb569d 100644
--- a/OvmfPkg/IoMmuDxe/AmdSevIoMmu.c
+++ b/OvmfPkg/IoMmuDxe/AmdSevIoMmu.c
@@ -23,40 +23,40 @@
typedef struct {
EDKII_IOMMU_OPERATION Operation;
UINTN NumberOfBytes;
UINTN NumberOfPages;
- EFI_PHYSICAL_ADDRESS HostAddress;
+ EFI_PHYSICAL_ADDRESS CryptedAddress;
EFI_PHYSICAL_ADDRESS PlainTextAddress;
} MAP_INFO;
#define NO_MAPPING (VOID *) (UINTN) -1
/**
Provides the controller-specific addresses required to access system memory
from a DMA bus master. On SEV guest, the DMA operations must be performed on
shared buffer hence we allocate a bounce buffer to map the HostAddress to a
DeviceAddress. The Encryption attribute is removed from the DeviceAddress
buffer.
@param This The protocol instance pointer.
@param Operation Indicates if the bus master is going to read or
write to system memory.
@param HostAddress The system memory address to map to the PCI
controller.
@param NumberOfBytes On input the number of bytes to map. On output
the number of bytes that were mapped.
@param DeviceAddress The resulting map address for the bus master
PCI controller to use to access the hosts
HostAddress.
@param Mapping A resulting value to pass to Unmap().
@retval EFI_SUCCESS The range was mapped for the returned
NumberOfBytes.
@retval EFI_UNSUPPORTED The HostAddress cannot be mapped as a common
buffer.
@retval EFI_INVALID_PARAMETER One or more parameters are invalid.
@retval EFI_OUT_OF_RESOURCES The request could not be completed due to a
lack of resources.
@retval EFI_DEVICE_ERROR The system hardware could not map the requested
address.
**/
@@ -65,160 +65,160 @@ EFIAPI
IoMmuMap (
IN EDKII_IOMMU_PROTOCOL *This,
IN EDKII_IOMMU_OPERATION Operation,
IN VOID *HostAddress,
IN OUT UINTN *NumberOfBytes,
OUT EFI_PHYSICAL_ADDRESS *DeviceAddress,
OUT VOID **Mapping
)
{
EFI_STATUS Status;
EFI_PHYSICAL_ADDRESS PhysicalAddress;
MAP_INFO *MapInfo;
EFI_PHYSICAL_ADDRESS DmaMemoryTop;
EFI_ALLOCATE_TYPE AllocateType;
if (HostAddress == NULL || NumberOfBytes == NULL || DeviceAddress == NULL ||
Mapping == NULL) {
return EFI_INVALID_PARAMETER;
}
//
// Make sure that Operation is valid
//
if ((UINT32) Operation >= EdkiiIoMmuOperationMaximum) {
return EFI_INVALID_PARAMETER;
}
PhysicalAddress = (EFI_PHYSICAL_ADDRESS) (UINTN) HostAddress;
DmaMemoryTop = (UINTN)-1;
AllocateType = AllocateAnyPages;
if (((Operation != EdkiiIoMmuOperationBusMasterRead64 &&
Operation != EdkiiIoMmuOperationBusMasterWrite64 &&
Operation != EdkiiIoMmuOperationBusMasterCommonBuffer64)) &&
((PhysicalAddress + *NumberOfBytes) > SIZE_4GB)) {
//
// If the root bridge or the device cannot handle performing DMA above
// 4GB but any part of the DMA transfer being mapped is above 4GB, then
// map the DMA transfer to a buffer below 4GB.
//
DmaMemoryTop = SIZE_4GB - 1;
AllocateType = AllocateMaxAddress;
if (Operation == EdkiiIoMmuOperationBusMasterCommonBuffer ||
Operation == EdkiiIoMmuOperationBusMasterCommonBuffer64) {
//
// Common Buffer operations can not be remapped. If the common buffer
// if above 4GB, then it is not possible to generate a mapping, so
// return an error.
//
return EFI_UNSUPPORTED;
}
}
//
// CommandBuffer was allocated by us (AllocateBuffer) and is already in
// unencryted buffer so no need to create bounce buffer
//
if (Operation == EdkiiIoMmuOperationBusMasterCommonBuffer ||
Operation == EdkiiIoMmuOperationBusMasterCommonBuffer64) {
*Mapping = NO_MAPPING;
*DeviceAddress = PhysicalAddress;
return EFI_SUCCESS;
}
//
// Allocate a MAP_INFO structure to remember the mapping when Unmap() is
// called later.
//
MapInfo = AllocatePool (sizeof (MAP_INFO));
if (MapInfo == NULL) {
*NumberOfBytes = 0;
return EFI_OUT_OF_RESOURCES;
}
//
// Initialize the MAP_INFO structure
//
MapInfo->Operation = Operation;
MapInfo->NumberOfBytes = *NumberOfBytes;
MapInfo->NumberOfPages = EFI_SIZE_TO_PAGES (MapInfo->NumberOfBytes);
- MapInfo->HostAddress = PhysicalAddress;
+ MapInfo->CryptedAddress = PhysicalAddress;
MapInfo->PlainTextAddress = DmaMemoryTop;
//
// Allocate a buffer to map the transfer to.
//
Status = gBS->AllocatePages (
AllocateType,
EfiBootServicesData,
MapInfo->NumberOfPages,
&MapInfo->PlainTextAddress
);
if (EFI_ERROR (Status)) {
FreePool (MapInfo);
*NumberOfBytes = 0;
return Status;
}
//
// Clear the memory encryption mask from the device buffer
//
Status = MemEncryptSevClearPageEncMask (
0,
MapInfo->PlainTextAddress,
MapInfo->NumberOfPages,
TRUE
);
ASSERT_EFI_ERROR(Status);
//
// If this is a read operation from the Bus Master's point of view,
// then copy the contents of the real buffer into the mapped buffer
// so the Bus Master can read the contents of the real buffer.
//
if (Operation == EdkiiIoMmuOperationBusMasterRead ||
Operation == EdkiiIoMmuOperationBusMasterRead64) {
CopyMem (
(VOID *) (UINTN) MapInfo->PlainTextAddress,
- (VOID *) (UINTN) MapInfo->HostAddress,
+ (VOID *) (UINTN) MapInfo->CryptedAddress,
MapInfo->NumberOfBytes
);
}
//
// The DeviceAddress is the address of the maped buffer below 4GB
//
*DeviceAddress = MapInfo->PlainTextAddress;
//
// Return a pointer to the MAP_INFO structure in Mapping
//
*Mapping = MapInfo;
DEBUG ((
DEBUG_VERBOSE,
- "%a PlainText 0x%Lx Host 0x%Lx Pages 0x%Lx Bytes 0x%Lx\n",
+ "%a PlainText 0x%Lx Crypted 0x%Lx Pages 0x%Lx Bytes 0x%Lx\n",
__FUNCTION__,
MapInfo->PlainTextAddress,
- MapInfo->HostAddress,
+ MapInfo->CryptedAddress,
MapInfo->NumberOfPages,
MapInfo->NumberOfBytes
));
return EFI_SUCCESS;
}
/**
Completes the Map() operation and releases any corresponding resources.
@param This The protocol instance pointer.
@param Mapping The mapping value returned from Map().
@retval EFI_SUCCESS The range was unmapped.
@retval EFI_INVALID_PARAMETER Mapping is not a value that was returned by
Map().
@retval EFI_DEVICE_ERROR The data was not committed to the target system
memory.
**/
@@ -227,87 +227,87 @@ EFIAPI
IoMmuUnmap (
IN EDKII_IOMMU_PROTOCOL *This,
IN VOID *Mapping
)
{
MAP_INFO *MapInfo;
EFI_STATUS Status;
if (Mapping == NULL) {
return EFI_INVALID_PARAMETER;
}
//
// See if the Map() operation associated with this Unmap() required a mapping
// buffer. If a mapping buffer was not required, then this function simply
// buffer. If a mapping buffer was not required, then this function simply
//
if (Mapping == NO_MAPPING) {
return EFI_SUCCESS;
}
MapInfo = (MAP_INFO *)Mapping;
//
// If this is a write operation from the Bus Master's point of view,
// then copy the contents of the mapped buffer into the real buffer
// so the processor can read the contents of the real buffer.
//
if (MapInfo->Operation == EdkiiIoMmuOperationBusMasterWrite ||
MapInfo->Operation == EdkiiIoMmuOperationBusMasterWrite64) {
CopyMem (
- (VOID *) (UINTN) MapInfo->HostAddress,
+ (VOID *) (UINTN) MapInfo->CryptedAddress,
(VOID *) (UINTN) MapInfo->PlainTextAddress,
MapInfo->NumberOfBytes
);
}
DEBUG ((
DEBUG_VERBOSE,
- "%a PlainText 0x%Lx Host 0x%Lx Pages 0x%Lx Bytes 0x%Lx\n",
+ "%a PlainText 0x%Lx Crypted 0x%Lx Pages 0x%Lx Bytes 0x%Lx\n",
__FUNCTION__,
MapInfo->PlainTextAddress,
- MapInfo->HostAddress,
+ MapInfo->CryptedAddress,
MapInfo->NumberOfPages,
MapInfo->NumberOfBytes
));
//
// Restore the memory encryption mask
//
Status = MemEncryptSevSetPageEncMask (
0,
MapInfo->PlainTextAddress,
MapInfo->NumberOfPages,
TRUE
);
ASSERT_EFI_ERROR(Status);
//
// Free the mapped buffer and the MAP_INFO structure.
//
gBS->FreePages (MapInfo->PlainTextAddress, MapInfo->NumberOfPages);
FreePool (Mapping);
return EFI_SUCCESS;
}
/**
Allocates pages that are suitable for an OperationBusMasterCommonBuffer or
OperationBusMasterCommonBuffer64 mapping.
@param This The protocol instance pointer.
@param Type This parameter is not used and must be ignored.
@param MemoryType The type of memory to allocate,
EfiBootServicesData or EfiRuntimeServicesData.
@param Pages The number of pages to allocate.
@param HostAddress A pointer to store the base system memory
address of the allocated range.
@param Attributes The requested bit mask of attributes for the
allocated range.
@retval EFI_SUCCESS The requested memory pages were allocated.
@retval EFI_UNSUPPORTED Attributes is unsupported. The only legal
attribute bits are MEMORY_WRITE_COMBINE and
MEMORY_CACHED.
@retval EFI_INVALID_PARAMETER One or more parameters are invalid.
@retval EFI_OUT_OF_RESOURCES The memory pages could not be allocated.
**/
--
2.13.1.3.g8be5a757fa67
^ permalink raw reply related [flat|nested] 22+ messages in thread
* [PATCH 04/12] OvmfPkg/IoMmuDxe: convert UINTN arguments to UINT64 for the %Lx fmt spec
2017-08-02 21:24 [PATCH 00/12] OvmfPkg/IoMmuDxe: cleanups and fixes Laszlo Ersek
` (2 preceding siblings ...)
2017-08-02 21:24 ` [PATCH 03/12] OvmfPkg/IoMmuDxe: rename HostAddress to CryptedAddress " Laszlo Ersek
@ 2017-08-02 21:24 ` Laszlo Ersek
2017-08-02 21:24 ` [PATCH 05/12] OvmfPkg/IoMmuDxe: don't initialize local variables Laszlo Ersek
` (9 subsequent siblings)
13 siblings, 0 replies; 22+ messages in thread
From: Laszlo Ersek @ 2017-08-02 21:24 UTC (permalink / raw)
To: edk2-devel-01; +Cc: Ard Biesheuvel, Brijesh Singh, Jordan Justen, Tom Lendacky
The portable way to print UINTN values is to use the %Lx format specifier,
and to convert the values to UINT64. The second step is currently missing,
add it.
Cc: Ard Biesheuvel <ard.biesheuvel@linaro.org>
Cc: Brijesh Singh <brijesh.singh@amd.com>
Cc: Jordan Justen <jordan.l.justen@intel.com>
Cc: Tom Lendacky <thomas.lendacky@amd.com>
Contributed-under: TianoCore Contribution Agreement 1.0
Signed-off-by: Laszlo Ersek <lersek@redhat.com>
---
OvmfPkg/IoMmuDxe/AmdSevIoMmu.c | 14 +++++++-------
1 file changed, 7 insertions(+), 7 deletions(-)
diff --git a/OvmfPkg/IoMmuDxe/AmdSevIoMmu.c b/OvmfPkg/IoMmuDxe/AmdSevIoMmu.c
index dfad2cbb569d..954062442782 100644
--- a/OvmfPkg/IoMmuDxe/AmdSevIoMmu.c
+++ b/OvmfPkg/IoMmuDxe/AmdSevIoMmu.c
@@ -65,160 +65,160 @@ EFIAPI
IoMmuMap (
IN EDKII_IOMMU_PROTOCOL *This,
IN EDKII_IOMMU_OPERATION Operation,
IN VOID *HostAddress,
IN OUT UINTN *NumberOfBytes,
OUT EFI_PHYSICAL_ADDRESS *DeviceAddress,
OUT VOID **Mapping
)
{
EFI_STATUS Status;
EFI_PHYSICAL_ADDRESS PhysicalAddress;
MAP_INFO *MapInfo;
EFI_PHYSICAL_ADDRESS DmaMemoryTop;
EFI_ALLOCATE_TYPE AllocateType;
if (HostAddress == NULL || NumberOfBytes == NULL || DeviceAddress == NULL ||
Mapping == NULL) {
return EFI_INVALID_PARAMETER;
}
//
// Make sure that Operation is valid
//
if ((UINT32) Operation >= EdkiiIoMmuOperationMaximum) {
return EFI_INVALID_PARAMETER;
}
PhysicalAddress = (EFI_PHYSICAL_ADDRESS) (UINTN) HostAddress;
DmaMemoryTop = (UINTN)-1;
AllocateType = AllocateAnyPages;
if (((Operation != EdkiiIoMmuOperationBusMasterRead64 &&
Operation != EdkiiIoMmuOperationBusMasterWrite64 &&
Operation != EdkiiIoMmuOperationBusMasterCommonBuffer64)) &&
((PhysicalAddress + *NumberOfBytes) > SIZE_4GB)) {
//
// If the root bridge or the device cannot handle performing DMA above
// 4GB but any part of the DMA transfer being mapped is above 4GB, then
// map the DMA transfer to a buffer below 4GB.
//
DmaMemoryTop = SIZE_4GB - 1;
AllocateType = AllocateMaxAddress;
if (Operation == EdkiiIoMmuOperationBusMasterCommonBuffer ||
Operation == EdkiiIoMmuOperationBusMasterCommonBuffer64) {
//
// Common Buffer operations can not be remapped. If the common buffer
// if above 4GB, then it is not possible to generate a mapping, so
// return an error.
//
return EFI_UNSUPPORTED;
}
}
//
// CommandBuffer was allocated by us (AllocateBuffer) and is already in
// unencryted buffer so no need to create bounce buffer
//
if (Operation == EdkiiIoMmuOperationBusMasterCommonBuffer ||
Operation == EdkiiIoMmuOperationBusMasterCommonBuffer64) {
*Mapping = NO_MAPPING;
*DeviceAddress = PhysicalAddress;
return EFI_SUCCESS;
}
//
// Allocate a MAP_INFO structure to remember the mapping when Unmap() is
// called later.
//
MapInfo = AllocatePool (sizeof (MAP_INFO));
if (MapInfo == NULL) {
*NumberOfBytes = 0;
return EFI_OUT_OF_RESOURCES;
}
//
// Initialize the MAP_INFO structure
//
MapInfo->Operation = Operation;
MapInfo->NumberOfBytes = *NumberOfBytes;
MapInfo->NumberOfPages = EFI_SIZE_TO_PAGES (MapInfo->NumberOfBytes);
MapInfo->CryptedAddress = PhysicalAddress;
MapInfo->PlainTextAddress = DmaMemoryTop;
//
// Allocate a buffer to map the transfer to.
//
Status = gBS->AllocatePages (
AllocateType,
EfiBootServicesData,
MapInfo->NumberOfPages,
&MapInfo->PlainTextAddress
);
if (EFI_ERROR (Status)) {
FreePool (MapInfo);
*NumberOfBytes = 0;
return Status;
}
//
// Clear the memory encryption mask from the device buffer
//
Status = MemEncryptSevClearPageEncMask (
0,
MapInfo->PlainTextAddress,
MapInfo->NumberOfPages,
TRUE
);
ASSERT_EFI_ERROR(Status);
//
// If this is a read operation from the Bus Master's point of view,
// then copy the contents of the real buffer into the mapped buffer
// so the Bus Master can read the contents of the real buffer.
//
if (Operation == EdkiiIoMmuOperationBusMasterRead ||
Operation == EdkiiIoMmuOperationBusMasterRead64) {
CopyMem (
(VOID *) (UINTN) MapInfo->PlainTextAddress,
(VOID *) (UINTN) MapInfo->CryptedAddress,
MapInfo->NumberOfBytes
);
}
//
// The DeviceAddress is the address of the maped buffer below 4GB
//
*DeviceAddress = MapInfo->PlainTextAddress;
//
// Return a pointer to the MAP_INFO structure in Mapping
//
*Mapping = MapInfo;
DEBUG ((
DEBUG_VERBOSE,
"%a PlainText 0x%Lx Crypted 0x%Lx Pages 0x%Lx Bytes 0x%Lx\n",
__FUNCTION__,
MapInfo->PlainTextAddress,
MapInfo->CryptedAddress,
- MapInfo->NumberOfPages,
- MapInfo->NumberOfBytes
+ (UINT64)MapInfo->NumberOfPages,
+ (UINT64)MapInfo->NumberOfBytes
));
return EFI_SUCCESS;
}
/**
Completes the Map() operation and releases any corresponding resources.
@param This The protocol instance pointer.
@param Mapping The mapping value returned from Map().
@retval EFI_SUCCESS The range was unmapped.
@retval EFI_INVALID_PARAMETER Mapping is not a value that was returned by
Map().
@retval EFI_DEVICE_ERROR The data was not committed to the target system
memory.
**/
@@ -227,87 +227,87 @@ EFIAPI
IoMmuUnmap (
IN EDKII_IOMMU_PROTOCOL *This,
IN VOID *Mapping
)
{
MAP_INFO *MapInfo;
EFI_STATUS Status;
if (Mapping == NULL) {
return EFI_INVALID_PARAMETER;
}
//
// See if the Map() operation associated with this Unmap() required a mapping
// buffer. If a mapping buffer was not required, then this function simply
// buffer. If a mapping buffer was not required, then this function simply
//
if (Mapping == NO_MAPPING) {
return EFI_SUCCESS;
}
MapInfo = (MAP_INFO *)Mapping;
//
// If this is a write operation from the Bus Master's point of view,
// then copy the contents of the mapped buffer into the real buffer
// so the processor can read the contents of the real buffer.
//
if (MapInfo->Operation == EdkiiIoMmuOperationBusMasterWrite ||
MapInfo->Operation == EdkiiIoMmuOperationBusMasterWrite64) {
CopyMem (
(VOID *) (UINTN) MapInfo->CryptedAddress,
(VOID *) (UINTN) MapInfo->PlainTextAddress,
MapInfo->NumberOfBytes
);
}
DEBUG ((
DEBUG_VERBOSE,
"%a PlainText 0x%Lx Crypted 0x%Lx Pages 0x%Lx Bytes 0x%Lx\n",
__FUNCTION__,
MapInfo->PlainTextAddress,
MapInfo->CryptedAddress,
- MapInfo->NumberOfPages,
- MapInfo->NumberOfBytes
+ (UINT64)MapInfo->NumberOfPages,
+ (UINT64)MapInfo->NumberOfBytes
));
//
// Restore the memory encryption mask
//
Status = MemEncryptSevSetPageEncMask (
0,
MapInfo->PlainTextAddress,
MapInfo->NumberOfPages,
TRUE
);
ASSERT_EFI_ERROR(Status);
//
// Free the mapped buffer and the MAP_INFO structure.
//
gBS->FreePages (MapInfo->PlainTextAddress, MapInfo->NumberOfPages);
FreePool (Mapping);
return EFI_SUCCESS;
}
/**
Allocates pages that are suitable for an OperationBusMasterCommonBuffer or
OperationBusMasterCommonBuffer64 mapping.
@param This The protocol instance pointer.
@param Type This parameter is not used and must be ignored.
@param MemoryType The type of memory to allocate,
EfiBootServicesData or EfiRuntimeServicesData.
@param Pages The number of pages to allocate.
@param HostAddress A pointer to store the base system memory
address of the allocated range.
@param Attributes The requested bit mask of attributes for the
allocated range.
@retval EFI_SUCCESS The requested memory pages were allocated.
@retval EFI_UNSUPPORTED Attributes is unsupported. The only legal
attribute bits are MEMORY_WRITE_COMBINE and
MEMORY_CACHED.
@retval EFI_INVALID_PARAMETER One or more parameters are invalid.
@retval EFI_OUT_OF_RESOURCES The memory pages could not be allocated.
**/
@@ -316,81 +316,81 @@ EFIAPI
IoMmuAllocateBuffer (
IN EDKII_IOMMU_PROTOCOL *This,
IN EFI_ALLOCATE_TYPE Type,
IN EFI_MEMORY_TYPE MemoryType,
IN UINTN Pages,
IN OUT VOID **HostAddress,
IN UINT64 Attributes
)
{
EFI_STATUS Status;
EFI_PHYSICAL_ADDRESS PhysicalAddress;
//
// Validate Attributes
//
if ((Attributes & EDKII_IOMMU_ATTRIBUTE_INVALID_FOR_ALLOCATE_BUFFER) != 0) {
return EFI_UNSUPPORTED;
}
//
// Check for invalid inputs
//
if (HostAddress == NULL) {
return EFI_INVALID_PARAMETER;
}
//
// The only valid memory types are EfiBootServicesData and
// EfiRuntimeServicesData
//
if (MemoryType != EfiBootServicesData &&
MemoryType != EfiRuntimeServicesData) {
return EFI_INVALID_PARAMETER;
}
PhysicalAddress = (UINTN)-1;
if ((Attributes & EDKII_IOMMU_ATTRIBUTE_DUAL_ADDRESS_CYCLE) == 0) {
//
// Limit allocations to memory below 4GB
//
PhysicalAddress = SIZE_4GB - 1;
}
Status = gBS->AllocatePages (
AllocateMaxAddress,
MemoryType,
Pages,
&PhysicalAddress
);
if (!EFI_ERROR (Status)) {
*HostAddress = (VOID *) (UINTN) PhysicalAddress;
//
// Clear memory encryption mask
//
Status = MemEncryptSevClearPageEncMask (0, PhysicalAddress, Pages, TRUE);
ASSERT_EFI_ERROR(Status);
}
DEBUG ((
DEBUG_VERBOSE,
"%a Address 0x%Lx Pages 0x%Lx\n",
__FUNCTION__,
PhysicalAddress,
- Pages
+ (UINT64)Pages
));
return Status;
}
/**
Frees memory that was allocated with AllocateBuffer().
@param This The protocol instance pointer.
@param Pages The number of pages to free.
@param HostAddress The base system memory address of the allocated
range.
@retval EFI_SUCCESS The requested memory pages were freed.
@retval EFI_INVALID_PARAMETER The memory range specified by HostAddress and
Pages was not allocated with AllocateBuffer().
**/
@@ -399,78 +399,78 @@ EFIAPI
IoMmuFreeBuffer (
IN EDKII_IOMMU_PROTOCOL *This,
IN UINTN Pages,
IN VOID *HostAddress
)
{
EFI_STATUS Status;
//
// Set memory encryption mask
//
Status = MemEncryptSevSetPageEncMask (
0,
(EFI_PHYSICAL_ADDRESS)(UINTN)HostAddress,
Pages,
TRUE
);
ASSERT_EFI_ERROR(Status);
DEBUG ((
DEBUG_VERBOSE,
"%a Address 0x%Lx Pages 0x%Lx\n",
__FUNCTION__,
- (UINTN)HostAddress,
- Pages
+ (UINT64)(UINTN)HostAddress,
+ (UINT64)Pages
));
return gBS->FreePages ((EFI_PHYSICAL_ADDRESS) (UINTN) HostAddress, Pages);
}
/**
Set IOMMU attribute for a system memory.
If the IOMMU protocol exists, the system memory cannot be used
for DMA by default.
When a device requests a DMA access for a system memory,
the device driver need use SetAttribute() to update the IOMMU
attribute to request DMA access (read and/or write).
The DeviceHandle is used to identify which device submits the request.
The IOMMU implementation need translate the device path to an IOMMU device
ID, and set IOMMU hardware register accordingly.
1) DeviceHandle can be a standard PCI device.
The memory for BusMasterRead need set EDKII_IOMMU_ACCESS_READ.
The memory for BusMasterWrite need set EDKII_IOMMU_ACCESS_WRITE.
The memory for BusMasterCommonBuffer need set
EDKII_IOMMU_ACCESS_READ|EDKII_IOMMU_ACCESS_WRITE.
After the memory is used, the memory need set 0 to keep it being
protected.
2) DeviceHandle can be an ACPI device (ISA, I2C, SPI, etc).
The memory for DMA access need set EDKII_IOMMU_ACCESS_READ and/or
EDKII_IOMMU_ACCESS_WRITE.
@param[in] This The protocol instance pointer.
@param[in] DeviceHandle The device who initiates the DMA access
request.
@param[in] Mapping The mapping value returned from Map().
@param[in] IoMmuAccess The IOMMU access.
@retval EFI_SUCCESS The IoMmuAccess is set for the memory range
specified by DeviceAddress and Length.
@retval EFI_INVALID_PARAMETER DeviceHandle is an invalid handle.
@retval EFI_INVALID_PARAMETER Mapping is not a value that was returned by
Map().
@retval EFI_INVALID_PARAMETER IoMmuAccess specified an illegal combination
of access.
@retval EFI_UNSUPPORTED DeviceHandle is unknown by the IOMMU.
@retval EFI_UNSUPPORTED The bit mask of IoMmuAccess is not supported
by the IOMMU.
@retval EFI_UNSUPPORTED The IOMMU does not support the memory range
specified by Mapping.
@retval EFI_OUT_OF_RESOURCES There are not enough resources available to
modify the IOMMU access.
@retval EFI_DEVICE_ERROR The IOMMU device reported an error while
attempting the operation.
**/
--
2.13.1.3.g8be5a757fa67
^ permalink raw reply related [flat|nested] 22+ messages in thread
* [PATCH 05/12] OvmfPkg/IoMmuDxe: don't initialize local variables
2017-08-02 21:24 [PATCH 00/12] OvmfPkg/IoMmuDxe: cleanups and fixes Laszlo Ersek
` (3 preceding siblings ...)
2017-08-02 21:24 ` [PATCH 04/12] OvmfPkg/IoMmuDxe: convert UINTN arguments to UINT64 for the %Lx fmt spec Laszlo Ersek
@ 2017-08-02 21:24 ` Laszlo Ersek
2017-08-02 21:24 ` [PATCH 06/12] OvmfPkg/IoMmuDxe: propagate errors from AmdSevInstallIoMmuProtocol() Laszlo Ersek
` (8 subsequent siblings)
13 siblings, 0 replies; 22+ messages in thread
From: Laszlo Ersek @ 2017-08-02 21:24 UTC (permalink / raw)
To: edk2-devel-01; +Cc: Ard Biesheuvel, Brijesh Singh, Jordan Justen, Tom Lendacky
The edk2 coding style requires separate assignments.
Cc: Ard Biesheuvel <ard.biesheuvel@linaro.org>
Cc: Brijesh Singh <brijesh.singh@amd.com>
Cc: Jordan Justen <jordan.l.justen@intel.com>
Cc: Tom Lendacky <thomas.lendacky@amd.com>
Contributed-under: TianoCore Contribution Agreement 1.0
Signed-off-by: Laszlo Ersek <lersek@redhat.com>
---
OvmfPkg/IoMmuDxe/IoMmuDxe.c | 8 ++++++--
1 file changed, 6 insertions(+), 2 deletions(-)
diff --git a/OvmfPkg/IoMmuDxe/IoMmuDxe.c b/OvmfPkg/IoMmuDxe/IoMmuDxe.c
index 5809afc44196..27b1856e0a17 100644
--- a/OvmfPkg/IoMmuDxe/IoMmuDxe.c
+++ b/OvmfPkg/IoMmuDxe/IoMmuDxe.c
@@ -31,23 +31,27 @@ EFIAPI
IoMmuDxeEntryPoint (
IN EFI_HANDLE ImageHandle,
IN EFI_SYSTEM_TABLE *SystemTable
)
{
- EFI_STATUS Status = EFI_SUCCESS;
- EFI_HANDLE Handle = NULL;
+ EFI_STATUS Status;
+ EFI_HANDLE Handle;
+
+ Status = EFI_SUCCESS;
//
// When SEV is enabled, install IoMmu protocol otherwise install the
// placeholder protocol so that other dependent module can run.
//
if (MemEncryptSevIsEnabled ()) {
AmdSevInstallIoMmuProtocol ();
} else {
+ Handle = NULL;
+
Status = gBS->InstallMultipleProtocolInterfaces (
&Handle,
&gIoMmuAbsentProtocolGuid,
NULL, NULL);
}
return Status;
}
--
2.13.1.3.g8be5a757fa67
^ permalink raw reply related [flat|nested] 22+ messages in thread
* [PATCH 06/12] OvmfPkg/IoMmuDxe: propagate errors from AmdSevInstallIoMmuProtocol()
2017-08-02 21:24 [PATCH 00/12] OvmfPkg/IoMmuDxe: cleanups and fixes Laszlo Ersek
` (4 preceding siblings ...)
2017-08-02 21:24 ` [PATCH 05/12] OvmfPkg/IoMmuDxe: don't initialize local variables Laszlo Ersek
@ 2017-08-02 21:24 ` Laszlo Ersek
2017-08-02 21:24 ` [PATCH 07/12] OvmfPkg/IoMmuDxe: clean up used library classes Laszlo Ersek
` (7 subsequent siblings)
13 siblings, 0 replies; 22+ messages in thread
From: Laszlo Ersek @ 2017-08-02 21:24 UTC (permalink / raw)
To: edk2-devel-01; +Cc: Ard Biesheuvel, Brijesh Singh, Jordan Justen, Tom Lendacky
If we cannot install the IOMMU protocol for whatever reason, exit the
driver with an error. The same is already done for the IOMMU Absent
protocol.
Cc: Ard Biesheuvel <ard.biesheuvel@linaro.org>
Cc: Brijesh Singh <brijesh.singh@amd.com>
Cc: Jordan Justen <jordan.l.justen@intel.com>
Cc: Tom Lendacky <thomas.lendacky@amd.com>
Contributed-under: TianoCore Contribution Agreement 1.0
Signed-off-by: Laszlo Ersek <lersek@redhat.com>
---
OvmfPkg/IoMmuDxe/AmdSevIoMmu.h | 2 +-
OvmfPkg/IoMmuDxe/AmdSevIoMmu.c | 4 ++--
OvmfPkg/IoMmuDxe/IoMmuDxe.c | 4 +---
3 files changed, 4 insertions(+), 6 deletions(-)
diff --git a/OvmfPkg/IoMmuDxe/AmdSevIoMmu.h b/OvmfPkg/IoMmuDxe/AmdSevIoMmu.h
index 88dabfc2c435..0f2155350817 100644
--- a/OvmfPkg/IoMmuDxe/AmdSevIoMmu.h
+++ b/OvmfPkg/IoMmuDxe/AmdSevIoMmu.h
@@ -34,7 +34,7 @@
MemEncryptSevLib.
**/
-VOID
+EFI_STATUS
EFIAPI
AmdSevInstallIoMmuProtocol (
VOID
diff --git a/OvmfPkg/IoMmuDxe/AmdSevIoMmu.c b/OvmfPkg/IoMmuDxe/AmdSevIoMmu.c
index 954062442782..8c2c23356a40 100644
--- a/OvmfPkg/IoMmuDxe/AmdSevIoMmu.c
+++ b/OvmfPkg/IoMmuDxe/AmdSevIoMmu.c
@@ -499,20 +499,20 @@ EDKII_IOMMU_PROTOCOL mAmdSev = {
Initialize Iommu Protocol.
**/
-VOID
+EFI_STATUS
EFIAPI
AmdSevInstallIoMmuProtocol (
VOID
)
{
EFI_STATUS Status;
EFI_HANDLE Handle;
Handle = NULL;
Status = gBS->InstallMultipleProtocolInterfaces (
&Handle,
&gEdkiiIoMmuProtocolGuid, &mAmdSev,
NULL
);
- ASSERT_EFI_ERROR (Status);
+ return Status;
}
diff --git a/OvmfPkg/IoMmuDxe/IoMmuDxe.c b/OvmfPkg/IoMmuDxe/IoMmuDxe.c
index 27b1856e0a17..0ea42cbc13ce 100644
--- a/OvmfPkg/IoMmuDxe/IoMmuDxe.c
+++ b/OvmfPkg/IoMmuDxe/IoMmuDxe.c
@@ -31,27 +31,25 @@ EFIAPI
IoMmuDxeEntryPoint (
IN EFI_HANDLE ImageHandle,
IN EFI_SYSTEM_TABLE *SystemTable
)
{
EFI_STATUS Status;
EFI_HANDLE Handle;
- Status = EFI_SUCCESS;
-
//
// When SEV is enabled, install IoMmu protocol otherwise install the
// placeholder protocol so that other dependent module can run.
//
if (MemEncryptSevIsEnabled ()) {
- AmdSevInstallIoMmuProtocol ();
+ Status = AmdSevInstallIoMmuProtocol ();
} else {
Handle = NULL;
Status = gBS->InstallMultipleProtocolInterfaces (
&Handle,
&gIoMmuAbsentProtocolGuid,
NULL, NULL);
}
return Status;
}
--
2.13.1.3.g8be5a757fa67
^ permalink raw reply related [flat|nested] 22+ messages in thread
* [PATCH 07/12] OvmfPkg/IoMmuDxe: clean up used library classes
2017-08-02 21:24 [PATCH 00/12] OvmfPkg/IoMmuDxe: cleanups and fixes Laszlo Ersek
` (5 preceding siblings ...)
2017-08-02 21:24 ` [PATCH 06/12] OvmfPkg/IoMmuDxe: propagate errors from AmdSevInstallIoMmuProtocol() Laszlo Ersek
@ 2017-08-02 21:24 ` Laszlo Ersek
2017-08-02 21:24 ` [PATCH 08/12] OvmfPkg/IoMmuDxe: zero out pages before releasing them Laszlo Ersek
` (6 subsequent siblings)
13 siblings, 0 replies; 22+ messages in thread
From: Laszlo Ersek @ 2017-08-02 21:24 UTC (permalink / raw)
To: edk2-devel-01; +Cc: Ard Biesheuvel, Brijesh Singh, Jordan Justen, Tom Lendacky
The following library classes are not used by this module, so remove them
from the INF file's [LibraryClasses] section:
- DxeServicesTableLib
- UefiLib
The following library classes are used by this module, so add them to the
INF file's [LibraryClasses] section:
- BaseMemoryLib (e.g. via CopyMem())
- MemoryAllocationLib (e.g. via AllocatePool())
Sort the list of library classes (in both "IoMmuDxe.inf" and
"AmdSevIoMmu.h").
Remove all non-local #include directives from "IoMmuDxe.c"; both C files
of this module include "AmdSevIoMmu.h", and "AmdSevIoMmu.h" includes all
non-local headers already.
Cc: Ard Biesheuvel <ard.biesheuvel@linaro.org>
Cc: Brijesh Singh <brijesh.singh@amd.com>
Cc: Jordan Justen <jordan.l.justen@intel.com>
Cc: Tom Lendacky <thomas.lendacky@amd.com>
Contributed-under: TianoCore Contribution Agreement 1.0
Signed-off-by: Laszlo Ersek <lersek@redhat.com>
---
OvmfPkg/IoMmuDxe/IoMmuDxe.inf | 8 ++++----
OvmfPkg/IoMmuDxe/AmdSevIoMmu.h | 4 ++--
OvmfPkg/IoMmuDxe/IoMmuDxe.c | 9 ---------
3 files changed, 6 insertions(+), 15 deletions(-)
diff --git a/OvmfPkg/IoMmuDxe/IoMmuDxe.inf b/OvmfPkg/IoMmuDxe/IoMmuDxe.inf
index 21dc39b9233a..307849706800 100644
--- a/OvmfPkg/IoMmuDxe/IoMmuDxe.inf
+++ b/OvmfPkg/IoMmuDxe/IoMmuDxe.inf
@@ -35,12 +35,12 @@ [Packages]
[LibraryClasses]
BaseLib
- UefiLib
- UefiDriverEntryPoint
- UefiBootServicesTableLib
- DxeServicesTableLib
+ BaseMemoryLib
DebugLib
MemEncryptSevLib
+ MemoryAllocationLib
+ UefiBootServicesTableLib
+ UefiDriverEntryPoint
[Protocols]
gEdkiiIoMmuProtocolGuid ## SOMETIME_PRODUCES
diff --git a/OvmfPkg/IoMmuDxe/AmdSevIoMmu.h b/OvmfPkg/IoMmuDxe/AmdSevIoMmu.h
index 0f2155350817..bdd83956aab4 100644
--- a/OvmfPkg/IoMmuDxe/AmdSevIoMmu.h
+++ b/OvmfPkg/IoMmuDxe/AmdSevIoMmu.h
@@ -1,36 +1,36 @@
/** @file
The protocol provides support to allocate, free, map and umap a DMA buffer
for bus master (e.g PciHostBridge). When SEV is enabled, the DMA operations
must be performed on unencrypted buffer hence protocol clear the encryption
bit from the DMA buffer.
Copyright (c) 2017, Intel Corporation. All rights reserved.<BR>
Copyright (c) 2017, AMD Inc. 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
distribution. The full text of the license may be found at
http://opensource.org/licenses/bsd-license.php
THE PROGRAM IS DISTRIBUTED UNDER THE BSD LICENSE ON AN "AS IS" BASIS,
WITHOUT WARRANTIES OR REPRESENTATIONS OF ANY KIND, EITHER EXPRESS OR IMPLIED.
**/
#ifndef __AMD_SEV_IOMMU_H_
#define __AMD_SEV_IOMMU_H
#include <Protocol/IoMmu.h>
#include <Library/BaseLib.h>
-#include <Library/DebugLib.h>
#include <Library/BaseMemoryLib.h>
+#include <Library/DebugLib.h>
+#include <Library/MemEncryptSevLib.h>
#include <Library/MemoryAllocationLib.h>
#include <Library/UefiBootServicesTableLib.h>
-#include <Library/MemEncryptSevLib.h>
/**
Install IOMMU protocol to provide the DMA support for PciHostBridge and
MemEncryptSevLib.
**/
diff --git a/OvmfPkg/IoMmuDxe/IoMmuDxe.c b/OvmfPkg/IoMmuDxe/IoMmuDxe.c
index 0ea42cbc13ce..70d30ea91627 100644
--- a/OvmfPkg/IoMmuDxe/IoMmuDxe.c
+++ b/OvmfPkg/IoMmuDxe/IoMmuDxe.c
@@ -1,29 +1,20 @@
/** @file
IoMmuDxe driver installs EDKII_IOMMU_PROTOCOL to provide the support for DMA
operations when SEV is enabled.
Copyright (c) 2017, AMD Inc. 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
distribution. The full text of the license may be found at
http://opensource.org/licenses/bsd-license.php
THE PROGRAM IS DISTRIBUTED UNDER THE BSD LICENSE ON AN "AS IS" BASIS,
WITHOUT WARRANTIES OR REPRESENTATIONS OF ANY KIND, EITHER EXPRESS OR IMPLIED.
**/
-#include <PiDxe.h>
-
-#include <Library/BaseLib.h>
-#include <Library/DebugLib.h>
-#include <Library/BaseMemoryLib.h>
-#include <Library/MemoryAllocationLib.h>
-#include <Library/UefiBootServicesTableLib.h>
-#include <Library/MemEncryptSevLib.h>
-
#include "AmdSevIoMmu.h"
EFI_STATUS
--
2.13.1.3.g8be5a757fa67
^ permalink raw reply related [flat|nested] 22+ messages in thread
* [PATCH 08/12] OvmfPkg/IoMmuDxe: zero out pages before releasing them
2017-08-02 21:24 [PATCH 00/12] OvmfPkg/IoMmuDxe: cleanups and fixes Laszlo Ersek
` (6 preceding siblings ...)
2017-08-02 21:24 ` [PATCH 07/12] OvmfPkg/IoMmuDxe: clean up used library classes Laszlo Ersek
@ 2017-08-02 21:24 ` Laszlo Ersek
2017-08-02 21:24 ` [PATCH 09/12] OvmfPkg/IoMmuDxe: rework setup of "MapInfo->PlainTextAddress" in Map() Laszlo Ersek
` (5 subsequent siblings)
13 siblings, 0 replies; 22+ messages in thread
From: Laszlo Ersek @ 2017-08-02 21:24 UTC (permalink / raw)
To: edk2-devel-01; +Cc: Ard Biesheuvel, Brijesh Singh, Jordan Justen, Tom Lendacky
Whenever we release the plaintext bounce buffer pages that were allocated
implicitly in Map() for BusMasterRead[64] and BusMasterWrite[64], we
restore the encryption mask on them. However, we should also rewrite the
area (fill it with zeros) so that the hypervisor is not left with a
plaintext view of the earlier data.
Similarly, whenever we release the plaintext common buffer pages that were
allocated explicitly in AllocateBuffer() for BusMasterCommonBuffer[64], we
restore the encryption mask on them. However, we should also rewrite the
area (fill it with zeros) so that the hypervisor is not left with a
plaintext view of the earlier data.
Cc: Ard Biesheuvel <ard.biesheuvel@linaro.org>
Cc: Brijesh Singh <brijesh.singh@amd.com>
Cc: Jordan Justen <jordan.l.justen@intel.com>
Cc: Tom Lendacky <thomas.lendacky@amd.com>
Contributed-under: TianoCore Contribution Agreement 1.0
Signed-off-by: Laszlo Ersek <lersek@redhat.com>
---
OvmfPkg/IoMmuDxe/AmdSevIoMmu.c | 5 +++++
1 file changed, 5 insertions(+)
diff --git a/OvmfPkg/IoMmuDxe/AmdSevIoMmu.c b/OvmfPkg/IoMmuDxe/AmdSevIoMmu.c
index 8c2c23356a40..d899b0ab9e41 100644
--- a/OvmfPkg/IoMmuDxe/AmdSevIoMmu.c
+++ b/OvmfPkg/IoMmuDxe/AmdSevIoMmu.c
@@ -227,87 +227,91 @@ EFIAPI
IoMmuUnmap (
IN EDKII_IOMMU_PROTOCOL *This,
IN VOID *Mapping
)
{
MAP_INFO *MapInfo;
EFI_STATUS Status;
if (Mapping == NULL) {
return EFI_INVALID_PARAMETER;
}
//
// See if the Map() operation associated with this Unmap() required a mapping
// buffer. If a mapping buffer was not required, then this function simply
// buffer. If a mapping buffer was not required, then this function simply
//
if (Mapping == NO_MAPPING) {
return EFI_SUCCESS;
}
MapInfo = (MAP_INFO *)Mapping;
//
// If this is a write operation from the Bus Master's point of view,
// then copy the contents of the mapped buffer into the real buffer
// so the processor can read the contents of the real buffer.
//
if (MapInfo->Operation == EdkiiIoMmuOperationBusMasterWrite ||
MapInfo->Operation == EdkiiIoMmuOperationBusMasterWrite64) {
CopyMem (
(VOID *) (UINTN) MapInfo->CryptedAddress,
(VOID *) (UINTN) MapInfo->PlainTextAddress,
MapInfo->NumberOfBytes
);
}
DEBUG ((
DEBUG_VERBOSE,
"%a PlainText 0x%Lx Crypted 0x%Lx Pages 0x%Lx Bytes 0x%Lx\n",
__FUNCTION__,
MapInfo->PlainTextAddress,
MapInfo->CryptedAddress,
(UINT64)MapInfo->NumberOfPages,
(UINT64)MapInfo->NumberOfBytes
));
//
// Restore the memory encryption mask
//
Status = MemEncryptSevSetPageEncMask (
0,
MapInfo->PlainTextAddress,
MapInfo->NumberOfPages,
TRUE
);
ASSERT_EFI_ERROR(Status);
+ ZeroMem (
+ (VOID*)(UINTN)MapInfo->PlainTextAddress,
+ EFI_PAGES_TO_SIZE (MapInfo->NumberOfPages)
+ );
//
// Free the mapped buffer and the MAP_INFO structure.
//
gBS->FreePages (MapInfo->PlainTextAddress, MapInfo->NumberOfPages);
FreePool (Mapping);
return EFI_SUCCESS;
}
/**
Allocates pages that are suitable for an OperationBusMasterCommonBuffer or
OperationBusMasterCommonBuffer64 mapping.
@param This The protocol instance pointer.
@param Type This parameter is not used and must be ignored.
@param MemoryType The type of memory to allocate,
EfiBootServicesData or EfiRuntimeServicesData.
@param Pages The number of pages to allocate.
@param HostAddress A pointer to store the base system memory
address of the allocated range.
@param Attributes The requested bit mask of attributes for the
allocated range.
@retval EFI_SUCCESS The requested memory pages were allocated.
@retval EFI_UNSUPPORTED Attributes is unsupported. The only legal
attribute bits are MEMORY_WRITE_COMBINE and
MEMORY_CACHED.
@retval EFI_INVALID_PARAMETER One or more parameters are invalid.
@retval EFI_OUT_OF_RESOURCES The memory pages could not be allocated.
**/
@@ -399,78 +403,79 @@ EFIAPI
IoMmuFreeBuffer (
IN EDKII_IOMMU_PROTOCOL *This,
IN UINTN Pages,
IN VOID *HostAddress
)
{
EFI_STATUS Status;
//
// Set memory encryption mask
//
Status = MemEncryptSevSetPageEncMask (
0,
(EFI_PHYSICAL_ADDRESS)(UINTN)HostAddress,
Pages,
TRUE
);
ASSERT_EFI_ERROR(Status);
+ ZeroMem (HostAddress, EFI_PAGES_TO_SIZE (Pages));
DEBUG ((
DEBUG_VERBOSE,
"%a Address 0x%Lx Pages 0x%Lx\n",
__FUNCTION__,
(UINT64)(UINTN)HostAddress,
(UINT64)Pages
));
return gBS->FreePages ((EFI_PHYSICAL_ADDRESS) (UINTN) HostAddress, Pages);
}
/**
Set IOMMU attribute for a system memory.
If the IOMMU protocol exists, the system memory cannot be used
for DMA by default.
When a device requests a DMA access for a system memory,
the device driver need use SetAttribute() to update the IOMMU
attribute to request DMA access (read and/or write).
The DeviceHandle is used to identify which device submits the request.
The IOMMU implementation need translate the device path to an IOMMU device
ID, and set IOMMU hardware register accordingly.
1) DeviceHandle can be a standard PCI device.
The memory for BusMasterRead need set EDKII_IOMMU_ACCESS_READ.
The memory for BusMasterWrite need set EDKII_IOMMU_ACCESS_WRITE.
The memory for BusMasterCommonBuffer need set
EDKII_IOMMU_ACCESS_READ|EDKII_IOMMU_ACCESS_WRITE.
After the memory is used, the memory need set 0 to keep it being
protected.
2) DeviceHandle can be an ACPI device (ISA, I2C, SPI, etc).
The memory for DMA access need set EDKII_IOMMU_ACCESS_READ and/or
EDKII_IOMMU_ACCESS_WRITE.
@param[in] This The protocol instance pointer.
@param[in] DeviceHandle The device who initiates the DMA access
request.
@param[in] Mapping The mapping value returned from Map().
@param[in] IoMmuAccess The IOMMU access.
@retval EFI_SUCCESS The IoMmuAccess is set for the memory range
specified by DeviceAddress and Length.
@retval EFI_INVALID_PARAMETER DeviceHandle is an invalid handle.
@retval EFI_INVALID_PARAMETER Mapping is not a value that was returned by
Map().
@retval EFI_INVALID_PARAMETER IoMmuAccess specified an illegal combination
of access.
@retval EFI_UNSUPPORTED DeviceHandle is unknown by the IOMMU.
@retval EFI_UNSUPPORTED The bit mask of IoMmuAccess is not supported
by the IOMMU.
@retval EFI_UNSUPPORTED The IOMMU does not support the memory range
specified by Mapping.
@retval EFI_OUT_OF_RESOURCES There are not enough resources available to
modify the IOMMU access.
@retval EFI_DEVICE_ERROR The IOMMU device reported an error while
attempting the operation.
**/
--
2.13.1.3.g8be5a757fa67
^ permalink raw reply related [flat|nested] 22+ messages in thread
* [PATCH 09/12] OvmfPkg/IoMmuDxe: rework setup of "MapInfo->PlainTextAddress" in Map()
2017-08-02 21:24 [PATCH 00/12] OvmfPkg/IoMmuDxe: cleanups and fixes Laszlo Ersek
` (7 preceding siblings ...)
2017-08-02 21:24 ` [PATCH 08/12] OvmfPkg/IoMmuDxe: zero out pages before releasing them Laszlo Ersek
@ 2017-08-02 21:24 ` Laszlo Ersek
2017-08-02 21:24 ` [PATCH 10/12] OvmfPkg/IoMmuDxe: implement in-place decryption/encryption for Map/Unmap Laszlo Ersek
` (4 subsequent siblings)
13 siblings, 0 replies; 22+ messages in thread
From: Laszlo Ersek @ 2017-08-02 21:24 UTC (permalink / raw)
To: edk2-devel-01; +Cc: Ard Biesheuvel, Brijesh Singh, Jordan Justen, Tom Lendacky
There are three issues with the current calculations:
- The initial logic that sets up "DmaMemoryTop" and "AllocateType" checks
for the BusMasterCommonBuffer64 operation in two places. The inner check
for BusMasterCommonBuffer64 will never evaluate to TRUE however, because
the outer check excludes BusMasterCommonBuffer64.
- In order to lower "DmaMemoryTop" to (SIZE_4GB - 1), the outer check
requires that the encrypted (original) buffer cross the 4GB mark. This
is wrong: for BusMasterRead[64] and BusMasterWrite[64] operations, we
unconditionally need a bounce buffer (a decrypted memory area), and for
the 32-bit variants, "DmaMemoryTop" should be lowered regardless of the
location of the original (encrypted) buffer.
- The current logic would be hard to extend for the in-place decryption
that we'll implement in the next patch.
Therefore rework the "MapInfo->PlainTextAddress" setup. No functional
changes beyond said bugfixes.
Cc: Ard Biesheuvel <ard.biesheuvel@linaro.org>
Cc: Brijesh Singh <brijesh.singh@amd.com>
Cc: Jordan Justen <jordan.l.justen@intel.com>
Cc: Tom Lendacky <thomas.lendacky@amd.com>
Contributed-under: TianoCore Contribution Agreement 1.0
Signed-off-by: Laszlo Ersek <lersek@redhat.com>
---
OvmfPkg/IoMmuDxe/AmdSevIoMmu.c | 156 +++++++++++---------
1 file changed, 87 insertions(+), 69 deletions(-)
diff --git a/OvmfPkg/IoMmuDxe/AmdSevIoMmu.c b/OvmfPkg/IoMmuDxe/AmdSevIoMmu.c
index d899b0ab9e41..0a85ee6559e7 100644
--- a/OvmfPkg/IoMmuDxe/AmdSevIoMmu.c
+++ b/OvmfPkg/IoMmuDxe/AmdSevIoMmu.c
@@ -65,160 +65,178 @@ EFIAPI
IoMmuMap (
IN EDKII_IOMMU_PROTOCOL *This,
IN EDKII_IOMMU_OPERATION Operation,
IN VOID *HostAddress,
IN OUT UINTN *NumberOfBytes,
OUT EFI_PHYSICAL_ADDRESS *DeviceAddress,
OUT VOID **Mapping
)
{
EFI_STATUS Status;
- EFI_PHYSICAL_ADDRESS PhysicalAddress;
MAP_INFO *MapInfo;
- EFI_PHYSICAL_ADDRESS DmaMemoryTop;
EFI_ALLOCATE_TYPE AllocateType;
if (HostAddress == NULL || NumberOfBytes == NULL || DeviceAddress == NULL ||
Mapping == NULL) {
return EFI_INVALID_PARAMETER;
}
//
- // Make sure that Operation is valid
- //
- if ((UINT32) Operation >= EdkiiIoMmuOperationMaximum) {
- return EFI_INVALID_PARAMETER;
- }
- PhysicalAddress = (EFI_PHYSICAL_ADDRESS) (UINTN) HostAddress;
-
- DmaMemoryTop = (UINTN)-1;
- AllocateType = AllocateAnyPages;
-
- if (((Operation != EdkiiIoMmuOperationBusMasterRead64 &&
- Operation != EdkiiIoMmuOperationBusMasterWrite64 &&
- Operation != EdkiiIoMmuOperationBusMasterCommonBuffer64)) &&
- ((PhysicalAddress + *NumberOfBytes) > SIZE_4GB)) {
- //
- // If the root bridge or the device cannot handle performing DMA above
- // 4GB but any part of the DMA transfer being mapped is above 4GB, then
- // map the DMA transfer to a buffer below 4GB.
- //
- DmaMemoryTop = SIZE_4GB - 1;
- AllocateType = AllocateMaxAddress;
-
- if (Operation == EdkiiIoMmuOperationBusMasterCommonBuffer ||
- Operation == EdkiiIoMmuOperationBusMasterCommonBuffer64) {
- //
- // Common Buffer operations can not be remapped. If the common buffer
- // if above 4GB, then it is not possible to generate a mapping, so
- // return an error.
- //
- return EFI_UNSUPPORTED;
- }
- }
-
- //
- // CommandBuffer was allocated by us (AllocateBuffer) and is already in
- // unencryted buffer so no need to create bounce buffer
- //
- if (Operation == EdkiiIoMmuOperationBusMasterCommonBuffer ||
- Operation == EdkiiIoMmuOperationBusMasterCommonBuffer64) {
- *Mapping = NO_MAPPING;
- *DeviceAddress = PhysicalAddress;
-
- return EFI_SUCCESS;
- }
-
- //
// Allocate a MAP_INFO structure to remember the mapping when Unmap() is
// called later.
//
MapInfo = AllocatePool (sizeof (MAP_INFO));
if (MapInfo == NULL) {
- *NumberOfBytes = 0;
- return EFI_OUT_OF_RESOURCES;
+ Status = EFI_OUT_OF_RESOURCES;
+ goto Failed;
}
//
- // Initialize the MAP_INFO structure
+ // Initialize the MAP_INFO structure, except the PlainTextAddress field
//
MapInfo->Operation = Operation;
MapInfo->NumberOfBytes = *NumberOfBytes;
MapInfo->NumberOfPages = EFI_SIZE_TO_PAGES (MapInfo->NumberOfBytes);
- MapInfo->CryptedAddress = PhysicalAddress;
- MapInfo->PlainTextAddress = DmaMemoryTop;
+ MapInfo->CryptedAddress = (UINTN)HostAddress;
//
- // Allocate a buffer to map the transfer to.
+ // In the switch statement below, we point "MapInfo->PlainTextAddress" to the
+ // plaintext buffer, according to Operation.
//
- Status = gBS->AllocatePages (
- AllocateType,
- EfiBootServicesData,
- MapInfo->NumberOfPages,
- &MapInfo->PlainTextAddress
- );
- if (EFI_ERROR (Status)) {
+ MapInfo->PlainTextAddress = MAX_ADDRESS;
+ AllocateType = AllocateAnyPages;
+ switch (Operation) {
+ //
+ // For BusMasterRead[64] and BusMasterWrite[64] operations, a bounce buffer
+ // is necessary regardless of whether the original (crypted) buffer crosses
+ // the 4GB limit or not -- we have to allocate a separate plaintext buffer.
+ // The only variable is whether the plaintext buffer should be under 4GB.
+ //
+ case EdkiiIoMmuOperationBusMasterRead:
+ case EdkiiIoMmuOperationBusMasterWrite:
+ MapInfo->PlainTextAddress = BASE_4GB - 1;
+ AllocateType = AllocateMaxAddress;
+ //
+ // fall through
+ //
+ case EdkiiIoMmuOperationBusMasterRead64:
+ case EdkiiIoMmuOperationBusMasterWrite64:
+ //
+ // Allocate the implicit plaintext bounce buffer.
+ //
+ Status = gBS->AllocatePages (
+ AllocateType,
+ EfiBootServicesData,
+ MapInfo->NumberOfPages,
+ &MapInfo->PlainTextAddress
+ );
+ if (EFI_ERROR (Status)) {
+ goto FreeMapInfo;
+ }
+ break;
+
+ //
+ // For BusMasterCommonBuffer[64] operations, a plaintext buffer has been
+ // allocated already, with AllocateBuffer(). We only check whether the
+ // address is low enough for the requested operation.
+ //
+ case EdkiiIoMmuOperationBusMasterCommonBuffer:
+ if ((MapInfo->CryptedAddress > BASE_4GB) ||
+ (EFI_PAGES_TO_SIZE (MapInfo->NumberOfPages) >
+ BASE_4GB - MapInfo->CryptedAddress)) {
+ //
+ // CommonBuffer operations cannot be remapped. If the common buffer is
+ // above 4GB, then it is not possible to generate a mapping, so return an
+ // error.
+ //
+ Status = EFI_UNSUPPORTED;
+ goto FreeMapInfo;
+ }
+ //
+ // fall through
+ //
+ case EdkiiIoMmuOperationBusMasterCommonBuffer64:
+ //
+ // The buffer at MapInfo->CryptedAddress comes from AllocateBuffer(),
+ // and it is already decrypted.
+ //
+ MapInfo->PlainTextAddress = MapInfo->CryptedAddress;
+
+ //
+ // Therefore no mapping is necessary.
+ //
+ *DeviceAddress = MapInfo->PlainTextAddress;
+ *Mapping = NO_MAPPING;
FreePool (MapInfo);
- *NumberOfBytes = 0;
- return Status;
+ return EFI_SUCCESS;
+
+ default:
+ //
+ // Operation is invalid
+ //
+ Status = EFI_INVALID_PARAMETER;
+ goto FreeMapInfo;
}
//
- // Clear the memory encryption mask from the device buffer
+ // Clear the memory encryption mask on the plaintext buffer.
//
Status = MemEncryptSevClearPageEncMask (
0,
MapInfo->PlainTextAddress,
MapInfo->NumberOfPages,
TRUE
);
ASSERT_EFI_ERROR(Status);
//
// If this is a read operation from the Bus Master's point of view,
// then copy the contents of the real buffer into the mapped buffer
// so the Bus Master can read the contents of the real buffer.
//
if (Operation == EdkiiIoMmuOperationBusMasterRead ||
Operation == EdkiiIoMmuOperationBusMasterRead64) {
CopyMem (
(VOID *) (UINTN) MapInfo->PlainTextAddress,
(VOID *) (UINTN) MapInfo->CryptedAddress,
MapInfo->NumberOfBytes
);
}
//
- // The DeviceAddress is the address of the maped buffer below 4GB
+ // Populate output parameters.
//
*DeviceAddress = MapInfo->PlainTextAddress;
-
- //
- // Return a pointer to the MAP_INFO structure in Mapping
- //
*Mapping = MapInfo;
DEBUG ((
DEBUG_VERBOSE,
"%a PlainText 0x%Lx Crypted 0x%Lx Pages 0x%Lx Bytes 0x%Lx\n",
__FUNCTION__,
MapInfo->PlainTextAddress,
MapInfo->CryptedAddress,
(UINT64)MapInfo->NumberOfPages,
(UINT64)MapInfo->NumberOfBytes
));
return EFI_SUCCESS;
+
+FreeMapInfo:
+ FreePool (MapInfo);
+
+Failed:
+ *NumberOfBytes = 0;
+ return Status;
}
/**
Completes the Map() operation and releases any corresponding resources.
@param This The protocol instance pointer.
@param Mapping The mapping value returned from Map().
@retval EFI_SUCCESS The range was unmapped.
@retval EFI_INVALID_PARAMETER Mapping is not a value that was returned by
Map().
@retval EFI_DEVICE_ERROR The data was not committed to the target system
memory.
**/
--
2.13.1.3.g8be5a757fa67
^ permalink raw reply related [flat|nested] 22+ messages in thread
* [PATCH 10/12] OvmfPkg/IoMmuDxe: implement in-place decryption/encryption for Map/Unmap
2017-08-02 21:24 [PATCH 00/12] OvmfPkg/IoMmuDxe: cleanups and fixes Laszlo Ersek
` (8 preceding siblings ...)
2017-08-02 21:24 ` [PATCH 09/12] OvmfPkg/IoMmuDxe: rework setup of "MapInfo->PlainTextAddress" in Map() Laszlo Ersek
@ 2017-08-02 21:24 ` Laszlo Ersek
2017-08-02 23:01 ` Brijesh Singh
2017-08-02 21:24 ` [PATCH 11/12] OvmfPkg/IoMmuDxe: abort harder on memory encryption mask failures Laszlo Ersek
` (3 subsequent siblings)
13 siblings, 1 reply; 22+ messages in thread
From: Laszlo Ersek @ 2017-08-02 21:24 UTC (permalink / raw)
To: edk2-devel-01; +Cc: Ard Biesheuvel, Brijesh Singh, Jordan Justen, Tom Lendacky
At the moment, we have the following distribution of actions between the
IOMMU protocol member functions:
- AllocateBuffer() allocates pages and clears the memory encryption mask.
- FreeBuffer() re-sets the memory encryption mask, and deallocates pages.
- Map() does nothing at all when BusMasterCommonBuffer[64] is requested
(and AllocateBuffer() was called previously). Otherwise, Map() allocates
pages, and clears the memory encryption mask.
- Unmap() does nothing when cleaning up a BusMasterCommonBuffer[64]
operation. Otherwise, Unmap() clears the encryption mask, and frees the
pages.
This is wrong: the AllocateBuffer() protocol member is not expected to
produce a buffer that is immediately usable, and client code is required
to call Map() unconditionally, even if BusMasterCommonBuffer[64] is the
desired operation. Implement the right distribution of actions as follows:
- AllocateBuffer() allocates pages and does not touch the encryption mask.
- FreeBuffer() deallocates pages and does not touch the encryption mask.
- Map() does not allocate pages when BusMasterCommonBuffer[64] is
requested, and it allocates pages (bounce buffer) otherwise. Regardless
of the BusMaster operation, Map() (and Map() only) clears the memory
encryption mask.
- Unmap() restores the encryption mask unconditionally. If the operation
was BusMasterCommonBuffer[64], then Unmap() does not release the pages.
Otherwise, the pages (bounce buffer) are released.
This approach also ensures that Unmap() can be called from
ExitBootServices() event handlers, for cleaning up
BusMasterCommonBuffer[64] operations. (More specifically, for restoring
the SEV encryption mask on any in-flight buffers, after resetting any
referring devices.) ExitBootServices() event handlers must not change the
UEFI memory map, thus any memory allocation or freeing in Unmap() would
disqualify Unmap() from being called in such a context.
Map()-ing and Unmap()-ing memory for a BusMasterCommonBuffer[64] operation
effectively means in-place decryption and encryption in a SEV context. As
an additional hurdle, section "7.10.8 Encrypt-in-Place" of AMD publication
Nr.24593 implies that we need a separate temporary buffer for decryption
and encryption that will eventually land in-place. Allocating said
temporary buffer in the straightforward way would violate the above
allocation/freeing restrictions on Map()/Unmap(), therefore pre-allocate
this "stash buffer" too in AllocateBuffer(), and free it in FreeBuffer().
To completely rid Unmap() of dynamic memory impact, for
BusMasterCommonBuffer[64] operations, we're going to rework the lifecycle of
the MAP_INFO structures in a later patch.
(The MemEncryptSevSetPageEncMask() call in Unmap() could theoretically
allocate memory internally for page splitting, however this won't happen
in practice: in Unmap() we only restore the memory encryption mask, and
don't genuinely set it. Any page splitting will have occurred in Map()'s
MemEncryptSevClearPageEncMask() call first.)
Cc: Ard Biesheuvel <ard.biesheuvel@linaro.org>
Cc: Brijesh Singh <brijesh.singh@amd.com>
Cc: Jordan Justen <jordan.l.justen@intel.com>
Cc: Tom Lendacky <thomas.lendacky@amd.com>
Contributed-under: TianoCore Contribution Agreement 1.0
Signed-off-by: Laszlo Ersek <lersek@redhat.com>
---
OvmfPkg/IoMmuDxe/AmdSevIoMmu.c | 242 +++++++++++++++-----
1 file changed, 185 insertions(+), 57 deletions(-)
diff --git a/OvmfPkg/IoMmuDxe/AmdSevIoMmu.c b/OvmfPkg/IoMmuDxe/AmdSevIoMmu.c
index 0a85ee6559e7..5049d19e9cb7 100644
--- a/OvmfPkg/IoMmuDxe/AmdSevIoMmu.c
+++ b/OvmfPkg/IoMmuDxe/AmdSevIoMmu.c
@@ -23,40 +23,64 @@
typedef struct {
EDKII_IOMMU_OPERATION Operation;
UINTN NumberOfBytes;
UINTN NumberOfPages;
EFI_PHYSICAL_ADDRESS CryptedAddress;
EFI_PHYSICAL_ADDRESS PlainTextAddress;
} MAP_INFO;
-#define NO_MAPPING (VOID *) (UINTN) -1
+#define COMMON_BUFFER_SIG SIGNATURE_64 ('C', 'M', 'N', 'B', 'U', 'F', 'F', 'R')
+
+//
+// The following structure enables Map() and Unmap() to perform in-place
+// decryption and encryption, respectively, for BusMasterCommonBuffer[64]
+// operations, without dynamic memory allocation or release.
+//
+// Both COMMON_BUFFER_HEADER and COMMON_BUFFER_HEADER.StashBuffer are allocated
+// by AllocateBuffer() and released by FreeBuffer().
+//
+#pragma pack (1)
+typedef struct {
+ UINT64 Signature;
+
+ //
+ // Always allocated from EfiBootServicesData type memory, and always
+ // encrypted.
+ //
+ VOID *StashBuffer;
+
+ //
+ // Followed by the actual common buffer, starting at the next page.
+ //
+} COMMON_BUFFER_HEADER;
+#pragma pack ()
/**
Provides the controller-specific addresses required to access system memory
from a DMA bus master. On SEV guest, the DMA operations must be performed on
shared buffer hence we allocate a bounce buffer to map the HostAddress to a
DeviceAddress. The Encryption attribute is removed from the DeviceAddress
buffer.
@param This The protocol instance pointer.
@param Operation Indicates if the bus master is going to read or
write to system memory.
@param HostAddress The system memory address to map to the PCI
controller.
@param NumberOfBytes On input the number of bytes to map. On output
the number of bytes that were mapped.
@param DeviceAddress The resulting map address for the bus master
PCI controller to use to access the hosts
HostAddress.
@param Mapping A resulting value to pass to Unmap().
@retval EFI_SUCCESS The range was mapped for the returned
NumberOfBytes.
@retval EFI_UNSUPPORTED The HostAddress cannot be mapped as a common
buffer.
@retval EFI_INVALID_PARAMETER One or more parameters are invalid.
@retval EFI_OUT_OF_RESOURCES The request could not be completed due to a
lack of resources.
@retval EFI_DEVICE_ERROR The system hardware could not map the requested
address.
**/
@@ -65,157 +89,175 @@ EFIAPI
IoMmuMap (
IN EDKII_IOMMU_PROTOCOL *This,
IN EDKII_IOMMU_OPERATION Operation,
IN VOID *HostAddress,
IN OUT UINTN *NumberOfBytes,
OUT EFI_PHYSICAL_ADDRESS *DeviceAddress,
OUT VOID **Mapping
)
{
EFI_STATUS Status;
MAP_INFO *MapInfo;
EFI_ALLOCATE_TYPE AllocateType;
+ COMMON_BUFFER_HEADER *CommonBufferHeader;
+ VOID *DecryptionSource;
if (HostAddress == NULL || NumberOfBytes == NULL || DeviceAddress == NULL ||
Mapping == NULL) {
return EFI_INVALID_PARAMETER;
}
//
// Allocate a MAP_INFO structure to remember the mapping when Unmap() is
// called later.
//
MapInfo = AllocatePool (sizeof (MAP_INFO));
if (MapInfo == NULL) {
Status = EFI_OUT_OF_RESOURCES;
goto Failed;
}
//
// Initialize the MAP_INFO structure, except the PlainTextAddress field
//
MapInfo->Operation = Operation;
MapInfo->NumberOfBytes = *NumberOfBytes;
MapInfo->NumberOfPages = EFI_SIZE_TO_PAGES (MapInfo->NumberOfBytes);
MapInfo->CryptedAddress = (UINTN)HostAddress;
//
// In the switch statement below, we point "MapInfo->PlainTextAddress" to the
- // plaintext buffer, according to Operation.
+ // plaintext buffer, according to Operation. We also set "DecryptionSource".
//
MapInfo->PlainTextAddress = MAX_ADDRESS;
AllocateType = AllocateAnyPages;
+ DecryptionSource = (VOID *)(UINTN)MapInfo->CryptedAddress;
switch (Operation) {
//
// For BusMasterRead[64] and BusMasterWrite[64] operations, a bounce buffer
// is necessary regardless of whether the original (crypted) buffer crosses
// the 4GB limit or not -- we have to allocate a separate plaintext buffer.
// The only variable is whether the plaintext buffer should be under 4GB.
//
case EdkiiIoMmuOperationBusMasterRead:
case EdkiiIoMmuOperationBusMasterWrite:
MapInfo->PlainTextAddress = BASE_4GB - 1;
AllocateType = AllocateMaxAddress;
//
// fall through
//
case EdkiiIoMmuOperationBusMasterRead64:
case EdkiiIoMmuOperationBusMasterWrite64:
//
// Allocate the implicit plaintext bounce buffer.
//
Status = gBS->AllocatePages (
AllocateType,
EfiBootServicesData,
MapInfo->NumberOfPages,
&MapInfo->PlainTextAddress
);
if (EFI_ERROR (Status)) {
goto FreeMapInfo;
}
break;
//
- // For BusMasterCommonBuffer[64] operations, a plaintext buffer has been
- // allocated already, with AllocateBuffer(). We only check whether the
- // address is low enough for the requested operation.
+ // For BusMasterCommonBuffer[64] operations, a to-be-plaintext buffer and a
+ // stash buffer (for in-place decryption) have been allocated already, with
+ // AllocateBuffer(). We only check whether the address of the to-be-plaintext
+ // buffer is low enough for the requested operation.
//
case EdkiiIoMmuOperationBusMasterCommonBuffer:
if ((MapInfo->CryptedAddress > BASE_4GB) ||
(EFI_PAGES_TO_SIZE (MapInfo->NumberOfPages) >
BASE_4GB - MapInfo->CryptedAddress)) {
//
// CommonBuffer operations cannot be remapped. If the common buffer is
// above 4GB, then it is not possible to generate a mapping, so return an
// error.
//
Status = EFI_UNSUPPORTED;
goto FreeMapInfo;
}
//
// fall through
//
case EdkiiIoMmuOperationBusMasterCommonBuffer64:
//
- // The buffer at MapInfo->CryptedAddress comes from AllocateBuffer(),
- // and it is already decrypted.
+ // The buffer at MapInfo->CryptedAddress comes from AllocateBuffer().
//
MapInfo->PlainTextAddress = MapInfo->CryptedAddress;
-
//
- // Therefore no mapping is necessary.
+ // Stash the crypted data.
//
- *DeviceAddress = MapInfo->PlainTextAddress;
- *Mapping = NO_MAPPING;
- FreePool (MapInfo);
- return EFI_SUCCESS;
+ CommonBufferHeader = (COMMON_BUFFER_HEADER *)(
+ (UINTN)MapInfo->CryptedAddress - EFI_PAGE_SIZE
+ );
+ ASSERT (CommonBufferHeader->Signature == COMMON_BUFFER_SIG);
+ CopyMem (
+ CommonBufferHeader->StashBuffer,
+ (VOID *)(UINTN)MapInfo->CryptedAddress,
+ MapInfo->NumberOfBytes
+ );
+ //
+ // Point "DecryptionSource" to the stash buffer so that we decrypt
+ // it to the original location, after the switch statement.
+ //
+ DecryptionSource = CommonBufferHeader->StashBuffer;
+ break;
default:
//
// Operation is invalid
//
Status = EFI_INVALID_PARAMETER;
goto FreeMapInfo;
}
//
// Clear the memory encryption mask on the plaintext buffer.
//
Status = MemEncryptSevClearPageEncMask (
0,
MapInfo->PlainTextAddress,
MapInfo->NumberOfPages,
TRUE
);
ASSERT_EFI_ERROR(Status);
//
// If this is a read operation from the Bus Master's point of view,
// then copy the contents of the real buffer into the mapped buffer
// so the Bus Master can read the contents of the real buffer.
//
+ // For BusMasterCommonBuffer[64] operations, the CopyMem() below will decrypt
+ // the original data (from the stash buffer) back to the original location.
+ //
if (Operation == EdkiiIoMmuOperationBusMasterRead ||
- Operation == EdkiiIoMmuOperationBusMasterRead64) {
+ Operation == EdkiiIoMmuOperationBusMasterRead64 ||
+ Operation == EdkiiIoMmuOperationBusMasterCommonBuffer ||
+ Operation == EdkiiIoMmuOperationBusMasterCommonBuffer64) {
CopyMem (
(VOID *) (UINTN) MapInfo->PlainTextAddress,
- (VOID *) (UINTN) MapInfo->CryptedAddress,
+ DecryptionSource,
MapInfo->NumberOfBytes
);
}
//
// Populate output parameters.
//
*DeviceAddress = MapInfo->PlainTextAddress;
*Mapping = MapInfo;
DEBUG ((
DEBUG_VERBOSE,
"%a PlainText 0x%Lx Crypted 0x%Lx Pages 0x%Lx Bytes 0x%Lx\n",
__FUNCTION__,
MapInfo->PlainTextAddress,
MapInfo->CryptedAddress,
(UINT64)MapInfo->NumberOfPages,
(UINT64)MapInfo->NumberOfBytes
));
return EFI_SUCCESS;
@@ -245,91 +287,129 @@ EFIAPI
IoMmuUnmap (
IN EDKII_IOMMU_PROTOCOL *This,
IN VOID *Mapping
)
{
MAP_INFO *MapInfo;
EFI_STATUS Status;
+ COMMON_BUFFER_HEADER *CommonBufferHeader;
+ VOID *EncryptionTarget;
if (Mapping == NULL) {
return EFI_INVALID_PARAMETER;
}
- //
- // See if the Map() operation associated with this Unmap() required a mapping
- // buffer. If a mapping buffer was not required, then this function simply
- // buffer. If a mapping buffer was not required, then this function simply
- //
- if (Mapping == NO_MAPPING) {
- return EFI_SUCCESS;
- }
-
MapInfo = (MAP_INFO *)Mapping;
//
- // If this is a write operation from the Bus Master's point of view,
- // then copy the contents of the mapped buffer into the real buffer
- // so the processor can read the contents of the real buffer.
+ // For BusMasterWrite[64] operations and BusMasterCommonBuffer[64] operations
+ // we have to encrypt the results, ultimately to the original place (i.e.,
+ // "MapInfo->CryptedAddress").
//
- if (MapInfo->Operation == EdkiiIoMmuOperationBusMasterWrite ||
- MapInfo->Operation == EdkiiIoMmuOperationBusMasterWrite64) {
+ // For BusMasterCommonBuffer[64] operations however, this encryption has to
+ // land in-place, so divert the encryption to the stash buffer first.
+ //
+ EncryptionTarget = (VOID *)(UINTN)MapInfo->CryptedAddress;
+
+ switch (MapInfo->Operation) {
+ case EdkiiIoMmuOperationBusMasterCommonBuffer:
+ case EdkiiIoMmuOperationBusMasterCommonBuffer64:
+ ASSERT (MapInfo->PlainTextAddress == MapInfo->CryptedAddress);
+
+ CommonBufferHeader = (COMMON_BUFFER_HEADER *)(
+ (UINTN)MapInfo->PlainTextAddress - EFI_PAGE_SIZE
+ );
+ ASSERT (CommonBufferHeader->Signature == COMMON_BUFFER_SIG);
+ EncryptionTarget = CommonBufferHeader->StashBuffer;
+ //
+ // fall through
+ //
+
+ case EdkiiIoMmuOperationBusMasterWrite:
+ case EdkiiIoMmuOperationBusMasterWrite64:
CopyMem (
- (VOID *) (UINTN) MapInfo->CryptedAddress,
+ EncryptionTarget,
(VOID *) (UINTN) MapInfo->PlainTextAddress,
MapInfo->NumberOfBytes
);
+ break;
+
+ default:
+ //
+ // nothing to encrypt after BusMasterRead[64] operations
+ //
+ break;
}
DEBUG ((
DEBUG_VERBOSE,
"%a PlainText 0x%Lx Crypted 0x%Lx Pages 0x%Lx Bytes 0x%Lx\n",
__FUNCTION__,
MapInfo->PlainTextAddress,
MapInfo->CryptedAddress,
(UINT64)MapInfo->NumberOfPages,
(UINT64)MapInfo->NumberOfBytes
));
+
//
- // Restore the memory encryption mask
+ // Restore the memory encryption mask on the area we used to hold the
+ // plaintext.
//
Status = MemEncryptSevSetPageEncMask (
0,
MapInfo->PlainTextAddress,
MapInfo->NumberOfPages,
TRUE
);
ASSERT_EFI_ERROR(Status);
- ZeroMem (
- (VOID*)(UINTN)MapInfo->PlainTextAddress,
- EFI_PAGES_TO_SIZE (MapInfo->NumberOfPages)
- );
//
- // Free the mapped buffer and the MAP_INFO structure.
+ // For BusMasterCommonBuffer[64] operations, copy the stashed data to the
+ // original (now encrypted) location.
+ //
+ // For all other operations, fill the late bounce buffer (which existed as
+ // plaintext at some point) with zeros, and then release it.
+ //
+ if (MapInfo->Operation == EdkiiIoMmuOperationBusMasterCommonBuffer ||
+ MapInfo->Operation == EdkiiIoMmuOperationBusMasterCommonBuffer64) {
+ CopyMem (
+ (VOID *)(UINTN)MapInfo->CryptedAddress,
+ CommonBufferHeader->StashBuffer,
+ MapInfo->NumberOfBytes
+ );
+ } else {
+ ZeroMem (
+ (VOID *)(UINTN)MapInfo->PlainTextAddress,
+ EFI_PAGES_TO_SIZE (MapInfo->NumberOfPages)
+ );
+ gBS->FreePages (MapInfo->PlainTextAddress, MapInfo->NumberOfPages);
+ }
+
+ //
+ // Free the MAP_INFO structure.
//
- gBS->FreePages (MapInfo->PlainTextAddress, MapInfo->NumberOfPages);
FreePool (Mapping);
return EFI_SUCCESS;
}
/**
Allocates pages that are suitable for an OperationBusMasterCommonBuffer or
OperationBusMasterCommonBuffer64 mapping.
@param This The protocol instance pointer.
@param Type This parameter is not used and must be ignored.
@param MemoryType The type of memory to allocate,
EfiBootServicesData or EfiRuntimeServicesData.
@param Pages The number of pages to allocate.
@param HostAddress A pointer to store the base system memory
address of the allocated range.
@param Attributes The requested bit mask of attributes for the
allocated range.
@retval EFI_SUCCESS The requested memory pages were allocated.
@retval EFI_UNSUPPORTED Attributes is unsupported. The only legal
attribute bits are MEMORY_WRITE_COMBINE and
MEMORY_CACHED.
@retval EFI_INVALID_PARAMETER One or more parameters are invalid.
@retval EFI_OUT_OF_RESOURCES The memory pages could not be allocated.
**/
@@ -338,81 +418,116 @@ EFIAPI
IoMmuAllocateBuffer (
IN EDKII_IOMMU_PROTOCOL *This,
IN EFI_ALLOCATE_TYPE Type,
IN EFI_MEMORY_TYPE MemoryType,
IN UINTN Pages,
IN OUT VOID **HostAddress,
IN UINT64 Attributes
)
{
EFI_STATUS Status;
EFI_PHYSICAL_ADDRESS PhysicalAddress;
+ VOID *StashBuffer;
+ UINTN CommonBufferPages;
+ COMMON_BUFFER_HEADER *CommonBufferHeader;
//
// Validate Attributes
//
if ((Attributes & EDKII_IOMMU_ATTRIBUTE_INVALID_FOR_ALLOCATE_BUFFER) != 0) {
return EFI_UNSUPPORTED;
}
//
// Check for invalid inputs
//
if (HostAddress == NULL) {
return EFI_INVALID_PARAMETER;
}
//
// The only valid memory types are EfiBootServicesData and
// EfiRuntimeServicesData
//
if (MemoryType != EfiBootServicesData &&
MemoryType != EfiRuntimeServicesData) {
return EFI_INVALID_PARAMETER;
}
+ //
+ // We'll need a header page for the COMMON_BUFFER_HEADER structure.
+ //
+ if (Pages > MAX_UINTN - 1) {
+ return EFI_OUT_OF_RESOURCES;
+ }
+ CommonBufferPages = Pages + 1;
+
+ //
+ // Allocate the stash in EfiBootServicesData type memory.
+ //
+ // Map() will temporarily save encrypted data in the stash for
+ // BusMasterCommonBuffer[64] operations, so the data can be decrypted to the
+ // original location.
+ //
+ // Unmap() will temporarily save plaintext data in the stash for
+ // BusMasterCommonBuffer[64] operations, so the data can be encrypted to the
+ // original location.
+ //
+ // StashBuffer always resides in encrypted memory.
+ //
+ StashBuffer = AllocatePages (Pages);
+ if (StashBuffer == NULL) {
+ return EFI_OUT_OF_RESOURCES;
+ }
+
PhysicalAddress = (UINTN)-1;
if ((Attributes & EDKII_IOMMU_ATTRIBUTE_DUAL_ADDRESS_CYCLE) == 0) {
//
// Limit allocations to memory below 4GB
//
PhysicalAddress = SIZE_4GB - 1;
}
Status = gBS->AllocatePages (
AllocateMaxAddress,
MemoryType,
- Pages,
+ CommonBufferPages,
&PhysicalAddress
);
- if (!EFI_ERROR (Status)) {
- *HostAddress = (VOID *) (UINTN) PhysicalAddress;
-
- //
- // Clear memory encryption mask
- //
- Status = MemEncryptSevClearPageEncMask (0, PhysicalAddress, Pages, TRUE);
- ASSERT_EFI_ERROR(Status);
+ if (EFI_ERROR (Status)) {
+ goto FreeStashBuffer;
}
+ CommonBufferHeader = (VOID *)(UINTN)PhysicalAddress;
+ PhysicalAddress += EFI_PAGE_SIZE;
+
+ CommonBufferHeader->Signature = COMMON_BUFFER_SIG;
+ CommonBufferHeader->StashBuffer = StashBuffer;
+
+ *HostAddress = (VOID *)(UINTN)PhysicalAddress;
+
DEBUG ((
DEBUG_VERBOSE,
"%a Address 0x%Lx Pages 0x%Lx\n",
__FUNCTION__,
PhysicalAddress,
(UINT64)Pages
));
+ return EFI_SUCCESS;
+
+FreeStashBuffer:
+ FreePages (StashBuffer, Pages);
return Status;
}
/**
Frees memory that was allocated with AllocateBuffer().
@param This The protocol instance pointer.
@param Pages The number of pages to free.
@param HostAddress The base system memory address of the allocated
range.
@retval EFI_SUCCESS The requested memory pages were freed.
@retval EFI_INVALID_PARAMETER The memory range specified by HostAddress and
Pages was not allocated with AllocateBuffer().
**/
@@ -421,79 +536,92 @@ EFIAPI
IoMmuFreeBuffer (
IN EDKII_IOMMU_PROTOCOL *This,
IN UINTN Pages,
IN VOID *HostAddress
)
{
- EFI_STATUS Status;
+ UINTN CommonBufferPages;
+ COMMON_BUFFER_HEADER *CommonBufferHeader;
+
+ CommonBufferPages = Pages + 1;
+ CommonBufferHeader = (COMMON_BUFFER_HEADER *)(
+ (UINTN)HostAddress - EFI_PAGE_SIZE
+ );
+
+ //
+ // Check the signature.
+ //
+ ASSERT (CommonBufferHeader->Signature == COMMON_BUFFER_SIG);
+ if (CommonBufferHeader->Signature != COMMON_BUFFER_SIG) {
+ return EFI_INVALID_PARAMETER;
+ }
//
- // Set memory encryption mask
+ // Free the stash buffer. This buffer was always encrypted, so no need to
+ // zero it.
//
- Status = MemEncryptSevSetPageEncMask (
- 0,
- (EFI_PHYSICAL_ADDRESS)(UINTN)HostAddress,
- Pages,
- TRUE
- );
- ASSERT_EFI_ERROR(Status);
- ZeroMem (HostAddress, EFI_PAGES_TO_SIZE (Pages));
+ FreePages (CommonBufferHeader->StashBuffer, Pages);
DEBUG ((
DEBUG_VERBOSE,
"%a Address 0x%Lx Pages 0x%Lx\n",
__FUNCTION__,
(UINT64)(UINTN)HostAddress,
(UINT64)Pages
));
- return gBS->FreePages ((EFI_PHYSICAL_ADDRESS) (UINTN) HostAddress, Pages);
+
+ //
+ // Release the common buffer itself. Unmap() has re-encrypted it in-place, so
+ // no need to zero it.
+ //
+ return gBS->FreePages ((UINTN)CommonBufferHeader, CommonBufferPages);
}
/**
Set IOMMU attribute for a system memory.
If the IOMMU protocol exists, the system memory cannot be used
for DMA by default.
When a device requests a DMA access for a system memory,
the device driver need use SetAttribute() to update the IOMMU
attribute to request DMA access (read and/or write).
The DeviceHandle is used to identify which device submits the request.
The IOMMU implementation need translate the device path to an IOMMU device
ID, and set IOMMU hardware register accordingly.
1) DeviceHandle can be a standard PCI device.
The memory for BusMasterRead need set EDKII_IOMMU_ACCESS_READ.
The memory for BusMasterWrite need set EDKII_IOMMU_ACCESS_WRITE.
The memory for BusMasterCommonBuffer need set
EDKII_IOMMU_ACCESS_READ|EDKII_IOMMU_ACCESS_WRITE.
After the memory is used, the memory need set 0 to keep it being
protected.
2) DeviceHandle can be an ACPI device (ISA, I2C, SPI, etc).
The memory for DMA access need set EDKII_IOMMU_ACCESS_READ and/or
EDKII_IOMMU_ACCESS_WRITE.
@param[in] This The protocol instance pointer.
@param[in] DeviceHandle The device who initiates the DMA access
request.
@param[in] Mapping The mapping value returned from Map().
@param[in] IoMmuAccess The IOMMU access.
@retval EFI_SUCCESS The IoMmuAccess is set for the memory range
specified by DeviceAddress and Length.
@retval EFI_INVALID_PARAMETER DeviceHandle is an invalid handle.
@retval EFI_INVALID_PARAMETER Mapping is not a value that was returned by
Map().
@retval EFI_INVALID_PARAMETER IoMmuAccess specified an illegal combination
of access.
@retval EFI_UNSUPPORTED DeviceHandle is unknown by the IOMMU.
@retval EFI_UNSUPPORTED The bit mask of IoMmuAccess is not supported
by the IOMMU.
@retval EFI_UNSUPPORTED The IOMMU does not support the memory range
specified by Mapping.
@retval EFI_OUT_OF_RESOURCES There are not enough resources available to
modify the IOMMU access.
@retval EFI_DEVICE_ERROR The IOMMU device reported an error while
attempting the operation.
**/
--
2.13.1.3.g8be5a757fa67
^ permalink raw reply related [flat|nested] 22+ messages in thread
* [PATCH 11/12] OvmfPkg/IoMmuDxe: abort harder on memory encryption mask failures
2017-08-02 21:24 [PATCH 00/12] OvmfPkg/IoMmuDxe: cleanups and fixes Laszlo Ersek
` (9 preceding siblings ...)
2017-08-02 21:24 ` [PATCH 10/12] OvmfPkg/IoMmuDxe: implement in-place decryption/encryption for Map/Unmap Laszlo Ersek
@ 2017-08-02 21:24 ` Laszlo Ersek
2017-08-02 21:24 ` [PATCH 12/12] OvmfPkg/IoMmuDxe: Unmap(): recycle MAP_INFO after BusMasterCommonBuffer[64] Laszlo Ersek
` (2 subsequent siblings)
13 siblings, 0 replies; 22+ messages in thread
From: Laszlo Ersek @ 2017-08-02 21:24 UTC (permalink / raw)
To: edk2-devel-01; +Cc: Ard Biesheuvel, Brijesh Singh, Jordan Justen, Tom Lendacky
Upon a MemEncryptSevClearPageEncMask() failure in Map(), it wouldn't be
difficult to release the bounce buffer that was implicitly allocated for
BusMasterRead[64] and BusMasterWrite[64] operations. However, undoing any
partial memory encryption mask changes -- partial page splitting and PTE
modifications -- is practically impossible. (For example, restoring the
encryption mask on the entire range has no reason to fare any better than
the MemEncryptSevClearPageEncMask() call itself.)
For this reason, keep ASSERT_EFI_ERROR(), but hang in RELEASE builds too,
if MemEncryptSevClearPageEncMask() or MemEncryptSevSetPageEncMask() fails.
Cc: Ard Biesheuvel <ard.biesheuvel@linaro.org>
Cc: Brijesh Singh <brijesh.singh@amd.com>
Cc: Jordan Justen <jordan.l.justen@intel.com>
Cc: Tom Lendacky <thomas.lendacky@amd.com>
Contributed-under: TianoCore Contribution Agreement 1.0
Signed-off-by: Laszlo Ersek <lersek@redhat.com>
---
OvmfPkg/IoMmuDxe/AmdSevIoMmu.c | 10 ++++++++--
1 file changed, 8 insertions(+), 2 deletions(-)
diff --git a/OvmfPkg/IoMmuDxe/AmdSevIoMmu.c b/OvmfPkg/IoMmuDxe/AmdSevIoMmu.c
index 5049d19e9cb7..ee94cd4efbe2 100644
--- a/OvmfPkg/IoMmuDxe/AmdSevIoMmu.c
+++ b/OvmfPkg/IoMmuDxe/AmdSevIoMmu.c
@@ -89,175 +89,178 @@ EFIAPI
IoMmuMap (
IN EDKII_IOMMU_PROTOCOL *This,
IN EDKII_IOMMU_OPERATION Operation,
IN VOID *HostAddress,
IN OUT UINTN *NumberOfBytes,
OUT EFI_PHYSICAL_ADDRESS *DeviceAddress,
OUT VOID **Mapping
)
{
EFI_STATUS Status;
MAP_INFO *MapInfo;
EFI_ALLOCATE_TYPE AllocateType;
COMMON_BUFFER_HEADER *CommonBufferHeader;
VOID *DecryptionSource;
if (HostAddress == NULL || NumberOfBytes == NULL || DeviceAddress == NULL ||
Mapping == NULL) {
return EFI_INVALID_PARAMETER;
}
//
// Allocate a MAP_INFO structure to remember the mapping when Unmap() is
// called later.
//
MapInfo = AllocatePool (sizeof (MAP_INFO));
if (MapInfo == NULL) {
Status = EFI_OUT_OF_RESOURCES;
goto Failed;
}
//
// Initialize the MAP_INFO structure, except the PlainTextAddress field
//
MapInfo->Operation = Operation;
MapInfo->NumberOfBytes = *NumberOfBytes;
MapInfo->NumberOfPages = EFI_SIZE_TO_PAGES (MapInfo->NumberOfBytes);
MapInfo->CryptedAddress = (UINTN)HostAddress;
//
// In the switch statement below, we point "MapInfo->PlainTextAddress" to the
// plaintext buffer, according to Operation. We also set "DecryptionSource".
//
MapInfo->PlainTextAddress = MAX_ADDRESS;
AllocateType = AllocateAnyPages;
DecryptionSource = (VOID *)(UINTN)MapInfo->CryptedAddress;
switch (Operation) {
//
// For BusMasterRead[64] and BusMasterWrite[64] operations, a bounce buffer
// is necessary regardless of whether the original (crypted) buffer crosses
// the 4GB limit or not -- we have to allocate a separate plaintext buffer.
// The only variable is whether the plaintext buffer should be under 4GB.
//
case EdkiiIoMmuOperationBusMasterRead:
case EdkiiIoMmuOperationBusMasterWrite:
MapInfo->PlainTextAddress = BASE_4GB - 1;
AllocateType = AllocateMaxAddress;
//
// fall through
//
case EdkiiIoMmuOperationBusMasterRead64:
case EdkiiIoMmuOperationBusMasterWrite64:
//
// Allocate the implicit plaintext bounce buffer.
//
Status = gBS->AllocatePages (
AllocateType,
EfiBootServicesData,
MapInfo->NumberOfPages,
&MapInfo->PlainTextAddress
);
if (EFI_ERROR (Status)) {
goto FreeMapInfo;
}
break;
//
// For BusMasterCommonBuffer[64] operations, a to-be-plaintext buffer and a
// stash buffer (for in-place decryption) have been allocated already, with
// AllocateBuffer(). We only check whether the address of the to-be-plaintext
// buffer is low enough for the requested operation.
//
case EdkiiIoMmuOperationBusMasterCommonBuffer:
if ((MapInfo->CryptedAddress > BASE_4GB) ||
(EFI_PAGES_TO_SIZE (MapInfo->NumberOfPages) >
BASE_4GB - MapInfo->CryptedAddress)) {
//
// CommonBuffer operations cannot be remapped. If the common buffer is
// above 4GB, then it is not possible to generate a mapping, so return an
// error.
//
Status = EFI_UNSUPPORTED;
goto FreeMapInfo;
}
//
// fall through
//
case EdkiiIoMmuOperationBusMasterCommonBuffer64:
//
// The buffer at MapInfo->CryptedAddress comes from AllocateBuffer().
//
MapInfo->PlainTextAddress = MapInfo->CryptedAddress;
//
// Stash the crypted data.
//
CommonBufferHeader = (COMMON_BUFFER_HEADER *)(
(UINTN)MapInfo->CryptedAddress - EFI_PAGE_SIZE
);
ASSERT (CommonBufferHeader->Signature == COMMON_BUFFER_SIG);
CopyMem (
CommonBufferHeader->StashBuffer,
(VOID *)(UINTN)MapInfo->CryptedAddress,
MapInfo->NumberOfBytes
);
//
// Point "DecryptionSource" to the stash buffer so that we decrypt
// it to the original location, after the switch statement.
//
DecryptionSource = CommonBufferHeader->StashBuffer;
break;
default:
//
// Operation is invalid
//
Status = EFI_INVALID_PARAMETER;
goto FreeMapInfo;
}
//
// Clear the memory encryption mask on the plaintext buffer.
//
Status = MemEncryptSevClearPageEncMask (
0,
MapInfo->PlainTextAddress,
MapInfo->NumberOfPages,
TRUE
);
- ASSERT_EFI_ERROR(Status);
+ ASSERT_EFI_ERROR (Status);
+ if (EFI_ERROR (Status)) {
+ CpuDeadLoop ();
+ }
//
// If this is a read operation from the Bus Master's point of view,
// then copy the contents of the real buffer into the mapped buffer
// so the Bus Master can read the contents of the real buffer.
//
// For BusMasterCommonBuffer[64] operations, the CopyMem() below will decrypt
// the original data (from the stash buffer) back to the original location.
//
if (Operation == EdkiiIoMmuOperationBusMasterRead ||
Operation == EdkiiIoMmuOperationBusMasterRead64 ||
Operation == EdkiiIoMmuOperationBusMasterCommonBuffer ||
Operation == EdkiiIoMmuOperationBusMasterCommonBuffer64) {
CopyMem (
(VOID *) (UINTN) MapInfo->PlainTextAddress,
DecryptionSource,
MapInfo->NumberOfBytes
);
}
//
// Populate output parameters.
//
*DeviceAddress = MapInfo->PlainTextAddress;
*Mapping = MapInfo;
DEBUG ((
DEBUG_VERBOSE,
"%a PlainText 0x%Lx Crypted 0x%Lx Pages 0x%Lx Bytes 0x%Lx\n",
__FUNCTION__,
MapInfo->PlainTextAddress,
MapInfo->CryptedAddress,
(UINT64)MapInfo->NumberOfPages,
(UINT64)MapInfo->NumberOfBytes
));
return EFI_SUCCESS;
@@ -287,129 +290,132 @@ EFIAPI
IoMmuUnmap (
IN EDKII_IOMMU_PROTOCOL *This,
IN VOID *Mapping
)
{
MAP_INFO *MapInfo;
EFI_STATUS Status;
COMMON_BUFFER_HEADER *CommonBufferHeader;
VOID *EncryptionTarget;
if (Mapping == NULL) {
return EFI_INVALID_PARAMETER;
}
MapInfo = (MAP_INFO *)Mapping;
//
// For BusMasterWrite[64] operations and BusMasterCommonBuffer[64] operations
// we have to encrypt the results, ultimately to the original place (i.e.,
// "MapInfo->CryptedAddress").
//
// For BusMasterCommonBuffer[64] operations however, this encryption has to
// land in-place, so divert the encryption to the stash buffer first.
//
EncryptionTarget = (VOID *)(UINTN)MapInfo->CryptedAddress;
switch (MapInfo->Operation) {
case EdkiiIoMmuOperationBusMasterCommonBuffer:
case EdkiiIoMmuOperationBusMasterCommonBuffer64:
ASSERT (MapInfo->PlainTextAddress == MapInfo->CryptedAddress);
CommonBufferHeader = (COMMON_BUFFER_HEADER *)(
(UINTN)MapInfo->PlainTextAddress - EFI_PAGE_SIZE
);
ASSERT (CommonBufferHeader->Signature == COMMON_BUFFER_SIG);
EncryptionTarget = CommonBufferHeader->StashBuffer;
//
// fall through
//
case EdkiiIoMmuOperationBusMasterWrite:
case EdkiiIoMmuOperationBusMasterWrite64:
CopyMem (
EncryptionTarget,
(VOID *) (UINTN) MapInfo->PlainTextAddress,
MapInfo->NumberOfBytes
);
break;
default:
//
// nothing to encrypt after BusMasterRead[64] operations
//
break;
}
DEBUG ((
DEBUG_VERBOSE,
"%a PlainText 0x%Lx Crypted 0x%Lx Pages 0x%Lx Bytes 0x%Lx\n",
__FUNCTION__,
MapInfo->PlainTextAddress,
MapInfo->CryptedAddress,
(UINT64)MapInfo->NumberOfPages,
(UINT64)MapInfo->NumberOfBytes
));
//
// Restore the memory encryption mask on the area we used to hold the
// plaintext.
//
Status = MemEncryptSevSetPageEncMask (
0,
MapInfo->PlainTextAddress,
MapInfo->NumberOfPages,
TRUE
);
- ASSERT_EFI_ERROR(Status);
+ ASSERT_EFI_ERROR (Status);
+ if (EFI_ERROR (Status)) {
+ CpuDeadLoop ();
+ }
//
// For BusMasterCommonBuffer[64] operations, copy the stashed data to the
// original (now encrypted) location.
//
// For all other operations, fill the late bounce buffer (which existed as
// plaintext at some point) with zeros, and then release it.
//
if (MapInfo->Operation == EdkiiIoMmuOperationBusMasterCommonBuffer ||
MapInfo->Operation == EdkiiIoMmuOperationBusMasterCommonBuffer64) {
CopyMem (
(VOID *)(UINTN)MapInfo->CryptedAddress,
CommonBufferHeader->StashBuffer,
MapInfo->NumberOfBytes
);
} else {
ZeroMem (
(VOID *)(UINTN)MapInfo->PlainTextAddress,
EFI_PAGES_TO_SIZE (MapInfo->NumberOfPages)
);
gBS->FreePages (MapInfo->PlainTextAddress, MapInfo->NumberOfPages);
}
//
// Free the MAP_INFO structure.
//
FreePool (Mapping);
return EFI_SUCCESS;
}
/**
Allocates pages that are suitable for an OperationBusMasterCommonBuffer or
OperationBusMasterCommonBuffer64 mapping.
@param This The protocol instance pointer.
@param Type This parameter is not used and must be ignored.
@param MemoryType The type of memory to allocate,
EfiBootServicesData or EfiRuntimeServicesData.
@param Pages The number of pages to allocate.
@param HostAddress A pointer to store the base system memory
address of the allocated range.
@param Attributes The requested bit mask of attributes for the
allocated range.
@retval EFI_SUCCESS The requested memory pages were allocated.
@retval EFI_UNSUPPORTED Attributes is unsupported. The only legal
attribute bits are MEMORY_WRITE_COMBINE and
MEMORY_CACHED.
@retval EFI_INVALID_PARAMETER One or more parameters are invalid.
@retval EFI_OUT_OF_RESOURCES The memory pages could not be allocated.
**/
--
2.13.1.3.g8be5a757fa67
^ permalink raw reply related [flat|nested] 22+ messages in thread
* [PATCH 12/12] OvmfPkg/IoMmuDxe: Unmap(): recycle MAP_INFO after BusMasterCommonBuffer[64]
2017-08-02 21:24 [PATCH 00/12] OvmfPkg/IoMmuDxe: cleanups and fixes Laszlo Ersek
` (10 preceding siblings ...)
2017-08-02 21:24 ` [PATCH 11/12] OvmfPkg/IoMmuDxe: abort harder on memory encryption mask failures Laszlo Ersek
@ 2017-08-02 21:24 ` Laszlo Ersek
2017-08-02 21:31 ` [PATCH 00/12] OvmfPkg/IoMmuDxe: cleanups and fixes Laszlo Ersek
2017-08-03 14:10 ` Brijesh Singh
13 siblings, 0 replies; 22+ messages in thread
From: Laszlo Ersek @ 2017-08-02 21:24 UTC (permalink / raw)
To: edk2-devel-01; +Cc: Ard Biesheuvel, Brijesh Singh, Jordan Justen, Tom Lendacky
In order for Unmap() to be callable from ExitBootServices() event handler
context (for cleaning up a BusMasterCommonBuffer[64] operation), we have
to completely liberate the affected path in Unmap() from dynamic memory
management.
The last remaining piece is the release of the MAP_INFO structure. Rather
than freeing it with FreePool(), recycle it to an internal list. Elements
of this "free list" can be reused for any kind of Map() operation, and can
be freed later, or recycled again.
Cc: Ard Biesheuvel <ard.biesheuvel@linaro.org>
Cc: Brijesh Singh <brijesh.singh@amd.com>
Cc: Jordan Justen <jordan.l.justen@intel.com>
Cc: Tom Lendacky <thomas.lendacky@amd.com>
Contributed-under: TianoCore Contribution Agreement 1.0
Signed-off-by: Laszlo Ersek <lersek@redhat.com>
---
OvmfPkg/IoMmuDxe/AmdSevIoMmu.c | 48 ++++++++++++++++----
1 file changed, 40 insertions(+), 8 deletions(-)
diff --git a/OvmfPkg/IoMmuDxe/AmdSevIoMmu.c b/OvmfPkg/IoMmuDxe/AmdSevIoMmu.c
index ee94cd4efbe2..b2c825123fbb 100644
--- a/OvmfPkg/IoMmuDxe/AmdSevIoMmu.c
+++ b/OvmfPkg/IoMmuDxe/AmdSevIoMmu.c
@@ -20,22 +20,36 @@
#include "AmdSevIoMmu.h"
+#define MAP_INFO_SIG SIGNATURE_64 ('M', 'A', 'P', '_', 'I', 'N', 'F', 'O')
+
typedef struct {
+ UINT64 Signature;
+ LIST_ENTRY Link;
EDKII_IOMMU_OPERATION Operation;
UINTN NumberOfBytes;
UINTN NumberOfPages;
EFI_PHYSICAL_ADDRESS CryptedAddress;
EFI_PHYSICAL_ADDRESS PlainTextAddress;
} MAP_INFO;
+//
+// List of MAP_INFO structures recycled by Unmap().
+//
+// Recycled MAP_INFO structures are equally good for future recycling and
+// freeing.
+//
+STATIC LIST_ENTRY mRecycledMapInfos = INITIALIZE_LIST_HEAD_VARIABLE (
+ mRecycledMapInfos
+ );
+
#define COMMON_BUFFER_SIG SIGNATURE_64 ('C', 'M', 'N', 'B', 'U', 'F', 'F', 'R')
//
// The following structure enables Map() and Unmap() to perform in-place
// decryption and encryption, respectively, for BusMasterCommonBuffer[64]
// operations, without dynamic memory allocation or release.
//
// Both COMMON_BUFFER_HEADER and COMMON_BUFFER_HEADER.StashBuffer are allocated
// by AllocateBuffer() and released by FreeBuffer().
//
#pragma pack (1)
@@ -89,178 +103,190 @@ EFIAPI
IoMmuMap (
IN EDKII_IOMMU_PROTOCOL *This,
IN EDKII_IOMMU_OPERATION Operation,
IN VOID *HostAddress,
IN OUT UINTN *NumberOfBytes,
OUT EFI_PHYSICAL_ADDRESS *DeviceAddress,
OUT VOID **Mapping
)
{
EFI_STATUS Status;
+ LIST_ENTRY *RecycledMapInfo;
MAP_INFO *MapInfo;
EFI_ALLOCATE_TYPE AllocateType;
COMMON_BUFFER_HEADER *CommonBufferHeader;
VOID *DecryptionSource;
if (HostAddress == NULL || NumberOfBytes == NULL || DeviceAddress == NULL ||
Mapping == NULL) {
return EFI_INVALID_PARAMETER;
}
//
// Allocate a MAP_INFO structure to remember the mapping when Unmap() is
// called later.
//
- MapInfo = AllocatePool (sizeof (MAP_INFO));
- if (MapInfo == NULL) {
- Status = EFI_OUT_OF_RESOURCES;
- goto Failed;
+ RecycledMapInfo = GetFirstNode (&mRecycledMapInfos);
+ if (RecycledMapInfo == &mRecycledMapInfos) {
+ //
+ // No recycled MAP_INFO structure, allocate a new one.
+ //
+ MapInfo = AllocatePool (sizeof (MAP_INFO));
+ if (MapInfo == NULL) {
+ Status = EFI_OUT_OF_RESOURCES;
+ goto Failed;
+ }
+ } else {
+ MapInfo = CR (RecycledMapInfo, MAP_INFO, Link, MAP_INFO_SIG);
+ RemoveEntryList (RecycledMapInfo);
}
//
// Initialize the MAP_INFO structure, except the PlainTextAddress field
//
+ ZeroMem (&MapInfo->Link, sizeof MapInfo->Link);
+ MapInfo->Signature = MAP_INFO_SIG;
MapInfo->Operation = Operation;
MapInfo->NumberOfBytes = *NumberOfBytes;
MapInfo->NumberOfPages = EFI_SIZE_TO_PAGES (MapInfo->NumberOfBytes);
MapInfo->CryptedAddress = (UINTN)HostAddress;
//
// In the switch statement below, we point "MapInfo->PlainTextAddress" to the
// plaintext buffer, according to Operation. We also set "DecryptionSource".
//
MapInfo->PlainTextAddress = MAX_ADDRESS;
AllocateType = AllocateAnyPages;
DecryptionSource = (VOID *)(UINTN)MapInfo->CryptedAddress;
switch (Operation) {
//
// For BusMasterRead[64] and BusMasterWrite[64] operations, a bounce buffer
// is necessary regardless of whether the original (crypted) buffer crosses
// the 4GB limit or not -- we have to allocate a separate plaintext buffer.
// The only variable is whether the plaintext buffer should be under 4GB.
//
case EdkiiIoMmuOperationBusMasterRead:
case EdkiiIoMmuOperationBusMasterWrite:
MapInfo->PlainTextAddress = BASE_4GB - 1;
AllocateType = AllocateMaxAddress;
//
// fall through
//
case EdkiiIoMmuOperationBusMasterRead64:
case EdkiiIoMmuOperationBusMasterWrite64:
//
// Allocate the implicit plaintext bounce buffer.
//
Status = gBS->AllocatePages (
AllocateType,
EfiBootServicesData,
MapInfo->NumberOfPages,
&MapInfo->PlainTextAddress
);
if (EFI_ERROR (Status)) {
goto FreeMapInfo;
}
break;
//
// For BusMasterCommonBuffer[64] operations, a to-be-plaintext buffer and a
// stash buffer (for in-place decryption) have been allocated already, with
// AllocateBuffer(). We only check whether the address of the to-be-plaintext
// buffer is low enough for the requested operation.
//
case EdkiiIoMmuOperationBusMasterCommonBuffer:
if ((MapInfo->CryptedAddress > BASE_4GB) ||
(EFI_PAGES_TO_SIZE (MapInfo->NumberOfPages) >
BASE_4GB - MapInfo->CryptedAddress)) {
//
// CommonBuffer operations cannot be remapped. If the common buffer is
// above 4GB, then it is not possible to generate a mapping, so return an
// error.
//
Status = EFI_UNSUPPORTED;
goto FreeMapInfo;
}
//
// fall through
//
case EdkiiIoMmuOperationBusMasterCommonBuffer64:
//
// The buffer at MapInfo->CryptedAddress comes from AllocateBuffer().
//
MapInfo->PlainTextAddress = MapInfo->CryptedAddress;
//
// Stash the crypted data.
//
CommonBufferHeader = (COMMON_BUFFER_HEADER *)(
(UINTN)MapInfo->CryptedAddress - EFI_PAGE_SIZE
);
ASSERT (CommonBufferHeader->Signature == COMMON_BUFFER_SIG);
CopyMem (
CommonBufferHeader->StashBuffer,
(VOID *)(UINTN)MapInfo->CryptedAddress,
MapInfo->NumberOfBytes
);
//
// Point "DecryptionSource" to the stash buffer so that we decrypt
// it to the original location, after the switch statement.
//
DecryptionSource = CommonBufferHeader->StashBuffer;
break;
default:
//
// Operation is invalid
//
Status = EFI_INVALID_PARAMETER;
goto FreeMapInfo;
}
//
// Clear the memory encryption mask on the plaintext buffer.
//
Status = MemEncryptSevClearPageEncMask (
0,
MapInfo->PlainTextAddress,
MapInfo->NumberOfPages,
TRUE
);
ASSERT_EFI_ERROR (Status);
if (EFI_ERROR (Status)) {
CpuDeadLoop ();
}
//
// If this is a read operation from the Bus Master's point of view,
// then copy the contents of the real buffer into the mapped buffer
// so the Bus Master can read the contents of the real buffer.
//
// For BusMasterCommonBuffer[64] operations, the CopyMem() below will decrypt
// the original data (from the stash buffer) back to the original location.
//
if (Operation == EdkiiIoMmuOperationBusMasterRead ||
Operation == EdkiiIoMmuOperationBusMasterRead64 ||
Operation == EdkiiIoMmuOperationBusMasterCommonBuffer ||
Operation == EdkiiIoMmuOperationBusMasterCommonBuffer64) {
CopyMem (
(VOID *) (UINTN) MapInfo->PlainTextAddress,
DecryptionSource,
MapInfo->NumberOfBytes
);
}
//
// Populate output parameters.
//
*DeviceAddress = MapInfo->PlainTextAddress;
*Mapping = MapInfo;
DEBUG ((
DEBUG_VERBOSE,
"%a PlainText 0x%Lx Crypted 0x%Lx Pages 0x%Lx Bytes 0x%Lx\n",
__FUNCTION__,
MapInfo->PlainTextAddress,
MapInfo->CryptedAddress,
(UINT64)MapInfo->NumberOfPages,
(UINT64)MapInfo->NumberOfBytes
));
return EFI_SUCCESS;
@@ -290,132 +316,138 @@ EFIAPI
IoMmuUnmap (
IN EDKII_IOMMU_PROTOCOL *This,
IN VOID *Mapping
)
{
MAP_INFO *MapInfo;
EFI_STATUS Status;
COMMON_BUFFER_HEADER *CommonBufferHeader;
VOID *EncryptionTarget;
if (Mapping == NULL) {
return EFI_INVALID_PARAMETER;
}
MapInfo = (MAP_INFO *)Mapping;
//
// For BusMasterWrite[64] operations and BusMasterCommonBuffer[64] operations
// we have to encrypt the results, ultimately to the original place (i.e.,
// "MapInfo->CryptedAddress").
//
// For BusMasterCommonBuffer[64] operations however, this encryption has to
// land in-place, so divert the encryption to the stash buffer first.
//
EncryptionTarget = (VOID *)(UINTN)MapInfo->CryptedAddress;
switch (MapInfo->Operation) {
case EdkiiIoMmuOperationBusMasterCommonBuffer:
case EdkiiIoMmuOperationBusMasterCommonBuffer64:
ASSERT (MapInfo->PlainTextAddress == MapInfo->CryptedAddress);
CommonBufferHeader = (COMMON_BUFFER_HEADER *)(
(UINTN)MapInfo->PlainTextAddress - EFI_PAGE_SIZE
);
ASSERT (CommonBufferHeader->Signature == COMMON_BUFFER_SIG);
EncryptionTarget = CommonBufferHeader->StashBuffer;
//
// fall through
//
case EdkiiIoMmuOperationBusMasterWrite:
case EdkiiIoMmuOperationBusMasterWrite64:
CopyMem (
EncryptionTarget,
(VOID *) (UINTN) MapInfo->PlainTextAddress,
MapInfo->NumberOfBytes
);
break;
default:
//
// nothing to encrypt after BusMasterRead[64] operations
//
break;
}
DEBUG ((
DEBUG_VERBOSE,
"%a PlainText 0x%Lx Crypted 0x%Lx Pages 0x%Lx Bytes 0x%Lx\n",
__FUNCTION__,
MapInfo->PlainTextAddress,
MapInfo->CryptedAddress,
(UINT64)MapInfo->NumberOfPages,
(UINT64)MapInfo->NumberOfBytes
));
//
// Restore the memory encryption mask on the area we used to hold the
// plaintext.
//
Status = MemEncryptSevSetPageEncMask (
0,
MapInfo->PlainTextAddress,
MapInfo->NumberOfPages,
TRUE
);
ASSERT_EFI_ERROR (Status);
if (EFI_ERROR (Status)) {
CpuDeadLoop ();
}
//
// For BusMasterCommonBuffer[64] operations, copy the stashed data to the
// original (now encrypted) location.
//
// For all other operations, fill the late bounce buffer (which existed as
// plaintext at some point) with zeros, and then release it.
//
if (MapInfo->Operation == EdkiiIoMmuOperationBusMasterCommonBuffer ||
MapInfo->Operation == EdkiiIoMmuOperationBusMasterCommonBuffer64) {
CopyMem (
(VOID *)(UINTN)MapInfo->CryptedAddress,
CommonBufferHeader->StashBuffer,
MapInfo->NumberOfBytes
);
+
+ //
+ // Recycle the MAP_INFO structure.
+ //
+ InsertTailList (&mRecycledMapInfos, &MapInfo->Link);
} else {
ZeroMem (
(VOID *)(UINTN)MapInfo->PlainTextAddress,
EFI_PAGES_TO_SIZE (MapInfo->NumberOfPages)
);
gBS->FreePages (MapInfo->PlainTextAddress, MapInfo->NumberOfPages);
+
+ //
+ // Free the MAP_INFO structure.
+ //
+ FreePool (MapInfo);
}
- //
- // Free the MAP_INFO structure.
- //
- FreePool (Mapping);
return EFI_SUCCESS;
}
/**
Allocates pages that are suitable for an OperationBusMasterCommonBuffer or
OperationBusMasterCommonBuffer64 mapping.
@param This The protocol instance pointer.
@param Type This parameter is not used and must be ignored.
@param MemoryType The type of memory to allocate,
EfiBootServicesData or EfiRuntimeServicesData.
@param Pages The number of pages to allocate.
@param HostAddress A pointer to store the base system memory
address of the allocated range.
@param Attributes The requested bit mask of attributes for the
allocated range.
@retval EFI_SUCCESS The requested memory pages were allocated.
@retval EFI_UNSUPPORTED Attributes is unsupported. The only legal
attribute bits are MEMORY_WRITE_COMBINE and
MEMORY_CACHED.
@retval EFI_INVALID_PARAMETER One or more parameters are invalid.
@retval EFI_OUT_OF_RESOURCES The memory pages could not be allocated.
**/
--
2.13.1.3.g8be5a757fa67
^ permalink raw reply related [flat|nested] 22+ messages in thread
* Re: [PATCH 00/12] OvmfPkg/IoMmuDxe: cleanups and fixes
2017-08-02 21:24 [PATCH 00/12] OvmfPkg/IoMmuDxe: cleanups and fixes Laszlo Ersek
` (11 preceding siblings ...)
2017-08-02 21:24 ` [PATCH 12/12] OvmfPkg/IoMmuDxe: Unmap(): recycle MAP_INFO after BusMasterCommonBuffer[64] Laszlo Ersek
@ 2017-08-02 21:31 ` Laszlo Ersek
2017-08-03 14:10 ` Brijesh Singh
13 siblings, 0 replies; 22+ messages in thread
From: Laszlo Ersek @ 2017-08-02 21:31 UTC (permalink / raw)
To: edk2-devel-01; +Cc: Jordan Justen, Tom Lendacky, Ard Biesheuvel, Brijesh Singh
On 08/02/17 23:24, Laszlo Ersek wrote:
> This series is proposed as a replacement (or a replacement "basis") for
> patches #1 through #3 of Brijesh's series
>
> [PATCH v1 0/4] OvmfPkg : IoMmuDxe: BusMasterCommonBuffer support when
> SEV is active
> http://mid.mail-archive.com/1501529474-20550-1-git-send-email-brijesh.singh@amd.com
I forgot to mention in the blurb that I have no access to SEV hardware
(yet), so this is completely untested ;) It's only build-tested. It
could cause demons to fly out of your nose! :)
Thanks
Laszlo
^ permalink raw reply [flat|nested] 22+ messages in thread
* Re: [PATCH 10/12] OvmfPkg/IoMmuDxe: implement in-place decryption/encryption for Map/Unmap
2017-08-02 21:24 ` [PATCH 10/12] OvmfPkg/IoMmuDxe: implement in-place decryption/encryption for Map/Unmap Laszlo Ersek
@ 2017-08-02 23:01 ` Brijesh Singh
2017-08-03 0:13 ` Laszlo Ersek
0 siblings, 1 reply; 22+ messages in thread
From: Brijesh Singh @ 2017-08-02 23:01 UTC (permalink / raw)
To: Laszlo Ersek, edk2-devel-01
Cc: brijesh.singh, Ard Biesheuvel, Jordan Justen, Tom Lendacky
On 8/2/17 4:24 PM, Laszlo Ersek wrote:
[Snip]
> At the moment, we have the foll+ // The buffer at MapInfo->CryptedAddress comes from AllocateBuffer().
> //
> MapInfo->PlainTextAddress = MapInfo->CryptedAddress;
> -
> //
> - // Therefore no mapping is necessary.
> + // Stash the crypted data.
> //
> - *DeviceAddress = MapInfo->PlainTextAddress;
> - *Mapping = NO_MAPPING;
> - FreePool (MapInfo);
> - return EFI_SUCCESS;
> + CommonBufferHeader = (COMMON_BUFFER_HEADER *)(
> + (UINTN)MapInfo->CryptedAddress - EFI_PAGE_SIZE
> + );
One question, per spec, is it legal for client to call Map() at some
offset within allocated buffer ?
e.g something like this:
* AllocateBuffer (, 1, &Buffer);
* MapBuffer = Buffer + 10;
* Map (, BusMasterCommonBuffer, MappedBuffer, 10, ..) // Bascially Map
10 bytes from offset 10
If this is legal then we may need to build MapInfo during
AllocateBuffer() to locate the "StashBuffer". So far, I have not came
across this usecase but I wanted to check with you so that we don't fail
on corner cases. Good part if you have ASSERT() so we should be able to
catch them (if any).
> + ASSERT (CommonBufferHeader->Signature == COMMON_BUFFER_SIG);
> + CopyMem (
> + CommonBufferHeader->StashBuffer,
> + (VOID *)(UINTN)MapInfo->CryptedAddress,
> + MapInfo->NumberOfBytes
> + );
> + //
> + // Point "DecryptionSource" to the stash buffer so that we decrypt
> + // it to the original location, after the switch statement.
> + //
> + DecryptionSource = CommonBufferHeader->StashBuffer;
> + break;
>
[Snip]
^ permalink raw reply [flat|nested] 22+ messages in thread
* Re: [PATCH 10/12] OvmfPkg/IoMmuDxe: implement in-place decryption/encryption for Map/Unmap
2017-08-02 23:01 ` Brijesh Singh
@ 2017-08-03 0:13 ` Laszlo Ersek
2017-08-03 1:09 ` Brijesh Singh
0 siblings, 1 reply; 22+ messages in thread
From: Laszlo Ersek @ 2017-08-03 0:13 UTC (permalink / raw)
To: Brijesh Singh
Cc: edk2-devel-01, Ard Biesheuvel, Jordan Justen, Tom Lendacky,
Andrew Fish
(CC Andrew)
On 08/03/17 01:01, Brijesh Singh wrote:
>
>
> On 8/2/17 4:24 PM, Laszlo Ersek wrote:
>
> [Snip]
>> At the moment, we have the foll+ // The buffer at MapInfo->CryptedAddress comes from AllocateBuffer().
>> //
>> MapInfo->PlainTextAddress = MapInfo->CryptedAddress;
>> -
>> //
>> - // Therefore no mapping is necessary.
>> + // Stash the crypted data.
>> //
>> - *DeviceAddress = MapInfo->PlainTextAddress;
>> - *Mapping = NO_MAPPING;
>> - FreePool (MapInfo);
>> - return EFI_SUCCESS;
>> + CommonBufferHeader = (COMMON_BUFFER_HEADER *)(
>> + (UINTN)MapInfo->CryptedAddress - EFI_PAGE_SIZE
>> + );
>
> One question, per spec, is it legal for client to call Map() at some
> offset within allocated buffer ?
>
> e.g something like this:
>
> * AllocateBuffer (, 1, &Buffer);
> * MapBuffer = Buffer + 10;
> * Map (, BusMasterCommonBuffer, MappedBuffer, 10, ..) // Bascially Map
> 10 bytes from offset 10
The input/output parameter names seem to counter-indicate such use.
Namely, AllocateBuffer() outputs a "HostAddress" param, and Map() takes
a "HostAddress" param. Plus we have sentences like this:
Under PciIo.Map():
> ... only memory allocated via the AllocateBuffer() interface can be
> mapped for this type of operation ...
Under PciIo.AllocateBuffer():
> The AllocateBuffer() function allocates pages that are suitable for an
> EfiPciOperationBusMasterCommonBuffer or
> EfiPciOperationBusMasterCommonBuffer64 mapping. This means that the
> buffer allocated by this function must support simultaneous access by
> both the processor and a PCI Bus Master. The device address that the
> PCI Bus Master uses to access *the* buffer can be retrieved with a
> call to Map().
This second passage says *the* buffer. (Emphasis mine above.)
> If this is legal then we may need to build MapInfo during
> AllocateBuffer() to locate the "StashBuffer".
Right, in that case we'd have to build a list of allocated ranges (an
interval tree of sorts) in AllocateBuffer, and convert any
CommonBuffer[64] Map() call to its containing allocation with a search.
It would be worse than that, actually... The pattern you have raised
could be taken one step further: do one AllocateBuffer(), and several
CommonBuffer[64] Map()s into it :) What should happen if those maps are
distinct? What should happen if they overlap? :) I can't even imagine
what this would mean for SEV.
... There are guide-like sections in the generic description of
EFI_PCI_IO_PROTOCOL; Andrew quoted them earlier:
http://mid.mail-archive.com/A29CDE8F-C82A-4C92-ABF8-008A9BF8F230@apple.com
> DMA Bus Master Common Buffer Operation
> ======================================
> * Call AllocateBuffer() to allocate a common buffer.
> * Call Map() for EfiPciIoOperationBusMasterCommonBuffer.
> * Program the DMA Bus Master with the DeviceAddress returned by Map().
> * The common buffer can now be accessed equally by the processor and
> the DMA bus master.
> * Call Unmap().
> * Call FreeBuffer().
Look at page 854 (printed page number: 784) in UEFI 2.7.
Thus, I don't think the usage you raise is permitted.
Thanks!
Laszlo
> So far, I have not came across this usecase but I wanted to check with
> you so that we don't fail on corner cases. Good part if you have
> ASSERT() so we should be able to catch them (if any).
>
>> + ASSERT (CommonBufferHeader->Signature == COMMON_BUFFER_SIG);
>> + CopyMem (
>> + CommonBufferHeader->StashBuffer,
>> + (VOID *)(UINTN)MapInfo->CryptedAddress,
>> + MapInfo->NumberOfBytes
>> + );
>> + //
>> + // Point "DecryptionSource" to the stash buffer so that we decrypt
>> + // it to the original location, after the switch statement.
>> + //
>> + DecryptionSource = CommonBufferHeader->StashBuffer;
>> + break;
>>
> [Snip]
>
^ permalink raw reply [flat|nested] 22+ messages in thread
* Re: [PATCH 10/12] OvmfPkg/IoMmuDxe: implement in-place decryption/encryption for Map/Unmap
2017-08-03 0:13 ` Laszlo Ersek
@ 2017-08-03 1:09 ` Brijesh Singh
2017-08-03 14:35 ` Brijesh Singh
0 siblings, 1 reply; 22+ messages in thread
From: Brijesh Singh @ 2017-08-03 1:09 UTC (permalink / raw)
To: Laszlo Ersek
Cc: brijesh.singh, edk2-devel-01, Ard Biesheuvel, Jordan Justen,
Tom Lendacky, Andrew Fish
On 8/2/17 7:13 PM, Laszlo Ersek wrote:
> (CC Andrew)
>
> On 08/03/17 01:01, Brijesh Singh wrote:
>>
>> On 8/2/17 4:24 PM, Laszlo Ersek wrote:
>>
>> [Snip]
>>> At the moment, we have the foll+ // The buffer at MapInfo->CryptedAddress comes from AllocateBuffer().
>>> //
>>> MapInfo->PlainTextAddress = MapInfo->CryptedAddress;
>>> -
>>> //
>>> - // Therefore no mapping is necessary.
>>> + // Stash the crypted data.
>>> //
>>> - *DeviceAddress = MapInfo->PlainTextAddress;
>>> - *Mapping = NO_MAPPING;
>>> - FreePool (MapInfo);
>>> - return EFI_SUCCESS;
>>> + CommonBufferHeader = (COMMON_BUFFER_HEADER *)(
>>> + (UINTN)MapInfo->CryptedAddress - EFI_PAGE_SIZE
>>> + );
>> One question, per spec, is it legal for client to call Map() at some
>> offset within allocated buffer ?
>>
>> e.g something like this:
>>
>> * AllocateBuffer (, 1, &Buffer);
>> * MapBuffer = Buffer + 10;
>> * Map (, BusMasterCommonBuffer, MappedBuffer, 10, ..) // Bascially Map
>> 10 bytes from offset 10
> The input/output parameter names seem to counter-indicate such use.
> Namely, AllocateBuffer() outputs a "HostAddress" param, and Map() takes
> a "HostAddress" param. Plus we have sentences like this:
>
> Under PciIo.Map():
>
>> ... only memory allocated via the AllocateBuffer() interface can be
>> mapped for this type of operation ...
> Under PciIo.AllocateBuffer():
>
>> The AllocateBuffer() function allocates pages that are suitable for an
>> EfiPciOperationBusMasterCommonBuffer or
>> EfiPciOperationBusMasterCommonBuffer64 mapping. This means that the
>> buffer allocated by this function must support simultaneous access by
>> both the processor and a PCI Bus Master. The device address that the
>> PCI Bus Master uses to access *the* buffer can be retrieved with a
>> call to Map().
> This second passage says *the* buffer. (Emphasis mine above.)
>
>> If this is legal then we may need to build MapInfo during
>> AllocateBuffer() to locate the "StashBuffer".
> Right, in that case we'd have to build a list of allocated ranges (an
> interval tree of sorts) in AllocateBuffer, and convert any
> CommonBuffer[64] Map() call to its containing allocation with a search.
>
> It would be worse than that, actually... The pattern you have raised
> could be taken one step further: do one AllocateBuffer(), and several
> CommonBuffer[64] Map()s into it :) What should happen if those maps are
> distinct? What should happen if they overlap? :) I can't even imagine
> what this would mean for SEV.
>
> ... There are guide-like sections in the generic description of
> EFI_PCI_IO_PROTOCOL; Andrew quoted them earlier:
>
> http://mid.mail-archive.com/A29CDE8F-C82A-4C92-ABF8-008A9BF8F230@apple.com
>
>> DMA Bus Master Common Buffer Operation
>> ======================================
>> * Call AllocateBuffer() to allocate a common buffer.
>> * Call Map() for EfiPciIoOperationBusMasterCommonBuffer.
>> * Program the DMA Bus Master with the DeviceAddress returned by Map().
>> * The common buffer can now be accessed equally by the processor and
>> the DMA bus master.
>> * Call Unmap().
>> * Call FreeBuffer().
> Look at page 854 (printed page number: 784) in UEFI 2.7.
>
> Thus, I don't think the usage you raise is permitted.
Sounds good. I did a quick test on SEV hardware, everything seems to be
working well. I have started my stresstest and report the result tomorrow.
-Brijesh
^ permalink raw reply [flat|nested] 22+ messages in thread
* Re: [PATCH 00/12] OvmfPkg/IoMmuDxe: cleanups and fixes
2017-08-02 21:24 [PATCH 00/12] OvmfPkg/IoMmuDxe: cleanups and fixes Laszlo Ersek
` (12 preceding siblings ...)
2017-08-02 21:31 ` [PATCH 00/12] OvmfPkg/IoMmuDxe: cleanups and fixes Laszlo Ersek
@ 2017-08-03 14:10 ` Brijesh Singh
2017-08-03 14:15 ` Laszlo Ersek
2017-08-05 1:25 ` Laszlo Ersek
13 siblings, 2 replies; 22+ messages in thread
From: Brijesh Singh @ 2017-08-03 14:10 UTC (permalink / raw)
To: Laszlo Ersek, edk2-devel-01
Cc: brijesh.singh, Ard Biesheuvel, Jordan Justen, Tom Lendacky
Hi Laszlo,
On 08/02/2017 04:24 PM, Laszlo Ersek wrote:
> This series is proposed as a replacement (or a replacement "basis") for
> patches #1 through #3 of Brijesh's series
>
> [PATCH v1 0/4] OvmfPkg : IoMmuDxe: BusMasterCommonBuffer support when
> SEV is active
> http://mid.mail-archive.com/1501529474-20550-1-git-send-email-brijesh.singh@amd.com
>
> Patch #4 of the same series ("OvmfPkg : QemuFwCfgLib: Map DMA buffer
> with CommonBuffer when SEV is enable") is required on top of this
> series; otherwise QemuFwCfgLib will break on SEV.
>
>
> In the present series, patches #1 through #7 are lightweight
> improvements for OvmfPkg/IoMmuDxe, concerning line width, MAP_INFO field
> names, conversion specifiers for DEBUG(), coding style, error
> propagation, and library class listings.
>
> Patch #8 ("zero out pages before releasing them") fixes the "information
> leak" issue pointed out in:
>
> http://mid.mail-archive.com/e1137140-f5d5-7eb8-0162-952694b24f96@redhat.com
>
> Patch #9 ('rework setup of "MapInfo->PlainTextAddress" in Map()') fixes
> as-yet undiscussed issues, and lays the groundwork for patch #10, by
> reworking the calculation of the plaintext buffer address.
>
> Patch #10 ("implement in-place decryption/encryption for Map/Unmap")
> fixes the issues around BusMasterCommonBuffer[64] operations that were
> discussed in the following messages:
>
> http://mid.mail-archive.com/4071596d-32c9-e6d9-8c93-0d43d28e9b5a@redhat.com
> http://mid.mail-archive.com/e1137140-f5d5-7eb8-0162-952694b24f96@redhat.com
> http://mid.mail-archive.com/84c3c5db-623e-181b-c472-7fd7ae1c1670@amd.com
> http://mid.mail-archive.com/89e1553a-1630-87a5-cffd-99174a380d41@redhat.com
>
> Patch #11 ("abort harder on memory encryption mask failures") settles
> the error handling for MemEncryptSevClearPageEncMask() and
> MemEncryptSevSetPageEncMask(), discussed in:
>
> http://mid.mail-archive.com/89e1553a-1630-87a5-cffd-99174a380d41@redhat.com
>
> Patch #12 ("Unmap(): recycle MAP_INFO after BusMasterCommonBuffer[64]")
> implements the "free list" proposed in:
>
> http://mid.mail-archive.com/e1137140-f5d5-7eb8-0162-952694b24f96@redhat.com
>
> The series has been formatted with "--function-context", for easier
> review.
>
> Repo: https://github.com/lersek/edk2.git
> Branch: amdsev_iommu_cleanups_fixes
>
> Cc: Ard Biesheuvel <ard.biesheuvel@linaro.org>
> Cc: Brijesh Singh <brijesh.singh@amd.com>
> Cc: Jordan Justen <jordan.l.justen@intel.com>
> Cc: Tom Lendacky <thomas.lendacky@amd.com>
>
Appreciate your help, the series looks good. I have ran some overnight tests
and so far things are looking positive. As you pointed out in blurb that we
still need Patch #4 from my series. I will soon send updated version.
Tested-By: Brijesh Singh <brijesh.singh@amd.com>
Reviewed-by: Brijesh Singh <brijesh.singh@amd.com>
^ permalink raw reply [flat|nested] 22+ messages in thread
* Re: [PATCH 00/12] OvmfPkg/IoMmuDxe: cleanups and fixes
2017-08-03 14:10 ` Brijesh Singh
@ 2017-08-03 14:15 ` Laszlo Ersek
2017-08-05 1:25 ` Laszlo Ersek
1 sibling, 0 replies; 22+ messages in thread
From: Laszlo Ersek @ 2017-08-03 14:15 UTC (permalink / raw)
To: Brijesh Singh, edk2-devel-01; +Cc: Ard Biesheuvel, Jordan Justen, Tom Lendacky
On 08/03/17 16:10, Brijesh Singh wrote:
> Hi Laszlo,
>
> On 08/02/2017 04:24 PM, Laszlo Ersek wrote:
>> This series is proposed as a replacement (or a replacement "basis") for
>> patches #1 through #3 of Brijesh's series
>>
>> [PATCH v1 0/4] OvmfPkg : IoMmuDxe: BusMasterCommonBuffer support when
>> SEV is active
>>
>> http://mid.mail-archive.com/1501529474-20550-1-git-send-email-brijesh.singh@amd.com
>>
>>
>> Patch #4 of the same series ("OvmfPkg : QemuFwCfgLib: Map DMA buffer
>> with CommonBuffer when SEV is enable") is required on top of this
>> series; otherwise QemuFwCfgLib will break on SEV.
>>
>>
>> In the present series, patches #1 through #7 are lightweight
>> improvements for OvmfPkg/IoMmuDxe, concerning line width, MAP_INFO field
>> names, conversion specifiers for DEBUG(), coding style, error
>> propagation, and library class listings.
>>
>> Patch #8 ("zero out pages before releasing them") fixes the "information
>> leak" issue pointed out in:
>>
>>
>> http://mid.mail-archive.com/e1137140-f5d5-7eb8-0162-952694b24f96@redhat.com
>>
>>
>> Patch #9 ('rework setup of "MapInfo->PlainTextAddress" in Map()') fixes
>> as-yet undiscussed issues, and lays the groundwork for patch #10, by
>> reworking the calculation of the plaintext buffer address.
>>
>> Patch #10 ("implement in-place decryption/encryption for Map/Unmap")
>> fixes the issues around BusMasterCommonBuffer[64] operations that were
>> discussed in the following messages:
>>
>>
>> http://mid.mail-archive.com/4071596d-32c9-e6d9-8c93-0d43d28e9b5a@redhat.com
>>
>>
>> http://mid.mail-archive.com/e1137140-f5d5-7eb8-0162-952694b24f96@redhat.com
>>
>>
>> http://mid.mail-archive.com/84c3c5db-623e-181b-c472-7fd7ae1c1670@amd.com
>>
>> http://mid.mail-archive.com/89e1553a-1630-87a5-cffd-99174a380d41@redhat.com
>>
>>
>> Patch #11 ("abort harder on memory encryption mask failures") settles
>> the error handling for MemEncryptSevClearPageEncMask() and
>> MemEncryptSevSetPageEncMask(), discussed in:
>>
>>
>> http://mid.mail-archive.com/89e1553a-1630-87a5-cffd-99174a380d41@redhat.com
>>
>>
>> Patch #12 ("Unmap(): recycle MAP_INFO after BusMasterCommonBuffer[64]")
>> implements the "free list" proposed in:
>>
>>
>> http://mid.mail-archive.com/e1137140-f5d5-7eb8-0162-952694b24f96@redhat.com
>>
>>
>> The series has been formatted with "--function-context", for easier
>> review.
>>
>> Repo: https://github.com/lersek/edk2.git
>> Branch: amdsev_iommu_cleanups_fixes
>>
>> Cc: Ard Biesheuvel <ard.biesheuvel@linaro.org>
>> Cc: Brijesh Singh <brijesh.singh@amd.com>
>> Cc: Jordan Justen <jordan.l.justen@intel.com>
>> Cc: Tom Lendacky <thomas.lendacky@amd.com>
>>
>
> Appreciate your help, the series looks good. I have ran some overnight
> tests
> and so far things are looking positive. As you pointed out in blurb that we
> still need Patch #4 from my series. I will soon send updated version.
>
> Tested-By: Brijesh Singh <brijesh.singh@amd.com>
> Reviewed-by: Brijesh Singh <brijesh.singh@amd.com>
>
Thank you!
I'll try to review your patch #4 soon after it hits the list. (Please
also consider pushing it to your public repo.)
Cheers
Laszlo
^ permalink raw reply [flat|nested] 22+ messages in thread
* Re: [PATCH 10/12] OvmfPkg/IoMmuDxe: implement in-place decryption/encryption for Map/Unmap
2017-08-03 1:09 ` Brijesh Singh
@ 2017-08-03 14:35 ` Brijesh Singh
2017-08-03 14:40 ` Laszlo Ersek
0 siblings, 1 reply; 22+ messages in thread
From: Brijesh Singh @ 2017-08-03 14:35 UTC (permalink / raw)
To: Laszlo Ersek
Cc: brijesh.singh, edk2-devel-01, Ard Biesheuvel, Jordan Justen,
Tom Lendacky, Andrew Fish
Laszlo,
One minor issue, I got compilation error with GCC48.
/home/brijesh/codomania/edk2-new/edk2/OvmfPkg/IoMmuDxe/AmdSevIoMmu.c: In function ‘IoMmuUnmap’:
/home/brijesh/codomania/edk2-new/edk2/OvmfPkg/IoMmuDxe/AmdSevIoMmu.c:408:25: error: ‘CommonBufferHeader’ may be used uninitialized in this function [-Werror=maybe-uninitialized]
CommonBufferHeader->StashBuffer,
Looks like we need to initialize CommonBufferHeader = NULL to keep GCC48 happy.
thanks
On 08/02/2017 08:09 PM, Brijesh Singh wrote:
>
>
> On 8/2/17 7:13 PM, Laszlo Ersek wrote:
>> (CC Andrew)
>>
>> On 08/03/17 01:01, Brijesh Singh wrote:
>>>
>>> On 8/2/17 4:24 PM, Laszlo Ersek wrote:
>>>
>>> [Snip]
>>>> At the moment, we have the foll+ // The buffer at MapInfo->CryptedAddress comes from AllocateBuffer().
>>>> //
>>>> MapInfo->PlainTextAddress = MapInfo->CryptedAddress;
>>>> -
>>>> //
>>>> - // Therefore no mapping is necessary.
>>>> + // Stash the crypted data.
>>>> //
>>>> - *DeviceAddress = MapInfo->PlainTextAddress;
>>>> - *Mapping = NO_MAPPING;
>>>> - FreePool (MapInfo);
>>>> - return EFI_SUCCESS;
>>>> + CommonBufferHeader = (COMMON_BUFFER_HEADER *)(
>>>> + (UINTN)MapInfo->CryptedAddress - EFI_PAGE_SIZE
>>>> + );
>>> One question, per spec, is it legal for client to call Map() at some
>>> offset within allocated buffer ?
>>>
>>> e.g something like this:
>>>
>>> * AllocateBuffer (, 1, &Buffer);
>>> * MapBuffer = Buffer + 10;
>>> * Map (, BusMasterCommonBuffer, MappedBuffer, 10, ..) // Bascially Map
>>> 10 bytes from offset 10
>> The input/output parameter names seem to counter-indicate such use.
>> Namely, AllocateBuffer() outputs a "HostAddress" param, and Map() takes
>> a "HostAddress" param. Plus we have sentences like this:
>>
>> Under PciIo.Map():
>>
>>> ... only memory allocated via the AllocateBuffer() interface can be
>>> mapped for this type of operation ...
>> Under PciIo.AllocateBuffer():
>>
>>> The AllocateBuffer() function allocates pages that are suitable for an
>>> EfiPciOperationBusMasterCommonBuffer or
>>> EfiPciOperationBusMasterCommonBuffer64 mapping. This means that the
>>> buffer allocated by this function must support simultaneous access by
>>> both the processor and a PCI Bus Master. The device address that the
>>> PCI Bus Master uses to access *the* buffer can be retrieved with a
>>> call to Map().
>> This second passage says *the* buffer. (Emphasis mine above.)
>>
>>> If this is legal then we may need to build MapInfo during
>>> AllocateBuffer() to locate the "StashBuffer".
>> Right, in that case we'd have to build a list of allocated ranges (an
>> interval tree of sorts) in AllocateBuffer, and convert any
>> CommonBuffer[64] Map() call to its containing allocation with a search.
>>
>> It would be worse than that, actually... The pattern you have raised
>> could be taken one step further: do one AllocateBuffer(), and several
>> CommonBuffer[64] Map()s into it :) What should happen if those maps are
>> distinct? What should happen if they overlap? :) I can't even imagine
>> what this would mean for SEV.
>>
>> ... There are guide-like sections in the generic description of
>> EFI_PCI_IO_PROTOCOL; Andrew quoted them earlier:
>>
>> http://mid.mail-archive.com/A29CDE8F-C82A-4C92-ABF8-008A9BF8F230@apple.com
>>
>>> DMA Bus Master Common Buffer Operation
>>> ======================================
>>> * Call AllocateBuffer() to allocate a common buffer.
>>> * Call Map() for EfiPciIoOperationBusMasterCommonBuffer.
>>> * Program the DMA Bus Master with the DeviceAddress returned by Map().
>>> * The common buffer can now be accessed equally by the processor and
>>> the DMA bus master.
>>> * Call Unmap().
>>> * Call FreeBuffer().
>> Look at page 854 (printed page number: 784) in UEFI 2.7.
>>
>> Thus, I don't think the usage you raise is permitted.
>
> Sounds good. I did a quick test on SEV hardware, everything seems to be
> working well. I have started my stresstest and report the result tomorrow.
>
> -Brijesh
>
^ permalink raw reply [flat|nested] 22+ messages in thread
* Re: [PATCH 10/12] OvmfPkg/IoMmuDxe: implement in-place decryption/encryption for Map/Unmap
2017-08-03 14:35 ` Brijesh Singh
@ 2017-08-03 14:40 ` Laszlo Ersek
0 siblings, 0 replies; 22+ messages in thread
From: Laszlo Ersek @ 2017-08-03 14:40 UTC (permalink / raw)
To: Brijesh Singh
Cc: edk2-devel-01, Ard Biesheuvel, Jordan Justen, Tom Lendacky,
Andrew Fish
On 08/03/17 16:35, Brijesh Singh wrote:
> Laszlo,
>
> One minor issue, I got compilation error with GCC48.
>
> /home/brijesh/codomania/edk2-new/edk2/OvmfPkg/IoMmuDxe/AmdSevIoMmu.c: In
> function ‘IoMmuUnmap’:
> /home/brijesh/codomania/edk2-new/edk2/OvmfPkg/IoMmuDxe/AmdSevIoMmu.c:408:25:
> error: ‘CommonBufferHeader’ may be used uninitialized in this function
> [-Werror=maybe-uninitialized]
> CommonBufferHeader->StashBuffer,
>
> Looks like we need to initialize CommonBufferHeader = NULL to keep GCC48
> happy.
Interesting, I use GCC48 all the time (as part of RHEL7 on my laptop),
and I didn't get this warning. I'll suppress it.
Thank you for the report!
Laszlo
>
> thanks
>
> On 08/02/2017 08:09 PM, Brijesh Singh wrote:
>>
>>
>> On 8/2/17 7:13 PM, Laszlo Ersek wrote:
>>> (CC Andrew)
>>>
>>> On 08/03/17 01:01, Brijesh Singh wrote:
>>>>
>>>> On 8/2/17 4:24 PM, Laszlo Ersek wrote:
>>>>
>>>> [Snip]
>>>>> At the moment, we have the foll+ // The buffer at
>>>>> MapInfo->CryptedAddress comes from AllocateBuffer().
>>>>> //
>>>>> MapInfo->PlainTextAddress = MapInfo->CryptedAddress;
>>>>> -
>>>>> //
>>>>> - // Therefore no mapping is necessary.
>>>>> + // Stash the crypted data.
>>>>> //
>>>>> - *DeviceAddress = MapInfo->PlainTextAddress;
>>>>> - *Mapping = NO_MAPPING;
>>>>> - FreePool (MapInfo);
>>>>> - return EFI_SUCCESS;
>>>>> + CommonBufferHeader = (COMMON_BUFFER_HEADER *)(
>>>>> + (UINTN)MapInfo->CryptedAddress -
>>>>> EFI_PAGE_SIZE
>>>>> + );
>>>> One question, per spec, is it legal for client to call Map() at some
>>>> offset within allocated buffer ?
>>>>
>>>> e.g something like this:
>>>>
>>>> * AllocateBuffer (, 1, &Buffer);
>>>> * MapBuffer = Buffer + 10;
>>>> * Map (, BusMasterCommonBuffer, MappedBuffer, 10, ..) // Bascially Map
>>>> 10 bytes from offset 10
>>> The input/output parameter names seem to counter-indicate such use.
>>> Namely, AllocateBuffer() outputs a "HostAddress" param, and Map() takes
>>> a "HostAddress" param. Plus we have sentences like this:
>>>
>>> Under PciIo.Map():
>>>
>>>> ... only memory allocated via the AllocateBuffer() interface can be
>>>> mapped for this type of operation ...
>>> Under PciIo.AllocateBuffer():
>>>
>>>> The AllocateBuffer() function allocates pages that are suitable for an
>>>> EfiPciOperationBusMasterCommonBuffer or
>>>> EfiPciOperationBusMasterCommonBuffer64 mapping. This means that the
>>>> buffer allocated by this function must support simultaneous access by
>>>> both the processor and a PCI Bus Master. The device address that the
>>>> PCI Bus Master uses to access *the* buffer can be retrieved with a
>>>> call to Map().
>>> This second passage says *the* buffer. (Emphasis mine above.)
>>>
>>>> If this is legal then we may need to build MapInfo during
>>>> AllocateBuffer() to locate the "StashBuffer".
>>> Right, in that case we'd have to build a list of allocated ranges (an
>>> interval tree of sorts) in AllocateBuffer, and convert any
>>> CommonBuffer[64] Map() call to its containing allocation with a search.
>>>
>>> It would be worse than that, actually... The pattern you have raised
>>> could be taken one step further: do one AllocateBuffer(), and several
>>> CommonBuffer[64] Map()s into it :) What should happen if those maps are
>>> distinct? What should happen if they overlap? :) I can't even imagine
>>> what this would mean for SEV.
>>>
>>> ... There are guide-like sections in the generic description of
>>> EFI_PCI_IO_PROTOCOL; Andrew quoted them earlier:
>>>
>>>
>>> http://mid.mail-archive.com/A29CDE8F-C82A-4C92-ABF8-008A9BF8F230@apple.com
>>>
>>>
>>>> DMA Bus Master Common Buffer Operation
>>>> ======================================
>>>> * Call AllocateBuffer() to allocate a common buffer.
>>>> * Call Map() for EfiPciIoOperationBusMasterCommonBuffer.
>>>> * Program the DMA Bus Master with the DeviceAddress returned by Map().
>>>> * The common buffer can now be accessed equally by the processor and
>>>> the DMA bus master.
>>>> * Call Unmap().
>>>> * Call FreeBuffer().
>>> Look at page 854 (printed page number: 784) in UEFI 2.7.
>>>
>>> Thus, I don't think the usage you raise is permitted.
>>
>> Sounds good. I did a quick test on SEV hardware, everything seems to be
>> working well. I have started my stresstest and report the result
>> tomorrow.
>>
>> -Brijesh
>>
^ permalink raw reply [flat|nested] 22+ messages in thread
* Re: [PATCH 00/12] OvmfPkg/IoMmuDxe: cleanups and fixes
2017-08-03 14:10 ` Brijesh Singh
2017-08-03 14:15 ` Laszlo Ersek
@ 2017-08-05 1:25 ` Laszlo Ersek
1 sibling, 0 replies; 22+ messages in thread
From: Laszlo Ersek @ 2017-08-05 1:25 UTC (permalink / raw)
To: Brijesh Singh, edk2-devel-01; +Cc: Jordan Justen, Tom Lendacky, Ard Biesheuvel
On 08/03/17 16:10, Brijesh Singh wrote:
> Hi Laszlo,
>
> On 08/02/2017 04:24 PM, Laszlo Ersek wrote:
>> This series is proposed as a replacement (or a replacement "basis") for
>> patches #1 through #3 of Brijesh's series
>>
>> [PATCH v1 0/4] OvmfPkg : IoMmuDxe: BusMasterCommonBuffer support when
>> SEV is active
>>
>> http://mid.mail-archive.com/1501529474-20550-1-git-send-email-brijesh.singh@amd.com
>>
>>
>> Patch #4 of the same series ("OvmfPkg : QemuFwCfgLib: Map DMA buffer
>> with CommonBuffer when SEV is enable") is required on top of this
>> series; otherwise QemuFwCfgLib will break on SEV.
>>
>>
>> In the present series, patches #1 through #7 are lightweight
>> improvements for OvmfPkg/IoMmuDxe, concerning line width, MAP_INFO field
>> names, conversion specifiers for DEBUG(), coding style, error
>> propagation, and library class listings.
>>
>> Patch #8 ("zero out pages before releasing them") fixes the "information
>> leak" issue pointed out in:
>>
>>
>> http://mid.mail-archive.com/e1137140-f5d5-7eb8-0162-952694b24f96@redhat.com
>>
>>
>> Patch #9 ('rework setup of "MapInfo->PlainTextAddress" in Map()') fixes
>> as-yet undiscussed issues, and lays the groundwork for patch #10, by
>> reworking the calculation of the plaintext buffer address.
>>
>> Patch #10 ("implement in-place decryption/encryption for Map/Unmap")
>> fixes the issues around BusMasterCommonBuffer[64] operations that were
>> discussed in the following messages:
>>
>>
>> http://mid.mail-archive.com/4071596d-32c9-e6d9-8c93-0d43d28e9b5a@redhat.com
>>
>>
>> http://mid.mail-archive.com/e1137140-f5d5-7eb8-0162-952694b24f96@redhat.com
>>
>>
>> http://mid.mail-archive.com/84c3c5db-623e-181b-c472-7fd7ae1c1670@amd.com
>>
>> http://mid.mail-archive.com/89e1553a-1630-87a5-cffd-99174a380d41@redhat.com
>>
>>
>> Patch #11 ("abort harder on memory encryption mask failures") settles
>> the error handling for MemEncryptSevClearPageEncMask() and
>> MemEncryptSevSetPageEncMask(), discussed in:
>>
>>
>> http://mid.mail-archive.com/89e1553a-1630-87a5-cffd-99174a380d41@redhat.com
>>
>>
>> Patch #12 ("Unmap(): recycle MAP_INFO after BusMasterCommonBuffer[64]")
>> implements the "free list" proposed in:
>>
>>
>> http://mid.mail-archive.com/e1137140-f5d5-7eb8-0162-952694b24f96@redhat.com
>>
>>
>> The series has been formatted with "--function-context", for easier
>> review.
>>
>> Repo: https://github.com/lersek/edk2.git
>> Branch: amdsev_iommu_cleanups_fixes
>>
>> Cc: Ard Biesheuvel <ard.biesheuvel@linaro.org>
>> Cc: Brijesh Singh <brijesh.singh@amd.com>
>> Cc: Jordan Justen <jordan.l.justen@intel.com>
>> Cc: Tom Lendacky <thomas.lendacky@amd.com>
>>
>
> Appreciate your help, the series looks good. I have ran some overnight
> tests
> and so far things are looking positive. As you pointed out in blurb that we
> still need Patch #4 from my series. I will soon send updated version.
>
> Tested-By: Brijesh Singh <brijesh.singh@amd.com>
> Reviewed-by: Brijesh Singh <brijesh.singh@amd.com>
Pushed as 8dccfa6d4807..d0c9afea42a0.
(With the patch #10 fixup you pointed out in
<http://mid.mail-archive.com/ce614a16-117c-e904-ac5c-ed56bd729e06@amd.com>.)
Thanks,
Laszlo
^ permalink raw reply [flat|nested] 22+ messages in thread
end of thread, other threads:[~2017-08-05 1:23 UTC | newest]
Thread overview: 22+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2017-08-02 21:24 [PATCH 00/12] OvmfPkg/IoMmuDxe: cleanups and fixes Laszlo Ersek
2017-08-02 21:24 ` [PATCH 01/12] OvmfPkg/IoMmuDxe: rewrap source code to 79 characters Laszlo Ersek
2017-08-02 21:24 ` [PATCH 02/12] OvmfPkg/IoMmuDxe: rename DeviceAddress to PlainTextAddress in MAP_INFO Laszlo Ersek
2017-08-02 21:24 ` [PATCH 03/12] OvmfPkg/IoMmuDxe: rename HostAddress to CryptedAddress " Laszlo Ersek
2017-08-02 21:24 ` [PATCH 04/12] OvmfPkg/IoMmuDxe: convert UINTN arguments to UINT64 for the %Lx fmt spec Laszlo Ersek
2017-08-02 21:24 ` [PATCH 05/12] OvmfPkg/IoMmuDxe: don't initialize local variables Laszlo Ersek
2017-08-02 21:24 ` [PATCH 06/12] OvmfPkg/IoMmuDxe: propagate errors from AmdSevInstallIoMmuProtocol() Laszlo Ersek
2017-08-02 21:24 ` [PATCH 07/12] OvmfPkg/IoMmuDxe: clean up used library classes Laszlo Ersek
2017-08-02 21:24 ` [PATCH 08/12] OvmfPkg/IoMmuDxe: zero out pages before releasing them Laszlo Ersek
2017-08-02 21:24 ` [PATCH 09/12] OvmfPkg/IoMmuDxe: rework setup of "MapInfo->PlainTextAddress" in Map() Laszlo Ersek
2017-08-02 21:24 ` [PATCH 10/12] OvmfPkg/IoMmuDxe: implement in-place decryption/encryption for Map/Unmap Laszlo Ersek
2017-08-02 23:01 ` Brijesh Singh
2017-08-03 0:13 ` Laszlo Ersek
2017-08-03 1:09 ` Brijesh Singh
2017-08-03 14:35 ` Brijesh Singh
2017-08-03 14:40 ` Laszlo Ersek
2017-08-02 21:24 ` [PATCH 11/12] OvmfPkg/IoMmuDxe: abort harder on memory encryption mask failures Laszlo Ersek
2017-08-02 21:24 ` [PATCH 12/12] OvmfPkg/IoMmuDxe: Unmap(): recycle MAP_INFO after BusMasterCommonBuffer[64] Laszlo Ersek
2017-08-02 21:31 ` [PATCH 00/12] OvmfPkg/IoMmuDxe: cleanups and fixes Laszlo Ersek
2017-08-03 14:10 ` Brijesh Singh
2017-08-03 14:15 ` Laszlo Ersek
2017-08-05 1:25 ` Laszlo Ersek
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox