Re: [PATCH 2/3] trace: improve usefulness of hypercall trace record

[David Vrabel <david.vrabel@citrix.com>]

On 02/10/12 17:09, George Dunlap wrote:
> On Mon, Oct 1, 2012 at 6:47 PM, David Vrabel <david.vrabel@citrix.com> wrote:
>> From: David Vrabel <david.vrabel@citrix.com>
>>
>> Trace hypercalls using a more useful trace record format.
>>
>> The EIP field is removed (it was always somewhere in the hypercall
>> page) and include selected hypercall arguments (e.g., the number of
>> calls in a multicall, and the number of PTE updates in an mmu_update
>> etc.).  12 bits in the first extra word are used to indicate which
>> arguments are present in the record and what size they are (32 or
>> 64-bit).
>>
>> This is an incompatible record format so a new event ID is used so
>> tools can distinguish between the two formats.
>>
>> Signed-off-by: David Vrabel <david.vrabel@citrix.com>
>> Acked-by: George Dunlap <george.dunlap@citrix.com>
> 
> Looking at it again, I do have one comment (see below) -- so I guess
> that would be technically withdrawing the Ack until we sort it out.
> 
>> diff --git a/xen/arch/x86/trace.c b/xen/arch/x86/trace.c
>> index 27fe150..f2c75bc 100644
>> --- a/xen/arch/x86/trace.c
>> +++ b/xen/arch/x86/trace.c
>> @@ -9,33 +9,28 @@
>>  void trace_hypercall(void)
> 
> In most places, "trace_*" means "Call unconditionally; inside it will
> check tb_init_done", while "__trace_*" means, "I have already checked
> that tb_init_done is set, don't bother checking it."

This isn't something that this patch changes but I'll add a new patch to
change this since it's a trivial change.

>> +    switch ( op )
>> +    {
>> +    case __HYPERVISOR_mmu_update:
>> +        APPEND_ARG32(1); /* count */
>> +        break;
>> +    case __HYPERVISOR_multicall:
>> +        APPEND_ARG32(1); /* count */
>> +        break;
>> +    case __HYPERVISOR_grant_table_op:
>> +        APPEND_ARG32(0); /* cmd */
>> +        APPEND_ARG32(2); /* count */
>> +        break;
>> +    case __HYPERVISOR_vcpu_op:
>> +        APPEND_ARG32(0); /* cmd */
>> +        APPEND_ARG32(1); /* vcpuid */
>> +        break;
>> +    case __HYPERVISOR_mmuext_op:
>> +        APPEND_ARG32(1); /* count */
>> +        break;
>> +    case __HYPERVISOR_sched_op:
>> +        APPEND_ARG32(0); /* cmd */
>> +        break;
>> +    }
> 
> I may have commented on this before -- I wonder if doing some kind of
> array might be better than a big switch statement.  I think with only
> a few hypercalls, a switch statement is probably acceptable; but as we
> add more, the code is going to get bloated.

I'll see what I can come up with.

David