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.810.1587747805930557460 for ; Fri, 24 Apr 2020 10:03:26 -0700 Authentication-Results: mx.groups.io; dkim=pass header.i=@oracle.com header.s=corp-2020-01-29 header.b=B+BiootN; spf=pass (domain: oracle.com, ip: 156.151.31.85, mailfrom: nikita.leshchenko@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 03OH32HY144162; Fri, 24 Apr 2020 17:03:22 GMT DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=oracle.com; h=content-type : mime-version : subject : from : in-reply-to : date : cc : content-transfer-encoding : message-id : references : to; s=corp-2020-01-29; bh=Xb7F+sWeiB5C/BvXc5UCqqppfHBj1iLn5w2lZfix3Tc=; b=B+BiootNab4q5DWjHacK5hqW0hzRVjEwsZjoPZub93e/JVR1My4HjYE+ARYjFqhASNDq smBOY2FUIQB1MXexEhUP2SV+IGmBmuDE7cFG1b2bOUl9YgWVObHR2QG6Fyu0ZuUDqv0k 0VuSkIS4w9zu6xORv9G8LYgTpXi3xdDZjmr1KHiWT6Rm/HS/UZ+6HE6nD61Hf1OazLRY MdilN0FdJMcJwUZXH+B15w60XPFBKX+9nz9eERhMZGFyYKy2vy/itMRiSpN+UndbForT bVJX/i8fVGX620t1rWJYOIPpnF2ORjWcpnlY07V0YzUbs3jmdK4qeZW2C8iwqXLVAm8M nQ== Received: from userp3020.oracle.com (userp3020.oracle.com [156.151.31.79]) by userp2120.oracle.com with ESMTP id 30k7qe8031-1 (version=TLSv1.2 cipher=ECDHE-RSA-AES256-GCM-SHA384 bits=256 verify=OK); Fri, 24 Apr 2020 17:03:22 +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 03OGvckn093744; Fri, 24 Apr 2020 17:03:22 GMT Received: from aserv0121.oracle.com (aserv0121.oracle.com [141.146.126.235]) by userp3020.oracle.com with ESMTP id 30k7qxgbj4-1 (version=TLSv1.2 cipher=ECDHE-RSA-AES256-GCM-SHA384 bits=256 verify=OK); Fri, 24 Apr 2020 17:03:22 +0000 Received: from abhmp0002.oracle.com (abhmp0002.oracle.com [141.146.116.8]) by aserv0121.oracle.com (8.14.4/8.13.8) with ESMTP id 03OH3KFi009756; Fri, 24 Apr 2020 17:03:20 GMT Received: from [10.30.3.6] (/213.57.127.2) by default (Oracle Beehive Gateway v4.0) with ESMTP ; Fri, 24 Apr 2020 10:03:20 -0700 Mime-Version: 1.0 (Mac OS X Mail 11.5 \(3445.9.1\)) Subject: Re: [edk2-devel] [PATCH v4 11/13] OvmfPkg/MptScsiDxe: Implement the PassThru method From: "Nikita Leshenko" In-Reply-To: <6be4886a-85e8-a343-0480-a758273cee5e@redhat.com> Date: Fri, 24 Apr 2020 20:03:16 +0300 Cc: liran.alon@oracle.com, aaron.young@oracle.com, Jordan Justen , Ard Biesheuvel Message-Id: References: <20200414173813.7715-1-nikita.leshchenko@oracle.com> <20200414173813.7715-12-nikita.leshchenko@oracle.com> <6be4886a-85e8-a343-0480-a758273cee5e@redhat.com> To: devel@edk2.groups.io, Laszlo Ersek X-Mailer: Apple Mail (2.3445.9.1) X-Proofpoint-Virus-Version: vendor=nai engine=6000 definitions=9601 signatures=668686 X-Proofpoint-Spam-Details: rule=notspam policy=default score=0 mlxscore=0 spamscore=0 mlxlogscore=999 adultscore=0 suspectscore=0 bulkscore=0 phishscore=0 malwarescore=0 classifier=spam adjust=0 reason=mlx scancount=1 engine=8.12.0-2003020000 definitions=main-2004240132 X-Proofpoint-Virus-Version: vendor=nai engine=6000 definitions=9601 signatures=668686 X-Proofpoint-Spam-Details: rule=notspam policy=default score=0 bulkscore=0 lowpriorityscore=0 priorityscore=1501 suspectscore=0 mlxlogscore=999 phishscore=0 impostorscore=0 mlxscore=0 clxscore=1015 malwarescore=0 adultscore=0 spamscore=0 classifier=spam adjust=0 reason=mlx scancount=1 engine=8.12.0-2003020000 definitions=main-2004240133 Content-Type: text/plain; charset=us-ascii Content-Transfer-Encoding: quoted-printable > On 20 Apr 2020, at 20:30, Laszlo Ersek wrote: >=20 > On 04/14/20 19:38, Nikita Leshenko wrote: >> Machines should be able to boot after this commit. Tested with = different >> Linux distributions (Ubuntu, CentOS) and different Windows >> versions (Windows 7, Windows 10, Server 2016). >>=20 >> Ref: = https://urldefense.com/v3/__https://bugzilla.tianocore.org/show_bug.cgi?id= =3D2390__;!!GqivPVa7Brio!JfeG4MsveGF5K9VB4618njXWmMprNv9SIfBg5g6ZHp5jolyqI= heckAunOt0SAymJ0j1Ywg$=20 >> Signed-off-by: Nikita Leshenko >> --- >> .../Include/IndustryStandard/FusionMptScsi.h | 19 +- >> OvmfPkg/MptScsiDxe/MptScsi.c | 369 = +++++++++++++++++- >> OvmfPkg/MptScsiDxe/MptScsiDxe.inf | 3 + >> OvmfPkg/OvmfPkg.dec | 3 + >> 4 files changed, 390 insertions(+), 4 deletions(-) >>=20 > [...] >=20 > (12) Does this device support 64-bit DMA? Yes it does, but in an inconvenient way. The high 32 bits are passed in = the handshake (SenseBufferHighAddr, HostMfaHighAddr) and the low 32 bits are = passed on the request queue/SenseBufferLowAddress. Now that I think about it = again, since we have only one request, reply and buffer, I'll just enable = dual-cycle and pass the high addresses on initialization. > [...] > (15) I don't see why hoisting MPT_SCSIIO_REQUEST_CONTROL_TXDIR_NONE > before the "switch" is helpful. That value can only take effect if the > XxxTransferLength field (matching the direction) is zero. But I don't > think those explicit zero checks buy us much. >=20 > I would simply remove the zero comparisons, and always go with either > TXDIR_READ or TXDIR_WRITE (dependent on Packet->DataDirection). And, = in > case the data size is zero, then just set / copy zero bytes. We can't just set/copy zero bytes. If transfer size is zero, we must set MPT_SCSIIO_REQUEST_CONTROL_TXDIR_NONE, otherwise the device (at least = under QEMU) will complain. But I'll simplify the code by setting TXDIR_NONE = only if the transfer size is zero. > [...] > (19) Assuming we can expose the reply frame, but not the request, will > the driver continue to behave fine (without explicit rollback actions > for the reply frame)? >=20 > Does "IoReplyEnqueued" help with handling this? Yes, IoReply is put on a queue and the device may use it to communicate = errors in case the request fails. If the request fails and we get an IoReply, = we set IoReplyEnqueued to FALSE so that we know to enqueue it on the next = request. If we fail while *submitting* the request, the reply frame will remain = enqueued (IoReplyEnqueued =3D=3D TRUE) and we'll be able to use it for a = following request. >=20 > (Just a question, not necessarily a code change request.) >=20 > [...] >> + if (Reply =3D=3D Dev->Dma->IoRequest.Data.Header.MessageContext) { >> + // >> + // This is a turbo reply, everything is good >> + // >> + Packet->HostAdapterStatus =3D = EFI_EXT_SCSI_STATUS_HOST_ADAPTER_OK; >> + Packet->TargetStatus =3D EFI_EXT_SCSI_STATUS_TARGET_GOOD; >=20 > (20) Does "turbo reply" mean that you have to update neither > XxxTransferLength, nor SenseDataLength, on output? Turbo reply means that the command was successful, which means that we = don't need to update XxxTransferLength. I think we need to zero = SenseDataLength since successful commands don't generate SENSE data. > [...] > (23) Would it make sense to check that the device only *lowers* > SenseDataLength with the above assignment? Yes, will add that. > [...] > I think TransferCount should be set in all cases. Does the device = really > only set TransferCount for TARGET_GOOD? I assumed that it only updates it when TARGET_GOOD for some reason, but = I checked again and I was wrong. I will read TransferCount = unconditionally. > [...] >> +[Pcd] >> + gUefiOvmfPkgTokenSpaceGuid.PcdMptScsiStallPerPollUsec ## CONSUMES >=20 > (36) General -- can you run "BaseTools/Scripts/SetupGit.py" in your = edk2 > clone? It will improve the file order in which changes are formatted > into patch files. A patch is easier to read if the INF, DEC, and = header > file changes come first. Sorry, I'm pretty sure I ran it in the past, I'll run it again... ACK on the rest of the comments (also in the other patches). Nikita >=20