From mboxrd@z Thu Jan 1 00:00:00 1970 From: David Vrabel Subject: Re: [PATCH 2/3] trace: improve usefulness of hypercall trace record Date: Tue, 2 Oct 2012 18:06:43 +0100 Message-ID: <506B1F23.105@citrix.com> References: <1349113629-6338-1-git-send-email-david.vrabel@citrix.com> <1349113629-6338-3-git-send-email-david.vrabel@citrix.com> Mime-Version: 1.0 Content-Type: text/plain; charset="us-ascii" Content-Transfer-Encoding: 7bit Return-path: In-Reply-To: List-Unsubscribe: , List-Post: List-Help: List-Subscribe: , Sender: xen-devel-bounces@lists.xen.org Errors-To: xen-devel-bounces@lists.xen.org To: George Dunlap Cc: "xen-devel@lists.xensource.com" List-Id: xen-devel@lists.xenproject.org On 02/10/12 17:09, George Dunlap wrote: > On Mon, Oct 1, 2012 at 6:47 PM, David Vrabel wrote: >> From: David Vrabel >> >> 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 >> Acked-by: George Dunlap > > 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