From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from aserp2120.oracle.com (aserp2120.oracle.com [141.146.126.78]) by mx.groups.io with SMTP id smtpd.web12.2581.1583368204555068188 for ; Wed, 04 Mar 2020 16:30:04 -0800 Authentication-Results: mx.groups.io; dkim=pass header.i=@oracle.com header.s=corp-2020-01-29 header.b=NtgyENXk; spf=pass (domain: oracle.com, ip: 141.146.126.78, mailfrom: liran.alon@oracle.com) Received: from pps.filterd (aserp2120.oracle.com [127.0.0.1]) by aserp2120.oracle.com (8.16.0.42/8.16.0.42) with SMTP id 0250OC2e064386; Thu, 5 Mar 2020 00:30:03 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=apSTFcyCi0dmW1K61NuP7684p1tL61UGR7FmLwKnjzg=; b=NtgyENXkPBy3y8yFTxoxPrGJmHbOVZzyPPBq8YugkXnmuYAjhr8T8ET8ooLXJKiMzugO SadIEPIk0XFeC9+FQFQNaxXqgOGu3fEBiqQjCpEZHTi2pdZN+JSuTV7ParJNzlE3zGFf jqhM8geR2/SIzGnEr1XXT3oiw4qDLizq1ei58BYbTmZAJCPmgd4pYkZRCVsaHFdIaYzO K7xgpEvxz49tKfVCADmKXRbOTi84C2VVd4ZXw/67c3bLNyFHueDRVq5NbxXD2HNkBJm6 PIBah7orXOQiElg+FvHYJFiGW9p3Nth7Iad2zIMqxFQkqi0W1yp/7aTGVJDbujS5aAoN 3g== Received: from userp3030.oracle.com (userp3030.oracle.com [156.151.31.80]) by aserp2120.oracle.com with ESMTP id 2yffwr1t88-1 (version=TLSv1.2 cipher=ECDHE-RSA-AES256-GCM-SHA384 bits=256 verify=OK); Thu, 05 Mar 2020 00:30:03 +0000 Received: from pps.filterd (userp3030.oracle.com [127.0.0.1]) by userp3030.oracle.com (8.16.0.42/8.16.0.42) with SMTP id 0250IiKY005033; Thu, 5 Mar 2020 00:30:02 GMT Received: from userv0122.oracle.com (userv0122.oracle.com [156.151.31.75]) by userp3030.oracle.com with ESMTP id 2yg1er6fyf-1 (version=TLSv1.2 cipher=ECDHE-RSA-AES256-GCM-SHA384 bits=256 verify=OK); Thu, 05 Mar 2020 00:30:02 +0000 Received: from abhmp0002.oracle.com (abhmp0002.oracle.com [141.146.116.8]) by userv0122.oracle.com (8.14.4/8.14.4) with ESMTP id 0250U0Wq028447; Thu, 5 Mar 2020 00:30:00 GMT Received: from [192.168.14.112] (/79.177.218.220) by default (Oracle Beehive Gateway v4.0) with ESMTP ; Wed, 04 Mar 2020 16:30:00 -0800 Subject: Re: [PATCH v3 11/13] OvmfPkg/MptScsiDxe: Initialize hardware To: Nikita Leshenko , devel@edk2.groups.io Cc: aaron.young@oracle.com, jordan.l.justen@intel.com, lersek@redhat.com, ard.biesheuvel@linaro.org References: <20200304192257.96736-1-nikita.leshchenko@oracle.com> <20200304192257.96736-12-nikita.leshchenko@oracle.com> From: Liran Alon Message-ID: Date: Thu, 5 Mar 2020 02:29:43 +0200 User-Agent: Mozilla/5.0 (Macintosh; Intel Mac OS X 10.13; rv:68.0) Gecko/20100101 Thunderbird/68.5.0 MIME-Version: 1.0 In-Reply-To: <20200304192257.96736-12-nikita.leshchenko@oracle.com> X-Proofpoint-Virus-Version: vendor=nai engine=6000 definitions=9550 signatures=668685 X-Proofpoint-Spam-Details: rule=notspam policy=default score=0 adultscore=0 suspectscore=0 spamscore=0 mlxlogscore=999 malwarescore=0 bulkscore=0 mlxscore=0 phishscore=0 classifier=spam adjust=0 reason=mlx scancount=1 engine=8.12.0-2001150001 definitions=main-2003050000 X-Proofpoint-Virus-Version: vendor=nai engine=6000 definitions=9550 signatures=668685 X-Proofpoint-Spam-Details: rule=notspam policy=default score=0 lowpriorityscore=0 spamscore=0 impostorscore=0 malwarescore=0 mlxlogscore=999 mlxscore=0 suspectscore=0 phishscore=0 clxscore=1015 bulkscore=0 adultscore=0 priorityscore=1501 classifier=spam adjust=0 reason=mlx scancount=1 engine=8.12.0-2001150001 definitions=main-2003050000 Content-Type: text/plain; charset=utf-8; format=flowed Content-Transfer-Encoding: 7bit Content-Language: en-US On 04/03/2020 21:22, Nikita Leshenko wrote: > Reset and send the IO controller initialization request. The reply is > read back to complete the doorbell function but it isn't useful to us > because it doesn't contain relevant data or status codes. > > See "LSI53C1030 PCI-X to Dual Channel Ultra320 SCSI Multifunction > Controller" technical manual for more information. > > Ref: https://bugzilla.tianocore.org/show_bug.cgi?id=2390 > Signed-off-by: Nikita Leshenko > --- > .../Include/IndustryStandard/FusionMptScsi.h | 115 ++++++++++++ > OvmfPkg/MptScsiDxe/MptScsi.c | 168 ++++++++++++++++++ > 2 files changed, 283 insertions(+) > > diff --git a/OvmfPkg/Include/IndustryStandard/FusionMptScsi.h b/OvmfPkg/Include/IndustryStandard/FusionMptScsi.h > index 1bd65ed40b1c..1ce2432bd3c2 100644 > --- a/OvmfPkg/Include/IndustryStandard/FusionMptScsi.h > +++ b/OvmfPkg/Include/IndustryStandard/FusionMptScsi.h > @@ -26,4 +26,119 @@ > #define LSI_SAS1068_PCI_DEVICE_ID 0x0054 > #define LSI_SAS1068E_PCI_DEVICE_ID 0x0058 > > +#define MPT_REG_DOORBELL 0x00 > +#define MPT_REG_WRITE_SEQ 0x04 > +#define MPT_REG_HOST_DIAG 0x08 > +#define MPT_REG_TEST 0x0c > +#define MPT_REG_DIAG_DATA 0x10 > +#define MPT_REG_DIAG_ADDR 0x14 > +#define MPT_REG_ISTATUS 0x30 > +#define MPT_REG_IMASK 0x34 > +#define MPT_REG_REQ_Q 0x40 > +#define MPT_REG_REP_Q 0x44 > + > +#define MPT_DOORBELL_RESET 0x40 > +#define MPT_DOORBELL_HANDSHAKE 0x42 > + > +#define MPT_IMASK_DOORBELL 0x01 > +#define MPT_IMASK_REPLY 0x08 > + > +#define MPT_MESSAGE_HDR_FUNCTION_SCSI_IO_REQUEST 0x00 > +#define MPT_MESSAGE_HDR_FUNCTION_IOC_INIT 0x02 > + > +#define MPT_SG_ENTRY_TYPE_SIMPLE 0x01 > + > +#define MPT_IOC_WHOINIT_ROM_BIOS 0x02 > + > +// > +// Device structures > +// > + > +typedef struct { > +#pragma pack (1) > + struct { > + UINT8 WhoInit; > + UINT8 Reserved1; > + UINT8 ChainOffset; > + UINT8 Function; > + UINT8 Flags; > + UINT8 MaxDevices; > + UINT8 MaxBuses; > + UINT8 MessageFlags; > + UINT32 MessageContext; > + UINT16 ReplyFrameSize; > + UINT16 Reserved2; > + UINT32 HostMfaHighAddr; > + UINT32 SenseBufferHighAddr; > + } Data; > +#pragma pack () > + UINT64 Uint64; // 8 byte alignment required by HW Can you further explain this part? Not sure I understood why this cause MPT_IO_CONTROLLER_INIT_REQUEST to be 8-byte aligned when used. In addition, isn't it a bit misleading that it's defined as part of the structure? E.g. sizeof(MPT_IO_CONTROLLER_INIT_REQUEST) doesn't return the size of the real INIT_REQUEST. Examining SeaBIOS mpt-scsi.c driver, the MptIOCInitRequest global variable doesn't seem to be defined as required to be 8-byte aligned. Is that a bug in SeaBIOS? > +} MPT_IO_CONTROLLER_INIT_REQUEST; Missing new-line here. > +#pragma pack (1) > +typedef struct { > + UINT8 WhoInit; > + UINT8 Reserved1; > + UINT8 MessageLength; > + UINT8 Function; > + UINT8 Flags; > + UINT8 MaxDevices; > + UINT8 MaxBuses; > + UINT8 MessageFlags; > + UINT32 MessageContext; > + UINT16 Reserved2; > + UINT16 IOCStatus; > + UINT32 IOCLogInfo; > +} MPT_IO_CONTROLLER_INIT_REPLY; Missing new-line. > +typedef struct { > + UINT8 TargetID; > + UINT8 Bus; > + UINT8 ChainOffset; > + UINT8 Function; > + UINT8 CDBLength; > + UINT8 SenseBufferLength; > + UINT8 Reserved; > + UINT8 MessageFlags; > + UINT32 MessageContext; > + UINT8 LUN[8]; > + UINT32 Control; > + UINT8 CDB[16]; > + UINT32 DataLength; > + UINT32 SenseBufferLowAddress; > +} MPT_SCSI_IO_REQUEST; Missing new-line. > +typedef struct { > + UINT32 Length: 24; > + UINT32 EndOfList: 1; > + UINT32 Is64BitAddress: 1; > + UINT32 BufferContainsData: 1; > + UINT32 LocalAddress: 1; > + UINT32 ElementType: 2; > + UINT32 EndOfBuffer: 1; > + UINT32 LastElement: 1; > + UINT64 DataBufferAddress; > +} MPT_SG_ENTRY_SIMPLE; > +#pragma pack () Missing new-line. > +typedef struct { > +#pragma pack (1) > + struct { > + UINT8 TargetID; > + UINT8 Bus; > + UINT8 MessageLength; > + UINT8 Function; > + UINT8 CDBLength; > + UINT8 SenseBufferLength; > + UINT8 Reserved; > + UINT8 MessageFlags; > + UINT32 MessageContext; > + UINT8 SCSIStatus; > + UINT8 SCSIState; > + UINT16 IOCStatus; > + UINT32 IOCLogInfo; > + UINT32 TransferCount; > + UINT32 SenseCount; > + UINT32 ResponseInfo; > + } Data; > +#pragma pack () > + UINT64 Uint64; // 8 byte alignment required by HW Same comment regarding this as mentioned above. > +} MPT_SCSI_IO_ERROR_REPLY; > + > #endif // __FUSION_MPT_SCSI_H__ > diff --git a/OvmfPkg/MptScsiDxe/MptScsi.c b/OvmfPkg/MptScsiDxe/MptScsi.c > index 4a52dee902c7..37f1ea4b3506 100644 > --- a/OvmfPkg/MptScsiDxe/MptScsi.c > +++ b/OvmfPkg/MptScsiDxe/MptScsi.c > @@ -47,6 +47,167 @@ typedef struct { > #define MPT_SCSI_FROM_PASS_THRU(PassThruPtr) \ > CR (PassThruPtr, MPT_SCSI_DEV, PassThru, MPT_SCSI_DEV_SIGNATURE) > > +// > +// Hardware functions > +// > + > +STATIC > +EFI_STATUS > +Out32 ( > + IN MPT_SCSI_DEV *Dev, > + IN UINT32 Addr, > + IN UINT32 Data > + ) > +{ > + return Dev->PciIo->Io.Write ( > + Dev->PciIo, > + EfiPciIoWidthUint32, > + 0, // BAR0 > + Addr, > + 1, > + &Data > + ); > +} > + > +STATIC > +EFI_STATUS > +In32 ( > + IN MPT_SCSI_DEV *Dev, > + IN UINT32 Addr, > + OUT UINT32 *Data > + ) > +{ > + return Dev->PciIo->Io.Read ( > + Dev->PciIo, > + EfiPciIoWidthUint32, > + 0, // BAR0 > + Addr, > + 1, > + Data > + ); > +} > + > +STATIC > +EFI_STATUS > +MptDoorbell ( > + IN MPT_SCSI_DEV *Dev, > + IN UINT8 DoorbellFunc, > + IN UINT8 DoorbellArg > + ) > +{ > + return Out32 ( > + Dev, > + MPT_REG_DOORBELL, > + (((UINT32)DoorbellFunc) << 24) | (DoorbellArg << 16) > + ); > +} > + > +STATIC > +EFI_STATUS > +MptScsiReset ( > + IN MPT_SCSI_DEV *Dev > + ) > +{ > + EFI_STATUS Status; > + > + // > + // Reset hardware > + // > + Status = MptDoorbell (Dev, MPT_DOORBELL_RESET, 0); > + if (EFI_ERROR (Status)) { > + return Status; > + } > + // > + // Mask interrupts > + // > + Status = Out32 (Dev, MPT_REG_IMASK, MPT_IMASK_DOORBELL|MPT_IMASK_REPLY); Nit: Add spaces between "|" sides. > + if (EFI_ERROR (Status)) { > + return Status; > + } > + // > + // Clear interrupt status > + // > + Status = Out32 (Dev, MPT_REG_ISTATUS, 0); > + if (EFI_ERROR (Status)) { > + return Status; > + } > + > + return EFI_SUCCESS; > +} > + > +STATIC > +EFI_STATUS > +MptScsiInit ( > + IN MPT_SCSI_DEV *Dev > + ) > +{ > + EFI_STATUS Status; > + MPT_IO_CONTROLLER_INIT_REQUEST Req; > + MPT_IO_CONTROLLER_INIT_REPLY Reply; > + UINT8 *ReplyBytes; > + UINT32 Reply32; Nit: I suggest renaming to "ReplyWord". > + > + Status = MptScsiReset (Dev); > + if (EFI_ERROR (Status)) { > + return Status; > + } > + > + ZeroMem (&Req, sizeof (Req)); > + ZeroMem (&Reply, sizeof (Reply)); > + Req.Data.WhoInit = MPT_IOC_WHOINIT_ROM_BIOS; > + Req.Data.Function = MPT_MESSAGE_HDR_FUNCTION_IOC_INIT; > + Req.Data.MaxDevices = 1; > + Req.Data.MaxBuses = 1; > + Req.Data.ReplyFrameSize = sizeof (MPT_SCSI_IO_ERROR_REPLY); > + > + // > + // Send controller init through doorbell > + // > + Status = MptDoorbell ( > + Dev, > + MPT_DOORBELL_HANDSHAKE, > + sizeof (Req) / sizeof (UINT32) This is exactly the case I was worried about when you added a dummy UINT64 field to MPT_IO_CONTROLLER_INIT_REQUEST for alignment purposes. As now sizeof (Req) is not really what should be sizeof (MPT_IO_CONTROLLER_INIT_REQUEST). I understand this probably work because device probably parse the request only once you write to BAR0 IOPort the amount of bytes you specified here. But it does seem a little bizarre. i.e. Device parse request from less bytes than actually sent to it. In addition, where does the alignment request comes from? As it seems you write this request as a stream of UINT32 to IOPort (Probably eventually by using "rep outsl"). > + ); > + if (EFI_ERROR (Status)) { > + return Status; > + } > + Status = Dev->PciIo->Io.Write ( > + Dev->PciIo, > + EfiPciIoWidthFifoUint32, > + 0, > + MPT_REG_DOORBELL, > + sizeof (Req) / sizeof (UINT32), > + &Req > + ); > + if (EFI_ERROR (Status)) { > + return Status; > + } Nit: I think it will be nice wrapping the sending here of INIT_REQUEST on it's own function that both does MptDoorbell() and writes Req with PciIo->Io.Write(). > + > + // > + // Read reply through doorbell > + // Each 32bit read produces 16bit of data > + // > + ReplyBytes = (UINT8 *)&Reply; > + while (ReplyBytes != (UINT8 *)(&Reply + 1)) { > + Status = In32 (Dev, MPT_REG_DOORBELL, &Reply32); > + if (EFI_ERROR (Status)) { > + return Status; > + } > + CopyMem (ReplyBytes, &Reply32, sizeof (UINT16)); > + ReplyBytes += sizeof (UINT16); > + } > + > + // > + // Clear interrupts generated by doorbell reply > + // > + Status = Out32 (Dev, MPT_REG_ISTATUS, 0); > + if (EFI_ERROR (Status)) { > + return Status; > + } > + > + return EFI_SUCCESS; > +} > + > // > // Ext SCSI Pass Thru > // > @@ -336,6 +497,11 @@ MptScsiControllerStart ( > goto CloseProtocol; > } > > + Status = MptScsiInit (Dev); > + if (EFI_ERROR (Status)) { > + goto CloseProtocol; > + } > + > // > // Host adapter channel, doesn't exist > // > @@ -422,6 +588,8 @@ MptScsiControllerStop ( > ); > ASSERT_EFI_ERROR (Status); > > + MptScsiReset (Dev); > + > Dev->PciIo->Attributes ( > Dev->PciIo, > EfiPciIoAttributeOperationEnable,