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

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

On 04/29/20 15:38, Laszlo Ersek wrote:
> 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
> 

... BTW I missed the same in the PvScsiDxe driver :/

Liran, can you post a patch for that, please?

Thanks,
Laszlo