From mboxrd@z Thu Jan 1 00:00:00 1970 From: Don Slutz Subject: Re: [PATCH 1/1] xentrace: Add TRC_HW_VCHIP Date: Fri, 28 Mar 2014 09:49:04 -0400 Message-ID: <53357DD0.60600@terremark.com> References: <1396005951-29160-1-git-send-email-dslutz@verizon.com> <20140328120416.GC87844@deinos.phlegethon.org> Mime-Version: 1.0 Content-Type: text/plain; charset="us-ascii"; Format="flowed" Content-Transfer-Encoding: 7bit Return-path: In-Reply-To: <20140328120416.GC87844@deinos.phlegethon.org> List-Unsubscribe: , List-Post: List-Help: List-Subscribe: , Sender: xen-devel-bounces@lists.xen.org Errors-To: xen-devel-bounces@lists.xen.org To: Tim Deegan Cc: Ian Campbell , Stefano Stabellini , George Dunlap , Ian Jackson , Don Slutz , xen-devel@lists.xen.org, Jan Beulich List-Id: xen-devel@lists.xenproject.org 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 > 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. >