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 14:20, Liran Alon wrote:
> 
> 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.

No, I'm fine with your reasoning.

Thanks,
Laszlo