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.web11.23362.1585344705405797747 for ; Fri, 27 Mar 2020 14:31:45 -0700 Authentication-Results: mx.groups.io; dkim=pass header.i=@oracle.com header.s=corp-2020-01-29 header.b=L36pNUsD; 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 02RLQKhG122610; Fri, 27 Mar 2020 21:31:44 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=l1squ+C4/eKO3XZchS+LfRErtPrSqCigy98pcK1TPL8=; b=L36pNUsDRtB/FVVovEj9nNR2LSWdPE7NW5ptkDbyDGBJ4U1fvFdF0vcNi9POyVgxZm9p e78TV/oIpZ96ylr187DNQfLRNrLPvk+7knlfEW8yzpWN17EGxPE/7IVgZOv3gw2X3X/M KBSa+/q7oxY+sTH2I0icVDVI+TuRyJW+Gfx0iARP8W1e/Pz+/aty7L9cDVV0FnEvW1pp 55/IY45sizklb1//NEjs94JMv71rZwflOVpFSw75S36o0XtDdTDsrAAZy9U4ANpvXOgn jVnH2CyjH8dAaTCelOQVndxF0GFqlwqGSjALBXIls4HNR2f+AORPO5CRe1/KevKdsytd Kw== Received: from aserp3030.oracle.com (aserp3030.oracle.com [141.146.126.71]) by userp2120.oracle.com with ESMTP id 3019vecha5-1 (version=TLSv1.2 cipher=ECDHE-RSA-AES256-GCM-SHA384 bits=256 verify=OK); Fri, 27 Mar 2020 21:31:44 +0000 Received: from pps.filterd (aserp3030.oracle.com [127.0.0.1]) by aserp3030.oracle.com (8.16.0.42/8.16.0.42) with SMTP id 02RLLxOD187655; Fri, 27 Mar 2020 21:31:43 GMT Received: from aserv0122.oracle.com (aserv0122.oracle.com [141.146.126.236]) by aserp3030.oracle.com with ESMTP id 3006raxkq0-1 (version=TLSv1.2 cipher=ECDHE-RSA-AES256-GCM-SHA384 bits=256 verify=OK); Fri, 27 Mar 2020 21:31:43 +0000 Received: from abhmp0004.oracle.com (abhmp0004.oracle.com [141.146.116.10]) by aserv0122.oracle.com (8.14.4/8.14.4) with ESMTP id 02RLVgVi009649; Fri, 27 Mar 2020 21:31:42 GMT Received: from [192.168.14.112] (/79.180.216.197) by default (Oracle Beehive Gateway v4.0) with ESMTP ; Fri, 27 Mar 2020 14:31:42 -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> <1d747d1e-064e-f24a-f8ac-ac07b91c3999@oracle.com> <2220b9a0-e550-6d9c-c102-f3f2f10326d3@redhat.com> From: "Liran Alon" Message-ID: Date: Sat, 28 Mar 2020 00:31:38 +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: <2220b9a0-e550-6d9c-c102-f3f2f10326d3@redhat.com> X-Proofpoint-Virus-Version: vendor=nai engine=6000 definitions=9573 signatures=668685 X-Proofpoint-Spam-Details: rule=notspam policy=default score=0 bulkscore=0 adultscore=0 suspectscore=0 phishscore=0 spamscore=0 mlxscore=0 mlxlogscore=999 malwarescore=0 classifier=spam adjust=0 reason=mlx scancount=1 engine=8.12.0-2003020000 definitions=main-2003270177 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-2003270177 Content-Type: text/plain; charset=utf-8; format=flowed Content-Transfer-Encoding: 7bit Content-Language: en-US On 27/03/2020 16:35, Laszlo Ersek wrote: > On 03/27/20 01:05, Liran Alon wrote: >> 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; >>>> +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 format sense data overview specifies: >> "The additional sense length shall be less than or equal to 244 (i.e., >> 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 >> 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 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 >> CdbLength and >> // SenseDataLength parameters of >> 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 32 >> #define VIRTIO_SCSI_SENSE_SIZE 96 >> >> Which seems to be wrong... > No, these macros / limits are valid. They match the virtio specification > (both 0.9.5 and 1.0) -- they match the "cdb_size" and "sense_size" > configuration values that the virtio-scsi device is required to expose > by default. > > While the driver could theoretically ask for larger sizes, these values > have never caused a problem in practice. I don't see a reason to change > the constants (let alone to complicate the code). > >> It seems most appropriate to add a SCSI_MAX_SENSE_SIZE constant to >> EDK2 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 >> this VIRTIO_SCSI_SENSE_SIZE constant. > Please don't; there's no reason to change the VirtioScsiDxe driver. > >> Note that VirtioScsi doesn't have a technical issue here because it >> provides this max sense length to the device in VirtioScsiInit(): >> >> Status = VIRTIO_CFG_WRITE (Dev, SenseSize, VIRTIO_SCSI_SENSE_SIZE); > Right, and even those config writes are mostly documentation / "just to > be sure" oriented. Because, there's also a comment in those parts: > > // > // We expect these maximum sizes from the host. Since they are > // guest-negotiable, ask for them rather than just checking them. > // > >> So if it's OK by you, I think I will just add a patch to this series >> defining SCSI_MAX_SENSE_SIZE in >> MdePkg/Include/IndustryStandard/Scsi.h, and then rely on that when >> defining SenseData in PVSCSI_DMA_BUFFER. > My experience tells me that making any OvmfPkg change dependent on new > patches for the core modules (MdePkg, MdeModulePkg, UefiCpuPkg, ...) > slows down the upstreaming of the OvmfPkg change significantly. > > Therefore, I would strongly advise against this ordering of changes. > > Today, upon reviewing patch#15, I commented again on the "SenseData" > array size (see under point (2) in my patch#15 feedback). Accordingly, > for now, please stick with the existent "SenseData" array declaration, > just add a comment. I expect / hope to merge your v3 posting. As an alternative, I can also define SenseData to be of size 252 (which I would #define as a constant in PvScsi.h), and explain that it is the limit of total length of SenseData according to SCSI specification. And indeed leave for a separate patch-series to move this PvScsi.h private constant and put it in the generic IndustryStandard/Scsi.h header file. I think it's nicer than just defining SenseData to be MAX_UINT8 because of the field size in PassThru packet. But I will of course do as you prefer. Just let me know your choice :) > > (Side comment: I would also like to ask Nikita to R-b the full final > patch set on the list, once I'm ready to merge the series, because I'd > like to testify to Nikita's review effort in the upstream git history.) Ok. I will let him know to review my v3 submission upstream. -Liran > > Then, with the series merged, you can propose a separate patch series (2 > patches), later -- introducing the new constant in MdePkg, plus putting > the new constant to use in the (then-upstream) PvScsi feature. No matter > how long that is delayed, the main feature will have been merged. >