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

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

On 27/03/2020 16:04, Liran Alon wrote:
>
> On 27/03/2020 14:26, Laszlo Ersek wrote:
>> On 03/25/20 17:10, Liran Alon wrote:
>>
>>> +
>>> +  //
>>> +  // Report target status
>>> +  //
>>> +  Packet->TargetStatus = Response->ScsiStatus;
>>> +
>>> +  //
>>> +  // Host adapter status and function return value depend on
>>> +  // device response's host status
>>> +  //
>>> +  switch (Response->HostStatus) {
>>> +    case PvScsiBtStatSuccess:
>>> +    case PvScsiBtStatLinkedCommandCompleted:
>>> +    case PvScsiBtStatLinkedCommandCompletedWithFlag:
>>> +      Packet->HostAdapterStatus = EFI_EXT_SCSI_STATUS_HOST_ADAPTER_OK;
>>> +      return EFI_SUCCESS;
>>> +
>>> +    case PvScsiBtStatSelTimeout:
>>> +      Packet->HostAdapterStatus =
>>> + EFI_EXT_SCSI_STATUS_HOST_ADAPTER_SELECTION_TIMEOUT;
>>> +      return EFI_TIMEOUT;
>>> +
>>> +    case PvScsiBtStatDatarun:
>>> +    case :
>>> +      //
>>> +      // Report residual data in overrun/underrun
>>> +      //
>>> +      if (Packet->DataDirection == EFI_EXT_SCSI_DATA_DIRECTION_READ) {
>>> +        Packet->InTransferLength = Response->DataLen;
>>> +      } else {
>>> +        Packet->OutTransferLength = Response->DataLen;
>>> +      }
>> OK, if we are sure that (a) the device will always report short
>> reads/writes like this, and that (b) the above assignments will never
>> cause InTransferLength / OutTransferLength to *grow*, then the
>> InTransferLength / OutTransferLength adjustments are sufficiently
>> covered.
> I believe both of these are indeed true.
> Even though that current QEMU VMware PVSCSI device emulation code have 
> a bug that it never sets this in pvscsi_command_complete() when it 
> does set BTSTAT_DATARUN...
>> Still:
>>
>> (8) The CopyMem() call above should not copy garbage (at the tail).
> I don't think it matters. We don't guarantee anything on the content 
> in Packet->InDataBuffer beyond Packet->InTransferLength.
> I think the code is simpler how it is currently written.
>>
>> Honestly, *if* the PVSCSI device model always sets "Response->DataLen",
> I don't think this is the case.
>> then I would prefer if:
>>
>> - we always updated InTransferLength / OutTransferLength (regardless of
>> "Response->HostStatus"),
>>
>> - and we only used these case labels (PvScsiBtStatDatarun /
>> PvScsiBtStatDataUnderrun) for setting "Packet->HostAdapterStatus".

Regarding all the above:
You can also see that Linux PVSCSI driver (drivers/scsi/vmw_pvscsi.c) 
reads the "DataLen" field only in case the "HostStatus" is 
BTSTAT_DATARUN or BTSTAT_DATA_UNDERRUN.
As I have done in my driver. In the lack of more detailed device 
specification (As PVSCSI is a proprietary VMware PV device), I prefer to 
remain with my implementation which seems
to be guaranteed to be safe and working. Please tell me if you think 
otherwise.

-Liran