Re: [edk2] [Patch] ShellPkg: update ping to use timer service instead of timer arch protocol .

[Ni, Ruiyu]

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

>
>-Original Message-
>From: edk2-devel [mailto:edk2-devel-boun...@lists.01.org] On Behalf Of Fu 
>Siyuan
>Sent: Monday, October 17, 2016 2:22 PM
>To: edk2-devel@lists.01.org
>Cc: Ni, Ruiyu <ruiyu...@intel.com>; Ye, Ting <ting...@intel.com>; Zhang, Lubo 
><lubo.zh...@intel.com>
>Subject: [edk2] [Patch] ShellPkg: update ping to use timer service instead of 
>timer arch protocol .
>
>This patch update the shell ping command to use timer service to calculate the
>RTT time, instead of using the timer arch protocol.
>
>Contributed-under: TianoCore Contribution Agreement 1.0
>Signed-off-by: Fu Siyuan <siyuan...@intel.com>
>Cc: Ni Ruiyu <ruiyu...@intel.com>
>Cc: Ye Ting <ting...@intel.com>
>Cc: Zhang Lubo <lubo.zh...@intel.com>
>---
> .../Library/UefiShellNetwork1CommandsLib/Ping.c| 233 +++--
> .../UefiShellNetwork1CommandsLib.inf   |   3 +-
> .../UefiShellNetwork1CommandsLib.uni   |   4 +-
> 3 files changed, 171 insertions(+), 69 deletions(-)
>
>diff --git a/ShellPkg/Library/UefiShellNetwork1CommandsLib/Ping.c
>b/ShellPkg/Library/UefiShellNetwork1CommandsLib/Ping.c
>index 38625fe..2e1e878 100644
>--- a/ShellPkg/Library/UefiShellNetwork1CommandsLib/Ping.c
>+++ b/ShellPkg/Library/UefiShellNetwork1CommandsLib/Ping.c
>@@ -86,7 +86,7 @@ typedef struct _ICMPX_ECHO_REQUEST_REPLY {
>   UINT16  Checksum;
>   UINT16  Identifier;
>   UINT16  SequenceNum;
>-  UINT64  TimeStamp;
>+  UINT32  TimeStamp;
>   UINT8   Data[1];
> } ICMPX_ECHO_REQUEST_REPLY;
> #pragma pack()
>@@ -94,7 +94,7 @@ typedef struct _ICMPX_ECHO_REQUEST_REPLY {
> typedef struct _PING_ICMP_TX_INFO {
>   LIST_ENTRYLink;
>   UINT16SequenceNum;
>-  UINT64TimeStamp;
>+  UINT32TimeStamp;
>   PING_IPX_COMPLETION_TOKEN *Token;
> } PING_ICMPX_TX_INFO;
>
>@@ -109,6 +109,7 @@ typedef struct _PING_ICMP_TX_INFO {
> #define DEFAULT_BUFFER_SIZE   16
> #define ICMP_V4_ECHO_REQUEST  0x8
> #define ICMP_V4_ECHO_REPLY0x0
>+#define STALL_1_MILLI_SECOND  1000
>
> #define PING_PRIVATE_DATA_SIGNATURE  SIGNATURE_32 ('P', 'i', 'n', 'g')
> typedef struct _PING_PRIVATE_DATA {
>@@ -117,6 +118,10 @@ typedef struct _PING_PRIVATE_DATA {
>   EFI_HANDLE  IpChildHandle;
>   EFI_EVENT   Timer;
>
>+  UINT32  TimerPeriod;
>+  UINT32  RttTimerTick;
>+  EFI_EVENT   RttTimer;
>+
>   EFI_STATUS  Status;
>   LIST_ENTRY  TxList;
>   UINT16  RxCount;
>@@ -221,93 +226,186 @@ STATIC CONST SHELL_PARAM_ITEMPingParamList[] = {
> //
> STATIC CONST CHAR16  *mDstString;
> STATIC CONST CHAR16  *mSrcString;
>-STATIC UINT64mFrequency = 0;
> EFI_CPU_ARCH_PROTOCOL*gCpu = NULL;
>
> /**
>-  Read the current time.
>+  RTT timer tick routine.
>+
>+  @param[in]EventA EFI_EVENT type event.
>+  @param[in]Context  The pointer to Context.
>
>-  @retval the current tick value.
> **/
>-UINT64
>-ReadTime (
>+VOID
>+EFIAPI
>+RttTimerTickRoutine (
>+  IN EFI_EVENTEvent,
>+  IN VOID *Context
>+  )
>+{
>+  UINT32 *RttTimerTick;
>+
>+  RttTimerTick = (UINT32*) Context;
>+  (*RttTimerTick)++;
>+}
>+
>+/**
>+  Get the timer period of the system.
>+
>+  This function tries to get the system timer period by creating
>+  an 1ms period timer.
>+
>+  @return System timer period in MS, or 0 if operation failed.
>+
>+**/
>+UINT32
>+GetTimerPeriod(
>   VOID
>   )
> {
>-  UINT64 TimerPeriod;
>-  EFI_STATUS Status;
>+  EFI_STATUS Status;
>+  UINT32 RttTimerTick;
>+  EFI_EVENT  TimerEvent;
>+  UINT32 StallCounter;
>+  EFI_TPLOldTpl;
>
>-  ASSERT (gCpu != NULL);
>+  RttTimerTick = 0;
>+  StallCounter   = 0;
>
>-  Status = gCpu->GetTimerValue (gCpu, 0, , );
>+  Status = gBS->CreateEvent (
>+  EVT_TIMER | EVT_NOTIFY_SIGNAL,
>+  TPL_NOTIFY,
>+  RttTimerTickRoutine,
>+  ,
>+  
>+  );
>   if (EFI_ERROR (Status)) {
>-//
>-// The WinntGetTimerValue will return EFI_UNSUPPORTED. Set the
>-// TimerPeriod by ourselves.
>-//
>-mCurrentTick += 100;
>+return 0;

Re: [edk2] [Patch] ShellPkg: update ping to use timer service instead of timer arch protocol .

[Hegde, Nagaraj P]

Thanks Siyuan.
Reviewed-by: Hegde, Nagaraj P <nagaraj-p.he...@hpe.com>

Regards,
Nagaraj.

-Original Message-
From: Fu, Siyuan [mailto:siyuan...@intel.com] 
Sent: Thursday, October 20, 2016 8:32 AM
To: Hegde, Nagaraj P <nagaraj-p.he...@hpe.com>; edk2-devel@lists.01.org
Cc: Ni, Ruiyu <ruiyu...@intel.com>; Ye, Ting <ting...@intel.com>; Zhang, Lubo 
<lubo.zh...@intel.com>
Subject: RE: [edk2] [Patch] ShellPkg: update ping to use timer service instead 
of timer arch protocol .

Hi, Nagaraj

The 0~2ms doesn't mean 0.2ms, it means 0 to 2 ms. It's not an accurate time 
because we are using the timer event service and gBS->Stall to estimate the 
system timer period, see the new added GetTimerPeriod() function. It first 
create a period timer with 1ms interval, and use a Stall(1m) to wait in a while 
loop until the 1ms timer is triggered 10 times, and finally return the estimate 
system timer period by StallCounter / RttTimerTick.

You got "0~2ms" result because you platform may set the timer period to 2ms (or 
bigger), and the minimum time interval we could count by the timer service is 
2ms, so we use 0~2ms, means the reply is arrived within 2ms.

The reason we make these change is because the UEFI shell is not allowed to use 
neither PI protocol nor platform specific library, so there is no standard way 
to get the real time period only by UEFI service. It's a little tricky, if you 
have better idea I'm glad to hear that.

I will make another patch to fix the ttl print issue, and also the ping6 
update. Thanks.

BestRegards
Fu Siyuan

> -Original Message-
> From: edk2-devel [mailto:edk2-devel-boun...@lists.01.org] On Behalf Of 
> Hegde, Nagaraj P
> Sent: Wednesday, October 19, 2016 6:38 PM
> To: Fu, Siyuan <siyuan...@intel.com>; edk2-devel@lists.01.org
> Cc: Ni, Ruiyu <ruiyu...@intel.com>; Ye, Ting <ting...@intel.com>; 
> Zhang, Lubo <lubo.zh...@intel.com>
> Subject: Re: [edk2] [Patch] ShellPkg: update ping to use timer service 
> instead of timer arch protocol .
> 
> Hi Siyuan,
> 
> Few comments:
> 
> 1. We need the same code change for Ping6
> (ShellPkg/Library/UefiShellNetwork2CommandsLib/Ping6.c)
> 2. From the patch that I could view, in STR_PING_REPLY_INFO, we print 
> time as 0~2ms for 0.2 ms. Why not use "." itself? Any reason for using tild 
> (~)?
> 3. We print ttl=%d in STR_PING_REPLY_INFO, which is always 0 for IPv4 Ping.
> We need to fix that.
> 
> I have tested your code and looks fine.
> 
> Regards,
> Nagaraj.
> 
> -Original Message-
> From: edk2-devel [mailto:edk2-devel-boun...@lists.01.org] On Behalf Of 
> Fu Siyuan
> Sent: Monday, October 17, 2016 11:52 AM
> To: edk2-devel@lists.01.org
> Cc: Ni Ruiyu <ruiyu...@intel.com>; Ye Ting <ting...@intel.com>; Zhang 
> Lubo <lubo.zh...@intel.com>
> Subject: [edk2] [Patch] ShellPkg: update ping to use timer service 
> instead of timer arch protocol .
> 
> This patch update the shell ping command to use timer service to 
> calculate the RTT time, instead of using the timer arch protocol.
> 
> Contributed-under: TianoCore Contribution Agreement 1.0
> Signed-off-by: Fu Siyuan <siyuan...@intel.com>
> Cc: Ni Ruiyu <ruiyu...@intel.com>
> Cc: Ye Ting <ting...@intel.com>
> Cc: Zhang Lubo <lubo.zh...@intel.com>
> ---
>  .../Library/UefiShellNetwork1CommandsLib/Ping.c| 233 +++-
> -
>  .../UefiShellNetwork1CommandsLib.inf   |   3 +-
>  .../UefiShellNetwork1CommandsLib.uni   |   4 +-
>  3 files changed, 171 insertions(+), 69 deletions(-)
> 
> diff --git a/ShellPkg/Library/UefiShellNetwork1CommandsLib/Ping.c
> b/ShellPkg/Library/UefiShellNetwork1CommandsLib/Ping.c
> index 38625fe..2e1e878 100644
> --- a/ShellPkg/Library/UefiShellNetwork1CommandsLib/Ping.c
> +++ b/ShellPkg/Library/UefiShellNetwork1CommandsLib/Ping.c
> @@ -86,7 +86,7 @@ typedef struct _ICMPX_ECHO_REQUEST_REPLY {
>UINT16  Checksum;
>UINT16  Identifier;
>UINT16  SequenceNum;
> -  UINT64  TimeStamp;
> +  UINT32  TimeStamp;
>UINT8   Data[1];
>  } ICMPX_ECHO_REQUEST_REPLY;
>  #pragma pack()
> @@ -94,7 +94,7 @@ typedef struct _ICMPX_ECHO_REQUEST_REPLY {  typedef 
> struct _PING_ICMP_TX_INFO {
>LIST_ENTRYLink;
>UINT16SequenceNum;
> -  UINT64TimeStamp;
> +  UINT32TimeStamp;
>PING_IPX_COMPLETION_TOKEN *Token;
>  } PING_ICMPX_TX_INFO;
> 
> @@ -109,6 +109,7 @@ typedef struct _PING_ICMP_TX_INFO {
>  #define DEFAULT_BUFFER_SIZE   16
>  #define ICMP_V4_ECHO_REQUEST  0x8

Re: [edk2] [Patch] ShellPkg: update ping to use timer service instead of timer arch protocol .

[Fu, Siyuan]

Hi, Nagaraj

The 0~2ms doesn't mean 0.2ms, it means 0 to 2 ms. It's not an accurate time 
because we are using the timer event service and gBS->Stall to estimate the 
system timer period, see the new added GetTimerPeriod() function. It first 
create a period timer with 1ms interval, and use a Stall(1m) to wait in a while 
loop until the 1ms timer is triggered 10 times, and finally return the estimate 
system timer period by StallCounter / RttTimerTick.

You got "0~2ms" result because you platform may set the timer period to 2ms (or 
bigger), and the minimum time interval we could count by the timer service is 
2ms, so we use 0~2ms, means the reply is arrived within 2ms.

The reason we make these change is because the UEFI shell is not allowed to use 
neither PI protocol nor platform specific library, so there is no standard way 
to get the real time period only by UEFI service. It's a little tricky, if you 
have better idea I'm glad to hear that.

I will make another patch to fix the ttl print issue, and also the ping6 
update. Thanks.

BestRegards
Fu Siyuan

> -Original Message-
> From: edk2-devel [mailto:edk2-devel-boun...@lists.01.org] On Behalf Of
> Hegde, Nagaraj P
> Sent: Wednesday, October 19, 2016 6:38 PM
> To: Fu, Siyuan <siyuan...@intel.com>; edk2-devel@lists.01.org
> Cc: Ni, Ruiyu <ruiyu...@intel.com>; Ye, Ting <ting...@intel.com>; Zhang,
> Lubo <lubo.zh...@intel.com>
> Subject: Re: [edk2] [Patch] ShellPkg: update ping to use timer service
> instead of timer arch protocol .
> 
> Hi Siyuan,
> 
> Few comments:
> 
> 1. We need the same code change for Ping6
> (ShellPkg/Library/UefiShellNetwork2CommandsLib/Ping6.c)
> 2. From the patch that I could view, in STR_PING_REPLY_INFO, we print time
> as 0~2ms for 0.2 ms. Why not use "." itself? Any reason for using tild (~)?
> 3. We print ttl=%d in STR_PING_REPLY_INFO, which is always 0 for IPv4 Ping.
> We need to fix that.
> 
> I have tested your code and looks fine.
> 
> Regards,
> Nagaraj.
> 
> -Original Message-
> From: edk2-devel [mailto:edk2-devel-boun...@lists.01.org] On Behalf Of Fu
> Siyuan
> Sent: Monday, October 17, 2016 11:52 AM
> To: edk2-devel@lists.01.org
> Cc: Ni Ruiyu <ruiyu...@intel.com>; Ye Ting <ting...@intel.com>; Zhang Lubo
> <lubo.zh...@intel.com>
> Subject: [edk2] [Patch] ShellPkg: update ping to use timer service instead
> of timer arch protocol .
> 
> This patch update the shell ping command to use timer service to calculate
> the RTT time, instead of using the timer arch protocol.
> 
> Contributed-under: TianoCore Contribution Agreement 1.0
> Signed-off-by: Fu Siyuan <siyuan...@intel.com>
> Cc: Ni Ruiyu <ruiyu...@intel.com>
> Cc: Ye Ting <ting...@intel.com>
> Cc: Zhang Lubo <lubo.zh...@intel.com>
> ---
>  .../Library/UefiShellNetwork1CommandsLib/Ping.c| 233 +++-
> -
>  .../UefiShellNetwork1CommandsLib.inf   |   3 +-
>  .../UefiShellNetwork1CommandsLib.uni   |   4 +-
>  3 files changed, 171 insertions(+), 69 deletions(-)
> 
> diff --git a/ShellPkg/Library/UefiShellNetwork1CommandsLib/Ping.c
> b/ShellPkg/Library/UefiShellNetwork1CommandsLib/Ping.c
> index 38625fe..2e1e878 100644
> --- a/ShellPkg/Library/UefiShellNetwork1CommandsLib/Ping.c
> +++ b/ShellPkg/Library/UefiShellNetwork1CommandsLib/Ping.c
> @@ -86,7 +86,7 @@ typedef struct _ICMPX_ECHO_REQUEST_REPLY {
>UINT16  Checksum;
>UINT16  Identifier;
>UINT16  SequenceNum;
> -  UINT64  TimeStamp;
> +  UINT32  TimeStamp;
>UINT8   Data[1];
>  } ICMPX_ECHO_REQUEST_REPLY;
>  #pragma pack()
> @@ -94,7 +94,7 @@ typedef struct _ICMPX_ECHO_REQUEST_REPLY {  typedef
> struct _PING_ICMP_TX_INFO {
>LIST_ENTRYLink;
>UINT16SequenceNum;
> -  UINT64TimeStamp;
> +  UINT32TimeStamp;
>PING_IPX_COMPLETION_TOKEN *Token;
>  } PING_ICMPX_TX_INFO;
> 
> @@ -109,6 +109,7 @@ typedef struct _PING_ICMP_TX_INFO {
>  #define DEFAULT_BUFFER_SIZE   16
>  #define ICMP_V4_ECHO_REQUEST  0x8
>  #define ICMP_V4_ECHO_REPLY0x0
> +#define STALL_1_MILLI_SECOND  1000
> 
>  #define PING_PRIVATE_DATA_SIGNATURE  SIGNATURE_32 ('P', 'i', 'n', 'g')
> typedef struct _PING_PRIVATE_DATA { @@ -117,6 +118,10 @@ typedef struct
> _PING_PRIVATE_DATA {
>EFI_HANDLE  IpChildHandle;
>EFI_EVENT   Timer;
> 
> +  UINT32  TimerPeriod;
> +  UINT32  RttTimerTick;
> +  EFI_EVENT   RttTimer;
> +
>EFI_STATU

[edk2] [Patch] ShellPkg: update ping to use timer service instead of timer arch protocol .

[Fu Siyuan]

This patch update the shell ping command to use timer service to calculate the
RTT time, instead of using the timer arch protocol.

Contributed-under: TianoCore Contribution Agreement 1.0
Signed-off-by: Fu Siyuan 
Cc: Ni Ruiyu 
Cc: Ye Ting 
Cc: Zhang Lubo 
---
 .../Library/UefiShellNetwork1CommandsLib/Ping.c| 233 +++--
 .../UefiShellNetwork1CommandsLib.inf   |   3 +-
 .../UefiShellNetwork1CommandsLib.uni   |   4 +-
 3 files changed, 171 insertions(+), 69 deletions(-)

diff --git a/ShellPkg/Library/UefiShellNetwork1CommandsLib/Ping.c 
b/ShellPkg/Library/UefiShellNetwork1CommandsLib/Ping.c
index 38625fe..2e1e878 100644
--- a/ShellPkg/Library/UefiShellNetwork1CommandsLib/Ping.c
+++ b/ShellPkg/Library/UefiShellNetwork1CommandsLib/Ping.c
@@ -86,7 +86,7 @@ typedef struct _ICMPX_ECHO_REQUEST_REPLY {
   UINT16  Checksum;
   UINT16  Identifier;
   UINT16  SequenceNum;
-  UINT64  TimeStamp;
+  UINT32  TimeStamp;
   UINT8   Data[1];
 } ICMPX_ECHO_REQUEST_REPLY;
 #pragma pack()
@@ -94,7 +94,7 @@ typedef struct _ICMPX_ECHO_REQUEST_REPLY {
 typedef struct _PING_ICMP_TX_INFO {
   LIST_ENTRYLink;
   UINT16SequenceNum;
-  UINT64TimeStamp;
+  UINT32TimeStamp;
   PING_IPX_COMPLETION_TOKEN *Token;
 } PING_ICMPX_TX_INFO;
 
@@ -109,6 +109,7 @@ typedef struct _PING_ICMP_TX_INFO {
 #define DEFAULT_BUFFER_SIZE   16
 #define ICMP_V4_ECHO_REQUEST  0x8
 #define ICMP_V4_ECHO_REPLY0x0
+#define STALL_1_MILLI_SECOND  1000
 
 #define PING_PRIVATE_DATA_SIGNATURE  SIGNATURE_32 ('P', 'i', 'n', 'g')
 typedef struct _PING_PRIVATE_DATA {
@@ -117,6 +118,10 @@ typedef struct _PING_PRIVATE_DATA {
   EFI_HANDLE  IpChildHandle;
   EFI_EVENT   Timer;
 
+  UINT32  TimerPeriod;
+  UINT32  RttTimerTick;   
+  EFI_EVENT   RttTimer;
+
   EFI_STATUS  Status;
   LIST_ENTRY  TxList;
   UINT16  RxCount;
@@ -221,93 +226,186 @@ STATIC CONST SHELL_PARAM_ITEMPingParamList[] = {
 //
 STATIC CONST CHAR16  *mDstString;
 STATIC CONST CHAR16  *mSrcString;
-STATIC UINT64mFrequency = 0;
 EFI_CPU_ARCH_PROTOCOL*gCpu = NULL;
 
 /**
-  Read the current time.
+  RTT timer tick routine.
+
+  @param[in]EventA EFI_EVENT type event.
+  @param[in]Context  The pointer to Context.
 
-  @retval the current tick value.
 **/
-UINT64
-ReadTime (
+VOID
+EFIAPI
+RttTimerTickRoutine (
+  IN EFI_EVENTEvent,
+  IN VOID *Context
+  )
+{
+  UINT32 *RttTimerTick;
+
+  RttTimerTick = (UINT32*) Context;
+  (*RttTimerTick)++;
+}
+
+/**
+  Get the timer period of the system.
+
+  This function tries to get the system timer period by creating
+  an 1ms period timer.
+
+  @return System timer period in MS, or 0 if operation failed.
+
+**/
+UINT32
+GetTimerPeriod(
   VOID
   )
 {
-  UINT64 TimerPeriod;
-  EFI_STATUS Status;
+  EFI_STATUS Status;
+  UINT32 RttTimerTick;
+  EFI_EVENT  TimerEvent;
+  UINT32 StallCounter;
+  EFI_TPLOldTpl;
 
-  ASSERT (gCpu != NULL);
+  RttTimerTick = 0;
+  StallCounter   = 0;
 
-  Status = gCpu->GetTimerValue (gCpu, 0, , );
+  Status = gBS->CreateEvent (
+  EVT_TIMER | EVT_NOTIFY_SIGNAL,
+  TPL_NOTIFY,
+  RttTimerTickRoutine,
+  ,
+  
+  );
   if (EFI_ERROR (Status)) {
-//
-// The WinntGetTimerValue will return EFI_UNSUPPORTED. Set the
-// TimerPeriod by ourselves.
-//
-mCurrentTick += 100;
+return 0;
+  }
+
+  OldTpl = gBS->RaiseTPL (TPL_CALLBACK);
+  Status = gBS->SetTimer (
+  TimerEvent,
+  TimerPeriodic,
+  TICKS_PER_MS
+  );
+  if (EFI_ERROR (Status)) {
+gBS->CloseEvent (TimerEvent);
+return 0;
+  }
+
+  while (RttTimerTick < 10) {
+gBS->Stall (STALL_1_MILLI_SECOND);
+++StallCounter;
   }
-  
-  return mCurrentTick;
-}
 
+  gBS->RestoreTPL (OldTpl);
+
+  gBS->SetTimer (TimerEvent, TimerCancel, 0);
+  gBS->CloseEvent (TimerEvent);
+
+  return StallCounter / RttTimerTick;
+}
 
 /**
-  Get and calculate the frequency in ticks/ms.
-  The result is saved in the global variable mFrequency
+  Initialize the timer event for RTT (round trip time).
 
-  @retval EFI_SUCCESSCalculated the frequency successfully.
-  @retval Others Failed to calculate the frequency.
+  @param[in]PrivateThe pointer to PING_PRIVATE_DATA.
+
+  @retval EFI_SUCCESS  RTT timer is started.
+  @retval Others   Failed to start the RTT