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

["Jan Beulich" <JBeulich@xxxxxxxx>]

>>> On 07.02.17 at 16:06, <sergey.dyasli@xxxxxxxxxx> wrote:
> If I understood correctly, you are suggesting the following change:

Mostly.

> --- a/xen/include/asm-x86/hvm/vmx/vmx.h
> +++ b/xen/include/asm-x86/hvm/vmx/vmx.h
> @@ -424,8 +424,8 @@ static inline unsigned long vmread_safe(unsigned long 
> field,
>      return ret;
>  }
>  
> -static always_inline unsigned long vmwrite_safe(unsigned long field,
> -                                                unsigned long value)
> +static always_inline enum vmx_insn_errno vmwrite_safe(unsigned long field,
> +                                                      unsigned long value)
>  {
>      unsigned long ret = 0;
>      bool fail_invalid, fail_valid;
> @@ -440,11 +440,16 @@ static always_inline unsigned long 
> vmwrite_safe(unsigned long field,
>                       [value] GAS_VMX_OP("rm", "c") (value));
>  
>      if ( unlikely(fail_invalid) )
> +    {
>          ret = VMX_INSN_FAIL_INVALID;
> +    }

No need to add braces here and ...

>      else if ( unlikely(fail_valid) )
> +    {
>          __vmread(VM_INSTRUCTION_ERROR, &ret);
> +        BUG_ON(ret >= ~0U);
> +    }
>  
> -    return ret;
> +    return (enum vmx_insn_errno) ret;

... no need for the cast here. (See Andrew's reply for the BUG_ON().)

> And I have noticed one inconsistency: vmwrite_safe() is "always_inline"
> while vmread_safe() is plain "inline". I believe that plain inline is
> enough here, what do you think?

I would assume plain inline to be enough, but maybe the VMX
maintainers know why always_inline was used.

Jan


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