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

["Wu, Hao A" <hao.a.wu@intel.com>]

> -----Original Message-----
> From: Ni, Ruiyu
> Sent: Friday, September 01, 2017 12:15 PM
> To: Wu, Hao A; edk2-devel@lists.01.org
> Subject: RE: [PATCH v2] SourceLevelDebugPkg: Use Pcd for the revision of
> transfer protocol
> 
> Hao,
> I have 2 comments regarding to the INF and DEC change. Check below.

Thanks for the comments.
I will refine them and send out a new version.

Best Regards,
Hao Wu

> 
> 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