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

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

On 07/02/17 16:22, Jan Beulich wrote:
>>>> On 07.02.17 at 16:06, <sergey.dyasli@citrix.com> 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.

The always_inline was my doing IIRC, because the use of unlikely
sections caused GCC to create a separate identical functions in each
translation unit, in an attempt to minimise the quantity of out-of-line
code.

~Andrew

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