Hi,

Hao, you are 100% correct. It seems that not only is SourceLevelDebugPkg using a different CRC-16 algorithm, my CalculateCrc16()'s lookup table is incorrect.

I'll re-submit a v2 that fixes these issues.
Sorry for the trouble.

Thanks,
Pedro

On Mon, Mar 28, 2022 at 1:14 AM Wu, Hao A <hao.a.wu@intel.com> wrote:
Hello,

My local test shows that the results between the old (SourceLevelDebugPkg) & new (MdePkg) CRC16 implementation are different:

UINT8  Array[16] = {1,2,3,4,5,6,7,8,8,7,6,5,4,3,2,1};
UINT16 a, b;   
a = CalculateCrc16New(Array, sizeof(Array), 0xFFFF);
b = CalculateCrc16Old(Array, sizeof(Array), 0xFFFF);

Result:
a = 0x0af5
b = 0x2778

My take is that for the SourceLevelDebugPkg case, the CRC16 result should be the same (at BIOS host side). Otherwise, the target side will have issue for the CRC check.

Best Regards,
Hao Wu

> -----Original Message-----
> From: Pedro Falcato <pedro.falcato@gmail.com>
> Sent: Sunday, March 27, 2022 8:58 AM
> To: devel@edk2.groups.io
> Cc: Wu, Hao A <hao.a.wu@intel.com>
> Subject: [PATCH 2/2] SourceLevelDebugPkg/DebugAgent: Delete the CRC16
> implementation
>
> BZ: https://bugzilla.tianocore.org/show_bug.cgi?id=3871
>
> Deletes the CRC16 implementation as CalculateCrc16() is being merged into
> BaseLib.
>
> Cc: Hao A Wu <hao.a.wu@intel.com>
> Signed-off-by: Pedro Falcato <pedro.falcato@gmail.com>
> ---
>  .../DebugAgent/DebugAgentCommon/DebugAgent.c  | 35 -------------------
>  1 file changed, 35 deletions(-)
>
> diff --git
> a/SourceLevelDebugPkg/Library/DebugAgent/DebugAgentCommon/DebugAgen
> t.c
> b/SourceLevelDebugPkg/Library/DebugAgent/DebugAgentCommon/DebugAgen
> t.c
> index a1e61a6ef90e..62c6a235d425 100644
> ---
> a/SourceLevelDebugPkg/Library/DebugAgent/DebugAgentCommon/DebugAgen
> t.c
> +++
> b/SourceLevelDebugPkg/Library/DebugAgent/DebugAgentCommon/DebugAgen
> t
> +++ .c
> @@ -126,41 +126,6 @@ GLOBAL_REMOVE_IF_UNREFERENCED
> EFI_VECTOR_HANDOFF_INFO  mVectorHandoffInfoDebugAg
>
>  GLOBAL_REMOVE_IF_UNREFERENCED UINTN  mVectorHandoffInfoCount =
> sizeof (mVectorHandoffInfoDebugAgent) / sizeof
> (EFI_VECTOR_HANDOFF_INFO);
>
> -/**
> -  Calculate CRC16 for target data.
> -
> -  @param[in]  Data              The target data.
> -  @param[in]  DataSize          The target data size.
> -  @param[in]  Crc               Initial CRC.
> -
> -  @return UINT16     The CRC16 value.
> -
> -**/
> -UINT16
> -CalculateCrc16 (
> -  IN UINT8   *Data,
> -  IN UINTN   DataSize,
> -  IN UINT16  Crc
> -  )
> -{
> -  UINTN  Index;
> -  UINTN  BitIndex;
> -
> -  for (Index = 0; Index < DataSize; Index++) {
> -    Crc ^= (UINT16)Data[Index];
> -    for (BitIndex = 0; BitIndex < 8; BitIndex++) {
> -      if ((Crc & 0x8000) != 0) {
> -        Crc <<= 1;
> -        Crc  ^= 0x1021;
> -      } else {
> -        Crc <<= 1;
> -      }
> -    }
> -  }
> -
> -  return Crc;
> -}
> -
>  /**
>    Read IDT entry to check if IDT entries are setup by Debug Agent.
>
> --
> 2.35.1



--
Pedro Falcato