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

[Paul Durrant <Paul.Durrant@xxxxxxxxxx>]

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