Re: [RFC v4 06/13] OvmfPkg:AmdSevDxe: add AmdSevDxe driver

[Brijesh Singh <brijesh.singh@amd.com>]

On 05/16/2017 12:56 PM, Jordan Justen wrote:
> On 2017-05-16 05:04:58, Brijesh Singh wrote:
>> Hi Jordan,
>>
>> On 5/15/17 12:47 PM, Jordan Justen wrote:
>>> On 2017-05-11 11:01:57, Brijesh Singh wrote:
>>>>
>>>> We basically need some kind of guarantee that this driver is run before any other
>>>> drivers or libs access MMIO register/buffers. In additional to clearing encryption
>>>> bit from MMIO spaces, the driver also installs IOMMU protocol. So far, IOMMU protocol
>>>> is directly consumed by PciHostBridgeDxe driver and QemuFwCfgDxeLib.
>>> What about adding a NULL protocol named
>>> gOvmfIoMmuDetectionProtocolGuid? (Better name suggestions welcomed. :)
>>> Then we can use this protocol in a depex where needed.
>> It should be doable, If I find better name then I will use that :)
>>> Maybe we should consider naming the driver IoMmuDxe instead?
>>>
>>> I think the generic PciRoot bridge driver shouldn't need this in the
>>> depex because it will not start until the BDS phase, and the IoMmuDxe
>>> driver would have been dispatched by then.
>> Are you suggesting that we introduce a new IoMmuDxe driver and install
>> IOMMU protocol unconditionally ?
>
> No. I'm suggesting we have a new protocol that only exists to allow
> dependency expressions to know that we've attempted to detect an IoMmu
> implementation.
>

Okay got it thanks.

> The driver would "install" the protocol with a NULL pointer regardless
> of whether the IoMmu protocol was installed. Maybe
> gOvmfIoMmuDetectionAttemptedProtocolGuid would be a better name?
>
> The DXE fw-cfg library should then list this under depex. I think the
> PCI Host bridge driver doesn't require the depex for the reason I
> mentioned abobe.
>
> The gEdkiiIoMmuProtocolGuid protocol would only be installed be
> installed when detected like your patches currently do.
>
> This method should allow the driver runtime order dependency to be
> explicitly indicated.
>
> Regarding the 'IoMmuDxe' name, I was suggesting that AmdSevDxe be
> renamed to IoMmuDxe. Since we would be installing the 'we tried to
> detect iommu' protocol, it probably makes sense to put all the iommu
> implementation support into a single driver and only install the
> 'detection attempted' protocol it after trying to detect all supported
> iommu implementations.
>
> There could be an issue with this. FvbServicesRuntimeDxe.inf is in the
> apriori currently if SMM_REQUIRE is set, so if it needs the iommu
> treatment, then this wouldn't work. This driver does use MM I/O just
> below 4GB. I don't think your current patches would change how this
> driver runs since it doesn't use the PCI host bridge protocol, so I
> guess it is ok?
>

We do need to ensure that AmdSevDxe runs before FvbServicesRuntimeDxe.inf.
As you rightly pointed, FvbServicesRuntimeDxe uses MM I/O below 4GB hence
we need to clear encryption attribute from MMIO space before QemuFlash
detection logic is invoked. In my patch set, I have listed AmdSevDxe.inf before
FvbServicesRuntimeDxe.inf to ensure that we clear the MMIO before QemuFlash
detection logic from FvbServicesRuntimeDxe.inf is invoked.

Here is fdt file snippet after my patches.

APRIORI DXE {
    INF  MdeModulePkg/Universal/DevicePathDxe/DevicePathDxe.inf
    INF  MdeModulePkg/Universal/PCD/Dxe/Pcd.inf
    INF  OvmfPkg/AmdSevDxe/AmdSevDxe.inf
  !if $(SMM_REQUIRE) == FALSE
    INF  OvmfPkg/QemuFlashFvbServicesRuntimeDxe/FvbServicesRuntimeDxe.inf
  !endif
  }


> (It would be nice to get FvbServicesRuntimeDxe.inf out of the apriori
> too, but that is a separate issue.)
>

Yes, if we can move that out from Apriori then maybe also need to add some
kind of depex to ensure that it gets called after we clear memory encryption
bit from MMIO regions.

-Brijesh