On Thu, Jul 16, 2020 at 07:37:09PM +0200, Laszlo Ersek wrote:
> On 07/16/20 09:46, 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.
> >
> > v2:
> > - Use the BITx macros for the most of LSI_* constants
> > - Fix a typo: contorller => controller
> > - Add SeaBIOS lsi-scsi driver as one of the references of the script
> > - Cast the result of sizeof to UINT32 for the instructions of the
> > script
> > - Drop the backslashes
> > - Replace LSI_SCSI_DMA_ADDR_LOW with LSI_SCSI_DMA_ADDR since we
> > already removed DUAL_ADDRESS_CYCLE
> > - Add more comments for the script
> > - Fix the check of the script size at the end of the script
> > - Always set SenseDataLength to 0 to avoid the caller to access
> > SenseData
> > - Improve the error handling in LsiScsiProcessRequest()
> >
> > Cc: Jordan Justen <jordan.l.jus...@intel.com>
> > Cc: Laszlo Ersek <ler...@redhat.com>
> > Cc: Ard Biesheuvel <ard.biesheu...@arm.com>
> > Signed-off-by: Gary Lin <g...@suse.com>
> > ---
> > OvmfPkg/Include/IndustryStandard/LsiScsi.h | 63 ++++
> > OvmfPkg/LsiScsiDxe/LsiScsi.c | 336 ++++++++++++++++++++-
> > OvmfPkg/LsiScsiDxe/LsiScsi.h | 21 ++
> > OvmfPkg/LsiScsiDxe/LsiScsiDxe.inf | 3 +
> > OvmfPkg/OvmfPkg.dec | 3 +
> > 5 files changed, 425 insertions(+), 1 deletion(-)
> >
> > diff --git a/OvmfPkg/Include/IndustryStandard/LsiScsi.h
> > b/OvmfPkg/Include/IndustryStandard/LsiScsi.h
> > index 185e553c8eb4..9964bd40385c 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 BIT0
> > +#define LSI_DSTAT_R BIT1
> > +#define LSI_DSTAT_SIR BIT2
> > +#define LSI_DSTAT_SSI BIT3
> > +#define LSI_DSTAT_ABRT BIT4
> > +#define LSI_DSTAT_BF BIT5
> > +#define LSI_DSTAT_MDPE BIT6
> > +#define LSI_DSTAT_DFE BIT7
> > +
> > //
> > // The status bits for Interrupt Status Zero (ISTAT0)
> > //
> > @@ -38,4 +50,55 @@
> > #define LSI_ISTAT0_SRST BIT6
> > #define LSI_ISTAT0_ABRT BIT7
> >
> > +//
> > +// The status bits for SCSI Interrupt Status Zero (SIST0)
> > +//
> > +#define LSI_SIST0_PAR BIT0
> > +#define LSI_SIST0_RST BIT1
> > +#define LSI_SIST0_UDC BIT2
> > +#define LSI_SIST0_SGE BIT3
> > +#define LSI_SIST0_RSL BIT4
> > +#define LSI_SIST0_SEL BIT5
> > +#define LSI_SIST0_CMP BIT6
> > +#define LSI_SIST0_MA BIT7
> > +
> > +//
> > +// The status bits for SCSI Interrupt Status One (SIST1)
> > +//
> > +#define LSI_SIST1_HTH BIT0
> > +#define LSI_SIST1_GEN BIT1
> > +#define LSI_SIST1_STO BIT2
> > +#define LSI_SIST1_R3 BIT3
> > +#define LSI_SIST1_SBMC BIT4
> > +#define LSI_SIST1_R5 BIT5
> > +#define LSI_SIST1_R6 BIT6
> > +#define LSI_SIST1_R7 BIT7
> > +
> > +//
> > +// LSI 53C895A Script Instructions
> > +//
> > +#define LSI_INS_TYPE_BLK 0x00000000
> > +#define LSI_INS_TYPE_IO BIT30
> > +#define LSI_INS_TYPE_TC BIT31
> > +
> > +#define LSI_INS_BLK_SCSIP_DAT_OUT 0x00000000
> > +#define LSI_INS_BLK_SCSIP_DAT_IN BIT24
> > +#define LSI_INS_BLK_SCSIP_CMD BIT25
> > +#define LSI_INS_BLK_SCSIP_STAT (BIT24 | BIT25)
> > +#define LSI_INS_BLK_SCSIP_MSG_OUT (BIT25 | BIT26)
> > +#define LSI_INS_BLK_SCSIP_MSG_IN (BIT24 | BIT25 | BIT26)
> > +
> > +#define LSI_INS_IO_OPC_SEL 0x00000000
> > +#define LSI_INS_IO_OPC_WAIT_RESEL BIT28
> > +
> > +#define LSI_INS_TC_CP BIT17
> > +#define LSI_INS_TC_JMP BIT19
> > +#define LSI_INS_TC_RA BIT23
> > +
> > +#define LSI_INS_TC_OPC_JMP 0x00000000
> > +#define LSI_INS_TC_OPC_INT (BIT27 | BIT28)
> > +
> > +#define LSI_INS_TC_SCSIP_DAT_OUT 0x00000000
> > +#define LSI_INS_TC_SCSIP_MSG_IN (BIT24 | BIT25 | BIT26)
> > +
> > #endif // _LSI_SCSI_H_
> > diff --git a/OvmfPkg/LsiScsiDxe/LsiScsi.c b/OvmfPkg/LsiScsiDxe/LsiScsi.c
> > index b3a88cc7119b..d5fe1d4ab3b8 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,303 @@ 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 controller.
> > +
> > + @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.
> > + //
> > + // References:
> > + // * LSI53C895A PCI to Ultra2 SCSI Controller Version 2.2
> > + // - Chapter 5 SCSI SCRIPT Instruction Set
> > + // * SEABIOS lsi-scsi driver
> > + //
> > + // 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
> > + *Script++ = LSI_INS_TYPE_BLK | LSI_INS_BLK_SCSIP_MSG_OUT |
> > + (UINT32)sizeof Dev->Dma->MsgOut;
> > + *Script++ = LSI_SCSI_DMA_ADDR (Dev, MsgOut);
> > +
> > + //
> > + // 3. Send the SCSI Command.
> > + //
> > + *Script++ = LSI_INS_TYPE_BLK | LSI_INS_BLK_SCSIP_CMD |
> > + (UINT32)sizeof Dev->Dma->Cdb;
> > + *Script++ = LSI_SCSI_DMA_ADDR (Dev, Cdb);
> > +
> > + //
> > + // 4. Check whether the current SCSI phase is "Message In" or not
> > + // and jump to 7 if it is.
> > + // Note: LSI_INS_TC_RA stands for "Relative Address Mode", so the
> > + // offset 0x18 in the second word means jumping forward
> > + // 3 (0x18/8) instructions.
> > + //
> > + *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;
> > +
> > + //
> > + // 5. Read "Message" from the initiator to trigger reselect.
> > + //
> > + *Script++ = LSI_INS_TYPE_BLK | LSI_INS_BLK_SCSIP_MSG_IN |
> > + (UINT32)sizeof Dev->Dma->MsgIn;
> > + *Script++ = LSI_SCSI_DMA_ADDR (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 |
> > + (UINT32)sizeof Dev->Dma->MsgIn;
> > + *Script++ = LSI_SCSI_DMA_ADDR (Dev, MsgIn);
> > +
> > + //
> > + // 8. Set the DMA command for the read/write operations.
> > + // Note: Some requests, e.g. "TEST UNIT READY", do not come with
> > + // allocated InDataBuffer or OutDataBuffer. We skip the DMA
> > + // data command for those requests or this script would fail
> > + // with LSI_SIST0_SGE due to the zero data length.
> > + //
> > + // LsiScsiCheckRequest() prevents both integer overflows in the command
> > + // opcodes, and buffer overflows.
> > + //
> > + if (Packet->DataDirection == EFI_EXT_SCSI_DATA_DIRECTION_READ &&
> > + Packet->InTransferLength > 0) {
> > + ASSERT (Packet->InTransferLength <= sizeof Dev->Dma->Data);
> > + *Script++ = LSI_INS_TYPE_BLK | LSI_INS_BLK_SCSIP_DAT_IN |
> > + Packet->InTransferLength;
> > + *Script++ = LSI_SCSI_DMA_ADDR (Dev, Data);
> > + } else if (Packet->DataDirection == EFI_EXT_SCSI_DATA_DIRECTION_WRITE &&
> > + Packet->OutTransferLength > 0) {
> > + 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);
> > + }
>
> I understand the explanation, thanks.
>
> However, we can still improve this code. The following sub-conditions:
>
> Packet->DataDirection == EFI_EXT_SCSI_DATA_DIRECTION_READ
>
> and
>
> Packet->DataDirection == EFI_EXT_SCSI_DATA_DIRECTION_WRITE
>
> are superfluous. What we care about are
>
> Packet->InTransferLength > 0
>
> and
>
> Packet->OutTransferLength > 0
>
> And the latter (length-based) conditions actually *imply* the former
> (direction-based) conditions, because:
>
> - LsiScsiCheckRequest() returns EFI_INVALID_PARAMETER if
> DataDirection > EFI_EXT_SCSI_DATA_DIRECTION_BIDIRECTIONAL
>
> - LsiScsiCheckRequest() returns EFI_UNSUPPORTED if
> DataDirection == EFI_EXT_SCSI_DATA_DIRECTION_BIDIRECTIONAL
>
> - LsiScsiCheckRequest() returns EFI_INVALID_PARAMETER if
> InTransferLength > 0, but
> DataDirection == EFI_EXT_SCSI_DATA_DIRECTION_WRITE
>
> - LsiScsiCheckRequest() returns EFI_INVALID_PARAMETER if
> OutTransferLength > 0, but
> DataDirection == EFI_EXT_SCSI_DATA_DIRECTION_READ
>
> This means that (InTransferLength > 0) guarantees
> EFI_EXT_SCSI_DATA_DIRECTION_READ, and (OutTransferLength > 0) guarantees
> EFI_EXT_SCSI_DATA_DIRECTION_WRITE.
>
> (1) Therefore I suggest moving the respective sub-conditions from the
> "if" statements into the dependent blocks, as assertions:
>
> if (Packet->InTransferLength > 0) {
> ASSERT (Packet->DataDirection == EFI_EXT_SCSI_DATA_DIRECTION_READ);
> ASSERT (Packet->InTransferLength <= sizeof Dev->Dma->Data);
> ...
> } else if (Packet->OutTransferLength > 0) {
> ASSERT (Packet->DataDirection == EFI_EXT_SCSI_DATA_DIRECTION_WRITE);
> ASSERT (Packet->OutTransferLength <= sizeof Dev->Dma->Data);
> ...
> }
>
Thanks for the suggestion. This makes the if statements easier to read.
Will modify them in v3.
> > +
> > + //
> > + // 9. Get the SCSI status.
> > + //
> > + *Script++ = LSI_INS_TYPE_BLK | LSI_INS_BLK_SCSIP_STAT |
> > + (UINT32)sizeof Dev->Dma->Status;
> > + *Script++ = LSI_SCSI_DMA_ADDR (Dev, Status);
> > +
> > + //
> > + // 10. Get the SCSI message.
> > + //
> > + *Script++ = LSI_INS_TYPE_BLK | LSI_INS_BLK_SCSIP_MSG_IN |
> > + (UINT32)sizeof Dev->Dma->MsgIn;
> > + *Script++ = LSI_SCSI_DMA_ADDR (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 + ARRAY_SIZE (Dev->Dma->Script));
> > +
> > + //
> > + // 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 (Dev, Script));
> > + if (EFI_ERROR (Status)) {
> > + goto 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) {
> > + goto Error;
> > + }
> > +
> > + //
> > + // Copy Data to InDataBuffer if necessary.
> > + //
> > + if (Packet->DataDirection == EFI_EXT_SCSI_DATA_DIRECTION_READ) {
> > + CopyMem (Packet->InDataBuffer, Data, Packet->InTransferLength);
> > + }
> > +
> > + //
> > + // Always set SenseDataLength to 0.
> > + // The instructions of LSI53C895A doesn't reply sense data. Instead, it
> > + // relies on the SCSI command, "REQUEST SENSE", to get sense data. We set
> > + // SenseDataLength to 0 to notify ScsiDiskDxe that there is no sense data
> > + // written even if this request is processed successfully, so that It
> > will
> > + // issue "REQUEST SENSE" later to retrieve sense data.
> > + //
> > + Packet->SenseDataLength = 0;
> > + Packet->HostAdapterStatus = EFI_EXT_SCSI_STATUS_HOST_ADAPTER_OK;
> > + Packet->TargetStatus = EFI_EXT_SCSI_STATUS_TARGET_GOOD;
> > +
> > + return EFI_SUCCESS;
> > +
> > +Error:
> > + DEBUG ((DEBUG_VERBOSE, "%a: dstat: %02X, sist0: %02X, sist1: %02X\n",
> > + __FUNCTION__, DStat, SIst0, SIst1));
>
> The Error label may be reached without setting some (or even any) of
> DStat, SIst0, SIst1.
>
> (2) Please set all three of DStat, SIst0, and SIst1 to zero, near the
> top of the function, with explicit assignments (not initialization).
>
Right. Will set them to zero in v3.
> > + //
> > + // Update the request packet to reflect the status.
> > + //
> > + if (*ScsiStatus != 0xFF) {
> > + Packet->TargetStatus = *ScsiStatus;
> > + } else {
> > + Packet->TargetStatus = EFI_EXT_SCSI_STATUS_TARGET_TASK_ABORTED;
> > + }
> > +
> > + if (SIst0 & LSI_SIST0_PAR) {
> > + Packet->HostAdapterStatus =
> > EFI_EXT_SCSI_STATUS_HOST_ADAPTER_PARITY_ERROR;
> > + } else if (SIst0 & LSI_SIST0_RST) {
> > + Packet->HostAdapterStatus = EFI_EXT_SCSI_STATUS_HOST_ADAPTER_BUS_RESET;
> > + } else if (SIst0 & LSI_SIST0_UDC) {
> > + //
> > + // The target device is disconnected unexpectedly. According to UEFI
> > spec,
> > + // this is TIMEOUT_COMMAND.
> > + //
> > + Packet->HostAdapterStatus =
> > EFI_EXT_SCSI_STATUS_HOST_ADAPTER_TIMEOUT_COMMAND;
> > + } else if (SIst0 & LSI_SIST0_SGE) {
> > + Packet->HostAdapterStatus =
> > EFI_EXT_SCSI_STATUS_HOST_ADAPTER_DATA_OVERRUN_UNDERRUN;
> > + } else if (SIst1 & LSI_SIST1_HTH) {
> > + Packet->HostAdapterStatus = EFI_EXT_SCSI_STATUS_HOST_ADAPTER_TIMEOUT;
> > + } else if (SIst1 & LSI_SIST1_GEN) {
> > + Packet->HostAdapterStatus = EFI_EXT_SCSI_STATUS_HOST_ADAPTER_TIMEOUT;
> > + } else if (SIst1 & LSI_SIST1_STO) {
> > + Packet->HostAdapterStatus =
> > EFI_EXT_SCSI_STATUS_HOST_ADAPTER_SELECTION_TIMEOUT;
> > + } else {
> > + Packet->HostAdapterStatus = EFI_EXT_SCSI_STATUS_HOST_ADAPTER_OTHER;
> > + }
>
> According to the UEFI spec, on EFI_DEVICE_ERROR (see below):
>
> See HostAdapterStatus, TargetStatus, SenseDataLength, and SenseData in
> that order for additional status information.
>
> (3) Therefore, on this error branch, please also set SenseDataLength to
> zero.
>
Will set SenseDataLength to 0 in v3.
> > +
> > + 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,7 +501,7 @@ LsiScsiPassThru (
> > return Status;
> > }
> >
> > - return EFI_UNSUPPORTED;
> > + return LsiScsiProcessRequest (Dev, *Target, Lun, Packet);
> > }
>
> The LsiScsiPassThru() function can now return EFI_SUCCESS, without
> updating InTransferLength and OutTransferLength.
>
> But sticking (for now) with EFI_UNSUPPORTED would also be wrong, because
> (according to the UEFI spec) EFI_UNSUPPORTED means "The SCSI Request
> Packet was not sent", and here we *did* talk to the device.
>
> (4) So please squash the next patch -- "[PATCH v2 11/12]
> OvmfPkg/LsiScsiDxe: Calculate the transferred bytes" -- into this one,
> in the v3 series.
>
> (The next patch, which handles the transfer lengths, is not large,
> thankfully.)
>
Since the calculation wasn't in v1, I thought it's easier to review the
change in an extra patch :)
Will squash the patch in v3.
Gary Lin
> For now I'll comment separately under that patch.
>
> Thanks!
> Laszlo
>
> >
> > EFI_STATUS
> > @@ -474,6 +807,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 05deeed379fe..6ecf523f5a9e 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.
> > //
> > @@ -25,6 +30,18 @@ typedef struct {
> > // Count (DBC), a 24-bit register, so the maximum is 0xFFFFFF
> > (16MB-1).
> > //
> > UINT8 Data[SIZE_64KB];
> > + //
> > + // 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 {
> > @@ -34,6 +51,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;
> > @@ -46,6 +64,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(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 6111449577a8..4c7abcec618f 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 ca13a3cb11d3..f16c00ad5b99 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
> > +
> >
> > gUefiOvmfPkgTokenSpaceGuid.PcdOvmfFlashNvStorageEventLogBase|0x0|UINT32|0x8
> >
> > gUefiOvmfPkgTokenSpaceGuid.PcdOvmfFlashNvStorageEventLogSize|0x0|UINT32|0x9
> > gUefiOvmfPkgTokenSpaceGuid.PcdOvmfFirmwareFdSize|0x0|UINT32|0xa
> >
>
-=-=-=-=-=-=-=-=-=-=-=-
Groups.io Links: You receive all messages sent to this group.
View/Reply Online (#62718): https://edk2.groups.io/g/devel/message/62718
Mute This Topic: https://groups.io/mt/75537215/21656
Group Owner: devel+ow...@edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/unsub [arch...@mail-archive.com]
-=-=-=-=-=-=-=-=-=-=-=-
