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.web10.2675.1588172153427986172 for ; Wed, 29 Apr 2020 07:55:54 -0700 Authentication-Results: mx.groups.io; dkim=pass header.i=@redhat.com header.s=mimecast20190719 header.b=iqi58rky; 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=1588172152; 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=lub9RS7HwgWYnliReKny0U5cWpdIXF1kf1+CUAd5jgA=; b=iqi58rkyjm6U14hmVtzpAvaj4GmnBtmPoEFf98ogVNJv2A7bYAVe+uEb9/BDtHet0B1muq dMgT75ZdYMdiq6c7rfpvWThCk0RbexKfEaHQQwKKphw/uJ/37tH8ydAe9DoIh3EXULENfo nNydB7BCxXZ+F7QJri1apxr8a2vRue4= 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-238-jACl03koOiWcaAP_rfYWcw-1; Wed, 29 Apr 2020 10:55:42 -0400 X-MC-Unique: jACl03koOiWcaAP_rfYWcw-1 Received: from smtp.corp.redhat.com (int-mx04.intmail.prod.int.phx2.redhat.com [10.5.11.14]) (using TLSv1.2 with cipher AECDH-AES256-SHA (256/256 bits)) (No client certificate requested) by mimecast-mx01.redhat.com (Postfix) with ESMTPS id 2B3991895912; Wed, 29 Apr 2020 14:55:41 +0000 (UTC) Received: from lacos-laptop-7.usersys.redhat.com (ovpn-114-1.ams2.redhat.com [10.36.114.1]) by smtp.corp.redhat.com (Postfix) with ESMTP id 16E885D9C9; Wed, 29 Apr 2020 14:55:38 +0000 (UTC) Subject: Re: [edk2-devel] [PATCH v5 10/12] 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: <20200424175927.41210-1-nikita.leshchenko@oracle.com> <20200424175927.41210-11-nikita.leshchenko@oracle.com> From: "Laszlo Ersek" Message-ID: <803cf42d-2c43-5c49-55ff-31b3acdba776@redhat.com> Date: Wed, 29 Apr 2020 16:55:38 +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: <20200424175927.41210-11-nikita.leshchenko@oracle.com> X-Scanned-By: MIMEDefang 2.79 on 10.5.11.14 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/24/20 19:59, 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 | 128 ++++++++++++ > OvmfPkg/MptScsiDxe/MptScsi.c | 187 +++++++++++++++++- > 2 files changed, 314 insertions(+), 1 deletion(-) > > diff --git a/OvmfPkg/Include/IndustryStandard/FusionMptScsi.h b/OvmfPkg/Include/IndustryStandard/FusionMptScsi.h > index df9bdc2f0348..655d629d902e 100644 > --- a/OvmfPkg/Include/IndustryStandard/FusionMptScsi.h > +++ b/OvmfPkg/Include/IndustryStandard/FusionMptScsi.h > @@ -20,4 +20,132 @@ > #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 > +// > + > +#pragma pack (1) > +typedef 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; > +} MPT_IO_CONTROLLER_INIT_REQUEST; > + > +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; > + > +typedef 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; > +} MPT_SCSI_IO_REPLY; > + > +typedef struct { > + MPT_SCSI_IO_REQUEST Header; > + MPT_SG_ENTRY_SIMPLE Sg; > +} MPT_SCSI_REQUEST_WITH_SG; > +#pragma pack () > + > +typedef union { > + MPT_SCSI_IO_REPLY Data; > + UINT64 Uint64; // 8 byte alignment required by HW > +} MPT_SCSI_IO_REPLY_ALIGNED; > + > +typedef union { > + MPT_SCSI_REQUEST_WITH_SG Data; > + UINT64 Uint64; // 8 byte alignment required by HW > +} MPT_SCSI_REQUEST_ALIGNED; > + > #endif // __FUSION_MPT_SCSI_H__ > diff --git a/OvmfPkg/MptScsiDxe/MptScsi.c b/OvmfPkg/MptScsiDxe/MptScsi.c > index e88ac2867a75..15d671b544c2 100644 > --- a/OvmfPkg/MptScsiDxe/MptScsi.c > +++ b/OvmfPkg/MptScsiDxe/MptScsi.c > @@ -43,6 +43,181 @@ 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; (1) Unfortunately, "Req" is not aligned to any particular boundary any more. From your response at http://mid.mail-archive.com/DFF6CD19-C013-44B1-8B0E-DE1C1A891BBF@oracle.com https://edk2.groups.io/g/devel/message/57471 under my then-remark (4), you seemed to agree that the alignment was still necessary, only the size should be lowered from 8 bytes to 4 bytes. Below we use Dev->PciIo->Io.Write() for sending the request, with "EfiPciIoWidthFifoUint32". And the code flow that's internal to that call is similar to what I described in the following message, in the PvScsiDxe review (see the end of the message): http://mid.mail-archive.com/5833e06c-bb10-5c8c-1827-25e723b5834e@redhat.com https://edk2.groups.io/g/devel/message/56326 So you get PciIoIoWrite() -> RootBridgeIoIoWrite() -> CpuIoServiceWrite() -> CpuIoCheckParameter() and the last function in that chain will reject an un-aligned request. That means we still need a union here, just with a Uint32 field as the "other" member. And we will still need to send "Req.Data" (not "Req") to the device. You can either introduce a new typedef for the alignment / union under IndustryStandard, or just define an ad-hoc union here in this function, like PvScsiDxe does. > + 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.WhoInit = MPT_IOC_WHOINIT_ROM_BIOS; > + Req.Function = MPT_MESSAGE_HDR_FUNCTION_IOC_INIT; > + STATIC_ASSERT( (2) Missing space character. > + FixedPcdGet8 (PcdMptScsiMaxTargetLimit) < 255, > + "Req supports 255 targets only (max target is 254)"); (3) The closing paren should be broken off to a separate line. > + Req.MaxDevices = Dev->MaxTarget + 1; > + Req.MaxBuses = 1; The assignment to "ReplyFrameSize" is lost in v5 -- did you remove it intentionally? ...Hmm, no, it's been moved to the next patch. OK. > + > + // > + // Send controller init through doorbell > + // > + STATIC_ASSERT ( > + sizeof (Req) % sizeof (UINT32) == 0, > + "Req must be multiple of UINT32" > + ); > + STATIC_ASSERT ( > + sizeof (Req) / sizeof (UINT32) <= MAX_UINT8, > + "Req must bit in MAX_UINT8 Dwords" > + ); (4) s/bit/fit/ > + Status = MptDoorbell ( > + Dev, > + MPT_DOORBELL_HANDSHAKE, > + (UINT8)(sizeof (Req) / sizeof (UINT32)) > + ); > + if (EFI_ERROR (Status)) { > + return Status; > + } > + Status = Dev->PciIo->Io.Write ( > + Dev->PciIo, > + EfiPciIoWidthFifoUint32, > + PCI_BAR_IDX0, > + 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 > + // (5) You missed my v4 request (10), at: http://mid.mail-archive.com/034ffcb9-e11b-4b31-8d62-2eba4eaef08f@redhat.com https://edk2.groups.io/g/devel/message/57464 Namely: "Please use another STATIC_ASSERT here for expressing that the response structure size is an integer multiple of sizeof (UINT16)." > + // The reply is read back to complete the doorbell function but it > + // isn't useful because it doesn't contain relevant data or status > + // codes. > + // > + 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; > +} > + > // > // Ext SCSI Pass Thru > // > @@ -382,6 +557,11 @@ MptScsiControllerStart ( > )); > } > > + Status = MptScsiInit (Dev); > + if (EFI_ERROR (Status)) { > + goto RestoreAttributes; Hmmm, git-range-diff flags this as a v4->v5 change, and I don't understand why... Ah, OK. In v4, we jumped to "RestorePciAttributes" -- which was a non-existent label. So I think the v4 variant of this patch didn't compile. I hope that's fixed now, with the above. :) The rest of the updates / patch look fine to me. Thanks! Laszlo > + } > + > // > // Host adapter channel, doesn't exist > // > @@ -406,11 +586,14 @@ MptScsiControllerStart ( > &Dev->PassThru > ); > if (EFI_ERROR (Status)) { > - goto RestoreAttributes; > + goto UninitDev; > } > > return EFI_SUCCESS; > > +UninitDev: > + MptScsiReset (Dev); > + > RestoreAttributes: > Dev->PciIo->Attributes ( > Dev->PciIo, > @@ -470,6 +653,8 @@ MptScsiControllerStop ( > return Status; > } > > + MptScsiReset (Dev); > + > Dev->PciIo->Attributes ( > Dev->PciIo, > EfiPciIoAttributeOperationSet, >