From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from aserp2120.oracle.com (aserp2120.oracle.com [141.146.126.78]) by mx.groups.io with SMTP id smtpd.web10.2902.1585099056190467125 for ; Tue, 24 Mar 2020 18:17:36 -0700 Authentication-Results: mx.groups.io; dkim=pass header.i=@oracle.com header.s=corp-2020-01-29 header.b=Cf8aye2D; spf=pass (domain: oracle.com, ip: 141.146.126.78, mailfrom: liran.alon@oracle.com) Received: from pps.filterd (aserp2120.oracle.com [127.0.0.1]) by aserp2120.oracle.com (8.16.0.42/8.16.0.42) with SMTP id 02P19BTr015598; Wed, 25 Mar 2020 01:17:34 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=xyiSf9o/eOLP9If3eQydBDitGys3ZqnvC1i9kIGFlnM=; b=Cf8aye2Dsa1EcAmlp9JcW1fiWyNG0O5vmWTeburGtGvt/7f/Yp+feWBo93to47modsud krG4sOde+rnWMM2hPsHOpRY3/NaoC8+R2dFczSLJ+OaCqNxMizuh0+XBRrjb8A7SX6mU KDbMHovkff7t0j18l9eK4g1kdcUfbmmFBn8dvZgGm0abnSqFU+zfpBjUvxQHpAFtRMDp nvrWz/ZuYQ8gUqHw5iHu5LhSmwyLDI0KVqocq3q2zj6IbAmC5BK7ah0swMuba8doq8Ii S7e4+FgCtSt4xb8DKpNaU4ZkVszDUyV0RBWk6o8dDwk8hZ8VDJCpRquI4gE4Xmc08LU5 iQ== Received: from userp3030.oracle.com (userp3030.oracle.com [156.151.31.80]) by aserp2120.oracle.com with ESMTP id 2ywavm7a40-1 (version=TLSv1.2 cipher=ECDHE-RSA-AES256-GCM-SHA384 bits=256 verify=OK); Wed, 25 Mar 2020 01:17:34 +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 02P18QfM002361; Wed, 25 Mar 2020 01:17:33 GMT Received: from userv0121.oracle.com (userv0121.oracle.com [156.151.31.72]) by userp3030.oracle.com with ESMTP id 2yxw4qdsk7-1 (version=TLSv1.2 cipher=ECDHE-RSA-AES256-GCM-SHA384 bits=256 verify=OK); Wed, 25 Mar 2020 01:17:33 +0000 Received: from abhmp0014.oracle.com (abhmp0014.oracle.com [141.146.116.20]) by userv0121.oracle.com (8.14.4/8.13.8) with ESMTP id 02P1HVks013319; Wed, 25 Mar 2020 01:17:31 GMT Received: from [10.30.3.10] (/213.57.127.2) by default (Oracle Beehive Gateway v4.0) with ESMTP ; Tue, 24 Mar 2020 18:17:31 -0700 Subject: Re: [edk2-devel] [PATCH 15/17] OvmfPkg/PvScsiDxe: Support sending SCSI request and receive response 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: <20200316150113.104630-1-liran.alon@oracle.com> <20200316150113.104630-16-liran.alon@oracle.com> <4a56d6b0-db50-8663-37f4-a835451acff6@redhat.com> From: "Liran Alon" Message-ID: <8ba83ba6-af49-8330-8a4b-59aa6b040cdc@oracle.com> Date: Wed, 25 Mar 2020 03:17:28 +0200 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: <4a56d6b0-db50-8663-37f4-a835451acff6@redhat.com> X-Proofpoint-Virus-Version: vendor=nai engine=6000 definitions=9570 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-2003250008 X-Proofpoint-Virus-Version: vendor=nai engine=6000 definitions=9570 signatures=668685 X-Proofpoint-Spam-Details: rule=notspam policy=default score=0 adultscore=0 malwarescore=0 priorityscore=1501 mlxscore=0 bulkscore=0 clxscore=1015 impostorscore=0 phishscore=0 suspectscore=0 mlxlogscore=999 spamscore=0 lowpriorityscore=0 classifier=spam adjust=0 reason=mlx scancount=1 engine=8.12.0-2003020000 definitions=main-2003250008 Content-Type: text/plain; charset=windows-1252; format=flowed Content-Transfer-Encoding: 7bit Content-Language: en-US On 24/03/2020 18:43, Laszlo Ersek wrote: > On 03/16/20 16:01, Liran Alon wrote: >> +STATIC >> +BOOLEAN >> +PvScsiIsReqRingFull ( >> + IN CONST PVSCSI_DEV *Dev >> + ) >> +{ >> + PVSCSI_RINGS_STATE *RingsState; >> + UINT64 ReqNumEntries; >> + >> + RingsState = Dev->RingDesc.RingState; >> + ReqNumEntries = 1 << RingsState->ReqNumEntriesLog2; > (5) Wrong for two reasons: > > (5a) Based on ReqNumEntries having type UINT64, the shift count may > presumably be larger than 31. But the constant "1" has type "signed int" > (mapping to INT32 in edk2), and so we should never left-shift that by > even 31 positions (we should never shift bits into the sign bit). Let > alone by more than 31 positions. > > In other words, the constant should be 1ULL. > > (5b) Please use RShiftU64() from BaseLib. Actually, I have noticed that ReqNumEntries should just be UINT32 and "1 <<" should be changed to "1U <<". So I made these changes for v2. Thanks. >> @@ -135,7 +492,70 @@ PvScsiPassThru ( >> IN EFI_EVENT Event OPTIONAL >> ) >> { ... >> + Dev->RingDesc.RingState->ReqProdIdx++; >> + >> + Status = PvScsiMmioWrite32 (Dev, PVSCSI_REG_OFFSET_KICK_RW_IO, 0); >> + if (EFI_ERROR (Status)) { >> + // >> + // If kicking the host fails, we must fake a host adapter error. >> + // EFI_NOT_READY would save us the effort, but it would also suggest that >> + // the caller retry. >> + // >> + goto FakeHostAdapterError; >> + } > (11) Hmmm. Not really happy about this. It doesn't feel like actual > error handling (= resource release / rollback); we're just factoring out > response composition. That's OK per se, but then it belongs to a helper > function, not a "function epilogue" here. Ok. I will move it to a helper function in v2. -Liran