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

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

Hao,
I have 2 comments regarding to the INF and DEC change. Check below.

Thanks/Ray

> -----Original Message-----
> From: Wu, Hao A
> Sent: Friday, September 1, 2017 10:13 AM
> To: edk2-devel@lists.01.org
> Cc: Wu, Hao A <hao.a.wu@intel.com>; Ni, Ruiyu <ruiyu.ni@intel.com>
> Subject: [PATCH v2] SourceLevelDebugPkg: Use Pcd for the revision of
> transfer protocol
> 
> 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                        | 7 ++++++-
>  SourceLevelDebugPkg/SourceLevelDebugPkg.uni                        | 6 +++++-
>  7 files changed, 18 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..17b1ac5a89 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
> ## SOMETIMES_CONSUMES
> 
> diff --git
> a/SourceLevelDebugPkg/Library/DebugAgent/SecPeiDebugAgentLib.inf
> b/SourceLevelDebugPkg/Library/DebugAgent/SecPeiDebugAgentLib.inf
> index 12c2a71b78..2f2bc6c162 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
> ## SOMETIMES_CONSUMES
> 
> diff --git
> a/SourceLevelDebugPkg/Library/DebugAgent/SmmDebugAgentLib.inf
> b/SourceLevelDebugPkg/Library/DebugAgent/SmmDebugAgentLib.inf
> index 1fa5745b1c..df7ad75d68 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
> ## SOMETIMES_CONSUMES
1. Why SOMETIMES_CONSUMES instead of CONSUMES?

> 
> diff --git a/SourceLevelDebugPkg/SourceLevelDebugPkg.dec
> b/SourceLevelDebugPkg/SourceLevelDebugPkg.dec
> index 9579c3e006..18f9410539 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,10 @@
>    # @Prompt Configure debug device detection timeout value.
> 
> gEfiSourceLevelDebugPkgTokenSpaceGuid.PcdUsbXhciDebugDetectTimeout
> |3000000|UINT64|0x00000009
> 
> +  ## Default revision of the debug agent transfer protocol.
> +  #  Debug packet compression and decompression is supported since
> revision 0.4.

2. Please describe the PcdTransferProtocolRevision layout. Upper 2-byte for
major revision, lower-2-byte for minor revision. 0x0004 stands for 0.4.

> +  # @Prompt Default revision of the debug agent transfer protocol.
> +
> gEfiSourceLevelDebugPkgTokenSpaceGuid.PcdTransferProtocolRevision|0x4
> |UINT32|0x0000000a
> +
>  [UserExtensions.TianoCore."ExtraFiles"]
>    SourceLevelDebugPkgExtra.uni
> diff --git a/SourceLevelDebugPkg/SourceLevelDebugPkg.uni
> b/SourceLevelDebugPkg/SourceLevelDebugPkg.uni
> index 533dafbfc8..d90a112e2c 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,7 @@
>  #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 "Debug packet compression and decompression is
> supported since revision 0.4."
> +
> --
> 2.12.0.windows.1