From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from us-smtp-delivery-1.mimecast.com (us-smtp-delivery-1.mimecast.com [205.139.110.61]) by mx.groups.io with SMTP id smtpd.web12.3392.1587030838950662573 for ; Thu, 16 Apr 2020 02:54:00 -0700 Authentication-Results: mx.groups.io; dkim=pass header.i=@redhat.com header.s=mimecast20190719 header.b=FMBKNSop; spf=pass (domain: redhat.com, ip: 205.139.110.61, mailfrom: lersek@redhat.com) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=redhat.com; s=mimecast20190719; t=1587030838; 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: content-transfer-encoding:content-transfer-encoding: in-reply-to:in-reply-to:references:references; bh=4VQalIR15eA0eTyoyhWw1MXwS7+S3jb17tQXjQUwbC8=; b=FMBKNSopLjknNNXDcXyc8PqyuboEROx9jKoLbQQSX4Tpfm5K5OWivE6KKAi5IAUa5D9WcS xOPenO+oBg0xvBbRlLc8x/agsfyBhFe9qIQM7aqf3OiN4yPEtxW9xt46g8vbQRe+0a1zJ2 G4o2WnMSv4lrzZ5/kZ7HQF4g5Fmncic= Received: from mimecast-mx01.redhat.com (mimecast-mx01.redhat.com [209.132.183.4]) (Using TLS) by relay.mimecast.com with ESMTP id us-mta-318-ZdwwnXU2MqWwRyFjDbnr2g-1; Thu, 16 Apr 2020 05:53:51 -0400 X-MC-Unique: ZdwwnXU2MqWwRyFjDbnr2g-1 Received: from smtp.corp.redhat.com (int-mx08.intmail.prod.int.phx2.redhat.com [10.5.11.23]) (using TLSv1.2 with cipher AECDH-AES256-SHA (256/256 bits)) (No client certificate requested) by mimecast-mx01.redhat.com (Postfix) with ESMTPS id 83FED108442D; Thu, 16 Apr 2020 09:53:50 +0000 (UTC) Received: from lacos-laptop-7.usersys.redhat.com (ovpn-114-225.ams2.redhat.com [10.36.114.225]) by smtp.corp.redhat.com (Postfix) with ESMTP id B998D272A3; Thu, 16 Apr 2020 09:53:48 +0000 (UTC) Subject: Re: [edk2-devel] [PATCH v4 10/13] OvmfPkg/MptScsiDxe: Initialize hardware To: devel@edk2.groups.io, nikita.leshchenko@oracle.com Cc: liran.alon@oracle.com, aaron.young@oracle.com, Jordan Justen , Ard Biesheuvel References: <20200414173813.7715-1-nikita.leshchenko@oracle.com> <20200414173813.7715-11-nikita.leshchenko@oracle.com> From: "Laszlo Ersek" Message-ID: <034ffcb9-e11b-4b31-8d62-2eba4eaef08f@redhat.com> Date: Thu, 16 Apr 2020 11:53:47 +0200 User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:52.0) Gecko/20100101 Thunderbird/52.9.1 MIME-Version: 1.0 In-Reply-To: <20200414173813.7715-11-nikita.leshchenko@oracle.com> X-Scanned-By: MIMEDefang 2.84 on 10.5.11.23 X-Mimecast-Spam-Score: 0 X-Mimecast-Originator: redhat.com Content-Language: en-US Content-Type: text/plain; charset=windows-1252 Content-Transfer-Encoding: 7bit On 04/14/20 19:38, 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 | 123 +++++++++++++ > OvmfPkg/MptScsiDxe/MptScsi.c | 173 +++++++++++++++++- > 2 files changed, 295 insertions(+), 1 deletion(-) > > diff --git a/OvmfPkg/Include/IndustryStandard/FusionMptScsi.h b/OvmfPkg/Include/IndustryStandard/FusionMptScsi.h > index df9bdc2f0348..d00a9e6db0bf 100644 > --- a/OvmfPkg/Include/IndustryStandard/FusionMptScsi.h > +++ b/OvmfPkg/Include/IndustryStandard/FusionMptScsi.h > @@ -20,4 +20,127 @@ > #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 (1) I suggest (but don't insist) that we adjust the whitespace after MPT_DOORBELL_RESET so that 0x40 line up with 0x42 below it. Just an idea. > + > +#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 union { > +#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 > +} MPT_IO_CONTROLLER_INIT_REQUEST; > + > +#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; > + > +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; > + > +typedef struct { > + UINT32 Length: 24; > + UINT32 EndOfList: 1; > + UINT32 Is64BitAddress: 1; > + // > + // True when the buffer contains data to be transfered. Otherwise it's the > + // destination buffer > + // > + UINT32 BufferContainsData: 1; > + UINT32 LocalAddress: 1; > + UINT32 ElementType: 2; > + UINT32 EndOfBuffer: 1; > + UINT32 LastElement: 1; > + UINT64 DataBufferAddress; > +} MPT_SG_ENTRY_SIMPLE; > +#pragma pack () > + > +typedef union { > +#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 > +} MPT_SCSI_IO_ERROR_REPLY; > + > #endif // __FUSION_MPT_SCSI_H__ (2) If you really want to sink the pragmas into the unions, so that they only surround the embedded structs, I'm OK with that. If you feel flexible about them, I'd suggest wrapping the entire sequence of typedefs into a single #pragma pack (1) / pack () pair, which is easier to read and feels more idiomatic in edk2. But, again, if you feel it's important to express the packing goals with this specific syntax, I'm OK that way too. > diff --git a/OvmfPkg/MptScsiDxe/MptScsi.c b/OvmfPkg/MptScsiDxe/MptScsi.c > index 4bfd03d2acb0..9c3bdc430e1a 100644 > --- a/OvmfPkg/MptScsiDxe/MptScsi.c > +++ b/OvmfPkg/MptScsiDxe/MptScsi.c > @@ -42,6 +42,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, > + PCI_BAR_IDX0, > + 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, > + PCI_BAR_IDX0, > + 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); > + 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 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); (3) The local variable that you're going to read the reply into has type MPT_IO_CONTROLLER_INIT_REPLY. But the number of bytes you announce for reading back matches MPT_SCSI_IO_ERROR_REPLY. I haven't checked the technical manual that the commit message references, so I don't know which reply format is the one that's needed here -- but whichever is needed, the number of bytes that we read back should actually match the structure that we populate. (General remark: for this reason, I avoid "sizeof (type)" on principle, whenever I can, and use "sizeof object" instead. With the latter, it's more difficult to get "out of sync" if I change the type of the object later on.) Hmmm, wait a second!... Is it possible that "ReplyFrameSize" is for configuring *future* replies, and the MPT_MESSAGE_HDR_FUNCTION_IOC_INIT request code guarantees that the device will produce an MPT_IO_CONTROLLER_INIT_REPLY object in response, regardless of "ReplyFrameSize"? In that case, please ignore my comment (3). (4) We don't align MPT_IO_CONTROLLER_INIT_REPLY to an 8-byte boundary, unlike MPT_IO_CONTROLLER_INIT_REQUEST. Can you please confirm that it's because we read MPT_IO_CONTROLLER_INIT_REPLY gradually (dword-wise), via "ReplyWord" -- which is automatically naturally aligned? (I.e., I'm not suggesting a code change, just asking for a confirmation.) > + > + // > + // Send controller init through doorbell > + // > + Status = MptDoorbell ( > + Dev, > + MPT_DOORBELL_HANDSHAKE, > + sizeof (Req) / sizeof (UINT32) (5) Please use a STATIC_ASSERT here, to express that this division produces a 0 remainder. Please see the similar STATIC_ASSERT in the PvScsi driver. (6) Furthermore, I'd suggest another STATIC_ASSERT to express that the quotient fits in a UINT8, and then maybe explicitly casting the quotient to UINT8. I'm asking for the explicit cast only because I'm concerned that the VS compiler will whine about "possible loss of precision". (I realize the quotient is a constant that's known at compile time -- but I'm afraid VS might still whine about the implicit UINTN->UINT8 conversion, for the "DoorbellArg" parameter.) (7) Note that the union pattern you use for alignment is *slightly* different from the one used in PvScsi. And this difference could hide a trap -- an obscure trap, admittedly, but I'd like to be clear about it. In PvScsi we have a local variable called "AlignedCmd", which is of the union type. The "AlignedCmd.Cmd" union member, which is itself a packed structure, is the actual datum that we serialize and send to the device. Here however, we seem to send the full union to the device. Now, per ISO C99 "6.7.2.1 Structure and union specifiers", paragraph 15, "[t]here may be unnamed padding at the end of a structure or union". And see my comment (2) above -- you specifically do *not* pack the unions, only the internals of their "Data" members. (The gcc documentation confirms that #pragma pack affects unions too, so packing a union *can* make a difference.) Thus, when you send the whole MPT_IO_CONTROLLER_INIT_REQUEST union to the device, and not just its "Data" member (which is itself packed), you could be writing from unnamed padding at the end of the union. Therefore I would: (7a) either send "Req.Data" (rather than "Req") to the device in this function, (7b) or else address my remark (2), and pack the unions too. (8) I believe same observation as (7) holds for the Req.Data.ReplyFrameSize = sizeof (MPT_SCSI_IO_ERROR_REPLY); assignment. Even if my comment (3) falls away, and "ReplyFrameSize" configures the size of *future* responses, those responses should likely not include any (potential) unnamed padding at the end of the MPT_SCSI_IO_ERROR_REPLY union. > + ); > + if (EFI_ERROR (Status)) { > + return Status; > + } > + Status = Dev->PciIo->Io.Write ( > + Dev->PciIo, > + EfiPciIoWidthFifoUint32, > + 0, (9) Please use PCI_BAR_IDX0 here too. > + MPT_REG_DOORBELL, > + sizeof (Req) / sizeof (UINT32), > + &Req > + ); > + if (EFI_ERROR (Status)) { > + return Status; > + } > + > + // > + // Read reply through doorbell > + // Each 32bit (Dword) read produces 16bit (Word) of data > + // (10) Please use another STATIC_ASSERT here for expressing that the response structure size is an integer multiple of sizeof (UINT16). (11) Please repeat the statement from the commit message here -- as a code comment -- that the reply is only read to complete the doorbell function of the device, and that we intentionally ignore the contents. (BTW, if we do not parse the response even at the end of this series, then saving the response into the "Reply" variable looks useless -- I guess we could remove the "Reply" variable altogether, in that case. But I'll have to see the rest of the patches.) > + ReplyBytes = (UINT8 *)&Reply; > + while (ReplyBytes != (UINT8 *)(&Reply + 1)) { > + Status = In32 (Dev, MPT_REG_DOORBELL, &ReplyWord); > + if (EFI_ERROR (Status)) { > + return Status; > + } > + CopyMem (ReplyBytes, &ReplyWord, 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; > +} (12) So in general I would have suggested that, in case the initialization fails and we take an early "return" branch in this function, we should first jump to an error handling label, and reset the device. (Because, when we return an error code from the function, we shouldn't leave the device in half-initialized state.) *But*, it seems like the reset operation *itself* occurs through the doorbell register and the MPT_REG_ISTATUS register -- and if any step in MptScsiInit() fails, then it's exactly those registers that are (apparently) in unknown / unreliable state. In other words, if MptScsiInit() fails, then we *can't* (expect to) reset the device (successfully). Can you please confirm my understanding? (Again, just a question, not a code change request.) > + > // > // Ext SCSI Pass Thru > // > @@ -333,6 +494,11 @@ MptScsiControllerStart ( > goto CloseProtocol; > } > > + Status = MptScsiInit (Dev); > + if (EFI_ERROR (Status)) { > + goto RestorePciAttributes; > + } > + > // > // Host adapter channel, doesn't exist > // > @@ -357,11 +523,14 @@ MptScsiControllerStart ( > &Dev->PassThru > ); > if (EFI_ERROR (Status)) { > - goto RestoreAttributes; > + goto UninitDev; > } > > return EFI_SUCCESS; > > +UninitDev: > + MptScsiReset (Dev); > + > RestoreAttributes: > Dev->PciIo->Attributes ( > Dev->PciIo, > @@ -421,6 +590,8 @@ MptScsiControllerStop ( > return Status; > } > > + MptScsiReset (Dev); > + > Dev->PciIo->Attributes ( > Dev->PciIo, > EfiPciIoAttributeOperationEnable, > These parts look fine to me. Before proceeding to patch#11 in this series, I'll wait for your answer; I think I need to read & understand it in order to continue the review. Thanks! Laszlo