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

["Jan Beulich" <JBeulich@xxxxxxxx>]

>>> 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.

Jan



_______________________________________________
Xen-devel mailing list
Xen-devel@xxxxxxxxxxxxxxxxxxxx
https://lists.xenproject.org/mailman/listinfo/xen-devel