Hi Laszlo,

On 08/22/2017 06:14 AM, Laszlo Ersek wrote:


So the question: is it possible that we may get called from both
ExitBootService notify and device uninit functions ?

This is a valid problem statement / question, and I verified your code
with regard to it, in the patches that I reviewed.

Let's use VirtioBlkDxe as an example:

(1) The VirtioBlkDriverBindingStart() function calls VirtioBlkInit().

(2) If, and only if, VirtioBlkInit() returns with success, then the ring
has been mapped.

(3) If, and only if, VirtioBlkInit() has returned with success, then we
create the Dev->ExitBoot event (EVT_SIGNAL_EXIT_BOOT_SERVICES) in
VirtioBlkDriverBindingStart(), the notification function being
VirtioBlkExitBoot(). The task priority level at which the notification
function will be enqueued is TPL_CALLBACK (just above TPL_APPLICATION).

(4) If the protocol interface installation fails in
VirtioBlkDriverBindingStart(), then the Dev->ExitBoot is closed first
(which unregisters the notification function), and VirtioBlkUninit() is
called second (more on this below).

(5) Side topic:

According to "Table 23. TPL Restrictions" in the UEFI spec v2.7,
ExitBootServices() may only be called at TPL_APPLICATION. This means
that the notification function will actually be executed before
ExitBootServices() returns.

I'm only mentioning this because in some cases the installation of a
protocol interface has to be done very carefully. Some other module may
have set up a protocol notify e.g. at TPL_CALLBACK. If the driver
installs the protocol interface at TPL_APPLICATION (the default), the
protocol notify in the other driver may be invoked immediately, and that
driver might call back into the first driver, in order to use the
just-installed protocol. This requires that the first driver either
raise the TPL temporarily (so that the protocol installation not result
in an immediate notification in the other driver), or make sure that the
protocol being installed be immediately usable.

With regard to ExitBootServices() however, this is irrelevant. Any
protocol install notification function invoked like above runs at
TPL_CALLBACK, or at a higher task priority level. Therefore it *must
not* call ExitBootServices().

Which, ultimately, guarantees that our BlockIo installation in
VirtioBlkDriverBindingStart() will never result in a direct callback to
VirtioBlkExitBoot().

(6) Now consider VirtioBlkUninit(). With the feature in place,
VirtioBlkUninit() both unmaps and frees the ring. It is called from two
contexts: (a) when we get "far enough" in VirtioBlkDriverBindingStart()
but then fail ultimately (see point (4), and the label "UninitDev"), and
(b) when the binding succeeds just fine, and then later the driver is
asked to unbind (disconnect from) the device -- in
VirtioBlkDriverBindingStop().

In VirtioBlkDriverBindingStop(), we close Dev->ExitBoot, before we call
VirtioBlkUninit() and unmap the vring. Closing the event deregisters the
notification function, and also clears any invocations for the notify
function originally enqueued via this event.

(FWIW, I don't see how any such invocations could be pending here,
because that would imply that some agent called *both*
ExitBootServices() and our VirtioBlkDriverBindingStop() at a TPL higher
than TPL_APPLICATION -- it would be totally invalid. Nonetheless, this
is how closing an event works.)


The upshot is that three scenarios are possible:

- VirtioBlkDriverBindingStart() fails. On return from that function, the
vring is not mapped, and the exit-boot-services callback does not exit.

- VirtioBlkDriverBindingStart() succeeds. On return from the function,
the vring is mapped, and the exit-boot-services callback is live. Then,
the driver is requested to unbind the device
(VirtioBlkDriverBindingStop()). The callback is removed, and the vring
is unmapped in VirtioBlkUninit(). If ExitBootServices() is called
sometime after this, then the callback is not there any longer.

- VirtioBlkDriverBindingStart() succeeds. On return from the function,
the vring is mapped, and the exit-boot-services callback is live. Then,
ExitBootServices() is called. VirtioBlkExitBoot() runs, resetting the
virtio device, and unmapping its vring. VirtioBlkDriverBindingStop() may
never be called after this point, because boot services will have been
exited after all the callbacks run and ExitBootServices() returns.


In general ExitBootServices() notify functions behave very differently
from BindingStop() functions. The latter tear down all the structures
nicely, in reverse order of construction, freeing memory, uninitializing
devices, and so on. In comparison, the former *must not* free memory,
only abort in-flight transfers and de-configure devices surgically,
"in-place", so that they forget previous system memory references. These
two are exclusive.

When people first write UEFI drivers that follow the UEFI driver model,
they are tempted to call (or imitate) the full BindingStop() logic in
the ExitBootServices() notify function. That's wrong. Those contexts are
different with regard to system memory ownership. In BindingStop(), the
memory is owned by the driver. In the ExitBootServices() callback, the
memory is already owned by the OS (sort of), it is only on "loan" to the
driver -- after which a call to BindingStop() is impossible.


Therefore the answer to your question is, "no, that is not possible".

If so, then we will
have issue because now we will end up calling Unmap() twice (once from
ExitBootServices then device uninit). In my debug statement I saw the
call to ExitBootServices but was never able to trigger code path where
the device uninit is called.

If you had been able to trigger such a sequence (exit-boot-services
callback followed by BindingStop), that would imply a huge problem in edk2.

I am wondering if you know any method to
trigger the device uninit code flow so that I can verify both the path.

Testing repeated BindingStart / BindingStop calls is indeed a good idea
-- sort of necessary -- for any UEFI driver that follows the UEFI driver
model. It's easiest to do with the CONNECT, DISCONNECT and RECONNECT
commands in the UEFI shell. (Please see their documentation in the UEFI
shell spec.)

In order to find suitable handles for testing (driver and device
handles), I recommend the DEVICES / DRIVERS / DEVTREE / DH commands.
Personally I find DH the most useful, in particular with the "-d -v" and
"-p" options.

For example, "dh -d -v -p BlockIo" should give you a detailed list of
handles with BlockIo protocol interfaces on them, and the output should
help you find the ones installed by VirtioBlkDxe.

Sorry about the length of this email (I realize a shorter email might
have been more to the point). I hope it helps a little.



Thanks for detail explanation, I was trying driver unload and reload from
the UEFI shell but that did not trigger the BindStop hence I was not sure
how it all works. But your explanation makes sense. I will try connect and
disconnect to trigger the code path and make sure that all logical code path
have been tested before we commit the patch :)

-Brijesh

_______________________________________________
edk2-devel mailing list
edk2-devel@lists.01.org
https://lists.01.org/mailman/listinfo/edk2-devel

Reply via email to