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

[Anthony PERARD <anthony.perard@xxxxxxxxxx>]

On Thu, Aug 03, 2017 at 08:59:10AM -0600, Jan Beulich wrote:
> >>> 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.

It would have been "New patch", since I've rewrite all patches and
reorganize the serie. But the revision log is in the cover letter.

> >+static void vlapic_update_timer(struct vlapic *vlapic, uint32_t lvtt);
> >+{
> >+    uint64_t period;
> >+    uint64_t delta;
> 
> Why two lines (but see also below)?

Why not? Anyway, I'll change it.

Also I realize that delta is going to be initialize to 0 here in the
next patch, which is why I think there is two lines.

> >+    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?

It might look like it but there are not the same values, the description
is accurate, even if the calculation at this stage is very simple.

More importantly, this line is going away in the next patch and will be
replaced by a more complexe calculation.


Thanks,

-- 
Anthony PERARD

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