[ipxe-devel] [PATCH 2/2] virtio-net: reset virtio NICs when booting under UEFI

[Laszlo Ersek]

On 04/10/15 22:16, Michael Brown wrote:
> On 10/04/15 20:53, Laszlo Ersek wrote:
>> In efi_init() [src/interface/efi/efi_init.c], iPXE registers the
>> efi_shutdown_hook() function as a callback for the
>> EVT_SIGNAL_EXIT_BOOT_SERVICES event. This event is emitted under UEFI
>> when
>> the OS loader (including the Linux EFI stub) calls ExitBootServices().
>>
>> Currently, that event results in the following call chain:
>>
>> efi_shutdown_hook()                        
>> [src/interface/efi/efi_init.c]
>>    shutdown_boot()                           [src/include/ipxe/init.h]
>>      shutdown(1)                             [src/core/init.c]
>>        /* Call registered shutdown functions (in reverse order) */
>>        forall startup_fn:
>>          startup_fn->shutdown(1)
>>
>> This infrastructure is fine. However, the virtio-net driver does not
>> register such a shutdown function at the moment.
>>
>> Consequently, virtio-net devices remain configured (active) after
>> ExitBootServices().
> 
> Individual iPXE drivers are not expected to have to register shutdown
> functions.  All drivers should have their remove() method called for all
> open devices already.  Registering custom shutdown functions is intended
> for code which needs to do something unusual at shutdown time (e.g. the
> EHCI/xHCI USB bus drivers, which need to decide whether or not to
> attempt to hand control back to the BIOS).
> 
> Looking at the code, I think there might be a missing piece:
> 
> - Under our (very) old EFI driver model, we enumerated the PCI bus
> directly and basically ignored EFI's concept of devices.  The
> efi_shutdown_hook() would therefore result in devices being closed via
> remove_devices() in core/device.c.  This no longer happens, because we
> now attach to devices only as requested by EFI.
> 
> - When built as an EFI application, the efi_root_device in efiprefix.c
> means that the call to remove_devices() will end up calling efi_remove()
> and hence efi_driver_disconnect_all().
> 
> - From a quick look, there seems to be no equivalent functionality for
> when built as an EFI driver.
> 
> I don't have time to look at it properly right now, but could you try
> the attached (totally untested) patch?

Right, I certainly would have wanted such a patch (eg. the UEFI driver
writers' guide mentions resetting all NICs at ExitBootServices(), for
aborting in-flight DMAs and such), I just missed this piece of
infrastructure, and I thought attacking virtio-net first (for which I
have an actual reproducer, although it is pure happenstance) would be a
start.

Another argument I had against "fat" teardown in the ExitBootServices()
event handler is that such handlers are forbidden by the UEFI spec from
doing anything that might change the UEFI memory map, directly or
indirectly -- ie. they must not allocate or release memory.
Theoretically anyway.

In practice the handler already seems to mess with memory allocations,
so I guess calling gBS->DisconnectController() won't matter much. Plus,
if I recall the Linux EFI stub at least, that does have a loop (or at
least a retry?) around the GetMemoryMap() + ExitBootServices() pair.

A UEFI memory map change due to the handlers' actions causes the MapKey
returned by GetMemoryMap() to go stale, then ExitBootServices() should
fail, and the pair should be repeated by the OS loader.

There's just one problem: edk2 (from which OVMF is built) doesn't
implement the above MapKey-goes-stale logic.

http://thread.gmane.org/gmane.comp.bios.tianocore.devel/5577

(As far as I can see, the shutdown() function in "core/init.c" is
armored against repeated calls, so at least the non-conformantly
repeated events should cause no problems.)

I'll test your patch next week. Many thanks!
Laszlo