> -----Original Message----- > From: Paul Durrant > Sent: 06 March 2019 13:06 > To: 'Jan Beulich' <[email protected]> > Cc: Julien Grall <[email protected]>; Andrew Cooper > <[email protected]>; George Dunlap > <[email protected]>; Ian Jackson <[email protected]>; Roger Pau > Monne > <[email protected]>; Wei Liu <[email protected]>; Stefano Stabellini > <[email protected]>; > xen-devel <[email protected]>; Konrad Rzeszutek Wilk > <[email protected]>; Tim > (Xen.org) <[email protected]> > Subject: RE: [PATCH v3 8/9] viridian: add implementation of synthetic timers > > > -----Original Message----- > > From: Jan Beulich [mailto:[email protected]] > > Sent: 06 March 2019 12:57 > > To: Paul Durrant <[email protected]> > > Cc: Julien Grall <[email protected]>; Andrew Cooper > > <[email protected]>; George Dunlap > > <[email protected]>; Ian Jackson <[email protected]>; Roger Pau > > Monne > > <[email protected]>; Wei Liu <[email protected]>; Stefano Stabellini > > <[email protected]>; > > xen-devel <[email protected]>; Konrad Rzeszutek Wilk > > <[email protected]>; Tim > > (Xen.org) <[email protected]> > > Subject: RE: [PATCH v3 8/9] viridian: add implementation of synthetic timers > > > > >>> On 06.03.19 at 12:23, <[email protected]> wrote: > > >> From: Jan Beulich [mailto:[email protected]] > > >> Sent: 25 February 2019 14:54 > > >> > > >> >>> On 31.01.19 at 11:47, <[email protected]> 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) > > >> > > >> Why would this return a signed value? And can't the function > > >> parameter be const? > > > > > > The function parameter can be const, but I think the result needs to be > > > signed for the missed ticks logic in start_timer() to work correctly. > > > > If something requires signed arithmetic, then this should be enforced > > there, not by the return type of an otherwise sufficiently generic > > function. Then again NOW() also produces a signed value ... > > > > >> > +{ > > >> > + 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" > > > > Well, yes, you want a 128-bit result. But for that you don't need to > > multiply 128-bit quantities. See e.g. our own scale_delta() or > > hvm_scale_tsc(). > > Testing showed that by not casting first things were broken. I assumed this > was because the result of > the multiplication was being truncated to 64-bits before assignment, but I > can check the generated > code. I'll also have a look at the examples you cite.
Both those examples do the multiplication by inline asm (presumably to avoid truncation). Is that what you'd prefer? Paul > > Paul _______________________________________________ Xen-devel mailing list [email protected] https://lists.xenproject.org/mailman/listinfo/xen-devel
