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

[Laszlo Ersek <lersek@redhat.com>]

On 05/16/17 22:25, Brijesh Singh wrote:
> 
> 
> 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.

Adding a couple of points down here.

(1) As Brijesh points out, AmdSevDxe does two things.

The second thing (the IOMMU protocol implementation) could be detached,
yes, and as Jordan suggests, we could introduce a synthetic /
placeholder protocol as well, for dependent modules to depend on.

DEPEXes can use "OR" operators (see "Table 21. Dependency Expression
Opcode Summary" in Vol2 of PI 1.5), so a library instance could impart
client modules with an alternative dependency on either the real IOMMU
protocol thing or the placeholder thing. AmdSevDxe would then look for
the SEV capability, and install the appropriate (IOMMU or placeholder)
protocol.

However, the other thing that AmdSevDxe provides doesn't look possible
to move out of the APRIORI DXE file. Clearing the C bit on all
nonexistent GCD ranges, and on the MMIO GCD ranges (which at that point
all come from PEI HOBs) enables *all* DXE drivers to go about their MMIO
range additions and allocations without knowing about SEV. I think
keeping generic MMIO-using DXE drivers blissfully unaware of SEV is an
important design goal.

(2) QemuFlashFvbServicesRuntimeDxe is in the APRIORI DXE file when
SMM_REQUIRE is *clear*, not when it is set.

When SMM_REQUIRE is set, then pflash is a hard requirement (*), and the
build includes only QemuFlashFvbServicesRuntimeDxe -- namely, the SMM
build thereof --; the build doesn't include EmuVariableFvbRuntimeDxe.
Therefore only one FVB provider exists, so there's no need to enforce
any dispatch order between "competing" FVB providers.

With SMM_REQUIRE being *clear*, there are two competing FVB providers,
and QemuFlashFvbServicesRuntimeDxe must get priority over
EmuVariableFvbRuntimeDxe. This is why we add
QemuFlashFvbServicesRuntimeDxe to the APRIORI DXE file in that case.

Please see commit 46df0216b0ed ("OvmfPkg: pull in SMM-based variable
driver stack", 2015-11-30).

(*) Dynamically degrading flash access from pflash to emulated would be
a security bug. This is why SMM_REQUIRE is called SMM_REQUIRE and not
SMM_ENABLE, and why hanging the SMM_REQUIRE build when pflash is missing
is the right thing.

Thanks,
Laszlo