Re: [PATCH 3/3] xenoprof: Make the escape code consistent across 32 and 64-bit xen

[George Dunlap <george.dunlap@citrix.com>]

[-- Attachment #1: Type: text/plain, Size: 2119 bytes --]

On Mon, 2012-01-23 at 14:04 +0000, Jan Beulich wrote:
> >>> On 23.01.12 at 11:47, "Jan Beulich" <JBeulich@suse.com> wrote:
> >>>> On 23.01.12 at 11:16, Ian Campbell <Ian.Campbell@citrix.com> wrote:
> >> On Mon, 2012-01-23 at 09:57 +0000, Jan Beulich wrote:
> >>> >>> On 20.01.12 at 19:45, Marcus Granado <marcus.granado@citrix.com> 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

[-- Attachment #2: xenoprof-explicit-u64.diff --]
[-- Type: text/x-patch, Size: 3033 bytes --]

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 <george.dunlap@eu.citrix.com>

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 <asm/vmx.h>    /* 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__ */

[-- Attachment #3: Type: text/plain, Size: 138 bytes --]

_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xensource.com
http://lists.xensource.com/xen-devel