Re: [edk2-devel] [PATCH v5 06/12] OvmfPkg/MptScsiDxe: Report targets and one LUN

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

On 04/24/20 19:59, Nikita Leshenko wrote:
> The controller supports up to 8 targets in practice (Not reported by the
> controller, but based on the implementation of the virtual device),
> report them in GetNextTarget and GetNextTargetLun. The firmware will
> then try to communicate with them and create a block device for each one
> that responds.
> 
> Support for multiple LUNs will be implemented in another series.
> 
> Ref: https://bugzilla.tianocore.org/show_bug.cgi?id=2390
> Signed-off-by: Nikita Leshenko <nikita.leshchenko@oracle.com>
> ---
>  OvmfPkg/MptScsiDxe/MptScsi.c      | 62 ++++++++++++++++++++++++++++++-
>  OvmfPkg/MptScsiDxe/MptScsiDxe.inf |  4 ++
>  OvmfPkg/OvmfPkg.dec               |  4 ++
>  3 files changed, 68 insertions(+), 2 deletions(-)
> 
> diff --git a/OvmfPkg/MptScsiDxe/MptScsi.c b/OvmfPkg/MptScsiDxe/MptScsi.c
> index 40d392c2346f..5e0544c8d9d2 100644
> --- a/OvmfPkg/MptScsiDxe/MptScsi.c
> +++ b/OvmfPkg/MptScsiDxe/MptScsi.c
> @@ -11,6 +11,7 @@
>  
>  #include <IndustryStandard/FusionMptScsi.h>
>  #include <IndustryStandard/Pci.h>
> +#include <Library/BaseMemoryLib.h>
>  #include <Library/DebugLib.h>
>  #include <Library/MemoryAllocationLib.h>
>  #include <Library/UefiBootServicesTableLib.h>
> @@ -34,6 +35,7 @@ typedef struct {
>    UINT32                          Signature;
>    EFI_EXT_SCSI_PASS_THRU_PROTOCOL PassThru;
>    EFI_EXT_SCSI_PASS_THRU_MODE     PassThruMode;
> +  UINT8                           MaxTarget;
>  } MPT_SCSI_DEV;
>  
>  #define MPT_SCSI_FROM_PASS_THRU(PassThruPtr) \
> @@ -57,6 +59,22 @@ MptScsiPassThru (
>    return EFI_UNSUPPORTED;
>  }
>  
> +STATIC
> +BOOLEAN
> +IsTargetInitialized (
> +  IN UINT8                                          *Target
> +  )
> +{
> +  UINTN Idx;
> +
> +  for (Idx = 0; Idx < TARGET_MAX_BYTES; ++Idx) {
> +    if (Target[Idx] != 0xFF) {
> +      return TRUE;
> +    }
> +  }
> +  return FALSE;
> +}
> +
>  STATIC
>  EFI_STATUS
>  EFIAPI
> @@ -66,7 +84,28 @@ MptScsiGetNextTargetLun (
>    IN OUT UINT64                                     *Lun
>    )
>  {
> -  return EFI_UNSUPPORTED;
> +  MPT_SCSI_DEV *Dev;
> +
> +  Dev = MPT_SCSI_FROM_PASS_THRU (This);
> +  //
> +  // Currently support only LUN 0, so hardcode it
> +  //
> +  if (!IsTargetInitialized (*Target)) {
> +    ZeroMem (*Target, TARGET_MAX_BYTES);
> +    *Lun = 0;
> +  } else if (**Target > Dev->MaxTarget || *Lun > 0) {
> +    return EFI_INVALID_PARAMETER;
> +  } else if (**Target < Dev->MaxTarget) {
> +    //
> +    // This device interface support 256 targets only, so it's enough to
> +    // increment the LSB of Target, as it will never overflow.
> +    //
> +    **Target += 1;
> +  } else {
> +    return EFI_NOT_FOUND;
> +  }
> +
> +  return EFI_SUCCESS;
>  }
>  
>  STATIC
> @@ -77,7 +116,24 @@ MptScsiGetNextTarget (
>    IN OUT UINT8                                     **Target
>    )
>  {
> -  return EFI_UNSUPPORTED;
> +  MPT_SCSI_DEV *Dev;
> +
> +  Dev = MPT_SCSI_FROM_PASS_THRU (This);
> +  if (!IsTargetInitialized (*Target)) {
> +    ZeroMem (*Target, TARGET_MAX_BYTES);
> +  } else if (**Target > Dev->MaxTarget) {
> +    return EFI_INVALID_PARAMETER;
> +  } else if (**Target < Dev->MaxTarget) {
> +    //
> +    // This device interface support 256 targets only, so it's enough to
> +    // increment the LSB of Target, as it will never overflow.
> +    //
> +    **Target += 1;
> +  } else {
> +    return EFI_NOT_FOUND;
> +  }
> +
> +  return EFI_SUCCESS;
>  }
>  
>  STATIC
> @@ -206,6 +262,8 @@ MptScsiControllerStart (
>  
>    Dev->Signature = MPT_SCSI_DEV_SIGNATURE;
>  
> +  Dev->MaxTarget = PcdGet8 (PcdMptScsiMaxTargetLimit);
> +
>    //
>    // Host adapter channel, doesn't exist
>    //
> diff --git a/OvmfPkg/MptScsiDxe/MptScsiDxe.inf b/OvmfPkg/MptScsiDxe/MptScsiDxe.inf
> index 9f7c98829ee1..4862ff9dd497 100644
> --- a/OvmfPkg/MptScsiDxe/MptScsiDxe.inf
> +++ b/OvmfPkg/MptScsiDxe/MptScsiDxe.inf
> @@ -24,6 +24,7 @@ [Packages]
>    OvmfPkg/OvmfPkg.dec
>  
>  [LibraryClasses]
> +  BaseMemoryLib
>    DebugLib
>    MemoryAllocationLib
>    UefiBootServicesTableLib
> @@ -33,3 +34,6 @@ [LibraryClasses]
>  [Protocols]
>    gEfiExtScsiPassThruProtocolGuid        ## BY_START
>    gEfiPciIoProtocolGuid                  ## TO_START
> +
> +[FixedPcd]
> +  gUefiOvmfPkgTokenSpaceGuid.PcdMptScsiMaxTargetLimit   ## CONSUMES
> diff --git a/OvmfPkg/OvmfPkg.dec b/OvmfPkg/OvmfPkg.dec
> index 28030391cff2..2d09444bbb16 100644
> --- a/OvmfPkg/OvmfPkg.dec
> +++ b/OvmfPkg/OvmfPkg.dec
> @@ -163,6 +163,10 @@ [PcdsFixedAtBuild]
>    #  polling loop iteration.
>    gUefiOvmfPkgTokenSpaceGuid.PcdPvScsiWaitForCmpStallInUsecs|5|UINT32|0x38
>  
> +  ## Set the *inclusive* number of targets that MptScsi exposes for scan
> +  #  by ScsiBusDxe.
> +  gUefiOvmfPkgTokenSpaceGuid.PcdMptScsiMaxTargetLimit|7|UINT8|0x39
> +
>    gUefiOvmfPkgTokenSpaceGuid.PcdOvmfFlashNvStorageEventLogBase|0x0|UINT32|0x8
>    gUefiOvmfPkgTokenSpaceGuid.PcdOvmfFlashNvStorageEventLogSize|0x0|UINT32|0x9
>    gUefiOvmfPkgTokenSpaceGuid.PcdOvmfFirmwareFdSize|0x0|UINT32|0xa
> 

(1) <PcdLib.h> should be #include'd in this patch, and PcdLib should be
listed under [LibraryClasses].

On one hand, that's a minor wart. The driver (and the OVMF platform(s))
build OK at this stage, in practice. (I tested that.) So this, per se,
does not justify a v6.

On the other hand, even at the end of the series, the module does not
spell out the PcdLib dependency (neither #include nor [LibraryClasses]).
That's not so nice.

But, I'll fix that up for you, if there's not going to be another reason
for a v6.

With (1) addressed (by you, or by me):

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

Thanks,
Laszlo