On 09/23/13 15:37, Olivier Martin wrote:
> Contributed-under: TianoCore Contribution Agreement 1.0
> Signed-off-by: Olivier Martin <olivier.mar...@arm.com>
> ---
> OvmfPkg/Include/Library/VirtioMmioDeviceLib.h | 48 ++++
> .../Library/VirtioMmioDeviceLib/VirtioMmioDevice.c | 201 +++++++++++++
> .../Library/VirtioMmioDeviceLib/VirtioMmioDevice.h | 160 +++++++++++
> .../VirtioMmioDeviceFunctions.c | 297
> ++++++++++++++++++++
> .../VirtioMmioDeviceLib/VirtioMmioDeviceLib.inf | 43 +++
> 5 files changed, 749 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..2168ed7
> --- /dev/null
> +++ b/OvmfPkg/Include/Library/VirtioMmioDeviceLib.h
> @@ -0,0 +1,48 @@
> +/** @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_
> +
> +#include <Protocol/VirtioDevice.h>
> +
> +#define VIRTIO_MMIO_DEVICE_SIGNATURE SIGNATURE_32 ('V', 'M', 'I', 'O')
> +
> +typedef struct {
> + UINT32 Signature;
> + VIRTIO_DEVICE_PROTOCOL VirtioDevice;
> + PHYSICAL_ADDRESS BaseAddress;
> + EFI_HANDLE Handle;
> +} VIRTIO_MMIO_DEVICE;
> +
> +#define VIRTIO_MMIO_DEVICE_FROM_VIRTIO_DEVICE(Device) \
> + CR (Device, VIRTIO_MMIO_DEVICE, VirtioDevice,
> VIRTIO_MMIO_DEVICE_SIGNATURE)
> +#define VIRTIO_MMIO_DEVICE_FROM_HANDLE(Device) \
> + CR (Device, VIRTIO_MMIO_DEVICE, Handle, VIRTIO_MMIO_DEVICE_SIGNATURE)
The VIRTIO_MMIO_DEVICE.Handle field, and the
VIRTIO_MMIO_DEVICE_FROM_HANDLE() macro, are only used in
VirtioMmioUninstallDevice().
VirtioMmioUninstallDevice() is never called in this patchset.
What is the intended use case? Are the Protocol Handler Services
insufficient for this purpose? I mean, if the client code knows about
Handle (and it can), then gVirtioDeviceProtocolGuid can be located on
it, and uninstalled form it.
Actually, I think VirtioMmioUninstallDevice() is incorrect (quoting it
out of order):
> +EFI_STATUS
> +VirtioMmioUninstallDevice (
> + IN EFI_HANDLE Handle
> + )
> +{
> + VIRTIO_MMIO_DEVICE *Device;
> + EFI_STATUS Status;
> +
> + // Get the device from handle
> + Device = VIRTIO_MMIO_DEVICE_FROM_HANDLE (Handle);
The VIRTIO_MMIO_DEVICE_FROM_HANDLE() macro does the following:
- Take a pointer into a VIRTIO_MMIO_DEVICE structure, let's denote it
with MmioDev,
- this pointer is interpreted as the *address* of the MmioDev.Handle
field,
- Subtract the relevant offset of the pointer so that we get the
beginning of the structure,
- assert that the structure begins with the correct signature.
The value passed to VIRTIO_MMIO_DEVICE_FROM_HANDLE() is not the
*address* of any Handle field embedded into a VIRTIO_MMIO_DEVICE
structure. For that the function would have to take an (EFI_HANDLE*),
not an EFI_HANDLE.
The compiler doesn't catch this, because EFI_HANDLE is itself a pointer
(a (VOID*)), and the CR() underlying VIRTIO_MMIO_DEVICE_FROM_HANDLE()
immediately casts it.
In practice (with debugging enabled), the assertion on the signature
would fire (we're poking into the data that is pointed-to by the VOID*
that the handle is), but the patchset never calls the function, so it's
not exposed.
> +
> + // Uninstall the protocol interface
> + Status = gBS->UninstallProtocolInterface (Handle,
> + &gVirtioDeviceProtocolGuid, &Device->VirtioDevice
> + );
> + if (EFI_ERROR (Status)) {
> + return Status;
> + }
> +
> + // Uninitialize the VirtIo Device
> + VirtioMmioUninit (Device);
> +
> + return EFI_SUCCESS;
> +}
I propose that:
- the VIRTIO_MMIO_DEVICE.Handle field be dropped,
- same for the dependent VIRTIO_MMIO_DEVICE_FROM_HANDLE() macro,
- the VirtioMmioUninstallDevice() function should precisely imitate the
DriverBindingStop() API:
- As input, take the handle that the protocol has been installed on.
Normally, this is a controller (device) handle, but under the use
pattern visible in patch #10, this is the empty driver's image
handle. Good, that can be used; the caller always has access to it.
- The VirtioMmioUninstallDevice() should look up the Virtio Device
protocol on that handle, using the GUID and the OpenProtocol() /
GET_PROTOCOL protocol service.
- The function should then use VIRTIO_MMIO_DEVICE_FROM_VIRTIO_DEVICE()
to get back to the mmio-specific container structure (checking the
signature as well), and proceed exactly as DriverBindingStop()
would.
> +
> +// Initialize VirtIo Device and Install VIRTIO_DEVICE_PROTOCOL protocol
> +EFI_STATUS
> +VirtioMmioInstallDevice (
> + IN PHYSICAL_ADDRESS BaseAddress,
> + IN EFI_HANDLE Handle
> + );
> +
> +EFI_STATUS
> +VirtioMmioUninstallDevice (
> + IN EFI_HANDLE Handle
> + );
> +
> +#endif // _VIRTIO_MMIO_DEVICE_LIB_H_
Still for this header file: since this header declares the library's
API, I think internals shouldn't be exposed. That is, all of the
following should be moved to
"OvmfPkg/Library/VirtioMmioDeviceLib/VirtioMmioDevice.h":
- VIRTIO_MMIO_DEVICE_SIGNATURE
- VIRTIO_MMIO_DEVICE
- VIRTIO_MMIO_DEVICE_FROM_VIRTIO_DEVICE
Only the declaration of public functions that are useful for client code
should remain.
> diff --git a/OvmfPkg/Library/VirtioMmioDeviceLib/VirtioMmioDevice.h
> b/OvmfPkg/Library/VirtioMmioDeviceLib/VirtioMmioDevice.h
> new file mode 100644
> index 0000000..66b3f76
> --- /dev/null
> +++ b/OvmfPkg/Library/VirtioMmioDeviceLib/VirtioMmioDevice.h
> @@ -0,0 +1,160 @@
> +/** @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.
> +
> +**/
> +
> +#ifndef _VIRTIO_MMIO_DEVICE_INTERNAL_H_
> +#define _VIRTIO_MMIO_DEVICE_INTERNAL_H_
> +
> +#include <Protocol/VirtioDevice.h>
> +
> +#include <IndustryStandard/Virtio.h>
> +
> +#include <Library/DebugLib.h>
> +#include <Library/IoLib.h>
> +#include <Library/UefiLib.h>
> +#include <Library/VirtioMmioDeviceLib.h>
> +
> +#define VIRTIO_MMIO_OFFSET_MAGIC 0x00
> +#define VIRTIO_MMIO_OFFSET_VERSION 0x04
> +#define VIRTIO_MMIO_OFFSET_DEVICE_ID 0x08
> +#define VIRTIO_MMIO_OFFSET_VENDOR_ID 0x0C
> +#define VIRTIO_MMIO_OFFSET_HOST_FEATURES 0x10
> +#define VIRTIO_MMIO_OFFSET_HOST_FEATURES_SEL 0x14
> +#define VIRTIO_MMIO_OFFSET_GUEST_FEATURES 0x20
> +#define VIRTIO_MMIO_OFFSET_GUEST_FEATURES_SEL 0x24
> +#define VIRTIO_MMIO_OFFSET_GUEST_PAGE_SIZE 0x28
> +#define VIRTIO_MMIO_OFFSET_QUEUE_SEL 0x30
> +#define VIRTIO_MMIO_OFFSET_QUEUE_NUM_MAX 0x34
> +#define VIRTIO_MMIO_OFFSET_QUEUE_NUM 0x38
> +#define VIRTIO_MMIO_OFFSET_QUEUE_ALIGN 0x3C
> +#define VIRTIO_MMIO_OFFSET_QUEUE_PAGE_OFF 0x40
(maybe call this one VIRTIO_MMIO_OFFSET_QUEUE_PFN?)
> +#define VIRTIO_MMIO_OFFSET_QUEUE_NOTIFY 0x50
> +#define VIRTIO_MMIO_OFFSET_INTERRUPT_STATUS 0x60
> +#define VIRTIO_MMIO_OFFSET_INTERRUPT_ACK 0x64
> +#define VIRTIO_MMIO_OFFSET_STATUS 0x70
> +#define VIRTIO_MMIO_OFFSET_DEVICE_SPECIFIC_CONFIG 0x100
These offset constants belong into
"OvmfPkg/Include/IndustryStandard/Virtio.h", near VIRTIO_HDR (which is
going to be removed in patch #7).
> +
> +#define VIRTIO_MMIO_MAGIC 0x74726976 // "virt"
Should go under IndustryStandard as well.
> +#define VIRTIO_MMIO_VENDOR_ID 0x1AF4
Same. And then, I think it should be renamed to VIRTIO_VENDOR_ID, and
reused by the PCI implementation.
> +
> +#define VIRTIO_CFG_WRITE(Device, Offset, Val) \
> + (MmioWrite32 (Device->BaseAddress + Offset, Val))
> +#define VIRTIO_CFG_READ(Device, Offset) \
> + (MmioRead32 (Device->BaseAddress + Offset))
Correct, these are library-specific.
Perhaps Offset should be parenthesized in the replacement text, so that
eg. the ternary operator ?: can be passed in ("+" binds more strongly).
> +
> +EFI_STATUS
> +EFIAPI
> +VirtioMmioDeviceRead (
> + IN VIRTIO_DEVICE_PROTOCOL *This,
> + IN UINTN FieldOFfset,
> + IN UINTN FieldSize,
> + IN UINTN BufferSize,
> + OUT VOID* Buffer
> + );
> +
> +EFI_STATUS
> +EFIAPI
> +VirtioMmioDeviceWrite (
> + IN VIRTIO_DEVICE_PROTOCOL *This,
> + IN UINTN FieldOffset,
> + IN UINTN FieldSize,
> + IN UINT64 Value
> + );
These seem to be exposed in the protocol. OK.
> +
> +UINT32
> +EFIAPI
> +VirtioMmioGetDeviceFeatures (
> + VIRTIO_DEVICE_PROTOCOL *This
> + );
My comment about getters suppressing return status applies. The
virtio-mmio implementation should always report EFI_SUCCESS, probably.
> +
> +UINT32
> +EFIAPI
> +VirtioMmioGetGuestFeatures (
> + VIRTIO_DEVICE_PROTOCOL *This
> + );
My comment about the GuestFeatures register (at
VIRTIO_MMIO_OFFSET_GUEST_FEATURES) being write-only applies; this
protocol member should be dropped altogether (seems to be quite useless
in general).
> +
> +UINT32
> +EFIAPI
> +VirtioMmioGetQueueAddress (
> + VIRTIO_DEVICE_PROTOCOL *This
> + );
> +
> +UINT32
> +EFIAPI
> +VirtioMmioGetQueueSel (
> + VIRTIO_DEVICE_PROTOCOL *This
> + );
> +
> +UINT16
> +EFIAPI
> +VirtioMmioGetQueueSize (
> + VIRTIO_DEVICE_PROTOCOL *This
> + );
> +
> +UINT8
> +EFIAPI
> +VirtioMmioGetDeviceStatus (
> + VIRTIO_DEVICE_PROTOCOL *This
> + );
> +
> +VOID
> +EFIAPI
> +VirtioMmioSetQueueSize (
> + VIRTIO_DEVICE_PROTOCOL *This,
> + UINT16 QueueSize
> + );
Just repeating my earlier comment: this setter should return a status
too.
> +
> +EFI_STATUS
> +EFIAPI
> +VirtioMmioSetDeviceStatus (
> + VIRTIO_DEVICE_PROTOCOL *This,
> + UINT8 DeviceStatus
> + );
> +
> +EFI_STATUS
> +EFIAPI
> +VirtioMmioSetQueueNotify (
> + VIRTIO_DEVICE_PROTOCOL *This,
> + UINT16 QueueNotify
> + );
> +
> +EFI_STATUS
> +EFIAPI
> +VirtioMmioSetQueueSel (
> + VIRTIO_DEVICE_PROTOCOL *This,
> + UINT16 Sel
> + );
> +
> +EFI_STATUS
> +VirtioMmioSetQueueAddress (
> + VIRTIO_DEVICE_PROTOCOL *This,
> + UINT32 Address
> + );
> +
> +EFI_STATUS
> +EFIAPI
> +VirtioMmioSetQueueAlignment (
> + VIRTIO_DEVICE_PROTOCOL *This,
> + UINT32 Alignment
> + );
> +
> +EFI_STATUS
> +EFIAPI
> +VirtioMmioSetGuestFeatures (
> + VIRTIO_DEVICE_PROTOCOL *This,
> + UINT32 Features
> + );
> +
> +#endif // _VIRTIO_MMIO_DEVICE_INTERNAL_H_
OK.
> diff --git a/OvmfPkg/Library/VirtioMmioDeviceLib/VirtioMmioDevice.c
> b/OvmfPkg/Library/VirtioMmioDeviceLib/VirtioMmioDevice.c
> new file mode 100644
> index 0000000..945da57
> --- /dev/null
> +++ b/OvmfPkg/Library/VirtioMmioDeviceLib/VirtioMmioDevice.c
> @@ -0,0 +1,201 @@
> +/** @file
> +
> + This driver produces Virtio Device Protocol instances for Virtio Mmio
> devices.
> +
> + 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 <Library/BaseMemoryLib.h>
> +#include <Library/MemoryAllocationLib.h>
> +#include <Library/UefiBootServicesTableLib.h>
> +
> +#include "VirtioMmioDevice.h"
> +
> +static VIRTIO_DEVICE_PROTOCOL mMmioDeviceProtocolTemplate = {
> + 0, // SubSystemDeviceId
> + VirtioMmioGetDeviceFeatures, // GetDeviceFeatures
> + VirtioMmioGetGuestFeatures, // GetGuestFeatures
> + VirtioMmioSetGuestFeatures, // SetGuestFeatures
> + VirtioMmioGetQueueAddress, // GetQueueAddress
> + VirtioMmioSetQueueAddress, // SetQueueAddress
> + VirtioMmioSetQueueSel, // SetQueueSel
> + VirtioMmioSetQueueNotify, // SetQueueNotify
> + VirtioMmioSetQueueAlignment, // SetQueueAlign
> + VirtioMmioGetQueueSize, // GetQueueNumMax
> + VirtioMmioSetQueueSize, // SetQueueNum
> + VirtioMmioGetDeviceStatus, // GetDeviceStatus
> + VirtioMmioSetDeviceStatus, // SetDeviceStatus
> + VirtioMmioDeviceWrite, // WriteDevice
> + VirtioMmioDeviceRead // ReadDevice
> +};
> +
> +/**
> +
> + Initialize the VirtIo PCI Device
s/PCI/MMIO/
> +
> + @param[in out] Dev The driver instance to configure. The caller is
> + responsible for Device->PciIo's validity (ie. working
> IO
> + access to the underlying virtio-blk PCI device).
- The caller is responsible for ensuring that Device->BaseAddress has
been initialized.
- [in,out] (again, apologies for that!)
- The parameter is called "Device".
> +
> + @retval EFI_SUCCESS Setup complete.
> +
> + @retval EFI_UNSUPPORTED The driver is unable to work with the virtio ring
> or
> + virtio-blk attributes the host provides.
The description of the retval should be updated (magic check etc.)
> +
> + @return Error codes from VirtioRingInit() or
> + VIRTIO_CFG_READ() / VIRTIO_CFG_WRITE().
VirtioRingInit() is the business of the dependent device driver, we
shouldn't mention it here.
> +
> +**/
> +STATIC
> +EFI_STATUS
> +EFIAPI
> +VirtioMmioInit (
> + IN OUT VIRTIO_MMIO_DEVICE *Device
> + )
> +{
> + // Set Mmio Page Size
> +
> + UINT8 NextDevStat;
> + UINT32 MagicValue;
> + UINT32 Version;
> +
> + //
> + // Execute virtio-0.9.5, 2.2.1 Device Initialization Sequence.
> + //
> + NextDevStat = 0; // step 1 -- reset device
> + VIRTIO_CFG_WRITE (Device, VIRTIO_MMIO_OFFSET_STATUS, NextDevStat);
I agree that within this protocol implementation, we can ignore the
retval check. I tried to follow what MmioWrite32() does -- it has a
bunch of platfom-dependent implementations, but the value it is the
value it takes as input; hence it should always succeed.
The implementation in "IntelFrameworkPkg/Library/DxeIoLibCpuIo/IoLib.c"
(which is I guess never used on ARM, but I'm only verifying the
interface contract) calls MmioWriteWorker() in the same file. That
function *does* call mCpuIo->Mem.Write(), which returns EFI_STATUS, but
MmioWriteWorker() asserts that it won't fail. Good.
> +
> + NextDevStat |= VSTAT_ACK; // step 2 -- acknowledge device presence
> + VIRTIO_CFG_WRITE (Device, VIRTIO_MMIO_OFFSET_STATUS, NextDevStat);
> +
> + NextDevStat |= VSTAT_DRIVER; // step 3 -- we know how to drive it
> + VIRTIO_CFG_WRITE (Device, VIRTIO_MMIO_OFFSET_STATUS, NextDevStat);
> +
> + // 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;
> + }
> +
> + // Set page size (for later queue allocations)
> + VIRTIO_CFG_WRITE (Device, VIRTIO_MMIO_OFFSET_GUEST_PAGE_SIZE,
> EFI_PAGE_SIZE);
> +
> + return EFI_SUCCESS;
> +}
I'm not familiar with virtio-mmio (and have been looking at that part of
the spec only in the last few days), so, questions/remarks:
(1) I think we shouldn't write anything before checking for the magic
value (r/o operation) and the version number (r/o operation). We
should first write once we're past these sanity checks. Otherwise,
in case of a misconfigured Address (which is coming from the library
client), we could be trampling over a random mmio register.
(2) Do we need to bring up the device at all (ie. massage its status
register)? I think the Virtio Subsystem Device ID (the register at
0x008) is available immediately. I believe logic even *dictates*
that it be available first -- without knowing the subsystem ID, a
driver is not expected to decide whether it can drive the device or
not.
The fact that the four read-only registers: MagicValue, Version,
DeviceID, VendorID are the very first ones in the MMIO space suggest
that we should be looking at them right off the bat.
(3) The VendorID register is not checked here, against
VIRTIO_MMIO_VENDOR_ID (0x1AF4), and it is also not exposed in the
protocol interface (which only saves the DeviceID). In effect I
believe the VendorID value is never checked on virtio-mmio.
For virtio-pci, the VirtioPciDeviceBindingSupported() function
covers the VendorID check, so I think it should be done here as well
(and it doesn't have to be exposed in the protocol).
(4) Regarding the guest page size, quoting the spec:
Device driver must write the guest page size in bytes to the
register during initialization, before any queues are used.
[...]
The device initialization is performed as described in 2.2.1
with one exception: the Guest must notify the Host about its
page size, writing the size in bytes to GuestPageSize register
before the initialization is finished.
Looking at the seven steps in 2.2.1 Device Initialization Sequence,
I agree that the relative order around setting the guest page size
should be:
- set VSTAT_DRIVER,
- [...]
- set guest page size,
- [...]
- allocate virtqueues,
- [...]
- set VSTAT_DRIVER_OK
The function above performs that correctly (it does the the first
two steps, and the rest is left to the device driver).
However: the virtio block/scsi/net drivers all *reset* the device as
part of their initialization. This is true before the series, and
patch #6 keeps it intact as well (correctly). See the three
occurrences of the comment
// step 1 -- reset device
in patch #6 (VirtioNet even resets it twice, see
VirtioNetGetFeatures()).
Resetting the device gives the host free hand to forget *all*
configuration about it. So, we must reconfigure the page size each
time we're going through the above sequence.
We can solve this in two ways:
(4a) turn the page-size-setting into a new member of the protocol.
All VSTAT_DRIVER...VSTAT_DRIVER_OK sequences must be located in
the current drivers, and extended with a call to the new
member.
(4b) internally to virtio-mmio, piggy-back this setting on another
operation, preferably SetQueueAddress().
The patchset has already chosen (4a) for the virtio-mmio-only
SetQueueAlignment() and SetQueueNum() members, so we should probably
follow suit here too.
The new member should do the following:
backend take page size return EFI_UNSUPPORTED configure
parameter if the value is not host side
EFI_PAGE_SIZE
----------- -------------- ---------------------- ---------
virtio-pci yes yes no
virtio-mmio yes yes yes
EFI_PAGE_SIZE is hardcoded in VirtioRingInit()
[OvmfPkg/Library/VirtioLib/VirtioLib.c], so any relaxation to the
above EFI_UNSUPPORTED case would have to be synched with
VirtioRingInit().
Regardless of the specific choice between (4a) and (4b):
VirtioMmioInit() can (and should) be implemented without *any* writes to
the device. (See point (2) again.)
> +
> +
> +/**
> +
> + Uninitialize the internals of a virtio-blk device that has been
> successfully
> + set up with VirtioMmioInit().
s/virtio-blk/virtio-mmio/
> +
> + @param[in out] Dev The device to clean up.
> +
> +**/
[in,out], and Device.
> +
> +STATIC
> +VOID
> +EFIAPI
> +VirtioMmioUninit (
> + IN VIRTIO_MMIO_DEVICE *Device
> + )
> +{
> + //
> + // Reset the virtual device -- see virtio-0.9.5, 2.2.2.1 Device Status.
> When
> + // VIRTIO_CFG_WRITE() returns, the host will have learned to stay away from
> + // the old comms area.
> + //
> + VIRTIO_CFG_WRITE (Device, VIRTIO_MMIO_OFFSET_STATUS, 0);
> +}
Same as for virtio-pci, and according to my point (2) above, this
function should exist, but do nothing.
> +
> +EFI_STATUS
> +VirtioMmioInstallDevice (
> + IN PHYSICAL_ADDRESS BaseAddress,
> + IN EFI_HANDLE Handle
> + )
The function lacks the leading comment -- it should have (a short) one,
considering this is the main function of the library.
> +{
> + 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->BaseAddress = BaseAddress;
> + VirtIo->Signature = VIRTIO_MMIO_DEVICE_SIGNATURE;
> + VirtIo->Handle = Handle;
The Handle assignment should be dropped (see at the top).
Plus, I'm tempted to say that the BaseAddress assignment should happen
in VirtioMmioInit(). The signature check is OK here, that's tied
directly to the protocol stuff.
> +
> + // Initialize VirtIo Mmio Device
> + CopyMem (&VirtIo->VirtioDevice, &mMmioDeviceProtocolTemplate,
> + sizeof (VIRTIO_DEVICE_PROTOCOL));
This should happen in VirtioMmioInit().
> + VirtIo->VirtioDevice.SubSystemDeviceId =
> + MmioRead32 (BaseAddress + VIRTIO_MMIO_OFFSET_DEVICE_ID);
This too, probably after the (currently missing) VendorID check there.
> + Status = VirtioMmioInit (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 UninstallVirtio;
> + }
I recommend to rename the label to UninitVirtio.
> +
> + return EFI_SUCCESS;
> +
> +UninstallVirtio:
> + VirtioMmioUninit (VirtIo);
> +
> +FreeVirtioMem:
> + FreePool (VirtIo);
> + return Status;
> +}
> +
OK.
I will continue the review of this patch from here.
I very much hope that I haven't come through as snarky or whatever --
that's certainly not my intent. My points are just food for thought.
This is huge work and I realize reworking such a series is big pain.
I'll have to see the rest, but right now I feel that I'd be OK with a
plan where
- you'd fix up the smaller stuff for the initial commit, and
- either of us would address the bigger stuff in a followup series.
(I'd be willing to do that; for me it's easier to write code than to
review code, and in that case you and Jordan would have to do the review
:) )
It is 100% your decision of course.
Thanks!
Laszlo
> diff --git a/OvmfPkg/Library/VirtioMmioDeviceLib/VirtioMmioDeviceFunctions.c
> b/OvmfPkg/Library/VirtioMmioDeviceLib/VirtioMmioDeviceFunctions.c
> new file mode 100644
> index 0000000..284c25a
> --- /dev/null
> +++ b/OvmfPkg/Library/VirtioMmioDeviceLib/VirtioMmioDeviceFunctions.c
> @@ -0,0 +1,297 @@
> +/** @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"
> +
> +UINT32
> +EFIAPI
> +VirtioMmioGetDeviceFeatures (
> + VIRTIO_DEVICE_PROTOCOL *This
> + )
> +{
> + VIRTIO_MMIO_DEVICE *Device;
> +
> + Device = VIRTIO_MMIO_DEVICE_FROM_VIRTIO_DEVICE (This);
> +
> + return VIRTIO_CFG_READ (Device, VIRTIO_MMIO_OFFSET_HOST_FEATURES);
> +}
> +
> +UINT32
> +EFIAPI
> +VirtioMmioGetGuestFeatures (
> + VIRTIO_DEVICE_PROTOCOL *This
> + )
> +{
> + VIRTIO_MMIO_DEVICE *Device;
> +
> + Device = VIRTIO_MMIO_DEVICE_FROM_VIRTIO_DEVICE (This);
> +
> + return VIRTIO_CFG_READ (Device, VIRTIO_MMIO_OFFSET_GUEST_FEATURES);
> +}
> +
> +UINT32
> +EFIAPI
> +VirtioMmioGetQueueAddress (
> + VIRTIO_DEVICE_PROTOCOL *This
> + )
> +{
> + VIRTIO_MMIO_DEVICE *Device;
> +
> + Device = VIRTIO_MMIO_DEVICE_FROM_VIRTIO_DEVICE (This);
> +
> + return VIRTIO_CFG_READ (Device, VIRTIO_MMIO_OFFSET_QUEUE_PAGE_OFF);
> +}
> +
> +UINT32
> +EFIAPI
> +VirtioMmioGetQueueSel (
> + VIRTIO_DEVICE_PROTOCOL *This
> + )
> +{
> + VIRTIO_MMIO_DEVICE *Device;
> +
> + Device = VIRTIO_MMIO_DEVICE_FROM_VIRTIO_DEVICE (This);
> +
> + return VIRTIO_CFG_READ (Device, VIRTIO_MMIO_OFFSET_QUEUE_SEL);
> +}
> +
> +UINT16
> +EFIAPI
> +VirtioMmioGetQueueSize (
> + VIRTIO_DEVICE_PROTOCOL *This
> + )
> +{
> + VIRTIO_MMIO_DEVICE *Device;
> +
> + Device = VIRTIO_MMIO_DEVICE_FROM_VIRTIO_DEVICE (This);
> +
> + return VIRTIO_CFG_READ (Device, VIRTIO_MMIO_OFFSET_QUEUE_NUM_MAX) & 0xFFFF;
> +}
> +
> +UINT8
> +EFIAPI
> +VirtioMmioGetDeviceStatus (
> + VIRTIO_DEVICE_PROTOCOL *This
> + )
> +{
> + VIRTIO_MMIO_DEVICE *Device;
> +
> + Device = VIRTIO_MMIO_DEVICE_FROM_VIRTIO_DEVICE (This);
> +
> + return VIRTIO_CFG_READ (Device, VIRTIO_MMIO_OFFSET_STATUS) & 0xFF;
> +}
> +
> +
> +VOID
> +EFIAPI
> +VirtioMmioSetQueueSize (
> + VIRTIO_DEVICE_PROTOCOL *This,
> + UINT16 QueueSize
> + )
> +{
> + VIRTIO_MMIO_DEVICE *Device;
> +
> + Device = VIRTIO_MMIO_DEVICE_FROM_VIRTIO_DEVICE (This);
> +
> + VIRTIO_CFG_WRITE (Device, VIRTIO_MMIO_OFFSET_QUEUE_NUM, QueueSize);
> +}
> +
> +EFI_STATUS
> +EFIAPI
> +VirtioMmioSetDeviceStatus (
> + VIRTIO_DEVICE_PROTOCOL *This,
> + UINT8 DeviceStatus
> + )
> +{
> + VIRTIO_MMIO_DEVICE *Device;
> +
> + Device = VIRTIO_MMIO_DEVICE_FROM_VIRTIO_DEVICE (This);
> +
> + VIRTIO_CFG_WRITE (Device, VIRTIO_MMIO_OFFSET_STATUS, DeviceStatus);
> +
> + return EFI_SUCCESS;
> +}
> +
> +EFI_STATUS
> +EFIAPI
> +VirtioMmioSetQueueNotify (
> + VIRTIO_DEVICE_PROTOCOL *This,
> + UINT16 QueueNotify
> + )
> +{
> + VIRTIO_MMIO_DEVICE *Device;
> +
> + Device = VIRTIO_MMIO_DEVICE_FROM_VIRTIO_DEVICE (This);
> +
> + VIRTIO_CFG_WRITE (Device, VIRTIO_MMIO_OFFSET_QUEUE_NOTIFY, QueueNotify);
> +
> + return EFI_SUCCESS;
> +}
> +
> +EFI_STATUS
> +EFIAPI
> +VirtioMmioSetQueueAlignment (
> + VIRTIO_DEVICE_PROTOCOL *This,
> + UINT32 Alignment
> + )
> +{
> + VIRTIO_MMIO_DEVICE *Device;
> +
> + Device = VIRTIO_MMIO_DEVICE_FROM_VIRTIO_DEVICE (This);
> +
> + VIRTIO_CFG_WRITE (Device, VIRTIO_MMIO_OFFSET_QUEUE_ALIGN, Alignment);
> +
> + return EFI_SUCCESS;
> +}
> +
> +EFI_STATUS
> +EFIAPI
> +VirtioMmioSetQueueSel (
> + VIRTIO_DEVICE_PROTOCOL *This,
> + UINT16 Sel
> + )
> +{
> + VIRTIO_MMIO_DEVICE *Device;
> +
> + Device = VIRTIO_MMIO_DEVICE_FROM_VIRTIO_DEVICE (This);
> +
> + VIRTIO_CFG_WRITE (Device, VIRTIO_MMIO_OFFSET_QUEUE_SEL, Sel);
> +
> + return EFI_SUCCESS;
> +}
> +
> +EFI_STATUS
> +VirtioMmioSetQueueAddress (
> + VIRTIO_DEVICE_PROTOCOL *This,
> + UINT32 Address
> + )
> +{
> + VIRTIO_MMIO_DEVICE *Device;
> +
> + Device = VIRTIO_MMIO_DEVICE_FROM_VIRTIO_DEVICE (This);
> +
> + VIRTIO_CFG_WRITE (Device, VIRTIO_MMIO_OFFSET_QUEUE_PAGE_OFF, Address);
> +
> + return EFI_SUCCESS;
> +}
> +
> +EFI_STATUS
> +EFIAPI
> +VirtioMmioSetGuestFeatures (
> + VIRTIO_DEVICE_PROTOCOL *This,
> + UINT32 Features
> + )
> +{
> + VIRTIO_MMIO_DEVICE *Device;
> +
> + Device = VIRTIO_MMIO_DEVICE_FROM_VIRTIO_DEVICE (This);
> +
> + VIRTIO_CFG_WRITE (Device, VIRTIO_MMIO_OFFSET_GUEST_FEATURES, Features);
> +
> + return EFI_SUCCESS;
> +}
> +
> +EFI_STATUS
> +EFIAPI
> +VirtioMmioDeviceWrite (
> + IN VIRTIO_DEVICE_PROTOCOL *This,
> + IN UINTN FieldOffset,
> + IN UINTN FieldSize,
> + IN UINT64 Value
> + )
> +{
> + UINTN DstBaseAddress;
> + UINT8 *DstByteAddress;
> + UINT8 SrcByte;
> + UINTN Index;
> + VIRTIO_MMIO_DEVICE *Device;
> +
> + Device = VIRTIO_MMIO_DEVICE_FROM_VIRTIO_DEVICE (This);
> +
> + // Double-check fieldsize
> + if (FieldSize > 8) {
> + return EFI_INVALID_PARAMETER;
> + }
> +
> + if ((FieldSize % 2) && FieldSize != 1) {
> + return EFI_INVALID_PARAMETER;
> + }
> +
> + // Compute base address
> + DstBaseAddress = Device->BaseAddress +
> + VIRTIO_MMIO_OFFSET_DEVICE_SPECIFIC_CONFIG + FieldOffset;
> +
> + //
> + // The device-specific memory area of Virtio-MMIO can only be written in
> + // byte accesses. This is not currently in the Virtio spec.
> + //
> + for (Index = 0; Index < FieldSize; Index++) {
> + DstByteAddress = ((UINT8 *)DstBaseAddress) + Index;
> + SrcByte = *(((UINT8 *)&Value) + Index);
> + MmioWrite8 ((UINTN) DstByteAddress, SrcByte);
> + }
> +
> + return EFI_SUCCESS;
> +}
> +
> +EFI_STATUS
> +EFIAPI
> +VirtioMmioDeviceRead (
> + IN VIRTIO_DEVICE_PROTOCOL *This,
> + IN UINTN FieldOffset,
> + IN UINTN FieldSize,
> + IN UINTN BufferSize,
> + OUT VOID *Buffer
> + )
> +{
> + UINTN SrcBaseAddress;
> + UINT8 *SrcByteAddress;
> + UINTN Index;
> + VIRTIO_MMIO_DEVICE *Device;
> +
> + Device = VIRTIO_MMIO_DEVICE_FROM_VIRTIO_DEVICE (This);
> +
> + // Parameter validation
> + if (FieldSize != BufferSize) {
> + return EFI_INVALID_PARAMETER;
> + }
> +
> + // Double-check fieldsize
> + if (FieldSize > 8) {
> + return EFI_INVALID_PARAMETER;
> + }
> +
> + if ((FieldSize % 2) && FieldSize != 1) {
> + return EFI_INVALID_PARAMETER;
> + }
> +
> + // Compute base address
> + SrcBaseAddress = Device->BaseAddress +
> + VIRTIO_MMIO_OFFSET_DEVICE_SPECIFIC_CONFIG + FieldOffset;
> +
> + //
> + // The device-specific memory area of Virtio-MMIO can only be read in
> + // byte reads. This is not currently in the Virtio spec.
> + //
> + for (Index = 0; Index < FieldSize; Index++) {
> + SrcByteAddress = ((UINT8 *)SrcBaseAddress) + Index;
> + *(UINT8 *)(Buffer + Index) = MmioRead8 ((UINTN) SrcByteAddress);
> + }
> +
> + return EFI_SUCCESS;
> +}
> diff --git a/OvmfPkg/Library/VirtioMmioDeviceLib/VirtioMmioDeviceLib.inf
> b/OvmfPkg/Library/VirtioMmioDeviceLib/VirtioMmioDeviceLib.inf
> new file mode 100644
> index 0000000..e6deccb
> --- /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 = 0x00010005
> + 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
> + UefiLib
> +
> +[Protocols]
> + gVirtioDeviceProtocolGuid
>
------------------------------------------------------------------------------
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
