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

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

On 07/01/20 06:04, 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.
> 
> 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      | 143 +++++++++++++++++++++++++++++-
>  OvmfPkg/LsiScsiDxe/LsiScsi.h      |   3 +
>  OvmfPkg/LsiScsiDxe/LsiScsiDxe.inf |   6 ++
>  OvmfPkg/OvmfPkg.dec               |   5 ++
>  4 files changed, 155 insertions(+), 2 deletions(-)
> 
> diff --git a/OvmfPkg/LsiScsiDxe/LsiScsi.c b/OvmfPkg/LsiScsiDxe/LsiScsi.c
> index f633c6793298..e10a81a5f9f7 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,36 @@ 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;
> +  }
> +
> +  //
> +  // This device support 8 targets only, so it's enough to set the LSB
> +  // of Target.
> +  //
> +  Target = *TargetPointer;
> +  *Target = (UINT8)ScsiDevicePath->Pun;

I don't like this shortcut.

The first reason is that the shortcut is not consistent with the rest of
the functions implemented in this patch. The other functions handle
target numbers that extend up to two bytes; there are no single-byte
shortcuts in them.

(1) So please either rewrite all of those functions to use a similar
shortcut like here, or please remove this shortcut, and stick with a
2-byte sized CopyMem() here too. I prefer the latter.

(2) Whether you use the single-byte shortcut or not, please zero out the
bytes in (*Target) that you do not set otherwise (from Pun).

(3) If the device supports only 8 targets, then please add an according
STATIC_ASSERT() on PcdLsiScsiMaxTargetLimit somewhere. For example:

  STATIC_ASSERT (
    FixedPcdGet8 (PcdLsiScsiMaxTargetLimit) < 8,
    "LSI 53C895A supports targets [0..7]"
    );

It's probably best to add the assert in LsiScsiControllerStart() below,
where you fetch the PCD anyway.

> +  *Lun = ScsiDevicePath->Lun;
> +
> +  return EFI_SUCCESS;
>  }
>  
>  EFI_STATUS
> @@ -107,6 +207,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 +325,9 @@ LsiScsiControllerStart (
>  
>    Dev->Signature = LSI_SCSI_DEV_SIGNATURE;
>  
> +  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 fca1007f9b98..a3d51d8f2386 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.

(4) Spurious newline, please drop it. (Or squash it in the previous
patch -- for example --, where you define the LSI_SCSI_FROM_PASS_THRU
macro.)

> diff --git a/OvmfPkg/LsiScsiDxe/LsiScsiDxe.inf b/OvmfPkg/LsiScsiDxe/LsiScsiDxe.inf
> index 8660513e2ffd..68844c6772e3 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 65bb2bb0eb4c..ae7d1d648d22 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|0x40
>  
> +  ## 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
> 

Commit 093cceaf79b5 ("OvmfPkg/MptScsiDxe: Report targets and one LUN",
2020-05-05) introduced PcdMptScsiMaxTargetLimit with token value 0x39.
That was OK.

Commit 505812ae1d2d ("OvmfPkg/MptScsiDxe: Implement the PassThru
method", 2020-05-05) then introduced PcdMptScsiStallPerPollUsec with
token value 0x40. That was not OK -- but I missed it. (The token value
should have been 0x3a; as 0x39 is not succeeded by 0x40 but by 0x3a.)

Token value 0x3a remains unused at this point.

(5) So please introduce the new PCDs with token values 0x3a and 0x3b, to
keep the token space dense.

Thanks
Laszlo