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

[George Dunlap <george.dunlap@xxxxxxxxxx>]

On Mon, 2012-01-23 at 14:04 +0000, Jan Beulich wrote:
> >>> On 23.01.12 at 11:47, "Jan Beulich" <JBeulich@xxxxxxxx> wrote:
> >>>> On 23.01.12 at 11:16, Ian Campbell <Ian.Campbell@xxxxxxxxxx> wrote:
> >> 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:
> >>> > --- 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: xenoprof-explicit-u64.diff
Description: Text Data

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