Re: [edk2-devel] [PATCH v2 15/17] OvmfPkg/PvScsiDxe: Support sending SCSI request and receive response

["Liran Alon" <liran.alon@oracle.com>]

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