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

[George Dunlap <George.Dunlap@xxxxxxxxxxxxx>]

On Tue, Oct 2, 2012 at 6:06 PM, David Vrabel <david.vrabel@xxxxxxxxxx> wrote:
> On 02/10/12 17:09, George Dunlap wrote:
>> On Mon, Oct 1, 2012 at 6:47 PM, David Vrabel <david.vrabel@xxxxxxxxxx> wrote:
>>> From: David Vrabel <david.vrabel@xxxxxxxxxx>
>>>
>>> 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@xxxxxxxxxx>
>>> Acked-by: George Dunlap <george.dunlap@xxxxxxxxxx>
>>
>> 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.

Great, thanks.

 -G

>
>>> +    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

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