Re: [PATCH v5 07/14] OvmfPkg:IoMmuDxe: Add IoMmuDxe driver

[Laszlo Ersek <lersek@redhat.com>]

On 05/24/17 17:09, Laszlo Ersek wrote:
> (1) So, I don't think that splitting this driver off of AmdSevDxe is a
> significant improvement, given that we still need to add AmdSevDxe to
> the APRIORI DXE file, in order to clear the C bit on NonExistent and
> MMIO ranges.
> 
> If Jordan thinks it is an improvement nonetheless, I don't mind the
> split, of course.
> 
> On 05/22/17 17:23, Brijesh Singh wrote:
>> The IOMMU protocol driver provides capabilities to set a DMA access
>> attribute and methods to allocate, free, map and unmap the DMA memory
>> for the PCI Bus devices.
>>
>> Due to security reasons all DMA operations inside the SEV guest must
>> be performed on shared (i.e unencrypted) pages. The IOMMU protocol
>> driver for the SEV guest uses a bounce buffer to map guest DMA buffer
>> to shared pages inorder to provide the support for DMA operations inside
>> SEV guest.
>>
>> The patch adds a new synthetic/placeholder protocol
>> 'gIoMmuDetectedProtocolGuid" to allow other dependent modules to depend
>> on IoMmuDxe driver being run.
> 
> (2) If we add the protocol GUID to the OVMF DEC file, that should be a
> separate patch, in my opinion. The commit message should explain, in a
> stand-alone manner, what the protocol stands for.
> 
> (I see Jordan's suggestion for the proto name in
> <http://mid.mail-archive.com/149487045771.31444.19976106484440238@jljusten-skl>,
> namely "gOvmfIoMmuDetectionProtocolGuid", but I think that Brijesh's
> suggestion is closer to the protocol names we already have under
> [Protocols] in OvmfPkg.dec.)
> 
>> IoMmuDxe driver looks for SEV capabilities, if present then it installs
>> the real IOMMU protocol otherwise it installs placeholder protocol.
>> Currently, PciRoot Bridge and QemuFWCfgLib need to know the existance
>> of IOMMU protocol. So the modules needing the IOMMU support should add
>> both gEdkiiIoMmuProtocolGuid and gIoMmuDetectedProtocolGuid in there depex.
> 
> (3) This description (which again belongs to the separate patch that
> introduces the protocol) should be formulated without mentioning SEV or
> QemuFwCfgLib. Something like:
> 
>   Platforms that optionally provide an IOMMU protocol should do so by
>   including a DXE driver (usually called IoMmuDxe) that produces either
>   the IOMMU protocol -- if the underlying capabilities are available --,
>   or gIoMmuDetectedProtocolGuid, to signal that the IOMMU capability
>   detection completed with negative result (i.e., no IOMMU will be
>   available in the system).
> 
>   In turn, DXE drivers (and library instances) that are supposed to use
>   the IOMMU protocol if it is available should add the following to
>   their DEPEX:
> 
>     gEdkiiIoMmuProtocolGuid OR gIoMmuDetectedProtocolGuid
> 
>   This ensures these client modules will only be dispatched after IOMMU
>   detection completes (with positive or negative result).
> 
>> Please note that since PciRoot Bridge driver does not run until the BDS
>> phase, and IoMmuDxe driver would have been dispatched by then hence we
>> do not need to add depex in PciRoot Bridge driver inf file.
> 
> (4) This statement is incorrect.
> 
> PciHostBridgeDxe is definitely dispatched before BDS is entered, it is a
> plain DXE driver. It uses platform knowledge (abstracted into
> PciHostBridgeLib --> PciHostBridgeGetRootBridges()) to produce root
> bridge IO protocol instances, in its entry point (-->
> InitializePciHostBridge()).
> 
> PciHostBridgeDxe indeed does not have a depex on the IOMMU protocol. It
> registers a protocol notify instead.
> 
> The reason why PciHostBridgeDxe gets away with this *usually* is that
> the EFI_PCI_ROOT_BRIDGE_IO_PROTOCOL member functions that it exposes are
> *usually* only called from within BDS, after:
> 
> - platform BDS connects the root bridge protocol instances to the PCI
> Bus driver (which is a UEFI driver),
> 
> - the PCI Bus driver produces PciIo protocol instances on top of the
> root bridge IO instances,
> 
> - then various PCI device drivers start massaging the devices via PciIo
> protocol instances, ultimately boiling down to PciHostBridgeDxe()'s
> Map() function and friends.
> 
> However, the following driver dispatch order is also possible, entirely
> within DXE:
> 
> (a) a platform DXE driver is dispatched and registers a protocol notify
> for root bridge IO,
> 
> (b) PciHostBridgeDxe is dispatched and produces a number of root bridge
> IO protocol instances,
> 
> (c) the platform DXE driver gets called back and it uses the root bridge
> IO member functions (such as Map etc),
> 
> (d) The IOMMU DXE driver is dispatched and installs the IOMMU protocol,
> 
> (e) the PciHostBridgeDxe driver is called back, and its Map() etc
> functions will rely on the IOMMU *only* from this point forward.
> 
> This suggests that:
> 
> -  "gIoMmuDetectedProtocolGuid" should actually be called
> "gEdkiiIoMmuAbsentProtocolGuid", and should be upstreamed to MdeModulePkg,
> 
> - all DXE drivers (no exceptions) that *conditionally* depend on
> gEdkiiIoMmuProtocolGuid (with a protocol notify or otherwise) should use
> the following DEPEX instead:
> 
>   gEdkiiIoMmuProtocolGuid OR gEdkiiIoMmuAbsentProtocolGuid
> 
> My point is basically that, when PciHostBridgeDxe installs the
> EFI_PCI_ROOT_BRIDGE_IO_PROTOCOL instances, they are not (necessarily)
> ready for use.

I've been thinking about this.

If you add a patch to the series where you change the DEPEX of
PciHostBridgeDxe to

  gEdkiiIoMmuProtocolGuid OR gEdkiiIoMmuAbsentProtocolGuid

then the maintainer (Ray or Jiewen I think) will likely reject that
patch, because afterwards, all platforms that include PciHostBridgeDxe
would suddenly have to produce gEdkiiIoMmuAbsentProtocolGuid, minimally.

We've faced a similar situation before. The solution was a header-less
library instance, linked into the affected MdeModulePkg driver via NULL
class resolution, through the platform DSC file. The lib instance did
nothing at all (it only had an empty constructor function), but its INF
file spelled out the necessary DEPEX. By linking the lib instance into
the driver, the driver's behavior didn't change, but it became dependent
on the DEPEX.

See the following commits:

05db0948cc60 EmbeddedPkg: introduce EDKII Platform Has ACPI GUID
786f476323a6 EmbeddedPkg: introduce PlatformHasAcpiLib

(See also later commit a391e5925dc3, "MdeModulePkg: move
PlatformHasAcpiGuid from EmbeddedPkg", 2017-04-05.)

We recognized that imparting a depex on a driver "from the outside" is a
somewhat general need, so we filed
<https://bugzilla.tianocore.org/show_bug.cgi?id=443>. The point of this
BZ is to have just one such depex-giving library, and to parametrize
that library with the actual GUID at build time (with a fixed-at-build PCD).

Alas, BZ#443 doesn't really apply here, because the depex we want to
impart on PciHostBridgeDxe here is a disjunction (=OR) of two protocol
GUIDs, not just a single protocol GUID.

So... Not sure if Jordan will agree, but I think ultimately my
suggestion is this:

* call the new placeholder protocol GUID "gIoMmuAbsentProtocolGuid",

* add it to OvmfPkg.dec, but split the GUID introduction off to a
separate patch (see commit message suggested above),

* add a new library instance like described above, under
OvmfPkg/Library, with the OR depex, following the pattern of
PlatformHasAcpiLib. I guess this library instance could be called
PlatformHasIoMmuLib.

* what remains of this patch, for the second part, should be correct
then (install either the real IOMMU proto or the placeholder)

* "[PATCH v5 12/14] OvmfPkg/QemuFwCfgLib: Implement SEV internal
function for Dxe phase" will also be correct, you'll just have to update
the placeholder protocol's name in the DEPEX (plus clean up the cosmetic
remarks)

* finally, identify all DXE_DRIVER, DXE_SMM_DRIVER and
DXE_RUNTIME_DRIVER modules pulled into the OVMF build(s) -- see the DSCs
-- that make use of the IOMMU protocol in some way. Then, hook the new
"OvmfPkg/Library/PlatformHasIoMmuLib/PlatformHasIoMmuLib.inf" into all
those modules in the DSCs, via NULL class resolution.

See commit 3a2c1548fe2d as an example ("ArmVirtPkg: enable AcpiTableDxe
and EFI_ACPI_TABLE_PROTOCOL dynamically", 2017-03-17).


These steps together will ensure:
- no modification to PciHostBridgeDxe, or the other platforms that
consume it
- all IOMMU-capable drivers in OVMF will be sufficiently delayed until
either of the protocols is installed.


Jordan, are you OK with this idea?

Thanks!
Laszlo


>> Cc: Jordan Justen <jordan.l.justen@intel.com>
>> Cc: Laszlo Ersek <lersek@redhat.com>
>> Cc: Leo Duran <leo.duran@amd.com>
>> Cc: Jiewen Yao <jiewen.yao@intel.com>
>> Contributed-under: TianoCore Contribution Agreement 1.0
>> Suggested-by: Jiewen Yao <jiewen.yao@intel.com>
>> Signed-off-by: Brijesh Singh <brijesh.singh@amd.com>
>> Reviewed-by: Jiewen Yao <jiewen.yao@intel.com>
>> ---
>>  OvmfPkg/OvmfPkg.dec            |   1 +
>>  OvmfPkg/OvmfPkgIa32.dsc        |   1 +
>>  OvmfPkg/OvmfPkgIa32X64.dsc     |   1 +
>>  OvmfPkg/OvmfPkgX64.dsc         |   1 +
>>  OvmfPkg/OvmfPkgIa32.fdf        |   1 +
>>  OvmfPkg/OvmfPkgIa32X64.fdf     |   1 +
>>  OvmfPkg/OvmfPkgX64.fdf         |   1 +
>>  OvmfPkg/IoMmuDxe/IoMmuDxe.inf  |  49 +++
>>  OvmfPkg/IoMmuDxe/AmdSevIoMmu.h |  43 ++
>>  OvmfPkg/IoMmuDxe/AmdSevIoMmu.c | 459 ++++++++++++++++++++
>>  OvmfPkg/IoMmuDxe/IoMmuDxe.c    |  53 +++
>>  11 files changed, 611 insertions(+)