Re: [PATCH v2 17/23] OvmfPkg/VirtioBlkDxe: map host address to device address

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

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