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

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

On 07/16/20 17:04, Laszlo Ersek wrote:
> 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".

OK, reading through my v1 review, I've realized (again) that 127 is the
maximum valid LUN (and so the max PCD value), therefore the problem is
in the message, not in the check. Please modify the message to "[0..127]".

Thanks
Laszlo

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