Re: [edk2-devel] [PATCH 15/17] OvmfPkg/PvScsiDxe: Support sending SCSI request and receive response

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

There are many style problems with this patch.

I'm going to focus on those, for now. I might not catch everything in a
single review run, though (there are quite a few warts), so please don't
be annoyed if I end up pointing out further style problems under v2.

On 03/16/20 16:01, Liran Alon wrote:
> Implement EXT_SCSI_PASS_THRU.PassThru().
> 
> Machines should be able to boot after this commit.
> Tested with Ubuntu 16.04 guest.
> 
> Ref: https://bugzilla.tianocore.org/show_bug.cgi?id=2567
> Reviewed-by: Nikita Leshenko <nikita.leshchenko@oracle.com>
> Signed-off-by: Liran Alon <liran.alon@oracle.com>
> ---
>  OvmfPkg/OvmfPkg.dec          |   6 +
>  OvmfPkg/PvScsiDxe/PvScsi.c   | 423 ++++++++++++++++++++++++++++++++++-
>  OvmfPkg/PvScsiDxe/PvScsi.h   |   1 +
>  OvmfPkg/PvScsiDxe/PvScsi.inf |   5 +-
>  4 files changed, 432 insertions(+), 3 deletions(-)
> 
> diff --git a/OvmfPkg/OvmfPkg.dec b/OvmfPkg/OvmfPkg.dec
> index 76ce507e8bd0..e78c771f53e9 100644
> --- a/OvmfPkg/OvmfPkg.dec
> +++ b/OvmfPkg/OvmfPkg.dec
> @@ -130,6 +130,12 @@
>    gUefiOvmfPkgTokenSpaceGuid.PcdPvScsiMaxTargetLimit|64|UINT8|0x40
>    gUefiOvmfPkgTokenSpaceGuid.PcdPvScsiMaxLunLimit|0|UINT8|0x41
>  
> +  ## After PvScsiDxe sends a SCSI request to the device, it waits for
> +  #  the request completion in a polling loop.
> +  #  This constant defines how many micro-seconds to wait between each
> +  #  polling loop iteration.
> +  gUefiOvmfPkgTokenSpaceGuid.PcdPvScsiWaitForCmpStallInUsecs|5|UINT32|0x42

(1) Please keep the token space condensed.

> +
>    gUefiOvmfPkgTokenSpaceGuid.PcdOvmfFlashNvStorageEventLogBase|0x0|UINT32|0x8
>    gUefiOvmfPkgTokenSpaceGuid.PcdOvmfFlashNvStorageEventLogSize|0x0|UINT32|0x9
>    gUefiOvmfPkgTokenSpaceGuid.PcdOvmfFirmwareFdSize|0x0|UINT32|0xa
> diff --git a/OvmfPkg/PvScsiDxe/PvScsi.c b/OvmfPkg/PvScsiDxe/PvScsi.c
> index e48929bf044c..e7d0a23db6ab 100644
> --- a/OvmfPkg/PvScsiDxe/PvScsi.c
> +++ b/OvmfPkg/PvScsiDxe/PvScsi.c
> @@ -30,6 +30,26 @@
>  // Ext SCSI Pass Thru utilities
>  //
>  
> +//
> +// Reads a 32-bit value into BAR0 using MMIO
> +//

(2) Please stick with the /** **/ comment style here.

> +STATIC
> +EFI_STATUS
> +PvScsiMmioRead32 (
> +  IN CONST PVSCSI_DEV   *Dev,
> +  IN UINT64             Offset,
> +  OUT UINT32            *Value
> +  )
> +{
> +  return Dev->PciIo->Mem.Read(
> +                           Dev->PciIo,
> +                           EfiPciIoWidthUint32,
> +                           0,   // BarIndex

(3) Please use PCI_BAR_IDX0.

> +                           Offset,
> +                           1,   // Count
> +                           Value
> +                           );
> +}
>  
>  //
>  // Writes a 32-bit value into BAR0 using MMIO
> @@ -100,6 +120,343 @@ PvScsiWriteCmdDesc (
>  
>    return EFI_SUCCESS;
>  }
> +
> +//
> +// Returns if PVSCSI request ring is full
> +//

(4) Same as (2).

Please rework the rest of the function-level comments accordingly.

> +STATIC
> +BOOLEAN
> +PvScsiIsReqRingFull (
> +  IN CONST PVSCSI_DEV   *Dev
> +  )
> +{
> +  PVSCSI_RINGS_STATE *RingsState;
> +  UINT64             ReqNumEntries;
> +
> +  RingsState = Dev->RingDesc.RingState;
> +  ReqNumEntries = 1 << RingsState->ReqNumEntriesLog2;

(5) Wrong for two reasons:

(5a) Based on ReqNumEntries having type UINT64, the shift count may
presumably be larger than 31. But the constant "1" has type "signed int"
(mapping to INT32 in edk2), and so we should never left-shift that by
even 31 positions (we should never shift bits into the sign bit). Let
alone by more than 31 positions.

In other words, the constant should be 1ULL.

(5b) Please use RShiftU64() from BaseLib.

> +  return (RingsState->ReqProdIdx - RingsState->CmpConsIdx) >= ReqNumEntries;
> +}
> +
> +//
> +// Returns pointer to current request descriptor to produce
> +//
> +STATIC
> +PVSCSI_RING_REQ_DESC *
> +PvScsiGetCurrentRequest (
> +  IN CONST PVSCSI_DEV   *Dev
> +  )
> +{
> +  PVSCSI_RINGS_STATE *RingState;
> +  UINT64             ReqNumEntries;
> +
> +  RingState = Dev->RingDesc.RingState;
> +  ReqNumEntries = 1 << RingState->ReqNumEntriesLog2;

(6) Same as (5).

Please rework further occurrences similarly.

> +  return Dev->RingDesc.RingReqs +
> +         (RingState->ReqProdIdx & (ReqNumEntries - 1));
> +}
> +
> +//
> +// Returns pointer to current completion descriptor to consume
> +//
> +STATIC
> +PVSCSI_RING_CMP_DESC *
> +PvScsiGetCurrentResponse (
> +  IN CONST PVSCSI_DEV   *Dev
> +  )
> +{
> +  PVSCSI_RINGS_STATE *RingState;
> +  UINT64             CmpNumEntries;
> +
> +  RingState = Dev->RingDesc.RingState;
> +  CmpNumEntries = 1 << RingState->CmpNumEntriesLog2;
> +  return Dev->RingDesc.RingCmps +
> +         (RingState->CmpConsIdx & (CmpNumEntries - 1));
> +}
> +
> +//
> +// Wait for device to signal completion of submitted requests
> +//
> +STATIC
> +EFI_STATUS
> +PvScsiWaitForRequestCompletion (
> +  IN CONST PVSCSI_DEV   *Dev
> +  )
> +{
> +  EFI_STATUS Status;
> +  UINT32     IntrStatus;
> +
> +  //
> +  // Note: We don't yet support Timeout according to
> +  // EFI_EXT_SCSI_PASS_THRU_SCSI_REQUEST_PACKET.Timeout.
> +  //
> +  // This is consistent with some other Scsi PassThru drivers
> +  // such as VirtioScsi.
> +  //
> +  for (;;) {
> +    Status = PvScsiMmioRead32 (Dev, PVSCSI_REG_OFFSET_INTR_STATUS, &IntrStatus);
> +    if (EFI_ERROR (Status)) {
> +      return Status;
> +    }
> +
> +    //
> +    // PVSCSI_INTR_CMPL_MASK is set if device completed submitted requests
> +    //
> +    if (IntrStatus & PVSCSI_INTR_CMPL_MASK) {

(7) When checking bitmasks in a logical context, please use an explicit
"!= 0" comparison.

(Remember that "==" and "!=" bind more strongly than binary "&" does,
thus, you'll need extra parens.)

> +        break;
> +    }
> +
> +    gBS->Stall (Dev->WaitForCmpStallInUsecs);
> +  }
> +
> +  //
> +  // Acknowledge PVSCSI_INTR_CMPL_MASK in device interrupt-status register
> +  //
> +  return PvScsiMmioWrite32 (
> +           Dev,
> +           PVSCSI_REG_OFFSET_INTR_STATUS,
> +           PVSCSI_INTR_CMPL_MASK
> +           );
> +}
> +
> +//
> +// Populate a PVSCSI request descriptor from the Extended SCSI Pass Thru
> +// Protocol packet.
> +//
> +STATIC
> +EFI_STATUS
> +PopulateRequest (
> +  IN CONST PVSCSI_DEV                               *Dev,
> +  IN UINT8                                          *Target,
> +  IN UINT64                                         Lun,
> +  IN OUT EFI_EXT_SCSI_PASS_THRU_SCSI_REQUEST_PACKET *Packet,
> +  OUT PVSCSI_RING_REQ_DESC                          *Request
> +  )
> +{
> +  UINT8 TargetValue;
> +
> +  //
> +  // We only use first byte of target identifer
> +  //
> +  TargetValue = *Target;
> +
> +  //
> +  // Check for unsupported requests
> +  //
> +  if (
> +      // Bidirectional transfer was requested

(8) Please prepend and append "empty" // lines.

Please rework the rest of the comments similarly. Edk2 permits //
comments without leading and trailing empty // lines only in case the //
comment is to the right of actual code on the same line, and the comment
applies to only that line.

> +      (Packet->InTransferLength > 0 && Packet->OutTransferLength > 0) ||
> +      (Packet->DataDirection == EFI_EXT_SCSI_DATA_DIRECTION_BIDIRECTIONAL) ||
> +      // Command Descriptor Block bigger than this constant should be considered
> +      // out-of-band. We currently don't support these CDBs.
> +      (Packet->CdbLength > PVSCSI_CDB_MAX_SIZE)
> +      ) {
> +
> +    //
> +    // This error code doesn't require updates to the Packet output fields
> +    //
> +    return EFI_UNSUPPORTED;
> +  }
> +
> +  //
> +  // Check for invalid parameters
> +  //
> +  if (
> +      // Addressed invalid device
> +      (TargetValue > Dev->MaxTarget) || (Lun > Dev->MaxLun) ||
> +      // Invalid direction (there doesn't seem to be a macro for the "no data
> +      // transferred" "direction", eg. for TEST UNIT READY)
> +      (Packet->DataDirection > EFI_EXT_SCSI_DATA_DIRECTION_BIDIRECTIONAL) ||
> +      // Trying to receive, but destination pointer is NULL, or contradicting
> +      // transfer direction
> +      ((Packet->InTransferLength > 0) &&
> +       ((Packet->InDataBuffer == NULL) ||
> +        (Packet->DataDirection == EFI_EXT_SCSI_DATA_DIRECTION_WRITE)
> +        )
> +       ) ||
> +
> +      //
> +      // Trying to send, but source pointer is NULL, or contradicting
> +      // transfer direction
> +      //
> +      ((Packet->OutTransferLength > 0) &&
> +       ((Packet->OutDataBuffer == NULL) ||
> +        (Packet->DataDirection == EFI_EXT_SCSI_DATA_DIRECTION_READ)
> +        )
> +       )
> +      ) {
> +
> +    //
> +    // This error code doesn't require updates to the Packet output fields
> +    //
> +    return EFI_INVALID_PARAMETER;
> +  }
> +
> +  //
> +  // Check for input/output buffer too large for DMA communication buffer
> +  //
> +  if (Packet->InTransferLength > sizeof (Dev->DmaBuf->Data)) {
> +    Packet->InTransferLength = sizeof (Dev->DmaBuf->Data);
> +    return EFI_BAD_BUFFER_SIZE;
> +  }
> +  if (Packet->OutTransferLength > sizeof (Dev->DmaBuf->Data)) {
> +    Packet->OutTransferLength = sizeof (Dev->DmaBuf->Data);
> +    return EFI_BAD_BUFFER_SIZE;
> +  }
> +
> +  //
> +  // Encode PVSCSI request
> +  //
> +  ZeroMem (Request, sizeof (*Request));
> +
> +  Request->Bus = 0;
> +  Request->Target = TargetValue;
> +  //
> +  // This cast is safe as MaxLun is defined as UINT8
> +  //
> +  Request->Lun[1] = (UINT8)Lun;
> +  Request->SenseLen = Packet->SenseDataLength;
> +  Request->SenseAddr = (UINT64)PVSCSI_DMA_BUF_DEV_ADDR (Dev, SenseData);
> +  Request->CdbLen = Packet->CdbLength;
> +  CopyMem (Request->Cdb, Packet->Cdb, Packet->CdbLength);
> +  Request->vCPUHint = 0;
> +  Request->Tag = PVSCSI_SIMPLE_QUEUE_TAG;
> +  if (Packet->DataDirection == EFI_EXT_SCSI_DATA_DIRECTION_READ) {
> +    Request->Flags = PVSCSI_FLAG_CMD_DIR_TOHOST;
> +    Request->DataLen = Packet->InTransferLength;
> +  } else {
> +    Request->Flags = PVSCSI_FLAG_CMD_DIR_TODEVICE;
> +    Request->DataLen = Packet->OutTransferLength;
> +    CopyMem (
> +      Dev->DmaBuf->Data,
> +      Packet->OutDataBuffer,
> +      Packet->OutTransferLength);

(9) Please pick one of the following styles:

    CopyMem (
      Dev->DmaBuf->Data,
      Packet->OutDataBuffer,
      Packet->OutTransferLength
      );

or

    CopyMem (Dev->DmaBuf->Data, Packet->OutDataBuffer,
      Packet->OutTransferLength);


> +  }
> +  Request->DataAddr = (UINT64)PVSCSI_DMA_BUF_DEV_ADDR (Dev, Data);
> +
> +  return EFI_SUCCESS;
> +}
> +
> +//
> +// Handle the PVSCSI device response:
> +// - Copy returned data from DMA communication buffer.
> +// - Update fields in Extended SCSI Pass Thru Protocol packet as required.
> +// - Translate response code to EFI status code and host adapter status.
> +//
> +STATIC
> +EFI_STATUS
> +HandleResponse (
> +  IN PVSCSI_DEV                                     *Dev,
> +  IN OUT EFI_EXT_SCSI_PASS_THRU_SCSI_REQUEST_PACKET *Packet,
> +  IN CONST PVSCSI_RING_CMP_DESC                     *Response
> +  )
> +{
> +  //
> +  // Check if device returned sense data
> +  //
> +  if (Response->ScsiStatus == EFI_EXT_SCSI_STATUS_TARGET_CHECK_CONDITION) {
> +    //
> +    // Fix SenseDataLength to amount of data returned
> +    //
> +    if (Packet->SenseDataLength > Response->SenseLen) {
> +      Packet->SenseDataLength = (UINT8)Response->SenseLen;
> +    }
> +    //
> +    // Copy sense data from DMA communication buffer
> +    //
> +    CopyMem (
> +      Packet->SenseData,
> +      Dev->DmaBuf->SenseData,
> +      Packet->SenseDataLength
> +      );
> +  } else {
> +    //
> +    // Signal no sense data returned
> +    //
> +    Packet->SenseDataLength = 0;
> +  }
> +
> +  //
> +  // Copy device output from DMA communication buffer
> +  //
> +  if (Packet->DataDirection == EFI_EXT_SCSI_DATA_DIRECTION_READ) {
> +    CopyMem (Packet->InDataBuffer, Dev->DmaBuf->Data, Packet->InTransferLength);
> +  }
> +
> +  //
> +  // Report target status
> +  //
> +  Packet->TargetStatus = Response->ScsiStatus;
> +
> +  //
> +  // Host adapter status and function return value depend on
> +  // device response's host status
> +  //
> +  switch (Response->HostStatus) {
> +    case BTSTAT_SUCCESS:
> +    case BTSTAT_LINKED_COMMAND_COMPLETED:
> +    case BTSTAT_LINKED_COMMAND_COMPLETED_WITH_FLAG:
> +      Packet->HostAdapterStatus = EFI_EXT_SCSI_STATUS_HOST_ADAPTER_OK;
> +      return EFI_SUCCESS;
> +
> +    case BTSTAT_SELTIMEO:
> +      Packet->HostAdapterStatus =
> +                EFI_EXT_SCSI_STATUS_HOST_ADAPTER_SELECTION_TIMEOUT;
> +      return EFI_TIMEOUT;
> +
> +    case BTSTAT_DATARUN:
> +    case BTSTAT_DATA_UNDERRUN:
> +      //
> +      // Report residual data in overrun/underrun
> +      //
> +      if (Packet->DataDirection == EFI_EXT_SCSI_DATA_DIRECTION_READ) {
> +        Packet->InTransferLength = Response->DataLen;
> +      } else {
> +        Packet->OutTransferLength = Response->DataLen;
> +      }
> +      Packet->HostAdapterStatus =
> +                EFI_EXT_SCSI_STATUS_HOST_ADAPTER_DATA_OVERRUN_UNDERRUN;
> +      return EFI_BAD_BUFFER_SIZE;
> +
> +    case BTSTAT_BUSFREE:
> +      Packet->HostAdapterStatus = EFI_EXT_SCSI_STATUS_HOST_ADAPTER_BUS_FREE;
> +      break;
> +
> +    case BTSTAT_INVPHASE:
> +      Packet->HostAdapterStatus = EFI_EXT_SCSI_STATUS_HOST_ADAPTER_PHASE_ERROR;
> +      break;
> +
> +    case BTSTAT_SENSFAILED:
> +      Packet->HostAdapterStatus =
> +                EFI_EXT_SCSI_STATUS_HOST_ADAPTER_REQUEST_SENSE_FAILED;
> +      break;
> +
> +    case BTSTAT_TAGREJECT:
> +    case BTSTAT_BADMSG:
> +      Packet->HostAdapterStatus =
> +          EFI_EXT_SCSI_STATUS_HOST_ADAPTER_MESSAGE_REJECT;
> +      break;
> +
> +    case BTSTAT_BUSRESET:
> +      Packet->HostAdapterStatus = EFI_EXT_SCSI_STATUS_HOST_ADAPTER_BUS_RESET;
> +      break;
> +
> +    case BTSTAT_HATIMEOUT:
> +      Packet->HostAdapterStatus = EFI_EXT_SCSI_STATUS_HOST_ADAPTER_TIMEOUT;
> +      return EFI_TIMEOUT;
> +
> +    case BTSTAT_SCSIPARITY:
> +      Packet->HostAdapterStatus = EFI_EXT_SCSI_STATUS_HOST_ADAPTER_PARITY_ERROR;
> +      break;
> +
> +    default:
> +      Packet->HostAdapterStatus = EFI_EXT_SCSI_STATUS_HOST_ADAPTER_OTHER;
> +      break;
> +  }
> +
> +  return EFI_DEVICE_ERROR;
> +}
> +
>  //
>  // Check if Target argument to EXT_SCSI_PASS_THRU.GetNextTarget() and
>  // EXT_SCSI_PASS_THRU.GetNextTargetLun() is initialized
> @@ -135,7 +492,70 @@ PvScsiPassThru (
>    IN EFI_EVENT                                      Event    OPTIONAL
>    )
>  {
> -  return EFI_UNSUPPORTED;
> +  PVSCSI_DEV            *Dev;
> +  EFI_STATUS            Status;
> +  PVSCSI_RING_REQ_DESC *Request;
> +  PVSCSI_RING_CMP_DESC *Response;
> +
> +  Dev = PVSCSI_FROM_PASS_THRU (This);
> +
> +  if (PvScsiIsReqRingFull (Dev)) {
> +    return EFI_NOT_READY;
> +  }
> +
> +  Request = PvScsiGetCurrentRequest (Dev);
> +
> +  Status = PopulateRequest (Dev, Target, Lun, Packet, Request);
> +  if (EFI_ERROR (Status)) {
> +    return Status;
> +  }
> +
> +  //
> +  // Writes to Request must be globally visible before making request
> +  // available to device
> +  //
> +  MemoryFence();

(10) Missing space before the opening paren.

Please check the rest of the code for the same style issue.


> +  Dev->RingDesc.RingState->ReqProdIdx++;
> +
> +  Status = PvScsiMmioWrite32 (Dev, PVSCSI_REG_OFFSET_KICK_RW_IO, 0);
> +  if (EFI_ERROR (Status)) {
> +    //
> +    // If kicking the host fails, we must fake a host adapter error.
> +    // EFI_NOT_READY would save us the effort, but it would also suggest that
> +    // the caller retry.
> +    //
> +    goto FakeHostAdapterError;
> +  }

(11) Hmmm. Not really happy about this. It doesn't feel like actual
error handling (= resource release / rollback); we're just factoring out
response composition. That's OK per se, but then it belongs to a helper
function, not a "function epilogue" here.

> +
> +  Status = PvScsiWaitForRequestCompletion (Dev);
> +  if (EFI_ERROR (Status)) {
> +    //
> +    // If waiting for request completion fails, we must fake a host adapter
> +    // error. EFI_NOT_READY would save us the effort, but it would also suggest
> +    // that the caller retry.
> +    //
> +    goto FakeHostAdapterError;
> +  }
> +
> +  Response = PvScsiGetCurrentResponse (Dev);
> +  Status = HandleResponse (Dev, Packet, Response);
> +
> +  //
> +  // Reads from response must complete before releasing completion entry
> +  // to device
> +  //
> +  MemoryFence();
> +  Dev->RingDesc.RingState->CmpConsIdx++;
> +
> +  return Status;
> +
> +FakeHostAdapterError:
> +    Packet->InTransferLength = 0;
> +    Packet->OutTransferLength = 0;
> +    Packet->SenseDataLength = 0;
> +    Packet->HostAdapterStatus = EFI_EXT_SCSI_STATUS_HOST_ADAPTER_OTHER;
> +    Packet->TargetStatus = EFI_EXT_SCSI_STATUS_TARGET_GOOD;
> +    return EFI_DEVICE_ERROR;

(12) also, wrong indentation.

Thanks
Laszlo

>  }
>  
>  STATIC
> @@ -613,6 +1033,7 @@ PvScsiInit (
>    //
>    Dev->MaxTarget = PcdGet8 (PcdPvScsiMaxTargetLimit);
>    Dev->MaxLun = PcdGet8 (PcdPvScsiMaxLunLimit);
> +  Dev->WaitForCmpStallInUsecs = PcdGet32 (PcdPvScsiWaitForCmpStallInUsecs);
>  
>    //
>    // Set PCI Attributes
> diff --git a/OvmfPkg/PvScsiDxe/PvScsi.h b/OvmfPkg/PvScsiDxe/PvScsi.h
> index 7f91d70fec79..08e876b75930 100644
> --- a/OvmfPkg/PvScsiDxe/PvScsi.h
> +++ b/OvmfPkg/PvScsiDxe/PvScsi.h
> @@ -47,6 +47,7 @@ typedef struct {
>    PVSCSI_DMA_DESC                 DmaBufDmaDesc;
>    UINT8                           MaxTarget;
>    UINT8                           MaxLun;
> +  UINTN                           WaitForCmpStallInUsecs;
>    EFI_EXT_SCSI_PASS_THRU_PROTOCOL PassThru;
>    EFI_EXT_SCSI_PASS_THRU_MODE     PassThruMode;
>  } PVSCSI_DEV;
> diff --git a/OvmfPkg/PvScsiDxe/PvScsi.inf b/OvmfPkg/PvScsiDxe/PvScsi.inf
> index 96bd4e4a9a8b..e3a85eba8dac 100644
> --- a/OvmfPkg/PvScsiDxe/PvScsi.inf
> +++ b/OvmfPkg/PvScsiDxe/PvScsi.inf
> @@ -37,5 +37,6 @@
>    gEfiExtScsiPassThruProtocolGuid   ## BY_START
>  
>  [Pcd]
> -  gUefiOvmfPkgTokenSpaceGuid.PcdPvScsiMaxTargetLimit    ## CONSUMES
> -  gUefiOvmfPkgTokenSpaceGuid.PcdPvScsiMaxLunLimit       ## CONSUMES
> +  gUefiOvmfPkgTokenSpaceGuid.PcdPvScsiMaxTargetLimit            ## CONSUMES
> +  gUefiOvmfPkgTokenSpaceGuid.PcdPvScsiMaxLunLimit               ## CONSUMES
> +  gUefiOvmfPkgTokenSpaceGuid.PcdPvScsiWaitForCmpStallInUsecs    ## CONSUMES
>