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.web10.2318.1585267546663699128 for ; Thu, 26 Mar 2020 17:05:46 -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=aPXL2lvv; 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 02R05bqQ030599; Fri, 27 Mar 2020 00:05:45 GMT DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=oracle.com; h=subject : to : cc : references : from : message-id : date : mime-version : in-reply-to : content-type : content-transfer-encoding; s=corp-2020-01-29; bh=UvKb1OdObyQKyfM0atHHBtb1O68UX2fYlYyCcbhoAek=; b=aPXL2lvvrcvheGyHuXdfBAu5MkCNSWbtSILjOgS+FovVXXJQ77INx0IBB9TQdCpgiJSY nHJBKiuO+eCiB351hDkDwEaZbkxu8VLCkvWFlQwmogWaLQM7Z1LAcPyh2LBoGctjix1J 38hinOleV3YWr9PnWJ22m4yxetL9eK/isW8GY5VfBFKmiR4jhNA2HFiDPf7x0XqCzM+7 ti3dNPGNyp3Wg0lRr8lHx7HMBtl+wLycREo9GAHNeOPzTgJwQhAAFdkTp12HU6OZVUyq pZ0zxOak3XuDk/9D+uByk/M6LDl4qvBTNR2e9g63tDumI4e1jUXuvA5vTR74wzLCSPkH gQ== Received: from userp3020.oracle.com (userp3020.oracle.com [156.151.31.79]) by userp2120.oracle.com with ESMTP id 300urk3du7-1 (version=TLSv1.2 cipher=ECDHE-RSA-AES256-GCM-SHA384 bits=256 verify=OK); Fri, 27 Mar 2020 00:05:45 +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 02R02sP9109380; Fri, 27 Mar 2020 00:05:45 GMT Received: from userv0122.oracle.com (userv0122.oracle.com [156.151.31.75]) by userp3020.oracle.com with ESMTP id 3003gmudk1-1 (version=TLSv1.2 cipher=ECDHE-RSA-AES256-GCM-SHA384 bits=256 verify=OK); Fri, 27 Mar 2020 00:05:45 +0000 Received: from abhmp0014.oracle.com (abhmp0014.oracle.com [141.146.116.20]) by userv0122.oracle.com (8.14.4/8.14.4) with ESMTP id 02R05iVg021408; Fri, 27 Mar 2020 00:05:44 GMT Received: from Lirans-MacBook-Pro.local (/213.57.127.2) by default (Oracle Beehive Gateway v4.0) with ESMTP ; Thu, 26 Mar 2020 17:05:44 -0700 Subject: Re: [PATCH v2 14/17] OvmfPkg/PvScsiDxe: Introduce DMA communication buffer 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-15-liran.alon@oracle.com> <45e5950c-ec82-c24b-4c38-2912402747b2@redhat.com> From: "Liran Alon" Message-ID: <1d747d1e-064e-f24a-f8ac-ac07b91c3999@oracle.com> Date: Fri, 27 Mar 2020 03:05:40 +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: <45e5950c-ec82-c24b-4c38-2912402747b2@redhat.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-2003260174 X-Proofpoint-Virus-Version: vendor=nai engine=6000 definitions=9572 signatures=668685 X-Proofpoint-Spam-Details: rule=notspam policy=default score=0 suspectscore=0 spamscore=0 mlxlogscore=999 clxscore=1015 lowpriorityscore=0 mlxscore=0 phishscore=0 bulkscore=0 impostorscore=0 adultscore=0 malwarescore=0 priorityscore=1501 classifier=spam adjust=0 reason=mlx scancount=1 engine=8.12.0-2003020000 definitions=main-2003260174 X-MIME-Autoconverted: from 8bit to quoted-printable by userp2120.oracle.com id 02R05bqQ030599 Content-Type: text/plain; charset=utf-8; format=flowed Content-Language: en-US Content-Transfer-Encoding: quoted-printable On 27/03/2020 0:17, Laszlo Ersek wrote: > On 03/25/20 17:10, Liran Alon wrote: >> PvScsiRestorePciAttributes (Dev); >> diff --git a/OvmfPkg/PvScsiDxe/PvScsi.h b/OvmfPkg/PvScsiDxe/PvScsi.h >> index 6d23b6e1eccf..7f91d70fec79 100644 >> --- a/OvmfPkg/PvScsiDxe/PvScsi.h >> +++ b/OvmfPkg/PvScsiDxe/PvScsi.h >> @@ -31,6 +31,11 @@ typedef struct { >> PVSCSI_DMA_DESC RingCmpsDmaDesc; >> } PVSCSI_RING_DESC; >> =20 >> +typedef struct { >> + UINT8 SenseData[MAX_UINT8]; > (4) Is the maximum possible size of the sense data specified somewhere? > If so, it would be nice to document it with a comment at least. This is a good point. Thanks for pointing it out. Turns out "SCSI Commands Reference Manual" section 2.4.1.1.1 Descriptor=20 format sense data overview specifies: "The additional sense length shall be less than or equal to 244 (i.e.,=20 limiting the total length of the sense data to 252 bytes)" i.e. The max possible size of sense data is 252. As another evidence, I saw QEMU's include/hw/scsi/scsi.h indeed defining: #define SCSI_SENSE_BUF_SIZE 252 This change was done by QEMU commit c5f52875b980 ("scsi: Change scsi=20 sense buf size to 252") which says in it's commit message: "According to SPC-4, section 4.5.2.1, 252 is the limit of sense data." Interestingly, this QEMU commit changed the value of SCSI_SENSE_BUF_SIZE=20 from 96 to 252. But then I found this in EDK2 OvmfPkg/Include/IndustryStandard/VirtioScsi= .h: // // We expect these maximum sizes from the host. Also we force the=20 CdbLength and // SenseDataLength parameters of=20 EFI_EXT_SCSI_PASS_THRU_PROTOCOL.PassThru() not // to exceed these limits. See UEFI 2.3.1 errata C 14.7. // #define VIRTIO_SCSI_CDB_SIZE=C2=A0=C2=A0 32 #define VIRTIO_SCSI_SENSE_SIZE 96 Which seems to be wrong... It seems most appropriate to add a SCSI_MAX_SENSE_SIZE constant to EDK2=20 MdePkg/Include/IndustryStandard/Scsi.h. And then use that from my PvScsi driver. I can also submit a separate patch (Not part of this series) to remove=20 this VIRTIO_SCSI_SENSE_SIZE constant. Note that VirtioScsi doesn't have a technical issue here because it=20 provides this max sense length to the device in VirtioScsiInit(): Status =3D VIRTIO_CFG_WRITE (Dev, SenseSize, VIRTIO_SCSI_SENSE_SIZE); So if it's OK by you, I think I will just add a patch to this series=20 defining SCSI_MAX_SENSE_SIZE in MdePkg/Include/IndustryStandard/Scsi.h, and then rely on that when defining SenseData in PVSCSI_DMA_BUFFER. >> + UINT8 Data[0x2000]; > (5) Same here. From peeking at the next patch, we seem to be choosing > this size arbitrarily. This is indeed arbitrary... > > If it works for you in all relevant boot scenarios, I'm OK with it, but > we should be clear that this value is arbitrarily chosen. No need for a > #define, but a comment would be nice. OK. Will just add a comment such as: // // This size is arbitrarily chosen, // It seems to be sufficient for all I/O requests sent through=20 EFI_SCSI_PASS_THRU_PROTOCOL.PassThru(). // If you wish to have something else, please say so. :) > > (6) Should we declare this structure as packed? There is no such requirement as it's not a wire-format or anything like=20 that. The only rational of defining it as packed is to avoid mapping=20 unnecessary pages to device. But this structure is less than a page-size anyway. So I think it's fine. If you still think it should be marked as packed, I can change this. Thanks! -Liran