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

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

On 26/08/2013 16:07, Andrew Cooper wrote:
> On 26/08/2013 15:29, Jan Beulich wrote:
>>>>> On 26.08.13 at 16:18, Andrew Cooper <andrew.cooper3@citrix.com> wrote:
>>> On 26/08/2013 15:03, Jan Beulich wrote:
>>>> +    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
>>> From what I can work out while googling, the q constraint is equivalent
>>> to the r constraint for 64bit code.
>>>
>>> For consistency sake, I would suggest "=rm" (okay) here
>> And I'd like to keep it the way it is for generality's  sake (i.e. not
>> making the code more 32-bit unclean than we need to).
> Ok
>
>>>> @@ -365,14 +398,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"
>>> Another stray q
>> No - operand 1 is of type "int", and while the high 32 bits get
>> ignored (i.e. we don't need to do any zero- or sign-extension), we
>> still need to specify the 64-bit register name here. Or wait - I
>> thought it would ignore the upper bits, but it's not documented to.
>> In which case this involves more than just dropping the q modifier.
> I was more referring to having a q in the instruction, yet an "r" in the
> parameter list.  I would suggest

Sorry - sent early.  I would suggest putting all constraints beside
their values, rather than mixing them in amongst the asm code itself,
but do admit that this is only a matter of preference.

>
> INV{EPT,VPID} is strictly defined to take r64 as the "type" parameter in
> long mode.  Invalid/unsupported values found in this register can be
> detected based on the state of EFLAGS afterwards.
>
> Therefore, I would suggest possibly changing "int type" to "unsigned
> long type" if we are going to the effort of getting this correct.  It
> shouldn't make a difference currently, as all calls use appropriate
> INVEPT_*_CONTEXT defines.
>
> As for the flags, should we be including "cc" to the clobber list as
> each of the VM*/INV* instructions explicitly sets the flags.  I would
> hope that the toolchain is pessimistic enough to not trust the state of
> the flags across some inline assembly, but I can't find any hard
> information one way or another.
>
> ~Andrew
>
> _______________________________________________
> Xen-devel mailing list
> Xen-devel@lists.xen.org
> http://lists.xen.org/xen-devel