Re: [PATCH V5 0/3] Add IOMMU support.

[Jordan Justen <jordan.l.justen@intel.com>]

This design seems to force a platform the use APRIORI to provide the
IoMmu protocol. Isn't this something we try to avoid?

One thought is that the PciHostBridge driver could add a depex on
IoMmu protocol conditionally based on a feature PCD, right?

Unfortunately, in this case it seems that the IoMmu protocol is
installed conditionally at runtime.

One thought I had was, could OVMF set the PCD to force an IoMmu
dependency, and then install a no-op IoMmu protocol instance if AMD's
SEV is not detected?

Another concern I have is that the IoMmu protocol causes big chunks of
the PciHostBridge Map/Unmap implementation to be skipped. Is this
potentially bypassing something important?

-Jordan

On 2017-05-04 09:32:38, Jiewen Yao wrote:
> ================ V5 ==============
> Minor update from V4.
> 
> 1) Remove unused SetAttribute() API in IOMMU protocol.
> (Feedback from Ruiyu and Ard)
> 2) Rename SetMappingAttribute() to SetAttribute().
> (Feedback from Ruiyu)
> 3) Fix the bug in PciBus driver for Operation
> (Thanks to Ard to catch it)
> 
> V4:
> Tested-by: Brijesh Singh <brijesh.singh@amd.com>
> With the issue in 3/3 addressed:
> Tested-by: Ard Biesheuvel <ard.biesheuvel@linaro.org>
> 
> ================ V4 ==============
> Refine the EDKII_IOMMU_PROTOCOL.
> 
> 1) Add AllocateBuffer/FreeBuffer/Map/Unmap() API.
> They are similar to DmaLib in EmbeddedPkg and
> similar to the previous BmDmaLib (by leo.duran@amd.com).
> 
> These APIs are invoked by PciHostBridge driver
> to allocate DMA memory.
> 
> The PciHostBridge driver (IOMMU consumer) is simplified:
> It uses IOMMU, if IOMMU protocol is present.
> Else it uses original logic.
> 
> 2) Add SetMappingAttribute() API.
> It is similar to SetAttribute() API in V1.
> 
> This API is invoked by PciBus driver to set DMA
> access attribute (read/write) for device.
> 
> The PciBus driver (IOMMU consumer) is simplified:
> It sets access attribute in Map/Unmap,
> if IOMMU protocol is present.
> 
> 3) Remove SetRemapAddress/GetRemapAddress() API.
> Because PciHostBridge/PciBus can call the APIs defined
> above, there is no need to provide remap capability.
> 
> -- Sample producer drivers:
> 1) The sample VTd driver (IOMMU producer)
> is at https://github.com/jyao1/edk2/tree/dma_v4/IntelSiliconPkg/IntelVTdDxe
> 
> It is added to show the concept. It is not fully implemented yet.
> It will not be checked in in this patch.
> 
> 2) The sample AMD SEV driver (IOMMU producer)
> is at https://github.com/jyao1/edk2/tree/dma_v4/IntelSiliconPkg/SampleAmdSevDxe
> (code is borrowed from leo.duran@amd.com and brijesh.singh@amd.com)
> 
> This is not a right place to put this driver.
> 
> It is added to show the concept.
> It is not fully implemented. It will not be checked in.
> Please do not use it directly.
> 
> 3) The sample STYX driver (IOMMU producer)
> is at https://github.com/jyao1/edk2/tree/dma_v4/IntelSiliconPkg/SampleStyxDxe
> (code is borrowed from ard.biesheuvel@linaro.org)
> 
> This is not a right place to put this driver.
> 
> It is added to show the concept.
> It is not fully implemented. It will not be checked in.
> Please do not use it directly.
> 
> 
> ================ V3 ==============
> 1) Add Remap capability (from Ard Biesheuvel)
> Add EDKII_IOMMU_REMAP_ADDRESS API in IOMMU_PROTOCOL.
> 
> NOTE: The code is not fully validated yet.
> The purpose is to collect feedback to decide the next step.
> 
> ================ V2 ==============
> 1) Enhance Unmap() in PciIo (From Ruiyu Ni)
> Maintain a local list of MapInfo and match it in Unmap.
> 
> 2) CopyMem for ReadOperation in PciIo after SetAttribute (Leo Duran)
> Fix a bug in V1 that copy mem for read happen before SetAttribute,
> which will break AMD SEV solution.
> 
> ================ V1 ==============
> 
> This patch series adds IOMMU protocol and updates the consumer
> to support IOMMU based DMA access in UEFI.
> 
> This patch series can support the BmDmaLib request for AMD SEV.
> submitted by Duran, Leo <leo.duran@amd.com> and Brijesh Singh <brijesh.ksingh@gmail.com>.
> https://lists.01.org/pipermail/edk2-devel/2017-March/008109.html, and
> https://lists.01.org/pipermail/edk2-devel/2017-March/008820.html.
> We can have an AMD SEV specific IOMMU driver to produce IOMMU protocol,
> and clear SEV in IOMMU->SetAttribute().
> 
> This patch series can also support Intel VTd based DMA protection,
> requested by Jiewen Yao <jiewen.yao@intel.com>, discussed in
> https://lists.01.org/pipermail/edk2-devel/2017-March/008157.html.
> We can have an Intel VTd specific IOMMU driver to produce IOMMU protocol,
> and update VTd engine to grant or deny access in IOMMU->SetAttribute().
> 
> This patch series does not provide a full Intel VTd driver, which
> will be provide in other patch in the future.
> 
> The purpose of this patch series to review if this IOMMU protocol design
> can meet all DMA access and management requirement.
> 
> 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>
> 
> Jiewen Yao (3):
>   MdeModulePkg/Include: Add IOMMU protocol definition.
>   MdeModulePkg/PciHostBridge: Add IOMMU support.
>   MdeModulePkg/PciBus: Add IOMMU support.
> 
>  MdeModulePkg/Bus/Pci/PciBusDxe/PciBus.c                    |   9 +
>  MdeModulePkg/Bus/Pci/PciBusDxe/PciBus.h                    |   1 +
>  MdeModulePkg/Bus/Pci/PciBusDxe/PciBusDxe.inf               |   1 +
>  MdeModulePkg/Bus/Pci/PciBusDxe/PciIo.c                     |  47 +++-
>  MdeModulePkg/Bus/Pci/PciHostBridgeDxe/PciHostBridge.c      |  37 +++
>  MdeModulePkg/Bus/Pci/PciHostBridgeDxe/PciHostBridgeDxe.inf |   2 +
>  MdeModulePkg/Bus/Pci/PciHostBridgeDxe/PciRootBridge.h      |   2 +
>  MdeModulePkg/Bus/Pci/PciHostBridgeDxe/PciRootBridgeIo.c    |  61 +++++
>  MdeModulePkg/Include/Protocol/IoMmu.h                      | 259 ++++++++++++++++++++
>  MdeModulePkg/MdeModulePkg.dec                              |   3 +
>  10 files changed, 418 insertions(+), 4 deletions(-)
>  create mode 100644 MdeModulePkg/Include/Protocol/IoMmu.h
> 
> -- 
> 2.7.4.windows.1
> 
> _______________________________________________
> edk2-devel mailing list
> edk2-devel@lists.01.org
> https://lists.01.org/mailman/listinfo/edk2-devel