Re: [PATCH v3 11/13] OvmfPkg/MptScsiDxe: Initialize hardware

[Liran Alon <liran.alon@oracle.com>]

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 <nikita.leshchenko@oracle.com>
> ---
>   .../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,