Re: [Qemu-devel] Re: [PATCH v3 2/7] wdt_ib700: Don't use SoftFloat int64 type

["Richard W.M. Jones" <rjones@redhat.com>]

On Sun, Dec 19, 2010 at 01:51:22PM +0100, Andreas Färber wrote:
> Am 18.12.2010 um 17:47 schrieb Richard W.M. Jones:
> 
> >On Sat, Dec 18, 2010 at 05:25:26PM +0100, Andreas Färber wrote:
> >>softfloat.h's int64 type has least-width semantics,
> >>but this doesn't seem intended here, so use plain int64_t.
> >>
> >>v3:
> >>* Split off.
> >>
> >>Cc: Richard W.M. Jones <rjones@redhat.com>
> >>Signed-off-by: Andreas Färber <andreas.faerber@web.de>
> >>---
> >>hw/wdt_ib700.c |    2 +-
> >>1 files changed, 1 insertions(+), 1 deletions(-)
> >>
> >>diff --git a/hw/wdt_ib700.c b/hw/wdt_ib700.c
> >>index b6235eb..1248464 100644
> >>--- a/hw/wdt_ib700.c
> >>+++ b/hw/wdt_ib700.c
> >>@@ -53,7 +53,7 @@ static void ib700_write_enable_reg(void *vp,
> >>uint32_t addr, uint32_t data)
> >>       30, 28, 26, 24, 22, 20, 18, 16,
> >>       14, 12, 10,  8,  6,  4,  2,  0
> >>   };
> >>-    int64 timeout;
> >>+    int64_t timeout;
> >>
> >>   ib700_debug("addr = %x, data = %x\n", addr, data);
> >
> >The use of int64(_t) was just so that the timeout calculation in the
> >next two lines would not overflow:
> >
> > timeout = (int64_t) time_map[data & 0xF] * get_ticks_per_sec();
> > qemu_mod_timer(s->timer, qemu_get_clock (vm_clock) + timeout);
> >
> >and from you say it does seem like it was a mistake to use int64
> >instead of int64_t.
> 
> int64_t should be the right choice then.
> 
> >ACK.
> >
> >In more general terms, am I doing the timeout correctly in this code?
> 
> Being unfamiliar with both the timer code and this device, hard to
> say for me.
> You're taking the lower nibble of uint32_t data and indexing
> time_map[] with it, which contains 16 elements, so okay, upcast it
> to 64-bit and multiply it to ticks. Assuming that vm_clock works in
> ticks, adding the calculated timeout for the next expiry technically
> looks good. Except for the extra space. ;)
> 
> Care to provide a formal Reviewed-by or Acked-by? I'd respin it for
> Blue with updated description.

Is it enough just to write this:

Reviewed-by: Richard W.M. Jones <rjones@redhat.com>
Acked-by: Richard W.M. Jones <rjones@redhat.com>

or do you want me to send the updated patch with this added?

Rich.

-- 
Richard Jones, Virtualization Group, Red Hat http://people.redhat.com/~rjones
New in Fedora 11: Fedora Windows cross-compiler. Compile Windows
programs, test, and build Windows installers. Over 70 libraries supprt'd
http://fedoraproject.org/wiki/MinGW http://www.annexia.org/fedora_mingw