On Wed, 6 Nov 2024 at 10:41, Dmitry Frolov <[email protected]> wrote: > > Both timeout and return value of imx_gpt_update_count() are unsigned. > Thus "limit" can not be negative, but obviously it was implied.
For changes to device models, you need to look at the data sheet for the device to determine the correct behaviour so you can confirm that the change you're making is right. This commit message doesn't include any of that reasoning, which means reviewers have to do it all for themselves. > Found by Linux Verification Center (linuxtesting.org) with SVACE. > > Signed-off-by: Dmitry Frolov <[email protected]> > --- > hw/timer/imx_gpt.c | 2 +- > 1 file changed, 1 insertion(+), 1 deletion(-) > > diff --git a/hw/timer/imx_gpt.c b/hw/timer/imx_gpt.c > index 23b3d79bdb..06e4837fed 100644 > --- a/hw/timer/imx_gpt.c > +++ b/hw/timer/imx_gpt.c > @@ -238,7 +238,7 @@ static void imx_gpt_compute_next_timeout(IMXGPTState *s, > bool event) > } > > /* the new range to count down from */ > - limit = timeout - imx_gpt_update_count(s); > + limit = (long long)timeout - imx_gpt_update_count(s); We really shouldn't be using "long long" here in the first place. If we need a 64-bit signed integer to give correct behaviour then that's int64_t. Almost all the handful of uses of 'long long' in hw/ are doing it just as a lazy way to avoid PRIx64 in a printf format string. The GPT has a 32-bit up-counter. If timeout is, for instance, 0x9000_0000 and the current count is 0x2000_0000 then we want the new "limit" value to be 0x7000_0000 because we still have 0x7000_0000 cycles to go til we hit the next point where we want to generate a compare event. So I think we do indeed want to cast to a 64-bit signed value here. (More broadly I think the problem with this device is that it's trying to use ptimer (which is a down-counter) to model an up-counter. This doesn't work very well, and it's better to not do that. Arguably we should have some nicer abstractions over raw qtimers for the up-counter case and/or get ptimer to handle that too, but that's a lot of work...) thanks -- PMM
