Re: [PATCH v3 09/11] x86/vpt: switch interrupt injection model

[Jan Beulich <jbeulich@xxxxxxxx>]

On 31.03.2021 12:33, Roger Pau Monne wrote:
> ---
>  xen/arch/x86/hvm/svm/intr.c   |   3 -
>  xen/arch/x86/hvm/vmx/intr.c   |  59 ------
>  xen/arch/x86/hvm/vpt.c        | 334 ++++++++++++++--------------------
>  xen/include/asm-x86/hvm/vpt.h |   5 +-
>  4 files changed, 143 insertions(+), 258 deletions(-)

Nice.

> @@ -285,189 +238,144 @@ static void pt_irq_fired(struct vcpu *v, struct 
> periodic_time *pt)
>              list_del(&pt->list);
>          pt->on_list = false;
>          pt->pending_intr_nr = 0;
> +
> +        return;
>      }
> -    else if ( mode_is(v->domain, one_missed_tick_pending) ||
> -              mode_is(v->domain, no_missed_ticks_pending) )
> +
> +    if ( mode_is(v->domain, one_missed_tick_pending) ||
> +         mode_is(v->domain, no_missed_ticks_pending) )
>      {
> -        pt->last_plt_gtime = hvm_get_guest_time(v);
>          pt_process_missed_ticks(pt);
>          pt->pending_intr_nr = 0; /* 'collapse' all missed ticks */
> +    }
> +    else if ( !pt->pending_intr_nr )
> +        pt_process_missed_ticks(pt);

Did you lose a -- here? I.e. does the condition mean to match ...

> +    if ( !pt->pending_intr_nr )
>          set_timer(&pt->timer, pt->scheduled);
> +}
> +
> +static void pt_timer_fn(void *data)
> +{
> +    struct periodic_time *pt = data;
> +    struct vcpu *v;
> +    time_cb *cb = NULL;
> +    void *cb_priv;
> +    unsigned int irq;
> +
> +    pt_lock(pt);
> +
> +    v = pt->vcpu;
> +    irq = pt->irq;
> +
> +    if ( inject_interrupt(pt) )
> +    {
> +        pt->scheduled += pt->period;
> +        pt->do_not_freeze = 0;
> +        cb = pt->cb;
> +        cb_priv = pt->priv;
>      }
>      else
>      {
> -        pt->last_plt_gtime += pt->period;
> -        if ( --pt->pending_intr_nr == 0 )

... this original code? Otherwise I can't see why the condition
guards a pt_process_missed_ticks() invocation.

> @@ -617,20 +556,29 @@ void pt_adjust_global_vcpu_target(struct vcpu *v)
>      write_unlock(&pl_time->vhpet.lock);
>  }
>  
> -
>  static void pt_resume(struct periodic_time *pt)
>  {
> +    struct vcpu *v;
> +    time_cb *cb = NULL;
> +    void *cb_priv;
> +
>      if ( pt->vcpu == NULL )
>          return;
>  
>      pt_lock(pt);
> -    if ( pt->pending_intr_nr && !pt->on_list )
> +    if ( pt->pending_intr_nr && !pt->on_list && inject_interrupt(pt) )
>      {
> +        pt->pending_intr_nr--;
> +        cb = pt->cb;
> +        cb_priv = pt->priv;
> +        v = pt->vcpu;
>          pt->on_list = 1;
>          list_add(&pt->list, &pt->vcpu->arch.hvm.tm_list);
> -        vcpu_kick(pt->vcpu);
>      }
>      pt_unlock(pt);
> +
> +    if ( cb )
> +        cb(v, cb_priv);
>  }

I'm afraid until we raise our supported gcc versions baseline, v and
cb_priv will need an initializer at the top of the function just like
cb.

Jan