[edk2-devel] [PATCH v2 15/17] OvmfPkg/PvScsiDxe: Support sending SCSI request and receive response
Liran Alon
liran.alon at oracle.com
Sat Mar 28 19:18:47 UTC 2020
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/1813853
Group Owner: devel+owner at edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/unsub [edk2-devel-archive at redhat.com]
-=-=-=-=-=-=-=-=-=-=-=-
More information about the edk2-devel-archive
mailing list