Re: [edk2-devel] [PATCH 05/17] OvmfPkg/PvScsiDxe: Install stubbed EXT_SCSI_PASS_THRU

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

On 03/24/20 13:27, Laszlo Ersek wrote:
> On 03/16/20 16:01, Liran Alon wrote:
>> Support dynamic insertion and removal of the protocol.
>>
>> Ref: https://bugzilla.tianocore.org/show_bug.cgi?id=2567
>> Reviewed-by: Nikita Leshenko <nikita.leshchenko@oracle.com>
>> Signed-off-by: Liran Alon <liran.alon@oracle.com>
>> ---
>>  OvmfPkg/PvScsiDxe/PvScsi.c   | 209 ++++++++++++++++++++++++++++++++++-
>>  OvmfPkg/PvScsiDxe/PvScsi.h   |  29 +++++
>>  OvmfPkg/PvScsiDxe/PvScsi.inf |   5 +-
>>  3 files changed, 240 insertions(+), 3 deletions(-)
>>  create mode 100644 OvmfPkg/PvScsiDxe/PvScsi.h
>>
>> diff --git a/OvmfPkg/PvScsiDxe/PvScsi.c b/OvmfPkg/PvScsiDxe/PvScsi.c
>> index f1fffe962233..46b430a34a57 100644
>> --- a/OvmfPkg/PvScsiDxe/PvScsi.c
>> +++ b/OvmfPkg/PvScsiDxe/PvScsi.c
>> @@ -11,16 +11,155 @@
>>  
>>  #include <IndustryStandard/Pci.h>
>>  #include <IndustryStandard/PvScsi.h>
>> +#include <Library/MemoryAllocationLib.h>
>>  #include <Library/UefiBootServicesTableLib.h>
>>  #include <Library/UefiLib.h>
>>  #include <Protocol/PciIo.h>
>>  
>> +#include "PvScsi.h"
>> +
>>  //
>>  // Higher versions will be used before lower, 0x10-0xffffffef is the version
>>  // range for IVH (Indie Hardware Vendors)
>>  //
>>  #define PVSCSI_BINDING_VERSION      0x10
>>  
>> +//
>> +// Ext SCSI Pass Thru
>> +//
>> +
>> +STATIC
>> +EFI_STATUS
>> +EFIAPI
>> +PvScsiPassThru (
>> +  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
>> +PvScsiGetNextTargetLun (
>> +  IN EFI_EXT_SCSI_PASS_THRU_PROTOCOL                *This,
>> +  IN OUT UINT8                                      **Target,
>> +  IN OUT UINT64                                     *Lun
>> +  )
>> +{
>> +  return EFI_UNSUPPORTED;
>> +}
>> +
>> +STATIC
>> +EFI_STATUS
>> +EFIAPI
>> +PvScsiBuildDevicePath (
>> +  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
>> +PvScsiGetTargetLun (
>> +  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
>> +PvScsiResetChannel (
>> +  IN EFI_EXT_SCSI_PASS_THRU_PROTOCOL                *This
>> +  )
>> +{
>> +  return EFI_UNSUPPORTED;
>> +}
>> +
>> +STATIC
>> +EFI_STATUS
>> +EFIAPI
>> +PvScsiResetTargetLun (
>> +  IN EFI_EXT_SCSI_PASS_THRU_PROTOCOL                *This,
>> +  IN UINT8                                          *Target,
>> +  IN UINT64                                         Lun
>> +  )
>> +{
>> +  return EFI_UNSUPPORTED;
>> +}
>> +
>> +STATIC
>> +EFI_STATUS
>> +EFIAPI
>> +PvScsiGetNextTarget (
>> +  IN EFI_EXT_SCSI_PASS_THRU_PROTOCOL                *This,
>> +  IN OUT UINT8                                      **Target
>> +  )
>> +{
>> +  return EFI_UNSUPPORTED;
>> +}
>> +
>> +STATIC
>> +EFI_STATUS
>> +PvScsiInit (
>> +  IN OUT PVSCSI_DEV *Dev
>> +  )
>> +{
>> +  //
>> +  // Populate the exported interface's attributes
>> +  //
>> +  Dev->PassThru.Mode             = &Dev->PassThruMode;
>> +  Dev->PassThru.PassThru         = &PvScsiPassThru;
>> +  Dev->PassThru.GetNextTargetLun = &PvScsiGetNextTargetLun;
>> +  Dev->PassThru.BuildDevicePath  = &PvScsiBuildDevicePath;
>> +  Dev->PassThru.GetTargetLun     = &PvScsiGetTargetLun;
>> +  Dev->PassThru.ResetChannel     = &PvScsiResetChannel;
>> +  Dev->PassThru.ResetTargetLun   = &PvScsiResetTargetLun;
>> +  Dev->PassThru.GetNextTarget    = &PvScsiGetNextTarget;
>> +
>> +  //
>> +  // AdapterId is a target for which no handle will be created during bus scan.
>> +  // Prevent any conflict with real devices.
>> +  //
>> +  Dev->PassThruMode.AdapterId = MAX_UINT32;
>> +
>> +  //
>> +  // Set both physical and logical attributes for non-RAID SCSI channel
>> +  //
>> +  Dev->PassThruMode.Attributes = EFI_EXT_SCSI_PASS_THRU_ATTRIBUTES_PHYSICAL |
>> +                                 EFI_EXT_SCSI_PASS_THRU_ATTRIBUTES_LOGICAL;
>> +
>> +  //
>> +  // No restriction on transfer buffer alignment
>> +  //
>> +  Dev->PassThruMode.IoAlign = 0;
>> +
>> +  return EFI_SUCCESS;
>> +}
>> +
>> +STATIC
>> +VOID
>> +PvScsiUninit (
>> +  IN OUT PVSCSI_DEV *Dev
>> +  )
>> +{
>> +  // Currently nothing to do here
>> +}
>> +
>>  //
>>  // Driver Binding
>>  //
>> @@ -89,7 +228,42 @@ PvScsiDriverBindingStart (
>>    IN EFI_DEVICE_PATH_PROTOCOL    *RemainingDevicePath OPTIONAL
>>    )
>>  {
>> -  return EFI_UNSUPPORTED;
>> +  PVSCSI_DEV *Dev;
>> +  EFI_STATUS Status;
>> +
>> +  Dev = (PVSCSI_DEV *) AllocateZeroPool (sizeof (*Dev));
>> +  if (Dev == NULL) {
>> +    return EFI_OUT_OF_RESOURCES;
>> +  }
>> +
>> +  Status = PvScsiInit (Dev);
>> +  if (EFI_ERROR (Status)) {
>> +    goto FreePvScsi;
>> +  }
>> +
>> +  //
>> +  // Setup complete, attempt to export the driver instance's PassThru interface
>> +  //
>> +  Dev->Signature = PVSCSI_SIG;
>> +  Status = gBS->InstallProtocolInterface (
>> +                  &ControllerHandle,
>> +                  &gEfiExtScsiPassThruProtocolGuid,
>> +                  EFI_NATIVE_INTERFACE,
>> +                  &Dev->PassThru
>> +                  );
>> +  if (EFI_ERROR (Status)) {
>> +    goto UninitDev;
>> +  }
>> +
>> +  return EFI_SUCCESS;
>> +
>> +UninitDev:
>> +  PvScsiUninit (Dev);
>> +
>> +FreePvScsi:
>> +  FreePool (Dev);
>> +
>> +  return Status;
>>  }
>>  
>>  STATIC
>> @@ -102,7 +276,38 @@ PvScsiDriverBindingStop (
>>    IN EFI_HANDLE                  *ChildHandleBuffer
>>    )
>>  {
>> -  return EFI_UNSUPPORTED;
>> +  EFI_STATUS                      Status;
>> +  EFI_EXT_SCSI_PASS_THRU_PROTOCOL *PassThru;
>> +  PVSCSI_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 = PVSCSI_FROM_PASS_THRU (PassThru);
>> +
>> +  Status = gBS->UninstallProtocolInterface (
>> +                  ControllerHandle,
>> +                  &gEfiExtScsiPassThruProtocolGuid,
>> +                  &Dev->PassThru
>> +                  );
>> +  if (EFI_ERROR (Status)) {
>> +    return Status;
>> +  }
>> +
>> +  PvScsiUninit (Dev);
>> +
>> +  FreePool (Dev);
>> +
>> +  return EFI_SUCCESS;
>>  }
>>  
>>  STATIC
>> diff --git a/OvmfPkg/PvScsiDxe/PvScsi.h b/OvmfPkg/PvScsiDxe/PvScsi.h
>> new file mode 100644
>> index 000000000000..3940b4c20019
>> --- /dev/null
>> +++ b/OvmfPkg/PvScsiDxe/PvScsi.h
>> @@ -0,0 +1,29 @@
>> +/** @file
>> +
>> +  Internal definitions for the PVSCSI driver, which produces Extended SCSI
>> +  Pass Thru Protocol instances for pvscsi devices.
>> +
>> +  Copyright (C) 2020, Oracle and/or its affiliates.
>> +
>> +  SPDX-License-Identifier: BSD-2-Clause-Patent
>> +
>> +**/
>> +
>> +#ifndef __PVSCSI_DXE_H_
>> +#define __PVSCSI_DXE_H_
>> +
>> +#include <Library/DebugLib.h>
>> +#include <Protocol/ScsiPassThruExt.h>
>> +
>> +#define PVSCSI_SIG SIGNATURE_32 ('P', 'S', 'C', 'S')
>> +
>> +typedef struct {
>> +  UINT32                          Signature;
>> +  EFI_EXT_SCSI_PASS_THRU_PROTOCOL PassThru;
>> +  EFI_EXT_SCSI_PASS_THRU_MODE     PassThruMode;
>> +} PVSCSI_DEV;
>> +
>> +#define PVSCSI_FROM_PASS_THRU(PassThruPointer) \
>> +  CR (PassThruPointer, PVSCSI_DEV, PassThru, PVSCSI_SIG)
>> +
>> +#endif // __PVSCSI_DXE_H_
>> diff --git a/OvmfPkg/PvScsiDxe/PvScsi.inf b/OvmfPkg/PvScsiDxe/PvScsi.inf
>> index c1f0663832ed..3a8b07872ba3 100644
>> --- a/OvmfPkg/PvScsiDxe/PvScsi.inf
>> +++ b/OvmfPkg/PvScsiDxe/PvScsi.inf
>> @@ -25,9 +25,12 @@
>>    OvmfPkg/OvmfPkg.dec
>>  
>>  [LibraryClasses]
>> +  DebugLib
>> +  MemoryAllocationLib
>>    UefiBootServicesTableLib
>>    UefiDriverEntryPoint
>>    UefiLib
>>  
>>  [Protocols]
>> -  gEfiPciIoProtocolGuid        ## TO_START
>> +  gEfiPciIoProtocolGuid             ## TO_START
>> +  gEfiExtScsiPassThruProtocolGuid   ## BY_START
>>
> 
> (1) In the [Sources] section of the INF file, please list the new header
> file ("PvScsi.h").

(2) Please keep the list of protocols lexicographically sorted. (I
understand that you may have intended to place BY_START logically under
TO_START -- that looks simple enough initially, but with time it can get
messy. With normal sorting, there's never any doubt though.)

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

This stands.

Thanks
Laszlo