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

["Jan Beulich" <JBeulich@xxxxxxxx>]

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

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

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

Jan



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