From mboxrd@z Thu Jan 1 00:00:00 1970 From: Andrew Cooper Subject: Re: [PATCH 2/4] VMX: move various uses of UD2 out of fast paths Date: Fri, 23 Aug 2013 23:06:57 +0100 Message-ID: <5217DD01.9080907@citrix.com> References: <521786A002000078000EE064@nat28.tlf.novell.com> <5217879D02000078000EE07B@nat28.tlf.novell.com> Mime-Version: 1.0 Content-Type: multipart/mixed; boundary="===============8101078259155857226==" Return-path: Received: from mail6.bemta4.messagelabs.com ([85.158.143.247]) by lists.xen.org with esmtp (Exim 4.72) (envelope-from ) id 1VCzVS-0006EL-KX for xen-devel@lists.xenproject.org; Fri, 23 Aug 2013 22:06:58 +0000 In-Reply-To: <5217879D02000078000EE07B@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 List-Id: xen-devel@lists.xenproject.org --===============8101078259155857226== Content-Type: multipart/alternative; boundary="------------080000050605080801070805" --------------080000050605080801070805 Content-Type: text/plain; charset="ISO-8859-1" Content-Transfer-Encoding: 7bit On 23/08/2013 15:02, Jan Beulich wrote: > ... at once making conditional forward jumps, which are statically > predicted to be not taken, only used for the unlikely (error) cases. > > Signed-off-by: Jan Beulich I presume it is intentional to create many different symbols for different ud2 instructions right next to each other? It isn't clear from the commit message, but would certainly be the logical choice for debugging an issue arising from such a bug. As each of these ud2 instructions will never result in continued normal execution, is it necessary to have the 'likely' tag and jump back? I cant see any non-buggy case where they would be executed, at which point they are superfluous. > > --- a/xen/include/asm-x86/asm_defns.h > +++ b/xen/include/asm-x86/asm_defns.h > @@ -67,6 +67,30 @@ void ret_from_intr(void); > #define ASSERT_NOT_IN_ATOMIC > #endif > > +#else > + > +#ifdef __clang__ /* clang's builtin assember can't do .subsection */ > + > +#define UNLIKELY_START_SECTION ".pushsection .fixup,\"ax\"" > +#define UNLIKELY_END_SECTION ".popsection" > + > +#else > + > +#define UNLIKELY_START_SECTION ".subsection 1" > +#define UNLIKELY_END_SECTION ".subsection 0" > + > +#endif > + > +#define UNLIKELY_START(cond, tag) \ > + "j" #cond " .Lunlikely%=.tag;\n\t" \ > + UNLIKELY_START_SECTION "\n" \ > + ".Lunlikely%=.tag:" > + > +#define UNLIKELY_END(tag) \ > + "jmp .Llikely%=.tag;\n\t" \ > + UNLIKELY_END_SECTION "\n" \ > + ".Llikely%=.tag:" > + > #endif > > #endif /* __X86_ASM_DEFNS_H__ */ > --- a/xen/include/asm-x86/hvm/vmx/vmx.h > +++ b/xen/include/asm-x86/hvm/vmx/vmx.h > @@ -285,7 +285,9 @@ static inline void __vmptrld(u64 addr) > asm volatile ( VMPTRLD_OPCODE > MODRM_EAX_06 > /* CF==1 or ZF==1 --> crash (ud2) */ > - "ja 1f ; ud2 ; 1:\n" > + UNLIKELY_START(be, vmptrld) > + "\tud2\n" > + UNLIKELY_END_SECTION > : > : "a" (&addr) > : "memory"); > @@ -296,7 +298,9 @@ static inline void __vmpclear(u64 addr) > asm volatile ( VMCLEAR_OPCODE > MODRM_EAX_06 > /* CF==1 or ZF==1 --> crash (ud2) */ > - "ja 1f ; ud2 ; 1:\n" > + UNLIKELY_START(be, vmclear) > + "\tud2\n" > + UNLIKELY_END_SECTION > : > : "a" (&addr) > : "memory"); > @@ -309,7 +313,9 @@ static inline unsigned long __vmread(uns > asm volatile ( VMREAD_OPCODE > MODRM_EAX_ECX > /* CF==1 or ZF==1 --> crash (ud2) */ > - "ja 1f ; ud2 ; 1:\n" > + UNLIKELY_START(be, vmread) > + "\tud2\n" > + UNLIKELY_END_SECTION > : "=c" (ecx) > : "a" (field) > : "memory"); > @@ -322,7 +328,9 @@ static inline void __vmwrite(unsigned lo > asm volatile ( VMWRITE_OPCODE > MODRM_EAX_ECX > /* CF==1 or ZF==1 --> crash (ud2) */ > - "ja 1f ; ud2 ; 1:\n" > + UNLIKELY_START(be, vmwrite) > + "\tud2\n" > + UNLIKELY_END_SECTION > : > : "a" (field) , "c" (value) > : "memory"); > @@ -360,7 +368,9 @@ static inline void __invept(int type, u6 > asm volatile ( INVEPT_OPCODE > MODRM_EAX_08 > /* CF==1 or ZF==1 --> crash (ud2) */ > - "ja 1f ; ud2 ; 1:\n" > + UNLIKELY_START(be, invept) > + "\tud2\n" > + UNLIKELY_END_SECTION > : > : "a" (&operand), "c" (type) > : "memory" ); > @@ -377,7 +387,9 @@ static inline void __invvpid(int type, u > /* Fix up #UD exceptions which occur when TLBs are flushed before VMXON. */ > asm volatile ( "1: " INVVPID_OPCODE MODRM_EAX_08 > /* CF==1 or ZF==1 --> crash (ud2) */ > - "ja 2f ; ud2 ; 2:\n" > + "2: " UNLIKELY_START(be, invvpid) > + "\tud2\n" > + UNLIKELY_END_SECTION "\n" > _ASM_EXTABLE(1b, 2b) Is this logic still correct? To my reading, the logic used to say "if there was an exception while executing INVVPID, then jump to 2: to fix up." where 2: happens to be the end of the inline assembly, bypassing the ud2. After the change, the jbe (formed from UNLIKELY_START) is part of the fixup section, which possibly includes the ud2. As the state of the flags while executing the INVVPID as unknown, this leaves an unknown chance of "fixing up" to a ud2 instruction. ~Andrew > : > : "a" (&operand), "c" (type) > > > > > > _______________________________________________ > Xen-devel mailing list > Xen-devel@lists.xen.org > http://lists.xen.org/xen-devel --------------080000050605080801070805 Content-Type: text/html; charset="ISO-8859-1" Content-Transfer-Encoding: 7bit
On 23/08/2013 15:02, Jan Beulich wrote:
... at once making conditional forward jumps, which are statically
predicted to be not taken, only used for the unlikely (error) cases.

Signed-off-by: Jan Beulich <jbeulich@suse.com>

I presume it is intentional to create many different symbols for different ud2 instructions right next to each other?  It isn't clear from the commit message, but would certainly be the logical choice for debugging an issue arising from such a bug.

As each of these ud2 instructions will never result in continued normal execution, is it necessary to have the 'likely' tag and jump back?  I cant see any non-buggy case where they would be executed, at which point they are superfluous. 


--- a/xen/include/asm-x86/asm_defns.h
+++ b/xen/include/asm-x86/asm_defns.h
@@ -67,6 +67,30 @@ void ret_from_intr(void);
 #define ASSERT_NOT_IN_ATOMIC
 #endif
 
+#else
+
+#ifdef __clang__ /* clang's builtin assember can't do .subsection */
+
+#define UNLIKELY_START_SECTION ".pushsection .fixup,\"ax\""
+#define UNLIKELY_END_SECTION   ".popsection"
+
+#else
+
+#define UNLIKELY_START_SECTION ".subsection 1"
+#define UNLIKELY_END_SECTION   ".subsection 0"
+
+#endif
+
+#define UNLIKELY_START(cond, tag)          \
+        "j" #cond " .Lunlikely%=.tag;\n\t" \
+        UNLIKELY_START_SECTION "\n"        \
+        ".Lunlikely%=.tag:"
+
+#define UNLIKELY_END(tag)                  \
+        "jmp .Llikely%=.tag;\n\t"          \
+        UNLIKELY_END_SECTION "\n"          \
+        ".Llikely%=.tag:"
+
 #endif
 
 #endif /* __X86_ASM_DEFNS_H__ */
--- a/xen/include/asm-x86/hvm/vmx/vmx.h
+++ b/xen/include/asm-x86/hvm/vmx/vmx.h
@@ -285,7 +285,9 @@ static inline void __vmptrld(u64 addr)
     asm volatile ( VMPTRLD_OPCODE
                    MODRM_EAX_06
                    /* CF==1 or ZF==1 --> crash (ud2) */
-                   "ja 1f ; ud2 ; 1:\n"
+                   UNLIKELY_START(be, vmptrld)
+                   "\tud2\n"
+                   UNLIKELY_END_SECTION
                    :
                    : "a" (&addr)
                    : "memory");
@@ -296,7 +298,9 @@ static inline void __vmpclear(u64 addr)
     asm volatile ( VMCLEAR_OPCODE
                    MODRM_EAX_06
                    /* CF==1 or ZF==1 --> crash (ud2) */
-                   "ja 1f ; ud2 ; 1:\n"
+                   UNLIKELY_START(be, vmclear)
+                   "\tud2\n"
+                   UNLIKELY_END_SECTION
                    :
                    : "a" (&addr)
                    : "memory");
@@ -309,7 +313,9 @@ static inline unsigned long __vmread(uns
     asm volatile ( VMREAD_OPCODE
                    MODRM_EAX_ECX
                    /* CF==1 or ZF==1 --> crash (ud2) */
-                   "ja 1f ; ud2 ; 1:\n"
+                   UNLIKELY_START(be, vmread)
+                   "\tud2\n"
+                   UNLIKELY_END_SECTION
                    : "=c" (ecx)
                    : "a" (field)
                    : "memory");
@@ -322,7 +328,9 @@ static inline void __vmwrite(unsigned lo
     asm volatile ( VMWRITE_OPCODE
                    MODRM_EAX_ECX
                    /* CF==1 or ZF==1 --> crash (ud2) */
-                   "ja 1f ; ud2 ; 1:\n"
+                   UNLIKELY_START(be, vmwrite)
+                   "\tud2\n"
+                   UNLIKELY_END_SECTION
                    : 
                    : "a" (field) , "c" (value)
                    : "memory");
@@ -360,7 +368,9 @@ static inline void __invept(int type, u6
     asm volatile ( INVEPT_OPCODE
                    MODRM_EAX_08
                    /* CF==1 or ZF==1 --> crash (ud2) */
-                   "ja 1f ; ud2 ; 1:\n"
+                   UNLIKELY_START(be, invept)
+                   "\tud2\n"
+                   UNLIKELY_END_SECTION
                    :
                    : "a" (&operand), "c" (type)
                    : "memory" );
@@ -377,7 +387,9 @@ static inline void __invvpid(int type, u
     /* Fix up #UD exceptions which occur when TLBs are flushed before VMXON. */
     asm volatile ( "1: " INVVPID_OPCODE MODRM_EAX_08
                    /* CF==1 or ZF==1 --> crash (ud2) */
-                   "ja 2f ; ud2 ; 2:\n"
+                   "2: " UNLIKELY_START(be, invvpid)
+                   "\tud2\n"
+                   UNLIKELY_END_SECTION "\n"
                    _ASM_EXTABLE(1b, 2b)

Is this logic still correct?

To my reading, the logic used to say "if there was an exception while executing INVVPID, then jump to 2: to fix up." where 2: happens to be the end of the inline assembly, bypassing the ud2.

After the change, the jbe (formed from UNLIKELY_START) is part of the fixup section, which possibly includes the ud2.  As the state of the flags while executing the INVVPID as unknown, this leaves an unknown chance of "fixing up" to a ud2 instruction.

~Andrew

                    :
                    : "a" (&operand), "c" (type)






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

--------------080000050605080801070805-- --===============8101078259155857226== Content-Type: text/plain; charset="us-ascii" MIME-Version: 1.0 Content-Transfer-Encoding: 7bit Content-Disposition: inline _______________________________________________ Xen-devel mailing list Xen-devel@lists.xen.org http://lists.xen.org/xen-devel --===============8101078259155857226==--