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

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

On 26/08/2013 16:31, Jan Beulich wrote:
> With the hex byte emission we were taking away a good part of
> flexibility from the compiler, as for simplicity reasons these were
> built using fixed operands. All half way modern build environments
> would allow using the mnemonics (but we can't disable the hex variants
> yet, since the binutils around at the time gcc 4.1 got released didn't
> support these yet).
>
> I didn't convert __vmread() yet because that would, just like for
> __vmread_safe(), imply converting to a macro so that the output operand
> can be the caller supplied variable rather than an intermediate one. As
> that would require touching all invocation points of __vmread() (of
> which there are quite a few), I'd first like to be certain the approach
> is acceptable; the main question being whether the now conditional code
> might be considered to cause future maintenance issues, and the second
> being that of parameter/argument ordering (here I made __vmread_safe()
> match __vmwrite(), but one could also take the position that read and
> write should use the inverse order of one another, in line with the
> actual instruction operands).
>
> Additionally I was quite puzzled to find that all the asm()-s involved
> here have memory clobbers - what are they needed for? Or can they be
> dropped at least in some cases?
>
> Signed-off-by: Jan Beulich <jbeulich@suse.com>

Reviewed-by: Andrew Cooper <andrew.cooper3@citrix.com>

> ---
> v5: drop "q" operand qualifiers from __invept() and __invvpid() inline
>     assembly (pointed out as questionable by Andrew Cooper), changing
>     the types of their "type" parameters to "unsigned long" (which is
>     by itself a fix to the original code)
>
> --- a/Config.mk
> +++ b/Config.mk
> @@ -4,6 +4,12 @@ ifeq ($(filter /%,$(XEN_ROOT)),)
>  $(error XEN_ROOT must be absolute)
>  endif
>  
> +# Convenient variables
> +comma   := ,
> +squote  := '
> +empty   :=
> +space   := $(empty) $(empty)
> +
>  # fallback for older make
>  realpath = $(wildcard $(foreach file,$(1),$(shell cd -P $(dir $(file)) && echo "$$PWD/$(notdir $(file))")))
>  
> @@ -128,6 +134,21 @@ endef
>  check-$(gcc) = $(call cc-ver-check,CC,0x040100,"Xen requires at least gcc-4.1")
>  $(eval $(check-y))
>  
> +# as-insn: Check whether assembler supports an instruction.
> +# Usage: cflags-y += $(call as-insn "insn",option-yes,option-no)
> +as-insn = $(if $(shell echo 'void _(void) { asm volatile ( $(2) ); }' \
> +                       | $(1) -c -x c -o /dev/null - 2>&1),$(4),$(3))
> +
> +# as-insn-check: Add an option to compilation flags, but only if insn is
> +#                supported by assembler.
> +# Usage: $(call as-insn-check CFLAGS,CC,"nop",-DHAVE_GAS_NOP)
> +as-insn-check = $(eval $(call as-insn-check-closure,$(1),$(2),$(3),$(4)))
> +define as-insn-check-closure
> +    ifeq ($$(call as-insn,$$($(2)),$(3),y,n),y)
> +        $(1) += $(4)
> +    endif
> +endef
> +
>  define buildmakevars2shellvars
>      export PREFIX="$(PREFIX)";                                            \
>      export XEN_SCRIPT_DIR="$(XEN_SCRIPT_DIR)";                            \
> --- a/xen/arch/x86/Rules.mk
> +++ b/xen/arch/x86/Rules.mk
> @@ -28,6 +28,8 @@ CFLAGS += -msoft-float
>  
>  $(call cc-options-add,CFLAGS,CC,$(EMBEDDED_EXTRA_CFLAGS))
>  $(call cc-option-add,CFLAGS,CC,-Wnested-externs)
> +$(call as-insn-check,CFLAGS,CC,"vmcall",-DHAVE_GAS_VMX)
> +$(call as-insn-check,CFLAGS,CC,"invept (%rax)$$(comma)%rax",-DHAVE_GAS_EPT)
>  
>  ifeq ($(supervisor_mode_kernel),y)
>  CFLAGS += -DCONFIG_X86_SUPERVISOR_MODE_KERNEL=1
> --- a/xen/arch/x86/hvm/vmx/vmcs.c
> +++ b/xen/arch/x86/hvm/vmx/vmcs.c
> @@ -1292,12 +1292,11 @@ void vmx_do_resume(struct vcpu *v)
>      reset_stack_and_jump(vmx_asm_do_vmentry);
>  }
>  
> -static unsigned long vmr(unsigned long field)
> +static inline unsigned long vmr(unsigned long field)
>  {
> -    int rc;
>      unsigned long val;
> -    val = __vmread_safe(field, &rc);
> -    return rc ? 0 : val;
> +
> +    return __vmread_safe(field, &val) ? val : 0;
>  }
>  
>  static void vmx_dump_sel(char *name, uint32_t selector)
> --- a/xen/arch/x86/hvm/vmx/vvmx.c
> +++ b/xen/arch/x86/hvm/vmx/vvmx.c
> @@ -951,13 +951,11 @@ fallback:
>          vvmcs_to_shadow(vvmcs, field[i]);
>  }
>  
> -static void shadow_to_vvmcs(void *vvmcs, unsigned int field)
> +static inline void shadow_to_vvmcs(void *vvmcs, unsigned int field)
>  {
> -    u64 value;
> -    int rc;
> +    unsigned long value;
>  
> -    value = __vmread_safe(field, &rc);
> -    if ( !rc )
> +    if ( __vmread_safe(field, &value) )
>          __set_vvmcs(vvmcs, field, value);
>  }
>  
> --- a/xen/include/asm-x86/hvm/vmx/vmx.h
> +++ b/xen/include/asm-x86/hvm/vmx/vmx.h
> @@ -282,27 +282,43 @@ 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)
> +#else
>                     : "a" (&addr)
> +#endif
>                     : "memory");
>  }
>  
> @@ -325,33 +341,50 @@ 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 %2, %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
>                     : "memory");
>  
> -    return ecx;
> +    return okay;
>  }
>  
> -static inline void __invept(int type, u64 eptp, u64 gpa)
> +static inline void __invept(unsigned long type, u64 eptp, u64 gpa)
>  {
>      struct {
>          u64 eptp, gpa;
> @@ -365,18 +398,26 @@ 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, %1\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" );
>  }
>  
> -static inline void __invvpid(int type, u16 vpid, u64 gva)
> +static inline void __invvpid(unsigned long type, u16 vpid, u64 gva)
>  {
>      struct {
>          u64 vpid:16;
> @@ -385,7 +426,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, %1\n"
> +#else
> +                   INVVPID_OPCODE MODRM_EAX_08
> +#endif
>                     /* CF==1 or ZF==1 --> crash (ud2) */
>                     UNLIKELY_START(be, invvpid)
>                     "\tud2\n"
> @@ -393,7 +439,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" );
>  }
>  
>
>