Re: [Xen-devel] [PATCH v9 10/11] viridian: add implementation of synthetic timers

[Paul Durrant <Paul.Durrant@xxxxxxxxxx>]

> -----Original Message-----
> From: Xen-devel [mailto:xen-devel-bounces@xxxxxxxxxxxxxxxxxxxx] On Behalf Of 
> Paul Durrant
> Sent: 19 March 2019 12:47
> To: 'Jan Beulich' <JBeulich@xxxxxxxx>
> Cc: Stefano Stabellini <sstabellini@xxxxxxxxxx>; Wei Liu 
> <wei.liu2@xxxxxxxxxx>; Konrad Rzeszutek Wilk
> <konrad.wilk@xxxxxxxxxx>; Andrew Cooper <Andrew.Cooper3@xxxxxxxxxx>; Tim 
> (Xen.org) <tim@xxxxxxx>;
> George Dunlap <George.Dunlap@xxxxxxxxxx>; Julien Grall 
> <julien.grall@xxxxxxx>; xen-devel <xen-
> devel@xxxxxxxxxxxxxxxxxxxx>; Ian Jackson <Ian.Jackson@xxxxxxxxxx>; Roger Pau 
> Monne
> <roger.pau@xxxxxxxxxx>
> Subject: Re: [Xen-devel] [PATCH v9 10/11] viridian: add implementation of 
> synthetic timers
> 
> > -----Original Message-----
> > From: Jan Beulich [mailto:JBeulich@xxxxxxxx]
> > Sent: 19 March 2019 12:18
> > To: Paul Durrant <Paul.Durrant@xxxxxxxxxx>
> > Cc: Julien Grall <julien.grall@xxxxxxx>; Andrew Cooper 
> > <Andrew.Cooper3@xxxxxxxxxx>; Roger Pau Monne
> > <roger.pau@xxxxxxxxxx>; Wei Liu <wei.liu2@xxxxxxxxxx>; George Dunlap 
> > <George.Dunlap@xxxxxxxxxx>; Ian
> > Jackson <Ian.Jackson@xxxxxxxxxx>; Stefano Stabellini 
> > <sstabellini@xxxxxxxxxx>; xen-devel <xen-
> > devel@xxxxxxxxxxxxxxxxxxxx>; Konrad Rzeszutek Wilk 
> > <konrad.wilk@xxxxxxxxxx>; Tim (Xen.org)
> > <tim@xxxxxxx>
> > Subject: Re: [PATCH v9 10/11] viridian: add implementation of synthetic 
> > timers
> >
> > >>> On 19.03.19 at 10:21, <paul.durrant@xxxxxxxxxx> wrote:
> > > +static void poll_stimer(struct vcpu *v, unsigned int stimerx)
> > > +{
> > > +    struct viridian_vcpu *vv = v->arch.hvm.viridian;
> > > +    struct viridian_stimer *vs = &vv->stimer[stimerx];
> > > +
> > > +    /*
> > > +     * Timer expiry may race with the timer being disabled. If the timer
> > > +     * is disabled make sure the pending bit is cleared to avoid re-
> > > +     * polling.
> > > +     */
> > > +    if ( !vs->config.enabled )
> > > +    {
> > > +        clear_bit(stimerx, &vv->stimer_pending);
> > > +        return;
> > > +    }
> >
> > It feels a little odd to squash an already pending event like this,
> > but I think I see why you want/need it this way. Of course the
> > question is whether an MSR write (turning the enabled bit off)
> > after the timer has expired should cancel the sending of a
> > notification message. I could imagine this not even being spelled
> > out anywhere in the spec...
> 
> No, it's not but it seems like the right thing to do.
> 
> >
> > > +    if ( !test_bit(stimerx, &vv->stimer_pending) )
> > > +        return;
> > > +
> > > +    if ( !viridian_synic_deliver_timer_msg(v, vs->config.sintx,
> > > +                                           stimerx, vs->expiration,
> > > +                                           time_now(v->domain)) )
> > > +        return;
> > > +
> > > +    clear_bit(stimerx, &vv->stimer_pending);
> > > +
> > > +    if ( vs->config.periodic )
> > > +        start_stimer(vs);
> > > +    else
> > > +        vs->config.enabled = 0;
> >
> > In v8 you started the timer here when config.enabled is true. Was
> > that with the implicit assumption that the bit would still have been
> > off from stimer_expire() for non-periodic timers? If so, the above
> > would indeed be a sufficiently close equivalent, while if not I would
> > be a little confused by this construct.
> 
> This should be a reversion to the v7 construct. The expiration function no 
> longer updates the config
> so the poll clears the enable bit for single-shot timers instead whereas 
> periodic times stay enabled
> (until an MSR write disables them) and get re-scheduled.
> 
> >
> > Is clearing the enabled bit appropriate here? stimer_expire() and
> > this function are asynchronous with one another, i.e. there's a
> > window where the guest may write the MSR again (with the enabled
> > bit set) after stimer_expire() has run but before we make it here.
> > I think the overall outcome is fine, but wouldn't start_stimer() better
> > clear the enabled bit right away after having called stimer_expire()?
> > Or wait - doing so would conflict with the enabled bit check at the
> > top of this function. Otoh an MSR read immediately following such
> > an MSR write should observe the enabled bit clear for a non-periodic
> > timer that was set in the past, shouldn't it?
> 
> I'm not sure about that because it's not clear when the timer expires as 
> such. The spec is not clear
> whether timer expiry means the point when it times out or when the message is 
> delivered.
> 
> > So perhaps a set
> > pending bit should result in the RDMSR handling to clear the enabled
> > bit in the returned value for a non-periodic timer?
> 
> I get tied in knots every time I think about this and without waiting for a 
> pending timer to stop when
> it is disabled I see no way of the race, but I think doing that would be 
> prohibitively slow because

                                ^ avoiding

> windows seems to flip between single-shot and periodic timers on quite a 
> frequent basis. At this point
> I'd prefer to leave the code as-is and run some tests. We've already had a 
> prior incarnation running
> in XenServer automated tests for several weeks with no discovered problems.
> 
>   Paul
> 
> >
> > Jan
> >
> 
> 
> _______________________________________________
> Xen-devel mailing list
> Xen-devel@xxxxxxxxxxxxxxxxxxxx
> https://lists.xenproject.org/mailman/listinfo/xen-devel
_______________________________________________
Xen-devel mailing list
Xen-devel@xxxxxxxxxxxxxxxxxxxx
https://lists.xenproject.org/mailman/listinfo/xen-devel