Re: [Xen-devel] [PATCH 1/1] xentrace: Add TRC_HW_VCHIP

[Tim Deegan <tim@xxxxxxx>]

At 07:25 -0400 on 28 Mar (1395987951), Don Slutz wrote:
> This add a set of trace events that track the setup of various
> virtual chips related to timers in domU.
> 
> This set is hpet, pit (i8253, i8254), rtc (MC146818), apic (lapic),
> and pic (i8259).  The pmtimer is not traced since it does not have a
> changeable rate.
> 
> Signed-off-by: Don Slutz <dslutz@xxxxxxxxxxx>

Seems like a good idea; thanks for the patch.  A few comments:

You include get_cycles() in a lot of places, but AIUI the TRACE_*D
macros already included the TSC in the trace record.

> @@ -152,8 +154,11 @@ static void rtc_timer_update(RTCState *s)
>                  else
>                      delta = period - ((now - s->start_time) % period);
>                  if ( s->hw.cmos_data[RTC_REG_B] & RTC_PIE )
> +                {
> +                    TRACE_3D(TRC_HW_VCHIP_RTC_START_TIMER, get_cycles(), 
> delta, period);

Please linewrap to <80 characters.

> @@ -950,6 +955,7 @@ void vlapic_tdt_msr_set(struct vlapic *vlapic, uint64_t 
> value)
>  
>          vlapic->hw.tdt_msr = value;
>          /* .... reprogram tdt timer */
> +        TRACE_4D(TRC_HW_VCHIP_LAPIC_START_TIMER, get_cycles(), delta, 0, 
> vlapic->pt.irq);
>          create_periodic_time(v, &vlapic->pt, delta, 0,
>                               vlapic->pt.irq, vlapic_tdt_pt_cb,
>                               &vlapic->timer_last_update);
> @@ -962,6 +968,7 @@ void vlapic_tdt_msr_set(struct vlapic *vlapic, uint64_t 
> value)
>          /* trigger a timer event if needed */
>          if ( value > 0 )
>          {
> +            TRACE_4D(TRC_HW_VCHIP_LAPIC_START_TIMER, get_cycles(), 0, 0, 
> vlapic->pt.irq);
>              create_periodic_time(v, &vlapic->pt, 0, 0,
>                                   vlapic->pt.irq, vlapic_tdt_pt_cb,
>                                   &vlapic->timer_last_update);

Likewise here (and anywhere else). 

> @@ -997,12 +1005,18 @@ static int __vlapic_accept_pic_intr(struct vcpu *v)
>                !vlapic_disabled(vlapic)) ||
>               /* LAPIC has LVT0 unmasked for ExtInts? */
>               ((lvt0 & (APIC_MODE_MASK|APIC_LVT_MASKED)) == APIC_DM_EXTINT) ||
> +             /* LAPIC has LVT0 unmasked for Fixed? */
> +             ((lvt0 & (APIC_MODE_MASK|APIC_LVT_MASKED)) == APIC_DM_FIXED) ||

What on earth is this doing here?

> diff --git a/xen/include/public/trace.h b/xen/include/public/trace.h
> index e2f60a6..e6f25f6 100644
> --- a/xen/include/public/trace.h
> +++ b/xen/include/public/trace.h
> @@ -86,6 +86,7 @@
>  /* Trace classes for Hardware */
>  #define TRC_HW_PM           0x00801000   /* Power management traces */
>  #define TRC_HW_IRQ          0x00802000   /* Traces relating to the handling 
> of IRQs */
> +#define TRC_HW_VCHIP        0x00804000   /* Traces relating to the handling 
> of virtual chips */

I think most of these belong under TRC_HVM, with the other emulated
hardware.  But I guess we might see PIT traces for PV guests?  In that
case, maybe the timers could have a new class like like TRC_VTIMER.

In any case, please avoid _HW_, which is used elsewhere for _real_
hardware events.

>  /* Trace events per class */
>  #define TRC_LOST_RECORDS        (TRC_GEN + 1)
> @@ -244,6 +245,27 @@
>  #define TRC_HW_IRQ_UNMAPPED_VECTOR    (TRC_HW_IRQ + 0x7)
>  #define TRC_HW_IRQ_HANDLED            (TRC_HW_IRQ + 0x8)
>  
> +/* Trace events for virtual chips */
> +#define TRC_HW_VCHIP_HPET_START_TIMER   (TRC_HW_VCHIP + 0x1)
> +#define TRC_HW_VCHIP_8254_START_TIMER   (TRC_HW_VCHIP + 0x2)

Can you use _PIT_ for those of us who don't always remember the old
Intel part numbers?

> +#define TRC_HW_VCHIP_RTC_START_TIMER    (TRC_HW_VCHIP + 0x3)
> +#define TRC_HW_VCHIP_LAPIC_START_TIMER  (TRC_HW_VCHIP + 0x4)
> +#define TRC_HW_VCHIP_HPET_STOP_TIMER    (TRC_HW_VCHIP + 0x5)
> +#define TRC_HW_VCHIP_8254_STOP_TIMER    (TRC_HW_VCHIP + 0x6)
> +#define TRC_HW_VCHIP_RTC_STOP_TIMER     (TRC_HW_VCHIP + 0x7)
> +#define TRC_HW_VCHIP_LAPIC_STOP_TIMER   (TRC_HW_VCHIP + 0x8)
> +#define TRC_HW_VCHIP_8254_TIMER_CB      (TRC_HW_VCHIP + 0x9)
> +#define TRC_HW_VCHIP_LAPIC_TIMER_CB     (TRC_HW_VCHIP + 0xA)
> +#define TRC_HW_VCHIP_8259_INT_OUTPUT    (TRC_HW_VCHIP + 0xB)

Likewise, _PIC_ would be better.

Cheers,

Tim.


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