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

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

Good question.

Comments below:

From: Justen, Jordan L
Sent: Friday, May 12, 2017 4:52 AM
To: Yao, Jiewen <jiewen.yao@intel.com>; edk2-devel@lists.01.org
Cc: Ni, Ruiyu <ruiyu.ni@intel.com>; Leo Duran <leo.duran@amd.com>; Ard Biesheuvel <ard.biesheuvel@linaro.org>; Brijesh Singh <brijesh.singh@amd.com>; Laszlo Ersek <lersek@redhat.com>
Subject: Re: [edk2] [PATCH V5 0/3] Add IOMMU support.

This design seems to force a platform the use APRIORI to provide the
IoMmu protocol. Isn't this something we try to avoid?
[Jiewen] I am confused. This design does not force a platform using APRIORI.
I assume you are talking PciHostBridge driver implementation.

We used protocol callback for IOMMU protocol in PCI HostBridge driver.
For PciHostBridge driver, IOMMU can be dispatched before or after PciHostBridge driver.

For PciBus driver, it is a driver model driver, Start() function is called by BDS.
IOMMU is installed in DXE phase. IOMMU is installed before Pci.Start().

For X86 VTd IOMMU platform, we need PCI bus driver or host bridge driver calls IOMMU in BDS.
That is enough. We do not need APRIORI.


If a platform wants to dispatch IOMMU earlier, that is OK.
But that is the platform choice, not enforcement.


On the other hand, enforcing IOMMU dependency means any platform MUST add a new IOMMU driver,
if this platform is using the open source PciHostBridge driver.
That is the main reason, I did not want to force IOMMU dependency.
I do not want to bring an incompatible change for existing platform.




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?
[Jiewen] APRIORI is defined by PI specification vol 2 DXE CIS - 10.3 The A Priori File
====================
The a priori file provides a deterministic execution order of DXE drivers. DXE drivers that are executed solely based on their dependency expression are weakly ordered. This means that the execution order is not completely deterministic between boots or between platforms. There are cases where a deterministic execution order is required.
……
The main purpose of the a priori file is to provide a greater degree of flexibility in the firmware design of a platform.
====================
This is industry standard. And this is the platform choice.

Looking at EDKII, we are already using that:
  C:\home\Edk-II\OvmfPkg\OvmfPkgIa32.fdf(149):APRIORI PEI {
  C:\home\Edk-II\OvmfPkg\OvmfPkgIa32.fdf(190):APRIORI DXE {
  C:\home\Edk-II\OvmfPkg\OvmfPkgIa32X64.fdf(149):APRIORI PEI {
  C:\home\Edk-II\OvmfPkg\OvmfPkgIa32X64.fdf(190):APRIORI DXE {
  C:\home\Edk-II\OvmfPkg\OvmfPkgX64.fdf(149):APRIORI PEI {
  C:\home\Edk-II\OvmfPkg\OvmfPkgX64.fdf(190):APRIORI DXE {
  C:\home\Edk-II\QuarkPlatformPkg\Quark.fdf(306):APRIORI PEI {
  C:\home\Edk-II\QuarkPlatformPkg\QuarkMin.fdf(306):APRIORI PEI {
  C:\home\Edk-II\Vlv2TbltDevicePkg\PlatformPkg.fdf(441):APRIORI DXE {
  C:\home\Edk-II\Vlv2TbltDevicePkg\PlatformPkgGcc.fdf(398):APRIORI DXE {
  C:\home\EdkIIGit\edk2\Nt32Pkg\Nt32Pkg.fdf(164):APRIORI PEI {
  C:\home\EdkIIGit\edk2\Nt32Pkg\Nt32Pkg.fdf(170):APRIORI DXE {
The APRIORI is added when the platform is created.

APRIORI is much simpler than a no-op IOMMU, from my point of view.

I do not have concern if a platform wants to use dependency. (Again, that is platform choice)
But at same time, may I know what is the concern if a platform wants to use APRIORI?




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?
[Jiewen] No.


-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<mailto:brijesh.singh@amd.com>>
> With the issue in 3/3 addressed:
> Tested-by: Ard Biesheuvel <ard.biesheuvel@linaro.org<mailto: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<mailto: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<mailto:leo.duran@amd.com> and brijesh.singh@amd.com<mailto: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<mailto: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<mailto:leo.duran@amd.com>> and Brijesh Singh <brijesh.ksingh@gmail.com<mailto: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<mailto: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<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>>
>
> 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<mailto:edk2-devel@lists.01.org>
> https://lists.01.org/mailman/listinfo/edk2-devel