Re: [PATCH 2/4] VMX: move various uses of UD2 out of fast paths

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

[-- Attachment #1.1: Type: text/plain, Size: 5230 bytes --]

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


[-- Attachment #1.2: Type: text/html, Size: 6177 bytes --]

[-- Attachment #2: Type: text/plain, Size: 126 bytes --]

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