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

[Jan Beulich <jbeulich@xxxxxxxx>]

On 30.09.2020 12:41, Roger Pau Monne wrote:
> Currently vPT relies on timers being assigned to a vCPU and performing
> checks on every return to HVM guest in order to check if an interrupt
> from a vPT timer assigned to the vCPU is currently being injected.
> 
> This model doesn't work properly since the interrupt destination vCPU
> of a vPT timer can be different from the vCPU where the timer is
> currently assigned, in which case the timer would get stuck because it
> never sees the interrupt as being injected.
> 
> Knowing when a vPT interrupt is injected is relevant for the guest
> timer modes where missed vPT interrupts are not discarded and instead
> are accumulated and injected when possible.
> 
> This change aims to modify the logic described above, so that vPT
> doesn't need to check on every return to HVM guest if a vPT interrupt
> is being injected. In order to achieve this the vPT code is modified
> to make use of the new EOI callbacks, so that virtual timers can
> detect when a interrupt has been serviced by the guest by waiting for
> the EOI callback to execute.
> 
> This model also simplifies some of the logic, as when executing the
> timer EOI callback Xen can try to inject another interrupt if the
> timer has interrupts pending for delivery.
> 
> Note that timers are still bound to a vCPU for the time being, this
> relation however doesn't limit the interrupt destination anymore, and
> will be removed by further patches.
> 
> This model has been tested with Windows 7 guests without showing any
> timer delay, even when the guest was limited to have very little CPU
> capacity and pending virtual timer interrupts accumulate.
> 
> Signed-off-by: Roger Pau Monné <roger.pau@xxxxxxxxxx>
> ---
> Changes since v1:
>  - New in this version.
> ---
> Sorry, this is a big change, but I'm having issues splitting it into
> smaller pieces as the functionality needs to be changed in one go, or
> else timers would be broken.
> 
> If this approach seems sensible I can try to split it up.

If it can't sensibly be split, so be it, I would say. And yes, the
approach does look sensible to me, supported by ...

> ---
>  xen/arch/x86/hvm/svm/intr.c   |   3 -
>  xen/arch/x86/hvm/vmx/intr.c   |  59 ------
>  xen/arch/x86/hvm/vpt.c        | 326 ++++++++++++++--------------------
>  xen/include/asm-x86/hvm/vpt.h |   5 +-
>  4 files changed, 135 insertions(+), 258 deletions(-)

... this diffstat. Good work!

Just a couple of nits, but before giving this my ack I may need to
go through it a 2nd time.

> +/*
> + * The same callback is shared between LAPIC and PIC/IO-APIC based timers, as
> + * we ignore the first parameter that's different between them.
> + */
> +static void eoi_callback(unsigned int unused, void *data)
>  {
> -    struct list_head *head = &v->arch.hvm.tm_list;
> -    struct periodic_time *pt, *temp, *earliest_pt;
> -    uint64_t max_lag;
> -    int irq, pt_vector = -1;
> -    bool level;
> +    struct periodic_time *pt = data;
> +    struct vcpu *v;
> +    time_cb *cb = NULL;
> +    void *cb_priv;
>  
> -    pt_vcpu_lock(v);
> +    pt_lock(pt);
>  
> -    earliest_pt = NULL;
> -    max_lag = -1ULL;
> -    list_for_each_entry_safe ( pt, temp, head, list )
> +    pt_irq_fired(pt->vcpu, pt);
> +    if ( pt->pending_intr_nr )
>      {
> -        if ( pt->pending_intr_nr )
> +        if ( inject_interrupt(pt) )
> +        {
> +            pt->pending_intr_nr--;
> +            cb = pt->cb;
> +            cb_priv = pt->priv;
> +            v = pt->vcpu;
> +        }
> +        else
>          {
> -            /* RTC code takes care of disabling the timer itself. */
> -            if ( (pt->irq != RTC_IRQ || !pt->priv) && pt_irq_masked(pt) &&
> -                 /* Level interrupts should be asserted even if masked. */
> -                 !pt->level )
> -            {
> -                /* suspend timer emulation */
> +            /* Masked. */
> +            if ( pt->on_list )
>                  list_del(&pt->list);
> -                pt->on_list = 0;
> -            }
> -            else
> -            {
> -                if ( (pt->last_plt_gtime + pt->period) < max_lag )
> -                {
> -                    max_lag = pt->last_plt_gtime + pt->period;
> -                    earliest_pt = pt;
> -                }
> -            }
> +            pt->on_list = false;
>          }
>      }
>  
> -    if ( earliest_pt == NULL )
> -    {
> -        pt_vcpu_unlock(v);
> -        return -1;
> -    }
> +    pt_unlock(pt);
>  
> -    earliest_pt->irq_issued = 1;
> -    irq = earliest_pt->irq;
> -    level = earliest_pt->level;
> +    if ( cb != NULL )
> +        cb(v, cb_priv);

Nit: Like done elsewhere, omit the " != NULL"?

> +    /* Update time when an interrupt is injected. */
> +    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);
> +    else
> +        pt->last_plt_gtime += pt->period;
>  
> -    pt_vcpu_unlock(v);
> +    if ( mode_is(v->domain, delay_for_missed_ticks) &&

This looks to be possible to move into the "else" above, but on the
whole maybe everything together would best be handled by switch()?

> @@ -543,6 +443,24 @@ void create_periodic_time(
>      pt->cb = cb;
>      pt->priv = data;
>  
> +    switch ( pt->source )
> +    {
> +        int rc;
> +
> +    case PTSRC_isa:
> +        irq = hvm_isa_irq_to_gsi(irq);
> +        /* fallthrough */
> +    case PTSRC_ioapic:
> +        pt->eoi_cb.callback = eoi_callback;
> +        pt->eoi_cb.data = pt;
> +        rc = hvm_gsi_register_callback(v->domain, irq, &pt->eoi_cb);
> +        if ( rc )
> +            gdprintk(XENLOG_WARNING,
> +                     "unable to register callback for timer GSI %u: %d\n",
> +                     irq, rc);

If this triggers, would it be helpful to also log pt->source?

Jan