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

[Tim Deegan <tim@xxxxxxx>]

At 16:31 +0100 on 26 Aug (1377534696), Jan Beulich wrote:
> With the hex byte emission we were taking away a good part of
> flexibility from the compiler, as for simplicity reasons these were
> built using fixed operands. All half way modern build environments
> would allow using the mnemonics (but we can't disable the hex variants
> yet, since the binutils around at the time gcc 4.1 got released didn't
> support these yet).
> 
> I didn't convert __vmread() yet because that would, just like for
> __vmread_safe(), imply converting to a macro so that the output operand
> can be the caller supplied variable rather than an intermediate one. As
> that would require touching all invocation points of __vmread() (of
> which there are quite a few), I'd first like to be certain the approach
> is acceptable; the main question being whether the now conditional code
> might be considered to cause future maintenance issues, and the second
> being that of parameter/argument ordering (here I made __vmread_safe()
> match __vmwrite(), but one could also take the position that read and
> write should use the inverse order of one another, in line with the
> actual instruction operands).
> 
> Additionally I was quite puzzled to find that all the asm()-s involved
> here have memory clobbers - what are they needed for? Or can they be
> dropped at least in some cases?

The vmread/write ones are, I think, red herrings.  We're not allowed to
make assumptions about the memory state of a loaded VMCS anyway.

invept I think does, to make sure all EPT changes have been written.
invvpid too, for similar reasons.

vmptrld/clear I'm not sure about: if we were to (say) copy a VMCS or
move it we'd need that barrier.  (AFAIK we don't do that but we might be
very surprised if we started).

> -static inline unsigned long __vmread_safe(unsigned long field, int *error)
> +static inline bool_t __vmread_safe(unsigned long field, unsigned long *value)
>  {
> -    unsigned long ecx;
> +    bool_t okay;
>  
> -    asm volatile ( VMREAD_OPCODE
> -                   MODRM_EAX_ECX
> -                   /* CF==1 or ZF==1 --> rc = -1 */
> -                   "setna %b0 ; neg %0"
> -                   : "=q" (*error), "=c" (ecx)
> -                   : "0" (0), "a" (field)
> +    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"

This inversion of the (undocumented) return value could be a nasty
surprise for anyone backporting code that uses __vmread_safe().  Can you
please leave it as it was?

Cheers,

Tim.


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