Re: [Xen-devel] [PATCH 1/3] x86/vlapic: Fix vLAPIC Timer to behave more like real-hw

["Jan Beulich" <JBeulich@xxxxxxxx>]

>>> On 23.03.17 at 12:46, <anthony.perard@xxxxxxxxxx> wrote:
> This patch takes care of change of timer mode between periodic and
> one-shot, because the timer is not reset this happen.

So I have to admit that I have some general difficulties with a
patch submission like this: The patch title is not really making
clear which aspect(s) are being changed, and the sentence
above is ambiguous according to my reading: Do you mean
the timer so far is wrongly not being reset when this happens,
or the timer should not be reset in such a case? Furthermore,
and this is really relevant in cases like this, you should clearly
spell out which part(s) of your change are addressing issues
with us not properly following the spec vs. which are based
on empirically collected information. In the case here, I
assume you mean the timer (or really TMICT) should not be
reset, but I can't seem to find anything int the SDM saying
so.

> There is still change of the Divide Configuration Register that is not
> handle by this patch, but will be in:
> x86/vlapic: Handle change of timer Divide Configuration Register
> 
> Testing has been done with XTF+(patch "Add vlapic timer checks").

On a broad range of hardware, I assume, if my observation
of you making a change not mandated by the spec is correct?

> --- a/xen/arch/x86/hvm/vlapic.c
> +++ b/xen/arch/x86/hvm/vlapic.c
> @@ -518,7 +518,11 @@ static uint32_t vlapic_get_tmcct(struct vlapic *vlapic)
>      counter_passed = ((hvm_get_guest_time(v) - vlapic->timer_last_update)
>                        / (APIC_BUS_CYCLE_NS * vlapic->hw.timer_divisor));
>  
> -    if ( tmict != 0 )
> +    /* If timer_last_update is 0, then TMCCT should be 0 as well.
> +     * This happen when the timer is set to periodic with TMICT != 0, but 
> TMCCT
> +     * was already down to 0.
> +     */
> +    if ( tmict != 0 && vlapic->timer_last_update )

Please be consistent - either both sides use "!= 0", or (preferably)
both sides don't. Also please fix comment style (also elsewhere in
the patch). And finally, do you mean "this happens when ..." or
"this can happen when ..." (I assume the former, and I assume
you specifically mean a transition from one-shot to periodic)? This
needs to be spelled out without leaving any room for interpretation,
namely, as said, when the implemented behavior is not mandated
by documentation.

> @@ -666,6 +670,82 @@ static void vlapic_tdt_pt_cb(struct vcpu *v, void *data)
>      vcpu_vlapic(v)->hw.tdt_msr = 0;
>  }
>  
> +static void vlapic_update_timer(struct vlapic *vlapic,
> +                                unsigned int offset,
> +                                uint32_t val)
> +{
> +
> +    uint64_t period;
> +    uint64_t delta = 0;
> +    bool is_oneshot, is_periodic;
> +
> +    switch (offset)
> +    {
> +    case APIC_LVTT:
> +        period = (uint64_t)vlapic_get_reg(vlapic, APIC_TMICT)
> +            * APIC_BUS_CYCLE_NS * vlapic->hw.timer_divisor;
> +        is_periodic = (val & APIC_TIMER_MODE_MASK) == 
> APIC_TIMER_MODE_PERIODIC;
> +        is_oneshot = (val & APIC_TIMER_MODE_MASK) == APIC_TIMER_MODE_ONESHOT;
> +
> +        /* Calculate the next time the timer should trigger an interrupt. */
> +        if ( period && vlapic->timer_last_update )
> +        {
> +            uint64_t time_passed = hvm_get_guest_time(current)
> +                - vlapic->timer_last_update;
> +            if ( vlapic_lvtt_period(vlapic) )

Blank line between declaration(s) and statements please. And then -
is this decision really to be taken based on the old LVTT value?

> +                time_passed %= period;
> +            if ( time_passed < period )
> +                delta = period - time_passed;
> +        }
> +        break;
> +    case APIC_TMICT:
> +        period = (uint64_t)val * APIC_BUS_CYCLE_NS * 
> vlapic->hw.timer_divisor;
> +        delta = period;
> +
> +        is_periodic = vlapic_lvtt_period(vlapic);
> +        is_oneshot = vlapic_lvtt_oneshot(vlapic);
> +        break;
> +    default:
> +        BUG();
> +    }

There are exactly two calls to this function, one each from the
code handling the writes of the respective registers handled in
the switch() above. Please move such portions of the code into
the callers (calculation of the two is_* variable can remain here,
for example, the callers would simply pass the LVTT value), using
suitable function parameters to pass the values needed here.
That way you can avoid the BUG() and make review as well as
future code inspection easier.

> +    if ( (is_oneshot || is_periodic) && delta != 0 )

Wouldn't this more obviously be

    if ( (is_oneshot && delta) || is_periodic )

(afaict delta would always non-zero here in periodic mode, if
the earlier mentioned use of the old LVTT value is indeed wrong;
otherwise delta may need forcing to period for the call to
create_periodic_time() below)?

> +    {
> +        TRACE_2_LONG_3D(TRC_HVM_EMUL_LAPIC_START_TIMER, TRC_PAR_LONG(delta),
> +                        TRC_PAR_LONG(is_periodic ? period : 0LL),
> +                        vlapic->pt.irq);
> +
> +        create_periodic_time(current, &vlapic->pt,
> +                             delta,
> +                             is_periodic ? period : 0,
> +                             vlapic->pt.irq,
> +                             is_periodic ? vlapic_pt_cb : NULL,
> +                             &vlapic->timer_last_update);
> +
> +        /* For the case where the timer was periodic and it is now
> +         * one-shot, timer_last_update should be the value of the last time
> +         * the interrupt was triggered.
> +         */
> +        vlapic->timer_last_update = vlapic->pt.last_plt_gtime + delta - 
> period;
> +
> +        HVM_DBG_LOG(DBG_LEVEL_VLAPIC,
> +                    "bus cycle is %uns, "
> +                    "initial count %u, period %"PRIu64"ns",
> +                    APIC_BUS_CYCLE_NS,
> +                    offset == APIC_TMICT
> +                    ? val : vlapic_get_reg(vlapic, APIC_TMICT),
> +                    period);
> +    }
> +    else
> +    {
> +        TRACE_0D(TRC_HVM_EMUL_LAPIC_STOP_TIMER);
> +        destroy_periodic_time(&vlapic->pt);
> +        /* From now, TMCCT should be 0 until TMICT is set. */
> +        vlapic->timer_last_update = 0;

Hmm, the deadline case would come here too, and other TDT
related code uses the field too, so I have to ask whether this
possibly breaks TDT (then being fixed by a later patch). If so
you'll need to find a way to not transiently break other
functionality. Furthermore, with documentation again saying
nothing about TMICT when using TDT, you should also clarify
again what the intended / observed behavior is.

Jan

_______________________________________________
Xen-devel mailing list
Xen-devel@xxxxxxxxxxxxx
https://lists.xen.org/xen-devel