Re: [edk2-devel] [PATCH 10/11] OvmfPkg/LsiScsiDxe: Process the SCSI Request Packet

["Gary Lin" <glin@suse.com>]

On Wed, Jul 08, 2020 at 02:02:27PM +0800, Gary Lin via groups.io wrote:
> On Tue, Jul 07, 2020 at 02:46:14PM +0200, Laszlo Ersek wrote:
> > On 07/01/20 06:04, Gary Lin wrote:
> > > This is the second part of LsiScsiPassThru(). LsiScsiProcessRequest() is
> > > added to translate the SCSI Request Packet into the LSI 53C895A
> > > commands. This function utilizes the so-called Script buffer to transmit
> > > a series of commands to the chip and then polls the DMA Status (DSTAT)
> > > register until the Scripts Interrupt Instruction Received (SIR) bit
> > > sets. Once the script is done, the SCSI Request Packet will be modified
> > > to reflect the result of the script.
> > >
> > > Cc: Jordan Justen <jordan.l.justen@intel.com>
> > > Cc: Laszlo Ersek <lersek@redhat.com>
> > > Cc: Ard Biesheuvel <ard.biesheuvel@arm.com>
> > > Signed-off-by: Gary Lin <glin@suse.com>
> > > ---
> > >  OvmfPkg/Include/IndustryStandard/LsiScsi.h |  39 +++
> > >  OvmfPkg/LsiScsiDxe/LsiScsi.c               | 308 +++++++++++++++++++++
> > >  OvmfPkg/LsiScsiDxe/LsiScsi.h               |  21 ++
> > >  OvmfPkg/LsiScsiDxe/LsiScsiDxe.inf          |   3 +
> > >  OvmfPkg/OvmfPkg.dec                        |   3 +
> > >  5 files changed, 374 insertions(+)
> > >
> > > diff --git a/OvmfPkg/Include/IndustryStandard/LsiScsi.h b/OvmfPkg/Include/IndustryStandard/LsiScsi.h
> > > index 60e527f1c6a7..cbf049c18310 100644
> > > --- a/OvmfPkg/Include/IndustryStandard/LsiScsi.h
> > > +++ b/OvmfPkg/Include/IndustryStandard/LsiScsi.h
> > > @@ -26,6 +26,18 @@
> > >  #define LSI_REG_SIST0             0x42
> > >  #define LSI_REG_SIST1             0x43
> > >
> > > +//
> > > +// The status bits for DMA Status (DSTAT)
> > > +//
> > > +#define LSI_DSTAT_IID             0x01
> > > +#define LSI_DSTAT_R               0x02
> > > +#define LSI_DSTAT_SIR             0x04
> > > +#define LSI_DSTAT_SSI             0x08
> > > +#define LSI_DSTAT_ABRT            0x10
> > > +#define LSI_DSTAT_BF              0x20
> > > +#define LSI_DSTAT_MDPE            0x40
> > > +#define LSI_DSTAT_DFE             0x80
> > > +
> > 
> > (1) Please use the BITx macros.
> > 
> Will fix it.
> 
> > 
> > >  //
> > >  // The status bits for Interrupt Status Zero (ISTAT0)
> > >  //
> > > @@ -38,4 +50,31 @@
> > >  #define LSI_ISTAT0_SRST           0x40
> > >  #define LSI_ISTAT0_ABRT           0x80
> > >
> > > +//
> > > +// LSI 53C895A Script Instructions
> > > +//
> > > +#define LSI_INS_TYPE_BLK          0x00000000
> > > +#define LSI_INS_TYPE_IO           0x40000000
> > > +#define LSI_INS_TYPE_TC           0x80000000
> > > +
> > > +#define LSI_INS_BLK_SCSIP_DAT_OUT 0x00000000
> > > +#define LSI_INS_BLK_SCSIP_DAT_IN  0x01000000
> > > +#define LSI_INS_BLK_SCSIP_CMD     0x02000000
> > > +#define LSI_INS_BLK_SCSIP_STAT    0x03000000
> > > +#define LSI_INS_BLK_SCSIP_MSG_OUT 0x06000000
> > > +#define LSI_INS_BLK_SCSIP_MSG_IN  0x07000000
> > > +
> > > +#define LSI_INS_IO_OPC_SEL        0x00000000
> > > +#define LSI_INS_IO_OPC_WAIT_RESEL 0x10000000
> > > +
> > > +#define LSI_INS_TC_CP             0x00020000
> > > +#define LSI_INS_TC_JMP            0x00080000
> > > +#define LSI_INS_TC_RA             0x00800000
> > > +
> > > +#define LSI_INS_TC_OPC_JMP        0x00000000
> > > +#define LSI_INS_TC_OPC_INT        0x18000000
> > > +
> > > +#define LSI_INS_TC_SCSIP_DAT_OUT  0x00000000
> > > +#define LSI_INS_TC_SCSIP_MSG_IN   0x07000000
> > > +
> > >  #endif // _LSI_SCSI_H_
> > > diff --git a/OvmfPkg/LsiScsiDxe/LsiScsi.c b/OvmfPkg/LsiScsiDxe/LsiScsi.c
> > > index 1bcebd92e455..090d7df15b34 100644
> > > --- a/OvmfPkg/LsiScsiDxe/LsiScsi.c
> > > +++ b/OvmfPkg/LsiScsiDxe/LsiScsi.c
> > > @@ -43,6 +43,42 @@ Out8 (
> > >                            );
> > >  }
> > >
> > > +STATIC
> > > +EFI_STATUS
> > > +Out32 (
> > > +  IN LSI_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
> > > +In8 (
> > > +  IN  LSI_SCSI_DEV *Dev,
> > > +  IN  UINT32       Addr,
> > > +  OUT UINT8        *Data
> > > +  )
> > > +{
> > > +  return Dev->PciIo->Io.Read (
> > > +                          Dev->PciIo,
> > > +                          EfiPciIoWidthUint8,
> > > +                          PCI_BAR_IDX0,
> > > +                          Addr,
> > > +                          1,
> > > +                          Data
> > > +                          );
> > > +}
> > > +
> > >  STATIC
> > >  EFI_STATUS
> > >  LsiScsiReset (
> > > @@ -141,6 +177,272 @@ LsiScsiCheckRequest (
> > >    return EFI_SUCCESS;
> > >  }
> > >
> > > +/**
> > > +
> > > +  Interpret the request packet from the Extended SCSI Pass Thru Protocol and
> > > +  compose the script to submit the command and data to the contorller.
> > 
> > (2) s/contorller/controller/
> > 
> Oops. Will fix it.
> 
> > 
> > > +
> > > +  @param[in] Dev          The LSI 53C895A SCSI device the packet targets.
> > > +
> > > +  @param[in] Target       The SCSI target controlled by the LSI 53C895A SCSI
> > > +                          device.
> > > +
> > > +  @param[in] Lun          The Logical Unit Number under the SCSI target.
> > > +
> > > +  @param[in out] Packet   The Extended SCSI Pass Thru Protocol packet.
> > > +
> > > +
> > > +  @retval EFI_SUCCESS  The Extended SCSI Pass Thru Protocol packet was valid.
> > > +
> > > +  @return              Otherwise, invalid or unsupported parameters were
> > > +                       detected. Status codes are meant for direct forwarding
> > > +                       by the EFI_EXT_SCSI_PASS_THRU_PROTOCOL.PassThru()
> > > +                       implementation.
> > > +
> > > + **/
> > > +STATIC
> > > +EFI_STATUS
> > > +LsiScsiProcessRequest (
> > > +  IN LSI_SCSI_DEV                                   *Dev,
> > > +  IN UINT8                                          Target,
> > > +  IN UINT64                                         Lun,
> > > +  IN OUT EFI_EXT_SCSI_PASS_THRU_SCSI_REQUEST_PACKET *Packet
> > > +  )
> > > +{
> > > +  EFI_STATUS Status;
> > > +  UINT32     *Script;
> > > +  UINT8      *Cdb;
> > > +  UINT8      *MsgOut;
> > > +  UINT8      *MsgIn;
> > > +  UINT8      *ScsiStatus;
> > > +  UINT8      *Data;
> > > +  UINT8      DStat;
> > > +  UINT8      SIst0;
> > > +  UINT8      SIst1;
> > > +
> > > +  Script      = Dev->Dma->Script;
> > > +  Cdb         = Dev->Dma->Cdb;
> > > +  Data        = Dev->Dma->Data;
> > > +  MsgIn       = Dev->Dma->MsgIn;
> > > +  MsgOut      = &Dev->Dma->MsgOut;
> > > +  ScsiStatus  = &Dev->Dma->Status;
> > > +
> > > +  *ScsiStatus = 0xFF;
> > > +
> > > +  SetMem (Cdb, sizeof Dev->Dma->Cdb, 0x00);
> > > +  CopyMem (Cdb, Packet->Cdb, Packet->CdbLength);
> > > +
> > > +  //
> > > +  // Clean up the DMA buffer for the script.
> > > +  //
> > > +  SetMem (Script, sizeof Dev->Dma->Script, 0x00);
> > > +
> > > +  //
> > > +  // Compose the script to transfer data between the host and the device.
> > > +  //
> > > +  // Reference:
> > > +  //   LSI53C895A PCI to Ultra2 SCSI Controller Version 2.2
> > > +  //   - Chapter 5 SCSI SCRIPT Instruction Set
> > > +  //
> > > +  // All instructions used here consist of 2 32bit words. The first word
> > > +  // contains the command to execute. The second word is loaded into the
> > > +  // DMA SCRIPTS Pointer Save (DSPS) register as either the DMA address
> > > +  // for data transmission or the address/offset for the jump command.
> > > +  // Some commands, such as the selection of the target, don't need to
> > > +  // transfer data through DMA or jump to another instruction, then DSPS
> > > +  // has to be zero.
> > > +  //
> > > +  // There are 3 major parts in this script. The first part (1~3) contains
> > > +  // the instructions to select target and LUN and send the SCSI command
> > > +  // from the request packet. The second part (4~7) is to handle the
> > > +  // potential disconnection and prepare for the data transmission. The
> > > +  // instructions in the third part (8~10) transmit the given data and
> > > +  // collect the result. Instruction 11 raises the interrupt and marks the
> > > +  // end of the script.
> > > +  //
> > > +
> > > +  //
> > > +  // 1. Select target.
> > > +  //
> > > +  *Script++ = LSI_INS_TYPE_IO | LSI_INS_IO_OPC_SEL | (UINT32)Target << 16;
> > > +  *Script++ = 0x00000000;
> > > +
> > > +  //
> > > +  // 2. Select LUN.
> > > +  //
> > > +  *MsgOut   = 0x80 | (UINT8) Lun; // 0x80: Identify bit
> > 
> > This implies that the maximum (inclusive) LUN can be 127.
> > 
> > (3) I suggest adding a STATIC_ASSERT() to LsiScsiControllerStart(),
> > where you fetch "PcdLsiScsiMaxLunLimit".
> > 
> > (If the device has a stricter limit on LUNs than 127, then please use
> > that limit.)
> > 
> Ok, will add an assert for MaxLun.
> 
> > 
> > > +  *Script++ = LSI_INS_TYPE_BLK | LSI_INS_BLK_SCSIP_MSG_OUT | \
> > > +              sizeof Dev->Dma->MsgOut;
> > 
> > (4) Please cast the result of the "sizeof" operator to UINT32
> > explicitly.
> > 
> > 
> > (5) Please drop the backslash; it is not needed.
> > 
> > Note that (4) and (5) apply to multiple locations in this function;
> > please update each location in the composition of the script.
> > 
> Will fix them in v2.
> 
> > 
> > > +  *Script++ = LSI_SCSI_DMA_ADDR_LOW (Dev, MsgOut);
> > 
> > This opcode seems to mean that the device is not capable of taking the
> > LUN selector message from outside of the 32-bit address space.
> > 
> > (6) If that's the case, then please remove DUAL_ADDRESS_CYCLE from
> > "[PATCH 08/11] OvmfPkg/LsiScsiDxe: Map DMA buffer".
> > 
> > Otherwise, AllocateBuffer() could theoretically allocate the buffer
> > structure (including the MsgOut member) such that
> > LSI_SCSI_DMA_ADDR_LOW() would truncate the address.
> > 
> > 
> > (7) Once you remove DUAL_ADDRESS_CYCLE (i.e. once we guarantee that the
> > allocation will be satisfied from the 32-bit address space), you can
> > drop the _LOW suffix as well, from the macro name.
> > 
> > Now, if the device is actually 64-bit capable, only it would take a more
> > complex script, and you don't want that, that's fine -- the above steps
> > remain the same. (IOW it doesn't matter whether the device is 64-bit
> > uncapable, or you don't want to write a 64-bit aware command script --
> > you should remove DUAL_ADDRESS_CYCLE just the same.)
> > 
> Ok. Will remove DUAL_ADDRESS_CYCLE.
> 
> > 
> > > +
> > > +  //
> > > +  // 3. Send the SCSI Command.
> > > +  //
> > > +  *Script++ = LSI_INS_TYPE_BLK | LSI_INS_BLK_SCSIP_CMD | \
> > > +              sizeof Dev->Dma->Cdb;
> > > +  *Script++ = LSI_SCSI_DMA_ADDR_LOW (Dev, Cdb);
> > > +
> > > +  //
> > > +  // 4. Check whether the current SCSI phase is "Message In" or not
> > > +  //    and jump to 8 if it is.
> > > +  //
> > > +  *Script++ = LSI_INS_TYPE_TC | LSI_INS_TC_OPC_JMP | \
> > > +              LSI_INS_TC_SCSIP_MSG_IN | LSI_INS_TC_RA | \
> > > +              LSI_INS_TC_CP;
> > > +  *Script++ = 0x00000018;
> > 
> > (8) The constant 0x18 is obscure. Can you extend the comment? How is
> > "step 8" connected to value 0x18?
> > 
> > (Step 8 starts at command offset 7 in the script, meaning UINT32 offset
> > 14, or byte offset 56. But 0x18 is 24, not matching any of those
> > constants.)
> > 
> Urghhh, it should be "jump to 7". LSI_INS_TC_RA stands for "Relative
> Addressing Mode", so it's 4 + 24/8 = 7. Will fix the comment.
> 
> > 
> > > +
> > > +  //
> > > +  // 5. Read "Message" from the initiator to trigger reselect.
> > > +  //
> > > +  *Script++ = LSI_INS_TYPE_BLK | LSI_INS_BLK_SCSIP_MSG_IN | \
> > > +              sizeof Dev->Dma->MsgIn;
> > > +  *Script++ = LSI_SCSI_DMA_ADDR_LOW (Dev, MsgIn);
> > > +
> > > +  //
> > > +  // 6. Wait reselect.
> > > +  //
> > > +  *Script++ = LSI_INS_TYPE_IO | LSI_INS_IO_OPC_WAIT_RESEL;
> > > +  *Script++ = 0x00000000;
> > > +
> > > +  //
> > > +  // 7. Read "Message" from the initiator again
> > > +  //
> > > +  *Script++ = LSI_INS_TYPE_BLK | LSI_INS_BLK_SCSIP_MSG_IN | \
> > > +              sizeof Dev->Dma->MsgIn;
> > > +  *Script++ = LSI_SCSI_DMA_ADDR_LOW (Dev, MsgIn);
> > > +
> > > +  //
> > > +  // 8. Set the DMA command for the read/write operations.
> > > +  //
> > > +  if (Packet->DataDirection == EFI_EXT_SCSI_DATA_DIRECTION_READ &&
> > > +      Packet->InTransferLength > 0) {
> > > +    *Script++ = LSI_INS_TYPE_BLK | LSI_INS_BLK_SCSIP_DAT_IN | \
> > > +                Packet->InTransferLength;
> > > +    *Script++ = LSI_SCSI_DMA_ADDR_LOW (Dev, Data);
> > > +  } else if (Packet->DataDirection == EFI_EXT_SCSI_DATA_DIRECTION_WRITE &&
> > > +             Packet->OutTransferLength > 0) {
> > > +    // LsiScsiCheckRequest() guarantees that OutTransferLength is no
> > > +    // larger than sizeof Dev->Dma->Data, so we can safely copy the
> > > +    // the data to Dev->Dma->Data.
> > > +    CopyMem (Data, Packet->OutDataBuffer, Packet->OutTransferLength);
> > > +    *Script++ = LSI_INS_TYPE_BLK | LSI_INS_BLK_SCSIP_DAT_OUT | \
> > > +                Packet->OutTransferLength;
> > > +    *Script++ = LSI_SCSI_DMA_ADDR_LOW (Dev, Data);
> > > +  }
> > 
> > The comment is not bad, but it could be improved.
> > 
> > First, the comment style is not 100% correct (empty "//" lines are
> > missing before and after.)
> > 
> > Second, instead of a long comment, I'd suggest a short comment, plus an
> > ASSERT.
> > 
> Will amend the comment and add an assert.
> 
> > Third, the transfer length in the packet ("in" and "out" alike) isn't
> > *only* relevant due to buffer overflow concerns, but also because the
> > command opcode has some bits set (in particular,
> > LSI_INS_BLK_SCSIP_DAT_IN) that effectively limit the transfer length to
> > a bitmask that's narrower than a UINT32.
> > 
> Yeah, the controller only takes 24 bits for the transfer length.
> 
> > 
> > Another observation is that control can flow through this snippet
> > without taking *either* branch.
> > 
> > (For example if the data direction is WRITE, but OutTransferLength is
> > zero. That's a condition that LsiScsiCheckRequest() does not catch. My
> > point is not that LsiScsiCheckRequest() should catch it -- I think it's
> > not an invalid request --, but that LsiScsiProcessRequest() should deal
> > with it.)
> > 
> Hmmm, this is possible when ScsiIo requests sense data throught
> Packet->SenseData, and it's a read request with 0 InTransferLength.
> I encountered that during the development of this driver. It's gone after
> I set SenseDataLength to 0 later to make ScsiIo request sense data through
> Transfer buffer. Maybe I should filter this kind of requests in
> LsiScsiCheckRequest().
> 
> > If we take neither branch, then (minimally) the jump address under step
> > 4. will be wrong -- there's not going to be a DAT_IN / DAT_OUT command
> > to jump to.
> >
> > 
> > (9) So, for addressing all of the above, I suggest:
> > 
> >   //
> >   // 8. Set the DMA command for the read/write operations.
> >   //
> >   // LsiScsiCheckRequest() prevents both integer overflows in the command
> >   // opcodes, and buffer overflows.
> >   //
> >   if (Packet->DataDirection == EFI_EXT_SCSI_DATA_DIRECTION_READ) {
> >     ASSERT (Packet->InTransferLength <= sizeof Dev->Dma->Data);
> >     *Script++ = LSI_INS_TYPE_BLK | LSI_INS_BLK_SCSIP_DAT_IN |
> >                 Packet->InTransferLength;
> >   } else {
> >     ASSERT (Packet->OutTransferLength <= sizeof Dev->Dma->Data);
> >     CopyMem (Data, Packet->OutDataBuffer, Packet->OutTransferLength);
> >     *Script++ = LSI_INS_TYPE_BLK | LSI_INS_BLK_SCSIP_DAT_OUT |
> >                 Packet->OutTransferLength;
> >   }
> >   *Script++ = LSI_SCSI_DMA_ADDR (Dev, Data);
> >
> Thanks. Will modify the code here.
> 

The above code actually doesn't work because "TEST UNIT READY", the scsi
command, generates a request packet with 0 InTransferLength, and the
controller returns an error for data underflow.

The original code skips the DMA command for the 0 InTransferLength
requests and avoids the error. I'll keep the original code and add some
comments about that error.

Gary Lin

> > 
> > > +
> > > +  //
> > > +  // 9. Get the SCSI status.
> > > +  //
> > > +  *Script++ = LSI_INS_TYPE_BLK | LSI_INS_BLK_SCSIP_STAT | \
> > > +              sizeof Dev->Dma->Status;
> > > +  *Script++ = LSI_SCSI_DMA_ADDR_LOW (Dev, Status);
> > > +
> > > +  //
> > > +  // 10. Get the SCSI message.
> > > +  //
> > > +  *Script++ = LSI_INS_TYPE_BLK | LSI_INS_BLK_SCSIP_MSG_IN | \
> > > +              sizeof Dev->Dma->MsgIn;
> > > +  *Script++ = LSI_SCSI_DMA_ADDR_LOW (Dev, MsgIn);
> > > +
> > > +  //
> > > +  // 11. Raise the interrupt to end the script.
> > > +  //
> > > +  *Script++ = LSI_INS_TYPE_TC | LSI_INS_TC_OPC_INT | \
> > > +              LSI_INS_TC_SCSIP_DAT_OUT | LSI_INS_TC_JMP;
> > > +  *Script++ = 0x00000000;
> > > +
> > > +  //
> > > +  // Make sure the size of the script doesn't exceed the buffer.
> > > +  //
> > > +  ASSERT (Script < Dev->Dma->Script + sizeof Dev->Dma->Script);
> > 
> > (10) Please replace "<" with "<=".
> > 
> > (11) Please replace "sizeof" with ARRAY_SIZE().
> >
> Will fix it.
> 
> > 
> > > +
> > > +  //
> > > +  // The controller starts to execute the script once the DMA Script
> > > +  // Pointer (DSP) register is set.
> > > +  //
> > > +  Status = Out32 (Dev, LSI_REG_DSP, LSI_SCSI_DMA_ADDR_LOW(Dev, Script));
> > 
> > (12) Missing space after "LSI_SCSI_DMA_ADDR".
> > 
> Ditto.
> 
> > 
> > > +  if (EFI_ERROR (Status)) {
> > > +    return Status;
> > > +  }
> > 
> > This return statement doesn't seem right. We do not translate the error
> > code to one of the error codes specified for
> > EFI_EXT_SCSI_PASS_THRU_PROTOCOL.PassThru(). We also do not set any
> > output field in Packet.
> > 
> > (13) I think we should jump to the "Error" label from here.
> > 
> Right. It should jump to "Error".
> 
> > 
> > > +
> > > +  //
> > > +  // Poll the device registers (DSTAT, SIST0, and SIST1) until the SIR
> > > +  // bit sets.
> > > +  //
> > > +  for(;;) {
> > > +    Status = In8 (Dev, LSI_REG_DSTAT, &DStat);
> > > +    if (EFI_ERROR (Status)) {
> > > +      goto Error;
> > > +    }
> > > +    Status = In8 (Dev, LSI_REG_SIST0, &SIst0);
> > > +    if (EFI_ERROR (Status)) {
> > > +      goto Error;
> > > +    }
> > > +    Status = In8 (Dev, LSI_REG_SIST1, &SIst1);
> > > +    if (EFI_ERROR (Status)) {
> > > +      goto Error;
> > > +    }
> > > +
> > > +    if (SIst0 != 0 || SIst1 != 0) {
> > > +      goto Error;
> > > +    }
> > > +
> > > +    //
> > > +    // Check the SIR (SCRIPTS Interrupt Instruction Received) bit.
> > > +    //
> > > +    if (DStat & LSI_DSTAT_SIR) {
> > > +      break;
> > > +    }
> > > +
> > > +    gBS->Stall (Dev->StallPerPollUsec);
> > > +  }
> > > +
> > > +  //
> > > +  // Check if everything is good.
> > > +  //   SCSI Message Code 0x00: COMMAND COMPLETE
> > > +  //   SCSI Status  Code 0x00: Good
> > > +  //
> > > +  if (MsgIn[0] == 0 && *ScsiStatus == 0) {
> > 
> > (14) Please negate this condition, and unnest the success branch:
> > 
> >   if (MsgIn[0] != 0 || *ScsiStatus != 0) {
> >     goto Error;
> >   }
> > 
> >   /* ... */
> > 
> >   return EFI_SUCCESS;
> > 
> > 
> Ok. Will do that.
> 
> > > +    //
> > > +    // Copy Data to InDataBuffer if necessary.
> > > +    //
> > > +    if (Packet->DataDirection == EFI_EXT_SCSI_DATA_DIRECTION_READ) {
> > > +      CopyMem (Packet->InDataBuffer, Data, Packet->InTransferLength);
> > > +    }
> > > +
> > > +    //
> > > +    // The controller doesn't return sense data when replying "TEST UNIT READY",
> > > +    // so we have to set SenseDataLength to 0 to notify ScsiIo to issue
> > > +    // "REQUEST SENSE" for the sense data.
> > > +    //
> > > +    if (Cdb[0] == 0x00) {
> > > +      Packet->SenseDataLength = 0;
> > > +    }
> > > +    Packet->HostAdapterStatus = EFI_EXT_SCSI_STATUS_HOST_ADAPTER_OK;
> > > +    Packet->TargetStatus      = EFI_EXT_SCSI_STATUS_TARGET_GOOD;
> > > +
> > > +    return EFI_SUCCESS;
> > > +  }
> > 
> > The mapping on success does not deal with short transfers (in or out) --
> > we're supposed to update InTransferLength / OutTransferLength on output.
> > 
> > (15) Does the device report short transfers somehow?
> > 
> I'm not sure. Have to check the spec of controller to see if there is
> any instruction to fetch the real transfer length.
> 
> > 
> > The spec says about SenseDataLength, "On input, the length in bytes of
> > the SenseData buffer. On output, the number of bytes written to the
> > SenseData buffer".
> > 
> > But we only set SenseDataLength conditionally. In case we do *not* set
> > it, the caller may attempt to parse garbage from the "SenseData" buffer.
> > 
> > (16) We should either get real sense data back to the caller, or always
> > zero out "SenseDataLength".
> > 
> > 
> For simplicity, I would choose zeroing out "SenseDataLength" since
> ScsiDiskDxe can handle it.
> 
> > > +
> > > +Error:
> > > +  DEBUG((DEBUG_VERBOSE, "%a: dstat: %02X, sist0: %02X, sist1: %02X\n",
> > > +         __FUNCTION__, DStat, SIst0, SIst1));
> > 
> > (17) missing space after "DEBUG".
> > 
> > (18) The line starting with __FUNCTION__ is not correctly indented.
> >
> Will fix them.
> 
> > 
> > > +  //
> > > +  // Update the request packet to reflect the status.
> > > +  //
> > > +  if (*ScsiStatus != 0xFF) {
> > > +    Packet->TargetStatus    = *ScsiStatus;
> > > +  } else {
> > > +    Packet->TargetStatus    = EFI_EXT_SCSI_STATUS_TARGET_TASK_ABORTED;
> > > +  }
> > > +  Packet->HostAdapterStatus = EFI_EXT_SCSI_STATUS_HOST_ADAPTER_OTHER;
> > > +  Packet->InTransferLength  = 0;
> > > +  Packet->OutTransferLength = 0;
> > > +  Packet->SenseDataLength   = 0;
> > > +
> > > +  return EFI_DEVICE_ERROR;
> > > +}
> > > +
> > >  //
> > >  // The next seven functions implement EFI_EXT_SCSI_PASS_THRU_PROTOCOL
> > >  // for the LSI 53C895A SCSI Controller. Refer to UEFI Spec 2.3.1 + Errata C,
> > > @@ -168,6 +470,11 @@ LsiScsiPassThru (
> > >      return Status;
> > >    }
> > >
> > > +  Status = LsiScsiProcessRequest (Dev, *Target, Lun, Packet);
> > > +  if (EFI_ERROR (Status)) {
> > > +    return Status;
> > > +  }
> > > +
> > >    return EFI_SUCCESS;
> > >  }
> > >
> > 
> > (19) I think this can be simplified -- just return Status after calling
> > LsiScsiProcessRequest().
> >
> Agree.
> 
> > 
> > > @@ -469,6 +776,7 @@ LsiScsiControllerStart (
> > >
> > >    Dev->MaxTarget = PcdGet8 (PcdLsiScsiMaxTargetLimit);
> > >    Dev->MaxLun = PcdGet8 (PcdLsiScsiMaxLunLimit);
> > > +  Dev->StallPerPollUsec = PcdGet32 (PcdLsiScsiStallPerPollUsec);
> > >
> > >    Status = gBS->OpenProtocol (
> > >                    ControllerHandle,
> > > diff --git a/OvmfPkg/LsiScsiDxe/LsiScsi.h b/OvmfPkg/LsiScsiDxe/LsiScsi.h
> > > index 9272eb7506c7..1a16ef9f7795 100644
> > > --- a/OvmfPkg/LsiScsiDxe/LsiScsi.h
> > > +++ b/OvmfPkg/LsiScsiDxe/LsiScsi.h
> > > @@ -13,6 +13,11 @@
> > >  #define _LSI_SCSI_DXE_H_
> > >
> > >  typedef struct {
> > > +  //
> > > +  // Allocate 32 UINT32 entries for the script and it's sufficient for
> > > +  // 16 instructions.
> > > +  //
> > > +  UINT32                          Script[32];
> > >    //
> > >    // The max size of CDB is 32.
> > >    //
> > > @@ -21,6 +26,18 @@ typedef struct {
> > >    // Allocate 64KB for read/write buffer.
> > >    //
> > >    UINT8                           Data[0x10000];
> > > +  //
> > > +  // For SCSI Message In phase
> > > +  //
> > > +  UINT8                           MsgIn[2];
> > > +  //
> > > +  // For SCSI Message Out phase
> > > +  //
> > > +  UINT8                           MsgOut;
> > > +  //
> > > +  // For SCSI Status phase
> > > +  //
> > > +  UINT8                           Status;
> > >  } LSI_SCSI_DMA_BUFFER;
> > >
> > >  typedef struct {
> > > @@ -30,6 +47,7 @@ typedef struct {
> > >    EFI_PCI_IO_PROTOCOL             *PciIo;
> > >    UINT8                           MaxTarget;
> > >    UINT8                           MaxLun;
> > > +  UINT32                          StallPerPollUsec;
> > >    LSI_SCSI_DMA_BUFFER             *Dma;
> > >    EFI_PHYSICAL_ADDRESS            DmaPhysical;
> > >    VOID                            *DmaMapping;
> > > @@ -42,6 +60,9 @@ typedef struct {
> > >  #define LSI_SCSI_FROM_PASS_THRU(PassThruPtr) \
> > >    CR (PassThruPtr, LSI_SCSI_DEV, PassThru, LSI_SCSI_DEV_SIGNATURE)
> > >
> > > +#define LSI_SCSI_DMA_ADDR_LOW(Dev, MemberName) \
> > > +  ((UINT32)(Dev->DmaPhysical + OFFSET_OF (LSI_SCSI_DMA_BUFFER, MemberName)))
> > > +
> > >
> > >  //
> > >  // Probe, start and stop functions of this driver, called by the DXE core for
> > > diff --git a/OvmfPkg/LsiScsiDxe/LsiScsiDxe.inf b/OvmfPkg/LsiScsiDxe/LsiScsiDxe.inf
> > > index 68844c6772e3..cbd7294573ac 100644
> > > --- a/OvmfPkg/LsiScsiDxe/LsiScsiDxe.inf
> > > +++ b/OvmfPkg/LsiScsiDxe/LsiScsiDxe.inf
> > > @@ -41,3 +41,6 @@ [Protocols]
> > >  [FixedPcd]
> > >    gUefiOvmfPkgTokenSpaceGuid.PcdLsiScsiMaxTargetLimit   ## CONSUMES
> > >    gUefiOvmfPkgTokenSpaceGuid.PcdLsiScsiMaxLunLimit      ## CONSUMES
> > > +
> > > +[Pcd]
> > > +  gUefiOvmfPkgTokenSpaceGuid.PcdLsiScsiStallPerPollUsec ## CONSUMES
> > > diff --git a/OvmfPkg/OvmfPkg.dec b/OvmfPkg/OvmfPkg.dec
> > > index ae7d1d648d22..57e418d4b8a9 100644
> > > --- a/OvmfPkg/OvmfPkg.dec
> > > +++ b/OvmfPkg/OvmfPkg.dec
> > > @@ -179,6 +179,9 @@ [PcdsFixedAtBuild]
> > >    gUefiOvmfPkgTokenSpaceGuid.PcdLsiScsiMaxTargetLimit|7|UINT8|0x3b
> > >    gUefiOvmfPkgTokenSpaceGuid.PcdLsiScsiMaxLunLimit|0|UINT8|0x3c
> > >
> > > +  ## Microseconds to stall between polling for LsiScsi request result
> > > +  gUefiOvmfPkgTokenSpaceGuid.PcdLsiScsiStallPerPollUsec|5|UINT32|0x3d
> > > +
> > 
> > (20) The token value should be 0x3c here, according to my request (5)
> > under "[PATCH 06/11] OvmfPkg/LsiScsiDxe: Report Targets and LUNs".
> >
> Ok, will do that if we won't change the toke value of PcdMptScsiStallPerPollUsec.
> 
> Thanks,
> 
> Gary Lin
> 
> > 
> > >    gUefiOvmfPkgTokenSpaceGuid.PcdOvmfFlashNvStorageEventLogBase|0x0|UINT32|0x8
> > >    gUefiOvmfPkgTokenSpaceGuid.PcdOvmfFlashNvStorageEventLogSize|0x0|UINT32|0x9
> > >    gUefiOvmfPkgTokenSpaceGuid.PcdOvmfFirmwareFdSize|0x0|UINT32|0xa
> > >
> > 
> > Thanks
> > Laszlo
> > 
> 
> 
> 
>