From mboxrd@z Thu Jan 1 00:00:00 1970 From: Tim Deegan Subject: Re: [PATCH v5 3/4] VMX: use proper instruction mnemonics if assembler supports them Date: Thu, 29 Aug 2013 12:47:42 +0100 Message-ID: <20130829114742.GC75385@ocelot.phlegethon.org> References: <521786A002000078000EE064@nat28.tlf.novell.com> <521787C602000078000EE07F@nat28.tlf.novell.com> <52193148.2090601@citrix.com> <521B36AA02000078000EE496@nat28.tlf.novell.com> <521B1F15.1030104@citrix.com> <521B479902000078000EE505@nat28.tlf.novell.com> <521B528C.8080107@citrix.com> <521B7C6402000078000EE6D4@nat28.tlf.novell.com> <521B63AF.7000008@citrix.com> <521B90F802000078000EE7A3@nat28.tlf.novell.com> Mime-Version: 1.0 Content-Type: text/plain; charset="us-ascii" Content-Transfer-Encoding: 7bit Return-path: Received: from mail6.bemta14.messagelabs.com ([193.109.254.103]) by lists.xen.org with esmtp (Exim 4.72) (envelope-from ) id 1VF0hd-0001uP-3I for xen-devel@lists.xenproject.org; Thu, 29 Aug 2013 11:47:53 +0000 Content-Disposition: inline In-Reply-To: <521B90F802000078000EE7A3@nat28.tlf.novell.com> List-Unsubscribe: , List-Post: List-Help: List-Subscribe: , Sender: xen-devel-bounces@lists.xen.org Errors-To: xen-devel-bounces@lists.xen.org To: Jan Beulich Cc: xen-devel , Keir Fraser , Eddie Dong , Jun Nakajima , Andrew Cooper List-Id: xen-devel@lists.xenproject.org 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.