From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from de-smtp-delivery-102.mimecast.com (de-smtp-delivery-102.mimecast.com [51.163.158.102]) by mx.groups.io with SMTP id smtpd.web12.15498.1594714798107092352 for ; Tue, 14 Jul 2020 01:19:58 -0700 Authentication-Results: mx.groups.io; dkim=pass header.i=@suse.com header.s=mimecast20200619 header.b=c3DiKwhk; spf=pass (domain: suse.com, ip: 51.163.158.102, mailfrom: glin@suse.com) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=suse.com; s=mimecast20200619; t=1594714794; h=from:from:reply-to:subject:subject:date:date:message-id:message-id: to:to:cc:cc:mime-version:mime-version:content-type:content-type: in-reply-to:in-reply-to:references:references; bh=rRvuBR7LlQJrdlpi7zG0EP9Pr3wrfPZUbRCaVOzaTMY=; b=c3DiKwhkllVFmlzIzivf/RpX+WSQWVEnDV90pdXK/z6YOSVZBN+jQRusbniA3n/lFoLDl3 4+mJX3/+3YaqZFaftUPCPXosAGxeMzWm5iKKELf9JEidGQOjKOSt1wn3BB/Nnie3Vspsw7 PvvfT4LFzAycs0s9E9nCW/INHhyCmCM= Received: from EUR05-DB8-obe.outbound.protection.outlook.com (mail-db8eur05lp2107.outbound.protection.outlook.com [104.47.17.107]) (Using TLS) by relay.mimecast.com with ESMTP id de-mta-27-lMKWkeo-Of6-PxZtBXL_qQ-1; Tue, 14 Jul 2020 10:19:53 +0200 X-MC-Unique: lMKWkeo-Of6-PxZtBXL_qQ-1 ARC-Seal: i=1; a=rsa-sha256; s=arcselector9901; d=microsoft.com; cv=none; b=ZbW0BxxXVwm9kxZ4MtrDxgtYy0NJQvQoc+I3Jxc/CEIUrOmOu8FWG/nzJlqtvvPCvlxEyq3KaEKpqy5ExpjIClCOTB7z2vMEn08iVByVxyenyHiBN6dnPBhrfdFE6yLXsqfLSFGwYuomb5t+T3QLG4/mjkngOJtFrZow8w39QHLrMwCyG1oKj2V/MmJ6ta1rryGAqiwJ9lIq3eJzr8L0gAPW4P1Wyxu1Q2yDfDJG58JEvrixu8EcjHI4D6dyqaLMopOWCxFVd7BG931e1rdpPGh5QQ84rS7R34noh2Gc4j0GfWY0EnAWL4PyXM5b8+UQruPouf3SDWvWtlW65GUKZQ== ARC-Message-Signature: i=1; a=rsa-sha256; c=relaxed/relaxed; d=microsoft.com; s=arcselector9901; h=From:Date:Subject:Message-ID:Content-Type:MIME-Version:X-MS-Exchange-SenderADCheck; bh=rRvuBR7LlQJrdlpi7zG0EP9Pr3wrfPZUbRCaVOzaTMY=; b=dKYKUISHVvbOktifHIMHk1mgejVyuIBmI6jDnN99TBPPAWJys/U3HDoRNLE5dcT5+0+xscBZomEok74Xe8qPjUh2mSwnCqP8N6YIjqSaWLh1jpaOcEpgM34seRnkNM5qjdSNEP2RQYbCReMF5QRl7Q7KRhg07VZP6WYHUEiG/S+QnI3kxQGKU+IbzjyYhBdtZ0fJZuCAXVx3ZXZXdC3zuLs20wvmqWoZ7Ie/zeasYmOFE16+n7q3Vcm1uzZzPKLcI/yDCYsAZhbCB/vdxrbWQGVzrOVX7bD6FaLAOSbKiUBpR5cOzh5MRky5Kj3k/zOgLmGJzAMsGaKsAol6lu6jUA== ARC-Authentication-Results: i=1; mx.microsoft.com 1; spf=pass smtp.mailfrom=suse.com; dmarc=pass action=none header.from=suse.com; dkim=pass header.d=suse.com; arc=none Authentication-Results: edk2.groups.io; dkim=none (message not signed) header.d=none;edk2.groups.io; dmarc=none action=none header.from=suse.com; Received: from AM0PR0402MB3809.eurprd04.prod.outlook.com (2603:10a6:208:10::30) by AM0PR04MB6530.eurprd04.prod.outlook.com (2603:10a6:208:16a::20) with Microsoft SMTP Server (version=TLS1_2, cipher=TLS_ECDHE_RSA_WITH_AES_256_GCM_SHA384) id 15.20.3174.21; Tue, 14 Jul 2020 08:19:51 +0000 Received: from AM0PR0402MB3809.eurprd04.prod.outlook.com ([fe80::a14c:d441:c8a9:77ba]) by AM0PR0402MB3809.eurprd04.prod.outlook.com ([fe80::a14c:d441:c8a9:77ba%6]) with mapi id 15.20.3174.025; Tue, 14 Jul 2020 08:19:51 +0000 Date: Tue, 14 Jul 2020 16:19:42 +0800 From: "Gary Lin" To: devel@edk2.groups.io Cc: Laszlo Ersek , Jordan Justen , Ard Biesheuvel Subject: Re: [edk2-devel] [PATCH 10/11] OvmfPkg/LsiScsiDxe: Process the SCSI Request Packet Message-ID: <20200714081942.GF6058@GaryWorkstation> References: <20200701040448.14871-1-glin@suse.com> <20200701040448.14871-11-glin@suse.com> <79b57aa5-c531-0953-2485-fef4b52005db@redhat.com> <161FB1B03BD2D339.11266@groups.io> In-Reply-To: <161FB1B03BD2D339.11266@groups.io> X-ClientProxiedBy: AM0PR10CA0101.EURPRD10.PROD.OUTLOOK.COM (2603:10a6:208:e6::18) To AM0PR0402MB3809.eurprd04.prod.outlook.com (2603:10a6:208:10::30) Return-Path: glin@suse.com MIME-Version: 1.0 X-MS-Exchange-MessageSentRepresentingType: 1 Received: from GaryWorkstation (60.251.47.115) by AM0PR10CA0101.EURPRD10.PROD.OUTLOOK.COM (2603:10a6:208:e6::18) with Microsoft SMTP Server (version=TLS1_2, cipher=TLS_ECDHE_RSA_WITH_AES_256_GCM_SHA384) id 15.20.3174.21 via Frontend Transport; Tue, 14 Jul 2020 08:19:49 +0000 X-Originating-IP: [60.251.47.115] X-MS-PublicTrafficType: Email X-MS-Office365-Filtering-Correlation-Id: f3ee51fc-bcc1-49fd-516f-08d827ceadc3 X-MS-TrafficTypeDiagnostic: AM0PR04MB6530: X-Microsoft-Antispam-PRVS: X-MS-Oob-TLC-OOBClassifiers: OLM:9508; X-MS-Exchange-SenderADCheck: 1 X-Microsoft-Antispam: BCL:0; X-Microsoft-Antispam-Message-Info: rRDRmi9LLXe2SFZdBgvYSIN9JfPFHTaaZLdu/BMIT+Jvz62VOF2oXuXFSJ1F+y2X6sfkfSbsynlmMOz+kHCpuyH3PgXnIjouEd34H3K7RlBKbgy1/7vWqfC9WefgkEwgDE2F6Gf6Dh3G3IT7n3aahwhPpduPmR6jIv8dKmKGX1vcdUBCQJ9I39NbDo1FIyMkWIa3HV4/xaVi1OjMFMhUfvsNc5p2vi12dXhmggR6OG6xspehDfEdtU0IVicKlO9KregHQibF9j5+tOZeEbGMLYWVqm5jdzmMywdMY9PNmU1TlEqRFDq4xBVy4ciAihzCQUlobTyOM+CgAYPkFdu3Lk6QVPpAYcqre1sddg4KOlVeSKTe2jxoStMa4FlDybWhXMkr9B2x3pT3y/Eif3mMa9ZFn/BifL1Ohj3Kbhy/E2o0GlyDOYTi9W4Y1FCCnfjDKr5ReVAywInGCeG4X7yT8A== X-Forefront-Antispam-Report: CIP:255.255.255.255;CTRY:;LANG:en;SCL:1;SRV:;IPV:NLI;SFV:NSPM;H:AM0PR0402MB3809.eurprd04.prod.outlook.com;PTR:;CAT:NONE;SFTY:;SFS:(346002)(366004)(396003)(39860400002)(136003)(376002)(2906002)(8676002)(186003)(4326008)(5660300002)(30864003)(52116002)(8936002)(1076003)(6496006)(6916009)(33656002)(33716001)(316002)(83380400001)(26005)(16526019)(6666004)(478600001)(66946007)(55016002)(9686003)(53546011)(55236004)(66556008)(66476007)(956004)(54906003)(966005)(86362001)(60764002);DIR:OUT;SFP:1101; X-MS-Exchange-AntiSpam-MessageData: KiCFe60WjkEpSl4Sf4MmszR+5NOI6Fq1ddn4JcZM5ZGUhJB/B5w6FPe09ceIzI5MYRCwYb3HvpIrD38zOeH8CgUOLzPNEMB2R8twGMJbF3g2UE9FfYW0RGn9YC53sMuPUjX4q/vlg84mpOHYxcfwFkgFkW144tFtgl81DnQBzZAaUBMWfE7DxUJnYWnyrubstwDvjb64C/xH1C72Q6OhXNnswW7ZHE6mL1Q8qRdg/bEgzZcBPLnO3iVPkWj0V+EkqbkDXnf0OE45Jk60QXdFsKFUSGOIIHbMlQBRH3+xcgTPxjVRrN4CokL9RY94rDny4dBEFrHHatBno6Uvsyy3DIyEmTEqHPE/fAH3TdN9gCOJTmrsjAjM27TGq+hOcSgAFMmclI66xkdRyT523NtLEO6Ij9lVLZJlTFFBj60I4gwqbDRiC8FVX8qiOybFncplGTHhO59z7m9Ct/166SOuA2arYsD/q7Zmrrr9aslmKYM= X-OriginatorOrg: suse.com X-MS-Exchange-CrossTenant-Network-Message-Id: f3ee51fc-bcc1-49fd-516f-08d827ceadc3 X-MS-Exchange-CrossTenant-AuthSource: AM0PR0402MB3809.eurprd04.prod.outlook.com X-MS-Exchange-CrossTenant-AuthAs: Internal X-MS-Exchange-CrossTenant-OriginalArrivalTime: 14 Jul 2020 08:19:51.0926 (UTC) X-MS-Exchange-CrossTenant-FromEntityHeader: Hosted X-MS-Exchange-CrossTenant-Id: f7a17af6-1c5c-4a36-aa8b-f5be247aa4ba X-MS-Exchange-CrossTenant-MailboxType: HOSTED X-MS-Exchange-CrossTenant-UserPrincipalName: H38TYBYwPeaw7A/6a+BYm7gJDapbbi6uJgSOHfJBVZfh5ckR2YcTpYHOvzxGA/Zv X-MS-Exchange-Transport-CrossTenantHeadersStamped: AM0PR04MB6530 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline On Wed, Jul 08, 2020 at 02:02:27PM +0800, Gary Lin via groups.io wrote: > On Tue, Jul 07, 2020 at 02:46:14PM +0200, Laszlo Ersek wrote: > > On 07/01/20 06:04, Gary Lin wrote: > > > This is the second part of LsiScsiPassThru(). LsiScsiProcessRequest() is > > > added to translate the SCSI Request Packet into the LSI 53C895A > > > commands. This function utilizes the so-called Script buffer to transmit > > > a series of commands to the chip and then polls the DMA Status (DSTAT) > > > register until the Scripts Interrupt Instruction Received (SIR) bit > > > sets. Once the script is done, the SCSI Request Packet will be modified > > > to reflect the result of the script. > > > > > > Cc: Jordan Justen > > > Cc: Laszlo Ersek > > > Cc: Ard Biesheuvel > > > Signed-off-by: Gary Lin > > > --- > > > OvmfPkg/Include/IndustryStandard/LsiScsi.h | 39 +++ > > > OvmfPkg/LsiScsiDxe/LsiScsi.c | 308 +++++++++++++++++++++ > > > OvmfPkg/LsiScsiDxe/LsiScsi.h | 21 ++ > > > OvmfPkg/LsiScsiDxe/LsiScsiDxe.inf | 3 + > > > OvmfPkg/OvmfPkg.dec | 3 + > > > 5 files changed, 374 insertions(+) > > > > > > diff --git a/OvmfPkg/Include/IndustryStandard/LsiScsi.h b/OvmfPkg/Include/IndustryStandard/LsiScsi.h > > > index 60e527f1c6a7..cbf049c18310 100644 > > > --- a/OvmfPkg/Include/IndustryStandard/LsiScsi.h > > > +++ b/OvmfPkg/Include/IndustryStandard/LsiScsi.h > > > @@ -26,6 +26,18 @@ > > > #define LSI_REG_SIST0 0x42 > > > #define LSI_REG_SIST1 0x43 > > > > > > +// > > > +// The status bits for DMA Status (DSTAT) > > > +// > > > +#define LSI_DSTAT_IID 0x01 > > > +#define LSI_DSTAT_R 0x02 > > > +#define LSI_DSTAT_SIR 0x04 > > > +#define LSI_DSTAT_SSI 0x08 > > > +#define LSI_DSTAT_ABRT 0x10 > > > +#define LSI_DSTAT_BF 0x20 > > > +#define LSI_DSTAT_MDPE 0x40 > > > +#define LSI_DSTAT_DFE 0x80 > > > + > > > > (1) Please use the BITx macros. > > > Will fix it. > > > > > > // > > > // The status bits for Interrupt Status Zero (ISTAT0) > > > // > > > @@ -38,4 +50,31 @@ > > > #define LSI_ISTAT0_SRST 0x40 > > > #define LSI_ISTAT0_ABRT 0x80 > > > > > > +// > > > +// LSI 53C895A Script Instructions > > > +// > > > +#define LSI_INS_TYPE_BLK 0x00000000 > > > +#define LSI_INS_TYPE_IO 0x40000000 > > > +#define LSI_INS_TYPE_TC 0x80000000 > > > + > > > +#define LSI_INS_BLK_SCSIP_DAT_OUT 0x00000000 > > > +#define LSI_INS_BLK_SCSIP_DAT_IN 0x01000000 > > > +#define LSI_INS_BLK_SCSIP_CMD 0x02000000 > > > +#define LSI_INS_BLK_SCSIP_STAT 0x03000000 > > > +#define LSI_INS_BLK_SCSIP_MSG_OUT 0x06000000 > > > +#define LSI_INS_BLK_SCSIP_MSG_IN 0x07000000 > > > + > > > +#define LSI_INS_IO_OPC_SEL 0x00000000 > > > +#define LSI_INS_IO_OPC_WAIT_RESEL 0x10000000 > > > + > > > +#define LSI_INS_TC_CP 0x00020000 > > > +#define LSI_INS_TC_JMP 0x00080000 > > > +#define LSI_INS_TC_RA 0x00800000 > > > + > > > +#define LSI_INS_TC_OPC_JMP 0x00000000 > > > +#define LSI_INS_TC_OPC_INT 0x18000000 > > > + > > > +#define LSI_INS_TC_SCSIP_DAT_OUT 0x00000000 > > > +#define LSI_INS_TC_SCSIP_MSG_IN 0x07000000 > > > + > > > #endif // _LSI_SCSI_H_ > > > diff --git a/OvmfPkg/LsiScsiDxe/LsiScsi.c b/OvmfPkg/LsiScsiDxe/LsiScsi.c > > > index 1bcebd92e455..090d7df15b34 100644 > > > --- a/OvmfPkg/LsiScsiDxe/LsiScsi.c > > > +++ b/OvmfPkg/LsiScsiDxe/LsiScsi.c > > > @@ -43,6 +43,42 @@ Out8 ( > > > ); > > > } > > > > > > +STATIC > > > +EFI_STATUS > > > +Out32 ( > > > + IN LSI_SCSI_DEV *Dev, > > > + IN UINT32 Addr, > > > + IN UINT32 Data > > > + ) > > > +{ > > > + return Dev->PciIo->Io.Write ( > > > + Dev->PciIo, > > > + EfiPciIoWidthUint32, > > > + PCI_BAR_IDX0, > > > + Addr, > > > + 1, > > > + &Data > > > + ); > > > +} > > > + > > > +STATIC > > > +EFI_STATUS > > > +In8 ( > > > + IN LSI_SCSI_DEV *Dev, > > > + IN UINT32 Addr, > > > + OUT UINT8 *Data > > > + ) > > > +{ > > > + return Dev->PciIo->Io.Read ( > > > + Dev->PciIo, > > > + EfiPciIoWidthUint8, > > > + PCI_BAR_IDX0, > > > + Addr, > > > + 1, > > > + Data > > > + ); > > > +} > > > + > > > STATIC > > > EFI_STATUS > > > LsiScsiReset ( > > > @@ -141,6 +177,272 @@ LsiScsiCheckRequest ( > > > return EFI_SUCCESS; > > > } > > > > > > +/** > > > + > > > + Interpret the request packet from the Extended SCSI Pass Thru Protocol and > > > + compose the script to submit the command and data to the contorller. > > > > (2) s/contorller/controller/ > > > Oops. Will fix it. > > > > > > + > > > + @param[in] Dev The LSI 53C895A SCSI device the packet targets. > > > + > > > + @param[in] Target The SCSI target controlled by the LSI 53C895A SCSI > > > + device. > > > + > > > + @param[in] Lun The Logical Unit Number under the SCSI target. > > > + > > > + @param[in out] Packet The Extended SCSI Pass Thru Protocol packet. > > > + > > > + > > > + @retval EFI_SUCCESS The Extended SCSI Pass Thru Protocol packet was valid. > > > + > > > + @return Otherwise, invalid or unsupported parameters were > > > + detected. Status codes are meant for direct forwarding > > > + by the EFI_EXT_SCSI_PASS_THRU_PROTOCOL.PassThru() > > > + implementation. > > > + > > > + **/ > > > +STATIC > > > +EFI_STATUS > > > +LsiScsiProcessRequest ( > > > + IN LSI_SCSI_DEV *Dev, > > > + IN UINT8 Target, > > > + IN UINT64 Lun, > > > + IN OUT EFI_EXT_SCSI_PASS_THRU_SCSI_REQUEST_PACKET *Packet > > > + ) > > > +{ > > > + EFI_STATUS Status; > > > + UINT32 *Script; > > > + UINT8 *Cdb; > > > + UINT8 *MsgOut; > > > + UINT8 *MsgIn; > > > + UINT8 *ScsiStatus; > > > + UINT8 *Data; > > > + UINT8 DStat; > > > + UINT8 SIst0; > > > + UINT8 SIst1; > > > + > > > + Script = Dev->Dma->Script; > > > + Cdb = Dev->Dma->Cdb; > > > + Data = Dev->Dma->Data; > > > + MsgIn = Dev->Dma->MsgIn; > > > + MsgOut = &Dev->Dma->MsgOut; > > > + ScsiStatus = &Dev->Dma->Status; > > > + > > > + *ScsiStatus = 0xFF; > > > + > > > + SetMem (Cdb, sizeof Dev->Dma->Cdb, 0x00); > > > + CopyMem (Cdb, Packet->Cdb, Packet->CdbLength); > > > + > > > + // > > > + // Clean up the DMA buffer for the script. > > > + // > > > + SetMem (Script, sizeof Dev->Dma->Script, 0x00); > > > + > > > + // > > > + // Compose the script to transfer data between the host and the device. > > > + // > > > + // Reference: > > > + // LSI53C895A PCI to Ultra2 SCSI Controller Version 2.2 > > > + // - Chapter 5 SCSI SCRIPT Instruction Set > > > + // > > > + // All instructions used here consist of 2 32bit words. The first word > > > + // contains the command to execute. The second word is loaded into the > > > + // DMA SCRIPTS Pointer Save (DSPS) register as either the DMA address > > > + // for data transmission or the address/offset for the jump command. > > > + // Some commands, such as the selection of the target, don't need to > > > + // transfer data through DMA or jump to another instruction, then DSPS > > > + // has to be zero. > > > + // > > > + // There are 3 major parts in this script. The first part (1~3) contains > > > + // the instructions to select target and LUN and send the SCSI command > > > + // from the request packet. The second part (4~7) is to handle the > > > + // potential disconnection and prepare for the data transmission. The > > > + // instructions in the third part (8~10) transmit the given data and > > > + // collect the result. Instruction 11 raises the interrupt and marks the > > > + // end of the script. > > > + // > > > + > > > + // > > > + // 1. Select target. > > > + // > > > + *Script++ = LSI_INS_TYPE_IO | LSI_INS_IO_OPC_SEL | (UINT32)Target << 16; > > > + *Script++ = 0x00000000; > > > + > > > + // > > > + // 2. Select LUN. > > > + // > > > + *MsgOut = 0x80 | (UINT8) Lun; // 0x80: Identify bit > > > > This implies that the maximum (inclusive) LUN can be 127. > > > > (3) I suggest adding a STATIC_ASSERT() to LsiScsiControllerStart(), > > where you fetch "PcdLsiScsiMaxLunLimit". > > > > (If the device has a stricter limit on LUNs than 127, then please use > > that limit.) > > > Ok, will add an assert for MaxLun. > > > > > > + *Script++ = LSI_INS_TYPE_BLK | LSI_INS_BLK_SCSIP_MSG_OUT | \ > > > + sizeof Dev->Dma->MsgOut; > > > > (4) Please cast the result of the "sizeof" operator to UINT32 > > explicitly. > > > > > > (5) Please drop the backslash; it is not needed. > > > > Note that (4) and (5) apply to multiple locations in this function; > > please update each location in the composition of the script. > > > Will fix them in v2. > > > > > > + *Script++ = LSI_SCSI_DMA_ADDR_LOW (Dev, MsgOut); > > > > This opcode seems to mean that the device is not capable of taking the > > LUN selector message from outside of the 32-bit address space. > > > > (6) If that's the case, then please remove DUAL_ADDRESS_CYCLE from > > "[PATCH 08/11] OvmfPkg/LsiScsiDxe: Map DMA buffer". > > > > Otherwise, AllocateBuffer() could theoretically allocate the buffer > > structure (including the MsgOut member) such that > > LSI_SCSI_DMA_ADDR_LOW() would truncate the address. > > > > > > (7) Once you remove DUAL_ADDRESS_CYCLE (i.e. once we guarantee that the > > allocation will be satisfied from the 32-bit address space), you can > > drop the _LOW suffix as well, from the macro name. > > > > Now, if the device is actually 64-bit capable, only it would take a more > > complex script, and you don't want that, that's fine -- the above steps > > remain the same. (IOW it doesn't matter whether the device is 64-bit > > uncapable, or you don't want to write a 64-bit aware command script -- > > you should remove DUAL_ADDRESS_CYCLE just the same.) > > > Ok. Will remove DUAL_ADDRESS_CYCLE. > > > > > > + > > > + // > > > + // 3. Send the SCSI Command. > > > + // > > > + *Script++ = LSI_INS_TYPE_BLK | LSI_INS_BLK_SCSIP_CMD | \ > > > + sizeof Dev->Dma->Cdb; > > > + *Script++ = LSI_SCSI_DMA_ADDR_LOW (Dev, Cdb); > > > + > > > + // > > > + // 4. Check whether the current SCSI phase is "Message In" or not > > > + // and jump to 8 if it is. > > > + // > > > + *Script++ = LSI_INS_TYPE_TC | LSI_INS_TC_OPC_JMP | \ > > > + LSI_INS_TC_SCSIP_MSG_IN | LSI_INS_TC_RA | \ > > > + LSI_INS_TC_CP; > > > + *Script++ = 0x00000018; > > > > (8) The constant 0x18 is obscure. Can you extend the comment? How is > > "step 8" connected to value 0x18? > > > > (Step 8 starts at command offset 7 in the script, meaning UINT32 offset > > 14, or byte offset 56. But 0x18 is 24, not matching any of those > > constants.) > > > Urghhh, it should be "jump to 7". LSI_INS_TC_RA stands for "Relative > Addressing Mode", so it's 4 + 24/8 = 7. Will fix the comment. > > > > > > + > > > + // > > > + // 5. Read "Message" from the initiator to trigger reselect. > > > + // > > > + *Script++ = LSI_INS_TYPE_BLK | LSI_INS_BLK_SCSIP_MSG_IN | \ > > > + sizeof Dev->Dma->MsgIn; > > > + *Script++ = LSI_SCSI_DMA_ADDR_LOW (Dev, MsgIn); > > > + > > > + // > > > + // 6. Wait reselect. > > > + // > > > + *Script++ = LSI_INS_TYPE_IO | LSI_INS_IO_OPC_WAIT_RESEL; > > > + *Script++ = 0x00000000; > > > + > > > + // > > > + // 7. Read "Message" from the initiator again > > > + // > > > + *Script++ = LSI_INS_TYPE_BLK | LSI_INS_BLK_SCSIP_MSG_IN | \ > > > + sizeof Dev->Dma->MsgIn; > > > + *Script++ = LSI_SCSI_DMA_ADDR_LOW (Dev, MsgIn); > > > + > > > + // > > > + // 8. Set the DMA command for the read/write operations. > > > + // > > > + if (Packet->DataDirection == EFI_EXT_SCSI_DATA_DIRECTION_READ && > > > + Packet->InTransferLength > 0) { > > > + *Script++ = LSI_INS_TYPE_BLK | LSI_INS_BLK_SCSIP_DAT_IN | \ > > > + Packet->InTransferLength; > > > + *Script++ = LSI_SCSI_DMA_ADDR_LOW (Dev, Data); > > > + } else if (Packet->DataDirection == EFI_EXT_SCSI_DATA_DIRECTION_WRITE && > > > + Packet->OutTransferLength > 0) { > > > + // LsiScsiCheckRequest() guarantees that OutTransferLength is no > > > + // larger than sizeof Dev->Dma->Data, so we can safely copy the > > > + // the data to Dev->Dma->Data. > > > + CopyMem (Data, Packet->OutDataBuffer, Packet->OutTransferLength); > > > + *Script++ = LSI_INS_TYPE_BLK | LSI_INS_BLK_SCSIP_DAT_OUT | \ > > > + Packet->OutTransferLength; > > > + *Script++ = LSI_SCSI_DMA_ADDR_LOW (Dev, Data); > > > + } > > > > The comment is not bad, but it could be improved. > > > > First, the comment style is not 100% correct (empty "//" lines are > > missing before and after.) > > > > Second, instead of a long comment, I'd suggest a short comment, plus an > > ASSERT. > > > Will amend the comment and add an assert. > > > Third, the transfer length in the packet ("in" and "out" alike) isn't > > *only* relevant due to buffer overflow concerns, but also because the > > command opcode has some bits set (in particular, > > LSI_INS_BLK_SCSIP_DAT_IN) that effectively limit the transfer length to > > a bitmask that's narrower than a UINT32. > > > Yeah, the controller only takes 24 bits for the transfer length. > > > > > Another observation is that control can flow through this snippet > > without taking *either* branch. > > > > (For example if the data direction is WRITE, but OutTransferLength is > > zero. That's a condition that LsiScsiCheckRequest() does not catch. My > > point is not that LsiScsiCheckRequest() should catch it -- I think it's > > not an invalid request --, but that LsiScsiProcessRequest() should deal > > with it.) > > > Hmmm, this is possible when ScsiIo requests sense data throught > Packet->SenseData, and it's a read request with 0 InTransferLength. > I encountered that during the development of this driver. It's gone after > I set SenseDataLength to 0 later to make ScsiIo request sense data through > Transfer buffer. Maybe I should filter this kind of requests in > LsiScsiCheckRequest(). > > > If we take neither branch, then (minimally) the jump address under step > > 4. will be wrong -- there's not going to be a DAT_IN / DAT_OUT command > > to jump to. > > > > > > (9) So, for addressing all of the above, I suggest: > > > > // > > // 8. Set the DMA command for the read/write operations. > > // > > // LsiScsiCheckRequest() prevents both integer overflows in the command > > // opcodes, and buffer overflows. > > // > > if (Packet->DataDirection == EFI_EXT_SCSI_DATA_DIRECTION_READ) { > > ASSERT (Packet->InTransferLength <= sizeof Dev->Dma->Data); > > *Script++ = LSI_INS_TYPE_BLK | LSI_INS_BLK_SCSIP_DAT_IN | > > Packet->InTransferLength; > > } else { > > ASSERT (Packet->OutTransferLength <= sizeof Dev->Dma->Data); > > CopyMem (Data, Packet->OutDataBuffer, Packet->OutTransferLength); > > *Script++ = LSI_INS_TYPE_BLK | LSI_INS_BLK_SCSIP_DAT_OUT | > > Packet->OutTransferLength; > > } > > *Script++ = LSI_SCSI_DMA_ADDR (Dev, Data); > > > Thanks. Will modify the code here. > The above code actually doesn't work because "TEST UNIT READY", the scsi command, generates a request packet with 0 InTransferLength, and the controller returns an error for data underflow. The original code skips the DMA command for the 0 InTransferLength requests and avoids the error. I'll keep the original code and add some comments about that error. Gary Lin > > > > > + > > > + // > > > + // 9. Get the SCSI status. > > > + // > > > + *Script++ = LSI_INS_TYPE_BLK | LSI_INS_BLK_SCSIP_STAT | \ > > > + sizeof Dev->Dma->Status; > > > + *Script++ = LSI_SCSI_DMA_ADDR_LOW (Dev, Status); > > > + > > > + // > > > + // 10. Get the SCSI message. > > > + // > > > + *Script++ = LSI_INS_TYPE_BLK | LSI_INS_BLK_SCSIP_MSG_IN | \ > > > + sizeof Dev->Dma->MsgIn; > > > + *Script++ = LSI_SCSI_DMA_ADDR_LOW (Dev, MsgIn); > > > + > > > + // > > > + // 11. Raise the interrupt to end the script. > > > + // > > > + *Script++ = LSI_INS_TYPE_TC | LSI_INS_TC_OPC_INT | \ > > > + LSI_INS_TC_SCSIP_DAT_OUT | LSI_INS_TC_JMP; > > > + *Script++ = 0x00000000; > > > + > > > + // > > > + // Make sure the size of the script doesn't exceed the buffer. > > > + // > > > + ASSERT (Script < Dev->Dma->Script + sizeof Dev->Dma->Script); > > > > (10) Please replace "<" with "<=". > > > > (11) Please replace "sizeof" with ARRAY_SIZE(). > > > Will fix it. > > > > > > + > > > + // > > > + // The controller starts to execute the script once the DMA Script > > > + // Pointer (DSP) register is set. > > > + // > > > + Status = Out32 (Dev, LSI_REG_DSP, LSI_SCSI_DMA_ADDR_LOW(Dev, Script)); > > > > (12) Missing space after "LSI_SCSI_DMA_ADDR". > > > Ditto. > > > > > > + if (EFI_ERROR (Status)) { > > > + return Status; > > > + } > > > > This return statement doesn't seem right. We do not translate the error > > code to one of the error codes specified for > > EFI_EXT_SCSI_PASS_THRU_PROTOCOL.PassThru(). We also do not set any > > output field in Packet. > > > > (13) I think we should jump to the "Error" label from here. > > > Right. It should jump to "Error". > > > > > > + > > > + // > > > + // Poll the device registers (DSTAT, SIST0, and SIST1) until the SIR > > > + // bit sets. > > > + // > > > + for(;;) { > > > + Status = In8 (Dev, LSI_REG_DSTAT, &DStat); > > > + if (EFI_ERROR (Status)) { > > > + goto Error; > > > + } > > > + Status = In8 (Dev, LSI_REG_SIST0, &SIst0); > > > + if (EFI_ERROR (Status)) { > > > + goto Error; > > > + } > > > + Status = In8 (Dev, LSI_REG_SIST1, &SIst1); > > > + if (EFI_ERROR (Status)) { > > > + goto Error; > > > + } > > > + > > > + if (SIst0 != 0 || SIst1 != 0) { > > > + goto Error; > > > + } > > > + > > > + // > > > + // Check the SIR (SCRIPTS Interrupt Instruction Received) bit. > > > + // > > > + if (DStat & LSI_DSTAT_SIR) { > > > + break; > > > + } > > > + > > > + gBS->Stall (Dev->StallPerPollUsec); > > > + } > > > + > > > + // > > > + // Check if everything is good. > > > + // SCSI Message Code 0x00: COMMAND COMPLETE > > > + // SCSI Status Code 0x00: Good > > > + // > > > + if (MsgIn[0] == 0 && *ScsiStatus == 0) { > > > > (14) Please negate this condition, and unnest the success branch: > > > > if (MsgIn[0] != 0 || *ScsiStatus != 0) { > > goto Error; > > } > > > > /* ... */ > > > > return EFI_SUCCESS; > > > > > Ok. Will do that. > > > > + // > > > + // Copy Data to InDataBuffer if necessary. > > > + // > > > + if (Packet->DataDirection == EFI_EXT_SCSI_DATA_DIRECTION_READ) { > > > + CopyMem (Packet->InDataBuffer, Data, Packet->InTransferLength); > > > + } > > > + > > > + // > > > + // The controller doesn't return sense data when replying "TEST UNIT READY", > > > + // so we have to set SenseDataLength to 0 to notify ScsiIo to issue > > > + // "REQUEST SENSE" for the sense data. > > > + // > > > + if (Cdb[0] == 0x00) { > > > + Packet->SenseDataLength = 0; > > > + } > > > + Packet->HostAdapterStatus = EFI_EXT_SCSI_STATUS_HOST_ADAPTER_OK; > > > + Packet->TargetStatus = EFI_EXT_SCSI_STATUS_TARGET_GOOD; > > > + > > > + return EFI_SUCCESS; > > > + } > > > > The mapping on success does not deal with short transfers (in or out) -- > > we're supposed to update InTransferLength / OutTransferLength on output. > > > > (15) Does the device report short transfers somehow? > > > I'm not sure. Have to check the spec of controller to see if there is > any instruction to fetch the real transfer length. > > > > > The spec says about SenseDataLength, "On input, the length in bytes of > > the SenseData buffer. On output, the number of bytes written to the > > SenseData buffer". > > > > But we only set SenseDataLength conditionally. In case we do *not* set > > it, the caller may attempt to parse garbage from the "SenseData" buffer. > > > > (16) We should either get real sense data back to the caller, or always > > zero out "SenseDataLength". > > > > > For simplicity, I would choose zeroing out "SenseDataLength" since > ScsiDiskDxe can handle it. > > > > + > > > +Error: > > > + DEBUG((DEBUG_VERBOSE, "%a: dstat: %02X, sist0: %02X, sist1: %02X\n", > > > + __FUNCTION__, DStat, SIst0, SIst1)); > > > > (17) missing space after "DEBUG". > > > > (18) The line starting with __FUNCTION__ is not correctly indented. > > > Will fix them. > > > > > > + // > > > + // Update the request packet to reflect the status. > > > + // > > > + if (*ScsiStatus != 0xFF) { > > > + Packet->TargetStatus = *ScsiStatus; > > > + } else { > > > + Packet->TargetStatus = EFI_EXT_SCSI_STATUS_TARGET_TASK_ABORTED; > > > + } > > > + Packet->HostAdapterStatus = EFI_EXT_SCSI_STATUS_HOST_ADAPTER_OTHER; > > > + Packet->InTransferLength = 0; > > > + Packet->OutTransferLength = 0; > > > + Packet->SenseDataLength = 0; > > > + > > > + return EFI_DEVICE_ERROR; > > > +} > > > + > > > // > > > // The next seven functions implement EFI_EXT_SCSI_PASS_THRU_PROTOCOL > > > // for the LSI 53C895A SCSI Controller. Refer to UEFI Spec 2.3.1 + Errata C, > > > @@ -168,6 +470,11 @@ LsiScsiPassThru ( > > > return Status; > > > } > > > > > > + Status = LsiScsiProcessRequest (Dev, *Target, Lun, Packet); > > > + if (EFI_ERROR (Status)) { > > > + return Status; > > > + } > > > + > > > return EFI_SUCCESS; > > > } > > > > > > > (19) I think this can be simplified -- just return Status after calling > > LsiScsiProcessRequest(). > > > Agree. > > > > > > @@ -469,6 +776,7 @@ LsiScsiControllerStart ( > > > > > > Dev->MaxTarget = PcdGet8 (PcdLsiScsiMaxTargetLimit); > > > Dev->MaxLun = PcdGet8 (PcdLsiScsiMaxLunLimit); > > > + Dev->StallPerPollUsec = PcdGet32 (PcdLsiScsiStallPerPollUsec); > > > > > > Status = gBS->OpenProtocol ( > > > ControllerHandle, > > > diff --git a/OvmfPkg/LsiScsiDxe/LsiScsi.h b/OvmfPkg/LsiScsiDxe/LsiScsi.h > > > index 9272eb7506c7..1a16ef9f7795 100644 > > > --- a/OvmfPkg/LsiScsiDxe/LsiScsi.h > > > +++ b/OvmfPkg/LsiScsiDxe/LsiScsi.h > > > @@ -13,6 +13,11 @@ > > > #define _LSI_SCSI_DXE_H_ > > > > > > typedef struct { > > > + // > > > + // Allocate 32 UINT32 entries for the script and it's sufficient for > > > + // 16 instructions. > > > + // > > > + UINT32 Script[32]; > > > // > > > // The max size of CDB is 32. > > > // > > > @@ -21,6 +26,18 @@ typedef struct { > > > // Allocate 64KB for read/write buffer. > > > // > > > UINT8 Data[0x10000]; > > > + // > > > + // For SCSI Message In phase > > > + // > > > + UINT8 MsgIn[2]; > > > + // > > > + // For SCSI Message Out phase > > > + // > > > + UINT8 MsgOut; > > > + // > > > + // For SCSI Status phase > > > + // > > > + UINT8 Status; > > > } LSI_SCSI_DMA_BUFFER; > > > > > > typedef struct { > > > @@ -30,6 +47,7 @@ typedef struct { > > > EFI_PCI_IO_PROTOCOL *PciIo; > > > UINT8 MaxTarget; > > > UINT8 MaxLun; > > > + UINT32 StallPerPollUsec; > > > LSI_SCSI_DMA_BUFFER *Dma; > > > EFI_PHYSICAL_ADDRESS DmaPhysical; > > > VOID *DmaMapping; > > > @@ -42,6 +60,9 @@ typedef struct { > > > #define LSI_SCSI_FROM_PASS_THRU(PassThruPtr) \ > > > CR (PassThruPtr, LSI_SCSI_DEV, PassThru, LSI_SCSI_DEV_SIGNATURE) > > > > > > +#define LSI_SCSI_DMA_ADDR_LOW(Dev, MemberName) \ > > > + ((UINT32)(Dev->DmaPhysical + OFFSET_OF (LSI_SCSI_DMA_BUFFER, MemberName))) > > > + > > > > > > // > > > // Probe, start and stop functions of this driver, called by the DXE core for > > > diff --git a/OvmfPkg/LsiScsiDxe/LsiScsiDxe.inf b/OvmfPkg/LsiScsiDxe/LsiScsiDxe.inf > > > index 68844c6772e3..cbd7294573ac 100644 > > > --- a/OvmfPkg/LsiScsiDxe/LsiScsiDxe.inf > > > +++ b/OvmfPkg/LsiScsiDxe/LsiScsiDxe.inf > > > @@ -41,3 +41,6 @@ [Protocols] > > > [FixedPcd] > > > gUefiOvmfPkgTokenSpaceGuid.PcdLsiScsiMaxTargetLimit ## CONSUMES > > > gUefiOvmfPkgTokenSpaceGuid.PcdLsiScsiMaxLunLimit ## CONSUMES > > > + > > > +[Pcd] > > > + gUefiOvmfPkgTokenSpaceGuid.PcdLsiScsiStallPerPollUsec ## CONSUMES > > > diff --git a/OvmfPkg/OvmfPkg.dec b/OvmfPkg/OvmfPkg.dec > > > index ae7d1d648d22..57e418d4b8a9 100644 > > > --- a/OvmfPkg/OvmfPkg.dec > > > +++ b/OvmfPkg/OvmfPkg.dec > > > @@ -179,6 +179,9 @@ [PcdsFixedAtBuild] > > > gUefiOvmfPkgTokenSpaceGuid.PcdLsiScsiMaxTargetLimit|7|UINT8|0x3b > > > gUefiOvmfPkgTokenSpaceGuid.PcdLsiScsiMaxLunLimit|0|UINT8|0x3c > > > > > > + ## Microseconds to stall between polling for LsiScsi request result > > > + gUefiOvmfPkgTokenSpaceGuid.PcdLsiScsiStallPerPollUsec|5|UINT32|0x3d > > > + > > > > (20) The token value should be 0x3c here, according to my request (5) > > under "[PATCH 06/11] OvmfPkg/LsiScsiDxe: Report Targets and LUNs". > > > Ok, will do that if we won't change the toke value of PcdMptScsiStallPerPollUsec. > > Thanks, > > Gary Lin > > > > > > gUefiOvmfPkgTokenSpaceGuid.PcdOvmfFlashNvStorageEventLogBase|0x0|UINT32|0x8 > > > gUefiOvmfPkgTokenSpaceGuid.PcdOvmfFlashNvStorageEventLogSize|0x0|UINT32|0x9 > > > gUefiOvmfPkgTokenSpaceGuid.PcdOvmfFirmwareFdSize|0x0|UINT32|0xa > > > > > > > Thanks > > Laszlo > > > > > >