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.web12.247.1585693081589167310 for ; Tue, 31 Mar 2020 15:18:01 -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=vax1WsBV; 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 02VM9YIo193543; Tue, 31 Mar 2020 22:18:01 GMT DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=oracle.com; h=subject : from : to : references : message-id : date : mime-version : in-reply-to : content-type : content-transfer-encoding; s=corp-2020-01-29; bh=hEAwEYb2AuzY5W/e+dmHBbxKKfugeyH/eg36OYLCrtc=; b=vax1WsBVmIbKokD+Rv23aQcs2shzb5ImZNCuDJ0+TrAE9dhFQGAeF3bVML1PRBvFNGUO w593HXB5CGzskRZY0HyuVBBVv+/baYiRRETt0yiAn5fIaE/VPmVNxEnbmWz44R7GXxGc vbvCg7KpRiekGWQGDX98NGD4jsBMkWVTYNak3IT9nJomoCfkSomzBwfWvxavr5fdeWPv viIGJ3HnYyXg7Hw9qMULiUtMe5crvaXrNHXjROOepcb0x2tj5qsHTeMLwBudOJTxQAJF y+DTGMeszVOgvQYjrlFwlXbRJtoZhH9Eni025g8JLpVxtv7tnen6iFfssYqdEzxKSYlO 6Q== Received: from aserp3020.oracle.com (aserp3020.oracle.com [141.146.126.70]) by userp2130.oracle.com with ESMTP id 303cev27bp-1 (version=TLSv1.2 cipher=ECDHE-RSA-AES256-GCM-SHA384 bits=256 verify=OK); Tue, 31 Mar 2020 22:18:01 +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 02VM77pg108217; Tue, 31 Mar 2020 22:18:00 GMT Received: from aserv0122.oracle.com (aserv0122.oracle.com [141.146.126.236]) by aserp3020.oracle.com with ESMTP id 302gcdvc6v-1 (version=TLSv1.2 cipher=ECDHE-RSA-AES256-GCM-SHA384 bits=256 verify=OK); Tue, 31 Mar 2020 22:18:00 +0000 Received: from abhmp0012.oracle.com (abhmp0012.oracle.com [141.146.116.18]) by aserv0122.oracle.com (8.14.4/8.14.4) with ESMTP id 02VMHxfV006656; Tue, 31 Mar 2020 22:17:59 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:17:59 -0700 Subject: Re: [edk2-devel] [PATCH] OvmfPkg/PvScsiDxe: Fix VS2019 build error because of implicit cast From: "Liran Alon" 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> <5221e324-9f86-24b4-56a0-301a948151f8@oracle.com> Message-ID: <30a57406-0c09-043f-6b19-7e6e94afcb44@oracle.com> Date: Wed, 1 Apr 2020 01:17:56 +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: <5221e324-9f86-24b4-56a0-301a948151f8@oracle.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 priorityscore=1501 adultscore=0 clxscore=1015 phishscore=0 lowpriorityscore=0 spamscore=0 malwarescore=0 suspectscore=0 mlxscore=0 impostorscore=0 mlxlogscore=999 bulkscore=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 userp2130.oracle.com id 02VM9YIo193543 Content-Type: text/plain; charset=utf-8; format=flowed Content-Language: en-US Content-Transfer-Encoding: quoted-printable On 01/04/2020 1:13, Liran Alon wrote: > > 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=20 >>> an invalid operation as ASSERT is usually disabled on release builds. >>> 2. We do have a library to make this more explicit and common.=20 >>> https://urldefense.com/v3/__https://github.com/tianocore/edk2/blob/ma= ster/MdePkg/Include/Library/SafeIntLib.h*L548__;Iw!!GqivPVa7Brio!LNp76poq= a4dly7M8C8F3aX66wzZA68yStF_9jS-pD3izw3uJ24i_mDygFJEBsLc$=20 >>> >> 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: >> >> =C2=A0=C2=A0 ASSERT (Response->ScsiStatus <=3D MAX_UINT8); >> =C2=A0=C2=A0 if (Response->ScsiStatus > MAX_UINT8) { >> =C2=A0=C2=A0=C2=A0=C2=A0 CpuDeadLoop (); >> =C2=A0=C2=A0 } >> =C2=A0=C2=A0 Packet->TargetStatus =3D (UINT8)Response->ScsiStatus; >> >> An alternative way to write it is (by moving the ASSERT into the block= ): >> >> =C2=A0=C2=A0 if (Response->ScsiStatus > MAX_UINT8) { >> =C2=A0=C2=A0=C2=A0=C2=A0 ASSERT (Response->ScsiStatus <=3D MAX_UINT8); >> =C2=A0=C2=A0=C2=A0=C2=A0 CpuDeadLoop (); >> =C2=A0=C2=A0 } >> =C2=A0=C2=A0 Packet->TargetStatus =3D (UINT8)Response->ScsiStatus; >> >> Yet another (simply assert FALSE in the block): >> >> =C2=A0=C2=A0 if (Response->ScsiStatus > MAX_UINT8) { >> =C2=A0=C2=A0=C2=A0=C2=A0 ASSERT (FALSE); >> =C2=A0=C2=A0=C2=A0=C2=A0 CpuDeadLoop (); >> =C2=A0=C2=A0 } >> =C2=A0=C2=A0 Packet->TargetStatus =3D (UINT8)Response->ScsiStatus; >> >> >> Why: >> >> - in DEBUG builds, the assertion failure will be logged, and the prope= r >> 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), = as >> 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=20 > than 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=20 > assist debugging but not worth putting this condition in RELEASE as it=20 > should 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,=20 > and if it does, does the user really care? > In contrast, when boot fails because of this, it makes sense to build=20 > in DEBUG in attempt to verify what happened. > Note that if this condition will ever evaluate to FALSE (i.e.=20 > ScsiStatus is bigger than MAX_UINT8), then it is probably very=20 > deterministic. As it means PVSCSI device emulation on host is broken > and broke because of a very specific commit on host userspace VMM=20 > (E.g. QEMU). > Therefore, I still think an ASSERT() is what fits here best. But if=20 > you still think otherwise, then I have no objection to change it (Or=20 > Laszlo change it when applying). > I would say though, that if the pattern above is common, why isn't=20 > there a macro in EDK2 for that instead of manually writing it?=20 > 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 > I would also mention that there are some bizzare code in EDK2 that=20 defines it's own ASSERT() macro that just does CpuDeadLoop(). E.g. ArmVirtPkg/Library/ArmVirtDxeHobLib/HobLib.c and=20 OvmfPkg/Library/XenConsoleSerialPortLib/XenConsoleSerialPortLib.c) -Liran