From mboxrd@z Thu Jan 1 00:00:00 1970 From: George Dunlap Subject: Re: [PATCH 3/3] xenoprof: Make the escape code consistent across 32 and 64-bit xen Date: Mon, 23 Jan 2012 16:01:22 +0000 Message-ID: <1327334483.26455.260.camel@elijah> References: <4F19B633.6090105@citrix.com> <4F1D3D1D020000780006E514@nat28.tlf.novell.com> <1327313777.24561.9.camel@zakaz.uk.xensource.com> <4F1D48BA020000780006E55F@nat28.tlf.novell.com> <4F1D7702020000780006E6C2@nat28.tlf.novell.com> Mime-Version: 1.0 Content-Type: multipart/mixed; boundary="=-6/O2dNGlmLiyevdn29Z7" Return-path: In-Reply-To: <4F1D7702020000780006E6C2@nat28.tlf.novell.com> List-Unsubscribe: , List-Post: List-Help: List-Subscribe: , Sender: xen-devel-bounces@lists.xensource.com Errors-To: xen-devel-bounces@lists.xensource.com To: Jan Beulich Cc: George Dunlap , Marcus Granado , xen-devel , "Keir (Xen.org)" , Ian Campbell List-Id: xen-devel@lists.xenproject.org --=-6/O2dNGlmLiyevdn29Z7 Content-Type: text/plain; charset="UTF-8" Content-Transfer-Encoding: 7bit On Mon, 2012-01-23 at 14:04 +0000, Jan Beulich wrote: > >>> On 23.01.12 at 11:47, "Jan Beulich" wrote: > >>>> On 23.01.12 at 11:16, Ian Campbell wrote: > >> On Mon, 2012-01-23 at 09:57 +0000, Jan Beulich wrote: > >>> >>> On 20.01.12 at 19:45, Marcus Granado wrote: > >>> > --- a/xen/include/public/xenoprof.h Wed Jan 18 17:39:19 2012 +0000 > >>> > +++ b/xen/include/public/xenoprof.h Wed Jan 18 17:46:28 2012 +0000 > >>> > @@ -68,7 +68,7 @@ struct event_log { > >>> > }; > >>> > > >>> > /* PC value that indicates a special code */ > >>> > -#define XENOPROF_ESCAPE_CODE ~0UL > >>> > +#define XENOPROF_ESCAPE_CODE (~0ULL) > >>> > >>> You cannot just go and change a public definition, as the consuming > >>> side will break. You need to introduce a new escape code, and > >>> provide a mechanism to negotiate (between hypervisor and kernel) > >>> which one to use. > >> > >> This seems more like a bug fix to me than an interface change, the fact > >> that both producer and consumer had the bug notwithstanding. > >> > >> Since this is a developer oriented interface I think we can be a little > >> more relaxed than we would be for other interface changes. In this case > >> we are fixing 32on64 (quite common) at the expense of 32on32 (very > >> uncommon these days, especially for the suybset of developers wanting to > >> do profiling) and leaving 64on64 (quite common) as it is . That seems > >> like a worthwhile tradeoff to me. > > > > I see your point. However, I'd still like to be careful with this - what > > we think things ought to be used for doesn't necessarily cover all > > cases that they are in reality. > > I see that it got applied unchanged. It introduces two warnings in > the 32-bit build, which not only fails that build, but also indicates > that the situation likely wasn't fully analyzed. The attached patch fixes the build (with the original patch un-reverted of course), by making the internal calls explicitly take 64-bit values for eip, rather than "unsigned long". Will that suffice? -George --=-6/O2dNGlmLiyevdn29Z7 Content-Disposition: attachment; filename="xenoprof-explicit-u64.diff" Content-Type: text/x-patch; name="xenoprof-explicit-u64.diff"; charset="UTF-8" Content-Transfer-Encoding: 7bit xenoprof: Use uint64_t explicitly for internal calls A recent changeset to make XENOPROF_ESCAPE_CODE consistent across 32- and 64-bit builds caused a build failure, because values were passed through functions as "unsigned long". Replace these with uint64_t explicitly. Also remove redundant function prototype from perfmon.c, now that it's in a header file. Signed-off-by: George Dunlap diff -r 137c16a83e40 xen/arch/ia64/xen/oprofile/perfmon.c --- a/xen/arch/ia64/xen/oprofile/perfmon.c Mon Jan 23 09:42:12 2012 +0000 +++ b/xen/arch/ia64/xen/oprofile/perfmon.c Mon Jan 23 15:58:36 2012 +0000 @@ -38,8 +38,6 @@ #include /* for vmx_user_mode() */ // XXX move them to an appropriate header file -extern void xenoprof_log_event(struct vcpu *vcpu, struct pt_regs * regs, - unsigned long eip, int mode, int event); extern int is_active(struct domain *d); static int allow_virq; diff -r 137c16a83e40 xen/common/xenoprof.c --- a/xen/common/xenoprof.c Mon Jan 23 09:42:12 2012 +0000 +++ b/xen/common/xenoprof.c Mon Jan 23 15:58:36 2012 +0000 @@ -475,7 +475,7 @@ static int xenoprof_buf_space(struct dom /* Check for space and add a sample. Return 1 if successful, 0 otherwise. */ static int xenoprof_add_sample(struct domain *d, xenoprof_buf_t *buf, - unsigned long eip, int mode, int event) + uint64_t eip, int mode, int event) { int head, tail, size; @@ -512,7 +512,7 @@ static int xenoprof_add_sample(struct do } int xenoprof_add_trace(struct domain *d, struct vcpu *vcpu, - unsigned long eip, int mode) + uint64_t eip, int mode) { xenoprof_buf_t *buf = d->xenoprof->vcpu[vcpu->vcpu_id].buffer; @@ -527,7 +527,7 @@ int xenoprof_add_trace(struct domain *d, } void xenoprof_log_event(struct vcpu *vcpu, - struct cpu_user_regs * regs, unsigned long eip, + struct cpu_user_regs * regs, uint64_t eip, int mode, int event) { struct domain *d = vcpu->domain; diff -r 137c16a83e40 xen/include/xen/xenoprof.h --- a/xen/include/xen/xenoprof.h Mon Jan 23 09:42:12 2012 +0000 +++ b/xen/include/xen/xenoprof.h Mon Jan 23 15:58:36 2012 +0000 @@ -69,7 +69,7 @@ int is_passive(struct domain *d); void free_xenoprof_pages(struct domain *d); int xenoprof_add_trace(struct domain *d, struct vcpu *v, - unsigned long eip, int mode); + uint64_t eip, int mode); #define PMU_OWNER_NONE 0 #define PMU_OWNER_XENOPROF 1 @@ -78,7 +78,7 @@ int acquire_pmu_ownship(int pmu_ownershi void release_pmu_ownship(int pmu_ownership); void xenoprof_log_event(struct vcpu *vcpu, - struct cpu_user_regs * regs, unsigned long eip, + struct cpu_user_regs * regs, uint64_t eip, int mode, int event); #endif /* __XEN__XENOPROF_H__ */ --=-6/O2dNGlmLiyevdn29Z7 Content-Type: text/plain; charset="us-ascii" MIME-Version: 1.0 Content-Transfer-Encoding: 7bit Content-Disposition: inline _______________________________________________ Xen-devel mailing list Xen-devel@lists.xensource.com http://lists.xensource.com/xen-devel --=-6/O2dNGlmLiyevdn29Z7--