On 02/27/14 14:17, Laszlo Ersek wrote:
> On 02/26/14 23:25, Jordan Justen wrote:
>> On Wed, Feb 26, 2014 at 1:59 PM, Laszlo Ersek <ler...@redhat.com> wrote:
>>> On 02/26/14 20:16, Jordan Justen wrote:
>>>> Commonly a platform will have a PlatformDxe driver. It has kind of
>>>> surprised me how long OVMF has lived without one. One of the main
>>>> things that lived in the PlatformDxe driver is the setup form data.
>>>>
>>>> So, rather than PlatformConfigDxe, could you add PlatformDxe?
>>>
>>> As long as this idea covers only the renaming / relocating of the
>>> driver, sure. But, in that case, would you mind if I squashed the
>>> relevant patches? Their cumulative diffstat is now
>>>
>>> OvmfPkg/OvmfPkgIa32.dsc | 2 +
>>> OvmfPkg/OvmfPkgIa32.fdf | 1 +
>>> OvmfPkg/OvmfPkgIa32X64.dsc | 2 +
>>> OvmfPkg/OvmfPkgIa32X64.fdf | 1 +
>>> OvmfPkg/OvmfPkgX64.dsc | 2 +
>>> OvmfPkg/OvmfPkgX64.fdf | 1 +
>>> OvmfPkg/PlatformConfigDxe/PlatformConfig.c | 730 ++++++++++++++++++
>>> OvmfPkg/PlatformConfigDxe/PlatformConfig.h | 42 +
>>> OvmfPkg/PlatformConfigDxe/PlatformConfig.inf | 59 ++
>>> OvmfPkg/PlatformConfigDxe/PlatformConfig.uni | Bin 0 -> 3322 bytes
>>> .../PlatformConfigDxe/PlatformConfigForms.vfr | 74 ++
>>> 11 files changed, 914 insertions(+)
>>>
>>> Basically it stays completely in PlatformConfigDxe.
>>>
>>> I'm asking because threading a rename through such a rebase is a big
>>> pain. Too bad I'd lose the nice (I hope!) structure of these patches,
>>> ie. the progress and the thought process. Perhaps if I re-enable rename
>>> detection temporarily in my repo, it could work.
>>
>> I think that would work. (Reenabling rename detection.)
>>
>> I don't have that rename detection disabled, and a quick git rebase
>> test seemed to show it should work.
>
> OK, I'll look into it.
>
>>> (2) The platfom config driver installs its HII stuff in its entry point
>>> (it must, it is a device-independent driver). The contents of the
>>> drop-down list is part of the HII stuff. In order to populate the
>>> drop-down list with the selectable resolutions, it needs the GOP, so
>>> there's a Depex on the GOP.
>>
>> PlatformDxe should not have a depex on GOP, but it could have a GOP callback.
>>
>> For a more complicated version, I think the setup data could be
>> published very early, and the form could be updated once GOP is
>> available.
>
> Do you have gBS->RegisterProtocolNotify() in mind? Ie. the platform
> driver would load and start early, but only register a callback for the
> time when the GOP is installed?
>
> I did consider that cursorily. I haven't done such before, and I'm
> worried about how the callback would be "scheduled".
>
> For example, *if* the callback is entered as soon as the video driver
> installs the GOP interface, there could be problems. (Technically this
> would mean that the callback is called on the stack of
> InstallProtocolInterface(), ie. the callback would run before
> InstallProtocolInterface() returns).
>
> Namely, the video driver's internals may not be fully consistent before
> it returns from its binding start function. However, in the callback we
> want to retrieve the mode list, so we call back into the GOP driver (ie.
> we reenter it).
>
> This could crash the video driver in general.
>
> Or, it could force the video driver to be reentrant, or to use locking
> (with TPLs). As in, the binding start function would acquire the lock,
> install the GOP interface, get all the internals in a row, then release
> the lock. The callback would then run inside the "release the lock"
> function.
>
> Is this your suggestion? Because then I'll have to test the above
> function calling orders in QemuVideoDxe, with DEBUG()s.
I checked how event signalling and notification dispatch are
implemented. I think both of my points are valid:
CoreInstallMultipleProtocolInterfaces()
OldTpl = CoreRaiseTpl (TPL_NOTIFY)
CoreInstallProtocolInterface()
CoreInstallProtocolInterfaceNotify()
CoreNotifyProtocolEntry()
CoreSignalEvent()
CoreNotifyEvent()
// queues notify func call [1]
CoreRestoreTpl (OldTpl) [2]
CoreDispatchEventNotifies()
Event->NotifyFunction()
Suppose the code calls gBS->InstallMultipleProtocolInterfaces() at
TPL_APPLICATION (which is the normal, expected way), and that a callback
for the protocol GUID has been previously registered, at TPL_CALLBACK (*).
Then CoreInstallMultipleProtocolInterfaces() will first queue the
callback in gEventQueue. [1]
Second, CoreInstallMultipleProtocolInterfaces() will dispatch the
callback, just before it returns to its caller, in the final
CoreRestoreTpl() call. [2]
Basically dispatch *always* happens inside RestoreTpl() -- that's the
only caller of CoreDispatchEventNotifies().
(a) To reiterate, if the guy installing the protocol interface runs at
TPL_APPLICATION, then the TPL_CALLBACK-level notification function is
dispatched *inside* InstallMultipleProtocolIterfaces() [2], when the
latter lowers the TPL from NOTIFY to APP as last act, and notices that
there are pending CALLBACK notifications.
(b) If the guy installing the protocol interface has been careful and
raised the TPL to CALLBACK on its own, then the CoreRestoreTpl() inside
CoreInstallMultipleProtocolInterfaces() will lower the TPL from NOTIFY
to CALLBACK only [2], and *not* dispatch the notify function that has
become pending. That will happen when the caller of
CoreInstallMultipleProtocolInterfaces() *itself* calls RestoreTpl,
lowering the TPL from CALLBACK to APPLICATION.
Hence if you suggest gBS->RegisterProtocolNotify() indeed, then I'd
either have to (b) raise the TPL in QemuVideoDxe to CALLBACK before
installing the GOP interface, or (a) make the GOP interface installation
the *very last* act in the binding start function, so that the world is
100% ready for reentrancy via the GOP members.
I think I'd prefer (b): gBS->RaiseTpl() is more explicit, and it's
easier to update the "scope of protection" with it. (I can explicitly
control what to protect, and when to allow the callbacks to run.)
We appear to have (a) already in QemuVideoControllerDriverStart() -- a
relatively safe order, with the only thing missing at GOP installation
being the child reference --, but I think that's just a lucky coincidence.
I think I'll keep the TPL thing out of the QemuVideoDxe cleanup series,
and introduce it in the other series, just before I set up the GOP
callback in PlatformDxe.
Thanks,
Laszlo
------------------------------------------------------------------------------
Flow-based real-time traffic analytics software. Cisco certified tool.
Monitor traffic, SLAs, QoS, Medianet, WAAS etc. with NetFlow Analyzer
Customize your own dashboards, set traffic alerts and generate reports.
Network behavioral analysis & security monitoring. All-in-one tool.
http://pubads.g.doubleclick.net/gampad/clk?id=126839071&iu=/4140/ostg.clktrk
_______________________________________________
edk2-devel mailing list
edk2-devel@lists.sourceforge.net
https://lists.sourceforge.net/lists/listinfo/edk2-devel
