> -----Original Message----- > From: Paul Durrant > Sent: 06 March 2019 13:06 > To: 'Jan Beulich' <jbeul...@suse.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 > > > -----Original Message----- > > From: Jan Beulich [mailto:jbeul...@suse.com] > > Sent: 06 March 2019 12:57 > > 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:23, <paul.durr...@citrix.com> wrote: > > >> 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) > > >> > > >> 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 Xen-devel@lists.xenproject.org https://lists.xenproject.org/mailman/listinfo/xen-devel
