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

[Don Slutz <dslutz@xxxxxxxxxxx>]

On 03/28/14 08:04, Tim Deegan wrote:
> 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.
It should be all.  I see that now.  I have followed:

TRACE_3D(TRC_HW_IRQ_HANDLED, irq, tsc_in, get_cycles());

and I did like seeing the ns value in the output.  I will remove for the 
next version.
> > @@ -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.
Will do.

> > @@ -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).
I will check them all.

> > @@ -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?
Opps, this was testing code that I missed catching.  Will just remove.

> > 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.
I am not sure.  Since the files changed are all in hvm, I would not
expect them for PV.  They may exist for PVH.

It could, but a sub class should be good enough.  Will switch to


#define TRC_HVM      0x0008f000    /* Xen HVM trace            */


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

> >   /* 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?
Sure, I was just follow the file names.

> > +#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.
Will do.
     -Don Slutz

> Cheers,
>
> Tim.

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