Re: [edk2] [PATCH 2/2] MdeModulePkg: CoreStall: improve accuracy in iterating branch

[Scott Duplichan]

Tian, Feng [mailto:feng.t...@intel.com] wrote:

]Hi, Andrew
]
]The change was proposed to solve incorrect truncation and overflow issue on 
old revision.
]
]The original code is:
]  Counter = (UINT32) DivU64x32Remainder (
]                       Microseconds * 10,
]                       gMetronome->TickPeriod,
]                       &Remainder
]                       );
]
]In original code, there is Microseconds * 10 which may be out of UINTN 
boundary. And Counter is forced to truncate to 32 bit,
]which may also bring issue.

It is good to see concern over integer overflow. So often it is just ignored. 
Here is an example. Which delay is longer:

MicroSecondDelay (5153376776577);
MicroSecondDelay (1);

Answer: the second one. The first malfunctions due to integer overflow.

Thanks,
Scott


]Thanks
]Feng
]
]-----Original Message-----
]From: Andrew Fish [mailto:af...@apple.com] 
]Sent: Tuesday, September 16, 2014 05:10
]To: edk2-devel@lists.sourceforge.net
]Subject: Re: [edk2] [PATCH 2/2] MdeModulePkg: CoreStall: improve accuracy in 
iterating branch
]
]
]On Sep 15, 2014, at 11:29 AM, Laszlo Ersek <ler...@redhat.com> wrote:
]
]> On 09/15/14 18:53, Andrew Fish wrote:
]>> 
]>> On Sep 15, 2014, at 12:00 AM, Tian, Feng <feng.t...@intel.com> wrote:
]>> 
]>>> Hi, Laszlo
]>>> 
]>>> Here is our intention for the logic:
]>>> 
]>>> If Microseconds is greater than or equal to 0x1999999999999999ULL(about 
58,494 years), then the divide operation is performed
]first, which means we will lose some accuracy, but this is a very large delay 
request, so waiting a little bit extra in this
]extreme case is an acceptable compromise.  
]>>> 
]>>> So I would prefer not to add this patch if there is no real impact.
]>>> 
]>> 
]>> Was this issue caught by some static analysis tool?
]> 
]> Nikolai submitted a patch that simply caused me to look at the 
]> function in question. In 2005-2006 I had written a UDP data transfer 
]> program that was obsessed with accurate packet rate, time keeping, and 
]> preventing long-term drift (I still use it occasionally, if the link 
]> quality justifies it); hence my interest in fractional ticks.
]> 
]
]Sorry I slotted this in the wrong location. I wanted to ask about the initial 
patch that added 0x199999999999999ULL?
]
]>> Why do we need to have code to deal with something that could never 
]>> happen?
]> 
]> Well, such function arguments *could* happen. How the boot service 
]> should react is a different question. Technically the implementation 
]> matches the UEFI spec.
]> 
]> (Compare for example the select() specification in POSIX:
]> <http://pubs.opengroup.org/onlinepubs/9699919799/functions/select.html
]> >
]> 
]>    Implementations may place limitations on the maximum timeout
]>    interval supported. All implementations shall support a maximum
]>    timeout interval of at least 31 days. If the timeout argument
]>    specifies a timeout interval greater than the implementation-
]>    defined maximum value, the maximum value shall be used as the
]>    actual timeout value. [...]
]> 
]> The next errata / release of the UEFI spec might want to state 
]> something like this too.)
]> 
]
]I was thinking that capping the max value would not be a bad idea. Thanks for 
the POSIX reference. 
]
]Thanks,
]
]Andrew Fish
]
]> Thanks,
]> Laszlo



------------------------------------------------------------------------------
Want excitement?
Manually upgrade your production database.
When you want reliability, choose Perforce.
Perforce version control. Predictably reliable.
http://pubads.g.doubleclick.net/gampad/clk?id=157508191&iu=/4140/ostg.clktrk
_______________________________________________
edk2-devel mailing list
edk2-devel@lists.sourceforge.net
https://lists.sourceforge.net/lists/listinfo/edk2-devel