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

["Nikita Leshenko" <nikita.leshchenko@oracle.com>]

> On 16 Apr 2020, at 12:53, Laszlo Ersek <lersek@redhat.com> wrote:
> 
> 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://urldefense.com/v3/__https://bugzilla.tianocore.org/show_bug.cgi?id=2390__;!!GqivPVa7Brio!Ph-KBaVDeTP4F0oR9YHybLz2_YpCTUBsXM5rymNRMgRKVeUaRsWXxogKvM9T1fsEfQg7pQ$ 
>> 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.
Good idea, will do in v5
> 
>> +
>> +#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.
I don't have a strong opinion about it, I prefer to do what feels more idiomatic
in EDK2 (although I don't always know what is idiomatic in EDK2 so feel free to
point out such things :) ).

My intent is to have the structs packed but still aligned to 8 bytes (for some
reason the hardware aligns the descriptor address to 8). I couldn't find any code
in EDK2 that does so I came up with my own pattern.

However, if I understand your suggestion correctly, if I wrap the
entire typedef in #pragma pack (1) / pack (), I would lose the alignment.
That's why I wrapped only the inner structs. Are are you suggesting
that I break out the inner structs into separate typedefs, wrap all of them in
#pragma pack (1) / pack () and then create the wrapping aligning unions?

> 
>> 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.)
Good idea, I will change it to `sizeof Dev->Dma->IoReply.Data`
> 
> 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).
Exactly, it's for future replies
> 
> 
> (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.)
Yes. Now that you mention it, I see that it's enough to align
MPT_IO_CONTROLLER_INIT_REQUEST to 4 bytes since it's read in 32-bit
chunks.
I will add a comment to the code.
> 
> 
>> +
>> +  //
>> +  // 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.
I wasn't aware of this padding, so I will update the code to use suggestion (7a).
We can't do (7b) since packing the union would discard the alignment.
> 
> 
>> +             );
>> +  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.)
We must read the reply fully to initialise the device (the device would reset the
interrupt status only after the host has fetched the entire reply). We could discard
the reply, but I thought it would be nicer to keep it on the stack temporary in
case someone would like to use it in later changes or in debugging (since the
reply is very small and should be hot in the cache, I don't this has any
performance impact).
> 
>> +  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.)
Your understanding it correct. In VirtualBox it looks like you can't write a
reset command to the doorbell while a handshake is in progress. In any case, wonder
what could realistically cause the initialization to fail. On EDK2 side, we're just
talking about a bunch of "in/out" opcodes (the API for which could fail, but I would
assume that if it fails then further "in/out"s will fail as well). On the hypervisor
side, the QEMU and VirtualBox don't even have a failure path for the init call.
So failure seems very theoretical here and I couldn't find anything realistically
helpful to do if the initialization fails in the middle.
> 
> 
>> +
>> //
>> // 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.
ACK on the rest of the points that I didn't leave a comment under, I will change
in v5.
Thanks for the quick review!
Nikita
> 
> Thanks!
> Laszlo
>