From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from mga05.intel.com (mga05.intel.com [192.55.52.43]) (using TLSv1.2 with cipher ECDHE-RSA-AES256-GCM-SHA384 (256/256 bits)) (No client certificate requested) by ml01.01.org (Postfix) with ESMTPS id 038BD21A18AA9 for ; Mon, 17 Apr 2017 20:45:52 -0700 (PDT) Received: from fmsmga005.fm.intel.com ([10.253.24.32]) by fmsmga105.fm.intel.com with ESMTP; 17 Apr 2017 20:45:52 -0700 X-ExtLoop1: 1 X-IronPort-AV: E=Sophos;i="5.37,217,1488873600"; d="scan'208,217";a="90671971" Received: from fmsmsx104.amr.corp.intel.com ([10.18.124.202]) by fmsmga005.fm.intel.com with ESMTP; 17 Apr 2017 20:45:52 -0700 Received: from fmsmsx121.amr.corp.intel.com (10.18.125.36) by fmsmsx104.amr.corp.intel.com (10.18.124.202) with Microsoft SMTP Server (TLS) id 14.3.319.2; Mon, 17 Apr 2017 20:45:52 -0700 Received: from shsmsx104.ccr.corp.intel.com (10.239.4.70) by fmsmsx121.amr.corp.intel.com (10.18.125.36) with Microsoft SMTP Server (TLS) id 14.3.319.2; Mon, 17 Apr 2017 20:45:51 -0700 Received: from shsmsx102.ccr.corp.intel.com ([169.254.2.246]) by SHSMSX104.ccr.corp.intel.com ([10.239.4.70]) with mapi id 14.03.0319.002; Tue, 18 Apr 2017 11:45:49 +0800 From: "Yao, Jiewen" To: "Duran, Leo" , "edk2-devel@lists.01.org" CC: "Ni, Ruiyu" , "Singh, Brijesh" , Ard Biesheuvel Thread-Topic: [RFC] [PATCH V3 3/3] MdeModulePkg/PciBus: Add IOMMU support. Thread-Index: AQHSt7Tvp+KyLbyPmEWQD+etx9dNzqHKaXuA Date: Tue, 18 Apr 2017 03:45:48 +0000 Message-ID: <74D8A39837DF1E4DA445A8C0B3885C503A92D8ED@shsmsx102.ccr.corp.intel.com> References: <1491289579-15888-1-git-send-email-jiewen.yao@intel.com> <1491289579-15888-4-git-send-email-jiewen.yao@intel.com> In-Reply-To: Accept-Language: zh-CN, en-US X-MS-Has-Attach: X-MS-TNEF-Correlator: dlp-product: dlpe-windows dlp-version: 10.0.102.7 dlp-reaction: no-action x-originating-ip: [10.239.127.40] MIME-Version: 1.0 X-Content-Filtered-By: Mailman/MimeDel 2.1.22 Subject: Re: [RFC] [PATCH V3 3/3] MdeModulePkg/PciBus: Add IOMMU support. X-BeenThere: edk2-devel@lists.01.org X-Mailman-Version: 2.1.22 Precedence: list List-Id: EDK II Development List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , X-List-Received-Date: Tue, 18 Apr 2017 03:45:53 -0000 Content-Language: en-US Content-Type: text/plain; charset="us-ascii" Content-Transfer-Encoding: quoted-printable Yes, I agree. It is a little complicated. NeedAllocateNonExisting is TRUE when: A. IoMMU is present (AND) B. We need remap a Buffer below 4GiB, but we cannot find enough memory the= re. Because CommonBuffer and Read/WriteBuffer is handled in different way, we h= ave to separate #B to be 2 cases. B.1 We need remap a CommBuffer, because no below 4GiB CommBuffer returned i= n AllocateBuffer API. (OR) B.2 We need remap a Read/Write buffer below 4GiB, but we cannot find enough= memory there. As a result: NeedAllocateNonExisting means (A AND (B.1 OR B.2)) Thank you Yao Jiewen From: Duran, Leo [mailto:leo.duran@amd.com] Sent: Tuesday, April 18, 2017 3:58 AM To: Yao, Jiewen ; edk2-devel@lists.01.org Cc: Ni, Ruiyu ; Singh, Brijesh ;= Ard Biesheuvel Subject: RE: [RFC] [PATCH V3 3/3] MdeModulePkg/PciBus: Add IOMMU support. Hi Yao, Regarding RootBridgeIoMap() I'm wondering if may be possible to simplify the logic requiring flags "Nee= dMap" and "NeedAllocateNonExisting"? For example, it seems like (NeedAllocateNonExisting=3D=3DTRUE) implies (gIo= MmuProtocol !=3D 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 >; Duran, Leo = >; > Singh, Brijesh >; Ard= Biesheuvel > > > Subject: [RFC] [PATCH V3 3/3] MdeModulePkg/PciBus: Add IOMMU support. > > The responsibility of PciBus driver is to set IOMMU attribute, because on= ly > 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 fo= r > 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 > > Cc: Leo Duran > > Cc: Brijesh Singh > > Cc: Ard Biesheuvel > > Contributed-under: TianoCore Contribution Agreement 1.0 > Signed-off-by: Jiewen Yao > > --- > 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 = =3D 0; > > EFI_PCI_PLATFORM_PROTOCOL *gPciPlatformProtocol; > EFI_PCI_OVERRIDE_PROTOCOL *gPciOverrideProtocol; > +EDKII_IOMMU_PROTOCOL *gIoMmuProtocol; > +UINTN mIoMmuPageSize =3D 1; > > > GLOBAL_REMOVE_IF_UNREFERENCED > EFI_PCI_HOTPLUG_REQUEST_PROTOCOL mPciHotPlugRequest =3D { @@ - > 256,6 +258,16 @@ PciBusDriverBindingStart ( > } > > gBS->LocateProtocol ( > + &gEdkiiIoMmuProtocolGuid, > + NULL, > + (VOID **) &gIoMmuProtocol > + ); > + if (gIoMmuProtocol !=3D NULL) { > + gIoMmuProtocol->GetPageSize (gIoMmuProtocol, &mIoMmuPageSize); > + ASSERT ((mIoMmuPageSize & (mIoMmuPageSize - 1)) =3D=3D 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 > #include > #include > +#include > > #include > #include @@ -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 *gPciOverrideProtoco= l; > 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 =3D=3D 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 ins= tance. > @@ -967,6 +990,9 @@ PciIoMap ( > { > EFI_STATUS Status; > PCI_IO_DEVICE *PciIoDevice; > + PCI_IO_MAP_INFO *PciIoMapInfo; > + UINT64 IoMmuAttribute; > + EFI_STATUS RemapStatus; > > PciIoDevice =3D PCI_IO_DEVICE_FROM_PCI_IO_THIS (This); > > @@ -982,15 +1008,60 @@ PciIoMap ( > Operation =3D (EFI_PCI_IO_PROTOCOL_OPERATION) (Operation + > EfiPciOperationBusMasterRead64); > } > > - Status =3D PciIoDevice->PciRootBridgeIo->Map ( > - PciIoDevice->PciRootBridgeIo, > - (EFI_PCI_ROOT_BRIDGE_IO_PROTOC= OL_OPERATION) > Operation, > - HostAddress, > - NumberOfBytes, > - DeviceAddress, > - Mapping > - ); > - > + if (gIoMmuProtocol !=3D NULL) { > + PciIoMapInfo =3D AllocatePool (sizeof(*PciIoMapInfo)); > + if (PciIoMapInfo =3D=3D NULL) { > + return EFI_OUT_OF_RESOURCES; > + } > + PciIoMapInfo->Signature =3D PCI_IO_MAP_INFO_SIGNATURE; > + PciIoMapInfo->Operation =3D Operation; > + PciIoMapInfo->NumberOfBytes =3D *NumberOfBytes; > + PciIoMapInfo->DeviceAddress =3D *DeviceAddress; > + PciIoMapInfo->HostAddress =3D 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 alloca= te > via PciIoAllocateBuffer. > + // We cannot use AllocateAlignedPages here, because there might be > more restriction in PciIoAllocateBuffer(). > + // > + PciIoMapInfo->AlignedNumberOfBytes =3D ALIGN_VALUE (PciIoMapInfo- > >NumberOfBytes, mIoMmuPageSize); > + if (PciIoMapInfo->Operation !=3D > EfiPciIoOperationBusMasterCommonBuffer) { > + PciIoMapInfo->AlignedHostAddress =3D AllocateAlignedPages > (EFI_SIZE_TO_PAGES(PciIoMapInfo->AlignedNumberOfBytes), > mIoMmuPageSize); > + if (PciIoMapInfo->AlignedHostAddress =3D=3D 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 =3D PciIoMapInfo->HostAddress; > + } > + PciIoMapInfo->PciRootBridgeIoMapping =3D NULL; > + Status =3D PciIoDevice->PciRootBridgeIo->Map ( > + PciIoDevice->PciRootBridgeIo= , > + > (EFI_PCI_ROOT_BRIDGE_IO_PROTOCOL_OPERATION) Operation, > + PciIoMapInfo->AlignedHostAdd= ress, > + &PciIoMapInfo->AlignedNumber= OfBytes, > + &PciIoMapInfo->DeviceAddress= , > + &PciIoMapInfo->PciRootBridge= IoMapping > + ); } else { > + Status =3D 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 !=3D NULL) { > + if (EFI_ERROR(Status)) { > + if (PciIoMapInfo->Operation !=3D > EfiPciIoOperationBusMasterCommonBuffer) { > + FreePages (PciIoMapInfo->AlignedHostAddress, > EFI_SIZE_TO_PAGES(PciIoMapInfo->AlignedNumberOfBytes)); > + } > + FreePool (PciIoMapInfo); > + } else { > + *DeviceAddress =3D PciIoMapInfo->DeviceAddress; > + *Mapping =3D PciIoMapInfo; > + > + switch (Operation) { > + case EfiPciIoOperationBusMasterRead: > + IoMmuAttribute =3D EDKII_IOMMU_ATTRIBUTE_READ; > + break; > + case EfiPciIoOperationBusMasterWrite: > + IoMmuAttribute =3D EDKII_IOMMU_ATTRIBUTE_WRITE; > + break; > + case EfiPciIoOperationBusMasterCommonBuffer: > + IoMmuAttribute =3D 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 =3D gIoMmuProtocol->GetRemapAddress ( > + gIoMmuProtocol, > + PciIoDevice->Handle, > + PciIoMapInfo->DeviceAddress, > + &PciIoMapInfo->MappedHostAddress > + ); > + if (EFI_ERROR(RemapStatus)) { > + PciIoMapInfo->MappedHostAddress =3D (VOID *)(UINTN)PciIoMapInfo- > >DeviceAddress; > + } > + if (Operation =3D=3D 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 =3D PCI_IO_DEVICE_FROM_PCI_IO_THIS (This); > > + PciIoMapInfo =3D NULL; > + if (gIoMmuProtocol !=3D NULL) { > + PciIoMapInfo =3D NULL; > + for (Link =3D GetFirstNode (&PciIoDevice->Maps) > + ; !IsNull (&PciIoDevice->Maps, Link) > + ; Link =3D GetNextNode (&PciIoDevice->Maps, Link) > + ) { > + PciIoMapInfo =3D PCI_IO_MAP_INFO_FROM_LINK (Link); > + if (PciIoMapInfo =3D=3D Mapping) { > + break; > + } > + } > + // > + // Mapping is not a valid value returned by Map() > + // > + if (PciIoMapInfo !=3D Mapping) { > + DEBUG ((DEBUG_INFO, "PciIoUnmap - PciIoMapInfo not found!\n")); > + return EFI_INVALID_PARAMETER; > + } > + RemoveEntryList (&PciIoMapInfo->Link); > + Mapping =3D 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 =3D=3D EfiPciIoOperationBusMasterWrite) = { > + CopyMem ( > + PciIoMapInfo->HostAddress, > + PciIoMapInfo->MappedHostAddress, > + PciIoMapInfo->NumberOfBytes > + ); > + } > + } > + > Status =3D PciIoDevice->PciRootBridgeIo->Unmap ( > PciIoDevice->PciRootBridgeIo, > Mapping @@ -1037,6 +1204,25 @@= PciIoUnmap ( > ); > } > > + if (gIoMmuProtocol !=3D 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 !=3D > 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))) !=3D 0){ @@ -1085,6 +1272,12 @@ > PciIoAllocateBuffer ( > Attributes |=3D EFI_PCI_ATTRIBUTE_DUAL_ADDRESS_CYCLE; > } > > + if (gIoMmuProtocol !=3D NULL) { > + Size =3D EFI_PAGES_TO_SIZE(Pages); > + Size =3D ALIGN_VALUE(Size, mIoMmuPageSize); > + Pages =3D EFI_SIZE_TO_PAGES (Size); > + } > + > Status =3D PciIoDevice->PciRootBridgeIo->AllocateBuffer ( > PciIoDevice->PciRootBridgeIo, > Type, @@ -1101,7 +1294,9 @@ Pc= iIoAllocateBuffer ( > 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 =3D PCI_IO_DEVICE_FROM_PCI_IO_THIS (This); > > + if (gIoMmuProtocol !=3D NULL) { > + Size =3D EFI_PAGES_TO_SIZE(Pages); > + Size =3D ALIGN_VALUE(Size, mIoMmuPageSize); > + Pages =3D EFI_SIZE_TO_PAGES (Size); > + } > + > Status =3D PciIoDevice->PciRootBridgeIo->FreeBuffer ( > PciIoDevice->PciRootBridgeIo, > Pages, @@ -1144,6 +1346,9 @@ P= ciIoFreeBuffer ( > ); > } > > + // > + // No need to set attribute here, it is done in Unmap. > + // > return Status; > } > > -- > 2.7.4.windows.1