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

["Laszlo Ersek" <lersek@redhat.com>]

On 03/27/20 23: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.

I agree there is a bug in the UEFI spec (but I do think the intent is
mostly clear). More below.

> 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.

My understanding from the spec is that EFI_BAD_BUFFER_SIZE is for the
case when the driver can determine up-front -- without talking to the
device for the specific request's sake -- that the request is over-sized.

Otherwise (= it makes sense to submit the request to the device), a
short transfer is normal / expected. If there is no other issue with the
transfer, then InTransferLength / OutTransferLength should be updated on
output, and EFI_SUCCESS should be returned.

> 
> 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).

This is what I wrote, verbatim:

'(9) Thus I feel we should use a "break" here.'

Note: "feel".

VirtioScsiDxe returns EFI_SUCCESS (and modifies
InTransferLength/OutTransferLength, per "residual") when the device
reports VIRTIO_SCSI_S_OK. That's a top-level success report from the device.

PVSCSI does not return a top-level success code in this case
(apparently). I have never before worked with PVSCSI, and I don't know
what exactly its return codes stand for. If "PvScsiBtStatDatarun" and
"PvScsiBtStatDataUnderrun" mean "normal partial transfer", then I fully
agree with you that EFI_DEVICE_ERROR would not be correct.

> 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().

If you look at the individual "InTransferLength" and "OutTransferLength"
parameter (i.e., Packet member) specifications, in the spec (not the
header file), they do clarify "on input" vs. "on output". They make
sense to me, in connection with the EFI_SUCCESS return status.

(There *is* a bug, IMO, in the documentation of EFI_BAD_BUFFER_SIZE, in
the spec.)

Quote:

----v----
InTransferLength  On Input, the size, in bytes, of InDataBuffer. On
                  output, the number of bytes transferred between the
                  SCSI controller and the SCSI device. If
                  InTransferLength is larger than the SCSI controller
                  can handle, no data will be transferred,
                  InTransferLength will be updated to contain the number
                  of bytes that the SCSI controller is able to transfer,
                  and EFI_BAD_BUFFER_SIZE will be returned.

OutTransferLength On Input, the size, in bytes of OutDataBuffer. On
                  Output, the Number of bytes transferred between SCSI
                  Controller and the SCSI device. If OutTransferLength
                  is larger than the SCSI controller can handle, no data
                  will be transferred, OutTransferLength will be updated
                  to contain the number of bytes that the SCSI
                  controller is able to transfer, and
                  EFI_BAD_BUFFER_SIZE will be returned.

If the data buffer described by InDataBuffer and InTransferLength is too
big to be transferred in a single command, then no data is transferred
and EFI_BAD_BUFFER_SIZE is returned. The number of bytes that can be
transferred in a single command are returned in InTransferLength.

If the data buffer described by OutDataBuffer and OutTransferLength is
too big to be transferred in a single command, then no data is
transferred and EFI_BAD_BUFFER_SIZE is returned. The number of bytes
that can be transferred in a single command are returned in
OutTransferLength.

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. See HostAdapterStatus,
                  TargetStatus, SenseDataLength, and SenseData in that
                  order for additional status information.

EFI_BAD_BUFFER_SIZE The SCSI Request Packet was not executed. The number
                    of bytes that could be transferred is returned in
                    InTransferLength. For write and bi-directional
                    commands, OutTransferLength bytes were transferred
                    by OutDataBuffer. See HostAdapterStatus,
                    TargetStatus, and in that order for additional
                    status information.
----^----

The spec bug (IMO) is the penultimate sentence of the
"EFI_BAD_BUFFER_SIZE" documentation -- "were transferred" seems to make
no sense, given that "The SCSI Request Packet was not executed".

Regarding why VirtioScsiDxe sets HostAdapterStatus to
EFI_EXT_SCSI_STATUS_HOST_ADAPTER_OK when reporting an (otherwise
successful) short transfer: I can list two reasons.

(1) Paolo had reviewed this code at least on two separate occasions --
once in October 2012 and another time in September 2015.

(2) To me, EFI_EXT_SCSI_STATUS_HOST_ADAPTER_DATA_OVERRUN_UNDERRUN seemed
much closer to VIRTIO_SCSI_S_OVERRUN, which -- per virtio spec -- stands
for something very different:

  VIRTIO_SCSI_S_OVERRUN if the content of the CDB (such as the
                        allocation length, parameter length or transfer
                        size) requires more data than is available in
                        the datain and dataout buffers.

Given that this part of VirtioScsiDxe has not caused any problems in the
past 7.5 years (it goes back to commit 37078a63b1911), i.e. setting
EFI_EXT_SCSI_STATUS_HOST_ADAPTER_OK on a partial (but otherwise OK)
transfer, I would like to change it (to what?) only if:

- Paolo recommends that too,

- there is an actual industry standard or spec that indicates the
current host status is wrong.


To summarize my (updated) opinion on your handling of
PvScsiBtStatDatarun / PvScsiBtStatDataUnderrun:

- I don't know what the HostAdapterStatus should be (on output), or
whether you should report EFI_SUCCESS vs. EFI_DEVICE_ERROR. (If those
PVSCSI response codes stand for "normal, just short, transfer", then I
would suggest to follow VirtioScsiDxe's handling -- but I don't insist
at all.)

- Either way, I am quite convinced that returning EFI_BAD_BUFFER_SIZE is
wrong, because the packet *was* submitted to the device.

> 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.

Great example!

And yes, I do feel that returning EFI_BAD_BUFFER_SIZE, after updating
InTransferLength / OutTransferLength from a field called
"ResidualCount", is wrong. "ResidualCount" is coming back in a response
structure, and its name ("residual") indicates the device transfer was
actually initiated on the remote end.

To me EFI_BAD_BUFFER_SIZE just has a different "spirit". It generally
means "I couldn't begin to act upon the request due to the wrong buffer
size. Adjust the buffer size, and try again".

A partial / short transfer is different from that.

> What do you think? Do you have any further insights? This is quite
> confusing to me.

I agree that it's very confusing.

I'm not sure if this is covered by the UEFI SCT (self conformance test).

Thanks,
Laszlo