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

[Liran Alon <liran.alon@oracle.com>]

On 04/03/2020 21:22, Nikita Leshenko wrote:
> Support dynamic insertion and removal of the protocol
>
> Ref: https://bugzilla.tianocore.org/show_bug.cgi?id=2390
> Signed-off-by: Nikita Leshenko <nikita.leshchenko@oracle.com>
> Reviewed-by: Laszlo Ersek <lersek@redhat.com>
> ---
>   OvmfPkg/MptScsiDxe/MptScsi.c      | 179 +++++++++++++++++++++++++++++-
>   OvmfPkg/MptScsiDxe/MptScsiDxe.inf |   5 +-
>   2 files changed, 181 insertions(+), 3 deletions(-)
>
> diff --git a/OvmfPkg/MptScsiDxe/MptScsi.c b/OvmfPkg/MptScsiDxe/MptScsi.c
> index 07f8fe267fc2..9598b82fda53 100644
> --- a/OvmfPkg/MptScsiDxe/MptScsi.c
> +++ b/OvmfPkg/MptScsiDxe/MptScsi.c
> @@ -17,9 +17,12 @@
>   
>   #include <IndustryStandard/FusionMptScsi.h>
>   #include <IndustryStandard/Pci.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
> @@ -27,6 +30,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)
> +

Nit: I personally prefer to put these structures & macros on a dedicated 
header file for driver. i.e. OvmfPkg/MptScsiDxe/MptScsi.h.
Similar to how this is for VirtioScsi (And also upcoming PvScsi) 
implementation.

> +//
> +// 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
>   //
> @@ -95,7 +201,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;

Shouldn't "|" be on the line before?

> +
> +  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 FreePool;
> +  }
> +
> +  return EFI_SUCCESS;
> +
> +FreePool:
> +  FreePool (Dev);
> +
> +  return Status;
>   }
>   
>   STATIC
> @@ -108,7 +256,34 @@ 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);
> +
> +  Status = gBS->UninstallProtocolInterface (
> +                  ControllerHandle,
> +                  &gEfiExtScsiPassThruProtocolGuid,
> +                  &Dev->PassThru
> +                  );
> +  ASSERT_EFI_ERROR (Status);
I think this should be replaced with:
if (EFI_ERROR (Status)) {
   return Status;
}

Because otherwise, next line will free Dev->PassThru memory while the 
protocol is still registered.
This is also consistent with VirtioScsi implementation.
> +
> +  FreePool (Dev);
> +
> +  return Status;
>   }
>   
>   STATIC
> diff --git a/OvmfPkg/MptScsiDxe/MptScsiDxe.inf b/OvmfPkg/MptScsiDxe/MptScsiDxe.inf
> index 105af20f325f..a253c5d96916 100644
> --- a/OvmfPkg/MptScsiDxe/MptScsiDxe.inf
> +++ b/OvmfPkg/MptScsiDxe/MptScsiDxe.inf
> @@ -30,9 +30,12 @@ [Packages]
>     OvmfPkg/OvmfPkg.dec
>   
>   [LibraryClasses]
> +  DebugLib
> +  MemoryAllocationLib
>     UefiBootServicesTableLib
>     UefiDriverEntryPoint
>     UefiLib
>   
>   [Protocols]
> -  gEfiPciIoProtocolGuid        ## TO_START
> +  gEfiPciIoProtocolGuid                  ## TO_START
> +  gEfiExtScsiPassThruProtocolGuid        ## BY_START