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

[Ian Campbell <Ian.Campbell@xxxxxxxxxx>]

On Mon, 2012-01-23 at 09:57 +0000, Jan Beulich wrote:
> >>> On 20.01.12 at 19:45, Marcus Granado <marcus.granado@xxxxxxxxxx> 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@xxxxxxxxxxxxx>
> > Signed-off-by: George Dunlap <george.dunlap@xxxxxxxxxxxxx>
> 
> 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@xxxxxxxxxxxxxxxxxxx
> http://lists.xensource.com/xen-devel



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