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.174.1585692838769585359 for ; Tue, 31 Mar 2020 15:13:58 -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=C5H8xMMw; 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 02VMAhcw060315; Tue, 31 Mar 2020 22:13:58 GMT DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=oracle.com; h=subject : to : references : from : message-id : date : mime-version : in-reply-to : content-type : content-transfer-encoding; s=corp-2020-01-29; bh=FpTlLeueaav8E5+s/3JDTmm1JmrehUPsHSUmjbeISlc=; b=C5H8xMMw5VaR0DSxUhDS4oX3w2Hsroq1zFCyoCobx0zjdxgo3cdmyNpIdFnr8vgfLSBR 7bB0phIRFZlGO2l1wcdMkL9NWSMlFWRceKKStmO0QfJMQFhFHM/Ksco0EdPJpLB+L1cG hPDhG3DknIY3Q/LVy9Clz6F8g/L+qLJ7zkW1sl7bbDr1hvjKFIVIC3gbacHqepz/POKf I8kmFuJca0BcWBhs28xq5AMuWeuuKvAjyyoy4CrtEBRHzKwUq6HcGGQRKkRN2iCoSCfQ sXcdfkS+zPObaa+Smw6vQM2v7i4b4UMpc4n6p4NrymDCtThME8qbu3gyqlU8wOh0U+yK PA== Received: from aserp3020.oracle.com (aserp3020.oracle.com [141.146.126.70]) by userp2120.oracle.com with ESMTP id 303aqhjptg-1 (version=TLSv1.2 cipher=ECDHE-RSA-AES256-GCM-SHA384 bits=256 verify=OK); Tue, 31 Mar 2020 22:13:58 +0000 Received: from pps.filterd (aserp3020.oracle.com [127.0.0.1]) by aserp3020.oracle.com (8.16.0.42/8.16.0.42) with SMTP id 02VM77fo108173; Tue, 31 Mar 2020 22:13:57 GMT Received: from userv0121.oracle.com (userv0121.oracle.com [156.151.31.72]) by aserp3020.oracle.com with ESMTP id 302gcdv1xn-1 (version=TLSv1.2 cipher=ECDHE-RSA-AES256-GCM-SHA384 bits=256 verify=OK); Tue, 31 Mar 2020 22:13:57 +0000 Received: from abhmp0015.oracle.com (abhmp0015.oracle.com [141.146.116.21]) by userv0121.oracle.com (8.14.4/8.13.8) with ESMTP id 02VMDtXs005769; Tue, 31 Mar 2020 22:13:56 GMT Received: from [192.168.14.112] (/79.180.216.197) by default (Oracle Beehive Gateway v4.0) with ESMTP ; Tue, 31 Mar 2020 15:13:55 -0700 Subject: Re: [edk2-devel] [PATCH] OvmfPkg/PvScsiDxe: Fix VS2019 build error because of implicit cast To: Laszlo Ersek , devel@edk2.groups.io, sean.brogan@microsoft.com References: <20200331110452.51992-1-liran.alon@oracle.com> <22866.1585670000047029434@groups.io> <4b90b41a-2cf1-8b6e-6619-1c652820265e@redhat.com> From: "Liran Alon" Message-ID: <5221e324-9f86-24b4-56a0-301a948151f8@oracle.com> Date: Wed, 1 Apr 2020 01:13:52 +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: <4b90b41a-2cf1-8b6e-6619-1c652820265e@redhat.com> X-Proofpoint-Virus-Version: vendor=nai engine=6000 definitions=9577 signatures=668685 X-Proofpoint-Spam-Details: rule=notspam policy=default score=0 bulkscore=0 spamscore=0 malwarescore=0 mlxlogscore=999 adultscore=0 suspectscore=0 phishscore=0 mlxscore=0 classifier=spam adjust=0 reason=mlx scancount=1 engine=8.12.0-2003020000 definitions=main-2003310180 X-Proofpoint-Virus-Version: vendor=nai engine=6000 definitions=9577 signatures=668685 X-Proofpoint-Spam-Details: rule=notspam policy=default score=0 bulkscore=0 phishscore=0 clxscore=1015 malwarescore=0 impostorscore=0 mlxlogscore=999 spamscore=0 mlxscore=0 priorityscore=1501 lowpriorityscore=0 adultscore=0 suspectscore=0 classifier=spam adjust=0 reason=mlx scancount=1 engine=8.12.0-2003020000 definitions=main-2003310180 X-MIME-Autoconverted: from 8bit to quoted-printable by userp2120.oracle.com id 02VMAhcw060315 Content-Type: text/plain; charset=utf-8; format=flowed Content-Language: en-US Content-Transfer-Encoding: quoted-printable On 01/04/2020 0:56, Laszlo Ersek wrote: > On 03/31/20 17:53, Sean via Groups.Io wrote: >> A couple of thoughts. >> 1. I would suggest that ASSERT should not be the only protection for a= n invalid operation as ASSERT is usually disabled on release builds. >> 2. We do have a library to make this more explicit and common. https:/= /urldefense.com/v3/__https://github.com/tianocore/edk2/blob/master/MdePkg= /Include/Library/SafeIntLib.h*L548__;Iw!!GqivPVa7Brio!LNp76poqa4dly7M8C8F= 3aX66wzZA68yStF_9jS-pD3izw3uJ24i_mDygFJEBsLc$ > In this case, when "Response->ScsiStatus" does not fit in > "Packet->TargetStatus", the device model is obviously (and blatantly) > misbehaving, so I would agree with Liran that trying to recover from > that (or to cover it up with a nice error code passed out) is futile. Exactly. > > I do agree with the observation however that ASSERT()s disappear from > RELEASE builds. > > Mike Kinney taught me a pattern to deal with this. There are various > ways to write it; one example (for this case) is: > > ASSERT (Response->ScsiStatus <=3D MAX_UINT8); > if (Response->ScsiStatus > MAX_UINT8) { > CpuDeadLoop (); > } > Packet->TargetStatus =3D (UINT8)Response->ScsiStatus; > > An alternative way to write it is (by moving the ASSERT into the block)= : > > if (Response->ScsiStatus > MAX_UINT8) { > ASSERT (Response->ScsiStatus <=3D MAX_UINT8); > CpuDeadLoop (); > } > Packet->TargetStatus =3D (UINT8)Response->ScsiStatus; > > Yet another (simply assert FALSE in the block): > > if (Response->ScsiStatus > MAX_UINT8) { > ASSERT (FALSE); > CpuDeadLoop (); > } > Packet->TargetStatus =3D (UINT8)Response->ScsiStatus; > > > Why: > > - in DEBUG builds, the assertion failure will be logged, and the proper > assertion failure action will be triggered (CpuDeadLoop / exception / > ..., as configured by the platform) > > - in RELEASE builds, we'll still hang, and might have a chance to > investigate (get a stack dump perhaps). > > Regarding SafeIntLib, I'm a fan in general. In this case, I did not > think of it (possible integer truncations seem so rare in this driver). > For this patch, I'm OK either way (with or without using SafeIntLib), a= s > long as we add both the ASSERT and the explicit CpuDeadLoop (either > variant of the three). > > Thanks > Laszlo Honestly, I don't quite understand why using SafeIntLib is useful in=20 this case. It just internally does even more branches and checks for exactly same=20 overflow and return RETURN_BUFFER_TOO_SMALL in case value is bigger than=20 MAX_UINT8. Externally, I would still need to do a check on SafeUint16ToUint8()=20 return-value. So what's the benefit?... Seems to me to just be an=20 useless overhead. I believe checking against MAX_UINT8 and casting immediately one line=20 afterwards, is explicit enough. Regarding above comment that ASSERT() doesn't do anything for RELEASE=20 builds: The point in ASSERT() is to be able to check a condition early to assist=20 debugging but not worth putting this condition in RELEASE as it should=20 never happen and just waste CPU cycles. I thought this is the case we have here. If a weird ScsiStatus would=20 return, it is highly unlikely that boot would just succeed as usual, and=20 if it does, does the user really care? In contrast, when boot fails because of this, it makes sense to build in=20 DEBUG in attempt to verify what happened. Note that if this condition will ever evaluate to FALSE (i.e. ScsiStatus=20 is bigger than MAX_UINT8), then it is probably very deterministic. As it=20 means PVSCSI device emulation on host is broken and broke because of a very specific commit on host userspace VMM (E.g.=20 QEMU). Therefore, I still think an ASSERT() is what fits here best. But if you=20 still think otherwise, then I have no objection to change it (Or Laszlo=20 change it when applying). I would say though, that if the pattern above is common, why isn't there=20 a macro in EDK2 for that instead of manually writing it? Something like: #define RELEASE_ASSERT(Cond) \ =C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0= =C2=A0=C2=A0 if (!(Cond)) {=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0= =C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0 \ =C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0= =C2=A0=C2=A0=C2=A0=C2=A0 ASSERT (FALSE);=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2= =A0=C2=A0 \ =C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0= =C2=A0=C2=A0=C2=A0=C2=A0 CpuDeadLoop ();=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2= =A0 \ =C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0= =C2=A0=C2=A0 } That would be more elegant. -Liran