On 28/03/2020 1:17, Liran Alon wrote:
On 28/03/2020 1:04, Liran Alon wrote:
On 28/03/2020 0:05, Laszlo Ersek wrote:
On 03/27/20 14:04, Liran Alon wrote:
On 27/03/2020 14:26, Laszlo Ersek wrote:
On 03/25/20 17:10, Liran Alon wrote:
+ Packet->HostAdapterStatus =
+ EFI_EXT_SCSI_STATUS_HOST_ADAPTER_DATA_OVERRUN_UNDERRUN;
+ return EFI_BAD_BUFFER_SIZE;
I think EFI_BAD_BUFFER_SIZE is invalid here. According to the UEFI
spec,
EFI_BAD_BUFFER_SIZE means "The SCSI Request Packet was not executed".
But that's not the case here -- we do have a partially completed
transfer.
Hmm... According to the documentation above
EFI_SCSI_PASS_THRU_PASSTHRU
in MdePkg/Include/Protocol/ScsiPassThru.h:
@retval EFI_BAD_BUFFER_SIZE The SCSI Request Packet was
executed, but the
entire DataBuffer could not be
transferred.
The actual number of bytes
transferred is returned
in TransferLength. See
HostAdapterStatus,
TargetStatus, SenseDataLength,
and
SenseData in
that order for additional status
information.
So I don't know who to believe... It does seem to me that this
documentation in the code makes more sense
and then my current code is correct. What do you think?
You are looking at the wrong protocol header file.
Oops. You are right.
The top of this
header file bears the comment
SCSI Pass Through protocol as defined in EFI 1.1.
and the UEFI-2.8 spec does not define EFI_SCSI_PASS_THRU_PROTOCOL; it
only refers to Mantis ticket 845
<https://urldefense.com/v3/__https://mantis.uefi.org/mantis/view.php?id=845__;!!GqivPVa7Brio!M5o2BC3kQvh7gEgQh98Kb7YJeFCM_ajknF8iRXjDer3Ir8hUy6dHOReS12oCRvE$
> with subject
"EFI_SCSI_PASS_THRU_PROTOCOL replacement".
Instead, please consult EFI_EXT_SCSI_PASS_THRU_PASSTHRU in
"MdePkg/Include/Protocol/ScsiPassThruExt.h". There, the
EFI_BAD_BUFFER_SIZE return value conforms to the UEFI 2.8 spec ("The
SCSI Request Packet was not executed").
Honestly, I'm not sure if this is not a bug in UEFI-2.8 spec and
ScsiPassThruExt.h header file. It doesn't make sense.
In the new header file, there is no way for PassThru() to indicate to
caller that a partial data-transfer have occurred.
Which seems to be a quite standard functionality. Almost all SCSI
devices are able to return information on partial data-transfer
completions.
Looking at QEMU's virtio-scsi.c virtio_scsi_command_complete()
implementation, we can see how it handles partial data-transfer
completions.
It sets Response->Response to VIRTIO_SCSI_S_OK and Response->Residual
accordingly.
Looking at EDK2 VirtioScsi driver, function ParseResponse(), this
will be handled by updating Packet->InTransferLength and
Packet->OutTransferLength based on Response->Residual,
but then setting Packet->HostAdapterStatus to
EFI_EXT_SCSI_STATUS_HOST_ADAPTER_OK and returning EFI_SUCCESS.
(In contrast to your suggestion for me to return EFI_DEVICE_ERROR in
this case).
The documentation of PassThru in ScsiPassThruExt.h for EFI_SUCCESS
specifies:
@retval EFI_SUCCESS The SCSI Request Packet was sent by
the host. For bi-directional
commands, InTransferLength bytes were
transferred from
InDataBuffer. For write and
bi-directional commands,
OutTransferLength bytes were
transferred by
OutDataBuffer.
But it seems to reference the InTransferLength and OutTransferLength
*before* the call to PassThru().
In contrast to old header file, ScsiPassThru.h, which for
EFI_BAD_BUFFER_SIZE specifies explicitly:
@retval EFI_BAD_BUFFER_SIZE The SCSI Request Packet was
executed, but the
entire DataBuffer could not be
transferred.
The actual number of bytes
transferred is returned
in TransferLength. See
HostAdapterStatus,
TargetStatus, SenseDataLength,
and SenseData in
that order for additional status
information.
In addition, looking at EDK2 iSCSI ScsiPassThru driver
(NetworkPkg/IScsiDxe/IScsiExtScsiPassThru.c), one can see it use
EFI_EXT_SCSI_PASS_THRU_SCSI_REQUEST_PACKET
(Which matches the new header file ScsiPassThruExt.h and not the old
ScsiPassThru.h).
If you would have a look at how IScsiOnScsiRspRcvd() handles
SCSI_RSP_PDU_FLAG_BI_READ_OVERFLOW and SCSI_RSP_PDU_FLAG_OVERFLOW
bits, it seems to always
set Status to EFI_BAD_BUFFER_SIZE as I have done. After updating
Packet->InTransferLength / Packet->OutTransferLength.
What do you think? Do you have any further insights? This is quite
confusing to me.
-Liran
Furthermore, I have later found ScsiExecuteSCSICommand() in
MdeModulePkg/Bus/Scsi/ScsiBusDxe/ScsiBus.c to be the one that calls
the PassThru() method.
Looking at it's "if (ScsiIoDevice->ExtScsiSupport)" branch (Which is
relevant to us), one can see it just simply executes the PassThru()
device and returns.
Examining ScsiExecuteSCSICommand() documentation specifies for
EFI_BAD_BUFFER_SIZE:
@retval EFI_BAD_BUFFER_SIZE The SCSI Request Packet was executed,
but the entire DataBuffer could not be
transferred.
The actual number of bytes transferred
is returned
in TransferLength. See HostAdapterStatus,
TargetStatus, SenseDataLength, and
SenseData in
that order for additional status
information.
Which again seems to align to what I have done in my PvScsi driver...
-Liran
Sorry for the spam but I think I eventually figured it out.
The call-chain in EDK2 looks like this:
ScsiDiskWriteSectors() (MdeModulePkg/Bus/Scsi/ScsiDiskDxe/ScsiDisk.c)
ScsiDiskWrite16() (MdeModulePkg/Bus/Scsi/ScsiDiskDxe/ScsiDisk.c)
ScsiWrite16Command() (MdePkg/Library/UefiScsiLib/UefiScsiLib.c)
ScsiExecuteSCSICommand() (MdeModulePkg/Bus/Scsi/ScsiBusDxe/ScsiBus.c)
ExtScsiPassThru->PassThru()
It can be seen that:
* ScsiExecuteSCSICommand() just passes Packet to
ExtScsiPassThru->PassThru() and returns it's return value.
* ScsiWrite16Command() always updates DataLength with
Packet->OutTransferLength and returns ScsiExecuteSCSICommand() return value.
* In ScsiDiskWrite16():
** If EFI_BAD_BUFFER_SIZE is returned, NeedRetry is set to TRUE and
EFI_DEVICE_ERROR is returned.
** Otherwise, if EFI_SUCCESS is returned but HostAdapterStatus is set
to EFI_EXT_SCSI_STATUS_HOST_ADAPTER_DATA_OVERRUN_UNDERRUN, then again
NeedRetry is set to TRUE and EFI_DEVICE_ERROR is returned.
** Otherwise, just return EFI_SUCCESS to caller.
* In ScsiDiskWriteSectors() we finally see the logic we were looking for:
** If ScsiDiskWrite16() returned EFI_SUCCESS, then we break out of
retry loop and indeed take into account ByteCount. (Which was updated
from Packet->OutTransferLength).
** Otherwise, (e.g. EFI_BAD_BUFFER_SIZE was returned), if NeedRetry
is set to TRUE, then next request retry is updated to be done with
ByteCount (Which was updated from Packet->OutTransferLength).
So it seems the UEFI-2.8 spec is right and a lot of EDK2 function
documentation is out-of-date.
Therefore, I will update my code accordingly. i.e.:
1) Change PvScsi.c PopulateRequest() such that in case TransferLength is
too big for DMA communication buffer Data field, we update
TransferLength and return EFI_BAD_BUFFER_SIZE.
But in addition, for completion, we should also set
HostAdapterStatus to
EFI_EXT_SCSI_STATUS_HOST_ADAPTER_DATA_OVERRUN_UNDERRUN and TargetStatus
to EFI_EXT_SCSI_STATUS_TARGET_GOOD
and SenseDataLength to 0 as done in VirtioScsi.c PopulateRequest()
in case it returns EFI_BAD_BUFFER_SIZE.
2) Change PvScsi.c HandleResponse() such that:
** If device returned PvScsiBtStatDataUnderrun: Update TransferLength
with Response->DataLen, set HostAdapterStatus to
EFI_EXT_SCSI_STATUS_HOST_ADAPTER_DATA_OVERRUN_UNDERRUN and return
EFI_SUCCESS.
** If device returned PvScsiBtStatDatarun: Just set HostAdapterStatus
to EFI_EXT_SCSI_STATUS_HOST_ADAPTER_DATA_OVERRUN_UNDERRUN and return
EFI_SUCCESS.
Regards,
-Liran
-=-=-=-=-=-=-=-=-=-=-=-
Groups.io Links: You receive all messages sent to this group.
View/Reply Online (#56531): https://edk2.groups.io/g/devel/message/56531
Mute This Topic: https://groups.io/mt/72544127/21656
Group Owner: devel+ow...@edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/unsub [arch...@mail-archive.com]
-=-=-=-=-=-=-=-=-=-=-=-
