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

["Gary Lin" <glin@suse.com>]

On Tue, Jul 07, 2020 at 11:04:21AM +0200, Laszlo Ersek wrote:
> 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.
> 
Will change it to CopyMem() and add an assert to make sure MaxTarget is
always below 8.

> > +  *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.)
> 
Will remove the newline.

> > 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.
> 
I actually was thinking about making PcdMptScsiStallPerPollUsec 0x3a in
another patch. Since it's not related to LsiScsi, I didn't do it in this
series. Sounds like you prefer to keeping 0x40 for
PcdMptScsiStallPerPollUsec?

Gary Lin