Re: [edk2-devel] [PATCH v4 10/13] OvmfPkg/MptScsiDxe: Initialize hardware

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

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