public inbox for devel@edk2.groups.io
 help / color / mirror / Atom feed
* [PATCH] EmulatorPkg: Fix bugs about MiscBootServices
@ 2019-05-24  8:17 Zhiguang Liu
  2019-05-24  9:08 ` Ni, Ray
  0 siblings, 1 reply; 3+ messages in thread
From: Zhiguang Liu @ 2019-05-24  8:17 UTC (permalink / raw)
  To: devel; +Cc: Jordan Justen, Andrew Fish, Ray Ni

REF:https://bugzilla.tianocore.org/show_bug.cgi?id=1757

This patch fix two issues.
1.
  The previous code "NanoSecondDelay (MicroSeconds * 1000)"
  may cause data overflow.
2.
  Delete some code in
    /EmulatorPkg/ResetRuntimeDxe/Reset.c
  which may cause Tpl problems when invoked by watchdog.
  I think it is ok to delete these code, because it will
  be more like what NT32 does.

Signed-off-by: Zhiguang Liu <zhiguang.liu@intel.com>
Cc: Jordan Justen <jordan.l.justen@intel.com>
Cc: Andrew Fish <afish@apple.com>
Cc: Ray Ni <ray.ni@intel.com>
---
 EmulatorPkg/Library/DxeCoreTimerLib/DxeCoreTimerLib.c | 14 +++++++++++++-
 EmulatorPkg/Library/DxeTimerLib/DxeTimerLib.c         | 14 +++++++++++++-
 EmulatorPkg/Library/PeiTimerLib/PeiTimerLib.c         | 14 +++++++++++++-
 EmulatorPkg/ResetRuntimeDxe/Reset.c                   | 24 ------------------------
 4 files changed, 39 insertions(+), 27 deletions(-)

diff --git a/EmulatorPkg/Library/DxeCoreTimerLib/DxeCoreTimerLib.c b/EmulatorPkg/Library/DxeCoreTimerLib/DxeCoreTimerLib.c
index c331cbba9c..813963de7b 100644
--- a/EmulatorPkg/Library/DxeCoreTimerLib/DxeCoreTimerLib.c
+++ b/EmulatorPkg/Library/DxeCoreTimerLib/DxeCoreTimerLib.c
@@ -33,7 +33,19 @@ MicroSecondDelay (
   IN      UINTN                     MicroSeconds
   )
 {
-  return NanoSecondDelay (MicroSeconds * 1000);
+  UINTN             Remainder;
+  UINTN             Counter;
+  UINTN             Index;
+  Counter = (UINTN) DivU64x32Remainder (
+                MultU64x32 (MicroSeconds, 1000),
+                0xffffffff,
+                &Remainder
+                );
+  for (Index = 0; Index < Counter; Index++) {
+    NanoSecondDelay (0xffffffff);
+  }
+  NanoSecondDelay (Remainder);
+  return MicroSeconds;
 }
 
 
diff --git a/EmulatorPkg/Library/DxeTimerLib/DxeTimerLib.c b/EmulatorPkg/Library/DxeTimerLib/DxeTimerLib.c
index 14cae4214c..590ce55fae 100644
--- a/EmulatorPkg/Library/DxeTimerLib/DxeTimerLib.c
+++ b/EmulatorPkg/Library/DxeTimerLib/DxeTimerLib.c
@@ -64,7 +64,19 @@ MicroSecondDelay (
   IN      UINTN                     MicroSeconds
   )
 {
-  return NanoSecondDelay (MicroSeconds * 1000);
+  UINTN             Remainder;
+  UINTN             Counter;
+  UINTN             Index;
+  Counter = (UINTN) DivU64x32Remainder (
+                MultU64x32 (MicroSeconds, 1000),
+                0xffffffff,
+                &Remainder
+                );
+  for (Index = 0; Index < Counter; Index++) {
+    NanoSecondDelay (0xffffffff);
+  }
+  NanoSecondDelay (Remainder);
+  return MicroSeconds;
 }
 
 
diff --git a/EmulatorPkg/Library/PeiTimerLib/PeiTimerLib.c b/EmulatorPkg/Library/PeiTimerLib/PeiTimerLib.c
index cce46fb366..dcac32a51f 100644
--- a/EmulatorPkg/Library/PeiTimerLib/PeiTimerLib.c
+++ b/EmulatorPkg/Library/PeiTimerLib/PeiTimerLib.c
@@ -30,7 +30,19 @@ MicroSecondDelay (
   IN      UINTN                     MicroSeconds
   )
 {
-  return NanoSecondDelay (MicroSeconds * 1000);
+  UINTN             Remainder;
+  UINTN             Counter;
+  UINTN             Index;
+  Counter = (UINTN) DivU64x32Remainder (
+                MultU64x32 (MicroSeconds, 1000),
+                0xffffffff,
+                &Remainder
+                );
+  for (Index = 0; Index < Counter; Index++) {
+    NanoSecondDelay (0xffffffff);
+  }
+  NanoSecondDelay (Remainder);
+  return MicroSeconds;
 }
 
 /**
diff --git a/EmulatorPkg/ResetRuntimeDxe/Reset.c b/EmulatorPkg/ResetRuntimeDxe/Reset.c
index 19504825c9..9439f9ccff 100644
--- a/EmulatorPkg/ResetRuntimeDxe/Reset.c
+++ b/EmulatorPkg/ResetRuntimeDxe/Reset.c
@@ -29,30 +29,6 @@ EmuResetSystem (
   IN VOID             *ResetData OPTIONAL
   )
 {
-  EFI_STATUS  Status;
-  UINTN       HandleCount;
-  EFI_HANDLE  *HandleBuffer;
-  UINTN       Index;
-
-  //
-  // Disconnect all
-  //
-  Status = gBS->LocateHandleBuffer (
-                  AllHandles,
-                  NULL,
-                  NULL,
-                  &HandleCount,
-                  &HandleBuffer
-                  );
-  if (!EFI_ERROR (Status)) {
-    for (Index = 0; Index < HandleCount; Index++) {
-      Status = gBS->DisconnectController (HandleBuffer[Index], NULL, NULL);
-    }
-
-    gBS->FreePool (HandleBuffer);
-  }
-
-
   //
   // Discard ResetType, always return 0 as exit code
   //
-- 
2.21.0.windows.1


^ permalink raw reply related	[flat|nested] 3+ messages in thread

* Re: [PATCH] EmulatorPkg: Fix bugs about MiscBootServices
  2019-05-24  8:17 [PATCH] EmulatorPkg: Fix bugs about MiscBootServices Zhiguang Liu
@ 2019-05-24  9:08 ` Ni, Ray
  2019-05-27  0:31   ` Zhiguang Liu
  0 siblings, 1 reply; 3+ messages in thread
From: Ni, Ray @ 2019-05-24  9:08 UTC (permalink / raw)
  To: Liu, Zhiguang, devel@edk2.groups.io; +Cc: Justen, Jordan L, Andrew Fish

Zhiguang,
For the first issue, where does the fix come from?
Or you did the implementation yourself?

I ask this because I want to make sure the code is
from an existing tested implementation.

Thanks,
Ray

> -----Original Message-----
> From: Liu, Zhiguang
> Sent: Friday, May 24, 2019 4:18 PM
> To: devel@edk2.groups.io
> Cc: Justen, Jordan L <jordan.l.justen@intel.com>; Andrew Fish
> <afish@apple.com>; Ni, Ray <ray.ni@intel.com>
> Subject: [PATCH] EmulatorPkg: Fix bugs about MiscBootServices
> 
> REF:https://bugzilla.tianocore.org/show_bug.cgi?id=1757
> 
> This patch fix two issues.
> 1.
>   The previous code "NanoSecondDelay (MicroSeconds * 1000)"
>   may cause data overflow.
> 2.
>   Delete some code in
>     /EmulatorPkg/ResetRuntimeDxe/Reset.c
>   which may cause Tpl problems when invoked by watchdog.
>   I think it is ok to delete these code, because it will
>   be more like what NT32 does.
> 
> Signed-off-by: Zhiguang Liu <zhiguang.liu@intel.com>
> Cc: Jordan Justen <jordan.l.justen@intel.com>
> Cc: Andrew Fish <afish@apple.com>
> Cc: Ray Ni <ray.ni@intel.com>
> ---
>  EmulatorPkg/Library/DxeCoreTimerLib/DxeCoreTimerLib.c | 14
> +++++++++++++-
>  EmulatorPkg/Library/DxeTimerLib/DxeTimerLib.c         | 14 +++++++++++++-
>  EmulatorPkg/Library/PeiTimerLib/PeiTimerLib.c         | 14 +++++++++++++-
>  EmulatorPkg/ResetRuntimeDxe/Reset.c                   | 24 ------------------------
>  4 files changed, 39 insertions(+), 27 deletions(-)
> 
> diff --git a/EmulatorPkg/Library/DxeCoreTimerLib/DxeCoreTimerLib.c
> b/EmulatorPkg/Library/DxeCoreTimerLib/DxeCoreTimerLib.c
> index c331cbba9c..813963de7b 100644
> --- a/EmulatorPkg/Library/DxeCoreTimerLib/DxeCoreTimerLib.c
> +++ b/EmulatorPkg/Library/DxeCoreTimerLib/DxeCoreTimerLib.c
> @@ -33,7 +33,19 @@ MicroSecondDelay (
>    IN      UINTN                     MicroSeconds
>    )
>  {
> -  return NanoSecondDelay (MicroSeconds * 1000);
> +  UINTN             Remainder;
> +  UINTN             Counter;
> +  UINTN             Index;
> +  Counter = (UINTN) DivU64x32Remainder (
> +                MultU64x32 (MicroSeconds, 1000),
> +                0xffffffff,
> +                &Remainder
> +                );
> +  for (Index = 0; Index < Counter; Index++) {
> +    NanoSecondDelay (0xffffffff);
> +  }
> +  NanoSecondDelay (Remainder);
> +  return MicroSeconds;
>  }
> 
> 
> diff --git a/EmulatorPkg/Library/DxeTimerLib/DxeTimerLib.c
> b/EmulatorPkg/Library/DxeTimerLib/DxeTimerLib.c
> index 14cae4214c..590ce55fae 100644
> --- a/EmulatorPkg/Library/DxeTimerLib/DxeTimerLib.c
> +++ b/EmulatorPkg/Library/DxeTimerLib/DxeTimerLib.c
> @@ -64,7 +64,19 @@ MicroSecondDelay (
>    IN      UINTN                     MicroSeconds
>    )
>  {
> -  return NanoSecondDelay (MicroSeconds * 1000);
> +  UINTN             Remainder;
> +  UINTN             Counter;
> +  UINTN             Index;
> +  Counter = (UINTN) DivU64x32Remainder (
> +                MultU64x32 (MicroSeconds, 1000),
> +                0xffffffff,
> +                &Remainder
> +                );
> +  for (Index = 0; Index < Counter; Index++) {
> +    NanoSecondDelay (0xffffffff);
> +  }
> +  NanoSecondDelay (Remainder);
> +  return MicroSeconds;
>  }
> 
> 
> diff --git a/EmulatorPkg/Library/PeiTimerLib/PeiTimerLib.c
> b/EmulatorPkg/Library/PeiTimerLib/PeiTimerLib.c
> index cce46fb366..dcac32a51f 100644
> --- a/EmulatorPkg/Library/PeiTimerLib/PeiTimerLib.c
> +++ b/EmulatorPkg/Library/PeiTimerLib/PeiTimerLib.c
> @@ -30,7 +30,19 @@ MicroSecondDelay (
>    IN      UINTN                     MicroSeconds
>    )
>  {
> -  return NanoSecondDelay (MicroSeconds * 1000);
> +  UINTN             Remainder;
> +  UINTN             Counter;
> +  UINTN             Index;
> +  Counter = (UINTN) DivU64x32Remainder (
> +                MultU64x32 (MicroSeconds, 1000),
> +                0xffffffff,
> +                &Remainder
> +                );
> +  for (Index = 0; Index < Counter; Index++) {
> +    NanoSecondDelay (0xffffffff);
> +  }
> +  NanoSecondDelay (Remainder);
> +  return MicroSeconds;
>  }
> 
>  /**
> diff --git a/EmulatorPkg/ResetRuntimeDxe/Reset.c
> b/EmulatorPkg/ResetRuntimeDxe/Reset.c
> index 19504825c9..9439f9ccff 100644
> --- a/EmulatorPkg/ResetRuntimeDxe/Reset.c
> +++ b/EmulatorPkg/ResetRuntimeDxe/Reset.c
> @@ -29,30 +29,6 @@ EmuResetSystem (
>    IN VOID             *ResetData OPTIONAL
>    )
>  {
> -  EFI_STATUS  Status;
> -  UINTN       HandleCount;
> -  EFI_HANDLE  *HandleBuffer;
> -  UINTN       Index;
> -
> -  //
> -  // Disconnect all
> -  //
> -  Status = gBS->LocateHandleBuffer (
> -                  AllHandles,
> -                  NULL,
> -                  NULL,
> -                  &HandleCount,
> -                  &HandleBuffer
> -                  );
> -  if (!EFI_ERROR (Status)) {
> -    for (Index = 0; Index < HandleCount; Index++) {
> -      Status = gBS->DisconnectController (HandleBuffer[Index], NULL, NULL);
> -    }
> -
> -    gBS->FreePool (HandleBuffer);
> -  }
> -
> -
>    //
>    // Discard ResetType, always return 0 as exit code
>    //
> --
> 2.21.0.windows.1


^ permalink raw reply	[flat|nested] 3+ messages in thread

* Re: [PATCH] EmulatorPkg: Fix bugs about MiscBootServices
  2019-05-24  9:08 ` Ni, Ray
@ 2019-05-27  0:31   ` Zhiguang Liu
  0 siblings, 0 replies; 3+ messages in thread
From: Zhiguang Liu @ 2019-05-27  0:31 UTC (permalink / raw)
  To: Ni, Ray, devel@edk2.groups.io; +Cc: Justen, Jordan L, Andrew Fish

Hello, Ray

I did the implementation myself.
I just found there did be the same function in
\Omap35xxPkg\Library\Omap35xxTimerLib\TimerLib.c
In old version.
Should I use the existing code like following 


\Omap35xxPkg\Library\Omap35xxTimerLib\TimerLib.c
UINTN
EFIAPI
MicroSecondDelay (
  IN  UINTN MicroSeconds
  )
{
  UINT64  NanoSeconds;

  NanoSeconds = MultU64x32(MicroSeconds, 1000);

  while (NanoSeconds > (UINTN)-1) {
    NanoSecondDelay((UINTN)-1);
    NanoSeconds -= (UINTN)-1;
  }

  NanoSecondDelay(NanoSeconds);

  return MicroSeconds;
}

Best Regards,
Zhiguang Liu


>-----Original Message-----
>From: Ni, Ray
>Sent: Friday, May 24, 2019 5:08 PM
>To: Liu, Zhiguang <zhiguang.liu@intel.com>; devel@edk2.groups.io
>Cc: Justen, Jordan L <jordan.l.justen@intel.com>; Andrew Fish
><afish@apple.com>
>Subject: RE: [PATCH] EmulatorPkg: Fix bugs about MiscBootServices
>
>Zhiguang,
>For the first issue, where does the fix come from?
>Or you did the implementation yourself?
>
>I ask this because I want to make sure the code is from an existing tested
>implementation.
>
>Thanks,
>Ray
>
>> -----Original Message-----
>> From: Liu, Zhiguang
>> Sent: Friday, May 24, 2019 4:18 PM
>> To: devel@edk2.groups.io
>> Cc: Justen, Jordan L <jordan.l.justen@intel.com>; Andrew Fish
>> <afish@apple.com>; Ni, Ray <ray.ni@intel.com>
>> Subject: [PATCH] EmulatorPkg: Fix bugs about MiscBootServices
>>
>> REF:https://bugzilla.tianocore.org/show_bug.cgi?id=1757
>>
>> This patch fix two issues.
>> 1.
>>   The previous code "NanoSecondDelay (MicroSeconds * 1000)"
>>   may cause data overflow.
>> 2.
>>   Delete some code in
>>     /EmulatorPkg/ResetRuntimeDxe/Reset.c
>>   which may cause Tpl problems when invoked by watchdog.
>>   I think it is ok to delete these code, because it will
>>   be more like what NT32 does.
>>
>> Signed-off-by: Zhiguang Liu <zhiguang.liu@intel.com>
>> Cc: Jordan Justen <jordan.l.justen@intel.com>
>> Cc: Andrew Fish <afish@apple.com>
>> Cc: Ray Ni <ray.ni@intel.com>
>> ---
>>  EmulatorPkg/Library/DxeCoreTimerLib/DxeCoreTimerLib.c | 14
>> +++++++++++++-
>>  EmulatorPkg/Library/DxeTimerLib/DxeTimerLib.c         | 14 +++++++++++++-
>>  EmulatorPkg/Library/PeiTimerLib/PeiTimerLib.c         | 14 +++++++++++++-
>>  EmulatorPkg/ResetRuntimeDxe/Reset.c                   | 24 ------------------------
>>  4 files changed, 39 insertions(+), 27 deletions(-)
>>
>> diff --git a/EmulatorPkg/Library/DxeCoreTimerLib/DxeCoreTimerLib.c
>> b/EmulatorPkg/Library/DxeCoreTimerLib/DxeCoreTimerLib.c
>> index c331cbba9c..813963de7b 100644
>> --- a/EmulatorPkg/Library/DxeCoreTimerLib/DxeCoreTimerLib.c
>> +++ b/EmulatorPkg/Library/DxeCoreTimerLib/DxeCoreTimerLib.c
>> @@ -33,7 +33,19 @@ MicroSecondDelay (
>>    IN      UINTN                     MicroSeconds
>>    )
>>  {
>> -  return NanoSecondDelay (MicroSeconds * 1000);
>> +  UINTN             Remainder;
>> +  UINTN             Counter;
>> +  UINTN             Index;
>> +  Counter = (UINTN) DivU64x32Remainder (
>> +                MultU64x32 (MicroSeconds, 1000),
>> +                0xffffffff,
>> +                &Remainder
>> +                );
>> +  for (Index = 0; Index < Counter; Index++) {
>> +    NanoSecondDelay (0xffffffff);
>> +  }
>> +  NanoSecondDelay (Remainder);
>> +  return MicroSeconds;
>>  }
>>
>>
>> diff --git a/EmulatorPkg/Library/DxeTimerLib/DxeTimerLib.c
>> b/EmulatorPkg/Library/DxeTimerLib/DxeTimerLib.c
>> index 14cae4214c..590ce55fae 100644
>> --- a/EmulatorPkg/Library/DxeTimerLib/DxeTimerLib.c
>> +++ b/EmulatorPkg/Library/DxeTimerLib/DxeTimerLib.c
>> @@ -64,7 +64,19 @@ MicroSecondDelay (
>>    IN      UINTN                     MicroSeconds
>>    )
>>  {
>> -  return NanoSecondDelay (MicroSeconds * 1000);
>> +  UINTN             Remainder;
>> +  UINTN             Counter;
>> +  UINTN             Index;
>> +  Counter = (UINTN) DivU64x32Remainder (
>> +                MultU64x32 (MicroSeconds, 1000),
>> +                0xffffffff,
>> +                &Remainder
>> +                );
>> +  for (Index = 0; Index < Counter; Index++) {
>> +    NanoSecondDelay (0xffffffff);
>> +  }
>> +  NanoSecondDelay (Remainder);
>> +  return MicroSeconds;
>>  }
>>
>>
>> diff --git a/EmulatorPkg/Library/PeiTimerLib/PeiTimerLib.c
>> b/EmulatorPkg/Library/PeiTimerLib/PeiTimerLib.c
>> index cce46fb366..dcac32a51f 100644
>> --- a/EmulatorPkg/Library/PeiTimerLib/PeiTimerLib.c
>> +++ b/EmulatorPkg/Library/PeiTimerLib/PeiTimerLib.c
>> @@ -30,7 +30,19 @@ MicroSecondDelay (
>>    IN      UINTN                     MicroSeconds
>>    )
>>  {
>> -  return NanoSecondDelay (MicroSeconds * 1000);
>> +  UINTN             Remainder;
>> +  UINTN             Counter;
>> +  UINTN             Index;
>> +  Counter = (UINTN) DivU64x32Remainder (
>> +                MultU64x32 (MicroSeconds, 1000),
>> +                0xffffffff,
>> +                &Remainder
>> +                );
>> +  for (Index = 0; Index < Counter; Index++) {
>> +    NanoSecondDelay (0xffffffff);
>> +  }
>> +  NanoSecondDelay (Remainder);
>> +  return MicroSeconds;
>>  }
>>
>>  /**
>> diff --git a/EmulatorPkg/ResetRuntimeDxe/Reset.c
>> b/EmulatorPkg/ResetRuntimeDxe/Reset.c
>> index 19504825c9..9439f9ccff 100644
>> --- a/EmulatorPkg/ResetRuntimeDxe/Reset.c
>> +++ b/EmulatorPkg/ResetRuntimeDxe/Reset.c
>> @@ -29,30 +29,6 @@ EmuResetSystem (
>>    IN VOID             *ResetData OPTIONAL
>>    )
>>  {
>> -  EFI_STATUS  Status;
>> -  UINTN       HandleCount;
>> -  EFI_HANDLE  *HandleBuffer;
>> -  UINTN       Index;
>> -
>> -  //
>> -  // Disconnect all
>> -  //
>> -  Status = gBS->LocateHandleBuffer (
>> -                  AllHandles,
>> -                  NULL,
>> -                  NULL,
>> -                  &HandleCount,
>> -                  &HandleBuffer
>> -                  );
>> -  if (!EFI_ERROR (Status)) {
>> -    for (Index = 0; Index < HandleCount; Index++) {
>> -      Status = gBS->DisconnectController (HandleBuffer[Index], NULL, NULL);
>> -    }
>> -
>> -    gBS->FreePool (HandleBuffer);
>> -  }
>> -
>> -
>>    //
>>    // Discard ResetType, always return 0 as exit code
>>    //
>> --
>> 2.21.0.windows.1


^ permalink raw reply	[flat|nested] 3+ messages in thread

end of thread, other threads:[~2019-05-27  0:31 UTC | newest]

Thread overview: 3+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2019-05-24  8:17 [PATCH] EmulatorPkg: Fix bugs about MiscBootServices Zhiguang Liu
2019-05-24  9:08 ` Ni, Ray
2019-05-27  0:31   ` Zhiguang Liu

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox