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 <nikita.leshche...@oracle.com> > --- > .../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 DFF6CD19-C013-44B1-8B0E-DE1C1A891BBF@oracle.com">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): 5833e06c-bb10-5c8c-1827-25e723b5834e@redhat.com">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: 034ffcb9-e11b-4b31-8d62-2eba4eaef08f@redhat.com">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, > -=-=-=-=-=-=-=-=-=-=-=- Groups.io Links: You receive all messages sent to this group. View/Reply Online (#58311): https://edk2.groups.io/g/devel/message/58311 Mute This Topic: https://groups.io/mt/73247276/21656 Group Owner: devel+ow...@edk2.groups.io Unsubscribe: https://edk2.groups.io/g/devel/unsub [arch...@mail-archive.com] -=-=-=-=-=-=-=-=-=-=-=-