| Message ID | 20210920172529.24932-6-andrew.cooper3@citrix.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | xen/trace: Fix leakage of uninitialised stack into the tracebuffer | expand |
On 20.09.2021 19:25, Andrew Cooper wrote: > 1) vpic_ack_pending_irq() calls vlapic_accept_pic_intr() twice, once in the > TRACE_2D() instantiation and once "for real". Make the call only once. > > 2) vlapic_accept_pic_intr() similarly calls __vlapic_accept_pic_intr() twice, > although this is more complicated to disentangle. > > v cannot be NULL because it has already been dereferenced in the function, > causing the ternary expression to always call __vlapic_accept_pic_intr(). > However, the return expression of the function takes care to skip the call > if this vCPU isn't the PIC target. As __vlapic_accept_pic_intr() is far > from trivial, make the TRACE_2D() semantics match the return semantics by > only calling __vlapic_accept_pic_intr() when the vCPU is the PIC target. > > 3) hpet_set_timer() duplicates calls to hpet_tick_to_ns(). Pull the logic out > which simplifies both the TRACE and create_periodic_time() calls. > > 4) lapic_rearm() makes multiple calls to vlapic_lvtt_period(). Pull it out > into a local variable. > > vlapic_accept_pic_intr() is called on every VMEntry, so this is a reduction in > VMEntry complexity across the board. > > Signed-off-by: Andrew Cooper <andrew.cooper3@citrix.com> Reviewed-by: Jan Beulich <jbeulich@suse.com> > --- a/xen/arch/x86/hvm/vpic.c > +++ b/xen/arch/x86/hvm/vpic.c > @@ -512,14 +512,15 @@ void vpic_irq_negative_edge(struct domain *d, int irq) > > int vpic_ack_pending_irq(struct vcpu *v) > { > - int irq; > + int irq, accept; Strictly speaking "accept" wants to be bool, and ... > struct hvm_hw_vpic *vpic = &v->domain->arch.hvm.vpic[0]; > > ASSERT(has_vpic(v->domain)); > > - TRACE_2D(TRC_HVM_EMUL_PIC_PEND_IRQ_CALL, vlapic_accept_pic_intr(v), > - vpic->int_output); > - if ( !vlapic_accept_pic_intr(v) || !vpic->int_output ) > + accept = vlapic_accept_pic_intr(v); ... vlapic_accept_pic_intr() would eventually also want to be converted to return bool. Jan
On 21/09/2021 13:18, Jan Beulich wrote: > On 20.09.2021 19:25, Andrew Cooper wrote: >> 1) vpic_ack_pending_irq() calls vlapic_accept_pic_intr() twice, once in the >> TRACE_2D() instantiation and once "for real". Make the call only once. >> >> 2) vlapic_accept_pic_intr() similarly calls __vlapic_accept_pic_intr() twice, >> although this is more complicated to disentangle. >> >> v cannot be NULL because it has already been dereferenced in the function, >> causing the ternary expression to always call __vlapic_accept_pic_intr(). >> However, the return expression of the function takes care to skip the call >> if this vCPU isn't the PIC target. As __vlapic_accept_pic_intr() is far >> from trivial, make the TRACE_2D() semantics match the return semantics by >> only calling __vlapic_accept_pic_intr() when the vCPU is the PIC target. >> >> 3) hpet_set_timer() duplicates calls to hpet_tick_to_ns(). Pull the logic out >> which simplifies both the TRACE and create_periodic_time() calls. >> >> 4) lapic_rearm() makes multiple calls to vlapic_lvtt_period(). Pull it out >> into a local variable. >> >> vlapic_accept_pic_intr() is called on every VMEntry, so this is a reduction in >> VMEntry complexity across the board. >> >> Signed-off-by: Andrew Cooper <andrew.cooper3@citrix.com> > Reviewed-by: Jan Beulich <jbeulich@suse.com> Thanks. > >> --- a/xen/arch/x86/hvm/vpic.c >> +++ b/xen/arch/x86/hvm/vpic.c >> @@ -512,14 +512,15 @@ void vpic_irq_negative_edge(struct domain *d, int irq) >> >> int vpic_ack_pending_irq(struct vcpu *v) >> { >> - int irq; >> + int irq, accept; > Strictly speaking "accept" wants to be bool, and ... > >> struct hvm_hw_vpic *vpic = &v->domain->arch.hvm.vpic[0]; >> >> ASSERT(has_vpic(v->domain)); >> >> - TRACE_2D(TRC_HVM_EMUL_PIC_PEND_IRQ_CALL, vlapic_accept_pic_intr(v), >> - vpic->int_output); >> - if ( !vlapic_accept_pic_intr(v) || !vpic->int_output ) >> + accept = vlapic_accept_pic_intr(v); > ... vlapic_accept_pic_intr() would eventually also want to be > converted to return bool. Yeah, but given the potential for backport here, I specifically avoided unrelated changed. We've got loads of functions wanting to change to bool. ~Andrew
diff --git a/xen/arch/x86/hvm/hpet.c b/xen/arch/x86/hvm/hpet.c index ee756abb824d..475c3f8bf471 100644 --- a/xen/arch/x86/hvm/hpet.c +++ b/xen/arch/x86/hvm/hpet.c @@ -240,6 +240,7 @@ static void hpet_set_timer(HPETState *h, unsigned int tn, uint64_t tn_cmp, cur_tick, diff; unsigned int irq; unsigned int oneshot; + s_time_t diff_ns, period_ns = 0; ASSERT(tn < HPET_TIMER_NUM); ASSERT(rw_is_write_locked(&h->lock)); @@ -311,13 +312,15 @@ static void hpet_set_timer(HPETState *h, unsigned int tn, * status register) before another interrupt can be delivered. */ oneshot = !timer_is_periodic(h, tn) || timer_level(h, tn); + diff_ns = hpet_tick_to_ns(h, diff); + if ( !oneshot ) + period_ns = hpet_tick_to_ns(h, h->hpet.period[tn]); + TRACE_2_LONG_4D(TRC_HVM_EMUL_HPET_START_TIMER, tn, irq, - TRC_PAR_LONG(hpet_tick_to_ns(h, diff)), - TRC_PAR_LONG(oneshot ? 0LL : - hpet_tick_to_ns(h, h->hpet.period[tn]))); - create_periodic_time(vhpet_vcpu(h), &h->pt[tn], - hpet_tick_to_ns(h, diff), - oneshot ? 0 : hpet_tick_to_ns(h, h->hpet.period[tn]), + TRC_PAR_LONG(diff_ns), + TRC_PAR_LONG(period_ns)); + + create_periodic_time(vhpet_vcpu(h), &h->pt[tn], diff_ns, period_ns, irq, timer_level(h, tn) ? hpet_timer_fired : NULL, timer_level(h, tn) ? (void *)(unsigned long)tn : NULL, timer_level(h, tn)); diff --git a/xen/arch/x86/hvm/vlapic.c b/xen/arch/x86/hvm/vlapic.c index 5e21fb4937d9..b8c84458ffdc 100644 --- a/xen/arch/x86/hvm/vlapic.c +++ b/xen/arch/x86/hvm/vlapic.c @@ -1267,15 +1267,18 @@ static int __vlapic_accept_pic_intr(struct vcpu *v) int vlapic_accept_pic_intr(struct vcpu *v) { + bool target, accept = false; + if ( vlapic_hw_disabled(vcpu_vlapic(v)) || !has_vpic(v->domain) ) return 0; - TRACE_2D(TRC_HVM_EMUL_LAPIC_PIC_INTR, - (v == v->domain->arch.hvm.i8259_target), - v ? __vlapic_accept_pic_intr(v) : -1); + target = v == v->domain->arch.hvm.i8259_target; + if ( target ) + accept = __vlapic_accept_pic_intr(v); + + TRACE_2D(TRC_HVM_EMUL_LAPIC_PIC_INTR, target, accept); - return ((v == v->domain->arch.hvm.i8259_target) && - __vlapic_accept_pic_intr(v)); + return target && accept; } void vlapic_adjust_i8259_target(struct domain *d) @@ -1449,6 +1452,7 @@ static void lapic_rearm(struct vlapic *s) { unsigned long tmict; uint64_t period, tdt_msr; + bool is_periodic; s->pt.irq = vlapic_get_reg(s, APIC_LVTT) & APIC_VECTOR_MASK; @@ -1464,12 +1468,15 @@ static void lapic_rearm(struct vlapic *s) period = ((uint64_t)APIC_BUS_CYCLE_NS * (uint32_t)tmict * s->hw.timer_divisor); + is_periodic = vlapic_lvtt_period(s); + TRACE_2_LONG_3D(TRC_HVM_EMUL_LAPIC_START_TIMER, TRC_PAR_LONG(period), - TRC_PAR_LONG(vlapic_lvtt_period(s) ? period : 0LL), s->pt.irq); + TRC_PAR_LONG(is_periodic ? period : 0LL), s->pt.irq); + create_periodic_time(vlapic_vcpu(s), &s->pt, period, - vlapic_lvtt_period(s) ? period : 0, + is_periodic ? period : 0, s->pt.irq, - vlapic_lvtt_period(s) ? vlapic_pt_cb : NULL, + is_periodic ? vlapic_pt_cb : NULL, &s->timer_last_update, false); s->timer_last_update = s->pt.last_plt_gtime; } diff --git a/xen/arch/x86/hvm/vpic.c b/xen/arch/x86/hvm/vpic.c index af988a868c8a..91c2c6983393 100644 --- a/xen/arch/x86/hvm/vpic.c +++ b/xen/arch/x86/hvm/vpic.c @@ -512,14 +512,15 @@ void vpic_irq_negative_edge(struct domain *d, int irq) int vpic_ack_pending_irq(struct vcpu *v) { - int irq; + int irq, accept; struct hvm_hw_vpic *vpic = &v->domain->arch.hvm.vpic[0]; ASSERT(has_vpic(v->domain)); - TRACE_2D(TRC_HVM_EMUL_PIC_PEND_IRQ_CALL, vlapic_accept_pic_intr(v), - vpic->int_output); - if ( !vlapic_accept_pic_intr(v) || !vpic->int_output ) + accept = vlapic_accept_pic_intr(v); + + TRACE_2D(TRC_HVM_EMUL_PIC_PEND_IRQ_CALL, accept, vpic->int_output); + if ( !accept || !vpic->int_output ) return -1; irq = vpic_intack(vpic);
1) vpic_ack_pending_irq() calls vlapic_accept_pic_intr() twice, once in the TRACE_2D() instantiation and once "for real". Make the call only once. 2) vlapic_accept_pic_intr() similarly calls __vlapic_accept_pic_intr() twice, although this is more complicated to disentangle. v cannot be NULL because it has already been dereferenced in the function, causing the ternary expression to always call __vlapic_accept_pic_intr(). However, the return expression of the function takes care to skip the call if this vCPU isn't the PIC target. As __vlapic_accept_pic_intr() is far from trivial, make the TRACE_2D() semantics match the return semantics by only calling __vlapic_accept_pic_intr() when the vCPU is the PIC target. 3) hpet_set_timer() duplicates calls to hpet_tick_to_ns(). Pull the logic out which simplifies both the TRACE and create_periodic_time() calls. 4) lapic_rearm() makes multiple calls to vlapic_lvtt_period(). Pull it out into a local variable. vlapic_accept_pic_intr() is called on every VMEntry, so this is a reduction in VMEntry complexity across the board. Signed-off-by: Andrew Cooper <andrew.cooper3@citrix.com> --- CC: Jan Beulich <JBeulich@suse.com> CC: Roger Pau Monné <roger.pau@citrix.com> CC: Wei Liu <wl@xen.org> v2: * New --- xen/arch/x86/hvm/hpet.c | 15 +++++++++------ xen/arch/x86/hvm/vlapic.c | 23 +++++++++++++++-------- xen/arch/x86/hvm/vpic.c | 9 +++++---- 3 files changed, 29 insertions(+), 18 deletions(-)