> -----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. Paul _______________________________________________ Xen-devel mailing list Xen-devel@lists.xenproject.org https://lists.xenproject.org/mailman/listinfo/xen-devel
