Re: [Xen-devel] [PATCH v2 1/3] x86/vlapic: Introduce vlapic_update_timer

["Jan Beulich" <jbeulich@xxxxxxxx>]

>>> Anthony PERARD <anthony.perard@xxxxxxxxxx> 07/18/17 7:10 PM >>>
>There should not be any functionality change with this patch.
>
>This function is used when the APIC_TMICT register is updated.
>
>vlapic_update_timer is introduce as it will be use also when the
>registers APIC_LVTT and APIC_TDCR are updated.
>
>Signed-off-by: Anthony PERARD <anthony.perard@xxxxxxxxxx>
>---

Missing brief revision log here.

>--- a/xen/arch/x86/hvm/vlapic.c
>+++ b/xen/arch/x86/hvm/vlapic.c
>@@ -668,6 +668,57 @@ static void vlapic_tdt_pt_cb(struct vcpu *v, void *data)
>vcpu_vlapic(v)->hw.tdt_msr = 0;
>}
 >
>+/*
>+ * This function is used when a register related to the APIC timer is updated.
>+ * It expect the new value for the register TMICT to be set *before*

expects

>+ * been called.

being

>+ * It expect the new value of LVTT to be set *after* been called, with this 
>new
>+ * values passed as parameter (only APIC_TIMER_MODE_MASK bits matter).
>+ */
>+static void vlapic_update_timer(struct vlapic *vlapic, uint32_t lvtt);
>+{
>+    uint64_t period;
>+    uint64_t delta;

Why two lines (but see also below)?

>+    bool is_periodic;
>+
>+    is_periodic = (lvtt & APIC_TIMER_MODE_MASK) == APIC_TIMER_MODE_PERIODIC;
>+
>+    period = (uint64_t)vlapic_get_reg(vlapic, APIC_TMICT)
>+        * APIC_BUS_CYCLE_NS * vlapic->hw.timer_divisor;
>+
>+    /* Calculate the next time the timer should trigger an interrupt. */
>+    delta = period;

What is the point of having the same value in two variables?

>+    if ( delta )
>+    {
>+        TRACE_2_LONG_3D(TRC_HVM_EMUL_LAPIC_START_TIMER, TRC_PAR_LONG(delta),
>+                        TRC_PAR_LONG(is_periodic ? period : 0LL),

I don't see the need for the LL suffix here.

>+                        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);

Please be consistent with argument wrapping: Either always place as many on
a line as will fit, or (less desirable imo) always have just one per line.

>+        vlapic->timer_last_update = vlapic->pt.last_plt_gtime;
>+
>+        HVM_DBG_LOG(DBG_LEVEL_VLAPIC,
>+                    "bus cycle is %uns, "
>+                    "initial count %u, period %"PRIu64"ns",
>+                    APIC_BUS_CYCLE_NS,
>+                    vlapic_get_reg(vlapic, APIC_TMICT),
>+                    period);
>+    }
>+    else
>+    {
>+        TRACE_0D(TRC_HVM_EMUL_LAPIC_STOP_TIMER);
>+        destroy_periodic_time(&vlapic->pt);
>+    }
>+}
>+
>+

No double blank lines please.

Jan


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