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

[Paul Durrant <Paul.Durrant@xxxxxxxxxx>]

> -----Original Message-----
> From: Jan Beulich [mailto:JBeulich@xxxxxxxx]
> Sent: 06 March 2019 13:00
> 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:47, <Paul.Durrant@xxxxxxxxxx> wrote:
> >> From: Xen-devel [mailto:xen-devel-bounces@xxxxxxxxxxxxxxxxxxxx] On Behalf 
> >> Of Paul Durrant
> >> Sent: 06 March 2019 11:24
> >>
> >> > -----Original Message-----
> >> > 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)
> >> > > +{
> >> > > +    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@xxxxxxxxxxxxxxxxxxxx
https://lists.xenproject.org/mailman/listinfo/xen-devel