From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from mail-vk1-f177.google.com (mail-vk1-f177.google.com [209.85.221.177]) by mx.groups.io with SMTP id smtpd.web09.6213.1648432165496662952 for ; Sun, 27 Mar 2022 18:49:25 -0700 Authentication-Results: mx.groups.io; dkim=pass header.i=@gmail.com header.s=20210112 header.b=BjGyEAaC; spf=pass (domain: gmail.com, ip: 209.85.221.177, mailfrom: pedro.falcato@gmail.com) Received: by mail-vk1-f177.google.com with SMTP id l184so7214947vkh.0 for ; Sun, 27 Mar 2022 18:49:25 -0700 (PDT) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=gmail.com; s=20210112; h=mime-version:references:in-reply-to:from:date:message-id:subject:to :cc; bh=+t6hqbpGOTYekq/+HDcUFStvEHs8MVIJCFNuJrcMMxs=; b=BjGyEAaCm51RGVPzjdG/Fhq6c+/sNgkiOBDejDaVWl8gHq2vPrw9ifx9RR8F9bZr6h 3ECF0k4GgbEmUP+hhWD8Az61X4c8ubWFyQtXaIBer7Bco6tAVAz6XEw7tgm36LU8KIyc amiXLYBPV3gHf4awySzN+p42S30+qM7/uiBe9PQddHRwaZy6fZk/pvwHYrzCI02abOeE kbDiJBysXPcsXKiJvTqEn4wRZvD2NnL+p0Pe+XmmNtAMdxf3EDhk+sWD8bs7isE9FrFK LEkFmlZeNZ43hez8hM2X1d+ywJAXkvwFLxOAlrNsitUydHb8L98KIIGaDpfSQ+s+KP9Y x0iA== X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20210112; h=x-gm-message-state:mime-version:references:in-reply-to:from:date :message-id:subject:to:cc; bh=+t6hqbpGOTYekq/+HDcUFStvEHs8MVIJCFNuJrcMMxs=; b=RohMV2Ug5g0MZPUPrixBuIFMBnfK0mXHJWXW6a/xewFMJwo1+sNBjlePAzXD4LfB0c Q8CspKhm2e42zePAkiWYq+51ahwcdtPXLh6bm7Jk61IEx04s+v5AywCfp4YjQ4J+/JpI p1PWQvvq2FS6J2CaESwo7HuULDZbMYZbHBtM9bl91TnM0GQAG4TI+Wmrc04Q6H4hiMxb xT1CTHOPgxKPHiQzpEWxteMnGFAcuFNTrhifLWmXiY0frjeJl2/gU5CgOcB9la7vxsCE 6vwQnInHX00R9rrZJQhjt85rbEPPrWzV2UyAZ8E8bwQj6cw69jkxhXE/DZa61C5omgH+ Mbng== X-Gm-Message-State: AOAM531qqLuxMl37s0YuOQJj1PfCDbbb0tuudTG9wVlGiw3NjAxY8kgv QQzdMCPiH0tXfKZmk8BhUhIrkGQue3O4X2lKmGI= X-Google-Smtp-Source: ABdhPJxzeaW36rdmtgM0SMgl7u1k8Z5dWt171+uY9Sf8w4lvFg5j8CwG/yIGtd72Tb2Ubp70mq7asEkYUlTbPZEOT/A= X-Received: by 2002:a05:6122:44a:b0:343:2fbb:6d2e with SMTP id f10-20020a056122044a00b003432fbb6d2emr1353805vkk.23.1648432164634; Sun, 27 Mar 2022 18:49:24 -0700 (PDT) MIME-Version: 1.0 References: <20220327005748.92041-1-pedro.falcato@gmail.com> <20220327005748.92041-3-pedro.falcato@gmail.com> In-Reply-To: From: "Pedro Falcato" Date: Mon, 28 Mar 2022 02:49:13 +0100 Message-ID: Subject: Re: [PATCH 2/2] SourceLevelDebugPkg/DebugAgent: Delete the CRC16 implementation To: "Wu, Hao A" Cc: "devel@edk2.groups.io" Content-Type: multipart/alternative; boundary="0000000000009c03c905db3d8582" --0000000000009c03c905db3d8582 Content-Type: text/plain; charset="UTF-8" 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 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 > > Sent: Sunday, March 27, 2022 8:58 AM > > To: devel@edk2.groups.io > > Cc: Wu, Hao A > > 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 > > Signed-off-by: Pedro Falcato > > --- > > .../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 --0000000000009c03c905db3d8582 Content-Type: text/html; charset="UTF-8" Content-Transfer-Encoding: quoted-printable
Hi,

Hao, you are 100% correc= t. It seems that not only is SourceLevelDebugPkg using a different CRC-16 a= lgorithm, my CalculateCrc16()'s lookup table is incorrect.
I'll re-submit a v2 that fixes these issues.
Sor= ry 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=C2=A0 Array[16] =3D {1,2,3,4,5,6,7,8,8,7,6,5,4,3,2,1};
UINT16 a, b;=C2=A0 =C2=A0
a =3D CalculateCrc16New(Array, sizeof(Array), 0xFFFF);
b =3D CalculateCrc16Old(Array, sizeof(Array), 0xFFFF);

Result:
a =3D 0x0af5
b =3D 0x2778

My take is that for the SourceLevelDebugPkg case, the CRC16 result should b= e 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@ed= k2.groups.io
> Cc: Wu, Hao A <hao.a.wu@intel.com>
> Subject: [PATCH 2/2] SourceLevelDebugPkg/DebugAgent: Delete the CRC16<= br> > implementation
>
> BZ: https://bugzilla.tianocore.org/show_bu= g.cgi?id=3D3871
>
> Deletes the CRC16 implementation as CalculateCrc16() is being merged i= nto
> BaseLib.
>
> Cc: Hao A Wu <hao.a.wu@intel.com>
> Signed-off-by: Pedro Falcato <pedro.falcato@gmail.com>
> ---
>=C2=A0 .../DebugAgent/DebugAgentCommon/DebugAgent.c=C2=A0 | 35 --------= -----------
>=C2=A0 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=C2=A0 mVectorHandoffInfoDebugAg
>
>=C2=A0 GLOBAL_REMOVE_IF_UNREFERENCED UINTN=C2=A0 mVectorHandoffInfoCoun= t =3D
> sizeof (mVectorHandoffInfoDebugAgent) / sizeof
> (EFI_VECTOR_HANDOFF_INFO);
>
> -/**
> -=C2=A0 Calculate CRC16 for target data.
> -
> -=C2=A0 @param[in]=C2=A0 Data=C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0= =C2=A0 The target data.
> -=C2=A0 @param[in]=C2=A0 DataSize=C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 Th= e target data size.
> -=C2=A0 @param[in]=C2=A0 Crc=C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 = =C2=A0 =C2=A0Initial CRC.
> -
> -=C2=A0 @return UINT16=C2=A0 =C2=A0 =C2=A0The CRC16 value.
> -
> -**/
> -UINT16
> -CalculateCrc16 (
> -=C2=A0 IN UINT8=C2=A0 =C2=A0*Data,
> -=C2=A0 IN UINTN=C2=A0 =C2=A0DataSize,
> -=C2=A0 IN UINT16=C2=A0 Crc
> -=C2=A0 )
> -{
> -=C2=A0 UINTN=C2=A0 Index;
> -=C2=A0 UINTN=C2=A0 BitIndex;
> -
> -=C2=A0 for (Index =3D 0; Index < DataSize; Index++) {
> -=C2=A0 =C2=A0 Crc ^=3D (UINT16)Data[Index];
> -=C2=A0 =C2=A0 for (BitIndex =3D 0; BitIndex < 8; BitIndex++) {
> -=C2=A0 =C2=A0 =C2=A0 if ((Crc & 0x8000) !=3D 0) {
> -=C2=A0 =C2=A0 =C2=A0 =C2=A0 Crc <<=3D 1;
> -=C2=A0 =C2=A0 =C2=A0 =C2=A0 Crc=C2=A0 ^=3D 0x1021;
> -=C2=A0 =C2=A0 =C2=A0 } else {
> -=C2=A0 =C2=A0 =C2=A0 =C2=A0 Crc <<=3D 1;
> -=C2=A0 =C2=A0 =C2=A0 }
> -=C2=A0 =C2=A0 }
> -=C2=A0 }
> -
> -=C2=A0 return Crc;
> -}
> -
>=C2=A0 /**
>=C2=A0 =C2=A0 Read IDT entry to check if IDT entries are setup by Debug= Agent.
>
> --
> 2.35.1



--
Pedro Falcato
--0000000000009c03c905db3d8582--