Re: [PATCH 1/3] MdeModulePkg/Include: Add IOMMU protocol definition.

["Yao, Jiewen" <jiewen.yao@intel.com>]

Thanks, Ard.

It is a good open that if we really need both SetAttribute and SetMappingAttribute.

Looking at code again, I prefer to remove SetAttribute().
I do not suggest we define MapInfo, because I want to make it DMA engine implementation choice.
All we want is just to set an access attribute for this mapping, there is no need to input base/length.
Because Map() should always be called by a device driver, no matter it is PCI DMA, ISA DMA, or low power controller DMA, I think SetMappingAttribute is good enough.

I will plan to remove SetAttribute() in V5.

Thank you
Yao Jiewen


From: edk2-devel [mailto:edk2-devel-bounces@lists.01.org] On Behalf Of Ard Biesheuvel
Sent: Tuesday, May 2, 2017 5:56 PM
To: Yao, Jiewen <jiewen.yao@intel.com>
Cc: Ni, Ruiyu <ruiyu.ni@intel.com>; edk2-devel@lists.01.org; Leo Duran <leo.duran@amd.com>
Subject: Re: [edk2] [PATCH 1/3] MdeModulePkg/Include: Add IOMMU protocol definition.

Hello Jiewen,

On 29 April 2017 at 14:48, Jiewen Yao <jiewen.yao@intel.com<mailto:jiewen.yao@intel.com>> wrote:
> This protocol is to abstract DMA access from IOMMU.
> 1) Intel "DMAR" ACPI table.
> 2) AMD "IVRS" ACPI table
> 3) ARM "IORT" ACPI table.
>
> There might be multiple IOMMU engines on one platform.
> For example, one for graphic and one for rest PCI devices
> (such as ATA/USB).
> All IOMMU engines are reported by one ACPI table.
>
> All IOMMU protocol provider should be based upon ACPI table.
> This single IOMMU protocol can handle multiple IOMMU engines on one system.
>
> This IOMMU protocol provider can use UEFI device path to distinguish
> if the device is graphic or ATA/USB, and find out corresponding
> IOMMU engine.
>
> The IOMMU protocol provides 2 capabilities:
> A) Set DMA access attribute - such as write/read control.
> B) Remap DMA memory - such as remap above 4GiB system memory address
> to below 4GiB device address.
> It provides AllocateBuffer/FreeBuffer/Map/Unmap for DMA memory.
> The remapping can be static (fixed at build time) or dynamic (allocate
> at runtime).
>
> 4) AMD "SEV" feature.
> We can have an AMD SEV specific IOMMU driver to produce IOMMU protocol,
> and manage SEV bit.
>
> Cc: Ruiyu Ni <ruiyu.ni@intel.com<mailto:ruiyu.ni@intel.com>>
> Cc: Leo Duran <leo.duran@amd.com<mailto:leo.duran@amd.com>>
> Cc: Brijesh Singh <brijesh.singh@amd.com<mailto:brijesh.singh@amd.com>>
> Cc: Ard Biesheuvel <ard.biesheuvel@linaro.org<mailto:ard.biesheuvel@linaro.org>>
> Contributed-under: TianoCore Contribution Agreement 1.0
> Signed-off-by: Jiewen Yao <jiewen.yao@intel.com<mailto:jiewen.yao@intel.com>>
> ---
>  MdeModulePkg/Include/Protocol/IoMmu.h | 310 ++++++++++++++++++++
>  MdeModulePkg/MdeModulePkg.dec         |   3 +
>  2 files changed, 313 insertions(+)
>
> diff --git a/MdeModulePkg/Include/Protocol/IoMmu.h b/MdeModulePkg/Include/Protocol/IoMmu.h
> new file mode 100644
> index 0000000..3f62f46
> --- /dev/null
> +++ b/MdeModulePkg/Include/Protocol/IoMmu.h
> @@ -0,0 +1,310 @@
> +/** @file
> +  EFI IOMMU Protocol.
> +
> +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 that 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 __IOMMU_H__
> +#define __IOMMU_H__
> +
> +//
> +// IOMMU Protocol GUID value
> +//
> +#define EDKII_IOMMU_PROTOCOL_GUID \
> +    { \
> +      0x4e939de9, 0xd948, 0x4b0f, { 0x88, 0xed, 0xe6, 0xe1, 0xce, 0x51, 0x7c, 0x1e } \
> +    }
> +
> +//
> +// Forward reference for pure ANSI compatability
> +//
> +typedef struct _EDKII_IOMMU_PROTOCOL  EDKII_IOMMU_PROTOCOL;
> +
> +//
> +// Revision The revision to which the IOMMU interface adheres.
> +//          All future revisions must be backwards compatible.
> +//          If a future version is not back wards compatible it is not the same GUID.
> +//
> +#define EDKII_IOMMU_PROTOCOL_REVISION 0x00010000
> +
> +//
> +// IOMMU Access for SetAttribute
> +//
> +// These types can be "ORed" together as needed.
> +// Any undefined bits are reserved and must be zero.
> +//
> +#define EDKII_IOMMU_ACCESS_READ   0x1
> +#define EDKII_IOMMU_ACCESS_WRITE  0x2
> +
> +//
> +// IOMMU Operation for Map
> +//
> +typedef enum {
> +  ///
> +  /// A read operation from system memory by a bus master that is not capable of producing
> +  /// PCI dual address cycles.
> +  ///
> +  EdkiiIoMmuOperationBusMasterRead,
> +  ///
> +  /// A write operation from system memory by a bus master that is not capable of producing
> +  /// PCI dual address cycles.
> +  ///
> +  EdkiiIoMmuOperationBusMasterWrite,
> +  ///
> +  /// Provides both read and write access to system memory by both the processor and a bus
> +  /// master that is not capable of producing PCI dual address cycles.
> +  ///
> +  EdkiiIoMmuOperationBusMasterCommonBuffer,
> +  ///
> +  /// A read operation from system memory by a bus master that is capable of producing PCI
> +  /// dual address cycles.
> +  ///
> +  EdkiiIoMmuOperationBusMasterRead64,
> +  ///
> +  /// A write operation to system memory by a bus master that is capable of producing PCI
> +  /// dual address cycles.
> +  ///
> +  EdkiiIoMmuOperationBusMasterWrite64,
> +  ///
> +  /// Provides both read and write access to system memory by both the processor and a bus
> +  /// master that is capable of producing PCI dual address cycles.
> +  ///
> +  EdkiiIoMmuOperationBusMasterCommonBuffer64,
> +  EdkiiIoMmuOperationMaximum
> +} EDKII_IOMMU_OPERATION;
> +
> +//
> +// IOMMU attribute for AllocateBuffer
> +// Any undefined bits are reserved and must be zero.
> +//
> +#define EDKII_IOMMU_ATTRIBUTE_MEMORY_WRITE_COMBINE        0x0080
> +#define EDKII_IOMMU_ATTRIBUTE_MEMORY_CACHED               0x0800
> +#define EDKII_IOMMU_ATTRIBUTE_DUAL_ADDRESS_CYCLE          0x8000
> +
> +#define EDKII_IOMMU_ATTRIBUTE_VALID_FOR_ALLOCATE_BUFFER   (EDKII_IOMMU_ATTRIBUTE_MEMORY_WRITE_COMBINE | EDKII_IOMMU_ATTRIBUTE_MEMORY_CACHED | EDKII_IOMMU_ATTRIBUTE_DUAL_ADDRESS_CYCLE)
> +
> +#define EDKII_IOMMU_ATTRIBUTE_INVALID_FOR_ALLOCATE_BUFFER (~EDKII_IOMMU_ATTRIBUTE_VALID_FOR_ALLOCATE_BUFFER)
> +
> +/**
> +  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]  DeviceAddress     The base of device memory address to be used as the DMA memory.
> +  @param[in]  Length            The length of device memory address to be used as the DMA memory.
> +  @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  DeviceAddress is not IoMmu Page size aligned.
> +  @retval EFI_INVALID_PARAMETER  Length is not IoMmu Page size aligned.
> +  @retval EFI_INVALID_PARAMETER  Length is 0.
> +  @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 DeviceAddress and Length.
> +  @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.
> +
> +**/
> +typedef
> +EFI_STATUS
> +(EFIAPI *EDKII_IOMMU_SET_ATTRIBUTE)(
> +  IN EDKII_IOMMU_PROTOCOL  *This,
> +  IN EFI_HANDLE            DeviceHandle,
> +  IN EFI_PHYSICAL_ADDRESS  DeviceAddress,
> +  IN UINT64                Length,
> +  IN UINT64                IoMmuAccess
> +  );
> +

It is a bit unfortunate that we need both SetAttribute and
SetMappingAttribute. Could we instead make the MAPPING_INFO struct (or
at least, some part of it) part of the protocol? This would allow the
PCI bus driver to call SetAttribute() directly, taking the values from
the struct.


> +/**
> +  Provides the controller-specific addresses required to access system memory from a
> +  DMA bus master.
> +
> +  @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.
> +
> +**/
> +typedef
> +EFI_STATUS
> +(EFIAPI *EDKII_IOMMU_MAP)(
> +  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
> +  );
> +
> +/**
> +  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.
> +**/
> +typedef
> +EFI_STATUS
> +(EFIAPI *EDKII_IOMMU_UNMAP)(
> +  IN  EDKII_IOMMU_PROTOCOL                     *This,
> +  IN  VOID                                     *Mapping
> +  );
> +
> +/**
> +  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.
> +
> +**/
> +typedef
> +EFI_STATUS
> +(EFIAPI *EDKII_IOMMU_ALLOCATE_BUFFER)(
> +  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
> +  );
> +
> +/**
> +  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().
> +
> +**/
> +typedef
> +EFI_STATUS
> +(EFIAPI *EDKII_IOMMU_FREE_BUFFER)(
> +  IN  EDKII_IOMMU_PROTOCOL                     *This,
> +  IN  UINTN                                    Pages,
> +  IN  VOID                                     *HostAddress
> +  );
> +
> +/**
> +  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.
> +
> +**/
> +typedef
> +EFI_STATUS
> +(EFIAPI *EDKII_IOMMU_SET_MAPPING_ATTRIBUTE)(
> +  IN EDKII_IOMMU_PROTOCOL  *This,
> +  IN EFI_HANDLE            DeviceHandle,
> +  IN VOID                  *Mapping,
> +  IN UINT64                IoMmuAccess
> +  );
> +
> +///
> +/// IOMMU Protocol structure.
> +///
> +struct _EDKII_IOMMU_PROTOCOL {
> +  UINT64                              Revision;
> +  EDKII_IOMMU_SET_ATTRIBUTE           SetAttribute;
> +  EDKII_IOMMU_MAP                     Map;
> +  EDKII_IOMMU_UNMAP                   Unmap;
> +  EDKII_IOMMU_ALLOCATE_BUFFER         AllocateBuffer;
> +  EDKII_IOMMU_FREE_BUFFER             FreeBuffer;
> +  EDKII_IOMMU_SET_MAPPING_ATTRIBUTE   SetMappingAttribute;
> +};
> +
> +///
> +/// IOMMU Protocol GUID variable.
> +///
> +extern EFI_GUID gEdkiiIoMmuProtocolGuid;
> +
> +#endif
> diff --git a/MdeModulePkg/MdeModulePkg.dec b/MdeModulePkg/MdeModulePkg.dec
> index 356b3e1..db596b6 100644
> --- a/MdeModulePkg/MdeModulePkg.dec
> +++ b/MdeModulePkg/MdeModulePkg.dec
> @@ -540,6 +540,9 @@
>    ## Include/Protocol/NonDiscoverableDevice.h
>    gEdkiiNonDiscoverableDeviceProtocolGuid = { 0x0d51905b, 0xb77e, 0x452a, {0xa2, 0xc0, 0xec, 0xa0, 0xcc, 0x8d, 0x51, 0x4a } }
>
> +  ## Include/Protocol/IoMmu.h
> +  gEdkiiIoMmuProtocolGuid = { 0x4e939de9, 0xd948, 0x4b0f, { 0x88, 0xed, 0xe6, 0xe1, 0xce, 0x51, 0x7c, 0x1e } }
> +
>  #
>  # [Error.gEfiMdeModulePkgTokenSpaceGuid]
>  #   0x80000001 | Invalid value provided.
> --
> 2.7.4.windows.1
>
_______________________________________________
edk2-devel mailing list
edk2-devel@lists.01.org<mailto:edk2-devel@lists.01.org>
https://lists.01.org/mailman/listinfo/edk2-devel