Re: [PATCH v1 4/5] x86/vmx: remove *_OPCODE

[Andrew Cooper <andrew.cooper3@xxxxxxxxxx>]

On 01/04/2025 11:21 pm, dmkhn@xxxxxxxxx wrote:
> diff --git a/xen/arch/x86/include/asm/hvm/vmx/vmx.h 
> b/xen/arch/x86/include/asm/hvm/vmx/vmx.h
> index 10c0619108..1d63e49288 100644
> --- a/xen/arch/x86/include/asm/hvm/vmx/vmx.h
> +++ b/xen/arch/x86/include/asm/hvm/vmx/vmx.h
> @@ -507,15 +487,17 @@ static inline int __vmxon(u64 addr)
>      int rc;
>  
>      asm volatile ( 
> -        "1: " VMXON_OPCODE MODRM_EAX_06 "\n"
> -        "   setna %b0 ; neg %0\n" /* CF==1 or ZF==1 --> rc = -1 */
> +        "1: vmxon (%[addr])\n"
> +        "   setna %b[rc]\n"
> +        "   neg %[rc]\n"          /* CF==1 or ZF==1 --> rc = -1 */
>          "2:\n"
>          ".section .fixup,\"ax\"\n"
> -        "3: sub $2,%0 ; jmp 2b\n"    /* #UD or #GP --> rc = -2 */
> +        "3: mov $-2, %[rc]\n"
> +        "   jmp 2b\n"             /* #UD   or #GP   --> rc = -2 */
>          ".previous\n"
>          _ASM_EXTABLE(1b, 3b)
> -        : "=q" (rc)
> -        : "0" (0), "a" (&addr)
> +        : [rc] "=q" (rc)
> +        : "0" (0), [addr] "r" (&addr)
>          : "memory");
>  
>      return rc;

A variant of this patch (improvements to __vmxon() helper, or whatever)
probably wants pulling out and doing earlier.

For the function parameter, u64 addr wants to become paddr_t addr.

Use "int rc = 0;" and [rc] "+q" (rc).  That takes away the "0" (0) that
is otherwise unconnected.

Next, "vmxon %[addr]" and [addr] "m" (addr).   The VMXON instruction
strictly takes an m64 operand, and it doesn't need bouncing through
another register.

Finally, __vmx{on,off}() have single callers only in vmcs.c, and really
shouldn't be in vmx.h which is included ~everywhere.  You can move them
into vmcs.c (probably after parse_ept_param_runtime()) to limit their scope.

~Andrew