Re: [PATCH v5 3/4] VMX: use proper instruction mnemonics if assembler supports them

[Tim Deegan <tim@xen.org>]

At 14:27 +0100 on 29 Aug (1377786433), Jan Beulich wrote:
> >>> On 29.08.13 at 15:11, Tim Deegan <tim@xen.org> wrote:
> > At 13:30 +0100 on 29 Aug (1377783058), Jan Beulich wrote:
> >> >>> On 29.08.13 at 13:47, Tim Deegan <tim@xen.org> wrote:
> >> > At 16:31 +0100 on 26 Aug (1377534696), Jan Beulich wrote:
> >> >> -static inline unsigned long __vmread_safe(unsigned long field, int *error)
> >> >> +static inline bool_t __vmread_safe(unsigned long field, unsigned long 
> > *value)
> >> >>  {
> >> >> -    unsigned long ecx;
> >> >> +    bool_t okay;
> >> >>  
> >> >> -    asm volatile ( VMREAD_OPCODE
> >> >> -                   MODRM_EAX_ECX
> >> >> -                   /* CF==1 or ZF==1 --> rc = -1 */
> >> >> -                   "setna %b0 ; neg %0"
> >> >> -                   : "=q" (*error), "=c" (ecx)
> >> >> -                   : "0" (0), "a" (field)
> >> >> +    asm volatile (
> >> >> +#ifdef HAVE_GAS_VMX
> >> >> +                   "vmread %2, %1\n\t"
> >> >> +#else
> >> >> +                   VMREAD_OPCODE MODRM_EAX_ECX
> >> >> +#endif
> >> >> +                   /* CF==1 or ZF==1 --> rc = 0 */
> >> >> +                   "setnbe %0"
> >> > 
> >> > This inversion of the (undocumented) return value could be a nasty
> >> > surprise for anyone backporting code that uses __vmread_safe().  Can you
> >> > please leave it as it was?
> >> 
> >> The prior return value was the value read
> > 
> > Sorry, I had somehow missed this.  That's enough to cause compile issues
> > so I guess it'll be obvious that the function has changed.  Would be
> > nice if the new verions had a comment to say when it returns 0 and when
> > 1.
> 
> If I had named the used variable e.g. "ret" or "rc", I could see the
> need for a comment. It being named "okay" I rather think a
> comment would redundant. But if you're strongly of different
> opinion, I can certainly add such a comment.

No, leave it if you prefer.  It occurs to me that the useful case would
be for someone going the other way (with a patch against xen-unstable
and porting to an older tree) which would require a comment in the
version being removed. :)

Tim.