> -----Original Message----- > From: Jan Beulich [mailto:jbeul...@suse.com] > Sent: 06 March 2019 13:00 > To: Paul Durrant <paul.durr...@citrix.com> > Cc: Julien Grall <julien.gr...@arm.com>; Andrew Cooper > <andrew.coop...@citrix.com>; George Dunlap > <george.dun...@citrix.com>; Ian Jackson <ian.jack...@citrix.com>; Roger Pau > Monne > <roger....@citrix.com>; Wei Liu <wei.l...@citrix.com>; Stefano Stabellini > <sstabell...@kernel.org>; > xen-devel <xen-devel@lists.xenproject.org>; Konrad Rzeszutek Wilk > <konrad.w...@oracle.com>; Tim > (Xen.org) <t...@xen.org> > Subject: RE: [PATCH v3 8/9] viridian: add implementation of synthetic timers > > >>> On 06.03.19 at 12:47, <paul.durr...@citrix.com> wrote: > >> From: Xen-devel [mailto:xen-devel-boun...@lists.xenproject.org] On Behalf > >> Of Paul Durrant > >> Sent: 06 March 2019 11:24 > >> > >> > -----Original Message----- > >> > From: Jan Beulich [mailto:jbeul...@suse.com] > >> > Sent: 25 February 2019 14:54 > >> > > >> > >>> On 31.01.19 at 11:47, <paul.durr...@citrix.com> wrote: > >> > > @@ -118,14 +119,237 @@ static int64_t time_ref_count(struct domain *d) > >> > > return raw_trc_val(d) + trc->off; > >> > > } > >> > > > >> > > +static int64_t time_now(struct domain *d) > >> > > +{ > >> > > + const struct viridian_page *rt = > >> > > &d->arch.hvm.viridian->reference_tsc; > >> > > + HV_REFERENCE_TSC_PAGE *p = rt->ptr; > >> > > + uint32_t start, end; > >> > > + __int128_t tsc; > >> > > + __int128_t scale; > >> > > >> > I don't think you need both of them be 128 bits wide. I also don't > >> > see why either would want to be of a signed type. > >> > >> The spec says (as in the comment below): > >> > >> "The partition reference time is computed by the following formula: > >> > >> ReferenceTime = ((VirtualTsc * TscScale) >> 64) + TscOffset > >> > >> The multiplication is a 64 bit multiplication, which results in a 128 bit > >> number which is then > shifted > >> 64 times to the right to obtain the high 64 bits.TscScale" > >> > >> Again, I'm using signed arithmetic as I think it's necessary for the > >> missed ticks logic to work > >> correctly in the event of an overflow. > > > > FAOD the code that I am concerned about is: > > > > /* > > * The timer is already started, so we're re-scheduling. > > * Hence advance the timer expiration by one tick. > > */ > > next = vs->expiration + vs->count; > > > > /* Now check to see if any expirations have been missed */ > > if ( now - next > 0 ) > > missed = (now - next) / vs->count; > > > > If now and next were unsigned then next may overflow such that (now - next) > > ends up being very large, rather than negative, so I'd end up calculating a > > completely bogus value for missed. > > And this is also what I've been referring to: If signedness matters, there > should be a cast here rather than enforcing signedness onto everyone by > a function logically never returning a signed value.
Ok, I'll redefine the function to return an unsigned value and leave now and next as int64_t. Paul > > Jan > _______________________________________________ Xen-devel mailing list Xen-devel@lists.xenproject.org https://lists.xenproject.org/mailman/listinfo/xen-devel
