On 09/30/13 15:02, Mark Salter wrote:
> On Mon, 2013-09-30 at 12:49 +0200, Laszlo Ersek wrote:
>> On 09/28/13 20:51, Mark Salter wrote:
>>> On Fri, 2013-09-27 at 02:31 +0200, Laszlo Ersek wrote:
>>>> Hi Mark,
>>>>
>>>> just my 2 cents:
>>>>
>>>> On 09/26/13 21:33, Mark Salter wrote:
>>>>> I've been trying out this patch series on AArch64 foundation model.
>>>>> I have UEFI booting a linux kernel image in an EFI system partition.
>>>>> But the kernel itself doesn't see the virtio device. If I boot with
>>>>> an UEFI image without virtio support built in, the same kernel does
>>>>> see the virtio disk device.
>>>>>
>>>>> So, could this be a case of the UEFI virtio driver not releasing the
>>>>> virtio device?
>>>>
>>>> Interesting. We'd only know for sure if we debugged into the kernel
>>>> ("drivers/virtio/virtio_mmio.c" more precisely).
>>>
>>> Well, the virtio_blk probe fails with error of -ENOENT. I tracked that
>>> down to vm_setup_vq() in virtio_mmio.c:
>>>
>>> /* Queue shouldn't already be set up. */
>>> if (readl(vm_dev->base + VIRTIO_MMIO_QUEUE_PFN)) {
>>> err = -ENOENT;
>>> goto error_available;
>>> }
>>>
>>> --Mark
>>
>> Could be a problem in the AArch64 foundation model. Both virtio-mmio and
>> virtio-pci in the guest seem to have good virtio_config_ops.reset
>> implementation, and the problem is not reproducible on virtio-pci. Maybe
>> the host/emulator is not forgetful enough.
>
> My thought was the foundation model as well. But virtio spec 0.9.5,
> appendix X, under QueuePFN description says:
>
> When Guest stops using the queue it must write zero (0x0) to
> this register.
I think that requiring this separately (as something that is not
included in the effects of resetting the device) may be a defect in the
spec. Independently of OVMF, suppose that the OS using virtio-mmio
unexpectedly reboots (BUG_ON() or crash etc). This will leave the device
unusable.
>
> I added an ExitBootServices event handler to do this and the kernel is
> happy with virtio after that. Patch follows although I don't know my way
> around EDK2, so it probably isn't the proper way.
>
>
> diff --git a/OvmfPkg/Library/VirtioMmioDeviceLib/VirtioMmioDevice.c
> b/OvmfPkg/Library/VirtioMmioDeviceLib/VirtioMmioDevice.c
> index 5c88419..2d75d67 100644
> --- a/OvmfPkg/Library/VirtioMmioDeviceLib/VirtioMmioDevice.c
> +++ b/OvmfPkg/Library/VirtioMmioDeviceLib/VirtioMmioDevice.c
> @@ -20,6 +20,11 @@
>
> #include "VirtioMmioDevice.h"
>
> +//
> +// Notifications
> +//
> +EFI_EVENT EfiExitBootServicesEvent = (EFI_EVENT)NULL;
> +
> static VIRTIO_DEVICE_PROTOCOL mMmioDeviceProtocolTemplate = {
> 0, // SubSystemDeviceId
> VirtioMmioGetDeviceFeatures, // GetDeviceFeatures
> @@ -120,6 +125,22 @@ VirtioMmioUninit (
> // the old comms area.
> //
> VIRTIO_CFG_WRITE (Device, VIRTIO_MMIO_OFFSET_STATUS, 0);
> + VIRTIO_CFG_WRITE (Device, VIRTIO_MMIO_OFFSET_QUEUE_PAGE_OFF, 0);
> +}
> +
> +/***************************************
> + * This function should be called
> + * on Event: ExitBootServices
> + * to free up memory, stop the driver
> + * and uninstall the protocols
> + ***************************************/
> +VOID
> +VirtioMmioExitBootServicesEvent (
> + IN EFI_EVENT Event,
> + IN VOID *Context
> + )
> +{
> + VirtioMmioUninit (Context);
> }
>
> EFI_STATUS
> @@ -165,6 +186,21 @@ VirtioMmioInstallDevice (
> goto UninstallVirtio;
> }
>
> + // Register for an ExitBootServicesEvent
> + // When ExitBootServices starts, this function here will make sure that
> the mmio
> + // interface is shutdown.
> + Status = gBS->CreateEvent (
> + EVT_SIGNAL_EXIT_BOOT_SERVICES,
> + TPL_NOTIFY,
> + VirtioMmioExitBootServicesEvent, VirtIo,
> + &EfiExitBootServicesEvent
> + );
> +
> + if (EFI_ERROR(Status)) {
> + DEBUG((DEBUG_ERROR, "VirtioMmioInstallDevice: Can not install the
> ExitBootServicesEvent handler. Exit Status=%r\n", Status));
> + goto UninstallVirtio;
> + }
> +
> return EFI_SUCCESS;
>
> UninstallVirtio:
I believe I argued that VirtioMmioUninit() shouldn't bring down anything
that doesn't mirror a setup/allocation step in VirtioMmioInit().
(Together with my comments for VirtioMmioInit(), that means
VirtioMmioUninit() shouldn't do anything.)
Such an event should probably be registered by the individual virtio
device drivers, on top of virtio-pci / virtio-mmio.
The Linux drivers for virtio-mmio and virtio-pci seem to have a del_vqs
callback, and the specific end-drivers call it (eg.
drivers/block/virtio_blk.c). I think that matches the above idea. Maybe
we need a new protocol member for this, maybe not, but this action is
initiated by the end drivers.
Regarding the NotifyTpl parameter of CreateEvent() -- I'm unsure about
TPL_NOTIFY. VirtioMmioExitBootServicesEvent() would run earlier than
VirtioNetExitBoot(), for example.
Regarding event lifecycle, all events should be closed with CloseEvent()
when the driver that created the event is disconnected from the device.
Thanks
Laszlo
------------------------------------------------------------------------------
October Webinars: Code for Performance
Free Intel webinars can help you accelerate application performance.
Explore tips for MPI, OpenMP, advanced profiling, and more. Get the most from
the latest Intel processors and coprocessors. See abstracts and register >
http://pubads.g.doubleclick.net/gampad/clk?id=60133471&iu=/4140/ostg.clktrk
_______________________________________________
edk2-devel mailing list
edk2-devel@lists.sourceforge.net
https://lists.sourceforge.net/lists/listinfo/edk2-devel
