On 08/22/17 03:42, Brijesh Singh wrote: > > > On 8/21/17 10:24 AM, Laszlo Ersek wrote: > > [Snip]... > >> (25) I think you intended to add the empty line above the "ReleaseQueue" >> label, not below it. >> >>> VirtioRingUninit (Dev->VirtIo, &Dev->Ring); >>> >>> Failed: >> (26) You forgot to call UnmapSharedBuffer() in the VirtioBlkUninit() >> function. Each ring must be unmapped in three places: >> >> - in the device init function, if there is an error and we're past >> mapping the ring, >> - in the device uninit function, >> - in the ExitBootServices notify function. > > I will go through each of your comments and try to address them in v3 > but I would like to understand this code flow a bit more. When we invoke > PciIo->UnmapBuffer(...,Map) [1], it searches for Mapping in its internal > list and if found then unmaps the buffer. So idea is each Map() should > have exactly one Unmap().
(Implementation details aside,) this is correct. Exactly one Unmap() is required. > 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 Laszlo > > [1] > https://github.com/tianocore/edk2/blob/master/OvmfPkg/IoMmuDxe/AmdSevIoMmu.c#L316 > > >>> @@ -885,6 +974,12 @@ VirtioBlkExitBoot ( >>> // >>> Dev = Context; >>> Dev->VirtIo->SetDeviceStatus (Dev->VirtIo, 0); >>> + >>> + // >>> + // Unmap the ring buffer so that hypervisor can not get a readable data >>> + // after device is reset. >>> + // >>> + Dev->VirtIo->UnmapSharedBuffer (Dev->VirtIo, Dev->RingMapping); >>> } >>> >>> /** >>> >> I think I'm skipping the rest of the series for now (except the last >> patch, I have comments for that). >> >> Please rework the rest of the transport-independent virtio drivers (scsi >> and net) based on my comments for this (blk) and the previous (rng) patch. >> >> Thanks! >> Laszlo > _______________________________________________ edk2-devel mailing list [email protected] https://lists.01.org/mailman/listinfo/edk2-devel
