Re: [PATCH v3 06/11] OvmfPkg/LsiScsiDxe: Report Targets and LUNs

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

On 07/17/20 08:11, Gary Lin wrote:
> Implement LsiScsiGetNextTargetLun(), LsiScsiBuildDevicePath(),
> LsiScsiGetTargetLun(), and LsiScsiGetNextTarget() to report Targets and
> LUNs and build the device path.
> 
> This commit also introduces two PCD value: PcdLsiScsiMaxTargetLimit and
> PcdLsiScsiMaxLunLimit as the limits for Targets and LUNs.
> 
> v3:
>   - Update the range of LUN in the assertioin
>   - Squash the spurious newline into the previous commit
> v2:
>   - Zero out (*Target) in LsiScsiGetTargetLun()
>   - Use CopyMem() instead of the one-byte shortcut to copy target from
>     ScsiDevicePath->Pun
>   - Add asserts for PcdLsiScsiMaxTargetLimit and PcdLsiScsiMaxLunLimit
> 
> 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/LsiScsiDxe/LsiScsi.c      | 148 +++++++++++++++++++++++++++++-
>  OvmfPkg/LsiScsiDxe/LsiScsi.h      |   2 +
>  OvmfPkg/LsiScsiDxe/LsiScsiDxe.inf |   6 ++
>  OvmfPkg/OvmfPkg.dec               |   5 +
>  4 files changed, 159 insertions(+), 2 deletions(-)

Reviewed-by: Laszlo Ersek <lersek@redhat.com>

Thanks,
Laszlo

> 
> diff --git a/OvmfPkg/LsiScsiDxe/LsiScsi.c b/OvmfPkg/LsiScsiDxe/LsiScsi.c
> index 67fadb411e85..172779240636 100644
> --- a/OvmfPkg/LsiScsiDxe/LsiScsi.c
> +++ b/OvmfPkg/LsiScsiDxe/LsiScsi.c
> @@ -15,6 +15,7 @@
>  #include <Library/BaseMemoryLib.h>
>  #include <Library/DebugLib.h>
>  #include <Library/MemoryAllocationLib.h>
> +#include <Library/PcdLib.h>
>  #include <Library/UefiBootServicesTableLib.h>
>  #include <Library/UefiLib.h>
>  #include <Protocol/PciIo.h>
> @@ -53,6 +54,49 @@ LsiScsiGetNextTargetLun (
>    IN OUT UINT64                      *Lun
>    )
>  {
> +  LSI_SCSI_DEV *Dev;
> +  UINTN        Idx;
> +  UINT8        *Target;
> +  UINT16       LastTarget;
> +
> +  //
> +  // the TargetPointer input parameter is unnecessarily a pointer-to-pointer
> +  //
> +  Target = *TargetPointer;
> +
> +  //
> +  // Search for first non-0xFF byte. If not found, return first target & LUN.
> +  //
> +  for (Idx = 0; Idx < TARGET_MAX_BYTES && Target[Idx] == 0xFF; ++Idx)
> +    ;
> +  if (Idx == TARGET_MAX_BYTES) {
> +    SetMem (Target, TARGET_MAX_BYTES, 0x00);
> +    *Lun = 0;
> +    return EFI_SUCCESS;
> +  }
> +
> +  CopyMem (&LastTarget, Target, sizeof LastTarget);
> +
> +  //
> +  // increment (target, LUN) pair if valid on input
> +  //
> +  Dev = LSI_SCSI_FROM_PASS_THRU (This);
> +  if (LastTarget > Dev->MaxTarget || *Lun > Dev->MaxLun) {
> +    return EFI_INVALID_PARAMETER;
> +  }
> +
> +  if (*Lun < Dev->MaxLun) {
> +    ++*Lun;
> +    return EFI_SUCCESS;
> +  }
> +
> +  if (LastTarget < Dev->MaxTarget) {
> +    *Lun = 0;
> +    ++LastTarget;
> +    CopyMem (Target, &LastTarget, sizeof LastTarget);
> +    return EFI_SUCCESS;
> +  }
> +
>    return EFI_NOT_FOUND;
>  }
>  
> @@ -65,7 +109,34 @@ LsiScsiBuildDevicePath (
>    IN OUT EFI_DEVICE_PATH_PROTOCOL    **DevicePath
>    )
>  {
> -  return EFI_NOT_FOUND;
> +  UINT16           TargetValue;
> +  LSI_SCSI_DEV     *Dev;
> +  SCSI_DEVICE_PATH *ScsiDevicePath;
> +
> +  if (DevicePath == NULL) {
> +    return EFI_INVALID_PARAMETER;
> +  }
> +
> +  CopyMem (&TargetValue, Target, sizeof TargetValue);
> +  Dev = LSI_SCSI_FROM_PASS_THRU (This);
> +  if (TargetValue > Dev->MaxTarget || Lun > Dev->MaxLun || Lun > 0xFFFF) {
> +    return EFI_NOT_FOUND;
> +  }
> +
> +  ScsiDevicePath = AllocatePool (sizeof *ScsiDevicePath);
> +  if (ScsiDevicePath == NULL) {
> +    return EFI_OUT_OF_RESOURCES;
> +  }
> +
> +  ScsiDevicePath->Header.Type      = MESSAGING_DEVICE_PATH;
> +  ScsiDevicePath->Header.SubType   = MSG_SCSI_DP;
> +  ScsiDevicePath->Header.Length[0] = (UINT8)  sizeof *ScsiDevicePath;
> +  ScsiDevicePath->Header.Length[1] = (UINT8) (sizeof *ScsiDevicePath >> 8);
> +  ScsiDevicePath->Pun              = TargetValue;
> +  ScsiDevicePath->Lun              = (UINT16) Lun;
> +
> +  *DevicePath = &ScsiDevicePath->Header;
> +  return EFI_SUCCESS;
>  }
>  
>  EFI_STATUS
> @@ -77,7 +148,33 @@ LsiScsiGetTargetLun (
>    OUT UINT64                          *Lun
>    )
>  {
> -  return EFI_UNSUPPORTED;
> +  SCSI_DEVICE_PATH *ScsiDevicePath;
> +  LSI_SCSI_DEV     *Dev;
> +  UINT8            *Target;
> +
> +  if (DevicePath == NULL || TargetPointer == NULL || *TargetPointer == NULL ||
> +      Lun == NULL) {
> +    return EFI_INVALID_PARAMETER;
> +  }
> +
> +  if (DevicePath->Type    != MESSAGING_DEVICE_PATH ||
> +      DevicePath->SubType != MSG_SCSI_DP) {
> +    return EFI_UNSUPPORTED;
> +  }
> +
> +  ScsiDevicePath = (SCSI_DEVICE_PATH *) DevicePath;
> +  Dev = LSI_SCSI_FROM_PASS_THRU (This);
> +  if (ScsiDevicePath->Pun > Dev->MaxTarget ||
> +      ScsiDevicePath->Lun > Dev->MaxLun) {
> +    return EFI_NOT_FOUND;
> +  }
> +
> +  Target = *TargetPointer;
> +  ZeroMem (Target, TARGET_MAX_BYTES);
> +  CopyMem (Target, &ScsiDevicePath->Pun, sizeof ScsiDevicePath->Pun);
> +  *Lun = ScsiDevicePath->Lun;
> +
> +  return EFI_SUCCESS;
>  }
>  
>  EFI_STATUS
> @@ -107,6 +204,42 @@ LsiScsiGetNextTarget (
>    IN OUT UINT8                       **TargetPointer
>    )
>  {
> +  LSI_SCSI_DEV *Dev;
> +  UINTN        Idx;
> +  UINT8        *Target;
> +  UINT16       LastTarget;
> +
> +  //
> +  // the TargetPointer input parameter is unnecessarily a pointer-to-pointer
> +  //
> +  Target = *TargetPointer;
> +
> +  //
> +  // Search for first non-0xFF byte. If not found, return first target.
> +  //
> +  for (Idx = 0; Idx < TARGET_MAX_BYTES && Target[Idx] == 0xFF; ++Idx)
> +    ;
> +  if (Idx == TARGET_MAX_BYTES) {
> +    SetMem (Target, TARGET_MAX_BYTES, 0x00);
> +    return EFI_SUCCESS;
> +  }
> +
> +  CopyMem (&LastTarget, Target, sizeof LastTarget);
> +
> +  //
> +  // increment target if valid on input
> +  //
> +  Dev = LSI_SCSI_FROM_PASS_THRU (This);
> +  if (LastTarget > Dev->MaxTarget) {
> +    return EFI_INVALID_PARAMETER;
> +  }
> +
> +  if (LastTarget < Dev->MaxTarget) {
> +    ++LastTarget;
> +    CopyMem (Target, &LastTarget, sizeof LastTarget);
> +    return EFI_SUCCESS;
> +  }
> +
>    return EFI_NOT_FOUND;
>  }
>  
> @@ -189,6 +322,17 @@ LsiScsiControllerStart (
>  
>    Dev->Signature = LSI_SCSI_DEV_SIGNATURE;
>  
> +  STATIC_ASSERT (
> +    FixedPcdGet8 (PcdLsiScsiMaxTargetLimit) < 8,
> +    "LSI 53C895A supports targets [0..7]"
> +    );
> +  STATIC_ASSERT (
> +    FixedPcdGet8 (PcdLsiScsiMaxLunLimit) < 128,
> +    "LSI 53C895A supports LUNs [0..127]"
> +    );
> +  Dev->MaxTarget = PcdGet8 (PcdLsiScsiMaxTargetLimit);
> +  Dev->MaxLun = PcdGet8 (PcdLsiScsiMaxLunLimit);
> +
>    //
>    // Host adapter channel, doesn't exist
>    //
> diff --git a/OvmfPkg/LsiScsiDxe/LsiScsi.h b/OvmfPkg/LsiScsiDxe/LsiScsi.h
> index 751d5b193b51..6c6ed25f1c33 100644
> --- a/OvmfPkg/LsiScsiDxe/LsiScsi.h
> +++ b/OvmfPkg/LsiScsiDxe/LsiScsi.h
> @@ -14,6 +14,8 @@
>  
>  typedef struct {
>    UINT32                          Signature;
> +  UINT8                           MaxTarget;
> +  UINT8                           MaxLun;
>    EFI_EXT_SCSI_PASS_THRU_MODE     PassThruMode;
>    EFI_EXT_SCSI_PASS_THRU_PROTOCOL PassThru;
>  } LSI_SCSI_DEV;
> diff --git a/OvmfPkg/LsiScsiDxe/LsiScsiDxe.inf b/OvmfPkg/LsiScsiDxe/LsiScsiDxe.inf
> index 14f9c68308cc..6111449577a8 100644
> --- a/OvmfPkg/LsiScsiDxe/LsiScsiDxe.inf
> +++ b/OvmfPkg/LsiScsiDxe/LsiScsiDxe.inf
> @@ -27,7 +27,9 @@ [Packages]
>  [LibraryClasses]
>    BaseLib
>    BaseMemoryLib
> +  DebugLib
>    MemoryAllocationLib
> +  PcdLib
>    UefiBootServicesTableLib
>    UefiDriverEntryPoint
>    UefiLib
> @@ -35,3 +37,7 @@ [LibraryClasses]
>  [Protocols]
>    gEfiExtScsiPassThruProtocolGuid        ## BY_START
>    gEfiPciIoProtocolGuid                  ## TO_START
> +
> +[FixedPcd]
> +  gUefiOvmfPkgTokenSpaceGuid.PcdLsiScsiMaxTargetLimit   ## CONSUMES
> +  gUefiOvmfPkgTokenSpaceGuid.PcdLsiScsiMaxLunLimit      ## CONSUMES
> diff --git a/OvmfPkg/OvmfPkg.dec b/OvmfPkg/OvmfPkg.dec
> index 2b0f137cbcce..ca13a3cb11d3 100644
> --- a/OvmfPkg/OvmfPkg.dec
> +++ b/OvmfPkg/OvmfPkg.dec
> @@ -174,6 +174,11 @@ [PcdsFixedAtBuild]
>    ## Microseconds to stall between polling for MptScsi request result
>    gUefiOvmfPkgTokenSpaceGuid.PcdMptScsiStallPerPollUsec|5|UINT32|0x3a
>  
> +  ## Set the *inclusive* number of targets and LUNs that LsiScsi exposes for
> +  #  scan by ScsiBusDxe.
> +  gUefiOvmfPkgTokenSpaceGuid.PcdLsiScsiMaxTargetLimit|7|UINT8|0x3b
> +  gUefiOvmfPkgTokenSpaceGuid.PcdLsiScsiMaxLunLimit|0|UINT8|0x3c
> +
>    gUefiOvmfPkgTokenSpaceGuid.PcdOvmfFlashNvStorageEventLogBase|0x0|UINT32|0x8
>    gUefiOvmfPkgTokenSpaceGuid.PcdOvmfFlashNvStorageEventLogSize|0x0|UINT32|0x9
>    gUefiOvmfPkgTokenSpaceGuid.PcdOvmfFirmwareFdSize|0x0|UINT32|0xa
>