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/VirtioPciDeviceDxe/VirtioPciDevice.c | 652 > +++++++++++++++++++++ > OvmfPkg/VirtioPciDeviceDxe/VirtioPciDevice.h | 267 +++++++++ > OvmfPkg/VirtioPciDeviceDxe/VirtioPciDeviceDxe.inf | 43 ++ > OvmfPkg/VirtioPciDeviceDxe/VirtioPciFunctions.c | 288 +++++++++ > 4 files changed, 1250 insertions(+), 0 deletions(-) > create mode 100644 OvmfPkg/VirtioPciDeviceDxe/VirtioPciDevice.c > create mode 100644 OvmfPkg/VirtioPciDeviceDxe/VirtioPciDevice.h > create mode 100644 OvmfPkg/VirtioPciDeviceDxe/VirtioPciDeviceDxe.inf > create mode 100644 OvmfPkg/VirtioPciDeviceDxe/VirtioPciFunctions.c
I'll probably only mention things here that aren't covered by -- and don't follow from -- points (1) to (3) in my review for v3 03/10. > > diff --git a/OvmfPkg/VirtioPciDeviceDxe/VirtioPciDevice.c > b/OvmfPkg/VirtioPciDeviceDxe/VirtioPciDevice.c > new file mode 100644 > index 0000000..816506d > --- /dev/null > +++ b/OvmfPkg/VirtioPciDeviceDxe/VirtioPciDevice.c > @@ -0,0 +1,652 @@ > +/** @file > + > + This driver produces Virtio Device Protocol instances for Virtio PCI > 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 <IndustryStandard/Pci.h> > +#include <Library/BaseMemoryLib.h> > +#include <Library/DebugLib.h> > +#include <Library/MemoryAllocationLib.h> > +#include <Library/UefiBootServicesTableLib.h> > +#include <Library/UefiLib.h> > + > +#include "VirtioPciDevice.h" > + > +STATIC VIRTIO_DEVICE_PROTOCOL mDeviceProtocolTemplate = { > + 0, // SubSystemDeviceId > + VirtioPciGetDeviceFeatures, // GetDeviceFeatures > + VirtioPciGetGuestFeatures, // GetGuestFeatures > + VirtioPciSetGuestFeatures, // SetGuestFeatures > + VirtioPciGetQueueAddress, // GetQueueAddress > + VirtioPciSetQueueAddress, // SetQueueAddress > + VirtioPciSetQueueSel, // SetQueueSel > + VirtioPciSetQueueNotify, // SetQueueNotify > + VirtioPciSetQueueAlignment, // SetQueueAlignment > + VirtioPciGetQueueSize, // GetQueueNumMax > + VirtioPciSetQueueSize, // SetQueueNum > + VirtioPciGetDeviceStatus, // GetDeviceStatus > + VirtioPciSetDeviceStatus, // SetDeviceStatus > + VirtioPciDeviceWrite, // WriteDevice > + VirtioPciDeviceRead // ReadDevice > +}; > + > +/** > + > + Read a word from Region 0 of the device specified by PciIo. > + > + Region 0 must be an iomem region. This is an internal function for the > + driver-specific VIRTIO_CFG_READ() macros. I think we should rather say "this is an internal function for the PCI implementation of the protocol". The VIRTIO_CFG_READ() macros in the drivers, if there will be such, won't call this directly. > + > + @param[in] PciIo Source PCI device. Stale parameter name; it should be called Dev. > + > + @param[in] FieldOffset Source offset. > + > + @param[in] FieldSize Source field size, must be in { 1, 2, 4, 8 }. > + > + @param[in] BufferSize Number of bytes available in the target buffer. > Must > + equal FieldSize. > + > + @param[out] Buffer Target buffer. > + > + > + @return Status code returned by PciIo->Io.Read(). > + > +**/ > +EFI_STATUS > +EFIAPI > +VirtioPciIoRead ( > + IN VIRTIO_PCI_DEVICE *Dev, > + IN UINTN FieldOffset, > + IN UINTN FieldSize, > + IN UINTN BufferSize, > + OUT VOID *Buffer > + ) > +{ > + UINTN Count; > + EFI_PCI_IO_PROTOCOL_WIDTH Width; > + EFI_PCI_IO_PROTOCOL *PciIo; > + > + ASSERT (FieldSize == BufferSize); > + > + PciIo = Dev->PciIo; > + > + Count = 1; > + switch (FieldSize) { > + case 1: > + Width = EfiPciIoWidthUint8; > + break; > + > + case 2: > + Width = EfiPciIoWidthUint16; > + break; > + > + case 8: > + // The 64bit PCI I/O is broken down into two 32bit reads to prevent > + // any alignment or width issues. > + // The UEFI spec says under EFI_PCI_IO_PROTOCOL.Io.Write(): > + // > + // The I/O operations are carried out exactly as requested. The caller > + // is responsible for any alignment and I/O width issues which the > + // bus, device, platform, or type of I/O might require. For example on > + // some platforms, width requests of EfiPciIoWidthUint64 do not work*/ Runaway "*/" at EOL. > + Count = 2; > + // fall through > + > + case 4: > + Width = EfiPciIoWidthUint32; > + break; > + > + default: > + ASSERT (FALSE); > + return EFI_INVALID_PARAMETER; > + } > + > + return PciIo->Io.Read ( > + PciIo, > + Width, > + PCI_BAR_IDX0, > + FieldOffset, > + Count, > + Buffer > + ); > +} > + > +/** > + > + Write a word into Region 0 of the device specified by PciIo. > + > + @param[in] PciIo Target PCI device. Parameter is called "Dev". > + > + @param[in] FieldOffset Destination offset. > + > + @param[in] FieldSize Destination field size, must be in { 1, 2, 4, 8 }. > + > + @param[in] Value Little endian value to write, converted to UINT64. > + The least significant FieldSize bytes will be used. OK, so this is where the endianness question comes in: > + > + > + @return Status code returned by PciIo->Io.Write(). > + > +**/ > +EFI_STATUS > +EFIAPI > +VirtioPciIoWrite ( > + IN VIRTIO_PCI_DEVICE *Dev, > + IN UINTN FieldOffset, > + IN UINTN FieldSize, > + IN UINT64 Value > + ) > +{ > + UINTN Count; > + EFI_PCI_IO_PROTOCOL_WIDTH Width; > + EFI_PCI_IO_PROTOCOL *PciIo; > + > + PciIo = Dev->PciIo; > + > + Count = 1; > + switch (FieldSize) { > + case 1: > + Width = EfiPciIoWidthUint8; > + break; > + > + case 2: > + Width = EfiPciIoWidthUint16; > + break; > + > + case 8: > + // The 64bit PCI I/O is broken down into two 32bit writes to prevent > + // any alignment or width issues. > + // The UEFI spec says under EFI_PCI_IO_PROTOCOL.Io.Write(): > + // > + // The I/O operations are carried out exactly as requested. The caller > + // is responsible for any alignment and I/O width issues which the > + // bus, device, platform, or type of I/O might require. For example on > + // some platforms, width requests of EfiPciIoWidthUint64 do not work*/ (Runaway "*/" at EOL.) > + Count = 2; > + // fall through > + > + case 4: > + Width = EfiPciIoWidthUint32; > + break; > + > + default: > + ASSERT (FALSE); > + return EFI_INVALID_PARAMETER; > + } > + > + return PciIo->Io.Write ( > + PciIo, > + Width, > + PCI_BAR_IDX0, > + FieldOffset, > + Count, > + &Value > + ); > +} The above matches the leading comment (ie. little-endian Value only). However, this function is used, via VIRTIO_CFG_WRITE(), from both VirtioPciDeviceWrite() and VirtioPciSetGuestFeatures(). The latter is indeed little-endian only (targets a field in the common virtio header), but the former has guest-native byte order. This effectively forces the WriteDevice protocol member to be little endian. I can't predict if virtio-pci is going to be used with a big endian platform. I think some layer should be endianness-aware. The outermost protocol members should likely all communicate in native endianness (ie. plain integer values). Then, - protocol implementations can write such values to the device-specific fields without byte-swapping, but the correct address into Value must be computed for big endian; - protocol implementations must correctly byte-swap Value to little-endian before writing it (always from its lowest address) into a common header field. This is of course all moot if we only support little endian. We can add big endian support later too, in another series; but then the main protocol header should state it's little-endian only. > + > +/** > + > + Device probe function for this driver. > + > + The DXE core calls this function for any given device in order to see if > the > + driver can drive the device. Some references present in OvmfPkg/VirtioBlkDxe/VirtioBlk.c are going lost here, like "Specs relevant in the general sense" etc, but I don't insist on keeping them. > + @param[in] This The EFI_DRIVER_BINDING_PROTOCOL object > + incorporating this driver (independently of > + any device). > + > + @param[in] DeviceHandle The device to probe. > + > + @param[in] RemainingDevicePath Relevant only for bus drivers, ignored. > + > + > + @retval EFI_SUCCESS The driver supports the device being probed. > + > + @retval EFI_UNSUPPORTED Based on virtio-blk PCI discovery, we do not > support > + the device. s/virtio-blk PCI/virtio-pci/ > + > + @return Error codes from the OpenProtocol() boot service > or > + the PciIo protocol. > + > +**/ > + > +EFI_STATUS > +EFIAPI > +VirtioPciDeviceBindingSupported ( > + IN EFI_DRIVER_BINDING_PROTOCOL *This, > + IN EFI_HANDLE DeviceHandle, > + IN EFI_DEVICE_PATH_PROTOCOL *RemainingDevicePath > + ) > +{ > + EFI_STATUS Status; > + EFI_PCI_IO_PROTOCOL *PciIo; > + PCI_TYPE00 Pci; > + > + // > + // Attempt to open the device with the PciIo set of interfaces. On success, > + // the protocol is "instantiated" for the PCI device. Covers duplicate open > + // attempts (EFI_ALREADY_STARTED). > + // > + Status = gBS->OpenProtocol ( > + DeviceHandle, // candidate device > + &gEfiPciIoProtocolGuid, // for generic PCI access > + (VOID **)&PciIo, // handle to instantiate > + This->DriverBindingHandle, // requestor driver identity > + DeviceHandle, // ControllerHandle, according > to > + // the UEFI Driver Model > + EFI_OPEN_PROTOCOL_BY_DRIVER // get exclusive PciIo access > to > + // the device; to be released > + ); > + if (EFI_ERROR (Status)) { > + return Status; > + } > + > + // > + // Read entire PCI configuration header for more extensive check ahead. > + // > + Status = PciIo->Pci.Read ( > + PciIo, // (protocol, device) > + // handle > + EfiPciIoWidthUint32, // access width & copy > + // mode > + 0, // Offset > + sizeof Pci / sizeof (UINT32), // Count > + &Pci // target buffer > + ); > + > + if (Status == EFI_SUCCESS) { > + // > + // virtio-0.9.5, 2.1 PCI Discovery > + // > + if ((Pci.Hdr.VendorId == 0x1AF4) && > + (Pci.Hdr.DeviceId >= 0x1000) && > + (Pci.Hdr.DeviceId <= 0x103F) && > + (Pci.Hdr.RevisionID == 0x00)) { > + Status = EFI_SUCCESS; > + } else { > + Status = EFI_UNSUPPORTED; > + } > + } > + > + // > + // We needed PCI IO access only transitorily, to see whether we support the > + // device or not. > + // > + gBS->CloseProtocol (DeviceHandle, &gEfiPciIoProtocolGuid, > + This->DriverBindingHandle, DeviceHandle); > + > + return Status; > +} OK. > + > +/** > + > + Initialize the VirtIo PCI Device > + > + @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 official format is [in,out] -- I must apologize for that, I've been using it inconsistently as well until Jordan educated me. I don't insist on fixing these though. Also, s/virtio-blk PCI device/PCI 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. This @retval should be dropped. > + > + @return Error codes from PciIo->Pci.Read(). > + > +**/ > +STATIC > +EFI_STATUS > +EFIAPI > +VirtioPciInit ( > + IN OUT VIRTIO_PCI_DEVICE *Device > + ) > +{ > + EFI_STATUS Status; > + EFI_PCI_IO_PROTOCOL *PciIo; > + PCI_TYPE00 Pci; > + > + ASSERT (Device != NULL); > + PciIo = Device->PciIo; > + ASSERT (PciIo != NULL); > + ASSERT (PciIo->Pci.Read != NULL); > + > + Status = PciIo->Pci.Read ( > + PciIo, // (protocol, device) > + // handle > + EfiPciIoWidthUint32, // access width & copy > + // mode > + 0, // Offset > + sizeof (Pci) / sizeof (UINT32), // Count > + &Pci // target buffer > + ); > + if (EFI_ERROR (Status)) { > + return Status; > + } > + Device->VirtioDevice.SubSystemDeviceId = Pci.Device.SubsystemID; (I guess this is also where we'd set the Revision field to something nonzero; for now the AllocateZeroPool() should cover that. OK.) > + > + // Note: We don't support the MSI-X capability. If we did, > + // the offset would become 24 after enabling MSI-X. > + Device->DeviceSpecificConfigurationOffset = 20; Apparently, DeviceSpecificConfigurationOffset is private to the PCI implementation of the protocol. In that case we might as well simply use a constant. But it doesn't hurt, and this comment is worth keeping. OK. > + > + return EFI_SUCCESS; > +} > + > +/** > + > + Uninitialize the internals of a virtio-blk device that has been > successfully > + set up with VirtioPciInit(). > + > + @param[in out] Dev The device to clean up. > + > +**/ > + > +STATIC > +VOID > +EFIAPI > +VirtioPciUninit ( > + IN OUT VIRTIO_PCI_DEVICE *Device > + ) > +{ > + // > + // Reset the virtual device -- see virtio-0.9.5, 2.2.2.1 Device Status. > When > + // VirtioPciSetDeviceStatus() returns, the host will have learned to stay > + // away from the old comms area. > + // > + VirtioPciSetDeviceStatus (&Device->VirtioDevice, 0); > +} I think *the body* of this function should be dropped entirely. The actions taken by this function should mirror VirtioPciInit(); release resources allocated in that function. However, VirtioPciInit() doesn't allocate any resources -- there's nothing to free here. Unlike the original VirtioBlkInit(), VirtioPciInit() above doesn't communicate with the device, so there's no reason to do it in VirtioPciUninit() either. Tearing down the virtio-ring & clearing the status etc. are the responsibility of the individual device driver. Note that the individual device driver instance (block, scsi, net) holds a reference to this protocol instance (just like we're holding a reference to PciIo, with EFI_OPEN_PROTOCOL_BY_DRIVER). Consequently, the VIRTIO_PCI_DEVICE instance cannot be stopped/removed/disconnected until the dependent block/scsi/net driver goes down. Resetting the device is part of *that* teardown. We only manage PCI in this protocol. Of course one could say it doesn't hurt, and if some dependent device driver fails to reset the virtio device before exiting, we're catching it here. I disagree with this notion. Responsibilities must be well separated. I do think the "shell" of this function should be preserved however: for symmetry, and in order to keep the control flow in BindingStart() intact, below. > + > +/** > + > + After we've pronounced support for a specific device in > + DriverBindingSupported(), we start managing said device (passed in by the > + Driver Exeuction Environment) with the following service. > + > + See DriverBindingSupported() for specification references. > + > + @param[in] This The EFI_DRIVER_BINDING_PROTOCOL object > + incorporating this driver (independently of > + any device). > + > + @param[in] DeviceHandle The supported device to drive. > + > + @param[in] RemainingDevicePath Relevant only for bus drivers, ignored. > + > + > + @retval EFI_SUCCESS Driver instance has been created and > + initialized for the virtio-blk PCI device, > it s/virtio-blk PCI/virtio-pci/ > + is now accessibla via EFI_BLOCK_IO_PROTOCOL. s/EFI_BLOCK_IO_PROTOCOL/VIRTIO_DEVICE_PROTOCOL/ > + > + @retval EFI_OUT_OF_RESOURCES Memory allocation failed. > + > + @return Error codes from the OpenProtocol() boot > + service, the PciIo protocol, VirtioBlkInit(), > + or the InstallProtocolInterface() boot > service. s/VirtioBlkInit/VirtioPciInit/ > + > +**/ > +EFI_STATUS > +EFIAPI > +VirtioPciDeviceBindingStart ( > + IN EFI_DRIVER_BINDING_PROTOCOL *This, > + IN EFI_HANDLE DeviceHandle, > + IN EFI_DEVICE_PATH_PROTOCOL *RemainingDevicePath > + ) > +{ > + VIRTIO_PCI_DEVICE *Device; > + EFI_STATUS Status; > + > + Device = (VIRTIO_PCI_DEVICE *) AllocateZeroPool (sizeof *Device); > + if (Device == NULL) { > + return EFI_OUT_OF_RESOURCES; > + } > + > + Status = gBS->OpenProtocol (DeviceHandle, &gEfiPciIoProtocolGuid, > + (VOID **)&Device->PciIo, This->DriverBindingHandle, > + DeviceHandle, EFI_OPEN_PROTOCOL_BY_DRIVER); > + if (EFI_ERROR (Status)) { > + goto FreeVirtioPci; > + } > + > + // > + // We must retain and ultimately restore the original PCI attributes of the > + // device. See Driver Writer's Guide for UEFI 2.3.1 v1.01, 18.3 PCI > drivers / > + // 18.3.2 Start() and Stop(). > + // > + // The third parameter ("Attributes", input) is ignored by the Get > operation. > + // The fourth parameter ("Result", output) is ignored by the Enable and Set > + // operations. > + // > + // For virtio-blk we only need IO space access. > + // s/virtio-blk/virtio-pci/ > + Status = Device->PciIo->Attributes (Device->PciIo, > + EfiPciIoAttributeOperationGet, 0, &Device->OriginalPciAttributes); > + if (EFI_ERROR (Status)) { > + goto ClosePciIo; > + } > + > + Status = Device->PciIo->Attributes (Device->PciIo, > + EfiPciIoAttributeOperationEnable, > + EFI_PCI_IO_ATTRIBUTE_IO, NULL); > + if (EFI_ERROR (Status)) { > + goto ClosePciIo; > + } > + > + // > + // PCI IO access granted, configure virtio-blk device. > + // s/configure virtio-blk device/configure protocol instance/ > + > + // Copy protocol template > + CopyMem (&Device->VirtioDevice, &mDeviceProtocolTemplate, > + sizeof (VIRTIO_DEVICE_PROTOCOL)); > + > + Status = VirtioPciInit (Device); > + if (EFI_ERROR (Status)) { > + goto RestorePciAttributes; > + } Can you move the CopyMem() into VirtioPciInit()? I don't insist of course but conceptually it would be cleaner. > + > + // > + // Setup complete, attempt to export the driver instance's BlockIo > interface. > + // s/BlockIo interface/VirtioDevice interface/ > + Device->Signature = VIRTIO_PCI_DEVICE_SIGNATURE; > + Status = gBS->InstallProtocolInterface (&DeviceHandle, > + &gVirtioDeviceProtocolGuid, EFI_NATIVE_INTERFACE, > + &Device->VirtioDevice); > + if (EFI_ERROR (Status)) { > + goto UninitDev; > + } Right; I think this illustrates my point about VirtioPciUninit() well. If InstallProtocolInterface() fails, there's no grounds for any virtio communication whatsoever, given that we haven't done any in VirtioPciInit() either. > + > + return EFI_SUCCESS; > + > +UninitDev: > + VirtioPciUninit (Device); > + > +RestorePciAttributes: > + Device->PciIo->Attributes (Device->PciIo, EfiPciIoAttributeOperationSet, > + Device->OriginalPciAttributes, NULL); > + > +ClosePciIo: > + gBS->CloseProtocol (DeviceHandle, &gEfiPciIoProtocolGuid, > + This->DriverBindingHandle, DeviceHandle); > + > +FreeVirtioPci: > + FreePool (Device); > + > + return Status; > +} OK. > + > +/** > + > + Stop driving the Virtio PCI device > + > + @param[in] This The EFI_DRIVER_BINDING_PROTOCOL object > + incorporating this driver (independently of > any > + device). > + > + @param[in] DeviceHandle Stop driving this device. > + > + @param[in] NumberOfChildren Since this function belongs to a device > driver > + only (as opposed to a bus driver), the caller > + environment sets NumberOfChildren to zero, > and > + we ignore it. > + > + @param[in] ChildHandleBuffer Ignored (corresponding to NumberOfChildren). > + Oops, I forgot to document the retvals in VirtioBlkDriverBindingStop() :) > +**/ > +EFI_STATUS > +EFIAPI > +VirtioPciDeviceBindingStop ( > + IN EFI_DRIVER_BINDING_PROTOCOL *This, > + IN EFI_HANDLE DeviceHandle, > + IN UINTN NumberOfChildren, > + IN EFI_HANDLE *ChildHandleBuffer > + ) > +{ > + EFI_STATUS Status; > + VIRTIO_DEVICE_PROTOCOL *VirtioDevice; > + VIRTIO_PCI_DEVICE *Device; > + > + Status = gBS->OpenProtocol ( > + DeviceHandle, // candidate device > + &gVirtioDeviceProtocolGuid, // retrieve the BlockIo > iface s/BlockIo/VirtioDevice/ in the comment; or just drop BlockIo if the new text wouldn't fit 79 columns. > + (VOID **)&VirtioDevice, // target pointer > + This->DriverBindingHandle, // requestor driver identity > + DeviceHandle, // requesting lookup for > dev. > + EFI_OPEN_PROTOCOL_GET_PROTOCOL // lookup only, no ref. > added > + ); > + if (EFI_ERROR (Status)) { > + return Status; > + } > + > + Device = VIRTIO_PCI_DEVICE_FROM_VIRTIO_DEVICE (VirtioDevice); > + > + // > + // Handle Stop() requests for in-use driver instances gracefully. > + // > + Status = gBS->UninstallProtocolInterface (DeviceHandle, > + &gVirtioDeviceProtocolGuid, &Device->VirtioDevice); > + if (EFI_ERROR (Status)) { > + return Status; > + } Right. This is where we're catching the dependent block/scsi/net/etc driver (disconnecting it gracefully, or if that fails, getting EFI_ACCESS_DENIED returned here). > + > + VirtioPciUninit (Device); Correct, but as discussed, VirtioPciUninit() should be doing nothing for now. > + > + Device->PciIo->Attributes (Device->PciIo, EfiPciIoAttributeOperationSet, > + Device->OriginalPciAttributes, NULL); > + > + Status = gBS->CloseProtocol (DeviceHandle, &gEfiPciIoProtocolGuid, > + This->DriverBindingHandle, DeviceHandle); > + > + FreePool (Device); > + > + return Status; > +} The retval propagation is in fact more careful here than in VirtioBlkDriverBindingStop(). This is the eternal problem with destructors. If CloseProtocol() succeeds (and it really should -- I should have asserted that in VirtioBlk); then both approaches are fine. What if it fails (which should be impossible)? VirtioBlkDriverBindingStop() leaks a PciIo reference, but convinces the caller (with EFI_SUCCESS) never to call it again. VirtioPciDeviceBindingStop() returns the error instead (which, again, should be impossible here). If the caller tries to call VirtioPciDeviceBindingStop() again, the very first OpenProtocol() invocation will fail -- always. The PciIo reference is leaked the same way. It's basically impossible to recover from partial destruction after some point. But it should never happen. OK. > + > + > +// > +// The static object that groups the Supported() (ie. probe), Start() and > +// Stop() functions of the driver together. Refer to UEFI Spec 2.3.1 + Errata > +// C, 10.1 EFI Driver Binding Protocol. > +// > +STATIC EFI_DRIVER_BINDING_PROTOCOL gDriverBinding = { > + &VirtioPciDeviceBindingSupported, > + &VirtioPciDeviceBindingStart, > + &VirtioPciDeviceBindingStop, > + 0x10, // Version, must be in [0x10 .. 0xFFFFFFEF] for IHV-developed drivers > + NULL, // ImageHandle, to be overwritten by > + // EfiLibInstallDriverBindingComponentName2() in > VirtioPciEntryPoint() > + NULL // DriverBindingHandle, ditto > +}; > + > + > +// > +// The purpose of the following scaffolding (EFI_COMPONENT_NAME_PROTOCOL and > +// EFI_COMPONENT_NAME2_PROTOCOL implementation) is to format the driver's > name > +// in English, for display on standard console devices. This is recommended > for > +// UEFI drivers that follow the UEFI Driver Model. Refer to the Driver > Writer's > +// Guide for UEFI 2.3.1 v1.01, 11 UEFI Driver and Controller Names. > +// > +STATIC > +EFI_UNICODE_STRING_TABLE mDriverNameTable[] = { > + { "eng;en", L"Virtio PCI Driver" }, > + { NULL, NULL } > +}; Seems to work fine: Shell> dh -p SimpleNetwork -d -v 99: UnknownDevice LoadFile PXEBaseCode MTFTPv4ServiceBinding DHCPv4ServiceBinding UDPv4ServiceBinding TCPv4ServiceBinding IPv4ServiceBinding IPv4Config ARPServiceBinding UnknownDevice ManagedNetworkServiceBinding EfiVlanConfigProtocolGuid DevicePath PciRoot(0x0)/Pci(0x3,0x0)/MAC(525400938342,0x1) SimpleNetwork Controller Name : Virtio Network Device Device Path : PciRoot(0x0)/Pci(0x3,0x0)/MAC(525400938342,0x1) Controller Type : BUS Configuration : NO Diagnostics : NO Managed by : Drv[67] : MNP Network Service Driver Drv[68] : VLAN Configuration Driver Drv[6B] : IP4 CONFIG Network Service Driver Parent Controllers : Parent[84] : Virtio Network Device Child Controllers : Child[9B] : MNP (MAC=52-54-00-93-83-42, ProtocolType=0x806, VlanId=0) Child[9C] : MNP (Not started) Child[9E] : MNP (MAC=52-54-00-93-83-42, ProtocolType=0x800, VlanId=0) Child[9A] : PciRoot(0x0)/Pci(0x3,0x0)/MAC(525400938342,0x1)/VenHw(D79DF6B0-EF44-43BD-9797-43E93BCF5FA8) Child[9D] : PciRoot(0x0)/Pci(0x3,0x0)/MAC(525400938342,0x1)/VenHw(D8944553-C4DD-41F4-9B30-E1397CFB267B) Shell> dh 84 -d -v 84: 3E7AE718 UnknownDevice PCIIO DevicePath PciRoot(0x0)/Pci(0x3,0x0) Controller Name : Virtio Network Device Device Path : PciRoot(0x0)/Pci(0x3,0x0) Controller Type : BUS Configuration : NO Diagnostics : NO Managed by : Drv[49] : Virtio PCI Driver ^^^^^^^^^^^^^^^^^ Drv[73] : Virtio Network Driver Parent Controllers : Parent[30] : PciRoot(0x0) Child Controllers : Child[99] : Virtio Network Device > + > +STATIC > +EFI_COMPONENT_NAME_PROTOCOL gComponentName; > + > +EFI_STATUS > +EFIAPI > +VirtioPciGetDriverName ( > + IN EFI_COMPONENT_NAME_PROTOCOL *This, > + IN CHAR8 *Language, > + OUT CHAR16 **DriverName > + ) > +{ > + return LookupUnicodeString2 ( > + Language, > + This->SupportedLanguages, > + mDriverNameTable, > + DriverName, > + (BOOLEAN)(This == &gComponentName) // Iso639Language > + ); > +} > + > +EFI_STATUS > +EFIAPI > +VirtioPciGetDeviceName ( > + IN EFI_COMPONENT_NAME_PROTOCOL *This, > + IN EFI_HANDLE DeviceHandle, > + IN EFI_HANDLE ChildHandle, > + IN CHAR8 *Language, > + OUT CHAR16 **ControllerName > + ) > +{ > + return EFI_UNSUPPORTED; > +} This is optional indeed, and clearly I didn't bother to implement it in VirtioBlk and VirtioScsi. I decided to support it in VirtioNetGetControllerName() [OvmfPkg/VirtioNetDxe/ComponentName.c], if an example can convince you to support it :) But, again, it's optional. > + > +STATIC > +EFI_COMPONENT_NAME_PROTOCOL gComponentName = { > + &VirtioPciGetDriverName, > + &VirtioPciGetDeviceName, > + "eng" // SupportedLanguages, ISO 639-2 language codes > +}; > + > +STATIC > +EFI_COMPONENT_NAME2_PROTOCOL gComponentName2 = { > + (EFI_COMPONENT_NAME2_GET_DRIVER_NAME) &VirtioPciGetDriverName, > + (EFI_COMPONENT_NAME2_GET_CONTROLLER_NAME) &VirtioPciGetDeviceName, > + "en" // SupportedLanguages, RFC 4646 language codes > +}; > + > + > +// > +// Entry point of this driver. > +// > +EFI_STATUS > +EFIAPI > +VirtioPciDeviceEntryPoint ( > + IN EFI_HANDLE ImageHandle, > + IN EFI_SYSTEM_TABLE *SystemTable > + ) > +{ > + return EfiLibInstallDriverBindingComponentName2 ( > + ImageHandle, > + SystemTable, > + &gDriverBinding, > + ImageHandle, > + &gComponentName, > + &gComponentName2 > + ); > +} OK. > diff --git a/OvmfPkg/VirtioPciDeviceDxe/VirtioPciDevice.h > b/OvmfPkg/VirtioPciDeviceDxe/VirtioPciDevice.h > new file mode 100644 > index 0000000..da5881f > --- /dev/null > +++ b/OvmfPkg/VirtioPciDeviceDxe/VirtioPciDevice.h > @@ -0,0 +1,267 @@ > +/** @file > + > + Internal definitions for the VirtIo PCI 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_PCI_DEVICE_DXE_H_ > +#define _VIRTIO_PCI_DEVICE_DXE_H_ > + > +#include <Protocol/ComponentName.h> > +#include <Protocol/DriverBinding.h> > +#include <Protocol/PciIo.h> > +#include <Protocol/VirtioDevice.h> > + > +#include <IndustryStandard/Virtio.h> > + > +#define OFFSET_OF_VNET(Field) OFFSET_OF (VIRTIO_NET_CONFIG, Field) > +#define SIZE_OF_VNET(Field) (sizeof ((VIRTIO_NET_CONFIG *) > 0)->Field) I think these don't belong here. > +#define VIRTIO_PCI_DEVICE_SIGNATURE SIGNATURE_32 ('V', 'P', 'C', 'I') OK. > + > +#define VIRTIO_CFG_WRITE(Dev, Offset, Size, Value) \ > + VirtioPciIoWrite (Dev, Offset, Size, (Value)) > +#define VIRTIO_CFG_READ(Dev, Offset, Size, Buf) \ > + VirtioPciIoRead (Dev, Offset, Size, Size, Buf) (a) It's a matter of taste, but since you use these macros only in the VirtioPciFunctions.c file, perhaps they should be defined there. (And the underlying functions could be moved too, but it's really tangential.) (b) The original idea behind VIRTIO_CFG_READ() was the following (from "OvmfPkg/VirtioBlkDxe/VirtioBlk.c"): #define VIRTIO_CFG_READ(Dev, Field, Pointer) (VirtioRead ( \ (Dev)->PciIo, \ OFFSET_OF_VBLK (Field), \ SIZE_OF_VBLK (Field), \ sizeof *(Pointer), \ (Pointer) \ )) This relied entirely on the packed structures with the Vhdr* fields. The point was to specify the header field and the recipient integer value, and size matching would be automatic. For example, from VirtioBlkInit(): Status = VIRTIO_CFG_READ (Dev, Generic.VhdrDeviceFeatureBits, &Features); I needed this because I was massaging the fields all the time, directly, and I wanted some safety. However, since you are going to - use VIRTIO_*_OFFSET values and sizeof()s *internally*, and - protect the driver implementor by hiding all the poking behind a protocol, I think it would be simpler to *drop* the FieldSize parameter of VirtioPciIoRead(). It buys you nothing, and it doesn't contribute to the (otherwise great) safety you're providing for the driver implementor. This should remove the mindless argument duplication in VIRTIO_CFG_READ() above. (c) You can even *drop* both of these macros. VIRTIO_CFG_WRITE() doesn't add any value. Nor does VIRTIO_CFG_READ(), after implementing (b). > + > +typedef struct { > + UINT32 Signature; > + VIRTIO_DEVICE_PROTOCOL VirtioDevice; > + EFI_PCI_IO_PROTOCOL *PciIo; > + UINT64 OriginalPciAttributes; > + UINT32 DeviceSpecificConfigurationOffset; > +} VIRTIO_PCI_DEVICE; > + > +#define VIRTIO_PCI_DEVICE_FROM_VIRTIO_DEVICE(Device) \ > + CR (Device, VIRTIO_PCI_DEVICE, VirtioDevice, VIRTIO_PCI_DEVICE_SIGNATURE) > + > + OK. > +/** > + > + Device probe function for this driver. > + > + The DXE core calls this function for any given device in order to see if > the > + driver can drive the device. > + > + @param[in] This The EFI_DRIVER_BINDING_PROTOCOL object > + incorporating this driver (independently of > + any device). > + > + @param[in] DeviceHandle The device to probe. > + > + @param[in] RemainingDevicePath Relevant only for bus drivers, ignored. > + > + > + @retval EFI_SUCCESS The driver supports the device being probed. > + > + @retval EFI_UNSUPPORTED Based on virtio-blk PCI discovery, we do not > support > + the device. > + > + @return Error codes from the OpenProtocol() boot service > or > + the PciIo protocol. > + > +**/ > + > +EFI_STATUS > +EFIAPI > +VirtioPciDeviceBindingSupported ( > + IN EFI_DRIVER_BINDING_PROTOCOL *This, > + IN EFI_HANDLE DeviceHandle, > + IN EFI_DEVICE_PATH_PROTOCOL *RemainingDevicePath > + ); I think it's not necessary to declare functions here that you only reference in precisely one .c file. (They could be STATIC even.) I'm aware that OvmfPkg/VirtioBlkDxe/VirtioBlk.h exports them all -- I *vagualy* recall that Jordan requested that, so maybe my point is invalid. In that case, you'll need to update each functions leading comment (that you choose to update, that is) in two places. For me that's been a major headache. [snip] > +EFI_STATUS > +EFIAPI > +VirtioPciIoRead ( > + IN VIRTIO_PCI_DEVICE *Dev, > + IN UINTN FieldOffset, > + IN UINTN FieldSize, > + IN UINTN BufferSize, > + OUT VOID *Buffer > + ); > + > +EFI_STATUS > +EFIAPI > +VirtioPciIoWrite ( > + IN VIRTIO_PCI_DEVICE *Dev, > + IN UINTN FieldOffset, > + IN UINTN FieldSize, > + IN UINT64 Value > + ); > + > +/******************************************** > + * PCI Functions for VIRTIO_DEVICE_PROTOCOL > + *******************************************/ > +EFI_STATUS > +EFIAPI > +VirtioPciDeviceRead ( > + IN VIRTIO_DEVICE_PROTOCOL *This, > + IN UINTN FieldOffset, > + IN UINTN FieldSize, > + IN UINTN BufferSize, > + OUT VOID *Buffer > + ); > + > +EFI_STATUS > +EFIAPI > +VirtioPciDeviceWrite ( > + IN VIRTIO_DEVICE_PROTOCOL *This, > + IN UINTN FieldOffset, > + IN UINTN FieldSize, > + IN UINT64 Value > + ); > + > +UINT32 > +EFIAPI > +VirtioPciGetDeviceFeatures ( > + VIRTIO_DEVICE_PROTOCOL *This > + ); > + > +UINT32 > +EFIAPI > +VirtioPciGetGuestFeatures ( > + VIRTIO_DEVICE_PROTOCOL *This > + ); > + > +UINT32 > +EFIAPI > +VirtioPciGetQueueAddress ( > + VIRTIO_DEVICE_PROTOCOL *This > + ); > + > +UINT16 > +EFIAPI > +VirtioPciGetQueueSize ( > + VIRTIO_DEVICE_PROTOCOL *This > + ); > + > +EFI_STATUS > +EFIAPI > +VirtioPciSetQueueAlignment ( > + VIRTIO_DEVICE_PROTOCOL *This, > + UINT32 Alignment > + ); > + > +UINT8 > +EFIAPI > +VirtioPciGetDeviceStatus ( > + VIRTIO_DEVICE_PROTOCOL *This > + ); > + > +EFI_STATUS > +EFIAPI > +VirtioPciSetGuestFeatures ( > + VIRTIO_DEVICE_PROTOCOL *This, > + UINT32 Features > + ); > + > +EFI_STATUS > +EFIAPI > +VirtioPciSetQueueAddress ( > + VIRTIO_DEVICE_PROTOCOL *This, > + UINT32 Address > + ); > + > +EFI_STATUS > +EFIAPI > +VirtioPciSetQueueSel ( > + VIRTIO_DEVICE_PROTOCOL *This, > + UINT16 Sel > + ); > + > +EFI_STATUS > +EFIAPI > +VirtioPciSetQueueNotify ( > + VIRTIO_DEVICE_PROTOCOL *This, > + UINT16 Index > + ); > + > +VOID > +EFIAPI > +VirtioPciSetQueueSize ( > + VIRTIO_DEVICE_PROTOCOL *This, > + UINT16 Size > + ); > + > +EFI_STATUS > +EFIAPI > +VirtioPciSetDeviceStatus ( > + VIRTIO_DEVICE_PROTOCOL *This, > + UINT8 DeviceStatus > + ); > + > +#endif // _VIRTIO_PCI_DEVICE_DXE_H_ I agree that it's best to keep the leading comment only in the .c file; only one place to update again and again and again... > diff --git a/OvmfPkg/VirtioPciDeviceDxe/VirtioPciDeviceDxe.inf > b/OvmfPkg/VirtioPciDeviceDxe/VirtioPciDeviceDxe.inf > new file mode 100644 > index 0000000..98c5226 > --- /dev/null > +++ b/OvmfPkg/VirtioPciDeviceDxe/VirtioPciDeviceDxe.inf > @@ -0,0 +1,43 @@ > +## @file > +# This driver produces the VirtIo Device Protocol instances for VirtIo PCI > +# 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 = VirtioPciDeviceDxe > + FILE_GUID = 83dd3b39-7caf-4fac-a542-e050b767e3a7 > + MODULE_TYPE = UEFI_DRIVER > + VERSION_STRING = 1.0 > + ENTRY_POINT = VirtioPciDeviceEntryPoint > + > +[Sources] > + VirtioPciDevice.c > + VirtioPciFunctions.c > + > +[Packages] > + MdePkg/MdePkg.dec > + OvmfPkg/OvmfPkg.dec > + > +[LibraryClasses] > + BaseMemoryLib > + DebugLib > + MemoryAllocationLib > + UefiBootServicesTableLib > + UefiDriverEntryPoint > + UefiLib > + > +[Protocols] > + gEfiPciIoProtocolGuid ## TO_START > + gVirtioDeviceProtocolGuid Maybe add ## BY_START after gVirtioDeviceProtocolGuid? It means that the driver produces protocol instances with that GUID. BY_START is documented under "3.11 [Protocols] Sections" in the INF spec. It's mostly a hint for the human reader I think. > diff --git a/OvmfPkg/VirtioPciDeviceDxe/VirtioPciFunctions.c > b/OvmfPkg/VirtioPciDeviceDxe/VirtioPciFunctions.c > new file mode 100644 > index 0000000..15c4348 > --- /dev/null > +++ b/OvmfPkg/VirtioPciDeviceDxe/VirtioPciFunctions.c > @@ -0,0 +1,288 @@ > +/** @file > + > + This driver produces Virtio Device Protocol instances for Virtio PCI > 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 <Library/BaseMemoryLib.h> > +#include <Library/DebugLib.h> > +#include <Library/MemoryAllocationLib.h> > +#include <Library/UefiBootServicesTableLib.h> > +#include <Library/UefiLib.h> > +#include "VirtioPciDevice.h" > + > +#define VIRTIO_FEATURE_BITS_OFFSET 0 > +#define VIRTIO_GUEST_FEATURE_BITS_OFFSET 4 > +#define VIRTIO_QUEUE_ADDRESS_OFFSET 8 > +#define VIRTIO_QUEUE_SIZE_OFFSET 12 > +#define VIRTIO_QUEUE_SELECT_OFFSET 14 > +#define VIRTIO_QUEUE_NOTIFY_OFFSET 16 > +#define VIRTIO_QUEUE_DEVICE_STATUS_OFFSET 18 > +#define VIRTIO_QUEUE_DEVICE_ISR_OFFSET 19 Seems precise, OK. > + > +/** > + > + Read a word from Region 0 of the device specified by PciIo. > + > + Region 0 must be an iomem region. This is an internal function for the > + driver-specific VIRTIO_CFG_READ() macros. I think this comment should instead explain that the function implements the ReadDevice protocol member. It is not an internal function in the same sense as VirtioPciIoRead() is. Also, it is not called by the macros. > + > + @param[in] PciIo Source PCI device. > + > + @param[in] FieldOffset Source offset. > + > + @param[in] FieldSize Source field size, must be in { 1, 2, 4, 8 }. > + > + @param[in] BufferSize Number of bytes available in the target buffer. > Must > + equal FieldSize. > + > + @param[out] Buffer Target buffer. > + > + > + @return Status code returned by PciIo->Io.Read(). > + > +**/ > +EFI_STATUS > +EFIAPI > +VirtioPciDeviceRead ( > + IN VIRTIO_DEVICE_PROTOCOL *This, > + IN UINTN FieldOffset, > + IN UINTN FieldSize, > + IN UINTN BufferSize, > + OUT VOID *Buffer > + ) > +{ > + VIRTIO_PCI_DEVICE *Dev; > + > + Dev = VIRTIO_PCI_DEVICE_FROM_VIRTIO_DEVICE (This); > + > + return VirtioPciIoRead (Dev, > + Dev->DeviceSpecificConfigurationOffset + FieldOffset, > + FieldSize, BufferSize, Buffer); > +} (d) Seems correct (device-specific region is native-endian). (e) Since this is a public interface (a protocol member), I do think we should keep both FieldSize and BufferSize in the prototype here, and actually *move* the ASSERT() from VirtioPciIoRead() to here. This is the boundary where the driver implementor needs safety. > + > +/** > + > + Write a word into Region 0 of the device specified by PciIo. > + > + @param[in] PciIo Target PCI device. This comment should be updated from PCI to Virtio. > + > + @param[in] FieldOffset Destination offset. > + > + @param[in] FieldSize Destination field size, must be in { 1, 2, 4, 8 }. > + > + @param[in] Value Little endian value to write, converted to UINT64. > + The least significant FieldSize bytes will be used. I think, according to my rant at the top about endiannes: (f) this function (= protocol member impl.) should always take a native endian Value, then... > + > + > + @return Status code returned by PciIo->Io.Write(). > + > +**/ > +EFI_STATUS > +EFIAPI > +VirtioPciDeviceWrite ( > + IN VIRTIO_DEVICE_PROTOCOL *This, > + IN UINTN FieldOffset, > + IN UINTN FieldSize, > + IN UINT64 Value > + ) > +{ > + VIRTIO_PCI_DEVICE *Dev; > + > + Dev = VIRTIO_PCI_DEVICE_FROM_VIRTIO_DEVICE (This); > + > + return VIRTIO_CFG_WRITE (Dev, > + Dev->DeviceSpecificConfigurationOffset + FieldOffset, FieldSize, > Value); > +} ... do the optional byte swapping here. ... Aaaargh. Huge facepalm here. I wanted to propose some code, and I searched the UEFI spec for a "canonical way" for determining endianness. This is what I found: 1.8.1 Data Structure Descriptions Supported processors are "little endian" machines. This distinction means that the low-order byte of a multibyte data item in memory is at the lowest address, while the high-order byte is at the highest address. Some supported 64-bit processors may be configured for both "little endian" and "big endian" operation. All implementations designed to conform to this specification use "little endian" operation. 2.3.5 AArch32 Platforms The core will be configured as follows (common across all processor architecture revisions): Little endian mode 2.3.5.3 Detailed Calling Convention Only little endian operation is supported. 2.3.6 AArch64 Platforms The core will be configured as follows: Little endian mode 2.3.6.4 Detailed Calling Convention Only little endian operation is supported. How embarrassing :) Scratch everything I said about making the code endianness-aware. (Comments, on the other hand, can apparently *never* emphasize little-endian enough! :) ) > + > +UINT32 > +EFIAPI > +VirtioPciGetDeviceFeatures ( > + VIRTIO_DEVICE_PROTOCOL *This > + ) > +{ > + VIRTIO_PCI_DEVICE *Dev; > + UINT32 FeatureBits; > + > + Dev = VIRTIO_PCI_DEVICE_FROM_VIRTIO_DEVICE (This); > + > + VIRTIO_CFG_READ (Dev, VIRTIO_FEATURE_BITS_OFFSET, sizeof (UINT32), > + &FeatureBits); > + > + return FeatureBits; > +} (g) My remark about the getters throwing away the return status applies. (h) In the vein of (c), the macro call should be replaced by a direct function call. > + > +UINT32 > +EFIAPI > +VirtioPciGetGuestFeatures ( > + VIRTIO_DEVICE_PROTOCOL *This > + ) > +{ > + VIRTIO_PCI_DEVICE *Dev; > + UINT32 FeatureBits; > + > + Dev = VIRTIO_PCI_DEVICE_FROM_VIRTIO_DEVICE (This); > + > + VIRTIO_CFG_READ (Dev, VIRTIO_GUEST_FEATURE_BITS_OFFSET, sizeof (UINT32), > + &FeatureBits); > + > + return FeatureBits; > +} As discussed before: this protocol member is undefined on virtio-mmio (write-only), so we should probably drop it. > + > +UINT32 > +EFIAPI > +VirtioPciGetQueueAddress ( > + VIRTIO_DEVICE_PROTOCOL *This > + ) > +{ > + VIRTIO_PCI_DEVICE *Dev; > + UINT32 QueueAddress; > + > + Dev = VIRTIO_PCI_DEVICE_FROM_VIRTIO_DEVICE (This); > + > + VIRTIO_CFG_READ (Dev, VIRTIO_QUEUE_ADDRESS_OFFSET, sizeof (UINT32), > + &QueueAddress); > + > + return QueueAddress; > +} > + > +UINT16 > +EFIAPI > +VirtioPciGetQueueSize ( > + VIRTIO_DEVICE_PROTOCOL *This > + ) > +{ > + VIRTIO_PCI_DEVICE *Dev; > + UINT16 QueueSize; > + > + Dev = VIRTIO_PCI_DEVICE_FROM_VIRTIO_DEVICE (This); > + > + VIRTIO_CFG_READ (Dev, VIRTIO_QUEUE_SIZE_OFFSET, sizeof (UINT16), > &QueueSize); > + return QueueSize; > +} > + > +UINT8 > +EFIAPI > +VirtioPciGetDeviceStatus ( > + VIRTIO_DEVICE_PROTOCOL *This > + ) > +{ > + VIRTIO_PCI_DEVICE *Dev; > + UINT8 DeviceStatus; > + > + Dev = VIRTIO_PCI_DEVICE_FROM_VIRTIO_DEVICE (This); > + > + VIRTIO_CFG_READ (Dev, VIRTIO_QUEUE_DEVICE_STATUS_OFFSET, sizeof (UINT8), > + &DeviceStatus); > + > + return DeviceStatus; > +} > + > +EFI_STATUS > +EFIAPI > +VirtioPciSetGuestFeatures ( > + VIRTIO_DEVICE_PROTOCOL *This, > + UINT32 Features > + ) > +{ > + VIRTIO_PCI_DEVICE *Dev; > + > + Dev = VIRTIO_PCI_DEVICE_FROM_VIRTIO_DEVICE (This); > + > + return VIRTIO_CFG_WRITE (Dev, VIRTIO_GUEST_FEATURE_BITS_OFFSET, > + sizeof (UINT32), Features); > +} > + > +EFI_STATUS > +EFIAPI > +VirtioPciSetQueueAddress ( > + VIRTIO_DEVICE_PROTOCOL *This, > + UINT32 Address > + ) > +{ > + VIRTIO_PCI_DEVICE *Dev; > + > + Dev = VIRTIO_PCI_DEVICE_FROM_VIRTIO_DEVICE (This); > + > + return VIRTIO_CFG_WRITE (Dev, VIRTIO_QUEUE_ADDRESS_OFFSET, sizeof (UINT32), > + Address); > +} > + > +EFI_STATUS > +EFIAPI > +VirtioPciSetQueueSel ( > + VIRTIO_DEVICE_PROTOCOL *This, > + UINT16 Sel > + ) > +{ > + VIRTIO_PCI_DEVICE *Dev; > + > + Dev = VIRTIO_PCI_DEVICE_FROM_VIRTIO_DEVICE (This); > + > + return VIRTIO_CFG_WRITE (Dev, VIRTIO_QUEUE_SELECT_OFFSET, sizeof (UINT16), > + Sel); > +} > + > +EFI_STATUS > +EFIAPI > +VirtioPciSetQueueAlignment ( > + VIRTIO_DEVICE_PROTOCOL *This, > + UINT32 Alignment > + ) > +{ > + return EFI_SUCCESS; > +} > + > +EFI_STATUS > +EFIAPI > +VirtioPciSetQueueNotify ( > + VIRTIO_DEVICE_PROTOCOL *This, > + UINT16 Index > + ) > +{ > + VIRTIO_PCI_DEVICE *Dev; > + > + Dev = VIRTIO_PCI_DEVICE_FROM_VIRTIO_DEVICE (This); > + > + return VIRTIO_CFG_WRITE (Dev, VIRTIO_QUEUE_NOTIFY_OFFSET, sizeof (UINT16), > + Index); > +} > + > +VOID > +EFIAPI > +VirtioPciSetQueueSize ( > + VIRTIO_DEVICE_PROTOCOL *This, > + UINT16 Size > + ) > +{ > + // > + // This function is only applicable in Virtio-MMIO. > + // (The QueueSize field is read-only in Virtio proper (PCI)) > + // > +} Hm. I missed that the return type of this protocol member was VOID. I agree that it can never fail on the current implementations. Not sure if we should promise that on the protocol level. > + > +EFI_STATUS > +EFIAPI > +VirtioPciSetDeviceStatus ( > + VIRTIO_DEVICE_PROTOCOL *This, > + UINT8 DeviceStatus > + ) > +{ > + VIRTIO_PCI_DEVICE *Dev; > + > + Dev = VIRTIO_PCI_DEVICE_FROM_VIRTIO_DEVICE (This); > + > + return VIRTIO_CFG_WRITE (Dev, VIRTIO_QUEUE_DEVICE_STATUS_OFFSET, > + sizeof (UINT8), DeviceStatus); > +} > I usually write up whatever crosses my mind during review. That doesn't mean everything should be addressed. Please decide what you deem important enough to fix -- I'd hate to block this great work forever. I don't think I've said too many important things here; whatever you choose to ignore or postpone I can always try to modify in a followup series. I'm not giving my R-b right now only because the changes I've asked for in patch #3 will affect this patch. 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
