[edk2-devel] [PATCH] OvmfPkg/IoMmuDxe: Provide an implementation for SetAttribute

[edk2-devel] [PATCH] OvmfPkg/IoMmuDxe: Provide an implementation for SetAttribute

[Lendacky, Thomas via groups.io]

A recent change to the PciIoMap() function now propagates the return code
from the IoMmu protocol SetAttribute() operation. The implementation of
this operation in OvmfPkg/IoMmuDxe/CcIoMmu.c returns EFI_UNSUPPORTED,
resulting in a failure to boot the guest.

Provide an implementation for SetAttribute() that validates the IoMmu
access method being requested against the IoMmu mapping operation.

Suggested-by: Laszlo Ersek <lersek@redhat.com>
Reviewed-by: Laszlo Ersek <lersek@redhat.com>
Signed-off-by: Tom Lendacky <thomas.lendacky@amd.com>
---
 OvmfPkg/IoMmuDxe/CcIoMmu.c | 55 +++++++++++++++++++-
 1 file changed, 53 insertions(+), 2 deletions(-)

diff --git a/OvmfPkg/IoMmuDxe/CcIoMmu.c b/OvmfPkg/IoMmuDxe/CcIoMmu.c
index b83a9690062b..795b945dacb0 100644
--- a/OvmfPkg/IoMmuDxe/CcIoMmu.c
+++ b/OvmfPkg/IoMmuDxe/CcIoMmu.c
@@ -5,7 +5,7 @@
   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 - 2024, AMD Inc. All rights reserved.<BR>
   Copyright (c) 2017, Intel Corporation. All rights reserved.<BR>
 
   SPDX-License-Identifier: BSD-2-Clause-Patent
@@ -751,7 +751,58 @@ IoMmuSetAttribute (
   IN UINT64                IoMmuAccess
   )
 {
-  return EFI_UNSUPPORTED;
+  MAP_INFO    *MapInfo;
+  EFI_STATUS  Status;
+
+  DEBUG ((DEBUG_VERBOSE, "%a: Mapping=0x%p Access=%lu\n", __func__, Mapping, IoMmuAccess));
+
+  if (Mapping == NULL) {
+    return EFI_INVALID_PARAMETER;
+  }
+
+  Status = EFI_SUCCESS;
+
+  //
+  // An IoMmuAccess value of 0 is always accepted, validate any non-zero value.
+  //
+  if (IoMmuAccess != 0) {
+    MapInfo = (MAP_INFO *)Mapping;
+
+    //
+    // The mapping operation already implied the access mode. Validate that
+    // the supplied access mode matches operation access mode.
+    //
+    switch (MapInfo->Operation) {
+      case EdkiiIoMmuOperationBusMasterRead:
+      case EdkiiIoMmuOperationBusMasterRead64:
+        if (IoMmuAccess != EDKII_IOMMU_ACCESS_READ) {
+          Status = EFI_INVALID_PARAMETER;
+        }
+
+        break;
+
+      case EdkiiIoMmuOperationBusMasterWrite:
+      case EdkiiIoMmuOperationBusMasterWrite64:
+        if (IoMmuAccess != EDKII_IOMMU_ACCESS_WRITE) {
+          Status = EFI_INVALID_PARAMETER;
+        }
+
+        break;
+
+      case EdkiiIoMmuOperationBusMasterCommonBuffer:
+      case EdkiiIoMmuOperationBusMasterCommonBuffer64:
+        if (IoMmuAccess != (EDKII_IOMMU_ACCESS_READ | EDKII_IOMMU_ACCESS_WRITE)) {
+          Status = EFI_INVALID_PARAMETER;
+        }
+
+        break;
+
+      default:
+        Status = EFI_UNSUPPORTED;
+    }
+  }
+
+  return Status;
 }
 
 EDKII_IOMMU_PROTOCOL  mIoMmu = {
-- 
2.42.0



-=-=-=-=-=-=-=-=-=-=-=-
Groups.io Links: You receive all messages sent to this group.
View/Reply Online (#114806): https://edk2.groups.io/g/devel/message/114806
Mute This Topic: https://groups.io/mt/104058148/7686176
Group Owner: devel+owner@edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/unsub [rebecca@openfw.io]
-=-=-=-=-=-=-=-=-=-=-=-

Re: [edk2-devel] [PATCH] OvmfPkg/IoMmuDxe: Provide an implementation for SetAttribute

[Laszlo Ersek]

On 1/30/24 18:15, Tom Lendacky wrote:
> A recent change to the PciIoMap() function now propagates the return code
> from the IoMmu protocol SetAttribute() operation. The implementation of
> this operation in OvmfPkg/IoMmuDxe/CcIoMmu.c returns EFI_UNSUPPORTED,
> resulting in a failure to boot the guest.
> 
> Provide an implementation for SetAttribute() that validates the IoMmu
> access method being requested against the IoMmu mapping operation.
> 
> Suggested-by: Laszlo Ersek <lersek@redhat.com>
> Reviewed-by: Laszlo Ersek <lersek@redhat.com>
> Signed-off-by: Tom Lendacky <thomas.lendacky@amd.com>
> ---
>  OvmfPkg/IoMmuDxe/CcIoMmu.c | 55 +++++++++++++++++++-
>  1 file changed, 53 insertions(+), 2 deletions(-)

Thanks -- I'd like Min or Jiewen to approve this, then I'm glad to merge it.

Laszlo

> 
> diff --git a/OvmfPkg/IoMmuDxe/CcIoMmu.c b/OvmfPkg/IoMmuDxe/CcIoMmu.c
> index b83a9690062b..795b945dacb0 100644
> --- a/OvmfPkg/IoMmuDxe/CcIoMmu.c
> +++ b/OvmfPkg/IoMmuDxe/CcIoMmu.c
> @@ -5,7 +5,7 @@
>    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 - 2024, AMD Inc. All rights reserved.<BR>
>    Copyright (c) 2017, Intel Corporation. All rights reserved.<BR>
>  
>    SPDX-License-Identifier: BSD-2-Clause-Patent
> @@ -751,7 +751,58 @@ IoMmuSetAttribute (
>    IN UINT64                IoMmuAccess
>    )
>  {
> -  return EFI_UNSUPPORTED;
> +  MAP_INFO    *MapInfo;
> +  EFI_STATUS  Status;
> +
> +  DEBUG ((DEBUG_VERBOSE, "%a: Mapping=0x%p Access=%lu\n", __func__, Mapping, IoMmuAccess));
> +
> +  if (Mapping == NULL) {
> +    return EFI_INVALID_PARAMETER;
> +  }
> +
> +  Status = EFI_SUCCESS;
> +
> +  //
> +  // An IoMmuAccess value of 0 is always accepted, validate any non-zero value.
> +  //
> +  if (IoMmuAccess != 0) {
> +    MapInfo = (MAP_INFO *)Mapping;
> +
> +    //
> +    // The mapping operation already implied the access mode. Validate that
> +    // the supplied access mode matches operation access mode.
> +    //
> +    switch (MapInfo->Operation) {
> +      case EdkiiIoMmuOperationBusMasterRead:
> +      case EdkiiIoMmuOperationBusMasterRead64:
> +        if (IoMmuAccess != EDKII_IOMMU_ACCESS_READ) {
> +          Status = EFI_INVALID_PARAMETER;
> +        }
> +
> +        break;
> +
> +      case EdkiiIoMmuOperationBusMasterWrite:
> +      case EdkiiIoMmuOperationBusMasterWrite64:
> +        if (IoMmuAccess != EDKII_IOMMU_ACCESS_WRITE) {
> +          Status = EFI_INVALID_PARAMETER;
> +        }
> +
> +        break;
> +
> +      case EdkiiIoMmuOperationBusMasterCommonBuffer:
> +      case EdkiiIoMmuOperationBusMasterCommonBuffer64:
> +        if (IoMmuAccess != (EDKII_IOMMU_ACCESS_READ | EDKII_IOMMU_ACCESS_WRITE)) {
> +          Status = EFI_INVALID_PARAMETER;
> +        }
> +
> +        break;
> +
> +      default:
> +        Status = EFI_UNSUPPORTED;
> +    }
> +  }
> +
> +  return Status;
>  }
>  
>  EDKII_IOMMU_PROTOCOL  mIoMmu = {



-=-=-=-=-=-=-=-=-=-=-=-
Groups.io Links: You receive all messages sent to this group.
View/Reply Online (#114829): https://edk2.groups.io/g/devel/message/114829
Mute This Topic: https://groups.io/mt/104058148/7686176
Group Owner: devel+owner@edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/leave/12367111/7686176/1913456212/xyzzy [rebecca@openfw.io]
-=-=-=-=-=-=-=-=-=-=-=-

Re: [edk2-devel] [PATCH] OvmfPkg/IoMmuDxe: Provide an implementation for SetAttribute

[Min Xu]

On Wednesday, January 31, 2024 5:33 AM, Laszlo Ersek wrote:
> On 1/30/24 18:15, Tom Lendacky wrote:
> > A recent change to the PciIoMap() function now propagates the return
> > code from the IoMmu protocol SetAttribute() operation. The
> > implementation of this operation in OvmfPkg/IoMmuDxe/CcIoMmu.c
> returns
> > EFI_UNSUPPORTED, resulting in a failure to boot the guest.
> >
> > Provide an implementation for SetAttribute() that validates the IoMmu
> > access method being requested against the IoMmu mapping operation.
> >
> > Suggested-by: Laszlo Ersek <lersek@redhat.com>
> > Reviewed-by: Laszlo Ersek <lersek@redhat.com>
> > Signed-off-by: Tom Lendacky <thomas.lendacky@amd.com>
> > ---
> >  OvmfPkg/IoMmuDxe/CcIoMmu.c | 55 +++++++++++++++++++-
> >  1 file changed, 53 insertions(+), 2 deletions(-)
> 
> Thanks -- I'd like Min or Jiewen to approve this, then I'm glad to merge it.
> 
We have tested the patch in TDX and it works.

Thanks
Min


-=-=-=-=-=-=-=-=-=-=-=-
Groups.io Links: You receive all messages sent to this group.
View/Reply Online (#114846): https://edk2.groups.io/g/devel/message/114846
Mute This Topic: https://groups.io/mt/104058148/7686176
Group Owner: devel+owner@edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/unsub [rebecca@openfw.io]
-=-=-=-=-=-=-=-=-=-=-=-

Re: [edk2-devel] [PATCH] OvmfPkg/IoMmuDxe: Provide an implementation for SetAttribute

[Min Xu]

It works in TDX.

Tested-by: Min Xu <min.m.xu@intel.com>
Reviewed-by: Min Xu <min.m.xu@intel.com>

> -----Original Message-----
> From: Tom Lendacky <thomas.lendacky@amd.com>
> Sent: Wednesday, January 31, 2024 1:16 AM
> To: devel@edk2.groups.io
> Cc: Ard Biesheuvel <ardb+tianocore@kernel.org>; Aktas, Erdem
> <erdemaktas@google.com>; Gerd Hoffmann <kraxel@redhat.com>; Yao,
> Jiewen <jiewen.yao@intel.com>; Laszlo Ersek <lersek@redhat.com>; Xu, Min
> M <min.m.xu@intel.com>; Michael Roth <michael.roth@amd.com>
> Subject: [PATCH] OvmfPkg/IoMmuDxe: Provide an implementation for
> SetAttribute
> 
> A recent change to the PciIoMap() function now propagates the return code
> from the IoMmu protocol SetAttribute() operation. The implementation of
> this operation in OvmfPkg/IoMmuDxe/CcIoMmu.c returns
> EFI_UNSUPPORTED, resulting in a failure to boot the guest.
> 
> Provide an implementation for SetAttribute() that validates the IoMmu access
> method being requested against the IoMmu mapping operation.
> 
> Suggested-by: Laszlo Ersek <lersek@redhat.com>
> Reviewed-by: Laszlo Ersek <lersek@redhat.com>
> Signed-off-by: Tom Lendacky <thomas.lendacky@amd.com>
> ---
>  OvmfPkg/IoMmuDxe/CcIoMmu.c | 55 +++++++++++++++++++-
>  1 file changed, 53 insertions(+), 2 deletions(-)
> 
> diff --git a/OvmfPkg/IoMmuDxe/CcIoMmu.c
> b/OvmfPkg/IoMmuDxe/CcIoMmu.c index b83a9690062b..795b945dacb0
> 100644
> --- a/OvmfPkg/IoMmuDxe/CcIoMmu.c
> +++ b/OvmfPkg/IoMmuDxe/CcIoMmu.c
> @@ -5,7 +5,7 @@
>    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 - 2024, AMD Inc. All rights reserved.<BR>
>    Copyright (c) 2017, Intel Corporation. All rights reserved.<BR>
> 
>    SPDX-License-Identifier: BSD-2-Clause-Patent @@ -751,7 +751,58 @@
> IoMmuSetAttribute (
>    IN UINT64                IoMmuAccess
>    )
>  {
> -  return EFI_UNSUPPORTED;
> +  MAP_INFO    *MapInfo;
> +  EFI_STATUS  Status;
> +
> +  DEBUG ((DEBUG_VERBOSE, "%a: Mapping=0x%p Access=%lu\n", __func__,
> + Mapping, IoMmuAccess));
> +
> +  if (Mapping == NULL) {
> +    return EFI_INVALID_PARAMETER;
> +  }
> +
> +  Status = EFI_SUCCESS;
> +
> +  //
> +  // An IoMmuAccess value of 0 is always accepted, validate any non-zero
> value.
> +  //
> +  if (IoMmuAccess != 0) {
> +    MapInfo = (MAP_INFO *)Mapping;
> +
> +    //
> +    // The mapping operation already implied the access mode. Validate that
> +    // the supplied access mode matches operation access mode.
> +    //
> +    switch (MapInfo->Operation) {
> +      case EdkiiIoMmuOperationBusMasterRead:
> +      case EdkiiIoMmuOperationBusMasterRead64:
> +        if (IoMmuAccess != EDKII_IOMMU_ACCESS_READ) {
> +          Status = EFI_INVALID_PARAMETER;
> +        }
> +
> +        break;
> +
> +      case EdkiiIoMmuOperationBusMasterWrite:
> +      case EdkiiIoMmuOperationBusMasterWrite64:
> +        if (IoMmuAccess != EDKII_IOMMU_ACCESS_WRITE) {
> +          Status = EFI_INVALID_PARAMETER;
> +        }
> +
> +        break;
> +
> +      case EdkiiIoMmuOperationBusMasterCommonBuffer:
> +      case EdkiiIoMmuOperationBusMasterCommonBuffer64:
> +        if (IoMmuAccess != (EDKII_IOMMU_ACCESS_READ |
> EDKII_IOMMU_ACCESS_WRITE)) {
> +          Status = EFI_INVALID_PARAMETER;
> +        }
> +
> +        break;
> +
> +      default:
> +        Status = EFI_UNSUPPORTED;
> +    }
> +  }
> +
> +  return Status;
>  }
> 
>  EDKII_IOMMU_PROTOCOL  mIoMmu = {
> --
> 2.42.0



-=-=-=-=-=-=-=-=-=-=-=-
Groups.io Links: You receive all messages sent to this group.
View/Reply Online (#114847): https://edk2.groups.io/g/devel/message/114847
Mute This Topic: https://groups.io/mt/104058148/7686176
Group Owner: devel+owner@edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/unsub [rebecca@openfw.io]
-=-=-=-=-=-=-=-=-=-=-=-

Re: [edk2-devel] [PATCH] OvmfPkg/IoMmuDxe: Provide an implementation for SetAttribute

[Laszlo Ersek]

On 1/30/24 18:15, Lendacky, Thomas via groups.io wrote:
> A recent change to the PciIoMap() function now propagates the return code
> from the IoMmu protocol SetAttribute() operation. The implementation of
> this operation in OvmfPkg/IoMmuDxe/CcIoMmu.c returns EFI_UNSUPPORTED,
> resulting in a failure to boot the guest.
> 
> Provide an implementation for SetAttribute() that validates the IoMmu
> access method being requested against the IoMmu mapping operation.
> 
> Suggested-by: Laszlo Ersek <lersek@redhat.com>
> Reviewed-by: Laszlo Ersek <lersek@redhat.com>
> Signed-off-by: Tom Lendacky <thomas.lendacky@amd.com>
> ---
>  OvmfPkg/IoMmuDxe/CcIoMmu.c | 55 +++++++++++++++++++-
>  1 file changed, 53 insertions(+), 2 deletions(-)
> 
> diff --git a/OvmfPkg/IoMmuDxe/CcIoMmu.c b/OvmfPkg/IoMmuDxe/CcIoMmu.c
> index b83a9690062b..795b945dacb0 100644
> --- a/OvmfPkg/IoMmuDxe/CcIoMmu.c
> +++ b/OvmfPkg/IoMmuDxe/CcIoMmu.c
> @@ -5,7 +5,7 @@
>    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 - 2024, AMD Inc. All rights reserved.<BR>
>    Copyright (c) 2017, Intel Corporation. All rights reserved.<BR>
>  
>    SPDX-License-Identifier: BSD-2-Clause-Patent
> @@ -751,7 +751,58 @@ IoMmuSetAttribute (
>    IN UINT64                IoMmuAccess
>    )
>  {
> -  return EFI_UNSUPPORTED;
> +  MAP_INFO    *MapInfo;
> +  EFI_STATUS  Status;
> +
> +  DEBUG ((DEBUG_VERBOSE, "%a: Mapping=0x%p Access=%lu\n", __func__, Mapping, IoMmuAccess));
> +
> +  if (Mapping == NULL) {
> +    return EFI_INVALID_PARAMETER;
> +  }
> +
> +  Status = EFI_SUCCESS;
> +
> +  //
> +  // An IoMmuAccess value of 0 is always accepted, validate any non-zero value.
> +  //
> +  if (IoMmuAccess != 0) {
> +    MapInfo = (MAP_INFO *)Mapping;
> +
> +    //
> +    // The mapping operation already implied the access mode. Validate that
> +    // the supplied access mode matches operation access mode.
> +    //
> +    switch (MapInfo->Operation) {
> +      case EdkiiIoMmuOperationBusMasterRead:
> +      case EdkiiIoMmuOperationBusMasterRead64:
> +        if (IoMmuAccess != EDKII_IOMMU_ACCESS_READ) {
> +          Status = EFI_INVALID_PARAMETER;
> +        }
> +
> +        break;
> +
> +      case EdkiiIoMmuOperationBusMasterWrite:
> +      case EdkiiIoMmuOperationBusMasterWrite64:
> +        if (IoMmuAccess != EDKII_IOMMU_ACCESS_WRITE) {
> +          Status = EFI_INVALID_PARAMETER;
> +        }
> +
> +        break;
> +
> +      case EdkiiIoMmuOperationBusMasterCommonBuffer:
> +      case EdkiiIoMmuOperationBusMasterCommonBuffer64:
> +        if (IoMmuAccess != (EDKII_IOMMU_ACCESS_READ | EDKII_IOMMU_ACCESS_WRITE)) {
> +          Status = EFI_INVALID_PARAMETER;
> +        }
> +
> +        break;
> +
> +      default:
> +        Status = EFI_UNSUPPORTED;
> +    }
> +  }
> +
> +  return Status;
>  }
>  
>  EDKII_IOMMU_PROTOCOL  mIoMmu = {

merged as commit

     3  97c3f5b8d272 OvmfPkg/IoMmuDxe: Provide an implementation for SetAttribute

via <https://github.com/tianocore/edk2/pull/5326>



-=-=-=-=-=-=-=-=-=-=-=-
Groups.io Links: You receive all messages sent to this group.
View/Reply Online (#114899): https://edk2.groups.io/g/devel/message/114899
Mute This Topic: https://groups.io/mt/104058148/7686176
Group Owner: devel+owner@edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/leave/12367111/7686176/1913456212/xyzzy [rebecca@openfw.io]
-=-=-=-=-=-=-=-=-=-=-=-