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

["Jan Beulich" <JBeulich@xxxxxxxx>]

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

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


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