> -----Original Message-----
> From: Xen-devel [mailto:[email protected]] On Behalf Of
> Paul Durrant
> Sent: 19 March 2019 12:47
> To: 'Jan Beulich' <[email protected]>
> Cc: Stefano Stabellini <[email protected]>; Wei Liu
> <[email protected]>; Konrad Rzeszutek Wilk
> <[email protected]>; Andrew Cooper <[email protected]>; Tim
> (Xen.org) <[email protected]>;
> George Dunlap <[email protected]>; Julien Grall
> <[email protected]>; xen-devel <xen-
> [email protected]>; Ian Jackson <[email protected]>; Roger Pau
> Monne
> <[email protected]>
> Subject: Re: [Xen-devel] [PATCH v9 10/11] viridian: add implementation of
> synthetic timers
>
> > -----Original Message-----
> > From: Jan Beulich [mailto:[email protected]]
> > Sent: 19 March 2019 12:18
> > To: Paul Durrant <[email protected]>
> > Cc: Julien Grall <[email protected]>; Andrew Cooper
> > <[email protected]>; Roger Pau Monne
> > <[email protected]>; Wei Liu <[email protected]>; George Dunlap
> > <[email protected]>; Ian
> > Jackson <[email protected]>; Stefano Stabellini
> > <[email protected]>; xen-devel <xen-
> > [email protected]>; Konrad Rzeszutek Wilk
> > <[email protected]>; Tim (Xen.org)
> > <[email protected]>
> > Subject: Re: [PATCH v9 10/11] viridian: add implementation of synthetic
> > timers
> >
> > >>> On 19.03.19 at 10:21, <[email protected]> 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
> [email protected]
> https://lists.xenproject.org/mailman/listinfo/xen-devel
_______________________________________________
Xen-devel mailing list
[email protected]
https://lists.xenproject.org/mailman/listinfo/xen-devel
