Xen devel - Jan 2012 - [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 <marcus.granado@eu.citrix.com>
Signed-off-by: George Dunlap <george.dunlap@eu.citrix.com>


diff -r 870db1ebc0df -r bcd2c4f0c502 xen/include/public/xenoprof.h
--- 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)
  /* Transient events for the xenoprof->oprofile cpu buf */
  #define XENOPROF_TRACE_BEGIN 1




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

>>> On 20.01.12 at 19:45, Marcus Granado
<marcus.granado@citrix.com> wrote:
> 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 <marcus.granado@eu.citrix.com>
> Signed-off-by: George Dunlap <george.dunlap@eu.citrix.com>
NAK.
> diff -r 870db1ebc0df -r bcd2c4f0c502 xen/include/public/xenoprof.h
> --- 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.

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
>   /* Transient events for the xenoprof->oprofile cpu buf */
>   #define XENOPROF_TRACE_BEGIN 1

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:
> > 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 <marcus.granado@eu.citrix.com>
> > Signed-off-by: George Dunlap <george.dunlap@eu.citrix.com>
> 
> NAK.
> 
> > diff -r 870db1ebc0df -r bcd2c4f0c502 xen/include/public/xenoprof.h
> > --- 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.


Ian.
> 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
> 
> >   /* Transient events for the xenoprof->oprofile cpu buf */
> >   #define XENOPROF_TRACE_BEGIN 1
> 
> 
> 
> 
> _______________________________________________
> Xen-devel mailing list
> Xen-devel@lists.xensource.com
> http://lists.xensource.com/xen-devel

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

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

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

Jan

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


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

>>> On 23.01.12 at 17:01, George Dunlap
<george.dunlap@citrix.com> wrote:
> 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?
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

On Mon, 2012-01-23 at 16:30 +0000, Jan Beulich wrote:
> > 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

>>> On 23.01.12 at 17:51, George Dunlap
<george.dunlap@citrix.com> wrote:
> On Mon, 2012-01-23 at 16:30 +0000, Jan Beulich wrote:
>> > 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. :-)
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

On 23/01/2012 16:51, "George Dunlap" <george.dunlap@citrix.com>
wrote:
> On Mon, 2012-01-23 at 16:30 +0000, Jan Beulich wrote:
>>> 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. :-)
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

On Mon, 2012-01-23 at 17:31 +0000, Keir Fraser wrote:
> > 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

>>> On 23.01.12 at 18:40, George Dunlap
<george.dunlap@citrix.com> wrote:
> 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