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
-=-=-=-=-=-=-=-=-=-=-=-
Groups.io Links: You receive all messages sent to this group.
View/Reply Online (#56510): https://edk2.groups.io/g/devel/message/56510
Mute This Topic: https://groups.io/mt/72544127/21656
Group Owner: devel+ow...@edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/unsub [arch...@mail-archive.com]
-=-=-=-=-=-=-=-=-=-=-=-
