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(-)
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);
--
2.11.0
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
© 2016 - 2026 Red Hat, Inc.