On Tue, Dec 10, 2013 at 1:54 PM, Laszlo Ersek <ler...@redhat.com> wrote:
> On 12/10/13 20:28, Jordan Justen wrote:
>> Laszlo,
>>
>> In comparing Olivier's v4 to your v5, I had some questions.
>>
>> Mainly, it seems like maybe some 'fixes' are sneaking into the commit
>> where we convert to using the protocol.
>
> Those fixes are not "sneaking". I requested them explicitly in my v4
> review. My method for preparing version 5 was:
> - take Olivier's v4
> - go through my review comments (and some of yours that I could find) on
> the list that have been posted for v4
> - those comments pointed out problems in v4
> - therefore I fixed those problems where they were introduced.
>
> Normally when a respin is done, if v(n) of a patch introduces a problem,
> then v(n+1) simply avoids introducing the problem. Followup patches are
> justified when:
> - v(n) has been committed, or
> - fixing problems in-place (ie. where v(n) introduces those problems)
> would be very hard / inconvenient (for example because it requires
> intrusive rebase in the rest of the series).
>
> None of this held for these changes.
I guess my question was, could any of these changes be considered
necessary separate from this series? Or, are they all required
specifically because of the conversion to using the protocol?
Your response above indicates they were issues introduced as part of
the conversion process, and yes, in that case they should be fixed in
the same commit in v5.
As such, I will move forward with the patches as is. (I still need to
review them a little more, and test VS builds.)
-Jordan
> I seem to recall that we discussed before the possibility of fixing
> stuff that had emerged in v4 review in future patches. But that was only
> mentioned because it's easier to do, not because it's better.
>
>> It seems like we should keep
>> those fixes separate, unless they are specifically necessary because
>> of the change to using the protocol.
>>
>> I think the fixes themselves could be grouped into a single commit.
>
> In this case I would prefer if the patches did the right thing
> immediately, as soon as they are committed. The fixes I added belong to
> the conversion.
>
>> I'm just worried if the fixes change something then a bisect would
>> point at the conversion to use protocol commit.
>
> I have not touched device driver operation.
>
>>
>> Comparing v4 to v5:
>>> diff --git a/ArmPlatformPkg/ArmVExpressPkg/ArmFvpDxe/ArmFvpDxe.c
>>> b/ArmPlatformPkg/ArmVExpressPkg/ArmFvpDxe/ArmFvpDxe.c
>>> index 0cf5b7b..1885b69 100644
>>> --- a/ArmPlatformPkg/ArmVExpressPkg/ArmFvpDxe/ArmFvpDxe.c
>>> +++ b/ArmPlatformPkg/ArmVExpressPkg/ArmFvpDxe/ArmFvpDxe.c
>>> @@ -19,10 +19,12 @@
>>>
>>> #define ARM_FVP_BASE_VIRTIO_BLOCK_BASE 0x1c130000
>>>
>>> +#pragma pack(1)
>>> typedef struct {
>>> VENDOR_DEVICE_PATH Vendor;
>>> EFI_DEVICE_PATH_PROTOCOL End;
>>> } VIRTIO_BLK_DEVICE_PATH;
>>> +#pragma pack()
>>>
>>> VIRTIO_BLK_DEVICE_PATH mVirtioBlockDevicePath =
>>> {
>>> @@ -40,7 +42,7 @@ VIRTIO_BLK_DEVICE_PATH mVirtioBlockDevicePath =
>>> {
>>> END_DEVICE_PATH_TYPE,
>>> END_ENTIRE_DEVICE_PATH_SUBTYPE,
>>> - {
>>> + {
>>> sizeof (EFI_DEVICE_PATH_PROTOCOL),
>>> 0
>>> }
>>> diff --git a/OvmfPkg/Include/Library/VirtioLib.h
>>> b/OvmfPkg/Include/Library/VirtioLib.h
>>> index df4fc62..36527a5 100644
>>> --- a/OvmfPkg/Include/Library/VirtioLib.h
>>> +++ b/OvmfPkg/Include/Library/VirtioLib.h
>>> @@ -168,7 +168,7 @@ VirtioAppendDesc (
>>> of the descriptor chain.
>>>
>>>
>>> - @return Error code from VirtioWriteDevice() if it fails.
>>> + @return Error code from VirtIo->SetQueueNotify() if it
>>> fails.
>>>
>>> @retval EFI_SUCCESS Otherwise, the host processed all descriptors.
>>>
>>> diff --git a/OvmfPkg/Include/Library/VirtioMmioDeviceLib.h
>>> b/OvmfPkg/Include/Library/VirtioMmioDeviceLib.h
>>> index ac71014..3f63a65 100644
>>> --- a/OvmfPkg/Include/Library/VirtioMmioDeviceLib.h
>>> +++ b/OvmfPkg/Include/Library/VirtioMmioDeviceLib.h
>>> @@ -23,12 +23,13 @@
>>>
>>> @param[in] BaseAddress Base Address of the VirtIo MMIO Device
>>>
>>> - @param[in] Handle Handle of the device the driver should be
>>> attached to.
>>> + @param[in] Handle Handle of the device the driver should be
>>> attached
>>> + to.
>>>
>>> @retval EFI_SUCCESS The VirtIo Device has been installed
>>> successfully.
>>>
>>> - @retval EFI_OUT_OF_RESOURCES The function failed too allocate memory
>>> require
>>> + @retval EFI_OUT_OF_RESOURCES The function failed to allocate memory
>>> required
>>> by the Virtio MMIO device initialization.
>>>
>>> @retval EFI_UNSUPPORTED BaseAddress does not point to a VirtIo MMIO
>>> diff --git a/OvmfPkg/Include/Protocol/VirtioDevice.h
>>> b/OvmfPkg/Include/Protocol/VirtioDevice.h
>>> index 71541f8..48fca2e 100644
>>> --- a/OvmfPkg/Include/Protocol/VirtioDevice.h
>>> +++ b/OvmfPkg/Include/Protocol/VirtioDevice.h
>>> @@ -1,6 +1,9 @@
>>> /** @file
>>> Virtio Device
>>>
>>> + DISCLAIMER: the VIRTIO_DEVICE_PROTOCOL introduced here is a work in
>>> progress,
>>> + and should not be used outside of the EDK II tree.
>>
>> Note: I moved this above the copyright.
>
> OK. I wasn't sure which one should have been more prominent, the
> disclaimer or the copyright.
>
>
>>
>>> Copyright (c) 2013, ARM Ltd. All rights reserved.<BR>
>>>
>>> This program and the accompanying materials are licensed and made
>>> available
>>> @@ -341,6 +344,9 @@ EFI_STATUS
>>> ///
>>> /// This protocol provides an abstraction over the VirtIo transport layer
>>> ///
>>> +/// DISCLAIMER: this protocol is a work in progress, and should not be
>>> used
>>> +/// outside of the EDK II tree.
>>> +///
>>> struct _VIRTIO_DEVICE_PROTOCOL {
>>> /// VirtIo Specification Revision encoded with VIRTIO_SPEC_REVISION()
>>> UINT32 Revision;
>>> diff --git a/OvmfPkg/Library/VirtioLib/VirtioLib.c
>>> b/OvmfPkg/Library/VirtioLib/VirtioLib.c
>>> index 9bbd141..54cf225 100644
>>> --- a/OvmfPkg/Library/VirtioLib/VirtioLib.c
>>> +++ b/OvmfPkg/Library/VirtioLib/VirtioLib.c
>>> @@ -250,7 +250,7 @@ VirtioAppendDesc (
>>> of the descriptor chain.
>>>
>>>
>>> - @return Error code from VirtioWriteDevice() if it fails.
>>> + @return Error code from VirtIo->SetQueueNotify() if it
>>> fails.
>>>
>>> @retval EFI_SUCCESS Otherwise, the host processed all descriptors.
>>>
>>> diff --git a/OvmfPkg/Library/VirtioMmioDeviceLib/VirtioMmioDevice.c
>>> b/OvmfPkg/Library/VirtioMmioDeviceLib/VirtioMmioDevice.c
>>> index 4d8578e..7543e15 100644
>>> --- a/OvmfPkg/Library/VirtioMmioDeviceLib/VirtioMmioDevice.c
>>> +++ b/OvmfPkg/Library/VirtioMmioDeviceLib/VirtioMmioDevice.c
>>> @@ -199,6 +199,7 @@ VirtioMmioUninstallDevice (
>>>
>>> // Uninitialize the VirtIo Device
>>> VirtioMmioUninit (MmioDevice);
>>> + FreePool (MmioDevice);
>>
>> I think this is part of the conversion to having the new protocol, so
>> it seems okay. (Ie, not a fix that should be separated out into it's
>> own patch, right?)
>
> Correct.
>
> (But, again, that's the case for all the other fixes.)
>
>>
>>> return EFI_SUCCESS;
>>> }
>>> diff --git
>>> a/OvmfPkg/Library/VirtioMmioDeviceLib/VirtioMmioDeviceFunctions.c
>>> b/OvmfPkg/Library/VirtioMmioDeviceLib/VirtioMmioDeviceFunctions.c
>>> index bcba062..068e1ed 100644
>>> --- a/OvmfPkg/Library/VirtioMmioDeviceLib/VirtioMmioDeviceFunctions.c
>>> +++ b/OvmfPkg/Library/VirtioMmioDeviceLib/VirtioMmioDeviceFunctions.c
>>> @@ -48,7 +48,7 @@ VirtioMmioGetQueueAddress (
>>> VIRTIO_MMIO_DEVICE *Device;
>>>
>>> if (QueueAddress == NULL) {
>>> -
>>> + return EFI_INVALID_PARAMETER;
>>> }
>>>
>>> Device = VIRTIO_MMIO_DEVICE_FROM_VIRTIO_DEVICE (This);
>>> @@ -109,7 +109,9 @@ VirtioMmioSetQueueSize (
>>>
>>> Device = VIRTIO_MMIO_DEVICE_FROM_VIRTIO_DEVICE (This);
>>>
>>> - return VIRTIO_CFG_WRITE (Device, VIRTIO_MMIO_OFFSET_QUEUE_NUM,
>>> QueueSize);
>>> + VIRTIO_CFG_WRITE (Device, VIRTIO_MMIO_OFFSET_QUEUE_NUM, QueueSize);
>>> +
>>> + return EFI_SUCCESS;
>>> }
>>>
>>> EFI_STATUS
>>> @@ -169,7 +171,9 @@ VirtioMmioSetPageSize (
>>> {
>>> VIRTIO_MMIO_DEVICE *Device;
>>>
>>> - ASSERT (PageSize == EFI_PAGE_SIZE);
>>> + if (PageSize != EFI_PAGE_SIZE) {
>>> + return EFI_UNSUPPORTED;
>>> + }
>>
>> Maybe a separate change?
>
> I don't think so. The MMIO implementation of the protocol is new; the
> change you're quoting is between v4 and v5 (ie. it affects the
> original-to-be commit). The protocol member is being introduced in:
>
> [edk2] [PATCH v5 3/8] OvmfPkg/VirtioMmioDeviceLib: Implement
> VIRTIO_DEVICE_PROTOCOL for VirtIo Devices over
> MMIO
>
> and I listed the change in the v5 section there:
>
> - VirtioMmioSetPageSize(): check against EFI_PAGE_SIZE with "if" plus
> EFI_UNSUPPORTED, rather than ASSERT()
>
> I had pointed this out in my v4 review:
> http://thread.gmane.org/gmane.comp.bios.tianocore.devel/4557/focus=4711
>
> On 10/29/13 13:43, Laszlo Ersek wrote:
>> On 10/16/13 19:29, Olivier Martin wrote:
>>> +EFI_STATUS
>>> +EFIAPI
>>> +VirtioMmioSetPageSize (
>>> + VIRTIO_DEVICE_PROTOCOL *This,
>>> + UINT32 PageSize
>>> + )
>>> +{
>>> + VIRTIO_MMIO_DEVICE *Device;
>>> +
>>> + ASSERT (PageSize == EFI_PAGE_SIZE);
>>
>> I would have preferred EFI_UNSUPPORTED in place of the assert, but I
>> don't mind.
>
> Going forward,
>
>>
>>> Device = VIRTIO_MMIO_DEVICE_FROM_VIRTIO_DEVICE (This);
>>>
>>> @@ -240,10 +244,6 @@ VirtioMmioDeviceWrite (
>>> Device = VIRTIO_MMIO_DEVICE_FROM_VIRTIO_DEVICE (This);
>>>
>>> // Double-check fieldsize
>>> - if (FieldSize > 8) {
>>> - return EFI_INVALID_PARAMETER;
>>> - }
>>
>> Separate change?
>
> Same status. I listed the change broken-out in the v5 3/8 posting:
>
> - VirtioMmioDeviceWrite(), VirtioMmioDeviceRead(): remove redundant
> (FieldSize > 8) checks
>
> And I had noticed it originally in the same v4 06/11 review as linked
> above:
>
> "This inequality is now redundant, given the below [...]".
>
>>
>>> if ((FieldSize != 1) && (FieldSize != 2) &&
>>> (FieldSize != 4) && (FieldSize != 8)) {
>>> return EFI_INVALID_PARAMETER;
>>> @@ -281,10 +281,6 @@ VirtioMmioDeviceRead (
>>> ASSERT (FieldSize == BufferSize);
>>>
>>> // Double-check fieldsize
>>> - if (FieldSize > 8) {
>>> - return EFI_INVALID_PARAMETER;
>>> - }
>>
>> Separate change?
>
> Same status (noticed in v4 06/11 review, fixed for v5), please see
> above.
>
>>
>>> if ((FieldSize != 1) && (FieldSize != 2) &&
>>> (FieldSize != 4) && (FieldSize != 8)) {
>>> return EFI_INVALID_PARAMETER;
>>> diff --git a/OvmfPkg/Library/VirtioMmioDeviceLib/VirtioMmioDeviceLib.inf
>>> b/OvmfPkg/Library/VirtioMmioDeviceLib/VirtioMmioDeviceLib.inf
>>> index 126afec..2e266a9 100644
>>> --- a/OvmfPkg/Library/VirtioMmioDeviceLib/VirtioMmioDeviceLib.inf
>>> +++ b/OvmfPkg/Library/VirtioMmioDeviceLib/VirtioMmioDeviceLib.inf
>>> @@ -36,7 +36,6 @@
>>> IoLib
>>> MemoryAllocationLib
>>> UefiBootServicesTableLib
>>> - UefiDriverEntryPoint
>>> UefiLib
>>>
>>> [Protocols]
>>> diff --git a/OvmfPkg/VirtioBlkDxe/VirtioBlk.c
>>> b/OvmfPkg/VirtioBlkDxe/VirtioBlk.c
>>> index 0dcb05c..e0be7b0 100644
>>> --- a/OvmfPkg/VirtioBlkDxe/VirtioBlk.c
>>> +++ b/OvmfPkg/VirtioBlkDxe/VirtioBlk.c
>>> @@ -622,7 +622,7 @@ VirtioBlkInit (
>>> //
>>> Status = Dev->VirtIo->SetPageSize (Dev->VirtIo, EFI_PAGE_SIZE);
>>> if (EFI_ERROR (Status)) {
>>> - goto ReleaseQueue;
>>> + goto Failed;
>>> }
>>>
>>> //
>>> @@ -683,16 +683,21 @@ VirtioBlkInit (
>>> }
>>>
>>> //
>>> - // Additional steps for MMIO: align the queue appropriately, and set the
>>> size
>>> - Dev->VirtIo->SetQueueNum (Dev->VirtIo, QueueSize);
>>> + // Additional steps for MMIO: align the queue appropriately, and set the
>>> + // size. If anything fails from here on, we must release the ring
>>> resources.
>>> + //
>>> + Status = Dev->VirtIo->SetQueueNum (Dev->VirtIo, QueueSize);
>>> + if (EFI_ERROR (Status)) {
>>> + goto ReleaseQueue;
>>> + }
>>
>> Separate change, or does the protocol alter the code flow making this
>> necessary?
>
> The v4 conversion of the virtio-blk driver to the new protocol had added
> the necessary SetQueueNum() call, but it lacked correct error handling.
> I pointed this out in my v4 review:
>
> http://thread.gmane.org/gmane.comp.bios.tianocore.devel/4557/focus=4714
>
> On 10/29/13 17:28, Laszlo Ersek wrote:
>> On 10/16/13 19:29, Olivier Martin wrote:
>>> @@ -700,22 +683,31 @@ VirtioBlkInit (
>>> }
>>>
>>> //
>>> + // Additional steps for MMIO: align the queue appropriately, and set the
>>> size
>>> + Dev->VirtIo->SetQueueNum (Dev->VirtIo, QueueSize);
>>
>> (2) Please don't shirk from the error checking here! :)
>
> Again, the change you're seeing is between v4 and v5. This is a new
> function call that becomes necessary with the conversion, and v4 was
> incomplete in this regard because it only added the call but not the
> error checking.
>
>>
>>> Status = Dev->VirtIo->SetQueueAlign (Dev->VirtIo, EFI_PAGE_SIZE);
>>> if (EFI_ERROR (Status)) {
>>> goto ReleaseQueue;
>>> }
>>>
>>> //
>>> - // step 4c -- Report GPFN (guest-physical frame number) of queue. If
>>> anything
>>> - // fails from here on, we must release the ring resources.
>>> + // step 4c -- Report GPFN (guest-physical frame number) of queue.
>>> //
>>> Status = Dev->VirtIo->SetQueueAddress (Dev->VirtIo,
>>> (UINTN) Dev->Ring.Base >> EFI_PAGE_SHIFT);
>>> @@ -841,10 +846,6 @@ VirtioBlkDriverBindingStart (
>>> goto FreeVirtioBlk;
>>> }
>>>
>>> - if (Dev->VirtIo->SubSystemDeviceId != VIRTIO_SUBSYSTEM_BLOCK_DEVICE) {
>>> - return EFI_UNSUPPORTED;
>>> - }
>>> -
>>> //
>>> // VirtIo access granted, configure virtio-blk device.
>>> //
>>> diff --git a/OvmfPkg/VirtioNetDxe/DriverBinding.c
>>> b/OvmfPkg/VirtioNetDxe/DriverBinding.c
>>> index 330f1f3..93995c6 100644
>>> --- a/OvmfPkg/VirtioNetDxe/DriverBinding.c
>>> +++ b/OvmfPkg/VirtioNetDxe/DriverBinding.c
>>> @@ -49,8 +49,7 @@
>>> unused.
>>>
>>> @retval EFI_UNSUPPORTED The host doesn't supply a MAC address.
>>> - @return Status codes from
>>> Dev->VirtIo->Io.Read(),
>>> - VIRTIO_CFG_READ() and
>>> VIRTIO_CFG_WRITE().
>>> + @return Status codes from VirtIo protocol
>>> members.
>>> @retval EFI_SUCCESS Configuration values retrieved.
>>> */
>>> STATIC
>>> @@ -66,6 +65,7 @@ VirtioNetGetFeatures (
>>> EFI_STATUS Status;
>>> UINT8 NextDevStat;
>>> UINT32 Features;
>>> + UINTN MacIdx;
>>> UINT16 LinkStatus;
>>>
>>> //
>>> @@ -105,15 +105,16 @@ VirtioNetGetFeatures (
>>> Status = EFI_UNSUPPORTED;
>>> goto YieldDevice;
>>> }
>>> - Status = Dev->VirtIo->ReadDevice (Dev->VirtIo,
>>> - OFFSET_OF_VNET (Mac), // Offset
>>> - sizeof(UINT8), // FieldSize
>>> - SIZE_OF_VNET (Mac), // BufferSize
>>> - MacAddress // Buffer
>>> - );
>>> -
>>> - if (EFI_ERROR (Status)) {
>>> - goto YieldDevice;
>>> + for (MacIdx = 0; MacIdx < SIZE_OF_VNET (Mac); ++MacIdx) {
>>> + Status = Dev->VirtIo->ReadDevice (Dev->VirtIo,
>>> + OFFSET_OF_VNET (Mac) + MacIdx, // Offset
>>> + 1, // FieldSize
>>> + 1, // BufferSize
>>> + &MacAddress->Addr[MacIdx] // Buffer
>>> + );
>>> + if (EFI_ERROR (Status)) {
>>> + goto YieldDevice;
>>> + }
>>
>> Separate change? Is this related to moving to using the protocol?
>
> This is not a separate change either.
>
> I reviewed v4 07/11 in two rounds. In the second round I commented:
>
> http://thread.gmane.org/gmane.comp.bios.tianocore.devel/4557/focus=4721
>
> On 10/30/13 14:21, Laszlo Ersek wrote:
>> On 10/16/13 19:29, Olivier Martin wrote:
>
>>> @@ -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.
>
> The v4 patchset converted the reading of the MAC address from PciIo to
> VirtIo. However, the conversion is invalid for the virtio-mmio backend,
> because it tries to read 6 bytes at once.
>
> The virtio-pci backend had satisfied this request in v4, due to a
> looping kludge added there to the virtio-pci backend. But, since I fixed
> this call site in v5 in order to work with the virtio-mmio backend,
> there was no longer any need for the kludge in the PCI backend. So I
> removed the looping from the PCI backend as well.
>
>>
>>> }
>>>
>>> //
>>> @@ -459,10 +460,6 @@ VirtioNetDriverBindingStart (
>>> goto FreeVirtioNet;
>>> }
>>>
>>> - if (Dev->VirtIo->SubSystemDeviceId != VIRTIO_SUBSYSTEM_NETWORK_CARD) {
>>> - return EFI_UNSUPPORTED;
>>> - }
>>> -
>>> //
>>> // now we can run a basic one-shot virtio-net initialization required to
>>> // retrieve the MAC address
>>> diff --git a/OvmfPkg/VirtioNetDxe/SnpInitialize.c
>>> b/OvmfPkg/VirtioNetDxe/SnpInitialize.c
>>> index 56a55db..8dcf9da 100644
>>> --- a/OvmfPkg/VirtioNetDxe/SnpInitialize.c
>>> +++ b/OvmfPkg/VirtioNetDxe/SnpInitialize.c
>>> @@ -79,13 +79,33 @@ VirtioNetInitRing (
>>> }
>>>
>>> //
>>> + // Additional steps for MMIO: align the queue appropriately, and set the
>>> + // size. If anything fails from here on, we must release the ring
>>> resources.
>>> + //
>>> + Status = Dev->VirtIo->SetQueueNum (Dev->VirtIo, QueueSize);
>>> + if (EFI_ERROR (Status)) {
>>> + goto ReleaseQueue;
>>> + }
>>> +
>>> + Status = Dev->VirtIo->SetQueueAlign (Dev->VirtIo, EFI_PAGE_SIZE);
>>> + if (EFI_ERROR (Status)) {
>>> + goto ReleaseQueue;
>>> + }
>>
>> Separate change, or does the protocol alter the code flow making this
>> necessary?
>
> The latter.
>
> The conversion of VirtioNetDxe to the new protocol had been incomplete
> in v4. I observed this too in the second round of my v4 07/11 review:
>
> http://thread.gmane.org/gmane.comp.bios.tianocore.devel/4557/focus=4721
>
> On 10/30/13 14:21, Laszlo Ersek wrote:
>> On 10/16/13 19:29, Olivier Martin wrote:
>
>>> @@ -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.
>
> I just added in v5 what had been missing from v4. My v5 4/8 posting
> says:
>
> - VirtioNetInitRing(): call SetQueueNum() and SetQueueAlign() for proper
> MMIO operation
>
>>
>>> + //
>>> // step 4c -- report GPFN (guest-physical frame number) of queue
>>> //
>>> Status = Dev->VirtIo->SetQueueAddress (Dev->VirtIo,
>>> (UINTN) Ring->Base >> EFI_PAGE_SHIFT);
>>> if (EFI_ERROR (Status)) {
>>> - VirtioRingUninit (Ring);
>>> + goto ReleaseQueue;
>>> }
>>> +
>>> + return EFI_SUCCESS;
>>> +
>>> +ReleaseQueue:
>>> + VirtioRingUninit (Ring);
>>> +
>>> return Status;
>>> }
>>>
>>> @@ -382,7 +402,7 @@ VirtioNetInitialize (
>>> //
>>> Status = Dev->VirtIo->SetPageSize (Dev->VirtIo, EFI_PAGE_SIZE);
>>> if (EFI_ERROR (Status)) {
>>> - goto ReleaseTxRing;
>>> + goto DeviceFailed;
>>> }
>>>
>>> //
>>> diff --git a/OvmfPkg/VirtioPciDeviceDxe/VirtioPciDevice.c
>>> b/OvmfPkg/VirtioPciDeviceDxe/VirtioPciDevice.c
>>> index c513622..c03a273 100644
>>> --- a/OvmfPkg/VirtioPciDeviceDxe/VirtioPciDevice.c
>>> +++ b/OvmfPkg/VirtioPciDeviceDxe/VirtioPciDevice.c
>>> @@ -80,11 +80,10 @@ VirtioPciIoRead (
>>> EFI_PCI_IO_PROTOCOL_WIDTH Width;
>>> EFI_PCI_IO_PROTOCOL *PciIo;
>>>
>>> - // The BufferSize must be a multiple of FieldSize
>>> - ASSERT ((BufferSize % FieldSize) == 0);
>>> + ASSERT (FieldSize == BufferSize);
>>>
>>> PciIo = Dev->PciIo;
>>> - Count = BufferSize / FieldSize;
>>> + Count = 1;
>>
>> Separate change, or does the protocol alter the code flow making this
>> necessary?
>
> The latter. (I've touched on this above.)
>
> This looping behavior (that didn't match the leading comment block) had
> been introduced in v4, solely in order to accommodat the one-call
> reading of the virtio-net MAC address. Since that reading had been
> invalid for MMIO, and I fixed it for v5 anyway, I removed the looping
> here as well. In any case, I remarked this in my v4 review explicitly
> for the hunk in question:
>
> http://thread.gmane.org/gmane.comp.bios.tianocore.devel/4557/focus=4687
>
> On 10/28/13 16:45, Laszlo Ersek wrote:
>> On 10/16/13 19:29, Olivier Martin wrote:
>>> +/**
>>> +
>>> + 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
>>> PCI
>>> + implementation of the protocol.
>>> +
>>> + @param[in] Dev Virtio 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
>>> +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;
>>> +
>>> + // The BufferSize must be a multiple of FieldSize
>>> + ASSERT ((BufferSize % FieldSize) == 0);
>>
>> Not sure what motivated this change (and the corresponding Count
>> doubling below). It doesn't match the comment on BufferSize.
>>
>> But, I don't mind.
>
> As I described above, later on I did figure out why this was necessary
> (for the VirtioNetDxe MAC-reading). Since the latter was wrong anyway
> and I fixed it, this change in v4 lost its raison d'etre as well. My v5
> 2/8 posting says:
>
> - VirtioPciIoRead(): restore the original requirement that FieldSize equal
> BufferSize exactly (not only divide it). The looping added in v4 did not
> match the comment block, and the only place that used it in v4 (ie.
> VirtioNetGetFeatures()) needs an open-coded loop anyway (will be done in
> a later part of v5).
>
> (And it was.)
>
>>
>>> switch (FieldSize) {
>>> case 1:
>>> @@ -96,6 +95,7 @@ VirtioPciIoRead (
>>> 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():
>>> @@ -103,8 +103,9 @@ VirtioPciIoRead (
>>> // 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
>>> - Count = Count * 2;
>>> + // some platforms, width requests of EfiPciIoWidthUint64 do not work.
>>> + //
>>> + Count = 2;
>>> // fall through
>>>
>>> case 4:
>>> diff --git a/OvmfPkg/VirtioPciDeviceDxe/VirtioPciFunctions.c
>>> b/OvmfPkg/VirtioPciDeviceDxe/VirtioPciFunctions.c
>>> index 586bec6..9c40fd9 100644
>>> --- a/OvmfPkg/VirtioPciDeviceDxe/VirtioPciFunctions.c
>>> +++ b/OvmfPkg/VirtioPciDeviceDxe/VirtioPciFunctions.c
>>> @@ -24,12 +24,12 @@
>>>
>>> /**
>>>
>>> - Read a word from Region 0 of the device specified by PciIo.
>>> + Read a word from Region 0 of the device specified by VirtIo Device
>>> protocol.
>>>
>>> The function implements the ReadDevice protocol member of
>>> VIRTIO_DEVICE_PROTOCOL.
>>>
>>> - @param[in] PciIo Source PCI device.
>>> + @param[in] This VirtIo Device protocol.
>>>
>>> @param[in] FieldOffset Source offset.
>>>
>>> @@ -235,9 +235,7 @@ VirtioPciSetPageSize (
>>> UINT32 PageSize
>>> )
>>> {
>>> - ASSERT (PageSize == EFI_PAGE_SIZE);
>>> -
>>> - return EFI_SUCCESS;
>>> + return (PageSize == EFI_PAGE_SIZE) ? EFI_SUCCESS : EFI_UNSUPPORTED;
>>
>> Separate change?
>
> No, it isn't.
>
> This even dates back to my v3 review. I mentioned it again in my v4
> review:
>
> http://thread.gmane.org/gmane.comp.bios.tianocore.devel/4557/focus=4687
>
> On 10/28/13 16:45, Laszlo Ersek wrote:
>> On 10/16/13 19:29, Olivier Martin wrote:
>>> +EFI_STATUS
>>> +EFIAPI
>>> +VirtioPciSetPageSize (
>>> + VIRTIO_DEVICE_PROTOCOL *This,
>>> + UINT32 PageSize
>>> + )
>>> +{
>>> + ASSERT (PageSize == EFI_PAGE_SIZE);
>>> +
>>> + return EFI_SUCCESS;
>>> +}
>>
>> In
>> <http://thread.gmane.org/gmane.comp.bios.tianocore.devel/4373/focus=4409> I
>> suggested
>>
>> 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
>>
>> but I don't mind the ASSERT() in place of EFI_UNSUPPORTED.
>
> And my v5 2/8 changelog says:
>
> - return EFI_UNSUPPORTED instead of failed ASSERT() in
> VirtioPciSetPageSize()
>
>>
>>> }
>>>
>>> EFI_STATUS
>>> diff --git a/OvmfPkg/VirtioScsiDxe/VirtioScsi.c
>>> b/OvmfPkg/VirtioScsiDxe/VirtioScsi.c
>>> index 9702985..267f5f9 100644
>>> --- a/OvmfPkg/VirtioScsiDxe/VirtioScsi.c
>>> +++ b/OvmfPkg/VirtioScsiDxe/VirtioScsi.c
>>> @@ -50,14 +50,14 @@
>>>
>>> /**
>>>
>>> - Convenience macros to read and write region 0 IO space elements of the
>>> - virtio-scsi VirtIo device, for configuration purposes.
>>> + Convenience macros to read and write configuration elements of the
>>> + virtio-scsi VirtIo device.
>>>
>>> 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 VirtIo Device Protocol
>>> + @param[in] Dev Pointer to the VSCSI_DEV structure.
>>>
>>> @param[in] Field A field name from VSCSI_HDR, identifying the
>>> virtio-scsi
>>> configuration item to access.
>>> @@ -738,7 +738,7 @@ VirtioScsiInit (
>>> //
>>> Status = Dev->VirtIo->SetPageSize (Dev->VirtIo, EFI_PAGE_SIZE);
>>> if (EFI_ERROR (Status)) {
>>> - goto ReleaseQueue;
>>> + goto Failed;
>>> }
>>>
>>> //
>>> @@ -825,8 +825,21 @@ VirtioScsiInit (
>>> }
>>>
>>> //
>>> - // step 4c -- Report GPFN (guest-physical frame number) of queue. If
>>> anything
>>> - // fails from here on, we must release the ring resources.
>>> + // Additional steps for MMIO: align the queue appropriately, and set the
>>> + // size. If anything fails from here on, we must release the ring
>>> resources.
>>> + //
>>> + Status = Dev->VirtIo->SetQueueNum (Dev->VirtIo, QueueSize);
>>> + if (EFI_ERROR (Status)) {
>>> + goto ReleaseQueue;
>>> + }
>>> +
>>> + Status = Dev->VirtIo->SetQueueAlign (Dev->VirtIo, EFI_PAGE_SIZE);
>>> + if (EFI_ERROR (Status)) {
>>> + goto ReleaseQueue;
>>> + }
>>> +
>>> + //
>>> + // step 4c -- Report GPFN (guest-physical frame number) of queue.
>>
>> Separate change?
>
> No; please see my v4 07/11 review (round #2):
>
> http://thread.gmane.org/gmane.comp.bios.tianocore.devel/4557/focus=4721
>
> On 10/30/13 14:21, Laszlo Ersek wrote:
>> On 10/16/13 19:29, Olivier Martin wrote:
>
>>> @@ -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()).
>
> This is the same (for VirtioScsiInit) as it is for VirtioNetInitRing
> above (hence you can even find note (6) above).
>
> The v4 conversion of virtio-blk to the new protocol was mostly complete.
> The v4 conversion of both virtio-net and virtio-scsi was incomplete,
> they both lacked two MMIO-specific steps each (for which the PCI backend
> provides no-ops). I pointed out the virtio-net problem in note (6), and
> the virtio-scsi problem in note (9). I addressed these (and completed
> the conversion) for both virtio-net and virtio-scsi in v5 4/8, and the
> changelog says there
>
> - VirtioScsiInit(): call SetQueueNum() and SetQueueAlign() for proper MMIO
> operation
>
>> -Jordan
>
> Again, each one of my changes in v5 has been made in response to an
> explicit remark made in the v4 (or an earlier) review.
>
> Thanks,
> Laszlo
------------------------------------------------------------------------------
Rapidly troubleshoot problems before they affect your business. Most IT
organizations don't have a clear picture of how application performance
affects their revenue. With AppDynamics, you get 100% visibility into your
Java,.NET, & PHP application. Start your 15-day FREE TRIAL of AppDynamics Pro!
http://pubads.g.doubleclick.net/gampad/clk?id=84349831&iu=/4140/ostg.clktrk
_______________________________________________
edk2-devel mailing list
edk2-devel@lists.sourceforge.net
https://lists.sourceforge.net/lists/listinfo/edk2-devel
