From: "Duran, Leo" <leo.duran@amd.com>
To: 'Jiewen Yao' <jiewen.yao@intel.com>,
"edk2-devel@lists.01.org" <edk2-devel@lists.01.org>
Cc: Ruiyu Ni <ruiyu.ni@intel.com>,
"Singh, Brijesh" <brijesh.singh@amd.com>,
Ard Biesheuvel <ard.biesheuvel@linaro.org>
Subject: Re: [RFC] [PATCH V3 3/3] MdeModulePkg/PciBus: Add IOMMU support.
Date: Mon, 17 Apr 2017 19:58:00 +0000 [thread overview]
Message-ID: <DM5PR12MB1243014A3B356EDE961B1D36F9060@DM5PR12MB1243.namprd12.prod.outlook.com> (raw)
In-Reply-To: <1491289579-15888-4-git-send-email-jiewen.yao@intel.com>
Hi Yao,
Regarding RootBridgeIoMap()
I'm wondering if may be possible to simplify the logic requiring flags "NeedMap" and "NeedAllocateNonExisting"?
For example, it seems like (NeedAllocateNonExisting==TRUE) implies (gIoMmuProtocol != NULL), but that did not seem obvious at a glance.
Leo.
> -----Original Message-----
> From: Jiewen Yao [mailto:jiewen.yao@intel.com]
> Sent: Tuesday, April 04, 2017 2:06 AM
> To: edk2-devel@lists.01.org
> Cc: Ruiyu Ni <ruiyu.ni@intel.com>; Duran, Leo <leo.duran@amd.com>;
> Singh, Brijesh <brijesh.singh@amd.com>; Ard Biesheuvel
> <ard.biesheuvel@linaro.org>
> Subject: [RFC] [PATCH V3 3/3] MdeModulePkg/PciBus: Add IOMMU support.
>
> The responsibility of PciBus driver is to set IOMMU attribute, because only
> PciBus knows which device submits DMA access request.
>
> PciBus driver guarantee that the request to PciHostBridge is IOMMU page
> aligned memory, as such PciHostBridge can allocate non-existent memory for
> device memory, to satisfy remap requirement.
>
> PciBus driver does not assume device address is same as the mapped host
> address, because IOMMU may remap it.
>
> Cc: Ruiyu Ni <ruiyu.ni@intel.com>
> Cc: Leo Duran <leo.duran@amd.com>
> Cc: Brijesh Singh <brijesh.singh@amd.com>
> Cc: Ard Biesheuvel <ard.biesheuvel@linaro.org>
> Contributed-under: TianoCore Contribution Agreement 1.0
> Signed-off-by: Jiewen Yao <jiewen.yao@intel.com>
> ---
> MdeModulePkg/Bus/Pci/PciBusDxe/PciBus.c | 12 ++
> MdeModulePkg/Bus/Pci/PciBusDxe/PciBus.h | 19 ++
> MdeModulePkg/Bus/Pci/PciBusDxe/PciBusDxe.inf | 1 +
> MdeModulePkg/Bus/Pci/PciBusDxe/PciIo.c | 225
> +++++++++++++++++++-
> 4 files changed, 247 insertions(+), 10 deletions(-)
>
> diff --git a/MdeModulePkg/Bus/Pci/PciBusDxe/PciBus.c
> b/MdeModulePkg/Bus/Pci/PciBusDxe/PciBus.c
> index f3be47a..c9ee4de 100644
> --- a/MdeModulePkg/Bus/Pci/PciBusDxe/PciBus.c
> +++ b/MdeModulePkg/Bus/Pci/PciBusDxe/PciBus.c
> @@ -42,6 +42,8 @@ UINT64 gAllZero = 0;
>
> EFI_PCI_PLATFORM_PROTOCOL *gPciPlatformProtocol;
> EFI_PCI_OVERRIDE_PROTOCOL *gPciOverrideProtocol;
> +EDKII_IOMMU_PROTOCOL *gIoMmuProtocol;
> +UINTN mIoMmuPageSize = 1;
>
>
> GLOBAL_REMOVE_IF_UNREFERENCED
> EFI_PCI_HOTPLUG_REQUEST_PROTOCOL mPciHotPlugRequest = { @@ -
> 256,6 +258,16 @@ PciBusDriverBindingStart (
> }
>
> gBS->LocateProtocol (
> + &gEdkiiIoMmuProtocolGuid,
> + NULL,
> + (VOID **) &gIoMmuProtocol
> + );
> + if (gIoMmuProtocol != NULL) {
> + gIoMmuProtocol->GetPageSize (gIoMmuProtocol, &mIoMmuPageSize);
> + ASSERT ((mIoMmuPageSize & (mIoMmuPageSize - 1)) == 0); }
> +
> + gBS->LocateProtocol (
> &gEfiIncompatiblePciDeviceSupportProtocolGuid,
> NULL,
> (VOID **) &gIncompatiblePciDeviceSupport diff --git
> a/MdeModulePkg/Bus/Pci/PciBusDxe/PciBus.h
> b/MdeModulePkg/Bus/Pci/PciBusDxe/PciBus.h
> index 39ba8b9..185d89c 100644
> --- a/MdeModulePkg/Bus/Pci/PciBusDxe/PciBus.h
> +++ b/MdeModulePkg/Bus/Pci/PciBusDxe/PciBus.h
> @@ -32,6 +32,7 @@ WITHOUT WARRANTIES OR REPRESENTATIONS OF ANY
> KIND, EITHER EXPRESS OR IMPLIED.
> #include <Protocol/IncompatiblePciDeviceSupport.h>
> #include <Protocol/PciOverride.h>
> #include <Protocol/PciEnumerationComplete.h>
> +#include <Protocol/IoMmu.h>
>
> #include <Library/DebugLib.h>
> #include <Library/UefiDriverEntryPoint.h> @@ -289,6 +290,8 @@ struct
> _PCI_IO_DEVICE {
> // This field is used to support this case.
> //
> UINT16 BridgeIoAlignment;
> +
> + LIST_ENTRY Maps;
> };
>
> #define PCI_IO_DEVICE_FROM_PCI_IO_THIS(a) \ @@ -304,6 +307,20 @@
> struct _PCI_IO_DEVICE {
> CR (a, PCI_IO_DEVICE, LoadFile2, PCI_IO_DEVICE_SIGNATURE)
>
>
> +#define PCI_IO_MAP_INFO_SIGNATURE SIGNATURE_32 ('p', 'm', 'a', 'p')
> +typedef struct {
> + UINT32 Signature;
> + LIST_ENTRY Link;
> + EFI_PCI_IO_PROTOCOL_OPERATION Operation;
> + VOID *HostAddress;
> + EFI_PHYSICAL_ADDRESS DeviceAddress;
> + UINTN NumberOfBytes;
> + VOID *AlignedHostAddress;
> + UINTN AlignedNumberOfBytes;
> + VOID *MappedHostAddress;
> + VOID *PciRootBridgeIoMapping;
> +} PCI_IO_MAP_INFO;
> +#define PCI_IO_MAP_INFO_FROM_LINK(a) CR (a, PCI_IO_MAP_INFO,
> Link,
> +PCI_IO_MAP_INFO_SIGNATURE)
>
> //
> // Global Variables
> @@ -321,6 +338,8 @@ extern EFI_PCI_PLATFORM_PROTOCOL
> *gPciPlatformProtocol;
> extern EFI_PCI_OVERRIDE_PROTOCOL *gPciOverrideProtocol;
> extern BOOLEAN mReserveIsaAliases;
> extern BOOLEAN mReserveVgaAliases;
> +extern EDKII_IOMMU_PROTOCOL *gIoMmuProtocol;
> +extern UINTN mIoMmuPageSize;
>
> /**
> Macro that checks whether device is a GFX device.
> diff --git a/MdeModulePkg/Bus/Pci/PciBusDxe/PciBusDxe.inf
> b/MdeModulePkg/Bus/Pci/PciBusDxe/PciBusDxe.inf
> index a3ab11f..5da094f 100644
> --- a/MdeModulePkg/Bus/Pci/PciBusDxe/PciBusDxe.inf
> +++ b/MdeModulePkg/Bus/Pci/PciBusDxe/PciBusDxe.inf
> @@ -95,6 +95,7 @@
> gEfiPciRootBridgeIoProtocolGuid ## TO_START
> gEfiIncompatiblePciDeviceSupportProtocolGuid ##
> SOMETIMES_CONSUMES
> gEfiLoadFile2ProtocolGuid ## SOMETIMES_PRODUCES
> + gEdkiiIoMmuProtocolGuid ## SOMETIMES_CONSUMES
>
> [FeaturePcd]
> gEfiMdeModulePkgTokenSpaceGuid.PcdPciBusHotplugDeviceSupport ##
> CONSUMES
> diff --git a/MdeModulePkg/Bus/Pci/PciBusDxe/PciIo.c
> b/MdeModulePkg/Bus/Pci/PciBusDxe/PciIo.c
> index f72598d..31b8c32 100644
> --- a/MdeModulePkg/Bus/Pci/PciBusDxe/PciIo.c
> +++ b/MdeModulePkg/Bus/Pci/PciBusDxe/PciIo.c
> @@ -58,6 +58,7 @@ InitializePciIoInstance (
> )
> {
> CopyMem (&PciIoDevice->PciIo, &mPciIoInterface, sizeof
> (EFI_PCI_IO_PROTOCOL));
> + InitializeListHead (&PciIoDevice->Maps);
> }
>
> /**
> @@ -936,6 +937,28 @@ PciIoCopyMem (
> }
>
> /**
> + Return if a value is aligned.
> +
> + @param Value the value to be checked
> + @param Alignment the alignment to be checked with.
> +
> + @retval TRUE The value is aligned
> + @retval FALSE The value is not aligned.
> +**/
> +BOOLEAN
> +InternalIsAlgined (
> + IN UINTN Value,
> + IN UINTN Alignment
> + )
> +{
> + if (Value == ALIGN_VALUE(Value, Alignment)) {
> + return TRUE;
> + } else {
> + return FALSE;
> + }
> +}
> +
> +/**
> Provides the PCI controller-specific addresses needed to access system
> memory.
>
> @param This A pointer to the EFI_PCI_IO_PROTOCOL instance.
> @@ -967,6 +990,9 @@ PciIoMap (
> {
> EFI_STATUS Status;
> PCI_IO_DEVICE *PciIoDevice;
> + PCI_IO_MAP_INFO *PciIoMapInfo;
> + UINT64 IoMmuAttribute;
> + EFI_STATUS RemapStatus;
>
> PciIoDevice = PCI_IO_DEVICE_FROM_PCI_IO_THIS (This);
>
> @@ -982,15 +1008,60 @@ PciIoMap (
> Operation = (EFI_PCI_IO_PROTOCOL_OPERATION) (Operation +
> EfiPciOperationBusMasterRead64);
> }
>
> - Status = PciIoDevice->PciRootBridgeIo->Map (
> - PciIoDevice->PciRootBridgeIo,
> - (EFI_PCI_ROOT_BRIDGE_IO_PROTOCOL_OPERATION)
> Operation,
> - HostAddress,
> - NumberOfBytes,
> - DeviceAddress,
> - Mapping
> - );
> -
> + if (gIoMmuProtocol != NULL) {
> + PciIoMapInfo = AllocatePool (sizeof(*PciIoMapInfo));
> + if (PciIoMapInfo == NULL) {
> + return EFI_OUT_OF_RESOURCES;
> + }
> + PciIoMapInfo->Signature = PCI_IO_MAP_INFO_SIGNATURE;
> + PciIoMapInfo->Operation = Operation;
> + PciIoMapInfo->NumberOfBytes = *NumberOfBytes;
> + PciIoMapInfo->DeviceAddress = *DeviceAddress;
> + PciIoMapInfo->HostAddress = HostAddress;
> + //
> + // For non common buffer, we always allocate a new memory if IOMMU
> exists.
> + // because the original memory might not be DMA capable.
> + //
> + // For common buffer, it is not needed, because common buffer allocate
> via PciIoAllocateBuffer.
> + // We cannot use AllocateAlignedPages here, because there might be
> more restriction in PciIoAllocateBuffer().
> + //
> + PciIoMapInfo->AlignedNumberOfBytes = ALIGN_VALUE (PciIoMapInfo-
> >NumberOfBytes, mIoMmuPageSize);
> + if (PciIoMapInfo->Operation !=
> EfiPciIoOperationBusMasterCommonBuffer) {
> + PciIoMapInfo->AlignedHostAddress = AllocateAlignedPages
> (EFI_SIZE_TO_PAGES(PciIoMapInfo->AlignedNumberOfBytes),
> mIoMmuPageSize);
> + if (PciIoMapInfo->AlignedHostAddress == NULL) {
> + FreePool (PciIoMapInfo);
> + return EFI_OUT_OF_RESOURCES;
> + }
> + } else {
> + //
> + // For common buffer, the HostAddress must be allocated via
> PciIoAllocateBuffer.
> + //
> + if (!InternalIsAlgined((UINTN)PciIoMapInfo->HostAddress,
> mIoMmuPageSize)) {
> + FreePool (PciIoMapInfo);
> + DEBUG ((DEBUG_ERROR, "PciIoMap - map unaligned common buffer
> with IOMMU\n"));
> + return EFI_UNSUPPORTED;
> + }
> + PciIoMapInfo->AlignedHostAddress = PciIoMapInfo->HostAddress;
> + }
> + PciIoMapInfo->PciRootBridgeIoMapping = NULL;
> + Status = PciIoDevice->PciRootBridgeIo->Map (
> + PciIoDevice->PciRootBridgeIo,
> +
> (EFI_PCI_ROOT_BRIDGE_IO_PROTOCOL_OPERATION) Operation,
> + PciIoMapInfo->AlignedHostAddress,
> + &PciIoMapInfo->AlignedNumberOfBytes,
> + &PciIoMapInfo->DeviceAddress,
> + &PciIoMapInfo->PciRootBridgeIoMapping
> + ); } else {
> + Status = PciIoDevice->PciRootBridgeIo->Map (
> + PciIoDevice->PciRootBridgeIo,
> +
> (EFI_PCI_ROOT_BRIDGE_IO_PROTOCOL_OPERATION) Operation,
> + HostAddress,
> + NumberOfBytes,
> + DeviceAddress,
> + Mapping
> + ); }
> if (EFI_ERROR (Status)) {
> REPORT_STATUS_CODE_WITH_DEVICE_PATH (
> EFI_ERROR_CODE | EFI_ERROR_MINOR, @@ -999,6 +1070,63 @@
> PciIoMap (
> );
> }
>
> + if (gIoMmuProtocol != NULL) {
> + if (EFI_ERROR(Status)) {
> + if (PciIoMapInfo->Operation !=
> EfiPciIoOperationBusMasterCommonBuffer) {
> + FreePages (PciIoMapInfo->AlignedHostAddress,
> EFI_SIZE_TO_PAGES(PciIoMapInfo->AlignedNumberOfBytes));
> + }
> + FreePool (PciIoMapInfo);
> + } else {
> + *DeviceAddress = PciIoMapInfo->DeviceAddress;
> + *Mapping = PciIoMapInfo;
> +
> + switch (Operation) {
> + case EfiPciIoOperationBusMasterRead:
> + IoMmuAttribute = EDKII_IOMMU_ATTRIBUTE_READ;
> + break;
> + case EfiPciIoOperationBusMasterWrite:
> + IoMmuAttribute = EDKII_IOMMU_ATTRIBUTE_WRITE;
> + break;
> + case EfiPciIoOperationBusMasterCommonBuffer:
> + IoMmuAttribute = EDKII_IOMMU_ATTRIBUTE_READ |
> EDKII_IOMMU_ATTRIBUTE_WRITE;
> + break;
> + default:
> + ASSERT(FALSE);
> + return EFI_INVALID_PARAMETER;
> + }
> + //
> + // PciHostBridge should map IOMMU page aligned HostAddress.
> + //
> + gIoMmuProtocol->SetAttribute (
> + gIoMmuProtocol,
> + PciIoDevice->Handle,
> + PciIoMapInfo->DeviceAddress,
> + PciIoMapInfo->AlignedNumberOfBytes,
> + IoMmuAttribute
> + );
> + //
> + // We need do copy mem after IoMmu->SetAttribute(),
> + // because it might change IOMMU state.
> + //
> + RemapStatus = gIoMmuProtocol->GetRemapAddress (
> + gIoMmuProtocol,
> + PciIoDevice->Handle,
> + PciIoMapInfo->DeviceAddress,
> + &PciIoMapInfo->MappedHostAddress
> + );
> + if (EFI_ERROR(RemapStatus)) {
> + PciIoMapInfo->MappedHostAddress = (VOID *)(UINTN)PciIoMapInfo-
> >DeviceAddress;
> + }
> + if (Operation == EfiPciIoOperationBusMasterRead) {
> + CopyMem (
> + PciIoMapInfo->MappedHostAddress,
> + PciIoMapInfo->HostAddress,
> + PciIoMapInfo->NumberOfBytes
> + );
> + }
> + InsertTailList (&PciIoDevice->Maps, &PciIoMapInfo->Link);
> + }
> + }
> return Status;
> }
>
> @@ -1021,9 +1149,48 @@ PciIoUnmap (
> {
> EFI_STATUS Status;
> PCI_IO_DEVICE *PciIoDevice;
> + PCI_IO_MAP_INFO *PciIoMapInfo;
> + LIST_ENTRY *Link;
>
> PciIoDevice = PCI_IO_DEVICE_FROM_PCI_IO_THIS (This);
>
> + PciIoMapInfo = NULL;
> + if (gIoMmuProtocol != NULL) {
> + PciIoMapInfo = NULL;
> + for (Link = GetFirstNode (&PciIoDevice->Maps)
> + ; !IsNull (&PciIoDevice->Maps, Link)
> + ; Link = GetNextNode (&PciIoDevice->Maps, Link)
> + ) {
> + PciIoMapInfo = PCI_IO_MAP_INFO_FROM_LINK (Link);
> + if (PciIoMapInfo == Mapping) {
> + break;
> + }
> + }
> + //
> + // Mapping is not a valid value returned by Map()
> + //
> + if (PciIoMapInfo != Mapping) {
> + DEBUG ((DEBUG_INFO, "PciIoUnmap - PciIoMapInfo not found!\n"));
> + return EFI_INVALID_PARAMETER;
> + }
> + RemoveEntryList (&PciIoMapInfo->Link);
> + Mapping = PciIoMapInfo->PciRootBridgeIoMapping;
> +
> + //
> + // PciHostBridge should map IOMMU page aligned HostAddress.
> + //
> + // We need do copy mem before PciRootBridgeIo->Unmap(),
> + // because it might free mapped host address.
> + //
> + if (PciIoMapInfo->Operation == EfiPciIoOperationBusMasterWrite) {
> + CopyMem (
> + PciIoMapInfo->HostAddress,
> + PciIoMapInfo->MappedHostAddress,
> + PciIoMapInfo->NumberOfBytes
> + );
> + }
> + }
> +
> Status = PciIoDevice->PciRootBridgeIo->Unmap (
> PciIoDevice->PciRootBridgeIo,
> Mapping @@ -1037,6 +1204,25 @@ PciIoUnmap (
> );
> }
>
> + if (gIoMmuProtocol != NULL) {
> + if (!EFI_ERROR(Status)) {
> + //
> + // PciHostBridge should map IOMMU page aligned HostAddress.
> + //
> + gIoMmuProtocol->SetAttribute (
> + gIoMmuProtocol,
> + PciIoDevice->Handle,
> + PciIoMapInfo->DeviceAddress,
> + PciIoMapInfo->AlignedNumberOfBytes,
> + 0
> + );
> + if (PciIoMapInfo->Operation !=
> EfiPciIoOperationBusMasterCommonBuffer) {
> + FreePages (PciIoMapInfo->AlignedHostAddress,
> EFI_SIZE_TO_PAGES(PciIoMapInfo->AlignedNumberOfBytes));
> + }
> + FreePool (PciIoMapInfo);
> + }
> + }
> +
> return Status;
> }
>
> @@ -1073,6 +1259,7 @@ PciIoAllocateBuffer ( {
> EFI_STATUS Status;
> PCI_IO_DEVICE *PciIoDevice;
> + UINTN Size;
>
> if ((Attributes &
> (~(EFI_PCI_ATTRIBUTE_MEMORY_WRITE_COMBINE |
> EFI_PCI_ATTRIBUTE_MEMORY_CACHED))) != 0){ @@ -1085,6 +1272,12 @@
> PciIoAllocateBuffer (
> Attributes |= EFI_PCI_ATTRIBUTE_DUAL_ADDRESS_CYCLE;
> }
>
> + if (gIoMmuProtocol != NULL) {
> + Size = EFI_PAGES_TO_SIZE(Pages);
> + Size = ALIGN_VALUE(Size, mIoMmuPageSize);
> + Pages = EFI_SIZE_TO_PAGES (Size);
> + }
> +
> Status = PciIoDevice->PciRootBridgeIo->AllocateBuffer (
> PciIoDevice->PciRootBridgeIo,
> Type, @@ -1101,7 +1294,9 @@ PciIoAllocateBuffer (
> PciIoDevice->DevicePath
> );
> }
> -
> + //
> + // No need to set attribute here, it is done in Map.
> + //
> return Status;
> }
>
> @@ -1127,9 +1322,16 @@ PciIoFreeBuffer ( {
> EFI_STATUS Status;
> PCI_IO_DEVICE *PciIoDevice;
> + UINTN Size;
>
> PciIoDevice = PCI_IO_DEVICE_FROM_PCI_IO_THIS (This);
>
> + if (gIoMmuProtocol != NULL) {
> + Size = EFI_PAGES_TO_SIZE(Pages);
> + Size = ALIGN_VALUE(Size, mIoMmuPageSize);
> + Pages = EFI_SIZE_TO_PAGES (Size);
> + }
> +
> Status = PciIoDevice->PciRootBridgeIo->FreeBuffer (
> PciIoDevice->PciRootBridgeIo,
> Pages, @@ -1144,6 +1346,9 @@ PciIoFreeBuffer (
> );
> }
>
> + //
> + // No need to set attribute here, it is done in Unmap.
> + //
> return Status;
> }
>
> --
> 2.7.4.windows.1
next prev parent reply other threads:[~2017-04-17 19:58 UTC|newest]
Thread overview: 10+ messages / expand[flat|nested] mbox.gz Atom feed top
2017-04-04 7:06 [RFC] [PATCH V3 0/3] Add IOMMU support Jiewen Yao
2017-04-04 7:06 ` [RFC] [PATCH V3 1/3] MdeModulePkg/Include: Add IOMMU protocol definition Jiewen Yao
2017-04-17 13:42 ` Ard Biesheuvel
2017-04-17 15:13 ` Yao, Jiewen
2017-04-04 7:06 ` [RFC] [PATCH V3 2/3] MdeModulePkg/PciHostBridge: Add IOMMU support Jiewen Yao
2017-04-17 19:33 ` Duran, Leo
2017-04-18 3:31 ` Yao, Jiewen
2017-04-04 7:06 ` [RFC] [PATCH V3 3/3] MdeModulePkg/PciBus: " Jiewen Yao
2017-04-17 19:58 ` Duran, Leo [this message]
2017-04-18 3:45 ` Yao, Jiewen
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-list from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=DM5PR12MB1243014A3B356EDE961B1D36F9060@DM5PR12MB1243.namprd12.prod.outlook.com \
--to=devel@edk2.groups.io \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox