From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from userp2130.oracle.com (userp2130.oracle.com [156.151.31.86]) by mx.groups.io with SMTP id smtpd.web11.13240.1585423135310407133 for ; Sat, 28 Mar 2020 12:18:55 -0700 Authentication-Results: mx.groups.io; dkim=fail reason="body hash did not verify" header.i=@oracle.com header.s=corp-2020-01-29 header.b=RLKyCX4i; spf=pass (domain: oracle.com, ip: 156.151.31.86, mailfrom: liran.alon@oracle.com) Received: from pps.filterd (userp2130.oracle.com [127.0.0.1]) by userp2130.oracle.com (8.16.0.42/8.16.0.42) with SMTP id 02SJDo4B016708; Sat, 28 Mar 2020 19:18:54 GMT DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=oracle.com; h=subject : from : to : cc : references : message-id : date : mime-version : in-reply-to : content-type : content-transfer-encoding; s=corp-2020-01-29; bh=Ao6RcQB8/OBcWtZOL60R8Kyk0DoBe8YJW68XtFvurgU=; b=RLKyCX4itQoeyDY5UF8z0EG9PIRN9niIeOrlMQB7OWtXKdDcgRS7hcTcHjHlzXeS5kkJ AFrptXYyJeAXnlBgDLOHTHMfdrcBqvf9V6afUuNlrujACn92m5JmY7T9Y1hnz8WWhMYH V7rwQ0zAWiOtdz/Fp2wh+/NJl3WK6+6wt8icavOtuw+3YO019MbKcDsKgEtiYlMqfG1n QrhwrtWejFMfm6al1MZRJLoKVHnlDtGsHTL3MwDm1hp/Atuzmw1r89lNOcSp98154ENs NTTcA96Rmvh5u4KDM4iWohAQ0f+D7FCPjGHWKHec2BMTPhDQEGB6J6uyN2dyQH8RQ/SV CA== Received: from userp3030.oracle.com (userp3030.oracle.com [156.151.31.80]) by userp2130.oracle.com with ESMTP id 301x0qsgj2-1 (version=TLSv1.2 cipher=ECDHE-RSA-AES256-GCM-SHA384 bits=256 verify=OK); Sat, 28 Mar 2020 19:18:54 +0000 Received: from pps.filterd (userp3030.oracle.com [127.0.0.1]) by userp3030.oracle.com (8.16.0.42/8.16.0.42) with SMTP id 02SJCdYs030472; Sat, 28 Mar 2020 19:18:54 GMT Received: from aserv0121.oracle.com (aserv0121.oracle.com [141.146.126.235]) by userp3030.oracle.com with ESMTP id 301ups51qn-1 (version=TLSv1.2 cipher=ECDHE-RSA-AES256-GCM-SHA384 bits=256 verify=OK); Sat, 28 Mar 2020 19:18:53 +0000 Received: from abhmp0013.oracle.com (abhmp0013.oracle.com [141.146.116.19]) by aserv0121.oracle.com (8.14.4/8.13.8) with ESMTP id 02SJIqVP010308; Sat, 28 Mar 2020 19:18:52 GMT Received: from Lirans-MacBook-Pro.local (/213.57.127.2) by default (Oracle Beehive Gateway v4.0) with ESMTP ; Sat, 28 Mar 2020 12:18:51 -0700 Subject: Re: [edk2-devel] [PATCH v2 15/17] OvmfPkg/PvScsiDxe: Support sending SCSI request and receive response From: "Liran Alon" To: Laszlo Ersek , devel@edk2.groups.io Cc: nikita.leshchenko@oracle.com, aaron.young@oracle.com, jordan.l.justen@intel.com, ard.biesheuvel@linaro.org References: <20200325161005.16743-1-liran.alon@oracle.com> <20200325161005.16743-16-liran.alon@oracle.com> <6f32838a-d136-050e-7a05-f817da7954a8@oracle.com> <65eb2511-e2f5-d2b5-4470-278b06a0e077@oracle.com> Message-ID: <9bbdb66f-9fdb-bd5a-74b8-6dc31015dc8a@oracle.com> Date: Sat, 28 Mar 2020 22:18:47 +0300 User-Agent: Mozilla/5.0 (Macintosh; Intel Mac OS X 10.13; rv:68.0) Gecko/20100101 Thunderbird/68.6.0 MIME-Version: 1.0 In-Reply-To: <65eb2511-e2f5-d2b5-4470-278b06a0e077@oracle.com> X-Proofpoint-Virus-Version: vendor=nai engine=6000 definitions=9574 signatures=668685 X-Proofpoint-Spam-Details: rule=notspam policy=default score=0 suspectscore=0 mlxscore=0 malwarescore=0 phishscore=0 mlxlogscore=999 adultscore=0 spamscore=0 bulkscore=0 classifier=spam adjust=0 reason=mlx scancount=1 engine=8.12.0-2003020000 definitions=main-2003280184 X-Proofpoint-Virus-Version: vendor=nai engine=6000 definitions=9574 signatures=668685 X-Proofpoint-Spam-Details: rule=notspam policy=default score=0 suspectscore=0 malwarescore=0 clxscore=1015 adultscore=0 mlxscore=0 priorityscore=1501 lowpriorityscore=0 mlxlogscore=999 bulkscore=0 phishscore=0 spamscore=0 impostorscore=0 classifier=spam adjust=0 reason=mlx scancount=1 engine=8.12.0-2003020000 definitions=main-2003280184 X-MIME-Autoconverted: from 8bit to quoted-printable by userp2130.oracle.com id 02SJDo4B016708 Content-Type: text/plain; charset=windows-1252; format=flowed Content-Language: en-US Content-Transfer-Encoding: quoted-printable 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 =3D >>>>>> + EFI_EXT_SCSI_STATUS_HOST_ADAPTER_DATA_OVERRUN_UNDERRUN; >>>>>> +=A0=A0=A0=A0=A0 return EFI_BAD_BUFFER_SIZE; >>>>> I think EFI_BAD_BUFFER_SIZE is invalid here. According to the UEFI=20 >>>>> 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=20 >>>> EFI_SCSI_PASS_THRU_PASSTHRU >>>> in MdePkg/Include/Protocol/ScsiPassThru.h: >>>> >>>> =A0=A0 @retval EFI_BAD_BUFFER_SIZE=A0=A0=A0=A0=A0=A0 The SCSI Reques= t Packet was >>>> executed, but the >>>> =A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0= =A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0 entire DataBuffer could not be >>>> transferred. >>>> =A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0= =A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0 The actual number of bytes >>>> transferred is returned >>>> =A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0= =A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0 in TransferLength. See >>>> HostAdapterStatus, >>>> =A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0= =A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0 TargetStatus, SenseDataLength,=20 >>>> and >>>> SenseData in >>>> =A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0= =A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0 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 >>> >>> =A0=A0 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 >>> >> > 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=20 >> ScsiPassThruExt.h header file. It doesn't make sense. >> In the new header file, there is no way for PassThru() to indicate to=20 >> caller that a partial data-transfer have occurred. >> Which seems to be a quite standard functionality. Almost all SCSI=20 >> devices are able to return information on partial data-transfer=20 >> completions. >> >> Looking at QEMU's virtio-scsi.c virtio_scsi_command_complete()=20 >> implementation, we can see how it handles partial data-transfer=20 >> completions. >> It sets Response->Response to VIRTIO_SCSI_S_OK and Response->Residual=20 >> accordingly. >> Looking at EDK2 VirtioScsi driver, function ParseResponse(), this=20 >> will be handled by updating Packet->InTransferLength and=20 >> Packet->OutTransferLength based on Response->Residual, >> but then setting Packet->HostAdapterStatus to=20 >> EFI_EXT_SCSI_STATUS_HOST_ADAPTER_OK and returning EFI_SUCCESS. >> (In contrast to your suggestion for me to return EFI_DEVICE_ERROR in=20 >> this case). >> >> The documentation of PassThru in ScsiPassThruExt.h for EFI_SUCCESS=20 >> specifies: >> >> =A0 @retval EFI_SUCCESS=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0 The SCSI Request= Packet was sent by=20 >> the host. For bi-directional >> =A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0= =A0=A0=A0=A0=A0=A0=A0 commands, InTransferLength bytes were=20 >> transferred from >> =A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0= =A0=A0=A0=A0=A0=A0=A0 InDataBuffer. For write and=20 >> bi-directional commands, >> =A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0= =A0=A0=A0=A0=A0=A0=A0 OutTransferLength bytes were=20 >> transferred by >> =A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0= =A0=A0=A0=A0=A0=A0=A0 OutDataBuffer. >> >> But it seems to reference the InTransferLength and OutTransferLength=20 >> *before* the call to PassThru(). >> >> In contrast to old header file, ScsiPassThru.h, which for=20 >> EFI_BAD_BUFFER_SIZE specifies explicitly: >> >> =A0@retval EFI_BAD_BUFFER_SIZE=A0=A0=A0=A0=A0=A0 The SCSI Request Pack= et was=20 >> executed, but the >> =A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0= =A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0 entire DataBuffer could not be=20 >> transferred. >> =A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0= =A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0 The actual number of bytes=20 >> transferred is returned >> =A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0= =A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0 in TransferLength. See=20 >> HostAdapterStatus, >> =A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0= =A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0 TargetStatus, SenseDataLength,=20 >> and SenseData in >> =A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0= =A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0 that order for additional status=20 >> information. >> >> In addition, looking at EDK2 iSCSI ScsiPassThru driver=20 >> (NetworkPkg/IScsiDxe/IScsiExtScsiPassThru.c), one can see it use=20 >> EFI_EXT_SCSI_PASS_THRU_SCSI_REQUEST_PACKET >> (Which matches the new header file ScsiPassThruExt.h and not the old=20 >> ScsiPassThru.h). >> If you would have a look at how IScsiOnScsiRspRcvd() handles=20 >> SCSI_RSP_PDU_FLAG_BI_READ_OVERFLOW and SCSI_RSP_PDU_FLAG_OVERFLOW=20 >> bits, it seems to always >> set Status to EFI_BAD_BUFFER_SIZE as I have done. After updating=20 >> Packet->InTransferLength / Packet->OutTransferLength. >> >> What do you think? Do you have any further insights? This is quite=20 >> confusing to me. >> >> -Liran >> > Furthermore, I have later found ScsiExecuteSCSICommand() in=20 > MdeModulePkg/Bus/Scsi/ScsiBusDxe/ScsiBus.c to be the one that calls=20 > the PassThru() method. > Looking at it's "if (ScsiIoDevice->ExtScsiSupport)" branch (Which is=20 > relevant to us), one can see it just simply executes the PassThru()=20 > device and returns. > Examining ScsiExecuteSCSICommand() documentation specifies for=20 > EFI_BAD_BUFFER_SIZE: > > =A0 @retval EFI_BAD_BUFFER_SIZE The SCSI Request Packet was executed, > =A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0= =A0=A0=A0=A0=A0 but the entire DataBuffer could not be=20 > transferred. > =A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0= =A0=A0=A0=A0=A0 The actual number of bytes transferred=20 > is returned > =A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0= =A0=A0=A0=A0=A0 in TransferLength. See HostAdapterStatus, > =A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0= =A0=A0=A0=A0=A0 TargetStatus, SenseDataLength, and=20 > SenseData in > =A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0= =A0=A0=A0=A0=A0 that order for additional status=20 > 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=20 ExtScsiPassThru->PassThru() and returns it's return value. * ScsiWrite16Command() always updates DataLength with=20 Packet->OutTransferLength and returns ScsiExecuteSCSICommand() return val= ue. * In ScsiDiskWrite16(): =A0 ** If EFI_BAD_BUFFER_SIZE is returned, NeedRetry is set to TRUE and=20 EFI_DEVICE_ERROR is returned. =A0 ** Otherwise, if EFI_SUCCESS is returned but HostAdapterStatus is se= t=20 to EFI_EXT_SCSI_STATUS_HOST_ADAPTER_DATA_OVERRUN_UNDERRUN, then again=20 NeedRetry is set to TRUE and EFI_DEVICE_ERROR is returned. =A0 ** Otherwise, just return EFI_SUCCESS to caller. * In ScsiDiskWriteSectors() we finally see the logic we were looking for: =A0 ** If ScsiDiskWrite16() returned EFI_SUCCESS, then we break out of=20 retry loop and indeed take into account ByteCount. (Which was updated=20 from Packet->OutTransferLength). =A0 ** Otherwise, (e.g. EFI_BAD_BUFFER_SIZE was returned), if NeedRetry=20 is set to TRUE, then next request retry is updated to be done with=20 ByteCount (Which was updated from Packet->OutTransferLength). So it seems the UEFI-2.8 spec is right and a lot of EDK2 function=20 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=20 too big for DMA communication buffer Data field, we update=20 TransferLength and return EFI_BAD_BUFFER_SIZE. =A0=A0=A0 But in addition, for completion, we should also set=20 HostAdapterStatus to=20 EFI_EXT_SCSI_STATUS_HOST_ADAPTER_DATA_OVERRUN_UNDERRUN and TargetStatus=20 to EFI_EXT_SCSI_STATUS_TARGET_GOOD =A0=A0=A0 and SenseDataLength to 0 as done in VirtioScsi.c PopulateReque= st()=20 in case it returns EFI_BAD_BUFFER_SIZE. 2) Change PvScsi.c HandleResponse() such that: =A0 ** If device returned PvScsiBtStatDataUnderrun: Update TransferLengt= h=20 with Response->DataLen, set HostAdapterStatus to=20 EFI_EXT_SCSI_STATUS_HOST_ADAPTER_DATA_OVERRUN_UNDERRUN and return=20 EFI_SUCCESS. =A0 ** If device returned PvScsiBtStatDatarun: Just set HostAdapterStatu= s=20 to EFI_EXT_SCSI_STATUS_HOST_ADAPTER_DATA_OVERRUN_UNDERRUN and return=20 EFI_SUCCESS. Regards, -Liran