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

[Jan Beulich <jbeulich@xxxxxxxx>]

On 14.04.2021 15:37, Roger Pau Monné wrote:
> On Wed, Apr 14, 2021 at 12:28:43PM +0200, Jan Beulich wrote:
>> 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.
> 
> I think the logic here changed enough to not match anymore. Certainly
> pending_intr_nr shouldn't be decreased there, as pt_irq_fired is
> invoked after an EOI in this patch, instead of being invoked when a
> vpt related interrupt was injected. I think I should better rename
> pt_irq_fired to pt_irq_eoi and that would make it clearer.

But pt_process_missed_ticks() should be called only when a tick was
missed, shouldn't it? Or actually, looking at the function, I guess
I'm confused. Does your patch change the meaning of the field?

> FWIW, decreasing pending_intr_nr should only be done after an
> inject_interrupt call.

Then this line (which you leave in place)

         pt->pending_intr_nr = 0; /* 'collapse' all missed ticks */

is contradicting the (new) model.

Jan