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