comments below
On 10/16/13 19:29, Olivier Martin wrote:
> diff --git a/OvmfPkg/VirtioNetDxe/ComponentName.c
> b/OvmfPkg/VirtioNetDxe/ComponentName.c
> index a291405..2c96adb 100644
> --- a/OvmfPkg/VirtioNetDxe/ComponentName.c
> +++ b/OvmfPkg/VirtioNetDxe/ComponentName.c
> @@ -139,20 +139,20 @@ VirtioNetGetControllerName (
> }
>
> //
> - // confirm that the device is managed by this driver, using the PCI IO
> + // confirm that the device is managed by this driver, using the VirtIo
> // Protocol
> //
> Status = EfiTestManagedDevice (
> ControllerHandle,
> gVirtioNetDriverBinding.DriverBindingHandle,
> - &gEfiPciIoProtocolGuid
> + &gVirtioDeviceProtocolGuid
> );
> if (EFI_ERROR (Status)) {
> return Status;
> }
>
> //
> - // we don't give different names to the bus (= parent, = PCI) handle and
> the
> + // we don't give different names to the bus (= parent) handle and the
> // child (= MAC) handle
> //
> return LookupUnicodeString2 (
> diff --git a/OvmfPkg/VirtioNetDxe/DriverBinding.c
> b/OvmfPkg/VirtioNetDxe/DriverBinding.c
> index c9259ab..330f1f3 100644
> --- a/OvmfPkg/VirtioNetDxe/DriverBinding.c
> +++ b/OvmfPkg/VirtioNetDxe/DriverBinding.c
> @@ -15,7 +15,6 @@
>
> **/
>
> -#include <IndustryStandard/Pci.h>
> #include <Library/BaseMemoryLib.h>
> #include <Library/DevicePathLib.h>
> #include <Library/MemoryAllocationLib.h>
> @@ -50,7 +49,7 @@
> unused.
>
> @retval EFI_UNSUPPORTED The host doesn't supply a MAC address.
> - @return Status codes from Dev->PciIo->Io.Read(),
> + @return Status codes from Dev->VirtIo->Io.Read(),
I'd rather say "status codes from virtio protocol members" or some such.
Not important.
> VIRTIO_CFG_READ() and VIRTIO_CFG_WRITE().
> @retval EFI_SUCCESS Configuration values retrieved.
> */
> @@ -74,19 +73,19 @@ VirtioNetGetFeatures (
> // Initialization Sequence), but don't complete setting it up.
> //
> NextDevStat = 0; // step 1 -- reset device
> - Status = VIRTIO_CFG_WRITE (Dev, Generic.VhdrDeviceStatus, NextDevStat);
> + Status = Dev->VirtIo->SetDeviceStatus (Dev->VirtIo, NextDevStat);
> if (EFI_ERROR (Status)) {
> return Status;
> }
>
> NextDevStat |= VSTAT_ACK; // step 2 -- acknowledge device presence
> - Status = VIRTIO_CFG_WRITE (Dev, Generic.VhdrDeviceStatus, NextDevStat);
> + Status = Dev->VirtIo->SetDeviceStatus (Dev->VirtIo, NextDevStat);
> if (EFI_ERROR (Status)) {
> goto YieldDevice;
> }
>
> NextDevStat |= VSTAT_DRIVER; // step 3 -- we know how to drive it
> - Status = VIRTIO_CFG_WRITE (Dev, Generic.VhdrDeviceStatus, NextDevStat);
> + Status = Dev->VirtIo->SetDeviceStatus (Dev->VirtIo, NextDevStat);
> if (EFI_ERROR (Status)) {
> goto YieldDevice;
> }
> @@ -94,7 +93,7 @@ VirtioNetGetFeatures (
> //
> // step 4a -- retrieve and validate features
> //
> - Status = VIRTIO_CFG_READ (Dev, Generic.VhdrDeviceFeatureBits, &Features);
> + Status = Dev->VirtIo->GetDeviceFeatures (Dev->VirtIo, &Features);
> if (EFI_ERROR (Status)) {
> goto YieldDevice;
> }
> @@ -106,13 +105,12 @@ VirtioNetGetFeatures (
> Status = EFI_UNSUPPORTED;
> goto YieldDevice;
> }
> - Status = Dev->PciIo->Io.Read (Dev->PciIo, // PciIo
> - EfiPciIoWidthUint8, // Width
> - PCI_BAR_IDX0, // BarIndex
> - OFFSET_OF_VNET (VhdrMac), // Offset
> - SIZE_OF_VNET (VhdrMac), // Count
> - MacAddress // Buffer
> - );
> + Status = Dev->VirtIo->ReadDevice (Dev->VirtIo,
> + OFFSET_OF_VNET (Mac), // Offset
> + sizeof(UINT8), // FieldSize
> + SIZE_OF_VNET (Mac), // BufferSize
> + MacAddress // Buffer
> + );
(4) OK, so I guess this is the incentive for the "looping" (new in v4)
in VirtioPciIoRead(). However, this call will trigger the ASSERT() in
VirtioMmioDeviceRead().
I think I would like an open-coded loop here :), but we seem to disagree
on that. I don't insist though; the PCI variant will work and the
failure on MMIO will be prominent.
>
> if (EFI_ERROR (Status)) {
> goto YieldDevice;
> @@ -126,7 +124,7 @@ VirtioNetGetFeatures (
> }
> else {
> *MediaPresentSupported = TRUE;
> - Status = VIRTIO_CFG_READ (Dev, VhdrLinkStatus, &LinkStatus);
> + Status = VIRTIO_CFG_READ (Dev, LinkStatus, &LinkStatus);
> if (EFI_ERROR (Status)) {
> goto YieldDevice;
> }
> @@ -134,7 +132,7 @@ VirtioNetGetFeatures (
> }
>
> YieldDevice:
> - VIRTIO_CFG_WRITE (Dev, Generic.VhdrDeviceStatus,
> + Dev->VirtIo->SetDeviceStatus (Dev->VirtIo,
> EFI_ERROR (Status) ? VSTAT_FAILED : 0);
>
> return Status;
> @@ -207,9 +205,9 @@ VirtioNetSnpPopulate (
> Dev->Snp.Mode = &Dev->Snm;
>
> Dev->Snm.State = EfiSimpleNetworkStopped;
> - Dev->Snm.HwAddressSize = SIZE_OF_VNET (VhdrMac);
> - Dev->Snm.MediaHeaderSize = SIZE_OF_VNET (VhdrMac) + // dst MAC
> - SIZE_OF_VNET (VhdrMac) + // src MAC
> + Dev->Snm.HwAddressSize = SIZE_OF_VNET (Mac);
> + Dev->Snm.MediaHeaderSize = SIZE_OF_VNET (Mac) + // dst MAC
> + SIZE_OF_VNET (Mac) + // src MAC
> 2; // Ethertype
> Dev->Snm.MaxPacketSize = 1500;
> Dev->Snm.NvRamSize = 0;
> @@ -222,7 +220,7 @@ VirtioNetSnpPopulate (
> Dev->Snm.MacAddressChangeable = FALSE;
> Dev->Snm.MultipleTxSupported = TRUE;
>
> - ASSERT (SIZE_OF_VNET (VhdrMac) <= sizeof (EFI_MAC_ADDRESS));
> + ASSERT (SIZE_OF_VNET (Mac) <= sizeof (EFI_MAC_ADDRESS));
>
> Status = VirtioNetGetFeatures (Dev, &Dev->Snm.CurrentAddress,
> &Dev->Snm.MediaPresentSupported, &Dev->Snm.MediaPresent);
> @@ -230,8 +228,8 @@ VirtioNetSnpPopulate (
> goto CloseWaitForPacket;
> }
> CopyMem (&Dev->Snm.PermanentAddress, &Dev->Snm.CurrentAddress,
> - SIZE_OF_VNET (VhdrMac));
> - SetMem (&Dev->Snm.BroadcastAddress, SIZE_OF_VNET (VhdrMac), 0xFF);
> + SIZE_OF_VNET (Mac));
> + SetMem (&Dev->Snm.BroadcastAddress, SIZE_OF_VNET (Mac), 0xFF);
>
> //
> // VirtioNetExitBoot() is queued by ExitBootServices(); its purpose is to
OK
> @@ -348,31 +346,36 @@ VirtioNetDriverBindingSupported (
> )
> {
> EFI_STATUS Status;
> - EFI_PCI_IO_PROTOCOL *PciIo;
> - PCI_TYPE00 Pci;
> -
> - Status = gBS->OpenProtocol (DeviceHandle, &gEfiPciIoProtocolGuid,
> - (VOID **)&PciIo, This->DriverBindingHandle, DeviceHandle,
> - EFI_OPEN_PROTOCOL_BY_DRIVER);
> + VIRTIO_DEVICE_PROTOCOL *VirtIo;
> +
> + //
> + // Attempt to open the device with the VirtIo set of interfaces. On
> success,
> + // the protocol is "instantiated" for the VirtIo device. Covers duplicate
> open
> + // attempts (EFI_ALREADY_STARTED).
> + //
> + Status = gBS->OpenProtocol (
> + DeviceHandle, // candidate device
> + &gVirtioDeviceProtocolGuid, // for generic VirtIo access
> + (VOID **)&VirtIo, // handle to instantiate
> + This->DriverBindingHandle, // requestor driver identity
> + DeviceHandle, // ControllerHandle, according
> to
> + // the UEFI Driver Model
> + EFI_OPEN_PROTOCOL_BY_DRIVER // get exclusive VirtIo access
> to
> + // the device; to be released
> + );
> if (EFI_ERROR (Status)) {
> return Status;
> }
>
> - Status = PciIo->Pci.Read (PciIo, EfiPciIoWidthUint32, 0,
> - sizeof Pci / sizeof (UINT32), &Pci);
> + if (VirtIo->SubSystemDeviceId != VIRTIO_SUBSYSTEM_NETWORK_CARD) {
> + Status = EFI_UNSUPPORTED;
> + }
>
> //
> - // virtio-0.9.5, 2.1 PCI Discovery:
> - // the network device has Subsystem Device ID 1
> + // We needed VirtIo access only transitorily, to see whether we support the
> + // device or not.
> //
> - if (Status == EFI_SUCCESS) {
> - Status = (Pci.Hdr.VendorId == 0x1AF4 &&
> - Pci.Hdr.DeviceId >= 0x1000 && Pci.Hdr.DeviceId <= 0x103F &&
> - Pci.Hdr.RevisionID == 0x00 &&
> - Pci.Device.SubsystemID == VIRTIO_SUBSYSTEM_NETWORK_CARD) ?
> EFI_SUCCESS : EFI_UNSUPPORTED;
> - }
> -
> - gBS->CloseProtocol (DeviceHandle, &gEfiPciIoProtocolGuid,
> + gBS->CloseProtocol (DeviceHandle, &gVirtioDeviceProtocolGuid,
> This->DriverBindingHandle, DeviceHandle);
> return Status;
> }
The result looks identical to VirtioBlkDriverBindingSupported() --
modulo VIRTIO_SUBSYSTEM_NETWORK_CARD vs. VIRTIO_SUBSYSTEM_BLOCK_DEVICE),
so it's OK.
> @@ -438,7 +441,7 @@ VirtioNetDriverBindingStart (
> VNET_DEV *Dev;
> EFI_DEVICE_PATH_PROTOCOL *DevicePath;
> MAC_ADDR_DEVICE_PATH MacNode;
> - VOID *ChildPciIo;
> + VOID *ChildVirtIo;
>
> //
> // allocate space for the driver instance
> @@ -449,30 +452,15 @@ VirtioNetDriverBindingStart (
> }
> Dev->Signature = VNET_SIG;
>
> - //
> - // get PCI access to the device and keep it open
> - //
> - Status = gBS->OpenProtocol (DeviceHandle, &gEfiPciIoProtocolGuid,
> - (VOID **)&Dev->PciIo, This->DriverBindingHandle,
> + Status = gBS->OpenProtocol (DeviceHandle, &gVirtioDeviceProtocolGuid,
> + (VOID **)&Dev->VirtIo, This->DriverBindingHandle,
> DeviceHandle, EFI_OPEN_PROTOCOL_BY_DRIVER);
> if (EFI_ERROR (Status)) {
> goto FreeVirtioNet;
> }
>
> - //
> - // save original PCI attributes and enable IO space access
> - //
> - Status = Dev->PciIo->Attributes (Dev->PciIo, EfiPciIoAttributeOperationGet,
> - 0, &Dev->OrigPciAttributes);
> - if (EFI_ERROR (Status)) {
> - goto ClosePciIo;
> - }
> -
> - Status = Dev->PciIo->Attributes (Dev->PciIo,
> - EfiPciIoAttributeOperationEnable,
> - EFI_PCI_IO_ATTRIBUTE_IO, NULL);
> - if (EFI_ERROR (Status)) {
> - goto ClosePciIo;
> + if (Dev->VirtIo->SubSystemDeviceId != VIRTIO_SUBSYSTEM_NETWORK_CARD) {
> + return EFI_UNSUPPORTED;
> }
(5) Same remark as (3) for VirtioBlkDriverBindingStart: the mismatch
handling is wrong, plus the check seems redundant in the first place.
>
> //
> @@ -481,11 +469,11 @@ VirtioNetDriverBindingStart (
> //
> Status = VirtioNetSnpPopulate (Dev);
> if (EFI_ERROR (Status)) {
> - goto RestorePciAttributes;
> + goto CloseVirtIo;
> }
>
> //
> - // get the device path of the virtio-net PCI device -- one-shot open
> + // get the device path of the virtio-net device -- one-shot open
> //
> Status = gBS->OpenProtocol (DeviceHandle, &gEfiDevicePathProtocolGuid,
> (VOID **)&DevicePath, This->DriverBindingHandle,
> @@ -523,11 +511,11 @@ VirtioNetDriverBindingStart (
> }
>
> //
> - // make a note that we keep this device open with PciIo for the sake of
> this
> + // make a note that we keep this device open with VirtIo for the sake of
> this
> // child
> //
> - Status = gBS->OpenProtocol (DeviceHandle, &gEfiPciIoProtocolGuid,
> - &ChildPciIo, This->DriverBindingHandle,
> + Status = gBS->OpenProtocol (DeviceHandle, &gVirtioDeviceProtocolGuid,
> + &ChildVirtIo, This->DriverBindingHandle,
> Dev->MacHandle, EFI_OPEN_PROTOCOL_BY_CHILD_CONTROLLER);
> if (EFI_ERROR (Status)) {
> goto UninstallMultiple;
> @@ -547,12 +535,8 @@ FreeMacDevicePath:
> Evacuate:
> VirtioNetSnpEvacuate (Dev);
>
> -RestorePciAttributes:
> - Dev->PciIo->Attributes (Dev->PciIo, EfiPciIoAttributeOperationSet,
> - Dev->OrigPciAttributes, NULL);
> -
> -ClosePciIo:
> - gBS->CloseProtocol (DeviceHandle, &gEfiPciIoProtocolGuid,
> +CloseVirtIo:
> + gBS->CloseProtocol (DeviceHandle, &gVirtioDeviceProtocolGuid,
> This->DriverBindingHandle, DeviceHandle);
>
> FreeVirtioNet:
OK.
> @@ -637,7 +621,7 @@ VirtioNetDriverBindingStop (
> Status = EFI_DEVICE_ERROR;
> }
> else {
> - gBS->CloseProtocol (DeviceHandle, &gEfiPciIoProtocolGuid,
> + gBS->CloseProtocol (DeviceHandle, &gVirtioDeviceProtocolGuid,
> This->DriverBindingHandle, Dev->MacHandle);
> gBS->UninstallMultipleProtocolInterfaces (Dev->MacHandle,
> &gEfiDevicePathProtocolGuid, Dev->MacDevicePath,
> @@ -645,8 +629,6 @@ VirtioNetDriverBindingStop (
> NULL);
> FreePool (Dev->MacDevicePath);
> VirtioNetSnpEvacuate (Dev);
> - Dev->PciIo->Attributes (Dev->PciIo, EfiPciIoAttributeOperationSet,
> - Dev->OrigPciAttributes, NULL);
> FreePool (Dev);
> }
>
> @@ -657,7 +639,7 @@ VirtioNetDriverBindingStop (
> //
> // release remaining resources, tied directly to the parent handle
> //
> - gBS->CloseProtocol (DeviceHandle, &gEfiPciIoProtocolGuid,
> + gBS->CloseProtocol (DeviceHandle, &gVirtioDeviceProtocolGuid,
> This->DriverBindingHandle, DeviceHandle);
>
> return EFI_SUCCESS;
OK
> diff --git a/OvmfPkg/VirtioNetDxe/Events.c b/OvmfPkg/VirtioNetDxe/Events.c
> index b9d7bbf..5be1af6 100644
> --- a/OvmfPkg/VirtioNetDxe/Events.c
> +++ b/OvmfPkg/VirtioNetDxe/Events.c
> @@ -86,6 +86,6 @@ VirtioNetExitBoot (
>
> Dev = Context;
> if (Dev->Snm.State == EfiSimpleNetworkInitialized) {
> - VIRTIO_CFG_WRITE (Dev, Generic.VhdrDeviceStatus, 0);
> + Dev->VirtIo->SetDeviceStatus (Dev->VirtIo, 0);
> }
> }
OK
> diff --git a/OvmfPkg/VirtioNetDxe/SnpGetStatus.c
> b/OvmfPkg/VirtioNetDxe/SnpGetStatus.c
> index adb57cf..4393d24 100644
> --- a/OvmfPkg/VirtioNetDxe/SnpGetStatus.c
> +++ b/OvmfPkg/VirtioNetDxe/SnpGetStatus.c
> @@ -90,7 +90,7 @@ VirtioNetGetStatus (
> if (Dev->Snm.MediaPresentSupported) {
> UINT16 LinkStatus;
>
> - Status = VIRTIO_CFG_READ (Dev, VhdrLinkStatus, &LinkStatus);
> + Status = VIRTIO_CFG_READ (Dev, LinkStatus, &LinkStatus);
> if (EFI_ERROR (Status)) {
> goto Exit;
> }
OK
> diff --git a/OvmfPkg/VirtioNetDxe/SnpInitialize.c
> b/OvmfPkg/VirtioNetDxe/SnpInitialize.c
> index 6cee014..56a55db 100644
> --- a/OvmfPkg/VirtioNetDxe/SnpInitialize.c
> +++ b/OvmfPkg/VirtioNetDxe/SnpInitialize.c
> @@ -57,14 +57,15 @@ VirtioNetInitRing (
> //
> // step 4b -- allocate selected queue
> //
> - Status = VIRTIO_CFG_WRITE (Dev, Generic.VhdrQueueSelect, Selector);
> + Status = Dev->VirtIo->SetQueueSel (Dev->VirtIo, Selector);
> if (EFI_ERROR (Status)) {
> return Status;
> }
> - Status = VIRTIO_CFG_READ (Dev, Generic.VhdrQueueSize, &QueueSize);
> + Status = Dev->VirtIo->GetQueueNumMax (Dev->VirtIo, &QueueSize);
> if (EFI_ERROR (Status)) {
> return Status;
> }
> +
> //
> // For each packet (RX and TX alike), we need two descriptors:
> // one for the virtio-net request header, and another one for the data
> @@ -80,8 +81,8 @@ VirtioNetInitRing (
> //
> // step 4c -- report GPFN (guest-physical frame number) of queue
> //
> - Status = VIRTIO_CFG_WRITE (Dev, Generic.VhdrQueueAddress,
> - (UINTN) Ring->Base >> EFI_PAGE_SHIFT);
> + Status = Dev->VirtIo->SetQueueAddress (Dev->VirtIo,
> + (UINTN) Ring->Base >> EFI_PAGE_SHIFT);
> if (EFI_ERROR (Status)) {
> VirtioRingUninit (Ring);
> }
(6) The conversion of this function is incomplete (for MMIO).
The "Additional steps for MMIO", that is, the SetQueueNum() and
SetQueueAlign() calls -- including my notes under (2), ie. error
checking -- are necessary here, between VirtioRingInit() and
SetQueueAddress().
You'll have to extract the VirtioRingUninit() "rollback" call under an
error handling label, and jump to it if any of SetQueueNum(),
SetQueueAlign(), SetQueueAddress() fail.
> @@ -287,10 +288,9 @@ VirtioNetInitRx (
> // virtio-0.9.5, 2.4.1.4 Notifying the Device
> //
> MemoryFence ();
> - Status = VIRTIO_CFG_WRITE (Dev, Generic.VhdrQueueNotify, VIRTIO_NET_Q_RX);
> -
> + Status = Dev->VirtIo->SetQueueNotify (Dev->VirtIo, VIRTIO_NET_Q_RX);
> if (EFI_ERROR (Status)) {
> - VIRTIO_CFG_WRITE (Dev, Generic.VhdrDeviceStatus, 0);
> + Dev->VirtIo->SetDeviceStatus (Dev->VirtIo, 0);
> FreePool (Dev->RxBuf);
> }
>
Seems OK.
> @@ -366,25 +366,34 @@ VirtioNetInitialize (
> // virtio-0.9.5 spec, 2.2.1 Device Initialization Sequence.
> //
> NextDevStat = VSTAT_ACK; // step 2 -- acknowledge device presence
> - Status = VIRTIO_CFG_WRITE (Dev, Generic.VhdrDeviceStatus, NextDevStat);
> + Status = Dev->VirtIo->SetDeviceStatus (Dev->VirtIo, NextDevStat);
> if (EFI_ERROR (Status)) {
> goto InitFailed;
> }
>
> NextDevStat |= VSTAT_DRIVER; // step 3 -- we know how to drive it
> - Status = VIRTIO_CFG_WRITE (Dev, Generic.VhdrDeviceStatus, NextDevStat);
> + Status = Dev->VirtIo->SetDeviceStatus (Dev->VirtIo, NextDevStat);
> if (EFI_ERROR (Status)) {
> goto DeviceFailed;
> }
>
> //
> + // Set Page Size - MMIO VirtIo Specific
> + //
> + Status = Dev->VirtIo->SetPageSize (Dev->VirtIo, EFI_PAGE_SIZE);
> + if (EFI_ERROR (Status)) {
> + goto ReleaseTxRing;
> + }
> +
(7) This seems to happen in the right place, but the error label is
wrong. It should be DeviceFailed.
> + //
> // step 4a -- retrieve features. Note that we're past validating required
> // features in VirtioNetGetFeatures().
> //
> - Status = VIRTIO_CFG_READ (Dev, Generic.VhdrDeviceFeatureBits, &Features);
> + Status = Dev->VirtIo->GetDeviceFeatures (Dev->VirtIo, &Features);
> if (EFI_ERROR (Status)) {
> goto DeviceFailed;
> }
> +
> ASSERT (Features & VIRTIO_NET_F_MAC);
> ASSERT (Dev->Snm.MediaPresentSupported ==
> !!(Features & VIRTIO_NET_F_STATUS));
> @@ -406,7 +415,7 @@ VirtioNetInitialize (
> // step 5 -- keep only the features we want
> //
> Features &= VIRTIO_NET_F_MAC | VIRTIO_NET_F_STATUS;
> - Status = VIRTIO_CFG_WRITE (Dev, Generic.VhdrGuestFeatureBits, Features);
> + Status = Dev->VirtIo->SetGuestFeatures (Dev->VirtIo, Features);
> if (EFI_ERROR (Status)) {
> goto ReleaseTxRing;
> }
> @@ -415,7 +424,7 @@ VirtioNetInitialize (
> // step 6 -- virtio-net initialization complete
> //
> NextDevStat |= VSTAT_DRIVER_OK;
> - Status = VIRTIO_CFG_WRITE (Dev, Generic.VhdrDeviceStatus, NextDevStat);
> + Status = Dev->VirtIo->SetDeviceStatus (Dev->VirtIo, NextDevStat);
> if (EFI_ERROR (Status)) {
> goto ReleaseTxRing;
> }
> @@ -441,7 +450,7 @@ ReleaseTxAux:
> VirtioNetShutdownTx (Dev);
>
> AbortDevice:
> - VIRTIO_CFG_WRITE (Dev, Generic.VhdrDeviceStatus, 0);
> + Dev->VirtIo->SetDeviceStatus (Dev->VirtIo, 0);
>
> ReleaseTxRing:
> VirtioRingUninit (&Dev->TxRing);
> @@ -453,7 +462,7 @@ DeviceFailed:
> //
> // restore device status invariant for the EfiSimpleNetworkStarted state
> //
> - VIRTIO_CFG_WRITE (Dev, Generic.VhdrDeviceStatus, 0);
> + Dev->VirtIo->SetDeviceStatus (Dev->VirtIo, 0);
>
> InitFailed:
> gBS->RestoreTPL (OldTpl);
The rest of these hunks seems fine.
> diff --git a/OvmfPkg/VirtioNetDxe/SnpReceive.c
> b/OvmfPkg/VirtioNetDxe/SnpReceive.c
> index 87c6ca9..99abd7e 100644
> --- a/OvmfPkg/VirtioNetDxe/SnpReceive.c
> +++ b/OvmfPkg/VirtioNetDxe/SnpReceive.c
> @@ -147,14 +147,14 @@ VirtioNetReceive (
> CopyMem (Buffer, RxPtr, RxLen);
>
> if (DestAddr != NULL) {
> - CopyMem (DestAddr, RxPtr, SIZE_OF_VNET (VhdrMac));
> + CopyMem (DestAddr, RxPtr, SIZE_OF_VNET (Mac));
> }
> - RxPtr += SIZE_OF_VNET (VhdrMac);
> + RxPtr += SIZE_OF_VNET (Mac);
>
> if (SrcAddr != NULL) {
> - CopyMem (SrcAddr, RxPtr, SIZE_OF_VNET (VhdrMac));
> + CopyMem (SrcAddr, RxPtr, SIZE_OF_VNET (Mac));
> }
> - RxPtr += SIZE_OF_VNET (VhdrMac);
> + RxPtr += SIZE_OF_VNET (Mac);
>
> if (Protocol != NULL) {
> *Protocol = (UINT16) ((RxPtr[0] << 8) | RxPtr[1]);
> @@ -177,9 +177,7 @@ RecycleDesc:
> *Dev->RxRing.Avail.Idx = AvailIdx;
>
> MemoryFence ();
> - NotifyStatus = VIRTIO_CFG_WRITE (Dev, Generic.VhdrQueueNotify,
> - VIRTIO_NET_Q_RX);
> -
> + NotifyStatus = Dev->VirtIo->SetQueueNotify (Dev->VirtIo, VIRTIO_NET_Q_RX);
> if (!EFI_ERROR (Status)) { // earlier error takes precedence
> Status = NotifyStatus;
> }
OK
> diff --git a/OvmfPkg/VirtioNetDxe/SnpShutdown.c
> b/OvmfPkg/VirtioNetDxe/SnpShutdown.c
> index 42aeca1..01409c0 100644
> --- a/OvmfPkg/VirtioNetDxe/SnpShutdown.c
> +++ b/OvmfPkg/VirtioNetDxe/SnpShutdown.c
> @@ -63,7 +63,7 @@ VirtioNetShutdown (
> break;
> }
>
> - VIRTIO_CFG_WRITE (Dev, Generic.VhdrDeviceStatus, 0);
> + Dev->VirtIo->SetDeviceStatus (Dev->VirtIo, 0);
> VirtioNetShutdownRx (Dev);
> VirtioNetShutdownTx (Dev);
> VirtioRingUninit (&Dev->TxRing);
OK
> diff --git a/OvmfPkg/VirtioNetDxe/SnpTransmit.c
> b/OvmfPkg/VirtioNetDxe/SnpTransmit.c
> index ff922ca..7ca40d5 100644
> --- a/OvmfPkg/VirtioNetDxe/SnpTransmit.c
> +++ b/OvmfPkg/VirtioNetDxe/SnpTransmit.c
> @@ -127,15 +127,15 @@ VirtioNetTransmit (
> goto Exit;
> }
> Ptr = Buffer;
> - ASSERT (SIZE_OF_VNET (VhdrMac) <= sizeof (EFI_MAC_ADDRESS));
> + ASSERT (SIZE_OF_VNET (Mac) <= sizeof (EFI_MAC_ADDRESS));
>
> - CopyMem (Ptr, DestAddr, SIZE_OF_VNET (VhdrMac));
> - Ptr += SIZE_OF_VNET (VhdrMac);
> + CopyMem (Ptr, DestAddr, SIZE_OF_VNET (Mac));
> + Ptr += SIZE_OF_VNET (Mac);
>
> CopyMem (Ptr,
> (SrcAddr == NULL) ? &Dev->Snm.CurrentAddress : SrcAddr,
> - SIZE_OF_VNET (VhdrMac));
> - Ptr += SIZE_OF_VNET (VhdrMac);
> + SIZE_OF_VNET (Mac));
> + Ptr += SIZE_OF_VNET (Mac);
>
> *Ptr++ = (UINT8) (*Protocol >> 8);
> *Ptr++ = (UINT8) *Protocol;
> @@ -161,7 +161,7 @@ VirtioNetTransmit (
> *Dev->TxRing.Avail.Idx = AvailIdx;
>
> MemoryFence ();
> - Status = VIRTIO_CFG_WRITE (Dev, Generic.VhdrQueueNotify, VIRTIO_NET_Q_TX);
> + Status = Dev->VirtIo->SetQueueNotify (Dev->VirtIo, VIRTIO_NET_Q_TX);
>
> Exit:
> gBS->RestoreTPL (OldTpl);
Seems OK.
> diff --git a/OvmfPkg/VirtioNetDxe/VirtioNet.h
> b/OvmfPkg/VirtioNetDxe/VirtioNet.h
> index 8c21bcd..31fca79 100644
> --- a/OvmfPkg/VirtioNetDxe/VirtioNet.h
> +++ b/OvmfPkg/VirtioNetDxe/VirtioNet.h
> @@ -24,7 +24,6 @@
> #include <Protocol/ComponentName2.h>
> #include <Protocol/DevicePath.h>
> #include <Protocol/DriverBinding.h>
> -#include <Protocol/PciIo.h>
> #include <Protocol/SimpleNetwork.h>
>
> #define VNET_SIG SIGNATURE_32 ('V', 'N', 'E', 'T')
> @@ -75,8 +74,7 @@ typedef struct {
> // field init function
> // ------------------
> ------------------------------
> UINT32 Signature; //
> VirtioNetDriverBindingStart
> - EFI_PCI_IO_PROTOCOL *PciIo; //
> VirtioNetDriverBindingStart
> - UINT64 OrigPciAttributes; //
> VirtioNetDriverBindingStart
> + VIRTIO_DEVICE_PROTOCOL *VirtIo; //
> VirtioNetDriverBindingStart
> EFI_SIMPLE_NETWORK_PROTOCOL Snp; // VirtioNetSnpPopulate
> EFI_SIMPLE_NETWORK_MODE Snm; // VirtioNetSnpPopulate
> EFI_EVENT ExitBoot; // VirtioNetSnpPopulate
> @@ -109,15 +107,15 @@ typedef struct {
> #define VIRTIO_NET_FROM_SNP(SnpPointer) \
> CR (SnpPointer, VNET_DEV, Snp, VNET_SIG)
>
> -#define VIRTIO_CFG_WRITE(Dev, Field, Value) (VirtioWrite ( \
> - (Dev)->PciIo, \
> +#define VIRTIO_CFG_WRITE(Dev, Field, Value) (VirtioWriteDevice ( \
> + (Dev)->VirtIo, \
> OFFSET_OF_VNET (Field), \
> SIZE_OF_VNET (Field), \
> (Value) \
> ))
>
> -#define VIRTIO_CFG_READ(Dev, Field, Pointer) (VirtioRead ( \
> - (Dev)->PciIo, \
> +#define VIRTIO_CFG_READ(Dev, Field, Pointer) (VirtioReadDevice ( \
> + (Dev)->VirtIo, \
> OFFSET_OF_VNET (Field), \
> SIZE_OF_VNET (Field), \
> sizeof *(Pointer), \
Seems OK.
> diff --git a/OvmfPkg/VirtioNetDxe/VirtioNet.inf
> b/OvmfPkg/VirtioNetDxe/VirtioNet.inf
> index 408a541..a855ad4 100644
> --- a/OvmfPkg/VirtioNetDxe/VirtioNet.inf
> +++ b/OvmfPkg/VirtioNetDxe/VirtioNet.inf
> @@ -57,4 +57,4 @@
> [Protocols]
> gEfiSimpleNetworkProtocolGuid ## BY_START
> gEfiDevicePathProtocolGuid ## BY_START
> - gEfiPciIoProtocolGuid ## TO_START
> + gVirtioDeviceProtocolGuid ## TO_START
OK.
> diff --git a/OvmfPkg/VirtioScsiDxe/VirtioScsi.c
> b/OvmfPkg/VirtioScsiDxe/VirtioScsi.c
> index b836fb3..006ba2d 100644
> --- a/OvmfPkg/VirtioScsiDxe/VirtioScsi.c
> +++ b/OvmfPkg/VirtioScsiDxe/VirtioScsi.c
> @@ -38,7 +38,6 @@
>
> **/
>
> -#include <IndustryStandard/Pci.h>
> #include <IndustryStandard/VirtioScsi.h>
> #include <Library/BaseMemoryLib.h>
> #include <Library/DebugLib.h>
> @@ -52,14 +51,13 @@
> /**
>
> Convenience macros to read and write region 0 IO space elements of the
> - virtio-scsi PCI device, for configuration purposes.
> + virtio-scsi VirtIo device, for configuration purposes.
("region 0" is not necessarily meaningful, but it's not important)
>
> The following macros make it possible to specify only the "core parameters"
> for such accesses and to derive the rest. By the time VIRTIO_CFG_WRITE()
> returns, the transaction will have been completed.
>
> - @param[in] Dev Pointer to the VSCSI_DEV structure whose PCI IO space
> - we're accessing. Dev->PciIo must be valid.
> + @param[in] Dev Pointer to the VirtIo Device Protocol
Dropping mentions of "PCI" is valid, but the pointer itself continues to
point to VSCSI_DEV, no?
>
> @param[in] Field A field name from VSCSI_HDR, identifying the
> virtio-scsi
> configuration item to access.
> @@ -72,19 +70,19 @@
> one of UINT8, UINT16, UINT32, UINT64.
>
>
> - @return Status codes returned by VirtioWrite() / VirtioRead().
> + @return Status codes returned by VirtioWriteDevice() / VirtioReadDevice().
>
> **/
This comment part is going to be updated in v4 11/11, good.
>
> -#define VIRTIO_CFG_WRITE(Dev, Field, Value) (VirtioWrite ( \
> - (Dev)->PciIo, \
> +#define VIRTIO_CFG_WRITE(Dev, Field, Value) (VirtioWriteDevice ( \
> + (Dev)->VirtIo, \
> OFFSET_OF_VSCSI (Field), \
> SIZE_OF_VSCSI (Field), \
> (Value) \
> ))
>
> -#define VIRTIO_CFG_READ(Dev, Field, Pointer) (VirtioRead ( \
> - (Dev)->PciIo, \
> +#define VIRTIO_CFG_READ(Dev, Field, Pointer) (VirtioReadDevice ( \
> + (Dev)->VirtIo, \
> OFFSET_OF_VSCSI (Field), \
> SIZE_OF_VSCSI (Field), \
> sizeof *(Pointer), \
OK
> @@ -471,7 +469,7 @@ VirtioScsiPassThru (
> // EFI_NOT_READY would save us the effort, but it would also suggest that
> the
> // caller retry.
> //
> - if (VirtioFlush (Dev->PciIo, VIRTIO_SCSI_REQUEST_QUEUE, &Dev->Ring,
> + if (VirtioFlush (Dev->VirtIo, VIRTIO_SCSI_REQUEST_QUEUE, &Dev->Ring,
> &Indices) != EFI_SUCCESS) {
> Packet->InTransferLength = 0;
> Packet->OutTransferLength = 0;
> @@ -718,33 +716,41 @@ VirtioScsiInit (
> // Execute virtio-0.9.5, 2.2.1 Device Initialization Sequence.
> //
> NextDevStat = 0; // step 1 -- reset device
> - Status = VIRTIO_CFG_WRITE (Dev, Generic.VhdrDeviceStatus, NextDevStat);
> + Status = Dev->VirtIo->SetDeviceStatus (Dev->VirtIo, NextDevStat);
> if (EFI_ERROR (Status)) {
> goto Failed;
> }
>
> NextDevStat |= VSTAT_ACK; // step 2 -- acknowledge device presence
> - Status = VIRTIO_CFG_WRITE (Dev, Generic.VhdrDeviceStatus, NextDevStat);
> + Status = Dev->VirtIo->SetDeviceStatus (Dev->VirtIo, NextDevStat);
> if (EFI_ERROR (Status)) {
> goto Failed;
> }
>
> NextDevStat |= VSTAT_DRIVER; // step 3 -- we know how to drive it
> - Status = VIRTIO_CFG_WRITE (Dev, Generic.VhdrDeviceStatus, NextDevStat);
> + Status = Dev->VirtIo->SetDeviceStatus (Dev->VirtIo, NextDevStat);
> if (EFI_ERROR (Status)) {
> goto Failed;
> }
>
> //
> + // Set Page Size - MMIO VirtIo Specific
> + //
> + Status = Dev->VirtIo->SetPageSize (Dev->VirtIo, EFI_PAGE_SIZE);
> + if (EFI_ERROR (Status)) {
> + goto ReleaseQueue;
> + }
(8) Jumping to wrong label for error handling (should be Failed),
similarly to (1).
> +
> + //
> // step 4a -- retrieve and validate features
> //
> - Status = VIRTIO_CFG_READ (Dev, Generic.VhdrDeviceFeatureBits, &Features);
> + Status = Dev->VirtIo->GetDeviceFeatures (Dev->VirtIo, &Features);
> if (EFI_ERROR (Status)) {
> goto Failed;
> }
> Dev->InOutSupported = !!(Features & VIRTIO_SCSI_F_INOUT);
>
> - Status = VIRTIO_CFG_READ (Dev, VhdrMaxChannel, &MaxChannel);
> + Status = VIRTIO_CFG_READ (Dev, MaxChannel, &MaxChannel);
> if (EFI_ERROR (Status)) {
> goto Failed;
> }
> @@ -756,7 +762,7 @@ VirtioScsiInit (
> goto Failed;
> }
>
> - Status = VIRTIO_CFG_READ (Dev, VhdrNumQueues, &NumQueues);
> + Status = VIRTIO_CFG_READ (Dev, NumQueues, &NumQueues);
> if (EFI_ERROR (Status)) {
> goto Failed;
> }
> @@ -765,7 +771,7 @@ VirtioScsiInit (
> goto Failed;
> }
>
> - Status = VIRTIO_CFG_READ (Dev, VhdrMaxTarget, &Dev->MaxTarget);
> + Status = VIRTIO_CFG_READ (Dev, MaxTarget, &Dev->MaxTarget);
> if (EFI_ERROR (Status)) {
> goto Failed;
> }
> @@ -773,7 +779,7 @@ VirtioScsiInit (
> Dev->MaxTarget = PcdGet16 (PcdVirtioScsiMaxTargetLimit);
> }
>
> - Status = VIRTIO_CFG_READ (Dev, VhdrMaxLun, &Dev->MaxLun);
> + Status = VIRTIO_CFG_READ (Dev, MaxLun, &Dev->MaxLun);
> if (EFI_ERROR (Status)) {
> goto Failed;
> }
> @@ -781,7 +787,7 @@ VirtioScsiInit (
> Dev->MaxLun = PcdGet32 (PcdVirtioScsiMaxLunLimit);
> }
>
> - Status = VIRTIO_CFG_READ (Dev, VhdrMaxSectors, &Dev->MaxSectors);
> + Status = VIRTIO_CFG_READ (Dev, MaxSectors, &Dev->MaxSectors);
> if (EFI_ERROR (Status)) {
> goto Failed;
> }
> @@ -797,12 +803,11 @@ VirtioScsiInit (
> //
> // step 4b -- allocate request virtqueue
> //
> - Status = VIRTIO_CFG_WRITE (Dev, Generic.VhdrQueueSelect,
> - VIRTIO_SCSI_REQUEST_QUEUE);
> + Status = Dev->VirtIo->SetQueueSel (Dev->VirtIo, VIRTIO_SCSI_REQUEST_QUEUE);
> if (EFI_ERROR (Status)) {
> goto Failed;
> }
> - Status = VIRTIO_CFG_READ (Dev, Generic.VhdrQueueSize, &QueueSize);
> + Status = Dev->VirtIo->GetQueueNumMax (Dev->VirtIo, &QueueSize);
> if (EFI_ERROR (Status)) {
> goto Failed;
> }
> @@ -823,8 +828,8 @@ VirtioScsiInit (
> // step 4c -- Report GPFN (guest-physical frame number) of queue. If
> anything
> // fails from here on, we must release the ring resources.
> //
> - Status = VIRTIO_CFG_WRITE (Dev, Generic.VhdrQueueAddress,
> - (UINTN) Dev->Ring.Base >> EFI_PAGE_SHIFT);
> + Status = Dev->VirtIo->SetQueueAddress (Dev->VirtIo,
> + (UINTN) Dev->Ring.Base >> EFI_PAGE_SHIFT);
> if (EFI_ERROR (Status)) {
> goto ReleaseQueue;
> }
(9) The conversion is incomplete for MMIO; note (6) applies.
When fixing this, please move the "If anything fails from here..."
sentence as well (pls. refer to (2)).
The error label to use with the new SetQueueNum() and SetQueueAlign()
calls is "ReleaseQueue" of course (they take place after VirtioRingInit()).
> @@ -834,8 +839,8 @@ VirtioScsiInit (
> // the known (or unknown) VIRTIO_SCSI_F_* or VIRTIO_F_* capabilities (see
> // virtio-0.9.5, Appendices B and I), except bidirectional transfers.
> //
> - Status = VIRTIO_CFG_WRITE (Dev, Generic.VhdrGuestFeatureBits,
> - Features & VIRTIO_SCSI_F_INOUT);
> + Status = Dev->VirtIo->SetGuestFeatures (Dev->VirtIo,
> + Features & VIRTIO_SCSI_F_INOUT);
> if (EFI_ERROR (Status)) {
> goto ReleaseQueue;
> }
> @@ -844,11 +849,11 @@ VirtioScsiInit (
> // We expect these maximum sizes from the host. Since they are
> // guest-negotiable, ask for them rather than just checking them.
> //
> - Status = VIRTIO_CFG_WRITE (Dev, VhdrCdbSize, VIRTIO_SCSI_CDB_SIZE);
> + Status = VIRTIO_CFG_WRITE (Dev, CdbSize, VIRTIO_SCSI_CDB_SIZE);
> if (EFI_ERROR (Status)) {
> goto ReleaseQueue;
> }
> - Status = VIRTIO_CFG_WRITE (Dev, VhdrSenseSize, VIRTIO_SCSI_SENSE_SIZE);
> + Status = VIRTIO_CFG_WRITE (Dev, SenseSize, VIRTIO_SCSI_SENSE_SIZE);
> if (EFI_ERROR (Status)) {
> goto ReleaseQueue;
> }
> @@ -857,7 +862,7 @@ VirtioScsiInit (
> // step 6 -- initialization complete
> //
> NextDevStat |= VSTAT_DRIVER_OK;
> - Status = VIRTIO_CFG_WRITE (Dev, Generic.VhdrDeviceStatus, NextDevStat);
> + Status = Dev->VirtIo->SetDeviceStatus (Dev->VirtIo, NextDevStat);
> if (EFI_ERROR (Status)) {
> goto ReleaseQueue;
> }
> @@ -901,10 +906,10 @@ ReleaseQueue:
> Failed:
> //
> // Notify the host about our failure to setup: virtio-0.9.5, 2.2.2.1 Device
> - // Status. PCI IO access failure here should not mask the original error.
> + // Status. VirtIo access failure here should not mask the original error.
> //
> NextDevStat |= VSTAT_FAILED;
> - VIRTIO_CFG_WRITE (Dev, Generic.VhdrDeviceStatus, NextDevStat);
> + Dev->VirtIo->SetDeviceStatus (Dev->VirtIo, NextDevStat);
>
> Dev->InOutSupported = FALSE;
> Dev->MaxTarget = 0;
> @@ -928,7 +933,7 @@ VirtioScsiUninit (
> // VIRTIO_CFG_WRITE() returns, the host will have learned to stay away from
> // the old comms area.
> //
> - VIRTIO_CFG_WRITE (Dev, Generic.VhdrDeviceStatus, 0);
> + Dev->VirtIo->SetDeviceStatus (Dev->VirtIo, 0);
>
> Dev->InOutSupported = FALSE;
> Dev->MaxTarget = 0;
> @@ -953,11 +958,8 @@ VirtioScsiUninit (
> // The implementation follows:
> // - Driver Writer's Guide for UEFI 2.3.1 v1.01
> // - 5.1.3.4 OpenProtocol() and CloseProtocol()
> -// - 18 PCI Driver Design Guidelines
> -// - 18.3 PCI drivers
> // - UEFI Spec 2.3.1 + Errata C
> // - 6.3 Protocol Handler Services
> -// - 13.4 EFI PCI I/O Protocol
> //
I like how this comment block is updated -- only the PCI references are
removed, not the entire block. Good.
>
> EFI_STATUS
> @@ -968,57 +970,37 @@ VirtioScsiDriverBindingSupported (
> IN EFI_DEVICE_PATH_PROTOCOL *RemainingDevicePath
> )
> {
> - EFI_STATUS Status;
> - EFI_PCI_IO_PROTOCOL *PciIo;
> - PCI_TYPE00 Pci;
> + EFI_STATUS Status;
> + VIRTIO_DEVICE_PROTOCOL *VirtIo;
>
> //
> - // Attempt to open the device with the PciIo set of interfaces. On success,
> - // the protocol is "instantiated" for the PCI device. Covers duplicate open
> + // Attempt to open the device with the VirtIo set of interfaces. On
> success,
> + // the protocol is "instantiated" for the VirtIo device. Covers duplicate
> open
> // attempts (EFI_ALREADY_STARTED).
> //
> Status = gBS->OpenProtocol (
> DeviceHandle, // candidate device
> - &gEfiPciIoProtocolGuid, // for generic PCI access
> - (VOID **)&PciIo, // handle to instantiate
> + &gVirtioDeviceProtocolGuid, // for generic VirtIo access
> + (VOID **)&VirtIo, // 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
> + EFI_OPEN_PROTOCOL_BY_DRIVER // get exclusive VirtIo 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
> - //
> - Status = (Pci.Hdr.VendorId == 0x1AF4 &&
> - Pci.Hdr.DeviceId >= 0x1000 && Pci.Hdr.DeviceId <= 0x103F &&
> - Pci.Hdr.RevisionID == 0x00 &&
> - Pci.Device.SubsystemID == VIRTIO_SUBSYSTEM_SCSI_HOST) ?
> EFI_SUCCESS : EFI_UNSUPPORTED;
> + if (VirtIo->SubSystemDeviceId != VIRTIO_SUBSYSTEM_SCSI_HOST) {
> + Status = EFI_UNSUPPORTED;
> }
>
> //
> - // We needed PCI IO access only transitorily, to see whether we support the
> + // We needed VirtIo access only transitorily, to see whether we support the
> // device or not.
> //
> - gBS->CloseProtocol (DeviceHandle, &gEfiPciIoProtocolGuid,
> + gBS->CloseProtocol (DeviceHandle, &gVirtioDeviceProtocolGuid,
> This->DriverBindingHandle, DeviceHandle);
> return Status;
> }
The result matches VirtioBlkDriverBindingSupported() modulo
VIRTIO_SUBSYSTEM_SCSI_HOST; it's good.
> @@ -1040,43 +1022,19 @@ VirtioScsiDriverBindingStart (
> return EFI_OUT_OF_RESOURCES;
> }
>
> - Status = gBS->OpenProtocol (DeviceHandle, &gEfiPciIoProtocolGuid,
> - (VOID **)&Dev->PciIo, This->DriverBindingHandle,
> + Status = gBS->OpenProtocol (DeviceHandle, &gVirtioDeviceProtocolGuid,
> + (VOID **)&Dev->VirtIo, This->DriverBindingHandle,
> DeviceHandle, EFI_OPEN_PROTOCOL_BY_DRIVER);
> if (EFI_ERROR (Status)) {
> goto FreeVirtioScsi;
> }
>
> //
> - // 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-scsi we only need IO space access.
> - //
> - Status = Dev->PciIo->Attributes (Dev->PciIo, EfiPciIoAttributeOperationGet,
> - 0, &Dev->OriginalPciAttributes);
> - if (EFI_ERROR (Status)) {
> - goto ClosePciIo;
> - }
> -
> - Status = Dev->PciIo->Attributes (Dev->PciIo,
> - EfiPciIoAttributeOperationEnable,
> - EFI_PCI_IO_ATTRIBUTE_IO, NULL);
> - if (EFI_ERROR (Status)) {
> - goto ClosePciIo;
> - }
> -
> - //
> - // PCI IO access granted, configure virtio-scsi device.
> + // VirtIo access granted, configure virtio-scsi device.
> //
You haven't added the superfluous SubSystemDeviceId check to this
BindingStart() function, and that's correct.
> Status = VirtioScsiInit (Dev);
> if (EFI_ERROR (Status)) {
> - goto RestorePciAttributes;
> + goto CloseVirtIo;
> }
>
> //
> @@ -1096,12 +1054,8 @@ VirtioScsiDriverBindingStart (
> UninitDev:
> VirtioScsiUninit (Dev);
>
> -RestorePciAttributes:
> - Dev->PciIo->Attributes (Dev->PciIo, EfiPciIoAttributeOperationSet,
> - Dev->OriginalPciAttributes, NULL);
> -
> -ClosePciIo:
> - gBS->CloseProtocol (DeviceHandle, &gEfiPciIoProtocolGuid,
> +CloseVirtIo:
> + gBS->CloseProtocol (DeviceHandle, &gVirtioDeviceProtocolGuid,
> This->DriverBindingHandle, DeviceHandle);
>
> FreeVirtioScsi:
OK.
> @@ -1149,10 +1103,7 @@ VirtioScsiDriverBindingStop (
>
> VirtioScsiUninit (Dev);
>
> - Dev->PciIo->Attributes (Dev->PciIo, EfiPciIoAttributeOperationSet,
> - Dev->OriginalPciAttributes, NULL);
> -
> - gBS->CloseProtocol (DeviceHandle, &gEfiPciIoProtocolGuid,
> + gBS->CloseProtocol (DeviceHandle, &gVirtioDeviceProtocolGuid,
> This->DriverBindingHandle, DeviceHandle);
>
> FreePool (Dev);
OK
> diff --git a/OvmfPkg/VirtioScsiDxe/VirtioScsi.h
> b/OvmfPkg/VirtioScsiDxe/VirtioScsi.h
> index f5220b2..0d3181d 100644
> --- a/OvmfPkg/VirtioScsiDxe/VirtioScsi.h
> +++ b/OvmfPkg/VirtioScsiDxe/VirtioScsi.h
> @@ -20,7 +20,6 @@
>
> #include <Protocol/ComponentName.h>
> #include <Protocol/DriverBinding.h>
> -#include <Protocol/PciIo.h>
> #include <Protocol/ScsiPassThruExt.h>
>
> #include <IndustryStandard/Virtio.h>
> @@ -49,18 +48,17 @@ typedef struct {
> // at various call depths. The table to the right should make it easier to
> // track them.
> //
> - // field init function
> init depth
> - // ----------------------
> ------------------ ----------
> - UINT32 Signature; //
> DriverBindingStart 0
> - EFI_PCI_IO_PROTOCOL *PciIo; //
> DriverBindingStart 0
> - UINT64 OriginalPciAttributes; //
> DriverBindingStart 0
> - BOOLEAN InOutSupported; // VirtioScsiInit
> 1
> - UINT16 MaxTarget; // VirtioScsiInit
> 1
> - UINT32 MaxLun; // VirtioScsiInit
> 1
> - UINT32 MaxSectors; // VirtioScsiInit
> 1
> - VRING Ring; // VirtioRingInit
> 2
> - EFI_EXT_SCSI_PASS_THRU_PROTOCOL PassThru; // VirtioScsiInit
> 1
> - EFI_EXT_SCSI_PASS_THRU_MODE PassThruMode; // VirtioScsiInit
> 1
> + // field init function
> init depth
> + // ---------------- ------------------
> ----------
> + UINT32 Signature; // DriverBindingStart 0
> + VIRTIO_DEVICE_PROTOCOL *VirtIo; // DriverBindingStart 0
> + BOOLEAN InOutSupported; // VirtioScsiInit 1
> + UINT16 MaxTarget; // VirtioScsiInit 1
> + UINT32 MaxLun; // VirtioScsiInit 1
> + UINT32 MaxSectors; // VirtioScsiInit 1
> + VRING Ring; // VirtioRingInit 2
> + EFI_EXT_SCSI_PASS_THRU_PROTOCOL PassThru; // VirtioScsiInit 1
> + EFI_EXT_SCSI_PASS_THRU_MODE PassThruMode; // VirtioScsiInit 1
> } VSCSI_DEV;
>
> #define VIRTIO_SCSI_FROM_PASS_THRU(PassThruPointer) \
OK
> diff --git a/OvmfPkg/VirtioScsiDxe/VirtioScsi.inf
> b/OvmfPkg/VirtioScsiDxe/VirtioScsi.inf
> index 8209c50..7558112 100644
> --- a/OvmfPkg/VirtioScsiDxe/VirtioScsi.inf
> +++ b/OvmfPkg/VirtioScsiDxe/VirtioScsi.inf
> @@ -40,7 +40,7 @@
>
> [Protocols]
> gEfiExtScsiPassThruProtocolGuid ## BY_START
> - gEfiPciIoProtocolGuid ## TO_START
> + gVirtioDeviceProtocolGuid ## TO_START
>
> [Pcd]
> gUefiOvmfPkgTokenSpaceGuid.PcdVirtioScsiMaxTargetLimit ## CONSUMES
>
Good.
(INF_VERSION has not been bumped, which is inconsistent with the rest of
the patchset, but since I don't see the motivation for the INF_VERSION
changes elsewhere anyway, I don't miss it here.)
Problems (1) to (9) should be addressed in this patch I think. The rest
is "nice to have" only, as usual.
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
