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.web11.3569.1594952940303616242 for ; Thu, 16 Jul 2020 19:29:00 -0700 Authentication-Results: mx.groups.io; dkim=pass header.i=@suse.com header.s=mimecast20200619 header.b=eucmDa8o; 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=1594952937; 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=KreGlHBNqO5JKjQJHs+pOy4XYzWvyS/zWl23xeAYvIE=; b=eucmDa8o78FYOA0VV2Mnp7EF+QmntsWINwTYkxHAXhB55HhVHOB7ahcht4eRRnRey7VpZx dxgUGkWIJEgj9OG4+F6vh2ZJxQfSxj4PCe7efRpmdSK7Im39KFWsTzNO+5YT56SXK7wEiA mpcqZgyQtrdUBeK1l2pmcIvmlGmx3uA= Received: from EUR02-AM5-obe.outbound.protection.outlook.com (mail-am5eur02lp2057.outbound.protection.outlook.com [104.47.4.57]) (Using TLS) by relay.mimecast.com with ESMTP id de-mta-11-1w7X3c-AMoab5CrTM_H45A-1; Fri, 17 Jul 2020 04:28:55 +0200 X-MC-Unique: 1w7X3c-AMoab5CrTM_H45A-1 ARC-Seal: i=1; a=rsa-sha256; s=arcselector9901; d=microsoft.com; cv=none; b=MwhwsF8vV+2bpJgkBkZjhq3Ztq4rUR275Oiysr/Gq1vrRFUgrSHPRwNWeDSexe113msGYU5w52u1fjsOXw8k0kbt5njJyPy01vTSMFJ6QA5AfvQBbW6CZgrs60Yz61lDQ4PbVzyi3M0r2I1nCrvCTeObMOx4Q3CpTQYrrVgV86A0Ghx/K8cybzac44uiKrRNAVlCSNeBzijJuOXZBqPTA+S2SjF+I+I16qpADnYM3D2arYYZWn6zNVvkZSQhUXbEeULNAjHZXg2PmH2n7zvI72QdqhuwMrzLqOwNyZDuJiBADnUW5aTupNm8NUDKbS+qFXt3c8oguXbWuSRBo1NXUA== 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=KreGlHBNqO5JKjQJHs+pOy4XYzWvyS/zWl23xeAYvIE=; b=EUsY10rI6YFg6Z823OMUJCf0HmxGTgL5VjIVixSwpCEmMKSP1mI5HDXBUTcQ4nGLpbSRiOy/T8ygJrOUUXxhgY2n79EjMeb+oqea3+f+cH/mO+AgdCEBDOvDdni5qIIir4Ot9ofK/pJ23aRhOMV0yly0cbpBjghr7/duzS8pR75XP8145llRBln0OPqR3LwgfygkPP6TitL2RH+y1UCBNLx8JxmManoaATl+B3czGYmhLu2Kr0akieBZQu2KY1rP32J3+h7iCaLpgloRzfyBRg2IVQeUXA3urkoeet1JEq0hs3S3/wmOkJUQP2Cjx/mwyKeXehROntcHWnM/fe1Y1w== 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: redhat.com; dkim=none (message not signed) header.d=none;redhat.com; dmarc=none action=none header.from=suse.com; Received: from AM0PR0402MB3809.eurprd04.prod.outlook.com (2603:10a6:208:10::30) by AM0PR04MB7041.eurprd04.prod.outlook.com (2603:10a6:208:19a::13) with Microsoft SMTP Server (version=TLS1_2, cipher=TLS_ECDHE_RSA_WITH_AES_256_GCM_SHA384) id 15.20.3174.22; Fri, 17 Jul 2020 02:28:54 +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.026; Fri, 17 Jul 2020 02:28:54 +0000 Date: Fri, 17 Jul 2020 10:28:45 +0800 From: "Gary Lin" To: Laszlo Ersek Cc: devel@edk2.groups.io, Jordan Justen , Ard Biesheuvel Subject: Re: [PATCH v2 10/12] OvmfPkg/LsiScsiDxe: Process the SCSI Request Packet Message-ID: <20200717022845.GN6058@GaryWorkstation> References: <20200716074607.18048-1-glin@suse.com> <20200716074607.18048-11-glin@suse.com> <10f3d64e-af1b-bd94-2ec3-92d0fb30d6cc@redhat.com> In-Reply-To: <10f3d64e-af1b-bd94-2ec3-92d0fb30d6cc@redhat.com> X-ClientProxiedBy: AM0PR02CA0023.eurprd02.prod.outlook.com (2603:10a6:208:3e::36) 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 AM0PR02CA0023.eurprd02.prod.outlook.com (2603:10a6:208:3e::36) with Microsoft SMTP Server (version=TLS1_2, cipher=TLS_ECDHE_RSA_WITH_AES_256_GCM_SHA384) id 15.20.3195.18 via Frontend Transport; Fri, 17 Jul 2020 02:28:51 +0000 X-Originating-IP: [60.251.47.115] X-MS-PublicTrafficType: Email X-MS-Office365-Filtering-Correlation-Id: c94c8766-9475-4f35-7cdc-08d829f925d4 X-MS-TrafficTypeDiagnostic: AM0PR04MB7041: 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: YacwomIWxrzoPrMeDmnpc+w4BjNqVLJ1DDp2TGiPbFjpQd/Nb7O+S7lWmdVOvUSr2kShvDaSYuA5xYgrv0K/o3cG6HWRj7cNVsQCgAtqWnttI2dT6X+zozesHisXm7Y0ShbyTxMqzGNKkvCM/xK+JChhhz4idZ5AJlKL9JnyVZCsZ88P01LwVC+kjwePgPrRstz8zXaUjwZEGEXfKEEO9D28WNojH1IOf0pfgJ4Zwhn5miIulsqcbLxAP/N7dykZky3BW/X2Rp0Mb1ORgoPSL3vSjrAWnroYEHBin/JKtw6TUOj6p0TDPC+WvC3XUGpYjO5S64CNeILUElSEoS4n4Q== 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:(366004)(376002)(39860400002)(136003)(346002)(396003)(83380400001)(6916009)(4326008)(5660300002)(6496006)(8936002)(33716001)(478600001)(2906002)(26005)(66556008)(55016002)(66476007)(52116002)(30864003)(66946007)(8676002)(86362001)(6666004)(1076003)(316002)(54906003)(9686003)(16526019)(186003)(956004)(55236004)(33656002)(53546011);DIR:OUT;SFP:1101; X-MS-Exchange-AntiSpam-MessageData: X1mnjvhpQBb95ZiUs3uVoKoNg0UXNFVtVkT3PYa/mNTdvXIx9rJge5/fA5B4uqguw26rjRwqnSmi1EZF3man/fqYDCRyjavOprSB0nUmc/tjWTzubved2zjuFE+dO5Q14L/KHsez4zaKvxAhjGmlZwZyaVFXCq1TqJ++Fi0pQIp7cXZx9v4ZhTneLk7n9zEzsguPElDYcJmmBGZ/PK3zGfEyas+H0XiuuN93qcBJhRQW33QbMyO5lFmwR/YSeUTXiTL4QSqaIfM9yGafx2dxBiWsN90WUU70hbxezB/7TLJF4lVSZv9OShW10Le3JPGonO6VS3bB3bPP4tCKRMnnIrCOvj0Yb8ns69rIf/Ku2QL8OZepnAio6Upfhr1HKxRb+auZmPxkowAnH+Sk91IwMPjkddjXYfUSOaeVJXCPUK4Wda6M2f2KVwAQKpKpem0Xz2Ags5r9X8/1RdQEELQ/tI43HFVqyDT/AnUFzpVMorIWQFPjZXrqvPShx2ceLDVa X-OriginatorOrg: suse.com X-MS-Exchange-CrossTenant-Network-Message-Id: c94c8766-9475-4f35-7cdc-08d829f925d4 X-MS-Exchange-CrossTenant-AuthSource: AM0PR0402MB3809.eurprd04.prod.outlook.com X-MS-Exchange-CrossTenant-AuthAs: Internal X-MS-Exchange-CrossTenant-OriginalArrivalTime: 17 Jul 2020 02:28:53.8289 (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: vtI9gOFhPqLwDqv2q2vbxPwVfdHFupSQ0httLfa9+MxyR+H4lYzp6ik3dd6lWzcB X-MS-Exchange-Transport-CrossTenantHeadersStamped: AM0PR04MB7041 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline On Thu, Jul 16, 2020 at 07:37:09PM +0200, Laszlo Ersek wrote: > On 07/16/20 09:46, 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. > > > > v2: > > - Use the BITx macros for the most of LSI_* constants > > - Fix a typo: contorller => controller > > - Add SeaBIOS lsi-scsi driver as one of the references of the script > > - Cast the result of sizeof to UINT32 for the instructions of the > > script > > - Drop the backslashes > > - Replace LSI_SCSI_DMA_ADDR_LOW with LSI_SCSI_DMA_ADDR since we > > already removed DUAL_ADDRESS_CYCLE > > - Add more comments for the script > > - Fix the check of the script size at the end of the script > > - Always set SenseDataLength to 0 to avoid the caller to access > > SenseData > > - Improve the error handling in LsiScsiProcessRequest() > > > > Cc: Jordan Justen > > Cc: Laszlo Ersek > > Cc: Ard Biesheuvel > > Signed-off-by: Gary Lin > > --- > > OvmfPkg/Include/IndustryStandard/LsiScsi.h | 63 ++++ > > OvmfPkg/LsiScsiDxe/LsiScsi.c | 336 ++++++++++++++++++++- > > OvmfPkg/LsiScsiDxe/LsiScsi.h | 21 ++ > > OvmfPkg/LsiScsiDxe/LsiScsiDxe.inf | 3 + > > OvmfPkg/OvmfPkg.dec | 3 + > > 5 files changed, 425 insertions(+), 1 deletion(-) > > > > diff --git a/OvmfPkg/Include/IndustryStandard/LsiScsi.h b/OvmfPkg/Include/IndustryStandard/LsiScsi.h > > index 185e553c8eb4..9964bd40385c 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 BIT0 > > +#define LSI_DSTAT_R BIT1 > > +#define LSI_DSTAT_SIR BIT2 > > +#define LSI_DSTAT_SSI BIT3 > > +#define LSI_DSTAT_ABRT BIT4 > > +#define LSI_DSTAT_BF BIT5 > > +#define LSI_DSTAT_MDPE BIT6 > > +#define LSI_DSTAT_DFE BIT7 > > + > > // > > // The status bits for Interrupt Status Zero (ISTAT0) > > // > > @@ -38,4 +50,55 @@ > > #define LSI_ISTAT0_SRST BIT6 > > #define LSI_ISTAT0_ABRT BIT7 > > > > +// > > +// The status bits for SCSI Interrupt Status Zero (SIST0) > > +// > > +#define LSI_SIST0_PAR BIT0 > > +#define LSI_SIST0_RST BIT1 > > +#define LSI_SIST0_UDC BIT2 > > +#define LSI_SIST0_SGE BIT3 > > +#define LSI_SIST0_RSL BIT4 > > +#define LSI_SIST0_SEL BIT5 > > +#define LSI_SIST0_CMP BIT6 > > +#define LSI_SIST0_MA BIT7 > > + > > +// > > +// The status bits for SCSI Interrupt Status One (SIST1) > > +// > > +#define LSI_SIST1_HTH BIT0 > > +#define LSI_SIST1_GEN BIT1 > > +#define LSI_SIST1_STO BIT2 > > +#define LSI_SIST1_R3 BIT3 > > +#define LSI_SIST1_SBMC BIT4 > > +#define LSI_SIST1_R5 BIT5 > > +#define LSI_SIST1_R6 BIT6 > > +#define LSI_SIST1_R7 BIT7 > > + > > +// > > +// LSI 53C895A Script Instructions > > +// > > +#define LSI_INS_TYPE_BLK 0x00000000 > > +#define LSI_INS_TYPE_IO BIT30 > > +#define LSI_INS_TYPE_TC BIT31 > > + > > +#define LSI_INS_BLK_SCSIP_DAT_OUT 0x00000000 > > +#define LSI_INS_BLK_SCSIP_DAT_IN BIT24 > > +#define LSI_INS_BLK_SCSIP_CMD BIT25 > > +#define LSI_INS_BLK_SCSIP_STAT (BIT24 | BIT25) > > +#define LSI_INS_BLK_SCSIP_MSG_OUT (BIT25 | BIT26) > > +#define LSI_INS_BLK_SCSIP_MSG_IN (BIT24 | BIT25 | BIT26) > > + > > +#define LSI_INS_IO_OPC_SEL 0x00000000 > > +#define LSI_INS_IO_OPC_WAIT_RESEL BIT28 > > + > > +#define LSI_INS_TC_CP BIT17 > > +#define LSI_INS_TC_JMP BIT19 > > +#define LSI_INS_TC_RA BIT23 > > + > > +#define LSI_INS_TC_OPC_JMP 0x00000000 > > +#define LSI_INS_TC_OPC_INT (BIT27 | BIT28) > > + > > +#define LSI_INS_TC_SCSIP_DAT_OUT 0x00000000 > > +#define LSI_INS_TC_SCSIP_MSG_IN (BIT24 | BIT25 | BIT26) > > + > > #endif // _LSI_SCSI_H_ > > diff --git a/OvmfPkg/LsiScsiDxe/LsiScsi.c b/OvmfPkg/LsiScsiDxe/LsiScsi.c > > index b3a88cc7119b..d5fe1d4ab3b8 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,303 @@ 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 controller. > > + > > + @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. > > + // > > + // References: > > + // * LSI53C895A PCI to Ultra2 SCSI Controller Version 2.2 > > + // - Chapter 5 SCSI SCRIPT Instruction Set > > + // * SEABIOS lsi-scsi driver > > + // > > + // 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 > > + *Script++ = LSI_INS_TYPE_BLK | LSI_INS_BLK_SCSIP_MSG_OUT | > > + (UINT32)sizeof Dev->Dma->MsgOut; > > + *Script++ = LSI_SCSI_DMA_ADDR (Dev, MsgOut); > > + > > + // > > + // 3. Send the SCSI Command. > > + // > > + *Script++ = LSI_INS_TYPE_BLK | LSI_INS_BLK_SCSIP_CMD | > > + (UINT32)sizeof Dev->Dma->Cdb; > > + *Script++ = LSI_SCSI_DMA_ADDR (Dev, Cdb); > > + > > + // > > + // 4. Check whether the current SCSI phase is "Message In" or not > > + // and jump to 7 if it is. > > + // Note: LSI_INS_TC_RA stands for "Relative Address Mode", so the > > + // offset 0x18 in the second word means jumping forward > > + // 3 (0x18/8) instructions. > > + // > > + *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; > > + > > + // > > + // 5. Read "Message" from the initiator to trigger reselect. > > + // > > + *Script++ = LSI_INS_TYPE_BLK | LSI_INS_BLK_SCSIP_MSG_IN | > > + (UINT32)sizeof Dev->Dma->MsgIn; > > + *Script++ = LSI_SCSI_DMA_ADDR (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 | > > + (UINT32)sizeof Dev->Dma->MsgIn; > > + *Script++ = LSI_SCSI_DMA_ADDR (Dev, MsgIn); > > + > > + // > > + // 8. Set the DMA command for the read/write operations. > > + // Note: Some requests, e.g. "TEST UNIT READY", do not come with > > + // allocated InDataBuffer or OutDataBuffer. We skip the DMA > > + // data command for those requests or this script would fail > > + // with LSI_SIST0_SGE due to the zero data length. > > + // > > + // LsiScsiCheckRequest() prevents both integer overflows in the command > > + // opcodes, and buffer overflows. > > + // > > + if (Packet->DataDirection == EFI_EXT_SCSI_DATA_DIRECTION_READ && > > + Packet->InTransferLength > 0) { > > + ASSERT (Packet->InTransferLength <= sizeof Dev->Dma->Data); > > + *Script++ = LSI_INS_TYPE_BLK | LSI_INS_BLK_SCSIP_DAT_IN | > > + Packet->InTransferLength; > > + *Script++ = LSI_SCSI_DMA_ADDR (Dev, Data); > > + } else if (Packet->DataDirection == EFI_EXT_SCSI_DATA_DIRECTION_WRITE && > > + Packet->OutTransferLength > 0) { > > + 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); > > + } > > I understand the explanation, thanks. > > However, we can still improve this code. The following sub-conditions: > > Packet->DataDirection == EFI_EXT_SCSI_DATA_DIRECTION_READ > > and > > Packet->DataDirection == EFI_EXT_SCSI_DATA_DIRECTION_WRITE > > are superfluous. What we care about are > > Packet->InTransferLength > 0 > > and > > Packet->OutTransferLength > 0 > > And the latter (length-based) conditions actually *imply* the former > (direction-based) conditions, because: > > - LsiScsiCheckRequest() returns EFI_INVALID_PARAMETER if > DataDirection > EFI_EXT_SCSI_DATA_DIRECTION_BIDIRECTIONAL > > - LsiScsiCheckRequest() returns EFI_UNSUPPORTED if > DataDirection == EFI_EXT_SCSI_DATA_DIRECTION_BIDIRECTIONAL > > - LsiScsiCheckRequest() returns EFI_INVALID_PARAMETER if > InTransferLength > 0, but > DataDirection == EFI_EXT_SCSI_DATA_DIRECTION_WRITE > > - LsiScsiCheckRequest() returns EFI_INVALID_PARAMETER if > OutTransferLength > 0, but > DataDirection == EFI_EXT_SCSI_DATA_DIRECTION_READ > > This means that (InTransferLength > 0) guarantees > EFI_EXT_SCSI_DATA_DIRECTION_READ, and (OutTransferLength > 0) guarantees > EFI_EXT_SCSI_DATA_DIRECTION_WRITE. > > (1) Therefore I suggest moving the respective sub-conditions from the > "if" statements into the dependent blocks, as assertions: > > if (Packet->InTransferLength > 0) { > ASSERT (Packet->DataDirection == EFI_EXT_SCSI_DATA_DIRECTION_READ); > ASSERT (Packet->InTransferLength <= sizeof Dev->Dma->Data); > ... > } else if (Packet->OutTransferLength > 0) { > ASSERT (Packet->DataDirection == EFI_EXT_SCSI_DATA_DIRECTION_WRITE); > ASSERT (Packet->OutTransferLength <= sizeof Dev->Dma->Data); > ... > } > Thanks for the suggestion. This makes the if statements easier to read. Will modify them in v3. > > + > > + // > > + // 9. Get the SCSI status. > > + // > > + *Script++ = LSI_INS_TYPE_BLK | LSI_INS_BLK_SCSIP_STAT | > > + (UINT32)sizeof Dev->Dma->Status; > > + *Script++ = LSI_SCSI_DMA_ADDR (Dev, Status); > > + > > + // > > + // 10. Get the SCSI message. > > + // > > + *Script++ = LSI_INS_TYPE_BLK | LSI_INS_BLK_SCSIP_MSG_IN | > > + (UINT32)sizeof Dev->Dma->MsgIn; > > + *Script++ = LSI_SCSI_DMA_ADDR (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 + ARRAY_SIZE (Dev->Dma->Script)); > > + > > + // > > + // 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 (Dev, Script)); > > + if (EFI_ERROR (Status)) { > > + goto 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) { > > + goto Error; > > + } > > + > > + // > > + // Copy Data to InDataBuffer if necessary. > > + // > > + if (Packet->DataDirection == EFI_EXT_SCSI_DATA_DIRECTION_READ) { > > + CopyMem (Packet->InDataBuffer, Data, Packet->InTransferLength); > > + } > > + > > + // > > + // Always set SenseDataLength to 0. > > + // The instructions of LSI53C895A doesn't reply sense data. Instead, it > > + // relies on the SCSI command, "REQUEST SENSE", to get sense data. We set > > + // SenseDataLength to 0 to notify ScsiDiskDxe that there is no sense data > > + // written even if this request is processed successfully, so that It will > > + // issue "REQUEST SENSE" later to retrieve sense data. > > + // > > + Packet->SenseDataLength = 0; > > + Packet->HostAdapterStatus = EFI_EXT_SCSI_STATUS_HOST_ADAPTER_OK; > > + Packet->TargetStatus = EFI_EXT_SCSI_STATUS_TARGET_GOOD; > > + > > + return EFI_SUCCESS; > > + > > +Error: > > + DEBUG ((DEBUG_VERBOSE, "%a: dstat: %02X, sist0: %02X, sist1: %02X\n", > > + __FUNCTION__, DStat, SIst0, SIst1)); > > The Error label may be reached without setting some (or even any) of > DStat, SIst0, SIst1. > > (2) Please set all three of DStat, SIst0, and SIst1 to zero, near the > top of the function, with explicit assignments (not initialization). > Right. Will set them to zero in v3. > > + // > > + // Update the request packet to reflect the status. > > + // > > + if (*ScsiStatus != 0xFF) { > > + Packet->TargetStatus = *ScsiStatus; > > + } else { > > + Packet->TargetStatus = EFI_EXT_SCSI_STATUS_TARGET_TASK_ABORTED; > > + } > > + > > + if (SIst0 & LSI_SIST0_PAR) { > > + Packet->HostAdapterStatus = EFI_EXT_SCSI_STATUS_HOST_ADAPTER_PARITY_ERROR; > > + } else if (SIst0 & LSI_SIST0_RST) { > > + Packet->HostAdapterStatus = EFI_EXT_SCSI_STATUS_HOST_ADAPTER_BUS_RESET; > > + } else if (SIst0 & LSI_SIST0_UDC) { > > + // > > + // The target device is disconnected unexpectedly. According to UEFI spec, > > + // this is TIMEOUT_COMMAND. > > + // > > + Packet->HostAdapterStatus = EFI_EXT_SCSI_STATUS_HOST_ADAPTER_TIMEOUT_COMMAND; > > + } else if (SIst0 & LSI_SIST0_SGE) { > > + Packet->HostAdapterStatus = EFI_EXT_SCSI_STATUS_HOST_ADAPTER_DATA_OVERRUN_UNDERRUN; > > + } else if (SIst1 & LSI_SIST1_HTH) { > > + Packet->HostAdapterStatus = EFI_EXT_SCSI_STATUS_HOST_ADAPTER_TIMEOUT; > > + } else if (SIst1 & LSI_SIST1_GEN) { > > + Packet->HostAdapterStatus = EFI_EXT_SCSI_STATUS_HOST_ADAPTER_TIMEOUT; > > + } else if (SIst1 & LSI_SIST1_STO) { > > + Packet->HostAdapterStatus = EFI_EXT_SCSI_STATUS_HOST_ADAPTER_SELECTION_TIMEOUT; > > + } else { > > + Packet->HostAdapterStatus = EFI_EXT_SCSI_STATUS_HOST_ADAPTER_OTHER; > > + } > > According to the UEFI spec, on EFI_DEVICE_ERROR (see below): > > See HostAdapterStatus, TargetStatus, SenseDataLength, and SenseData in > that order for additional status information. > > (3) Therefore, on this error branch, please also set SenseDataLength to > zero. > Will set SenseDataLength to 0 in v3. > > + > > + 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,7 +501,7 @@ LsiScsiPassThru ( > > return Status; > > } > > > > - return EFI_UNSUPPORTED; > > + return LsiScsiProcessRequest (Dev, *Target, Lun, Packet); > > } > > The LsiScsiPassThru() function can now return EFI_SUCCESS, without > updating InTransferLength and OutTransferLength. > > But sticking (for now) with EFI_UNSUPPORTED would also be wrong, because > (according to the UEFI spec) EFI_UNSUPPORTED means "The SCSI Request > Packet was not sent", and here we *did* talk to the device. > > (4) So please squash the next patch -- "[PATCH v2 11/12] > OvmfPkg/LsiScsiDxe: Calculate the transferred bytes" -- into this one, > in the v3 series. > > (The next patch, which handles the transfer lengths, is not large, > thankfully.) > Since the calculation wasn't in v1, I thought it's easier to review the change in an extra patch :) Will squash the patch in v3. Gary Lin > For now I'll comment separately under that patch. > > Thanks! > Laszlo > > > > > EFI_STATUS > > @@ -474,6 +807,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 05deeed379fe..6ecf523f5a9e 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. > > // > > @@ -25,6 +30,18 @@ typedef struct { > > // Count (DBC), a 24-bit register, so the maximum is 0xFFFFFF (16MB-1). > > // > > UINT8 Data[SIZE_64KB]; > > + // > > + // 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 { > > @@ -34,6 +51,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; > > @@ -46,6 +64,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(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 6111449577a8..4c7abcec618f 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 ca13a3cb11d3..f16c00ad5b99 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 > > + > > gUefiOvmfPkgTokenSpaceGuid.PcdOvmfFlashNvStorageEventLogBase|0x0|UINT32|0x8 > > gUefiOvmfPkgTokenSpaceGuid.PcdOvmfFlashNvStorageEventLogSize|0x0|UINT32|0x9 > > gUefiOvmfPkgTokenSpaceGuid.PcdOvmfFirmwareFdSize|0x0|UINT32|0xa > > >