xenoprof: Make the escape code consistent across 32 and 64-bit xen

PATCH NAK Marcus Granado 5 participants 12 messages Jan 20, 2012 – Jan 24, 2012

lore

Share & tools

lore

Marcus Granado

author

2012-01-20 18:45 UTC #0 lore raw

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

xenoprof: Make the escape code consistent across 32 and 64-bit xen

At the moment, the xenoprof escape code is defined as "~0UL". Unfortunately, this expands to 0xffffffff on 32-bit systems and 0xffffffffffffffff on 64-bit systems; with the result that while 32-on-32 and 64-in-64 work fine, 32-on-64 (also known as "compat mode") is broken.

This patch makes the definition consistent across architectures. In so doing, it will break old-32-bit-on-new-Xen, and vice versa; but this was seen as an acceptable thing to do.

Signed-off-by Marcus Granado <[email protected]>

Signed-off-by George Dunlap <[email protected]>

xen/include/public/xenoprof.h Wed Jan 18 17:46:28 2012 +0000 renamed from xen/include/public/xenoprof.h Wed Jan 18 17:39:19 2012 +0000 +1 -1

diff -r 870db1ebc0df -r bcd2c4f0c502 xen/include/public/xenoprof.h
@@ -68,7 +68,7 @@ struct event_log {
 };
 
 /* PC value that indicates a special code */
-#define XENOPROF_ESCAPE_CODE ~0UL
+#define XENOPROF_ESCAPE_CODE (~0ULL)
 /* Transient events for the xenoprof->oprofile cpu buf */
 #define XENOPROF_TRACE_BEGIN 1

xenoprof: Make the escape code consistent across 32 and 64-bit xen

This patch makes the definition consistent across architectures. In so doing, it will break old-32-bit-on-new-Xen, and vice versa; but this was seen as an acceptable thing to do.

Signed-off-by Marcus Granado <[email protected]>

Signed-off-by George Dunlap <[email protected]>

xen/include/public/xenoprof.h +1 -1

diff -r 870db1ebc0df -r bcd2c4f0c502 xen/include/public/xenoprof.h
@@ -68,7 +68,7 @@ struct event_log {
 };
 
 /* PC value that indicates a special code */
-#define XENOPROF_ESCAPE_CODE ~0UL
+#define XENOPROF_ESCAPE_CODE (~0ULL)
 /* Transient events for the xenoprof->oprofile cpu buf */
 #define XENOPROF_TRACE_BEGIN 1

_______________________________________________
Xen-devel mailing list
[email protected]
http://lists.xensource.com/xen-devel

Jan Beulich

NAK

2012-01-23 09:57 UTC #1 lore raw

Quoted 9 lines

Quoted 1 line
Quoted 1 line · Marcus Granado
xenoprof: Make the escape code consistent across 32 and 64-bit xen
At the moment, the xenoprof escape code is defined as "~0UL". Unfortunately, this expands to 0xffffffff on 32-bit systems and 0xffffffffffffffff on 64-bit systems; with the result that while 32-on-32 and 64-in-64 work fine, 32-on-64 (also known as "compat mode") is broken.

Quoted 6 lines

This patch makes the definition consistent across architectures. In so doing, it will break old-32-bit-on-new-Xen, and vice versa; but this was seen as an acceptable thing to do.
Signed-off-by Marcus Granado <[email protected]>
Signed-off-by George Dunlap <[email protected]>

NAK.

xen/include/public/xenoprof.h Wed Jan 18 17:46:28 2012 +0000 struct event_log { · line 71

Quoted 9 lines

diff -r 870db1ebc0df -r bcd2c4f0c502 xen/include/public/xenoprof.h
xen/include/public/xenoprof.h Wed Jan 18 17:46:28 2012 +0000 renamed from xen/include/public/xenoprof.h Wed Jan 18 17:39:19 2012 +0000 +1 -1
@@ -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.

I'd also be curious to see the kernel side change accompanying this (if trivial, it might be a good idea to commit this to the old 2.6.18 tree for future reference).

Jan

Quoted 2 lines

xen/include/public/xenoprof.h Wed Jan 18 17:46:28 2012 +0000 renamed from xen/include/public/xenoprof.h Wed Jan 18 17:39:19 2012 +0000 +0 -0
  /* Transient events for the xenoprof->oprofile cpu buf */
  #define XENOPROF_TRACE_BEGIN 1

Ian Campbell

2012-01-23 10:16 UTC #2 lore raw

Quoted 24 lines · Jan Beulich

Quoted 9 lines
Quoted 1 line
Quoted 1 line · Marcus Granado
xenoprof: Make the escape code consistent across 32 and 64-bit xen
At the moment, the xenoprof escape code is defined as "~0UL". Unfortunately, this expands to 0xffffffff on 32-bit systems and 0xffffffffffffffff on 64-bit systems; with the result that while 32-on-32 and 64-in-64 work fine, 32-on-64 (also known as "compat mode") is broken.
Quoted 6 lines
This patch makes the definition consistent across architectures. In so doing, it will break old-32-bit-on-new-Xen, and vice versa; but this was seen as an acceptable thing to do.
Signed-off-by Marcus Granado <[email protected]>
Signed-off-by George Dunlap <[email protected]>
NAK.
Quoted 6 lines
diff -r 870db1ebc0df -r bcd2c4f0c502 xen/include/public/xenoprof.h
xen/include/public/xenoprof.h Wed Jan 18 17:46:28 2012 +0000 renamed from xen/include/public/xenoprof.h Wed Jan 18 17:39:19 2012 +0000 +0 -0
@@ -68,7 +68,7 @@ struct event_log {
  };
 

Quoted 8 lines

xen/include/public/xenoprof.h Wed Jan 18 17:46:28 2012 +0000 struct event_log { · line 71
Quoted 3 lines
xen/include/public/xenoprof.h Wed Jan 18 17:46:28 2012 +0000 renamed from xen/include/public/xenoprof.h Wed Jan 18 17:39:19 2012 +0000 +1 -1
  /* 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.

Ian.

Quoted 16 lines

I'd also be curious to see the kernel side change accompanying this (if trivial, it might be a good idea to commit this to the old 2.6.18 tree for future reference).
Jan
xen/include/public/xenoprof.h Wed Jan 18 17:46:28 2012 +0000 struct event_log { · line 73
Quoted 2 lines
xen/include/public/xenoprof.h Wed Jan 18 17:46:28 2012 +0000 renamed from xen/include/public/xenoprof.h Wed Jan 18 17:39:19 2012 +0000 +0 -0
  /* Transient events for the xenoprof->oprofile cpu buf */
  #define XENOPROF_TRACE_BEGIN 1
_______________________________________________
Xen-devel mailing list
[email protected]
http://lists.xensource.com/xen-devel

Jan Beulich

2012-01-23 10:47 UTC #3 lore raw

Quoted 20 lines

Quoted 1 line
Quoted 1 line · Ian Campbell
Quoted 6 lines · Jan Beulich
Quoted 6 lines
Quoted 1 line
Quoted 1 line · Marcus Granado
xen/include/public/xenoprof.h Wed Jan 18 17:46:28 2012 +0000 renamed from xen/include/public/xenoprof.h Wed Jan 18 17:39:19 2012 +0000 +0 -0
@@ -68,7 +68,7 @@ struct event_log {
  };
 
Quoted 8 lines
xen/include/public/xenoprof.h Wed Jan 18 17:46:28 2012 +0000 struct event_log { · line 71
Quoted 3 lines
xen/include/public/xenoprof.h Wed Jan 18 17:46:28 2012 +0000 renamed from xen/include/public/xenoprof.h Wed Jan 18 17:39:19 2012 +0000 +1 -1
  /* 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.

Quoted 6 lines

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.

Additionally, making as exception here (where the developer nature of the change is deemed obvious) may get us to the question of making a similar adjustment in not as clear a situation at some point in the future.

As negotiation isn't really difficult to implement (e.g. a new ELF note) I don't really see why we shouldn't remain on the safe side here. But Keir would have the final word anyway.

Jan

Jan Beulich

2012-01-23 14:04 UTC #4 lore raw

Quoted 28 lines

Quoted 21 lines
Quoted 2 lines · Jan Beulich
Quoted 1 line · Ian Campbell
Quoted 6 lines · Jan Beulich
Quoted 6 lines
Quoted 1 line
Quoted 1 line · Marcus Granado
xen/include/public/xenoprof.h Wed Jan 18 17:46:28 2012 +0000 renamed from xen/include/public/xenoprof.h Wed Jan 18 17:39:19 2012 +0000 +0 -0
@@ -68,7 +68,7 @@ struct event_log {
  };
 
Quoted 8 lines
xen/include/public/xenoprof.h Wed Jan 18 17:46:28 2012 +0000 struct event_log { · line 71
Quoted 3 lines
xen/include/public/xenoprof.h Wed Jan 18 17:46:28 2012 +0000 renamed from xen/include/public/xenoprof.h Wed Jan 18 17:39:19 2012 +0000 +1 -1
  /* 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.
Quoted 6 lines
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.

Quoted 3 lines

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.

Jan

George Dunlap

2012-01-23 16:01 UTC #5 lore raw

Quoted 28 lines · Jan Beulich

Quoted 28 lines
Quoted 27 lines
Quoted 2 lines · Jan Beulich
Quoted 1 line · Ian Campbell
Quoted 6 lines · Jan Beulich
Quoted 6 lines
Quoted 1 line
Quoted 1 line · Marcus Granado
xen/include/public/xenoprof.h Wed Jan 18 17:46:28 2012 +0000 renamed from xen/include/public/xenoprof.h Wed Jan 18 17:39:19 2012 +0000 +0 -0
@@ -68,7 +68,7 @@ struct event_log {
  };
 
Quoted 8 lines
xen/include/public/xenoprof.h Wed Jan 18 17:46:28 2012 +0000 struct event_log { · line 71
Quoted 3 lines
xen/include/public/xenoprof.h Wed Jan 18 17:46:28 2012 +0000 renamed from xen/include/public/xenoprof.h Wed Jan 18 17:39:19 2012 +0000 +1 -1
  /* 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.

Quoted 7 lines

Quoted 3 lines
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

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 <[email protected]>

xen/arch/ia64/xen/oprofile/perfmon.c +0 -2

diff -r 137c16a83e40 xen/arch/ia64/xen/oprofile/perfmon.c
@@ -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;

xen/common/xenoprof.c +3 -3

diff -r 137c16a83e40 xen/common/xenoprof.c
@@ -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;

xen/include/xen/xenoprof.h +2 -2

diff -r 137c16a83e40 xen/include/xen/xenoprof.h
@@ -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__ */

_______________________________________________
Xen-devel mailing list
[email protected]
http://lists.xensource.com/xen-devel

Jan Beulich

2012-01-23 16:30 UTC #6 lore raw

Quoted 34 lines

Quoted 1 line
Quoted 1 line · George Dunlap
Quoted 32 lines · Jan Beulich
Quoted 31 lines
Quoted 21 lines
Quoted 2 lines · Jan Beulich
Quoted 1 line · Ian Campbell
Quoted 6 lines · Jan Beulich
Quoted 6 lines
Quoted 1 line
Quoted 1 line · Marcus Granado
xen/include/public/xenoprof.h Wed Jan 18 17:46:28 2012 +0000 renamed from xen/include/public/xenoprof.h Wed Jan 18 17:39:19 2012 +0000 +0 -0
@@ -68,7 +68,7 @@ struct event_log {
  };
 
Quoted 8 lines
xen/include/public/xenoprof.h Wed Jan 18 17:46:28 2012 +0000 struct event_log { · line 71
Quoted 3 lines
xen/include/public/xenoprof.h Wed Jan 18 17:46:28 2012 +0000 renamed from xen/include/public/xenoprof.h Wed Jan 18 17:39:19 2012 +0000 +1 -1
  /* 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.
Quoted 6 lines
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.

Quoted 7 lines

Quoted 3 lines
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?

Looks correct and sufficient, but with the original patch already reverted folding the changes here into the original and re-submitting would probably the best route to go.

Jan

George Dunlap

2012-01-23 16:51 UTC #7 lore raw

Quoted 7 lines · Jan Beulich

Quoted 3 lines
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?
Looks correct and sufficient, but with the original patch already reverted folding the changes here into the original and re-submitting would probably the best route to go.

I really prefer to keep patches with no functional change separate from those with a pretty major functional change; even if the major functional change is only one line. :-)

-George

Jan Beulich

2012-01-23 17:01 UTC #8 lore raw

Quoted 6 lines

Quoted 1 line
Quoted 1 line · George Dunlap
Quoted 4 lines · Jan Beulich
Quoted 3 lines
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?

Quoted 7 lines

Quoted 3 lines
Looks correct and sufficient, but with the original patch already reverted folding the changes here into the original and re-submitting would probably the best route to go.
I really prefer to keep patches with no functional change separate from those with a pretty major functional change; even if the major functional change is only one line. :-)

Not sure I follow - the (now reverted) patch was buggy, so why not fix the patch and re-submit? And besides - the fixup patch is certainly not "no functional change", at least not on 32-bits.

Jan

Keir Fraser

2012-01-23 17:31 UTC #9 lore raw

In reply to George Dunlap

Quoted 5 lines · George Dunlap

Quoted 4 lines · Jan Beulich
Quoted 3 lines
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?

Quoted 7 lines

Quoted 3 lines
Looks correct and sufficient, but with the original patch already reverted folding the changes here into the original and re-submitting would probably the best route to go.
I really prefer to keep patches with no functional change separate from those with a pretty major functional change; even if the major functional change is only one line. :-)

The one-line major functional change didn't work. Obviously the other changes to make it build must be part of the same logical patch.

-- Keir

George Dunlap

2012-01-23 17:40 UTC #10 lore raw

Quoted 6 lines · Keir Fraser

Quoted 3 lines
I really prefer to keep patches with no functional change separate from those with a pretty major functional change; even if the major functional change is only one line. :-)
The one-line major functional change didn't work. Obviously the other changes to make it build must be part of the same logical patch.

Perhaps "functional" change isn't what I meant, so much as "guest-visible behavior change".

If I were writing this patch series from the beginning, I'd have one patch which "fixed" the various internal functions to use 64-bits for EIP, since even in 32-bit that value in the struct can be 64-bit. That patch by itself could have a bug in it, and so as a reviewer / archaeologist, *I* would have preferred it did just that one thing, so I didn't have to figure out what each individual line was supposed to be doing. Then the guest-visible functional change would have been its own patch.

In any case, I'll send a combined patch tomorrow, since that seems preferred.

-George

Jan Beulich

2012-01-24 08:50 UTC #11 lore raw

Quoted 9 lines

Quoted 1 line
Quoted 1 line · George Dunlap
If I were writing this patch series from the beginning, I'd have one patch which "fixed" the various internal functions to use 64-bits for EIP, since even in 32-bit that value in the struct can be 64-bit. That patch by itself could have a bug in it, and so as a reviewer / archaeologist, *I* would have preferred it did just that one thing, so I didn't have to figure out what each individual line was supposed to be doing. Then the guest-visible functional change would have been its own patch.

This way round it would seem a reasonable split to me. Yet that prerequisite patch wasn't sent as such, but instead as a follow up one.

Jan