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

[Andrew Cooper <andrew.cooper3@xxxxxxxxxx>]

On 26/08/2013 15:29, Jan Beulich wrote:
>>>> On 26.08.13 at 16:18, Andrew Cooper <andrew.cooper3@xxxxxxxxxx> 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

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@xxxxxxxxxxxxx
http://lists.xen.org/xen-devel