From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from userp2120.oracle.com (userp2120.oracle.com [156.151.31.85]) by mx.groups.io with SMTP id smtpd.web12.410.1585347441948033756 for ; Fri, 27 Mar 2020 15:17:22 -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=nb8xfao9; spf=pass (domain: oracle.com, ip: 156.151.31.85, mailfrom: liran.alon@oracle.com) Received: from pps.filterd (userp2120.oracle.com [127.0.0.1]) by userp2120.oracle.com (8.16.0.42/8.16.0.42) with SMTP id 02RMEPWH045733; Fri, 27 Mar 2020 22:17:21 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=Yfstr+HNTV1s5+rFU49kmNK8pmsUzDvdFNtzfqVW+Rw=; b=nb8xfao9+7n27PX5mSO86KeZxtMd4DIoQK+MDU3C6H9SlM9U4YZN03rJhMUyCnISoE/t 28tkPOlDLuRH8zWkkD9nceYx0c6h0nTz7G+yyBmivagKVfgdf2fMNhTHMGqYVKp4sLhB bsuTHhmWuNRgShreJ4ZPsMqJ8Fjz0ZjSw6yXyXzrwN4nut61L8dyH/T2luLsYj3sH8Hn MXqImwYaRtiBsK1zgNBdvReyGT4IRSea8oe94qyFF5ovyJpUN0UMnpaPR4jRySQVbKqq 0oeJ+wOdFVZ4XH4xV+ipSv8QkmDOeU54ApGlo8tuFw8CMz7Fv6DTtvNPzCK6VWCDAW5k rQ== Received: from userp3030.oracle.com (userp3030.oracle.com [156.151.31.80]) by userp2120.oracle.com with ESMTP id 3019vecn1u-1 (version=TLSv1.2 cipher=ECDHE-RSA-AES256-GCM-SHA384 bits=256 verify=OK); Fri, 27 Mar 2020 22:17:21 +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 02RMCwb8014405; Fri, 27 Mar 2020 22:17:20 GMT Received: from aserv0122.oracle.com (aserv0122.oracle.com [141.146.126.236]) by userp3030.oracle.com with ESMTP id 2yxw4wpxge-1 (version=TLSv1.2 cipher=ECDHE-RSA-AES256-GCM-SHA384 bits=256 verify=OK); Fri, 27 Mar 2020 22:17:20 +0000 Received: from abhmp0005.oracle.com (abhmp0005.oracle.com [141.146.116.11]) by aserv0122.oracle.com (8.14.4/8.14.4) with ESMTP id 02RMHIJ6000806; Fri, 27 Mar 2020 22:17:18 GMT Received: from [192.168.14.112] (/79.180.216.197) by default (Oracle Beehive Gateway v4.0) with ESMTP ; Fri, 27 Mar 2020 15:17:17 -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> Message-ID: <65eb2511-e2f5-d2b5-4470-278b06a0e077@oracle.com> Date: Sat, 28 Mar 2020 01:17:13 +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: X-Proofpoint-Virus-Version: vendor=nai engine=6000 definitions=9573 signatures=668685 X-Proofpoint-Spam-Details: rule=notspam policy=default score=0 malwarescore=0 suspectscore=0 spamscore=0 mlxlogscore=999 adultscore=0 phishscore=0 mlxscore=0 bulkscore=0 classifier=spam adjust=0 reason=mlx scancount=1 engine=8.12.0-2003020000 definitions=main-2003270182 X-Proofpoint-Virus-Version: vendor=nai engine=6000 definitions=9573 signatures=668685 X-Proofpoint-Spam-Details: rule=notspam policy=default score=0 mlxscore=0 impostorscore=0 phishscore=0 clxscore=1015 adultscore=0 spamscore=0 malwarescore=0 mlxlogscore=999 lowpriorityscore=0 bulkscore=0 priorityscore=1501 suspectscore=0 classifier=spam adjust=0 reason=mlx scancount=1 engine=8.12.0-2003020000 definitions=main-2003270182 X-MIME-Autoconverted: from 8bit to quoted-printable by userp2120.oracle.com id 02RMEPWH045733 Content-Type: text/plain; charset=windows-1252; format=flowed Content-Language: en-US Content-Transfer-Encoding: quoted-printable 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: >>>>> +=A0=A0=A0=A0=A0 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 EFI_SCSI_PASS_THRU_PASSTH= RU >>> in MdePkg/Include/Protocol/ScsiPassThru.h: >>> >>> =A0=A0 @retval EFI_BAD_BUFFER_SIZE=A0=A0=A0=A0=A0=A0 The SCSI Request= 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, an= d >>> 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 will=20 > 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 Packe= t 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, 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=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 the=20 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 is=20 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 information. Which again seems to align to what I have done in my PvScsi driver... -Liran