| Message ID | 20170718170935.25648-2-anthony.perard@citrix.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
>>> Anthony PERARD <anthony.perard@citrix.com> 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@citrix.com> >--- 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
On Thu, Aug 03, 2017 at 08:59:10AM -0600, Jan Beulich wrote: > >>> Anthony PERARD <anthony.perard@citrix.com> 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@citrix.com> > >--- > > 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 <anthony.perard@citrix.com> 08/04/17 12:52 PM >>> >On Thu, Aug 03, 2017 at 08:59:10AM -0600, Jan Beulich wrote: >> >>> Anthony PERARD <anthony.perard@citrix.com> 07/18/17 7:10 PM >>> >> >+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. For both this and ... >> >+ 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. ... and this - irrespective of subsequent patches, the one here would better be self-contained, or otherwise its description should point out that certain things are done in a way easing subsequent ones (but only if that was really the case, which I don't think it is here - as you say, the questionable constructs are being touched again later anyway, so could as well be left out). Jan
On Fri, Aug 04, 2017 at 09:40:32AM -0600, Jan Beulich wrote: > >>> Anthony PERARD <anthony.perard@citrix.com> 08/04/17 12:52 PM >>> > >On Thu, Aug 03, 2017 at 08:59:10AM -0600, Jan Beulich wrote: > >> >>> Anthony PERARD <anthony.perard@citrix.com> 07/18/17 7:10 PM >>> > >> >+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. > > For both this and ... > > >> >+ 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. > > ... and this - irrespective of subsequent patches, the one here would better > be self-contained, or otherwise its description should point out that certain > things are done in a way easing subsequent ones (but only if that was really > the case, which I don't think it is here - as you say, the questionable > constructs are being touched again later anyway, so could as well be left > out). Fine, I'll get rid of delta in this patch.
diff --git a/xen/arch/x86/hvm/vlapic.c b/xen/arch/x86/hvm/vlapic.c index 4320c6e30a..883114c824 100644 --- 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* + * been called. + * 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; + 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; + + if ( delta ) + { + 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); + + 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); + } +} + + static void vlapic_reg_write(struct vcpu *v, unsigned int offset, uint32_t val) { @@ -764,37 +815,13 @@ static void vlapic_reg_write(struct vcpu *v, break; case APIC_TMICT: - { - uint64_t period; - if ( !vlapic_lvtt_oneshot(vlapic) && !vlapic_lvtt_period(vlapic) ) break; vlapic_set_reg(vlapic, APIC_TMICT, val); - if ( val == 0 ) - { - TRACE_0D(TRC_HVM_EMUL_LAPIC_STOP_TIMER); - destroy_periodic_time(&vlapic->pt); - break; - } - - period = (uint64_t)APIC_BUS_CYCLE_NS * val * vlapic->hw.timer_divisor; - TRACE_2_LONG_3D(TRC_HVM_EMUL_LAPIC_START_TIMER, TRC_PAR_LONG(period), - TRC_PAR_LONG(vlapic_lvtt_period(vlapic) ? period : 0LL), - vlapic->pt.irq); - create_periodic_time(current, &vlapic->pt, period, - vlapic_lvtt_period(vlapic) ? period : 0, - vlapic->pt.irq, - vlapic_lvtt_period(vlapic) ? vlapic_pt_cb : NULL, - &vlapic->timer_last_update); - 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, val, period); - } - break; + vlapic_update_timer(vlapic, vlapic_get_reg(vlapic, APIC_LVTT)); + break; case APIC_TDCR: vlapic_set_tdcr(vlapic, val & 0xb);
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@citrix.com> --- xen/arch/x86/hvm/vlapic.c | 79 +++++++++++++++++++++++++++++++---------------- 1 file changed, 53 insertions(+), 26 deletions(-)