Re: [PATCH 1/1] xentrace: Add TRC_HW_VCHIP

[Don Slutz <dslutz@verizon.com>]

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@verizon.com>
> 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.
>