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.2796.1585098699802451663 for ; Tue, 24 Mar 2020 18:11:39 -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=meZ710qQ; 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 02P1AXdC014930; Wed, 25 Mar 2020 01:11:39 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=LAlBW9mTGCKd6q+HB0cSiVT8i/TeQBhzNaoTVvH+g6U=; b=meZ710qQ3X6pzKvWewYmS19kK3d6afMxmh4O9h18F+Bqp9TrbBsZCIYHy+22LeqsSTsy WWnHzlismx8Y2Fd0/YfxIE/kR1YGJ8UnRejyzzx4ovIgk+3h6/b3IvS2cEbb9Pyti2AR +uCzWQKTw/KNJvBSezn/d20ZTyJeBH+Ki8UQT0xm/MLpON8nYjGO3Dcmh7mnMwaJ83Ex ZMPRYnnjxfvzCeUwJ2lEtoAHXO+HnxW+DK6F8tGw7lvum3zwZd9GK2SOuhluriJx+ZFm z3v5PZnBu188dImuApIBYBwq7mfJaZKoBMbkIO5b++n2+5zznd3t2iYqm7B291hJAVYi hg== Received: from userp3020.oracle.com (userp3020.oracle.com [156.151.31.79]) by userp2120.oracle.com with ESMTP id 2yx8ac493t-1 (version=TLSv1.2 cipher=ECDHE-RSA-AES256-GCM-SHA384 bits=256 verify=OK); Wed, 25 Mar 2020 01:11:39 +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 02P17aOZ016289; Wed, 25 Mar 2020 01:11:38 GMT Received: from userv0122.oracle.com (userv0122.oracle.com [156.151.31.75]) by userp3020.oracle.com with ESMTP id 2yxw6ntwby-1 (version=TLSv1.2 cipher=ECDHE-RSA-AES256-GCM-SHA384 bits=256 verify=OK); Wed, 25 Mar 2020 01:11:38 +0000 Received: from abhmp0012.oracle.com (abhmp0012.oracle.com [141.146.116.18]) by userv0122.oracle.com (8.14.4/8.14.4) with ESMTP id 02P1BaTd015847; Wed, 25 Mar 2020 01:11:37 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:11:36 -0700 Subject: Re: [edk2-devel] [PATCH 12/17] OvmfPkg/PvScsiDxe: Reset adapter on init 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-13-liran.alon@oracle.com> From: "Liran Alon" Message-ID: Date: Wed, 25 Mar 2020 03:11:33 +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: X-Proofpoint-Virus-Version: vendor=nai engine=6000 definitions=9570 signatures=668685 X-Proofpoint-Spam-Details: rule=notspam policy=default score=0 suspectscore=0 adultscore=0 malwarescore=0 mlxscore=0 spamscore=0 mlxlogscore=999 bulkscore=0 phishscore=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 spamscore=0 clxscore=1015 lowpriorityscore=0 suspectscore=0 priorityscore=1501 malwarescore=0 mlxscore=0 adultscore=0 phishscore=0 impostorscore=0 mlxlogscore=999 bulkscore=0 classifier=spam adjust=0 reason=mlx scancount=1 engine=8.12.0-2003020000 definitions=main-2003250008 X-MIME-Autoconverted: from 8bit to quoted-printable by userp2120.oracle.com id 02P1AXdC014930 Content-Type: text/plain; charset=windows-1252; format=flowed Content-Language: en-US Content-Transfer-Encoding: quoted-printable On 24/03/2020 18:00, Laszlo Ersek wrote: > On 03/16/20 16:01, Liran Alon wrote: >> +STATIC >> +EFI_STATUS >> +PvScsiWriteCmdDesc ( >> + IN CONST PVSCSI_DEV *Dev, >> + IN UINT32 Cmd, >> + IN VOID *Desc, >> + IN UINTN Length >> + ) >> +{ >> + EFI_STATUS Status; >> + UINTN LengthInWords; >> + UINT8 *WordPtr; >> + UINT8 *DescEndPtr; >> + UINT32 Word; >> + >> + LengthInWords =3D Length / sizeof (UINT32); > (4) What guarantees that "Length" is a whole multiple of sizeof > (UINT32)? Nothing. Besides the fact that all commands passed to this function are indeed=20 multiple of sizeof (UINT32). > In this review I have not insisted on including full-blown interface > contracts in the top-level function comment blocks (with @param[in] and > @retval etc). Thanks for that. I think too it would be an overkill with little value. > But, for this function, it really is unclear. Will it be sufficient for you if I just replace the prototype with=20 something like the following? /** =A0 Send PVSCSI command to device **/ STATIC EFI_STATUS PvScsiWriteCmdDesc ( =A0=A0 IN CONST PVSCSI_DEV=A0=A0 *Dev, =A0=A0 IN UINT32=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0= =A0 Cmd, =A0=A0 IN VOID=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0= =A0=A0=A0=A0 *Desc, =A0=A0 IN UINTN=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0= =A0=A0=A0 LengthInWords =A0=A0=A0 // Note: Word is UINT32 =A0=A0 ) >> + >> + if (LengthInWords > PVSCSI_MAX_CMD_DATA_WORDS) { >> + return EFI_INVALID_PARAMETER; >> + } >> + >> + Status =3D PvScsiMmioWrite32 (Dev, PVSCSI_REG_OFFSET_COMMAND, Cmd); >> + if (EFI_ERROR (Status)) { >> + return Status; >> + } >> + >> + WordPtr =3D Desc; >> + DescEndPtr =3D WordPtr + Length; >> + >> + while (WordPtr !=3D DescEndPtr) { >> + // >> + // CopyMem() is used to avoid strict-aliasing issues >> + // > (5) In edk2, we -- completely intentionally -- disable the enforcement > of the effective type rules / strict aliasing rules. See > "-fno-strict-aliasing" in "BaseTools/Conf/tools_def.template". > >> + CopyMem (&Word, WordPtr, sizeof (UINT32)); >> + >> + Status =3D PvScsiMmioWrite32 (Dev, PVSCSI_REG_OFFSET_COMMAND_DATA= , Word); >> + if (EFI_ERROR (Status)) { >> + return Status; >> + } >> + >> + WordPtr +=3D sizeof (UINT32); >> + } >> + >> + return EFI_SUCCESS; >> +} > (6) I think the open-coded loop is suboptimal -- the PciIo protocol > seems to offer EfiPciIoWidthFifoUint32 for exactly this purpose (=3D > advance in the memory buffer, while accessing the same offset in the > BAR). > > Have you perhaps tried that? I actually haven't noticed EfiPciIoWidthFifoUint32 until you mentioned it. As it seems there isn't even a single line of code in EDK2 that use it. := ) In fact, the only code that use one of the EfiPciIoWidthFifo* is=20 MdeModulePkg/Bus/Ata/AtaAtapiPassThru/IdeMode.c. > (I can imagine that you ruled it out, due to "Desc" being unaligned. Th= e > UEFI spec does say, "The caller is responsible for any alignment and I/= O > width issues which the bus, device, platform, or type of I/O might > require.) Why is this an issue? It's easy to document with one-line comment at end of Desc parameter=20 deceleration that it must be aligned. It's also not difficult to modify callers as only a single caller=20 actually pass a descriptor (The caller PvScsiInitRings()). To avoid further style comments, what is the coding convention in EDK2=20 to align the "PVSCSI_CMD_DESC_SETUP_RINGS Cmd;" var properly? In addition, I assume I don't need to add any validation of alignment to=20 PvScsiWriteCmdDesc(). -Liran