On 10/16/13 19:29, Olivier Martin wrote:
> Why is the virtio-mmio implementation of the protocol a library,
> instead of a driver binary?
> The UEFI driver model would encourage to create a virtio-mmio driver
> instead of a library. But the reasons why I created a library are:
>
> - A virtio-mmio driver would imply an additional protocol that would
> probably have a single attribute field:
>
> typedef struct {
> PHYSICAL_ADDRESS BaseAddress;
> } VIRTIO_MMIO_DEVICE_PROTOCOL;
>
> - There is no (easy) way to scan the available VirtIo devices on a
> platform. So, the UEFI firmware for this platform would need a driver
> to produce instances for every virtio devices it wants to expose in
> UEFI. A single call to a helper library (ie: VirtioMmioDeviceLib)
> make the porting easier.
OK.
Comparing again with v3:
>
> Contributed-under: TianoCore Contribution Agreement 1.0
> Signed-off-by: Olivier Martin <olivier.mar...@arm.com>
> ---
> OvmfPkg/Include/Library/VirtioMmioDeviceLib.h | 65 +++++
> .../Library/VirtioMmioDeviceLib/VirtioMmioDevice.c | 204 +++++++++++++
> .../Library/VirtioMmioDeviceLib/VirtioMmioDevice.h | 147 ++++++++++
> .../VirtioMmioDeviceFunctions.c | 304
> ++++++++++++++++++++
> .../VirtioMmioDeviceLib/VirtioMmioDeviceLib.inf | 43 +++
> 5 files changed, 763 insertions(+), 0 deletions(-)
> create mode 100644 OvmfPkg/Include/Library/VirtioMmioDeviceLib.h
> create mode 100644 OvmfPkg/Library/VirtioMmioDeviceLib/VirtioMmioDevice.c
> create mode 100644 OvmfPkg/Library/VirtioMmioDeviceLib/VirtioMmioDevice.h
> create mode 100644
> OvmfPkg/Library/VirtioMmioDeviceLib/VirtioMmioDeviceFunctions.c
> create mode 100644
> OvmfPkg/Library/VirtioMmioDeviceLib/VirtioMmioDeviceLib.inf
>
> diff --git a/OvmfPkg/Include/Library/VirtioMmioDeviceLib.h
> b/OvmfPkg/Include/Library/VirtioMmioDeviceLib.h
> new file mode 100644
> index 0000000..ac71014
> --- /dev/null
> +++ b/OvmfPkg/Include/Library/VirtioMmioDeviceLib.h
> @@ -0,0 +1,65 @@
> +/** @file
> +
> + Definitions for the VirtIo MMIO Device Library
> +
> + Copyright (C) 2013, ARM Ltd
> +
> + This program and the accompanying materials are licensed and made available
> + under the terms and conditions of the BSD License which accompanies this
> + distribution. The full text of the license may be found at
> + http://opensource.org/licenses/bsd-license.php
> +
> + THE PROGRAM IS DISTRIBUTED UNDER THE BSD LICENSE ON AN "AS IS" BASIS,
> WITHOUT
> + WARRANTIES OR REPRESENTATIONS OF ANY KIND, EITHER EXPRESS OR IMPLIED.
> +
> +**/
> +
> +#ifndef _VIRTIO_MMIO_DEVICE_LIB_H_
> +#define _VIRTIO_MMIO_DEVICE_LIB_H_
> +
> +/**
> +
> + Initialize VirtIo Device and Install VIRTIO_DEVICE_PROTOCOL protocol
> +
> + @param[in] BaseAddress Base Address of the VirtIo MMIO Device
> +
> + @param[in] Handle Handle of the device the driver should be attached
> to.
> +
> + @retval EFI_SUCCESS The VirtIo Device has been installed
> + successfully.
> +
> + @retval EFI_OUT_OF_RESOURCES The function failed too allocate memory
> require
> + by the Virtio MMIO device initialization.
Some typos but nothing serious.
[snip]
> +/**
> +
> + Initialize the VirtIo MMIO Device
> +
> + @param[in] BaseAddress Base Address of the VirtIo MMIO Device
> +
> + @param[in, out] Device The driver instance to configure.
> +
> + @retval EFI_SUCCESS Setup complete.
> +
> + @retval EFI_UNSUPPORTED The driver is not a VirtIo MMIO device.
> +
> +**/
The comment block could use some cleanup, but it's not a showstopper of
course.
> +STATIC
> +EFI_STATUS
> +EFIAPI
> +VirtioMmioInit (
> + IN PHYSICAL_ADDRESS BaseAddress,
> + IN OUT VIRTIO_MMIO_DEVICE *Device
> + )
> +{
> + UINT32 MagicValue;
> + UINT32 VendorId;
> + UINT32 Version;
> +
> + // Initialize VirtIo Mmio Device
> + CopyMem (&Device->VirtioDevice, &mMmioDeviceProtocolTemplate,
> + sizeof (VIRTIO_DEVICE_PROTOCOL));
> + Device->BaseAddress = BaseAddress;
> + Device->VirtioDevice.Revision = VIRTIO_SPEC_REVISION (0, 9, 5);
> + Device->VirtioDevice.SubSystemDeviceId =
> + MmioRead32 (BaseAddress + VIRTIO_MMIO_OFFSET_DEVICE_ID);
> +
> + // Double-check MMIO-specific values
> + MagicValue = VIRTIO_CFG_READ (Device, VIRTIO_MMIO_OFFSET_MAGIC);
> + if (MagicValue != VIRTIO_MMIO_MAGIC) {
> + return EFI_UNSUPPORTED;
> + }
> +
> + Version = VIRTIO_CFG_READ (Device, VIRTIO_MMIO_OFFSET_VERSION);
> + if (Version != 1) {
> + return EFI_UNSUPPORTED;
> + }
> +
> + // Double-check MMIO-specific values
> + VendorId = VIRTIO_CFG_READ (Device, VIRTIO_MMIO_OFFSET_VENDOR_ID);
> + if (VendorId != VIRTIO_VENDOR_ID) {
> + // The ARM Base and Foundation Models do not report a valid VirtIo
> VendorId.
> + // They return a value of 0x0 for the VendorId.
> + DEBUG((EFI_D_WARN, "VirtioMmioInit: Warning: The VendorId (0x%X) does
> not "
> + "match the VirtIo VendorId (0x%X).\n",
> + VendorId, VIRTIO_VENDOR_ID));
> + //return EFI_UNSUPPORTED;
> + }
I wouldn't mind a "strict vs. lenient" PCD for this, maybe, but let's
not over-complicate it. OK.
I like version 4 of this function.
> +
> + return EFI_SUCCESS;
> +}
> +
> +
> +/**
> +
> + Uninitialize the internals of a virtio-mmio device that has been
> successfully
> + set up with VirtioMmioInit().
> +
> + @param[in, out] Device The device to clean up.
> +
> +**/
> +
> +STATIC
> +VOID
> +EFIAPI
> +VirtioMmioUninit (
> + IN VIRTIO_MMIO_DEVICE *Device
> + )
> +{
> + // Note: This function mirrors VirtioMmioInit() that does not allocate any
> + // resources - there's nothing to free here.
> +}
> +
> +EFI_STATUS
> +VirtioMmioInstallDevice (
> + IN PHYSICAL_ADDRESS BaseAddress,
> + IN EFI_HANDLE Handle
> + )
> +{
> + EFI_STATUS Status;
> + VIRTIO_MMIO_DEVICE *VirtIo;
> +
> + if (!BaseAddress) {
> + return EFI_INVALID_PARAMETER;
> + }
> + if (Handle == NULL) {
> + return EFI_INVALID_PARAMETER;
> + }
> +
> + // Allocate VIRTIO_MMIO_DEVICE
> + VirtIo = AllocateZeroPool (sizeof (VIRTIO_MMIO_DEVICE));
> + if (VirtIo == NULL) {
> + return EFI_OUT_OF_RESOURCES;
> + }
> +
> + VirtIo->Signature = VIRTIO_MMIO_DEVICE_SIGNATURE;
> +
> + Status = VirtioMmioInit (BaseAddress, VirtIo);
> + if (EFI_ERROR (Status)) {
> + goto FreeVirtioMem;
> + }
> +
> + // Install VIRTIO_DEVICE_PROTOCOL to Handle
> + Status = gBS->InstallProtocolInterface (&Handle,
> + &gVirtioDeviceProtocolGuid, EFI_NATIVE_INTERFACE,
> + &VirtIo->VirtioDevice);
> + if (EFI_ERROR (Status)) {
> + goto UninitVirtio;
> + }
> +
> + return EFI_SUCCESS;
> +
> +UninitVirtio:
> + VirtioMmioUninit (VirtIo);
> +
> +FreeVirtioMem:
> + FreePool (VirtIo);
> + return Status;
> +}
(kept this from the context only for what's coming up below:)
> +
> +EFI_STATUS
> +VirtioMmioUninstallDevice (
> + IN EFI_HANDLE DeviceHandle
> + )
> +{
> + VIRTIO_DEVICE_PROTOCOL *VirtioDevice;
> + VIRTIO_MMIO_DEVICE *MmioDevice;
> + EFI_STATUS Status;
> +
> + Status = gBS->OpenProtocol (
> + DeviceHandle, // candidate device
> + &gVirtioDeviceProtocolGuid, // retrieve the VirtIo iface
> + (VOID **)&VirtioDevice, // target pointer
> + DeviceHandle, // requestor driver identity
> + DeviceHandle, // requesting lookup for
> dev.
> + EFI_OPEN_PROTOCOL_GET_PROTOCOL // lookup only, no ref.
> added
> + );
> + if (EFI_ERROR (Status)) {
> + return Status;
> + }
> +
> + // Get the MMIO device from the VirtIo Device instance
> + MmioDevice = VIRTIO_MMIO_DEVICE_FROM_VIRTIO_DEVICE (VirtioDevice);
> +
> + // Uninstall the protocol interface
> + Status = gBS->UninstallProtocolInterface (DeviceHandle,
> + &gVirtioDeviceProtocolGuid, &MmioDevice->VirtioDevice
> + );
> + if (EFI_ERROR (Status)) {
> + return Status;
> + }
> +
> + // Uninitialize the VirtIo Device
> + VirtioMmioUninit (MmioDevice);
> +
> + return EFI_SUCCESS;
> +}
Yes. This is good.
Except: MmioDevice should be freed with FreePool(). See the error path
at the end of VirtioMmioInstallDevice() above; we should mirror that here.
(1) I think this leak warrants a fix.
> diff --git a/OvmfPkg/Library/VirtioMmioDeviceLib/VirtioMmioDevice.h
> b/OvmfPkg/Library/VirtioMmioDeviceLib/VirtioMmioDevice.h
> new file mode 100644
> index 0000000..3e4e560
> --- /dev/null
> +++ b/OvmfPkg/Library/VirtioMmioDeviceLib/VirtioMmioDevice.h
> @@ -0,0 +1,147 @@
> +/** @file
> +
> + Internal definitions for the VirtIo MMIO Device driver
> +
> + Copyright (C) 2013, ARM Ltd
> +
> + This program and the accompanying materials are licensed and made available
> + under the terms and conditions of the BSD License which accompanies this
> + distribution. The full text of the license may be found at
> + http://opensource.org/licenses/bsd-license.php
> +
> + THE PROGRAM IS DISTRIBUTED UNDER THE BSD LICENSE ON AN "AS IS" BASIS,
> WITHOUT
> + WARRANTIES OR REPRESENTATIONS OF ANY KIND, EITHER EXPRESS OR IMPLIED.
> +
> +**/
I can see that you've dropped VirtioMmioGetQueueSel(), it was unused. OK.
[snip]
> diff --git a/OvmfPkg/Library/VirtioMmioDeviceLib/VirtioMmioDeviceFunctions.c
> b/OvmfPkg/Library/VirtioMmioDeviceLib/VirtioMmioDeviceFunctions.c
> new file mode 100644
> index 0000000..bcba062
> --- /dev/null
> +++ b/OvmfPkg/Library/VirtioMmioDeviceLib/VirtioMmioDeviceFunctions.c
> @@ -0,0 +1,304 @@
> +/** @file
> +
> + This driver produces Virtio Device Protocol instances for Virtio MMIO
> devices.
> +
> + Copyright (C) 2012, Red Hat, Inc.
> + Copyright (c) 2012, Intel Corporation. All rights reserved.<BR>
> + Copyright (C) 2013, ARM Ltd.
> +
> + This program and the accompanying materials are licensed and made available
> + under the terms and conditions of the BSD License which accompanies this
> + distribution. The full text of the license may be found at
> + http://opensource.org/licenses/bsd-license.php
> +
> + THE PROGRAM IS DISTRIBUTED UNDER THE BSD LICENSE ON AN "AS IS" BASIS,
> WITHOUT
> + WARRANTIES OR REPRESENTATIONS OF ANY KIND, EITHER EXPRESS OR IMPLIED.
> +
> +**/
> +
> +#include "VirtioMmioDevice.h"
> +
> +EFI_STATUS
> +EFIAPI
> +VirtioMmioGetDeviceFeatures (
> + IN VIRTIO_DEVICE_PROTOCOL *This,
> + OUT UINT32 *DeviceFeatures
> + )
> +{
> + VIRTIO_MMIO_DEVICE *Device;
> +
> + if (DeviceFeatures == NULL) {
> + return EFI_INVALID_PARAMETER;
> + }
> +
> + Device = VIRTIO_MMIO_DEVICE_FROM_VIRTIO_DEVICE (This);
> +
> + *DeviceFeatures = VIRTIO_CFG_READ (Device,
> VIRTIO_MMIO_OFFSET_HOST_FEATURES);
> +
> + return EFI_SUCCESS;
> +}
> +
> +EFI_STATUS
> +EFIAPI
> +VirtioMmioGetQueueAddress (
> + IN VIRTIO_DEVICE_PROTOCOL *This,
> + OUT UINT32 *QueueAddress
> + )
> +{
> + VIRTIO_MMIO_DEVICE *Device;
> +
> + if (QueueAddress == NULL) {
> +
> + }
(2) Probably cut'n'paste error, you forgot to return
EFI_INVALID_PARAMETER here.
[snip]
> +EFI_STATUS
> +EFIAPI
> +VirtioMmioSetQueueSize (
> + VIRTIO_DEVICE_PROTOCOL *This,
> + UINT16 QueueSize
> + )
> +{
> + VIRTIO_MMIO_DEVICE *Device;
> +
> + Device = VIRTIO_MMIO_DEVICE_FROM_VIRTIO_DEVICE (This);
> +
> + return VIRTIO_CFG_WRITE (Device, VIRTIO_MMIO_OFFSET_QUEUE_NUM, QueueSize);
> +}
(3) This is incorrect use of VIRTIO_CFG_WRITE(), which is a wrapper
around MmioWrite32(). MmioWrite32() always returns the *value* that it
was passed, and it always succeeds.
See MmioWrite32() in "MdePkg/Include/Library/IoLib.h".
This remark applies to VirtioMmioSetQueueSize() only, the other setters
are OK. Just return EFI_SUCCESS here as well.
[snip]
> +EFI_STATUS
> +EFIAPI
> +VirtioMmioSetPageSize (
> + VIRTIO_DEVICE_PROTOCOL *This,
> + UINT32 PageSize
> + )
> +{
> + VIRTIO_MMIO_DEVICE *Device;
> +
> + ASSERT (PageSize == EFI_PAGE_SIZE);
I would have preferred EFI_UNSUPPORTED in place of the assert, but I
don't mind.
[snip]
> +EFI_STATUS
> +EFIAPI
> +VirtioMmioDeviceWrite (
> + IN VIRTIO_DEVICE_PROTOCOL *This,
> + IN UINTN FieldOffset,
> + IN UINTN FieldSize,
> + IN UINT64 Value
> + )
> +{
> + UINTN DstBaseAddress;
> + VIRTIO_MMIO_DEVICE *Device;
> +
> + Device = VIRTIO_MMIO_DEVICE_FROM_VIRTIO_DEVICE (This);
> +
> + // Double-check fieldsize
> + if (FieldSize > 8) {
> + return EFI_INVALID_PARAMETER;
> + }
This inequality is now redundant, given the below:
> +
> + if ((FieldSize != 1) && (FieldSize != 2) &&
> + (FieldSize != 4) && (FieldSize != 8)) {
> + return EFI_INVALID_PARAMETER;
> + }
> +
> + // Compute base address
> + DstBaseAddress = Device->BaseAddress +
> + VIRTIO_DEVICE_SPECIFIC_CONFIGURATION_OFFSET_MMIO + FieldOffset;
> +
> + //
> + // The device-specific memory area of Virtio-MMIO can only be written in
> + // byte accesses. This is not currently in the Virtio spec.
> + //
> + MmioWriteBuffer8 (DstBaseAddress, FieldSize, (UINT8*)&Value);
> +
> + return EFI_SUCCESS;
> +}
Seems OK otherwise.
> +
> +EFI_STATUS
> +EFIAPI
> +VirtioMmioDeviceRead (
> + IN VIRTIO_DEVICE_PROTOCOL *This,
> + IN UINTN FieldOffset,
> + IN UINTN FieldSize,
> + IN UINTN BufferSize,
> + OUT VOID *Buffer
> + )
> +{
> + UINTN SrcBaseAddress;
> + VIRTIO_MMIO_DEVICE *Device;
> +
> + Device = VIRTIO_MMIO_DEVICE_FROM_VIRTIO_DEVICE (This);
> +
> + // Parameter validation
> + ASSERT (FieldSize == BufferSize);
Good, thanks.
> +
> + // Double-check fieldsize
> + if (FieldSize > 8) {
> + return EFI_INVALID_PARAMETER;
> + }
Again, the inequality check has become redundant.
> +
> + if ((FieldSize != 1) && (FieldSize != 2) &&
> + (FieldSize != 4) && (FieldSize != 8)) {
> + return EFI_INVALID_PARAMETER;
> + }
> +
> + // Compute base address
> + SrcBaseAddress = Device->BaseAddress +
> + VIRTIO_DEVICE_SPECIFIC_CONFIGURATION_OFFSET_MMIO + FieldOffset;
> +
> + //
> + // The device-specific memory area of Virtio-MMIO can only be read in
> + // byte reads. This is not currently in the Virtio spec.
> + //
> + MmioReadBuffer8 (SrcBaseAddress, BufferSize, Buffer);
> +
> + return EFI_SUCCESS;
> +}
OK.
> diff --git a/OvmfPkg/Library/VirtioMmioDeviceLib/VirtioMmioDeviceLib.inf
> b/OvmfPkg/Library/VirtioMmioDeviceLib/VirtioMmioDeviceLib.inf
> new file mode 100644
> index 0000000..126afec
> --- /dev/null
> +++ b/OvmfPkg/Library/VirtioMmioDeviceLib/VirtioMmioDeviceLib.inf
> @@ -0,0 +1,43 @@
> +## @file
> +# This driver produces the VirtIo Device Protocol instances for VirtIo Mmio
> +# Device
> +#
> +# Copyright (C) 2013, ARM Ltd
> +#
> +# This program and the accompanying materials are licensed and made available
> +# under the terms and conditions of the BSD License which accompanies this
> +# distribution. The full text of the license may be found at
> +# http://opensource.org/licenses/bsd-license.php
> +#
> +# THE PROGRAM IS DISTRIBUTED UNDER THE BSD LICENSE ON AN "AS IS" BASIS,
> WITHOUT
> +# WARRANTIES OR REPRESENTATIONS OF ANY KIND, EITHER EXPRESS OR IMPLIED.
> +#
> +##
> +
> +[Defines]
> + INF_VERSION = 0x00010006
OK, in synch with the other INF changes in v4.
> + BASE_NAME = VirtioMmioDeviceLib
> + FILE_GUID = 3b6ed966-b5d1-46a8-965b-867ff22d9c89
> + MODULE_TYPE = UEFI_DRIVER
> + VERSION_STRING = 1.0
> + LIBRARY_CLASS = VirtioMmioDeviceLib
> +
> +[Sources]
> + VirtioMmioDevice.c
> + VirtioMmioDeviceFunctions.c
> +
> +[Packages]
> + MdePkg/MdePkg.dec
> + OvmfPkg/OvmfPkg.dec
> +
> +[LibraryClasses]
> + BaseMemoryLib
> + DebugLib
> + IoLib
> + MemoryAllocationLib
> + UefiBootServicesTableLib
> + UefiDriverEntryPoint
As I had said, I would have preferred if UefiDriverEntryPoint had been
dropped -- it probably causes no problems in practice, but it's
confusing for the human reader (we have a library here). Anyway I'll
leave it up to you.
> + UefiLib
> +
> +[Protocols]
> + gVirtioDeviceProtocolGuid ## PRODUCES
>
I think (1) to (3) should be fixed in v5, the rest is just nice-to-have.
Thank you!
Laszlo
------------------------------------------------------------------------------
Android is increasing in popularity, but the open development platform that
developers love is also attractive to malware creators. Download this white
paper to learn more about secure code signing practices that can help keep
Android apps secure.
http://pubads.g.doubleclick.net/gampad/clk?id=65839951&iu=/4140/ostg.clktrk
_______________________________________________
edk2-devel mailing list
edk2-devel@lists.sourceforge.net
https://lists.sourceforge.net/lists/listinfo/edk2-devel
