Re: [PATCH v3] SourceLevelDebugPkg: Use Pcd for the revision of transfer protocol

["Ni, Ruiyu" <ruiyu.ni@intel.com>]

Reviewed-by: Ruiyu Ni <ruiyu.ni@intel.com>

Thanks/Ray

> -----Original Message-----
> From: Wu, Hao A
> Sent: Friday, September 1, 2017 12:43 PM
> To: edk2-devel@lists.01.org
> Cc: Wu, Hao A <hao.a.wu@intel.com>; Ni, Ruiyu <ruiyu.ni@intel.com>
> Subject: [PATCH v3] SourceLevelDebugPkg: Use Pcd for the revision of
> transfer protocol
> 
> V3 changes:
> Add detailed description for the usage of the Pcd in package DEC file.
> Update the comment for the introduced Pcd in module INF files.
> 
> V2 changes:
> Instead of using a global variable, use a Pcd for transfer protocol
> revision.
> 
> Previously, the revison of the debug agent transfer protocol is
> reflected by a macro.
> 
> This commit introduces a Pcd to reflect the revision in order to avoid the
> comparision of two macros, which will generate a constant result
> detected by code checkers.
> 
> Cc: Ruiyu Ni <ruiyu.ni@intel.com>
> Contributed-under: TianoCore Contribution Agreement 1.1
> Signed-off-by: Hao Wu <hao.a.wu@intel.com>
> ---
>  SourceLevelDebugPkg/Include/TransferProtocol.h                   | 3 +--
>  .../Library/DebugAgent/DebugAgentCommon/DebugAgent.c             | 6 +++--
> -
>  SourceLevelDebugPkg/Library/DebugAgent/DxeDebugAgentLib.inf      | 1 +
>  SourceLevelDebugPkg/Library/DebugAgent/SecPeiDebugAgentLib.inf   | 1 +
>  SourceLevelDebugPkg/Library/DebugAgent/SmmDebugAgentLib.inf      | 1 +
>  SourceLevelDebugPkg/SourceLevelDebugPkg.dec                      | 9 ++++++++-
>  SourceLevelDebugPkg/SourceLevelDebugPkg.uni                      | 9 ++++++++-
>  7 files changed, 23 insertions(+), 7 deletions(-)
> 
> diff --git a/SourceLevelDebugPkg/Include/TransferProtocol.h
> b/SourceLevelDebugPkg/Include/TransferProtocol.h
> index ef7c891c39..5f9f35b5d7 100644
> --- a/SourceLevelDebugPkg/Include/TransferProtocol.h
> +++ b/SourceLevelDebugPkg/Include/TransferProtocol.h
> @@ -2,7 +2,7 @@
>    Transfer protocol defintions used by debug agent and host. It is only
>    intended to be used by Debug related module implementation.
> 
> -  Copyright (c) 2010 - 2015, Intel Corporation. All rights reserved.<BR>
> +  Copyright (c) 2010 - 2017, Intel Corporation. All rights reserved.<BR>
>    This program and the accompanying materials
>    are licensed and made available under the terms and conditions of the BSD
> License
>    which accompanies this distribution.  The full text of the license may be
> found at
> @@ -24,7 +24,6 @@
>  //
>  #define DEBUG_AGENT_REVISION_03         ((0 << 16) | 03)
>  #define DEBUG_AGENT_REVISION_04         ((0 << 16) | 04)
> -#define DEBUG_AGENT_REVISION            DEBUG_AGENT_REVISION_04
>  #define DEBUG_AGENT_CAPABILITIES        0
> 
>  //
> diff --git
> a/SourceLevelDebugPkg/Library/DebugAgent/DebugAgentCommon/Debug
> Agent.c
> b/SourceLevelDebugPkg/Library/DebugAgent/DebugAgentCommon/Debug
> Agent.c
> index f156fe24db..36b1ef924c 100644
> ---
> a/SourceLevelDebugPkg/Library/DebugAgent/DebugAgentCommon/Debug
> Agent.c
> +++
> b/SourceLevelDebugPkg/Library/DebugAgent/DebugAgentCommon/Debug
> Agent.c
> @@ -1564,7 +1564,7 @@ ReadMemoryAndSendResponsePacket (
>      // Compression/decompression support was added since revision 0.4.
>      // Revision 0.3 shouldn't compress the packet.
>      //
> -    if (DEBUG_AGENT_REVISION >= DEBUG_AGENT_REVISION_04) {
> +    if (PcdGet32(PcdTransferProtocolRevision) >=
> DEBUG_AGENT_REVISION_04) {
>        //
>        // Get the compressed data size without modifying the packet.
>        //
> @@ -1711,7 +1711,7 @@ AttachHost (
>    }
>    if (IncompatibilityFlag) {
>      //
> -    // If the incompatible Debug Packet received, the HOST should be running
> transfer protocol before DEBUG_AGENT_REVISION.
> +    // If the incompatible Debug Packet received, the HOST should be running
> transfer protocol before PcdTransferProtocolRevision.
>      // It could be UDK Debugger for Windows v1.1/v1.2 or for Linux v0.8/v1.2.
>      //
>      DebugPortWriteBuffer (Handle, (UINT8 *) mErrorMsgVersionAlert,
> AsciiStrLen (mErrorMsgVersionAlert));
> @@ -2192,7 +2192,7 @@ CommandCommunication (
>        break;
> 
>      case DEBUG_COMMAND_GET_REVISION:
> -      DebugAgentRevision.Revision = DEBUG_AGENT_REVISION;
> +      DebugAgentRevision.Revision = PcdGet32(PcdTransferProtocolRevision);
>        DebugAgentRevision.Capabilities = DEBUG_AGENT_CAPABILITIES;
>        Status = SendDataResponsePacket ((UINT8 *) &DebugAgentRevision,
> (UINT16) sizeof (DEBUG_DATA_RESPONSE_GET_REVISION), DebugHeader);
>        break;
> diff --git
> a/SourceLevelDebugPkg/Library/DebugAgent/DxeDebugAgentLib.inf
> b/SourceLevelDebugPkg/Library/DebugAgent/DxeDebugAgentLib.inf
> index ce36345bab..7a2b4378a6 100644
> --- a/SourceLevelDebugPkg/Library/DebugAgent/DxeDebugAgentLib.inf
> +++ b/SourceLevelDebugPkg/Library/DebugAgent/DxeDebugAgentLib.inf
> @@ -101,4 +101,5 @@
>    gEfiMdePkgTokenSpaceGuid.PcdFSBClock                                  ##
> SOMETIMES_CONSUMES
> 
> gEfiSourceLevelDebugPkgTokenSpaceGuid.PcdExceptionsIgnoredByDebugg
> er  ## SOMETIMES_CONSUMES
>    gEfiSourceLevelDebugPkgTokenSpaceGuid.PcdDebugPortHandleBufferSize
> ## CONSUMES
> +  gEfiSourceLevelDebugPkgTokenSpaceGuid.PcdTransferProtocolRevision
> ## CONSUMES
> 
> diff --git
> a/SourceLevelDebugPkg/Library/DebugAgent/SecPeiDebugAgentLib.inf
> b/SourceLevelDebugPkg/Library/DebugAgent/SecPeiDebugAgentLib.inf
> index 12c2a71b78..12534a298c 100644
> --- a/SourceLevelDebugPkg/Library/DebugAgent/SecPeiDebugAgentLib.inf
> +++ b/SourceLevelDebugPkg/Library/DebugAgent/SecPeiDebugAgentLib.inf
> @@ -91,4 +91,5 @@
>    gEfiMdePkgTokenSpaceGuid.PcdFSBClock                                  ##
> SOMETIMES_CONSUMES
> 
> gEfiSourceLevelDebugPkgTokenSpaceGuid.PcdExceptionsIgnoredByDebugg
> er  ## SOMETIMES_CONSUMES
>    gEfiSourceLevelDebugPkgTokenSpaceGuid.PcdDebugPortHandleBufferSize
> ## SOMETIMES_CONSUMES
> +  gEfiSourceLevelDebugPkgTokenSpaceGuid.PcdTransferProtocolRevision
> ## CONSUMES
> 
> diff --git
> a/SourceLevelDebugPkg/Library/DebugAgent/SmmDebugAgentLib.inf
> b/SourceLevelDebugPkg/Library/DebugAgent/SmmDebugAgentLib.inf
> index 1fa5745b1c..580ae5bc80 100644
> --- a/SourceLevelDebugPkg/Library/DebugAgent/SmmDebugAgentLib.inf
> +++ b/SourceLevelDebugPkg/Library/DebugAgent/SmmDebugAgentLib.inf
> @@ -86,4 +86,5 @@
>    gEfiMdePkgTokenSpaceGuid.PcdFSBClock                                             ##
> SOMETIMES_CONSUMES
>    # Skip Page Fault exception (14) by default in SMM
> 
> gEfiSourceLevelDebugPkgTokenSpaceGuid.PcdExceptionsIgnoredByDebugg
> er|0x00004000  ## SOMETIMES_CONSUMES
> +  gEfiSourceLevelDebugPkgTokenSpaceGuid.PcdTransferProtocolRevision
> ## CONSUMES
> 
> diff --git a/SourceLevelDebugPkg/SourceLevelDebugPkg.dec
> b/SourceLevelDebugPkg/SourceLevelDebugPkg.dec
> index 9579c3e006..c9a47f6255 100644
> --- a/SourceLevelDebugPkg/SourceLevelDebugPkg.dec
> +++ b/SourceLevelDebugPkg/SourceLevelDebugPkg.dec
> @@ -6,7 +6,7 @@
>  # and host, PeCoffExtraActionLib instance to report symbol path information,
>  # etc.
>  #
> -# Copyright (c) 2010 - 2015, Intel Corporation. All rights reserved.<BR>
> +# Copyright (c) 2010 - 2017, Intel Corporation. All rights reserved.<BR>
>  # This program and the accompanying materials are licensed and made
> available under
>  # the terms and conditions of the BSD License that accompanies this
> distribution.
>  # The full text of the license may be found at
> @@ -112,5 +112,12 @@
>    # @Prompt Configure debug device detection timeout value.
> 
> gEfiSourceLevelDebugPkgTokenSpaceGuid.PcdUsbXhciDebugDetectTimeout
> |3000000|UINT64|0x00000009
> 
> +  ## Default revision of the debug agent transfer protocol.
> +  #  The upper 16 bits indicate the major revision and the lower 16 bits
> indicate the minor revision.
> +  #  For example, a value of 0x00000004 stands for revision 0.4.
> +  #  Debug packet compression and decompression is supported since
> revision 0.4.
> +  # @Prompt Default revision of the debug agent transfer protocol.
> +
> gEfiSourceLevelDebugPkgTokenSpaceGuid.PcdTransferProtocolRevision|0x0
> 0000004|UINT32|0x0000000a
> +
>  [UserExtensions.TianoCore."ExtraFiles"]
>    SourceLevelDebugPkgExtra.uni
> diff --git a/SourceLevelDebugPkg/SourceLevelDebugPkg.uni
> b/SourceLevelDebugPkg/SourceLevelDebugPkg.uni
> index 533dafbfc8..781b717e4e 100644
> --- a/SourceLevelDebugPkg/SourceLevelDebugPkg.uni
> +++ b/SourceLevelDebugPkg/SourceLevelDebugPkg.uni
> @@ -8,7 +8,7 @@
>  // and host, PeCoffExtraActionLib instance to report symbol path
> information,
>  // etc.
>  //
> -// Copyright (c) 2010 - 2015, Intel Corporation. All rights reserved.<BR>
> +// Copyright (c) 2010 - 2017, Intel Corporation. All rights reserved.<BR>
>  //
>  // This program and the accompanying materials are licensed and made
> available under
>  // the terms and conditions of the BSD License that accompanies this
> distribution.
> @@ -91,3 +91,10 @@
>  #string
> STR_gEfiSourceLevelDebugPkgTokenSpaceGuid_PcdUsbXhciDebugDetectTi
> meout_HELP  #language en-US "Per XHCI spec, software shall impose a
> timeout between the detection of the Debug Host\n"
>                                                                                                       "connection and the DbC
> Run transition to 1. This PCD specifies the timeout value in microsecond."
> 
> +#string
> STR_gEfiSourceLevelDebugPkgTokenSpaceGuid_PcdTransferProtocolRevisio
> n_PROMPT  #language en-US "Default revision of the debug agent transfer
> protocol."
> +
> +#string
> STR_gEfiSourceLevelDebugPkgTokenSpaceGuid_PcdTransferProtocolRevisio
> n_HELP  #language en-US "The upper 16 bits indicate the major revision and
> the lower 16 bits indicate\n"
> +                                                                                                    "the minor revision.\n"
> +                                                                                                    "For example, a value of
> 0x00000004 stands for revision 0.4.\n"
> +                                                                                                    "Debug packet
> compression and decompression is supported since revision 0.4."
> +
> --
> 2.12.0.windows.1