Re: [edk2-devel] [PATCH 06/17] OvmfPkg/PvScsiDxe: Report the number of targets and LUNs

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

On 03/16/20 16:01, Liran Alon wrote:
> Implement EXT_SCSI_PASS_THRU.GetNextTarget() and
> EXT_SCSI_PASS_THRU.GetNextTargetLun().
> 
> ScsiBusDxe scans all MaxTarget * MaxLun possible devices.
> This can take unnecessarily long for large number of targets.
> To deal with this, VirtioScsiDxe has defined PCDs to limit the
> MaxTarget & MaxLun to desired values which gives sufficient
> performance. It is very important in virtio-scsi as it can have
> very big MaxTarget & MaxLun.
> Even though a common PVSCSI device has a default MaxTarget=64 and
> MaxLun=0, we implement similar mechanism as virtio-scsi for completeness.
> This may be useful in the future when PVSCSI will have bigger values
> for MaxTarget and MaxLun.
> 
> 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          |   9 +++
>  OvmfPkg/PvScsiDxe/PvScsi.c   | 122 ++++++++++++++++++++++++++++++++++-
>  OvmfPkg/PvScsiDxe/PvScsi.h   |   2 +
>  OvmfPkg/PvScsiDxe/PvScsi.inf |   5 ++
>  4 files changed, 136 insertions(+), 2 deletions(-)
> 
> diff --git a/OvmfPkg/OvmfPkg.dec b/OvmfPkg/OvmfPkg.dec
> index 4c5b6511cb97..76ce507e8bd0 100644
> --- a/OvmfPkg/OvmfPkg.dec
> +++ b/OvmfPkg/OvmfPkg.dec
> @@ -121,6 +121,15 @@
>    gUefiOvmfPkgTokenSpaceGuid.PcdVirtioScsiMaxTargetLimit|31|UINT16|6
>    gUefiOvmfPkgTokenSpaceGuid.PcdVirtioScsiMaxLunLimit|7|UINT32|7
>  
> +  ## Sets the *inclusive* number of targets and LUNs that PvScsi exposes for
> +  # scan by ScsiBusDxe.
> +  # As specified above for VirtioScsi, ScsiBusDxe scans all MaxTarget * MaxLun
> +  # possible devices, which can take extremely long. Thus, the blow constants
> +  # are used so that scanning the number of devices given by their product
> +  # is still acceptably fast.
> +  gUefiOvmfPkgTokenSpaceGuid.PcdPvScsiMaxTargetLimit|64|UINT8|0x40
> +  gUefiOvmfPkgTokenSpaceGuid.PcdPvScsiMaxLunLimit|0|UINT8|0x41
> +

Three cosmetic requests:

(1) s/blow/below/


(2) in the comment block, starting with the second line, please use a

  "#  "

prefix, rather than

  "# "

so that the actual text line up with the first line.


(3) As tokens for the new PCDs, please use 0x36 and 0x37, for keeping
the OVMF token space densely populated.


Regarding the UINT8 type for PcdPvScsiMaxTargetLimit -- the QEMU source
indeed seems to #define PVSCSI_MAX_DEVS as 64, so I guess a UINT8 should
suffice.

>    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 46b430a34a57..76bb361c7c94 100644
> --- a/OvmfPkg/PvScsiDxe/PvScsi.c
> +++ b/OvmfPkg/PvScsiDxe/PvScsi.c
> @@ -11,6 +11,7 @@
>  
>  #include <IndustryStandard/Pci.h>
>  #include <IndustryStandard/PvScsi.h>
> +#include <Library/BaseMemoryLib.h>
>  #include <Library/MemoryAllocationLib.h>
>  #include <Library/UefiBootServicesTableLib.h>
>  #include <Library/UefiLib.h>
> @@ -24,6 +25,30 @@
>  //
>  #define PVSCSI_BINDING_VERSION      0x10
>  
> +//
> +// Ext SCSI Pass Thru utilities
> +//

OK.

> +
> +//
> +// Check if Target argument to EXT_SCSI_PASS_THRU.GetNextTarget() and
> +// EXT_SCSI_PASS_THRU.GetNextTargetLun() is initialized
> +//

(4) For leading function comment blocks, please use the following style:

/**
  Blah.
**/

> +STATIC
> +BOOLEAN
> +IsTargetInitialized (
> +  IN UINT8                                          *Target
> +  )
> +{
> +  UINTN Idx;
> +
> +  for (Idx = 0; Idx < TARGET_MAX_BYTES; ++Idx) {
> +    if (Target[Idx] != 0xFF) {
> +      return TRUE;
> +    }
> +  }
> +  return FALSE;
> +}
> +
>  //
>  // Ext SCSI Pass Thru
>  //
> @@ -51,7 +76,54 @@ PvScsiGetNextTargetLun (
>    IN OUT UINT64                                     *Lun
>    )
>  {
> -  return EFI_UNSUPPORTED;
> +  UINT8      *TargetPtr;
> +  UINT8      LastTarget;
> +  PVSCSI_DEV *Dev;
> +
> +  if (Target == NULL) {
> +    return EFI_INVALID_PARAMETER;
> +  }
> +
> +  //
> +  // The TargetPointer input parameter is unnecessarily a pointer-to-pointer
> +  //
> +  TargetPtr = *Target;

(5) Please update the comment: in this function, the subject parameter
is called "Target", not "TargetPointer".

> +
> +  //
> +  // If target not initialized, return first target & LUN
> +  //
> +  if (!IsTargetInitialized (TargetPtr)) {
> +    ZeroMem (TargetPtr, TARGET_MAX_BYTES);
> +    *Lun = 0;
> +    return EFI_SUCCESS;
> +  }
> +
> +  //
> +  // We only use first byte of target identifer
> +  //
> +  LastTarget = *TargetPtr;
> +
> +  //
> +  // Increment (target, LUN) pair if valid on input
> +  //
> +  Dev = PVSCSI_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;
> +    *TargetPtr = LastTarget;
> +    return EFI_SUCCESS;
> +  }
> +
> +  return EFI_NOT_FOUND;
>  }
>  
>  STATIC
> @@ -110,7 +182,47 @@ PvScsiGetNextTarget (
>    IN OUT UINT8                                      **Target
>    )
>  {
> -  return EFI_UNSUPPORTED;
> +  UINT8      *TargetPtr;
> +  UINT8      LastTarget;
> +  PVSCSI_DEV *Dev;
> +
> +  if (Target == NULL) {
> +    return EFI_INVALID_PARAMETER;
> +  }
> +
> +  //
> +  // The Target input parameter is unnecessarily a pointer-to-pointer
> +  //
> +  TargetPtr = *Target;

Got the comment right, here :)

> +
> +  //
> +  // If target not initialized, return first target
> +  //
> +  if (!IsTargetInitialized (TargetPtr)) {
> +    ZeroMem (TargetPtr, TARGET_MAX_BYTES);
> +    return EFI_SUCCESS;
> +  }
> +
> +  //
> +  // We only use first byte of target identifer
> +  //
> +  LastTarget = *TargetPtr;
> +
> +  //
> +  // Increment target if valid on input
> +  //
> +  Dev = PVSCSI_FROM_PASS_THRU (This);
> +  if (LastTarget > Dev->MaxTarget) {
> +    return EFI_INVALID_PARAMETER;
> +  }
> +
> +  if (LastTarget < Dev->MaxTarget) {
> +    ++LastTarget;
> +    *TargetPtr = LastTarget;
> +    return EFI_SUCCESS;
> +  }
> +
> +  return EFI_NOT_FOUND;
>  }
>  
>  STATIC
> @@ -119,6 +231,12 @@ PvScsiInit (
>    IN OUT PVSCSI_DEV *Dev
>    )
>  {
> +  //
> +  // Init configuration
> +  //
> +  Dev->MaxTarget = PcdGet8 (PcdPvScsiMaxTargetLimit);
> +  Dev->MaxLun = PcdGet8 (PcdPvScsiMaxLunLimit);
> +
>    //
>    // Populate the exported interface's attributes
>    //
> diff --git a/OvmfPkg/PvScsiDxe/PvScsi.h b/OvmfPkg/PvScsiDxe/PvScsi.h
> index 3940b4c20019..dd3e0c68e6da 100644
> --- a/OvmfPkg/PvScsiDxe/PvScsi.h
> +++ b/OvmfPkg/PvScsiDxe/PvScsi.h
> @@ -19,6 +19,8 @@
>  
>  typedef struct {
>    UINT32                          Signature;
> +  UINT8                           MaxTarget;
> +  UINT8                           MaxLun;
>    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 3a8b07872ba3..96bd4e4a9a8b 100644
> --- a/OvmfPkg/PvScsiDxe/PvScsi.inf
> +++ b/OvmfPkg/PvScsiDxe/PvScsi.inf
> @@ -25,6 +25,7 @@
>    OvmfPkg/OvmfPkg.dec
>  
>  [LibraryClasses]
> +  BaseMemoryLib
>    DebugLib
>    MemoryAllocationLib
>    UefiBootServicesTableLib
> @@ -34,3 +35,7 @@
>  [Protocols]
>    gEfiPciIoProtocolGuid             ## TO_START
>    gEfiExtScsiPassThruProtocolGuid   ## BY_START
> +
> +[Pcd]
> +  gUefiOvmfPkgTokenSpaceGuid.PcdPvScsiMaxTargetLimit    ## CONSUMES
> +  gUefiOvmfPkgTokenSpaceGuid.PcdPvScsiMaxLunLimit       ## CONSUMES
> 

(6) Please keep the list of PCDs alphabetically sorted, in this section.


With (1) through (6) addressed:

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

Thanks,
Laszlo