Re: [PATCH v2 05/13] OvmfPkg/MptScsiDxe: Install stubbed EXT_SCSI_PASS_THRU

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

On 02/26/20 17:41, Nikita Leshenko wrote:
> Support dynamic insertion and removal of the protocol
> 
> Ref: https://bugzilla.tianocore.org/show_bug.cgi?id=2390
> Contributed-under: TianoCore Contribution Agreement 1.1
> Signed-off-by: Nikita Leshenko <nikita.leshchenko@oracle.com>
> Reviewed-by: Konrad Rzeszutek Wilk <konrad.wilk@oracle.com>
> Reviewed-by: Aaron Young <aaron.young@oracle.com>
> Reviewed-by: Liran Alon <liran.alon@oracle.com>
> ---
>  OvmfPkg/MptScsiDxe/MptScsi.c      | 178 +++++++++++++++++++++++++++++-
>  OvmfPkg/MptScsiDxe/MptScsiDxe.inf |   5 +-
>  2 files changed, 180 insertions(+), 3 deletions(-)
> 
> diff --git a/OvmfPkg/MptScsiDxe/MptScsi.c b/OvmfPkg/MptScsiDxe/MptScsi.c
> index 6dc6257eba..b8eabfb23c 100644
> --- a/OvmfPkg/MptScsiDxe/MptScsi.c
> +++ b/OvmfPkg/MptScsiDxe/MptScsi.c
> @@ -17,10 +17,13 @@
>  
>  #include <IndustryStandard/Pci.h>
>  #include <IndustryStandard/FusionMptScsi.h>
> +#include <Library/BaseMemoryLib.h>
>  #include <Library/DebugLib.h>
> +#include <Library/MemoryAllocationLib.h>
>  #include <Library/UefiBootServicesTableLib.h>
>  #include <Library/UefiLib.h>
>  #include <Protocol/PciIo.h>
> +#include <Protocol/ScsiPassThruExt.h>
>  
>  //
>  // Higher versions will be used before lower, 0x10-0xffffffef is the version
> @@ -28,6 +31,109 @@
>  //
>  #define MPT_SCSI_BINDING_VERSION 0x10
>  
> +//
> +// Runtime Structures
> +//
> +
> +#define MPT_SCSI_DEV_SIGNATURE SIGNATURE_32 ('M','P','T','S')
> +typedef struct {
> +  UINT32                          Signature;
> +  EFI_EXT_SCSI_PASS_THRU_PROTOCOL PassThru;
> +  EFI_EXT_SCSI_PASS_THRU_MODE     PassThruMode;
> +} MPT_SCSI_DEV;
> +
> +#define MPT_SCSI_FROM_PASS_THRU(PassThruPtr) \
> +  CR (PassThruPtr, MPT_SCSI_DEV, PassThru, MPT_SCSI_DEV_SIGNATURE)
> +
> +//
> +// Ext SCSI Pass Thru
> +//
> +
> +STATIC
> +EFI_STATUS
> +EFIAPI
> +MptScsiPassThru (
> +  IN EFI_EXT_SCSI_PASS_THRU_PROTOCOL                *This,
> +  IN UINT8                                          *Target,
> +  IN UINT64                                         Lun,
> +  IN OUT EFI_EXT_SCSI_PASS_THRU_SCSI_REQUEST_PACKET *Packet,
> +  IN EFI_EVENT                                      Event     OPTIONAL
> +  )
> +{
> +  return EFI_UNSUPPORTED;
> +}
> +
> +STATIC
> +EFI_STATUS
> +EFIAPI
> +MptScsiGetNextTargetLun (
> +  IN EFI_EXT_SCSI_PASS_THRU_PROTOCOL                *This,
> +  IN OUT UINT8                                      **Target,
> +  IN OUT UINT64                                     *Lun
> +  )
> +{
> +  return EFI_UNSUPPORTED;
> +}
> +
> +STATIC
> +EFI_STATUS
> +EFIAPI
> +MptScsiGetNextTarget (
> +  IN EFI_EXT_SCSI_PASS_THRU_PROTOCOL               *This,
> +  IN OUT UINT8                                     **Target
> +  )
> +{
> +  return EFI_UNSUPPORTED;
> +}
> +
> +STATIC
> +EFI_STATUS
> +EFIAPI
> +MptScsiBuildDevicePath (
> +  IN EFI_EXT_SCSI_PASS_THRU_PROTOCOL               *This,
> +  IN UINT8                                         *Target,
> +  IN UINT64                                        Lun,
> +  IN OUT EFI_DEVICE_PATH_PROTOCOL                  **DevicePath
> +  )
> +{
> +  return EFI_UNSUPPORTED;
> +}
> +
> +STATIC
> +EFI_STATUS
> +EFIAPI
> +MptScsiGetTargetLun (
> +  IN EFI_EXT_SCSI_PASS_THRU_PROTOCOL               *This,
> +  IN EFI_DEVICE_PATH_PROTOCOL                      *DevicePath,
> +  OUT UINT8                                        **Target,
> +  OUT UINT64                                       *Lun
> +  )
> +{
> +  return EFI_UNSUPPORTED;
> +}
> +
> +STATIC
> +EFI_STATUS
> +EFIAPI
> +MptScsiResetChannel (
> +  IN EFI_EXT_SCSI_PASS_THRU_PROTOCOL               *This
> +  )
> +{
> +  return EFI_UNSUPPORTED;
> +}
> +
> +STATIC
> +EFI_STATUS
> +EFIAPI
> +MptScsiResetTargetLun (
> +  IN EFI_EXT_SCSI_PASS_THRU_PROTOCOL               *This,
> +  IN UINT8                                         *Target,
> +  IN UINT64                                        Lun
> +  )
> +{
> +  return EFI_UNSUPPORTED;
> +}
> +
>  //
>  // Driver Binding
>  //
> @@ -96,7 +202,49 @@ MptScsiControllerStart (
>    IN EFI_DEVICE_PATH_PROTOCOL               *RemainingDevicePath OPTIONAL
>    )
>  {
> -  return EFI_UNSUPPORTED;
> +  EFI_STATUS           Status;
> +  MPT_SCSI_DEV         *Dev;
> +
> +  Dev = AllocateZeroPool (sizeof (*Dev));
> +  if (Dev == NULL) {
> +    return EFI_OUT_OF_RESOURCES;
> +  }
> +
> +  Dev->Signature = MPT_SCSI_DEV_SIGNATURE;
> +
> +  //
> +  // Host adapter channel, doesn't exist
> +  //
> +  Dev->PassThruMode.AdapterId = MAX_UINT32;
> +  Dev->PassThruMode.Attributes =
> +    EFI_EXT_SCSI_PASS_THRU_ATTRIBUTES_PHYSICAL
> +    | EFI_EXT_SCSI_PASS_THRU_ATTRIBUTES_LOGICAL;
> +
> +  Dev->PassThru.Mode = &Dev->PassThruMode;
> +  Dev->PassThru.PassThru = &MptScsiPassThru;
> +  Dev->PassThru.GetNextTargetLun = &MptScsiGetNextTargetLun;
> +  Dev->PassThru.BuildDevicePath = &MptScsiBuildDevicePath;
> +  Dev->PassThru.GetTargetLun = &MptScsiGetTargetLun;
> +  Dev->PassThru.ResetChannel = &MptScsiResetChannel;
> +  Dev->PassThru.ResetTargetLun = &MptScsiResetTargetLun;
> +  Dev->PassThru.GetNextTarget = &MptScsiGetNextTarget;
> +
> +  Status = gBS->InstallProtocolInterface (
> +                  &ControllerHandle,
> +                  &gEfiExtScsiPassThruProtocolGuid,
> +                  EFI_NATIVE_INTERFACE,
> +                  &Dev->PassThru
> +                  );
> +  if (EFI_ERROR (Status)) {
> +    goto Done;
> +  }
> +
> +Done:
> +  if (EFI_ERROR (Status)) {
> +    FreePool (Dev);
> +  }
> +
> +  return Status;
>  }
>  
>  STATIC
> @@ -109,7 +257,33 @@ MptScsiControllerStop (
>    IN  EFI_HANDLE                            *ChildHandleBuffer
>    )
>  {
> -  return EFI_UNSUPPORTED;
> +  EFI_STATUS                      Status;
> +  EFI_EXT_SCSI_PASS_THRU_PROTOCOL *PassThru;
> +  MPT_SCSI_DEV                    *Dev;
> +
> +  Status = gBS->OpenProtocol (
> +                  ControllerHandle,
> +                  &gEfiExtScsiPassThruProtocolGuid,
> +                  (VOID **)&PassThru,
> +                  This->DriverBindingHandle,
> +                  ControllerHandle,
> +                  EFI_OPEN_PROTOCOL_GET_PROTOCOL // Lookup only
> +                  );
> +  if (EFI_ERROR (Status)) {
> +    return Status;
> +  }
> +
> +  Dev = MPT_SCSI_FROM_PASS_THRU (PassThru);
> +
> +  gBS->UninstallProtocolInterface (
> +         ControllerHandle,
> +         &gEfiExtScsiPassThruProtocolGuid,
> +         &Dev->PassThru
> +         );

(1) I think it's reasonable to assume that this will always succeed, but
please at least assign the retval to Status, and then follow up with an
ASSERT_EFI_ERROR(), before proceeding to the FreePool() below.

With this fixed:

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

Thanks
Laszlo

> +
> +  FreePool (Dev);
> +
> +  return Status;
>  }
>  
>  STATIC
> diff --git a/OvmfPkg/MptScsiDxe/MptScsiDxe.inf b/OvmfPkg/MptScsiDxe/MptScsiDxe.inf
> index dc3795c867..1cb5df4233 100644
> --- a/OvmfPkg/MptScsiDxe/MptScsiDxe.inf
> +++ b/OvmfPkg/MptScsiDxe/MptScsiDxe.inf
> @@ -30,10 +30,13 @@
>    OvmfPkg/OvmfPkg.dec
>  
>  [LibraryClasses]
> +  BaseMemoryLib
>    DebugLib
> +  MemoryAllocationLib
>    UefiBootServicesTableLib
>    UefiDriverEntryPoint
>    UefiLib
>  
>  [Protocols]
> -  gEfiPciIoProtocolGuid        ## TO_START
> +  gEfiPciIoProtocolGuid                  ## TO_START
> +  gEfiExtScsiPassThruProtocolGuid        ## BY_START
>