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.11148.1585315260356759764 for ; Fri, 27 Mar 2020 06:21:00 -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=Yq2wM8j7; 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 02RDDMhJ175791; Fri, 27 Mar 2020 13:20:59 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=6CR1T63EV76u97HwKJ0zvWR6zHfK8XujzPZmSNzhHiI=; b=Yq2wM8j75Id2k5+t67oNlqts+Jw2ivUl8QT6xI9TGMlCLAPWxRL1eT5qZ7Bd1QsLA+Hw DH9moqtH2+lH8eYA0CkgRGGr7tcXmZgKJDv6pPBLywG+4E0+qpXBOBNaLeF+3B76hzPj xN8onNtWJEyNJ2IZ1jbCl9podcrFQTP0V9xjS41bTD+wYnhKy5Z9RyVQLF/2GE3c79o3 GnWTBYXT4D55SwNSPkVdtExom1WqmpJeLECVFbKjuycHCxr/MaiUysRhnd2bU49pGLHZ blyVf36xxCmnYJ0dmr5O/tzEuI8X3g3hceg47WvaRSI2uUWlHugf47qSRIbt9mOoB9Gm bg== Received: from userp3020.oracle.com (userp3020.oracle.com [156.151.31.79]) by userp2130.oracle.com with ESMTP id 301459b224-1 (version=TLSv1.2 cipher=ECDHE-RSA-AES256-GCM-SHA384 bits=256 verify=OK); Fri, 27 Mar 2020 13:20:59 +0000 Received: from pps.filterd (userp3020.oracle.com [127.0.0.1]) by userp3020.oracle.com (8.16.0.42/8.16.0.42) with SMTP id 02RDK95l034169; Fri, 27 Mar 2020 13:20:59 GMT Received: from userv0122.oracle.com (userv0122.oracle.com [156.151.31.75]) by userp3020.oracle.com with ESMTP id 3003gnxpbc-1 (version=TLSv1.2 cipher=ECDHE-RSA-AES256-GCM-SHA384 bits=256 verify=OK); Fri, 27 Mar 2020 13:20:59 +0000 Received: from abhmp0013.oracle.com (abhmp0013.oracle.com [141.146.116.19]) by userv0122.oracle.com (8.14.4/8.14.4) with ESMTP id 02RDKvkr011219; Fri, 27 Mar 2020 13:20:58 GMT Received: from [192.168.14.112] (/79.180.216.197) by default (Oracle Beehive Gateway v4.0) with ESMTP ; Fri, 27 Mar 2020 06:20:57 -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: <50ebcafb-b0c2-f948-6adb-6e533d3ed3ba@oracle.com> Date: Fri, 27 Mar 2020 16:20:53 +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: <6f32838a-d136-050e-7a05-f817da7954a8@oracle.com> X-Proofpoint-Virus-Version: vendor=nai engine=6000 definitions=9572 signatures=668685 X-Proofpoint-Spam-Details: rule=notspam policy=default score=0 mlxscore=0 mlxlogscore=999 bulkscore=0 phishscore=0 adultscore=0 spamscore=0 malwarescore=0 suspectscore=0 classifier=spam adjust=0 reason=mlx scancount=1 engine=8.12.0-2003020000 definitions=main-2003270123 X-Proofpoint-Virus-Version: vendor=nai engine=6000 definitions=9572 signatures=668685 X-Proofpoint-Spam-Details: rule=notspam policy=default score=0 malwarescore=0 phishscore=0 priorityscore=1501 bulkscore=0 lowpriorityscore=0 mlxlogscore=999 adultscore=0 suspectscore=0 impostorscore=0 mlxscore=0 spamscore=0 clxscore=1015 classifier=spam adjust=0 reason=mlx scancount=1 engine=8.12.0-2003020000 definitions=main-2003270122 X-MIME-Autoconverted: from 8bit to quoted-printable by userp2130.oracle.com id 02RDDMhJ175791 Content-Type: text/plain; charset=windows-1252; format=flowed Content-Language: en-US Content-Transfer-Encoding: quoted-printable 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: >> >>> + >>> +=A0 // >>> +=A0 // Report target status >>> +=A0 // >>> +=A0 Packet->TargetStatus =3D Response->ScsiStatus; >>> + >>> +=A0 // >>> +=A0 // Host adapter status and function return value depend on >>> +=A0 // device response's host status >>> +=A0 // >>> +=A0 switch (Response->HostStatus) { >>> +=A0=A0=A0 case PvScsiBtStatSuccess: >>> +=A0=A0=A0 case PvScsiBtStatLinkedCommandCompleted: >>> +=A0=A0=A0 case PvScsiBtStatLinkedCommandCompletedWithFlag: >>> +=A0=A0=A0=A0=A0 Packet->HostAdapterStatus =3D EFI_EXT_SCSI_STATUS_HO= ST_ADAPTER_OK; >>> +=A0=A0=A0=A0=A0 return EFI_SUCCESS; >>> + >>> +=A0=A0=A0 case PvScsiBtStatSelTimeout: >>> +=A0=A0=A0=A0=A0 Packet->HostAdapterStatus =3D >>> + EFI_EXT_SCSI_STATUS_HOST_ADAPTER_SELECTION_TIMEOUT; >>> +=A0=A0=A0=A0=A0 return EFI_TIMEOUT; >>> + >>> +=A0=A0=A0 case PvScsiBtStatDatarun: >>> +=A0=A0=A0 case : >>> +=A0=A0=A0=A0=A0 // >>> +=A0=A0=A0=A0=A0 // Report residual data in overrun/underrun >>> +=A0=A0=A0=A0=A0 // >>> +=A0=A0=A0=A0=A0 if (Packet->DataDirection =3D=3D EFI_EXT_SCSI_DATA_D= IRECTION_READ) { >>> +=A0=A0=A0=A0=A0=A0=A0 Packet->InTransferLength =3D Response->DataLen= ; >>> +=A0=A0=A0=A0=A0 } else { >>> +=A0=A0=A0=A0=A0=A0=A0 Packet->OutTransferLength =3D Response->DataLe= n; >>> +=A0=A0=A0=A0=A0 } >> 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=20 > a bug that it never sets this in pvscsi_command_complete() when it=20 > 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=20 > 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 o= f >> "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)=20 reads the "DataLen" field only in case the "HostStatus" is=20 BTSTAT_DATARUN or BTSTAT_DATA_UNDERRUN. As I have done in my driver. In the lack of more detailed device=20 specification (As PVSCSI is a proprietary VMware PV device), I prefer to=20 remain with my implementation which seems to be guaranteed to be safe and working. Please tell me if you think=20 otherwise. -Liran