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

[Andrew Cooper <andrew.cooper3@citrix.com>]

On 26/08/2013 11:18, Jan Beulich wrote:
> --- a/xen/include/asm-x86/hvm/vmx/vmx.h
> +++ b/xen/include/asm-x86/hvm/vmx/vmx.h
> @@ -282,27 +282,44 @@ extern uint8_t posted_intr_vector;
>  
>  static inline void __vmptrld(u64 addr)
>  {
> -    asm volatile ( VMPTRLD_OPCODE
> -                   MODRM_EAX_06
> +    asm volatile (
> +#ifdef HAVE_GAS_VMX
> +                   "vmptrld %0\n"
> +#else
> +                   VMPTRLD_OPCODE MODRM_EAX_06
> +#endif
>                     /* CF==1 or ZF==1 --> crash (ud2) */
>                     UNLIKELY_START(be, vmptrld)
>                     "\tud2\n"
>                     UNLIKELY_END_SECTION
>                     :
> +#ifdef HAVE_GAS_VMX
> +                   : "m" (addr)
> +#else
>                     : "a" (&addr)
> +#endif
>                     : "memory");
>  }
>  
>  static inline void __vmpclear(u64 addr)
>  {
> -    asm volatile ( VMCLEAR_OPCODE
> -                   MODRM_EAX_06
> +    asm volatile (
> +#ifdef HAVE_GAS_VMX
> +                   "vmclear %0\n"
> +#else
> +                   VMCLEAR_OPCODE MODRM_EAX_06
> +#endif
>                     /* CF==1 or ZF==1 --> crash (ud2) */
>                     UNLIKELY_START(be, vmclear)
>                     "\tud2\n"
>                     UNLIKELY_END_SECTION
> +#ifdef HAVE_GAS_VMX
> +                   : "+m" (addr)

Why is this "+m" ?  (and specifically different to some of its counterparts)

> +                   :
> +#else
>                     :
>                     : "a" (&addr)
> +#endif
>                     : "memory");
>  }
>  
> @@ -325,30 +342,47 @@ static inline unsigned long __vmread(uns
>  
>  static inline void __vmwrite(unsigned long field, unsigned long value)
>  {
> -    asm volatile ( VMWRITE_OPCODE
> -                   MODRM_EAX_ECX
> +    asm volatile (
> +#ifdef HAVE_GAS_VMX
> +                   "vmwrite %1, %0\n"
> +#else
> +                   VMWRITE_OPCODE MODRM_EAX_ECX
> +#endif
>                     /* CF==1 or ZF==1 --> crash (ud2) */
>                     UNLIKELY_START(be, vmwrite)
>                     "\tud2\n"
>                     UNLIKELY_END_SECTION
>                     : 
> +#ifdef HAVE_GAS_VMX
> +                   : "r" (field) , "rm" (value)
> +#else
>                     : "a" (field) , "c" (value)
> +#endif
>                     : "memory");
>  }
>  
> -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 %q2, %1\n\t"
> +#else
> +                   VMREAD_OPCODE MODRM_EAX_ECX
> +#endif
> +                   /* CF==1 or ZF==1 --> rc = 0 */
> +                   "setnbe %0"
> +#ifdef HAVE_GAS_VMX
> +                   : "=qm" (okay), "=rm" (*value)
> +                   : "r" (field)
> +#else
> +                   : "=qm" (okay), "=c" (*value)
> +                   : "a" (field)
> +#endif

Why the %q2 qualifer with "r" (field).  Would it not be cleaner as %2
and "q" (field) ?  (And similar lower down)

(My understanding of asm constraints is not fantastic, so I apologise if
there is a good and obvious reason for this)

Otherwise, looking much cleaner. :)

~Andrew

>                     : "memory");
>  
> -    return ecx;
> +    return okay;
>  }
>  
>  static inline void __invept(int type, u64 eptp, u64 gpa)
> @@ -365,14 +399,22 @@ static inline void __invept(int type, u6
>           !cpu_has_vmx_ept_invept_single_context )
>          type = INVEPT_ALL_CONTEXT;
>  
> -    asm volatile ( INVEPT_OPCODE
> -                   MODRM_EAX_08
> +    asm volatile (
> +#ifdef HAVE_GAS_EPT
> +                   "invept %0, %q1\n"
> +#else
> +                   INVEPT_OPCODE MODRM_EAX_08
> +#endif
>                     /* CF==1 or ZF==1 --> crash (ud2) */
>                     UNLIKELY_START(be, invept)
>                     "\tud2\n"
>                     UNLIKELY_END_SECTION
>                     :
> +#ifdef HAVE_GAS_EPT
> +                   : "m" (operand), "r" (type)
> +#else
>                     : "a" (&operand), "c" (type)
> +#endif
>                     : "memory" );
>  }
>  
> @@ -385,7 +427,12 @@ static inline void __invvpid(int type, u
>      } __attribute__ ((packed)) operand = {vpid, 0, gva};
>  
>      /* Fix up #UD exceptions which occur when TLBs are flushed before VMXON. */
> -    asm volatile ( "1: " INVVPID_OPCODE MODRM_EAX_08
> +    asm volatile ( "1: "
> +#ifdef HAVE_GAS_EPT
> +                   "invvpid %0, %q1\n"
> +#else
> +                   INVVPID_OPCODE MODRM_EAX_08
> +#endif
>                     /* CF==1 or ZF==1 --> crash (ud2) */
>                     UNLIKELY_START(be, invvpid)
>                     "\tud2\n"
> @@ -393,7 +440,11 @@ static inline void __invvpid(int type, u
>                     "2:"
>                     _ASM_EXTABLE(1b, 2b)
>                     :
> +#ifdef HAVE_GAS_EPT
> +                   : "m" (operand), "r" (type)
> +#else
>                     : "a" (&operand), "c" (type)
> +#endif
>                     : "memory" );
>  }
>