Re: [Xen-devel] [PATCH v3 8/9] viridian: add implementation of synthetic timers

[Paul Durrant <Paul.Durrant@xxxxxxxxxx>]

> -----Original Message-----
> From: Paul Durrant
> Sent: 06 March 2019 13:06
> To: 'Jan Beulich' <JBeulich@xxxxxxxx>
> Cc: Julien Grall <julien.grall@xxxxxxx>; Andrew Cooper 
> <Andrew.Cooper3@xxxxxxxxxx>; George Dunlap
> <George.Dunlap@xxxxxxxxxx>; Ian Jackson <Ian.Jackson@xxxxxxxxxx>; Roger Pau 
> Monne
> <roger.pau@xxxxxxxxxx>; Wei Liu <wei.liu2@xxxxxxxxxx>; Stefano Stabellini 
> <sstabellini@xxxxxxxxxx>;
> xen-devel <xen-devel@xxxxxxxxxxxxxxxxxxxx>; Konrad Rzeszutek Wilk 
> <konrad.wilk@xxxxxxxxxx>; Tim
> (Xen.org) <tim@xxxxxxx>
> Subject: RE: [PATCH v3 8/9] viridian: add implementation of synthetic timers
> 
> > -----Original Message-----
> > From: Jan Beulich [mailto:JBeulich@xxxxxxxx]
> > Sent: 06 March 2019 12:57
> > To: Paul Durrant <Paul.Durrant@xxxxxxxxxx>
> > Cc: Julien Grall <julien.grall@xxxxxxx>; Andrew Cooper 
> > <Andrew.Cooper3@xxxxxxxxxx>; George Dunlap
> > <George.Dunlap@xxxxxxxxxx>; Ian Jackson <Ian.Jackson@xxxxxxxxxx>; Roger Pau 
> > Monne
> > <roger.pau@xxxxxxxxxx>; Wei Liu <wei.liu2@xxxxxxxxxx>; Stefano Stabellini 
> > <sstabellini@xxxxxxxxxx>;
> > xen-devel <xen-devel@xxxxxxxxxxxxxxxxxxxx>; Konrad Rzeszutek Wilk 
> > <konrad.wilk@xxxxxxxxxx>; Tim
> > (Xen.org) <tim@xxxxxxx>
> > Subject: RE: [PATCH v3 8/9] viridian: add implementation of synthetic timers
> >
> > >>> On 06.03.19 at 12:23, <Paul.Durrant@xxxxxxxxxx> wrote:
> > >> From: Jan Beulich [mailto:JBeulich@xxxxxxxx]
> > >> Sent: 25 February 2019 14:54
> > >>
> > >> >>> On 31.01.19 at 11:47, <paul.durrant@xxxxxxxxxx> 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@xxxxxxxxxxxxxxxxxxxx
https://lists.xenproject.org/mailman/listinfo/xen-devel