Re: [PATCH v2 06/12] OvmfPkg/LsiScsiDxe: Report Targets and LUNs

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

On 07/16/20 09:46, 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.
> 
> 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      |   3 +
>  OvmfPkg/LsiScsiDxe/LsiScsiDxe.inf |   6 ++
>  OvmfPkg/OvmfPkg.dec               |   5 +
>  4 files changed, 160 insertions(+), 2 deletions(-)
> 
> diff --git a/OvmfPkg/LsiScsiDxe/LsiScsi.c b/OvmfPkg/LsiScsiDxe/LsiScsi.c
> index 67fadb411e85..989bda88d209 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..128]"
> +    );

(1) The condition on the LUN limit, and the error message for the same,
are not consistent. The PCD is an inclusive limit. We assert that the
PCD is strictly smaller than 128. Therefore the highest permitted PCD
value is 127. Therefore the highest permitted LUN (because the PCD is
inclusive) is 127. Therefore the error message should advertize
"[0..127]", not "[0..128]". In other words, LUN 128 itself is not valid.

If LUN 128 *is* valid, and so PCD=128 too is valid, then the condition
should be updated, to say "< 129".

> +  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 c194dfbf3347..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;
> @@ -23,6 +25,7 @@ typedef struct {
>  #define LSI_SCSI_FROM_PASS_THRU(PassThruPtr) \
>    CR (PassThruPtr, LSI_SCSI_DEV, PassThru, LSI_SCSI_DEV_SIGNATURE)
>  
> +
>  //
>  // Probe, start and stop functions of this driver, called by the DXE core for
>  // specific devices.

(2) You missed point (4) in my v1 review -- the above newline is
spurious, so it should be removed from this patch. (It should either be
removed completely, or squashed into the patch that introduces the
LSI_SCSI_FROM_PASS_THRU macro.)

With (1) and (2) fixed:

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

Thanks
Laszlo

> 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
>