Re: [PATCH 5/7] xen: Switch to new TRACE() API

[Jan Beulich <jbeulich@xxxxxxxx>]

On 18.03.2024 17:35, Andrew Cooper wrote:
> @@ -736,9 +736,19 @@ static void vlapic_update_timer(struct vlapic *vlapic, 
> uint32_t lvtt,
>              delta = delta * vlapic->hw.timer_divisor / old_divisor;
>          }
>  
> -        TRACE_2_LONG_3D(TRC_HVM_EMUL_LAPIC_START_TIMER, TRC_PAR_LONG(delta),
> -                        TRC_PAR_LONG(is_periodic ? period : 0),
> -                        vlapic->pt.irq);
> +        if ( unlikely(tb_init_done) )
> +        {
> +            struct {
> +                uint64_t delta, period;
> +                uint32_t irq;
> +            } __packed d = {
> +                .delta = delta,
> +                .period = is_periodic ? period : 0,
> +                .irq = vlapic->pt.irq,
> +            };
> +
> +            trace_time(TRC_HVM_EMUL_LAPIC_START_TIMER, sizeof(d), &d);
> +        }

Instead of this open-coding, how about

        if ( is_periodic )
            TRACE_TIME(TRC_HVM_EMUL_LAPIC_START_TIMER, delta, delta >> 32,
                       period, period >> 32, vlapic->pt.irq);
        else
            TRACE_TIME(TRC_HVM_EMUL_LAPIC_START_TIMER, delta, delta >> 32,
                       0, 0, vlapic->pt.irq);

thus improving similarity / grep-ability with ...

> @@ -1208,8 +1218,8 @@ void vlapic_tdt_msr_set(struct vlapic *vlapic, uint64_t 
> value)
>  
>          vlapic->hw.tdt_msr = value;
>          /* .... reprogram tdt timer */
> -        TRACE_2_LONG_3D(TRC_HVM_EMUL_LAPIC_START_TIMER, TRC_PAR_LONG(delta),
> -                        TRC_PAR_LONG(0LL), vlapic->pt.irq);
> +        TRACE_TIME(TRC_HVM_EMUL_LAPIC_START_TIMER, delta, delta >> 32,
> +                   0, 0, vlapic->pt.irq);
>          create_periodic_time(v, &vlapic->pt, delta, 0,
>                               vlapic->pt.irq, vlapic_tdt_pt_cb,
>                               &vlapic->timer_last_update, false);
> @@ -1222,8 +1232,8 @@ void vlapic_tdt_msr_set(struct vlapic *vlapic, uint64_t 
> value)
>          /* trigger a timer event if needed */
>          if ( value > 0 )
>          {
> -            TRACE_2_LONG_3D(TRC_HVM_EMUL_LAPIC_START_TIMER, 
> TRC_PAR_LONG(0LL),
> -                            TRC_PAR_LONG(0LL), vlapic->pt.irq);
> +            TRACE_TIME(TRC_HVM_EMUL_LAPIC_START_TIMER, 0, 0,
> +                       0, 0, vlapic->pt.irq);
>              create_periodic_time(v, &vlapic->pt, 0, 0,
>                                   vlapic->pt.irq, vlapic_tdt_pt_cb,
>                                   &vlapic->timer_last_update, false);

... these.

> @@ -1476,8 +1486,19 @@ static void lapic_rearm(struct vlapic *s)
>                (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(is_periodic ? period : 0LL), s->pt.irq);
> +    if ( unlikely(tb_init_done) )
> +    {
> +         struct {
> +             uint64_t delta, period;
> +             uint32_t irq;
> +        } __packed d = {
> +             .delta = period,
> +             .period = is_periodic ? period : 0,
> +             .irq = s->pt.irq,
> +        };
> +
> +        trace_time(TRC_HVM_EMUL_LAPIC_START_TIMER, sizeof(d), &d);
> +    }

Same here then.

Other than that you had an ack from me for this already, which you may
very well retain / reinstate.

Jan