Re: [Xen-devel] [PATCH 1/4] x86/vmx: introduce __vmwrite_safe()

["Jan Beulich" <JBeulich@xxxxxxxx>]

>>> On 31.01.17 at 12:20, <sergey.dyasli@xxxxxxxxxx> wrote:
> --- a/tools/tests/x86_emulator/x86_emulate.h
> +++ b/tools/tests/x86_emulator/x86_emulate.h
> @@ -46,6 +46,12 @@
>  #define MMAP_SZ 16384
>  bool emul_test_make_stack_executable(void);
>  
> +#ifdef __GCC_ASM_FLAG_OUTPUTS__
> +# define ASM_FLAG_OUT(yes, no) yes
> +#else
> +# define ASM_FLAG_OUT(yes, no) no
> +#endif

I ought to be sufficient to put this in
tools/tests/x86_emulator/x86_emulate.c - no need for other
consumers of this header to see it.

> --- a/xen/include/asm-x86/hvm/vmx/vmcs.h
> +++ b/xen/include/asm-x86/hvm/vmx/vmcs.h
> @@ -526,6 +526,7 @@ enum vmx_insn_errno
>      VMX_INSN_VMPTRLD_INVALID_PHYADDR       = 9,
>      VMX_INSN_UNSUPPORTED_VMCS_COMPONENT    = 12,
>      VMX_INSN_VMXON_IN_VMX_ROOT             = 15,
> +    VMX_INSN_FAIL_INVALID                  = ~0UL,
>  };

Is the UL really need here? I'd think -1, ~0, or ~0U to suffice.

> @@ -423,6 +429,29 @@ static inline bool_t __vmread_safe(unsigned long field, 
> unsigned long *value)
>      return okay;
>  }
>  
> +static always_inline unsigned long __vmwrite_safe(unsigned long field,
> +                                                  unsigned long value)

Can we please avoid adding more of these (even double) underscore
prefixed symbols? They're reserved to the compiler, so we should
limit their use to places where we absolutely can't think of better
alternatives.

> +{
> +    unsigned long ret = 0;
> +    bool fail_invalid, fail_valid;
> +
> +    asm volatile ( GAS_VMX_OP("vmwrite %[value], %[field]\n",
> +                              VMWRITE_OPCODE MODRM_EAX_ECX)
> +                   ASM_FLAG_OUT(, "setb %[fail_invalid]\n")

While they're synonyms, I'd prefer setc (and @ccc below) to be
used here, as this is not the result of a comparison you're looking
at.

> +                   ASM_FLAG_OUT(, "sete %[fail_valid]\n")

Similarly setz / @ccz here / below

Also I think for the constraint names (but not the variable ones) you
could omit the fail_ prefixes, to help readability.

Plus, strictly speaking it is wrong for instruction mnemonics to start
in column zero. Granted we have many violations of this, but please
add \t here to avoid introducing more slightly wrong code.

> +                   : ASM_FLAG_OUT("=@ccb", [fail_invalid] "=rm") 
> (fail_invalid),
> +                     ASM_FLAG_OUT("=@cce", [fail_valid] "=rm") (fail_valid)
> +                   : [field] GAS_VMX_OP("r", "a") (field),
> +                     [value] GAS_VMX_OP("rm", "c") (value));
> +
> +    if ( fail_invalid )
> +        ret = VMX_INSN_FAIL_INVALID;
> +    else if ( fail_valid )
> +        __vmread(VM_INSTRUCTION_ERROR, &ret);

If already you don't fold this into the asm(), please at least add
unlikely().

> --- a/xen/include/xen/lib.h
> +++ b/xen/include/xen/lib.h
> @@ -162,4 +162,10 @@ void init_constructors(void);
>  void *bsearch(const void *key, const void *base, size_t num, size_t size,
>                int (*cmp)(const void *key, const void *elt));
>  
> +#ifdef __GCC_ASM_FLAG_OUTPUTS__
> +# define ASM_FLAG_OUT(yes, no) yes
> +#else
> +# define ASM_FLAG_OUT(yes, no) no
> +#endif

Is this useful on other than x86?

Jan


_______________________________________________
Xen-devel mailing list
Xen-devel@xxxxxxxxxxxxx
https://lists.xen.org/xen-devel