Re: [edk2-devel] [PATCH v5 10/12] OvmfPkg/MptScsiDxe: Initialize hardware

["Laszlo Ersek" <lersek@redhat.com>]

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.leshchenko@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

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,
>