Re: [edk2-devel] [PATCH v5 1/4] MdePkg: Implement SCSI commands for Security Protocol In/Out

["Liming Gao" <liming.gao@intel.com>]

Hao:
  I add my comments. 

Thanks
Liming
> -----Original Message-----
> From: Wu, Hao A
> Sent: Monday, September 2, 2019 9:11 AM
> To: Gao, Liming <liming.gao@intel.com>; Zurcher, Christopher J <christopher.j.zurcher@intel.com>; devel@edk2.groups.io; Kinney,
> Michael D <michael.d.kinney@intel.com>
> Cc: Yao, Jiewen <jiewen.yao@intel.com>; Wang, Jian J <jian.j.wang@intel.com>
> Subject: RE: [edk2-devel] [PATCH v5 1/4] MdePkg: Implement SCSI commands for Security Protocol In/Out
> 
> > -----Original Message-----
> > From: Gao, Liming
> > Sent: Friday, August 30, 2019 5:15 PM
> > To: Wu, Hao A; Zurcher, Christopher J; devel@edk2.groups.io; Kinney,
> > Michael D
> > Cc: Yao, Jiewen; Wang, Jian J; Gao, Liming
> > Subject: RE: [edk2-devel] [PATCH v5 1/4] MdePkg: Implement SCSI
> > commands for Security Protocol In/Out
> >
> > UefiScsiLib is designed for the convenient usage with SCSI commands. They
> > should try to align to UEFI definition.
> > If you check current SCSI APIs, their interface matches
> > EFI_SCSI_IO_SCSI_REQUEST_PACKET strut.
> > So, new added APIs had better match
> > EFI_STORAGE_SECURITY_COMMAND_PROTOCOL.
> >
> > For the change in MdePkg\Include\Protocol\ScsiIo.h, where is new definition
> > EFI_SCSI_IO_TYPE_WLUN from?
> 
> 
> Hello Liming,
> 
> The macro "EFI_SCSI_IO_TYPE_WLUN" comes from the SCSI Primary Commands standard
> (SPC), just like other existing definitions listed together in the header file.
I check UEFI spec. Other existing definitions are defined in UEFI spec. 
So, I think new added one should be proposed to UEFI spec for ScsiIo.h. 
I am OK to the change in IndustryStandard/Scsi.h. Can the consumer code get 
new definition from IndustryStandard/Scsi.h instead of Protocol/ScsiIo.h?
If so, this patch doesn't need to update Protocol/ScsiIo.h.

Thanks
Liming
> 
> Best Regards,
> Hao Wu
> 
> 
> >
> > Thanks
> > Liming
> > >-----Original Message-----
> > >From: Wu, Hao A
> > >Sent: Friday, August 30, 2019 1:18 PM
> > >To: Zurcher, Christopher J <christopher.j.zurcher@intel.com>;
> > >devel@edk2.groups.io; Gao, Liming <liming.gao@intel.com>; Kinney,
> > Michael
> > >D <michael.d.kinney@intel.com>
> > >Cc: Yao, Jiewen <jiewen.yao@intel.com>; Wang, Jian J
> > ><jian.j.wang@intel.com>
> > >Subject: RE: [edk2-devel] [PATCH v5 1/4] MdePkg: Implement SCSI
> > commands
> > >for Security Protocol In/Out
> > >
> > >Hello,
> > >
> > >Sorry for top-posting.
> > >
> > >I was thinking to make the parameters interface match between the
> > >UefiScsiLib
> > >API and the EFI Storage Security Command Protocol service, since the
> > >implementation of the SSC protocol will directly call the UefiScsiLib API.
> > >
> > >More specifically, for UefiScsiLib API:
> > >EFI_STATUS
> > >EFIAPI
> > >ScsiSecurityProtocolInCommand (
> > >  ...
> > >  IN     UINT32  TransferLength,
> > >  ...
> > >  IN OUT UINT32  *DataLength
> > >  )
> > >
> > >to match the SSC protocol service:
> > >typedef
> > >EFI_STATUS
> > >(EFIAPI *EFI_STORAGE_SECURITY_RECEIVE_DATA)(
> > >  ...
> > >  IN UINTN   PayloadBufferSize,
> > >  ...
> > >  OUT UINTN  *PayloadTransferSize
> > >  )
> > >
> > >and for UefiScsiLib API:
> > >EFI_STATUS
> > >EFIAPI
> > >ScsiSecurityProtocolOutCommand (
> > >  ...
> > >  IN     UINT32  TransferLength,
> > >  ...
> > >  )
> > >
> > >to match the SSC protocol service:
> > >typedef
> > >EFI_STATUS
> > >(EFIAPI *EFI_STORAGE_SECURITY_SEND_DATA) (
> > >  ...
> > >  IN UINTN  PayloadBufferSize,
> > >  ...
> > >  )
> > >
> > >I am okay with the cast from UINTN to UINT32, as long as we can ensure
> > >truncation will not happen (which I think should be safe when dealing with
> > >data transfer with actual devices).
> > >
> > >But for casting from UINTN* to UINT32*, I am not sure if this is a
> > >recommended
> > >coding style. Maybe within the BIOS perspective, little endian is always the
> > >case where such cast should work well.
> > >
> > >I will leave this open to MdePkg package maintainers for their inputs.
> > >
> > >Best Regards,
> > >Hao Wu
> > >
> > >
> > >> -----Original Message-----
> > >> From: Zurcher, Christopher J
> > >> Sent: Friday, August 30, 2019 8:35 AM
> > >> To: Wu, Hao A; devel@edk2.groups.io
> > >> Cc: Yao, Jiewen; Wang, Jian J; Gao, Liming
> > >> Subject: RE: [edk2-devel] [PATCH v5 1/4] MdePkg: Implement SCSI
> > >> commands for Security Protocol In/Out
> > >>
> > >> I've implemented all the suggested changes except changing the
> > arguments
> > >> from UINT32 to UINTN. No other functions in UefiScsiLib take UINTN
> > >> arguments, and since the library is directly packing the CDB, I think it
> > makes
> > >> sense to force the caller to provide the correct-size length value. That way
> > >> there is no ambiguity on what is going to the device.
> > >> If you agree I will send the updated patchset.
> > >>
> > >> Thanks,
> > >> Christopher Zurcher
> > >>
> > >> -----Original Message-----
> > >> From: Wu, Hao A
> > >> Sent: Monday, August 26, 2019 20:03
> > >> To: devel@edk2.groups.io; Zurcher, Christopher J
> > >> <christopher.j.zurcher@intel.com>
> > >> Cc: Yao, Jiewen <jiewen.yao@intel.com>; Wang, Jian J
> > >> <jian.j.wang@intel.com>; Gao, Liming <liming.gao@intel.com>
> > >> Subject: RE: [edk2-devel] [PATCH v5 1/4] MdePkg: Implement SCSI
> > >> commands for Security Protocol In/Out
> > >>
> > >> Hello,
> > >>
> > >> Please refer to the below inline comments:
> > >>
> > >>
> > >> > -----Original Message-----
> > >> > From: devel@edk2.groups.io [mailto:devel@edk2.groups.io] On Behalf
> > Of
> > >> > Zurcher, Christopher J
> > >> > Sent: Friday, August 23, 2019 6:02 AM
> > >> > To: devel@edk2.groups.io
> > >> > Cc: Yao, Jiewen; Wang, Jian J; Gao, Liming
> > >> > Subject: [edk2-devel] [PATCH v5 1/4] MdePkg: Implement SCSI
> > commands
> > >> > for Security Protocol In/Out
> > >> >
> > >> > This patch implements the Security Protocol In and Security Protocol Out
> > >> > commands in UefiScsiLib to prepare support for the Storage Security
> > >> > Command Protocol.
> > >> >
> > >> > Cc: Jiewen Yao <jiewen.yao@intel.com>
> > >> > Cc: Jian J Wang <jian.j.wang@intel.com>
> > >> > Cc: Liming Gao <liming.gao@intel.com>
> > >> > Signed-off-by: Christopher J Zurcher <christopher.j.zurcher@intel.com>
> > >> > ---
> > >> >  MdePkg/Include/IndustryStandard/Scsi.h   |  48 +++--
> > >> >  MdePkg/Include/Library/UefiScsiLib.h     | 126 +++++++++++-
> > >> >  MdePkg/Include/Protocol/ScsiIo.h         |   9 +-
> > >> >  MdePkg/Library/UefiScsiLib/UefiScsiLib.c | 205
> > +++++++++++++++++++-
> > >> >  4 files changed, 366 insertions(+), 22 deletions(-)
> > >> >
> > >> > diff --git a/MdePkg/Include/IndustryStandard/Scsi.h
> > >> > b/MdePkg/Include/IndustryStandard/Scsi.h
> > >> > index cbe5709fe5..10d7b49ba7 100644
> > >> > --- a/MdePkg/Include/IndustryStandard/Scsi.h
> > >> > +++ b/MdePkg/Include/IndustryStandard/Scsi.h
> > >> > @@ -1,7 +1,7 @@
> > >> >  /** @file
> > >> >    Support for SCSI-2 standard
> > >> >
> > >> > -  Copyright (c) 2006 - 2018, Intel Corporation. All rights reserved.<BR>
> > >> > +  Copyright (c) 2006 - 2019, Intel Corporation. All rights reserved.<BR>
> > >> >    SPDX-License-Identifier: BSD-2-Clause-Patent
> > >> >
> > >> >  **/
> > >> > @@ -163,6 +163,12 @@
> > >> >  #define EFI_SCSI_OP_SEND_MESSAGE10  0x2a
> > >> >  #define EFI_SCSI_OP_SEND_MESSAGE12  0xaa
> > >> >
> > >> > +//
> > >> > +// Additional commands for Secure Transactions
> > >> > +//
> > >> > +#define EFI_SCSI_OP_SECURITY_PROTOCOL_IN  0xa2
> > >> > +#define EFI_SCSI_OP_SECURITY_PROTOCOL_OUT 0xb5
> > >> > +
> > >> >  //
> > >> >  // SCSI Data Transfer Direction
> > >> >  //
> > >> > @@ -172,22 +178,30 @@
> > >> >  //
> > >> >  // Peripheral Device Type Definitions
> > >> >  //
> > >> > -#define EFI_SCSI_TYPE_DISK          0x00  ///< Direct-access device (e.g.
> > >> > magnetic disk)
> > >> > -#define EFI_SCSI_TYPE_TAPE          0x01  ///< Sequential-access device
> > >(e.g.
> > >> > magnetic tape)
> > >> > -#define EFI_SCSI_TYPE_PRINTER       0x02  ///< Printer device
> > >> > -#define EFI_SCSI_TYPE_PROCESSOR     0x03  ///< Processor device
> > >> > -#define EFI_SCSI_TYPE_WORM          0x04  ///< Write-once device (e.g.
> > >> some
> > >> > optical disks)
> > >> > -#define EFI_SCSI_TYPE_CDROM         0x05  ///< CD-ROM device
> > >> > -#define EFI_SCSI_TYPE_SCANNER       0x06  ///< Scanner device
> > >> > -#define EFI_SCSI_TYPE_OPTICAL       0x07  ///< Optical memory device
> > >(e.g.
> > >> > some optical disks)
> > >> > -#define EFI_SCSI_TYPE_MEDIUMCHANGER 0x08  ///< Medium changer
> > >> > device (e.g. jukeboxes)
> > >> > -#define EFI_SCSI_TYPE_COMMUNICATION 0x09  ///< Communications
> > >> > device
> > >> > -#define EFI_SCSI_TYPE_ASCIT8_1      0x0A  ///< Defined by ASC IT8
> > >> (Graphic
> > >> > arts pre-press devices)
> > >> > -#define EFI_SCSI_TYPE_ASCIT8_2      0x0B  ///< Defined by ASC IT8
> > >> (Graphic
> > >> > arts pre-press devices)
> > >>
> > >>
> > >> Could you help to address Liming's comment in the V4 series that to
> > >preserve
> > >> the definition for EFI_SCSI_TYPE_ASCIT8_1 & EFI_SCSI_TYPE_ASCIT8_2
> > for
> > >> compatibility consideration:
> > >>
> > >>
> > https://edk2.groups.io/g/devel/message/42361?p=,,,20,0,0,0::Created,,scsi,
> > >> 20,2,40,32048246
> > >>
> > >>
> > >> > -//
> > >> > -// 0Ch - 1Eh are reserved
> > >> > -//
> > >> > -#define EFI_SCSI_TYPE_UNKNOWN       0x1F  ///< Unknown or no
> > device
> > >> > type
> > >> > +#define EFI_SCSI_TYPE_DISK            0x00  ///< Direct-access device (e.g.
> > >> > magnetic disk)
> > >> > +#define EFI_SCSI_TYPE_TAPE            0x01  ///< Sequential-access device
> > >> (e.g.
> > >> > magnetic tape)
> > >> > +#define EFI_SCSI_TYPE_PRINTER         0x02  ///< Printer device
> > >> > +#define EFI_SCSI_TYPE_PROCESSOR       0x03  ///< Processor device
> > >> > +#define EFI_SCSI_TYPE_WORM            0x04  ///< Write-once device (e.g.
> > >> > some optical disks)
> > >> > +#define EFI_SCSI_TYPE_CDROM           0x05  ///< CD/DVD device
> > >> > +#define EFI_SCSI_TYPE_SCANNER         0x06  ///< Scanner device
> > >(obsolete)
> > >> > +#define EFI_SCSI_TYPE_OPTICAL         0x07  ///< Optical memory device
> > >> (e.g.
> > >> > some optical disks)
> > >> > +#define EFI_SCSI_TYPE_MEDIUMCHANGER   0x08  ///< Medium
> > changer
> > >> > device (e.g. jukeboxes)
> > >> > +#define EFI_SCSI_TYPE_COMMUNICATION   0x09  ///<
> > Communications
> > >> > device (obsolete)
> > >> > +#define EFI_SCSI_TYPE_A               0x0A  ///< Obsolete
> > >> > +#define EFI_SCSI_TYPE_B               0x0B  ///< Obsolete
> > >> > +#define EFI_SCSI_TYPE_RAID            0x0C  ///< Storage array controller
> > >> > device (e.g., RAID)
> > >> > +#define EFI_SCSI_TYPE_SES             0x0D  ///< Enclosure services device
> > >> > +#define EFI_SCSI_TYPE_RBC             0x0E  ///< Simplified direct-access
> > >> device
> > >> > (e.g., magnetic disk)
> > >> > +#define EFI_SCSI_TYPE_OCRW            0x0F  ///< Optical card
> > reader/writer
> > >> > device
> > >> > +#define EFI_SCSI_TYPE_BRIDGE          0x10  ///< Bridge Controller
> > >> Commands
> > >> > +#define EFI_SCSI_TYPE_OSD             0x11  ///< Object-based Storage
> > >> Device
> > >> > +#define EFI_SCSI_TYPE_AUTOMATION      0x12  ///< Automation/Drive
> > >> > Interface
> > >> > +#define EFI_SCSI_TYPE_SECURITYMANAGER 0x13  ///< Security
> > manager
> > >> > device
> > >> > +#define EFI_SCSI_TYPE_RESERVED_LOW    0x14  ///< Reserved (low)
> > >> > +#define EFI_SCSI_TYPE_RESERVED_HIGH   0x1D  ///< Reserved (high)
> > >> > +#define EFI_SCSI_TYPE_WLUN            0x1E  ///< Well known logical unit
> > >> > +#define EFI_SCSI_TYPE_UNKNOWN         0x1F  ///< Unknown or no
> > >device
> > >> > type
> > >> >
> > >> >  //
> > >> >  // Page Codes for INQUIRY command
> > >> > diff --git a/MdePkg/Include/Library/UefiScsiLib.h
> > >> > b/MdePkg/Include/Library/UefiScsiLib.h
> > >> > index 10dd81902b..a0d99e703a 100644
> > >> > --- a/MdePkg/Include/Library/UefiScsiLib.h
> > >> > +++ b/MdePkg/Include/Library/UefiScsiLib.h
> > >> > @@ -5,7 +5,7 @@
> > >> >    for hard drive, CD and DVD devices that are the most common SCSI
> > boot
> > >> > targets used by UEFI platforms.
> > >> >    This library class depends on SCSI I/O Protocol defined in UEFI
> > >> Specification
> > >> > and SCSI-2 industry standard.
> > >> >
> > >> > -Copyright (c) 2006 - 2018, Intel Corporation. All rights reserved.<BR>
> > >> > +Copyright (c) 2006 - 2019, Intel Corporation. All rights reserved.<BR>
> > >> >  SPDX-License-Identifier: BSD-2-Clause-Patent
> > >> >
> > >> >  **/
> > >> > @@ -813,6 +813,130 @@ ScsiWrite16Command (
> > >> >    );
> > >> >
> > >> >
> > >> > +/**
> > >> > +  Execute Security Protocol In SCSI command on a specific SCSI target.
> > >> > +
> > >> > +  Executes the SCSI Security Protocol In command on the SCSI target
> > >> > specified by ScsiIo.
> > >> > +  If Timeout is zero, then this function waits indefinitely for the
> > command
> > >> to
> > >> > complete.
> > >> > +  If Timeout is greater than zero, then the command is executed and
> > will
> > >> > timeout after
> > >> > +  Timeout 100 ns units.  The StartLba and SectorSize parameters are
> > used
> > >> to
> > >> > construct
> > >>
> > >>
> > >> As mentioned in V4 series:
> > >>
> > >> There is no 'StartLba' & 'SectorSize' parameters for APIs:
> > >>
> > >> ScsiSecurityProtocolInCommand
> > >> ScsiSecurityProtocolOutCommand
> > >>
> > >> Could you help to update the comments to address this?
> > >> (Please help to update UefiScsiLib.c as well.)
> > >>
> > >>
> > >> > +  the CDB for this SCSI command.
> > >> > +  If ScsiIo is NULL, then ASSERT().
> > >> > +  If SenseDataLength is NULL, then ASSERT().
> > >> > +  If HostAdapterStatus is NULL, then ASSERT().
> > >> > +  If TargetStatus is NULL, then ASSERT().
> > >> > +  If DataLength is NULL, then ASSERT().
> > >> > +
> > >> > +  If SenseDataLength is non-zero and SenseData is not NULL,
> > SenseData
> > >> > must meet buffer
> > >> > +  alignment requirement defined in EFI_SCSI_IO_PROTOCOL.
> > Otherwise
> > >> > EFI_INVALID_PARAMETER
> > >> > +  gets returned.
> > >> > +
> > >> > +  If DataLength is non-zero and DataBuffer is not NULL, DataBuffer
> > must
> > >> > meet buffer
> > >> > +  alignment requirement defined in EFI_SCSI_IO_PROTOCOL.
> > Otherwise
> > >> > EFI_INVALID_PARAMETER
> > >> > +  gets returned.
> > >> > +
> > >> > +  @param[in]      ScsiIo               SCSI IO Protocol to use.
> > >> > +  @param[in]      Timeout              The length of timeout period.
> > >> > +  @param[in, out] SenseData            A pointer to output sense data.
> > >> > +  @param[in, out] SenseDataLength      The length of output sense data.
> > >> > +  @param[out]     HostAdapterStatus    The status of Host Adapter.
> > >> > +  @param[out]     TargetStatus         The status of the target.
> > >> > +  @param[in]      SecurityProtocol     The Security Protocol to use.
> > >> > +  @param[in]      SecurityProtocolSpecific  The Security Protocol Specific
> > >> data.
> > >> > +  @param[in]      TransferLength       The size in bytes of the data
> > allocation.
> > >> > +  @param[in, out] DataBuffer           A pointer to a data buffer.
> > >> > +  @param[in, out] DataLength           The length of data buffer.
> > >>
> > >>
> > >> As mentioned in V4 series:
> > >>
> > >> Referring to the implementation of the library (changes made in
> > >> MdePkg/Library/UefiScsiLib/UefiScsiLib.c):
> > >>
> > >> 'TransferLength' (input) specifies the length of content in 'DataBuffer';
> > >> 'DataLength' (input & output) reflects the actual number of bytes
> > >> transferred.
> > >>
> > >> How about swapping their names and changing the description comments
> > >to:
> > >> (Please help to update UefiScsiLib.c as well.)
> > >>
> > >>   @param[in]      DataLength           The size in bytes of the data buffer.
> > >>   ...
> > >>   @param[out]     TransferLength       A pointer to a buffer to store the size
> > >>                                        in bytes of the data written to the data
> > >>                                        buffer.
> > >>
> > >>
> > >> > +
> > >> > +  @retval  EFI_SUCCESS                 Command is executed successfully.
> > >> > +  @retval  EFI_BAD_BUFFER_SIZE         The SCSI Request Packet was
> > >> > executed, but the entire DataBuffer could
> > >> > +                                       not be transferred. The actual number of bytes
> > >> > transferred is returned in DataLength.
> > >> > +  @retval  EFI_NOT_READY               The SCSI Request Packet could not
> > be
> > >> > sent because there are too many
> > >> > +                                       SCSI Command Packets already queued.
> > >> > +  @retval  EFI_DEVICE_ERROR            A device error occurred while
> > >> > attempting to send SCSI Request Packet.
> > >> > +  @retval  EFI_UNSUPPORTED             The command described by the
> > SCSI
> > >> > Request Packet is not supported by
> > >> > +                                       the SCSI initiator(i.e., SCSI  Host Controller)
> > >> > +  @retval  EFI_TIMEOUT                 A timeout occurred while waiting for
> > the
> > >> > SCSI Request Packet to execute.
> > >> > +  @retval  EFI_INVALID_PARAMETER       The contents of the SCSI
> > Request
> > >> > Packet are invalid.
> > >> > +
> > >> > +**/
> > >> > +EFI_STATUS
> > >> > +EFIAPI
> > >> > +ScsiSecurityProtocolInCommand (
> > >> > +  IN     EFI_SCSI_IO_PROTOCOL  *ScsiIo,
> > >> > +  IN     UINT64                Timeout,
> > >> > +  IN OUT VOID                  *SenseData,   OPTIONAL
> > >> > +  IN OUT UINT8                 *SenseDataLength,
> > >> > +     OUT UINT8                 *HostAdapterStatus,
> > >> > +     OUT UINT8                 *TargetStatus,
> > >> > +  IN     UINT8                 SecurityProtocol,
> > >> > +  IN     UINT16                SecurityProtocolSpecific,
> > >> > +  IN     UINT32                TransferLength,
> > >> > +  IN OUT VOID                  *DataBuffer,  OPTIONAL
> > >> > +  IN OUT UINT32                *DataLength
> > >> > +  );
> > >>
> > >>
> > >> As mentioned in V4 series, could you help to add a new parameter
> > "Inc512"
> > >> for
> > >> both new APIs:
> > >> ScsiSecurityProtocolInCommand
> > >> ScsiSecurityProtocolOutCommand
> > >>
> > >> Though UFS spec requires the INC_512 field of a CDB to be set to 0, but
> > >> for other devices, setting this field to 1 may be a valid configuration.
> > >>
> > >>
> > >> Also, I would suggest the below parameter type changes to match with
> > the
> > >> services
> > >> definition of the EFI_STORAGE_SECURITY_COMMAND_PROTOCOL
> > >(including
> > >> the
> > >> name swap mentioned above):
> > >>
> > >> IN     UINT32                TransferLength,
> > >> to
> > >> IN     UINTN                 DataLength,
> > >>
> > >> IN OUT UINT32                *DataLength
> > >> to
> > >>    OUT UINTN                 *TransferLength
> > >>
> > >>
> > >> > +
> > >> > +
> > >> > +/**
> > >> > +  Execute Security Protocol Out SCSI command on a specific SCSI target.
> > >> > +
> > >> > +  Executes the SCSI Security Protocol Out command on the SCSI target
> > >> > specified by ScsiIo.
> > >> > +  If Timeout is zero, then this function waits indefinitely for the
> > command
> > >> to
> > >> > complete.
> > >> > +  If Timeout is greater than zero, then the command is executed and
> > will
> > >> > timeout after
> > >> > +  Timeout 100 ns units.  The StartLba and SectorSize parameters are
> > used
> > >> to
> > >> > construct
> > >>
> > >>
> > >> As mentioned in V4 series:
> > >>
> > >> There is no 'StartLba' & 'SectorSize' parameters for APIs:
> > >>
> > >> ScsiSecurityProtocolInCommand
> > >> ScsiSecurityProtocolOutCommand
> > >>
> > >> Could you help to update the comments to address this?
> > >> (Please help to update UefiScsiLib.c as well.)
> > >>
> > >>
> > >> > +  the CDB for this SCSI command.
> > >> > +  If ScsiIo is NULL, then ASSERT().
> > >> > +  If SenseDataLength is NULL, then ASSERT().
> > >> > +  If HostAdapterStatus is NULL, then ASSERT().
> > >> > +  If TargetStatus is NULL, then ASSERT().
> > >> > +  If DataLength is NULL, then ASSERT().
> > >> > +
> > >> > +  If SenseDataLength is non-zero and SenseData is not NULL,
> > SenseData
> > >> > must meet buffer
> > >> > +  alignment requirement defined in EFI_SCSI_IO_PROTOCOL.
> > Otherwise
> > >> > EFI_INVALID_PARAMETER
> > >> > +  gets returned.
> > >> > +
> > >> > +  If DataLength is non-zero and DataBuffer is not NULL, DataBuffer
> > must
> > >> > meet buffer
> > >> > +  alignment requirement defined in EFI_SCSI_IO_PROTOCOL.
> > Otherwise
> > >> > EFI_INVALID_PARAMETER
> > >> > +  gets returned.
> > >> > +
> > >> > +  @param[in]      ScsiIo               SCSI IO Protocol to use.
> > >> > +  @param[in]      Timeout              The length of timeout period.
> > >> > +  @param[in, out] SenseData            A pointer to output sense data.
> > >> > +  @param[in, out] SenseDataLength      The length of output sense data.
> > >> > +  @param[out]     HostAdapterStatus    The status of Host Adapter.
> > >> > +  @param[out]     TargetStatus         The status of the target.
> > >> > +  @param[in]      SecurityProtocol     The Security Protocol to use.
> > >> > +  @param[in]      SecurityProtocolSpecific  The Security Protocol Specific
> > >> data.
> > >> > +  @param[in]      TransferLength       The size in bytes of the transfer
> > data.
> > >> > +  @param[in, out] DataBuffer           A pointer to a data buffer.
> > >>
> > >>
> > >> As mentioned in V4:
> > >>
> > >> Suggest to rename 'TransferLength' to 'DataLength' so that it may be a bit
> > >> more clear for users to know 'DataLength' reflects the size of 'DataBuffer'.
> > >>
> > >>
> > >> > +
> > >> > +  @retval  EFI_SUCCESS                 Command is executed successfully.
> > >> > +  @retval  EFI_BAD_BUFFER_SIZE         The SCSI Request Packet was
> > >> > executed, but the entire DataBuffer could
> > >> > +                                       not be transferred. The actual number of bytes
> > >> > transferred is returned in DataLength.
> > >> > +  @retval  EFI_NOT_READY               The SCSI Request Packet could not
> > be
> > >> > sent because there are too many
> > >> > +                                       SCSI Command Packets already queued.
> > >> > +  @retval  EFI_DEVICE_ERROR            A device error occurred while
> > >> > attempting to send SCSI Request Packet.
> > >> > +  @retval  EFI_UNSUPPORTED             The command described by the
> > SCSI
> > >> > Request Packet is not supported by
> > >> > +                                       the SCSI initiator(i.e., SCSI  Host Controller)
> > >> > +  @retval  EFI_TIMEOUT                 A timeout occurred while waiting for
> > the
> > >> > SCSI Request Packet to execute.
> > >> > +  @retval  EFI_INVALID_PARAMETER       The contents of the SCSI
> > Request
> > >> > Packet are invalid.
> > >> > +
> > >> > +**/
> > >> > +EFI_STATUS
> > >> > +EFIAPI
> > >> > +ScsiSecurityProtocolOutCommand (
> > >> > +  IN     EFI_SCSI_IO_PROTOCOL  *ScsiIo,
> > >> > +  IN     UINT64                Timeout,
> > >> > +  IN OUT VOID                  *SenseData,   OPTIONAL
> > >> > +  IN OUT UINT8                 *SenseDataLength,
> > >> > +     OUT UINT8                 *HostAdapterStatus,
> > >> > +     OUT UINT8                 *TargetStatus,
> > >> > +  IN     UINT8                 SecurityProtocol,
> > >> > +  IN     UINT16                SecurityProtocolSpecific,
> > >> > +  IN     UINT32                TransferLength,
> > >> > +  IN OUT VOID                  *DataBuffer  OPTIONAL
> > >> > +  );
> > >>
> > >>
> > >> As mentioned in V4 series:
> > >>
> > >> Suggest the below parameter type changes to match with the services
> > >> definition of the EFI_STORAGE_SECURITY_COMMAND_PROTOCOL
> > >(including
> > >> the
> > >> name change mentioned above):
> > >>
> > >> IN     UINT32                TransferLength
> > >> to
> > >> IN     UINTN                 DataLength
> > >>
> > >> Best Regards,
> > >> Hao Wu
> > >>
> > >>
> > >> > +
> > >> > +
> > >> >  /**
> > >> >    Execute blocking/non-blocking Read(10) SCSI command on a specific
> > SCSI
> > >> >    target.
> > >> > diff --git a/MdePkg/Include/Protocol/ScsiIo.h
> > >> > b/MdePkg/Include/Protocol/ScsiIo.h
> > >> > index 05e46bda9c..27c31fe7f9 100644
> > >> > --- a/MdePkg/Include/Protocol/ScsiIo.h
> > >> > +++ b/MdePkg/Include/Protocol/ScsiIo.h
> > >> > @@ -4,7 +4,7 @@
> > >> >    services environment to access SCSI devices. In particular, functions
> > for
> > >> >    managing devices on SCSI buses are defined here.
> > >> >
> > >> > -  Copyright (c) 2006 - 2018, Intel Corporation. All rights reserved.<BR>
> > >> > +  Copyright (c) 2006 - 2019, Intel Corporation. All rights reserved.<BR>
> > >> >    SPDX-License-Identifier: BSD-2-Clause-Patent
> > >> >
> > >> >  **/
> > >> > @@ -43,8 +43,11 @@ typedef struct _EFI_SCSI_IO_PROTOCOL
> > >> > EFI_SCSI_IO_PROTOCOL;
> > >> >  #define MFI_SCSI_IO_TYPE_OCRW                                  0x0F    ///< Optical
> > >> card
> > >> > reader/writer device
> > >> >  #define MFI_SCSI_IO_TYPE_BRIDGE                                0x10    ///< Bridge
> > >> > Controller Commands
> > >> >  #define MFI_SCSI_IO_TYPE_OSD                                   0x11    ///< Object-
> > >> based
> > >> > Storage Device
> > >> > -#define EFI_SCSI_IO_TYPE_RESERVED_LOW                          0x12    ///<
> > >> > Reserved (low)
> > >> > -#define EFI_SCSI_IO_TYPE_RESERVED_HIGH                         0x1E    ///<
> > >> > Reserved (high)
> > >> > +#define MFI_SCSI_IO_TYPE_AUTOMATION                            0x12    ///<
> > >> > Automation/Drive Interface
> > >> > +#define MFI_SCSI_IO_TYPE_SECURITYMANAGER                       0x13
> > ///<
> > >> > Security manager device
> > >> > +#define EFI_SCSI_IO_TYPE_RESERVED_LOW                          0x14    ///<
> > >> > Reserved (low)
> > >> > +#define EFI_SCSI_IO_TYPE_RESERVED_HIGH                         0x1D    ///<
> > >> > Reserved (high)
> > >> > +#define EFI_SCSI_IO_TYPE_WLUN                                  0x1E    ///< Well
> > >> known
> > >> > logical unit
> > >> >  #define EFI_SCSI_IO_TYPE_UNKNOWN                               0x1F    ///<
> > >> Unknown
> > >> > no device type
> > >> >
> > >> >  //
> > >> > diff --git a/MdePkg/Library/UefiScsiLib/UefiScsiLib.c
> > >> > b/MdePkg/Library/UefiScsiLib/UefiScsiLib.c
> > >> > index c7491d1436..7584d717ad 100644
> > >> > --- a/MdePkg/Library/UefiScsiLib/UefiScsiLib.c
> > >> > +++ b/MdePkg/Library/UefiScsiLib/UefiScsiLib.c
> > >> > @@ -1,7 +1,7 @@
> > >> >  /** @file
> > >> >    UEFI SCSI Library implementation
> > >> >
> > >> > -  Copyright (c) 2006 - 2018, Intel Corporation. All rights reserved.<BR>
> > >> > +  Copyright (c) 2006 - 2019, Intel Corporation. All rights reserved.<BR>
> > >> >    SPDX-License-Identifier: BSD-2-Clause-Patent
> > >> >
> > >> >  **/
> > >> > @@ -23,6 +23,7 @@
> > >> >    //
> > >> >  #define EFI_SCSI_OP_LENGTH_SIX      0x6
> > >> >  #define EFI_SCSI_OP_LENGTH_TEN      0xa
> > >> > +#define EFI_SCSI_OP_LENGTH_TWELVE   0xc
> > >> >  #define EFI_SCSI_OP_LENGTH_SIXTEEN  0x10
> > >> >
> > >> >  //
> > >> > @@ -1280,6 +1281,208 @@ ScsiWrite16Command (
> > >> >  }
> > >> >
> > >> >
> > >> > +/**
> > >> > +  Execute Security Protocol In SCSI command on a specific SCSI target.
> > >> > +
> > >> > +  Executes the SCSI Security Protocol In command on the SCSI target
> > >> > specified by ScsiIo.
> > >> > +  If Timeout is zero, then this function waits indefinitely for the
> > command
> > >> to
> > >> > complete.
> > >> > +  If Timeout is greater than zero, then the command is executed and
> > will
> > >> > timeout after
> > >> > +  Timeout 100 ns units.  The StartLba and SectorSize parameters are
> > used
> > >> to
> > >> > construct
> > >>
> > >>
> > >> As mentioned in V4 series:
> > >>
> > >> There is no 'StartLba' & 'SectorSize' parameters for APIs:
> > >>
> > >> ScsiSecurityProtocolInCommand
> > >> ScsiSecurityProtocolOutCommand
> > >>
> > >> Could you help to update the comments to address this?
> > >> (Please help to update UefiScsiLib.c as well.)
> > >>
> > >>
> > >> > +  the CDB for this SCSI command.
> > >> > +  If ScsiIo is NULL, then ASSERT().
> > >> > +  If SenseDataLength is NULL, then ASSERT().
> > >> > +  If HostAdapterStatus is NULL, then ASSERT().
> > >> > +  If TargetStatus is NULL, then ASSERT().
> > >> > +  If DataLength is NULL, then ASSERT().
> > >> > +
> > >> > +  If SenseDataLength is non-zero and SenseData is not NULL,
> > SenseData
> > >> > must meet buffer
> > >> > +  alignment requirement defined in EFI_SCSI_IO_PROTOCOL.
> > Otherwise
> > >> > EFI_INVALID_PARAMETER
> > >> > +  gets returned.
> > >> > +
> > >> > +  If DataLength is non-zero and DataBuffer is not NULL, DataBuffer
> > must
> > >> > meet buffer
> > >> > +  alignment requirement defined in EFI_SCSI_IO_PROTOCOL.
> > Otherwise
> > >> > EFI_INVALID_PARAMETER
> > >> > +  gets returned.
> > >> > +
> > >> > +  @param[in]      ScsiIo               SCSI IO Protocol to use.
> > >> > +  @param[in]      Timeout              The length of timeout period.
> > >> > +  @param[in, out] SenseData            A pointer to output sense data.
> > >> > +  @param[in, out] SenseDataLength      The length of output sense data.
> > >> > +  @param[out]     HostAdapterStatus    The status of Host Adapter.
> > >> > +  @param[out]     TargetStatus         The status of the target.
> > >> > +  @param[in]      SecurityProtocol     The Security Protocol to use.
> > >> > +  @param[in]      SecurityProtocolSpecific  The Security Protocol Specific
> > >> data.
> > >> > +  @param[in]      TransferLength       The size in bytes of the data
> > allocation.
> > >> > +  @param[in, out] DataBuffer           A pointer to a data buffer.
> > >> > +  @param[in, out] DataLength           The length of data buffer.
> > >> > +
> > >> > +  @retval  EFI_SUCCESS                 Command is executed successfully.
> > >> > +  @retval  EFI_BAD_BUFFER_SIZE         The SCSI Request Packet was
> > >> > executed, but the entire DataBuffer could
> > >> > +                                       not be transferred. The actual number of bytes
> > >> > transferred is returned in DataLength.
> > >> > +  @retval  EFI_NOT_READY               The SCSI Request Packet could not
> > be
> > >> > sent because there are too many
> > >> > +                                       SCSI Command Packets already queued.
> > >> > +  @retval  EFI_DEVICE_ERROR            A device error occurred while
> > >> > attempting to send SCSI Request Packet.
> > >> > +  @retval  EFI_UNSUPPORTED             The command described by the
> > SCSI
> > >> > Request Packet is not supported by
> > >> > +                                       the SCSI initiator(i.e., SCSI  Host Controller)
> > >> > +  @retval  EFI_TIMEOUT                 A timeout occurred while waiting for
> > the
> > >> > SCSI Request Packet to execute.
> > >> > +  @retval  EFI_INVALID_PARAMETER       The contents of the SCSI
> > Request
> > >> > Packet are invalid.
> > >> > +
> > >> > +**/
> > >> > +EFI_STATUS
> > >> > +EFIAPI
> > >> > +ScsiSecurityProtocolInCommand (
> > >> > +  IN     EFI_SCSI_IO_PROTOCOL  *ScsiIo,
> > >> > +  IN     UINT64                Timeout,
> > >> > +  IN OUT VOID                  *SenseData,   OPTIONAL
> > >> > +  IN OUT UINT8                 *SenseDataLength,
> > >> > +     OUT UINT8                 *HostAdapterStatus,
> > >> > +     OUT UINT8                 *TargetStatus,
> > >> > +  IN     UINT8                 SecurityProtocol,
> > >> > +  IN     UINT16                SecurityProtocolSpecific,
> > >> > +  IN     UINT32                TransferLength,
> > >> > +  IN OUT VOID                  *DataBuffer,  OPTIONAL
> > >> > +  IN OUT UINT32                *DataLength
> > >> > +  )
> > >> > +{
> > >> > +  EFI_SCSI_IO_SCSI_REQUEST_PACKET CommandPacket;
> > >> > +  EFI_STATUS                      Status;
> > >> > +  UINT8                           Cdb[EFI_SCSI_OP_LENGTH_TWELVE];
> > >> > +
> > >> > +  ASSERT (SenseDataLength != NULL);
> > >> > +  ASSERT (HostAdapterStatus != NULL);
> > >> > +  ASSERT (TargetStatus != NULL);
> > >> > +  ASSERT (DataLength != NULL);
> > >> > +  ASSERT (ScsiIo != NULL);
> > >> > +
> > >> > +  ZeroMem (&CommandPacket, sizeof
> > >> > (EFI_SCSI_IO_SCSI_REQUEST_PACKET));
> > >> > +  ZeroMem (Cdb, EFI_SCSI_OP_LENGTH_TWELVE);
> > >> > +
> > >> > +  CommandPacket.Timeout           = Timeout;
> > >> > +  CommandPacket.InDataBuffer      = DataBuffer;
> > >> > +  CommandPacket.SenseData         = SenseData;
> > >> > +  CommandPacket.InTransferLength  = TransferLength;
> > >> > +  CommandPacket.Cdb               = Cdb;
> > >> > +  //
> > >> > +  // Fill Cdb for Security Protocol In Command
> > >> > +  //
> > >> > +  Cdb[0]                        = EFI_SCSI_OP_SECURITY_PROTOCOL_IN;
> > >> > +  Cdb[1]                        = SecurityProtocol;
> > >> > +  WriteUnaligned16 ((UINT16 *)&Cdb[2], SwapBytes16
> > >> > (SecurityProtocolSpecific));
> > >> > +  WriteUnaligned32 ((UINT32 *)&Cdb[6], SwapBytes32
> > (TransferLength));
> > >> > +
> > >> > +  CommandPacket.CdbLength       = EFI_SCSI_OP_LENGTH_TWELVE;
> > >> > +  CommandPacket.DataDirection   = EFI_SCSI_DATA_IN;
> > >> > +  CommandPacket.SenseDataLength = *SenseDataLength;
> > >> > +
> > >> > +  Status                        = ScsiIo->ExecuteScsiCommand (ScsiIo,
> > >> > &CommandPacket, NULL);
> > >> > +
> > >> > +  *HostAdapterStatus            = CommandPacket.HostAdapterStatus;
> > >> > +  *TargetStatus                 = CommandPacket.TargetStatus;
> > >> > +  *SenseDataLength              = CommandPacket.SenseDataLength;
> > >> > +  *DataLength                   = CommandPacket.InTransferLength;
> > >> > +
> > >> > +  return Status;
> > >> > +}
> > >> > +
> > >> > +
> > >> > +/**
> > >> > +  Execute Security Protocol Out SCSI command on a specific SCSI target.
> > >> > +
> > >> > +  Executes the SCSI Security Protocol Out command on the SCSI target
> > >> > specified by ScsiIo.
> > >> > +  If Timeout is zero, then this function waits indefinitely for the
> > command
> > >> to
> > >> > complete.
> > >> > +  If Timeout is greater than zero, then the command is executed and
> > will
> > >> > timeout after
> > >> > +  Timeout 100 ns units.  The StartLba and SectorSize parameters are
> > used
> > >> to
> > >> > construct
> > >> > +  the CDB for this SCSI command.
> > >> > +  If ScsiIo is NULL, then ASSERT().
> > >> > +  If SenseDataLength is NULL, then ASSERT().
> > >> > +  If HostAdapterStatus is NULL, then ASSERT().
> > >> > +  If TargetStatus is NULL, then ASSERT().
> > >> > +  If DataLength is NULL, then ASSERT().
> > >> > +
> > >> > +  If SenseDataLength is non-zero and SenseData is not NULL,
> > SenseData
> > >> > must meet buffer
> > >> > +  alignment requirement defined in EFI_SCSI_IO_PROTOCOL.
> > Otherwise
> > >> > EFI_INVALID_PARAMETER
> > >> > +  gets returned.
> > >> > +
> > >> > +  If DataLength is non-zero and DataBuffer is not NULL, DataBuffer
> > must
> > >> > meet buffer
> > >> > +  alignment requirement defined in EFI_SCSI_IO_PROTOCOL.
> > Otherwise
> > >> > EFI_INVALID_PARAMETER
> > >> > +  gets returned.
> > >> > +
> > >> > +  @param[in]      ScsiIo               SCSI IO Protocol to use.
> > >> > +  @param[in]      Timeout              The length of timeout period.
> > >> > +  @param[in, out] SenseData            A pointer to output sense data.
> > >> > +  @param[in, out] SenseDataLength      The length of output sense data.
> > >> > +  @param[out]     HostAdapterStatus    The status of Host Adapter.
> > >> > +  @param[out]     TargetStatus         The status of the target.
> > >> > +  @param[in]      SecurityProtocol     The Security Protocol to use.
> > >> > +  @param[in]      SecurityProtocolSpecific  The Security Protocol Specific
> > >> data.
> > >> > +  @param[in]      TransferLength       The size in bytes of the transfer
> > data.
> > >> > +  @param[in, out] DataBuffer           A pointer to a data buffer.
> > >> > +
> > >> > +  @retval  EFI_SUCCESS                 Command is executed successfully.
> > >> > +  @retval  EFI_BAD_BUFFER_SIZE         The SCSI Request Packet was
> > >> > executed, but the entire DataBuffer could
> > >> > +                                       not be transferred. The actual number of bytes
> > >> > transferred is returned in DataLength.
> > >> > +  @retval  EFI_NOT_READY               The SCSI Request Packet could not
> > be
> > >> > sent because there are too many
> > >> > +                                       SCSI Command Packets already queued.
> > >> > +  @retval  EFI_DEVICE_ERROR            A device error occurred while
> > >> > attempting to send SCSI Request Packet.
> > >> > +  @retval  EFI_UNSUPPORTED             The command described by the
> > SCSI
> > >> > Request Packet is not supported by
> > >> > +                                       the SCSI initiator(i.e., SCSI  Host Controller)
> > >> > +  @retval  EFI_TIMEOUT                 A timeout occurred while waiting for
> > the
> > >> > SCSI Request Packet to execute.
> > >> > +  @retval  EFI_INVALID_PARAMETER       The contents of the SCSI
> > Request
> > >> > Packet are invalid.
> > >> > +
> > >> > +**/
> > >> > +EFI_STATUS
> > >> > +EFIAPI
> > >> > +ScsiSecurityProtocolOutCommand (
> > >> > +  IN     EFI_SCSI_IO_PROTOCOL  *ScsiIo,
> > >> > +  IN     UINT64                Timeout,
> > >> > +  IN OUT VOID                  *SenseData,   OPTIONAL
> > >> > +  IN OUT UINT8                 *SenseDataLength,
> > >> > +     OUT UINT8                 *HostAdapterStatus,
> > >> > +     OUT UINT8                 *TargetStatus,
> > >> > +  IN     UINT8                 SecurityProtocol,
> > >> > +  IN     UINT16                SecurityProtocolSpecific,
> > >> > +  IN     UINT32                TransferLength,
> > >> > +  IN OUT VOID                  *DataBuffer   OPTIONAL
> > >> > +  )
> > >> > +{
> > >> > +  EFI_SCSI_IO_SCSI_REQUEST_PACKET CommandPacket;
> > >> > +  EFI_STATUS                      Status;
> > >> > +  UINT8                           Cdb[EFI_SCSI_OP_LENGTH_TWELVE];
> > >> > +
> > >> > +  ASSERT (SenseDataLength != NULL);
> > >> > +  ASSERT (HostAdapterStatus != NULL);
> > >> > +  ASSERT (TargetStatus != NULL);
> > >> > +  ASSERT (ScsiIo != NULL);
> > >> > +
> > >> > +  ZeroMem (&CommandPacket, sizeof
> > >> > (EFI_SCSI_IO_SCSI_REQUEST_PACKET));
> > >> > +  ZeroMem (Cdb, EFI_SCSI_OP_LENGTH_TWELVE);
> > >> > +
> > >> > +  CommandPacket.Timeout           = Timeout;
> > >> > +  CommandPacket.OutDataBuffer     = DataBuffer;
> > >> > +  CommandPacket.SenseData         = SenseData;
> > >> > +  CommandPacket.OutTransferLength = TransferLength;
> > >> > +  CommandPacket.Cdb               = Cdb;
> > >> > +  //
> > >> > +  // Fill Cdb for Security Protocol Out Command
> > >> > +  //
> > >> > +  Cdb[0]                        = EFI_SCSI_OP_SECURITY_PROTOCOL_OUT;
> > >> > +  Cdb[1]                        = SecurityProtocol;
> > >> > +  WriteUnaligned16 ((UINT16 *)&Cdb[2], SwapBytes16
> > >> > (SecurityProtocolSpecific));
> > >> > +  WriteUnaligned32 ((UINT32 *)&Cdb[6], SwapBytes32
> > (TransferLength));
> > >> > +
> > >> > +  CommandPacket.CdbLength       = EFI_SCSI_OP_LENGTH_TWELVE;
> > >> > +  CommandPacket.DataDirection   = EFI_SCSI_DATA_OUT;
> > >> > +  CommandPacket.SenseDataLength = *SenseDataLength;
> > >> > +
> > >> > +  Status                        = ScsiIo->ExecuteScsiCommand (ScsiIo,
> > >> > &CommandPacket, NULL);
> > >> > +
> > >> > +  *HostAdapterStatus            = CommandPacket.HostAdapterStatus;
> > >> > +  *TargetStatus                 = CommandPacket.TargetStatus;
> > >> > +  *SenseDataLength              = CommandPacket.SenseDataLength;
> > >> > +
> > >> > +  return Status;
> > >> > +}
> > >> > +
> > >> > +
> > >> >  /**
> > >> >    Internal helper notify function in which update the result of the
> > >> >    non-blocking SCSI Read/Write commands and signal caller event.
> > >> > --
> > >> > 2.16.2.windows.1
> > >> >
> > >> >
> > >> >