Re: [PATCH v2 6/6] x86/vmx: rework VMX wrappers to use `asm goto()`

[Andrew Cooper <andrew.cooper3@xxxxxxxxxx>]

On 03/04/2025 7:23 pm, dmkhn@xxxxxxxxx wrote:
> From: Denis Mukhin <dmukhin@xxxxxxxx>
>
> Improve error handling in VMX wrappers by switching to `asm goto()` where
> possible.
>
> vmread_safe() kept as is because the minimally required baseline GCC does
> not support output operands in `asm goto`.
>
> Resolves: https://gitlab.com/xen-project/xen/-/work_items/210
> Signed-off-by: Denis Mukhin <dmukhin@xxxxxxxx>

I'd suggest limiting this patch to the asm-goto transformations only.

> ---
>  xen/arch/x86/include/asm/hvm/vmx/vmx.h | 141 +++++++++++++------------
>  1 file changed, 73 insertions(+), 68 deletions(-)
>
> diff --git a/xen/arch/x86/include/asm/hvm/vmx/vmx.h 
> b/xen/arch/x86/include/asm/hvm/vmx/vmx.h
> index ed6a6986b9..19d41f7b90 100644
> --- a/xen/arch/x86/include/asm/hvm/vmx/vmx.h
> +++ b/xen/arch/x86/include/asm/hvm/vmx/vmx.h
> @@ -294,54 +294,57 @@ extern uint8_t posted_intr_vector;
>  
>  static always_inline void __vmptrld(u64 addr)
>  {
> -    asm volatile ( "vmptrld %0\n"
> -                   /* CF==1 or ZF==1 --> BUG() */
> -                   UNLIKELY_START(be, vmptrld)
> -                   _ASM_BUGFRAME_TEXT(0)
> -                   UNLIKELY_END_SECTION
> -                   :
> -                   : "m" (addr),
> -                     _ASM_BUGFRAME_INFO(BUGFRAME_bug, __LINE__, __FILE__, 0)
> -                   : "memory" );
> +    asm goto ( "vmptrld %[addr]\n"
> +               "jbe %l[vmfail]\n\t"

Also cosmetic, but the very final line in the asm block ideally doesn't
want the \n\t.  Except, this tends to be hard to spot because of the way
we use macros such as UNLIKELY_START() or _ASM_EXTABLE() which do just
expand to more strings under the hood.

> +               :
> +               : [addr] "m" (addr)
> +               : "memory"
> +               : vmfail );
> +    return;
> +
> + vmfail:
> +    BUG();
>  }
>  
>  static always_inline void __vmpclear(u64 addr)
>  {
> -    asm volatile ( "vmclear %0\n"
> -                   /* CF==1 or ZF==1 --> BUG() */
> -                   UNLIKELY_START(be, vmclear)
> -                   _ASM_BUGFRAME_TEXT(0)
> -                   UNLIKELY_END_SECTION
> -                   :
> -                   : "m" (addr),
> -                     _ASM_BUGFRAME_INFO(BUGFRAME_bug, __LINE__, __FILE__, 0)
> -                   : "memory" );
> +    asm goto ( "vmclear %[addr]\n"
> +               "jbe %l[vmfail]\n\t"
> +               :
> +               : [addr] "m" (addr)
> +               : "memory"
> +               : vmfail );
> +    return;
> +
> + vmfail:
> +    BUG();
>  }
>  
>  static always_inline void __vmread(unsigned long field, unsigned long *value)
>  {
> -    asm volatile ( "vmread %1, %0\n\t"
> -                   /* CF==1 or ZF==1 --> BUG() */
> -                   UNLIKELY_START(be, vmread)
> -                   _ASM_BUGFRAME_TEXT(0)
> -                   UNLIKELY_END_SECTION
> -                   : "=rm" (*value)
> -                   : "r" (field),
> -                     _ASM_BUGFRAME_INFO(BUGFRAME_bug, __LINE__, __FILE__, 0)
> -        );
> +    bool vmfail;
> +
> +    asm volatile ( "vmread %[field], %[value]\n\t"
> +                    "setbe %[vmfail]\n\t"
> +                    : [value] "=rm" (*value), [vmfail] "=rm" (vmfail)
> +                    : [field] "r" (field)
> +                    : "cc" );
> +    if ( vmfail )
> +        BUG();

This is almost certainly not an improvement in generated code.  You now
need register to hold the boolean, where previously there was a jbe to a
ud2 and no extra state required.

Here's an example https://godbolt.org/z/GG4r1c7bK showing the
difference.   (It also shows up the Clang "rm" constraint bug.  Change
vmfail from "=rm" to "=r" to get sane(er) code generation.)

Also, you've added a "cc" clobber.  This is one thing you don't need on
x86; it's simply assumed, given how many instructions update flags
naturally.

> @@ -369,22 +372,22 @@ static inline enum vmx_insn_errno vmread_safe(unsigned 
> long field,
>  static inline enum vmx_insn_errno vmwrite_safe(unsigned long field,
>                                                 unsigned long value)
>  {
> -    unsigned long ret = VMX_INSN_SUCCEED;
> -    bool fail_invalid, fail_valid;
> +    unsigned long ret;
>  
> -    asm volatile ( "vmwrite %[value], %[field]\n\t"
> -                   ASM_FLAG_OUT(, "setc %[invalid]\n\t")
> -                   ASM_FLAG_OUT(, "setz %[valid]\n\t")
> -                   : ASM_FLAG_OUT("=@ccc", [invalid] "=rm") (fail_invalid),
> -                     ASM_FLAG_OUT("=@ccz", [valid] "=rm") (fail_valid)
> -                   : [field] "r" (field),
> -                     [value] "rm" (value) );
> +    asm goto ( "vmwrite %[value], %[field]\n\t"
> +               "jc %l[vmfail_invalid]\n\t"
> +               "jz %l[vmfail_error]\n\t"
> +               :
> +               : [field] "r" (field), [value] "rm" (value)
> +               :
> +               : vmfail_invalid, vmfail_error );
> +    return VMX_INSN_SUCCEED;
>  
> -    if ( unlikely(fail_invalid) )
> -        ret = VMX_INSN_FAIL_INVALID;
> -    else if ( unlikely(fail_valid) )
> -        __vmread(VM_INSTRUCTION_ERROR, &ret);
> + vmfail_invalid:
> +    return VMX_INSN_FAIL_INVALID;
>  
> + vmfail_error:
> +    __vmread(VM_INSTRUCTION_ERROR, &ret);

Something not technically toolchain cleanup, but is in desperate need of
doing.

__vmread() being void and producing an output by pointer is insane, and
leads to ugly code such as this, even if the compiler can usually fix up
behind the scenes.

It would be lovely to have:

unsigned long vmread(unsigned int field)

which can be wrapped by __vmread() with "*value = vmread(field);" or
so.  (Don't go converting all users; that's a huge task).

Then, this vmfail_error can be simply "return
vmread(VM_INSTRUCTION_ERROR);" and you can drop the ret variable.

~Andrew