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