Re: [PATCH v2] x86/vmx: Use asm goto() in _vmx_cpu_up()

[Jan Beulich <jbeulich@xxxxxxxx>]

On 02.04.2025 11:56, Andrew Cooper wrote:
> --- a/xen/arch/x86/hvm/vmx/vmcs.c
> +++ b/xen/arch/x86/hvm/vmx/vmcs.c
> @@ -749,30 +749,15 @@ static int _vmx_cpu_up(bool bsp)
>      if ( bsp && (rc = vmx_cpu_up_prepare(cpu)) != 0 )
>          return rc;
>  
> -    switch ( __vmxon(this_cpu(vmxon_region)) )
> -    {
> -    case -2: /* #UD or #GP */
> -        if ( bios_locked &&
> -             test_bit(X86_FEATURE_SMX, &boot_cpu_data.x86_capability) &&
> -             (!(eax & IA32_FEATURE_CONTROL_ENABLE_VMXON_OUTSIDE_SMX) ||
> -              !(eax & IA32_FEATURE_CONTROL_ENABLE_VMXON_INSIDE_SMX)) )
> -        {
> -            printk("CPU%d: VMXON failed: perhaps because of TXT settings "
> -                   "in your BIOS configuration?\n", cpu);
> -            printk(" --> Disable TXT in your BIOS unless using a secure "
> -                   "bootloader.\n");
> -            return -EINVAL;
> -        }
> -        /* fall through */
> -    case -1: /* CF==1 or ZF==1 */
> -        printk("CPU%d: unexpected VMXON failure\n", cpu);
> -        return -EINVAL;
> -    case 0: /* success */
> -        this_cpu(vmxon) = 1;
> -        break;
> -    default:
> -        BUG();
> -    }
> +    asm goto ( "1: vmxon %[addr]\n\t"
> +               "jbe %l[vmxon_fail]\n\t"
> +               _ASM_EXTABLE(1b, %l[vmxon_fault])
> +               :
> +               : [addr] "m" (this_cpu(vmxon_region))
> +               :
> +               : vmxon_fail, vmxon_fault );

I must have overlooked this in the RFC - you're losing ...

> --- a/xen/arch/x86/include/asm/hvm/vmx/vmx.h
> +++ b/xen/arch/x86/include/asm/hvm/vmx/vmx.h
> @@ -559,25 +559,6 @@ static inline void __vmxoff(void)
>          : : : "memory" );
>  }
>  
> -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 */
> -        "2:\n"
> -        ".section .fixup,\"ax\"\n"
> -        "3: sub $2,%0 ; jmp 2b\n"    /* #UD or #GP --> rc = -2 */
> -        ".previous\n"
> -        _ASM_EXTABLE(1b, 3b)
> -        : "=q" (rc)
> -        : "0" (0), "a" (&addr)
> -        : "memory");

... the memory barrier here. I will admit I'm not sure I see why it's
there, but if the removal was deliberate, then a sentence wants saying
about this in the description. With that or with it re-added:
Reviewed-by: Jan Beulich <jbeulich@xxxxxxxx>

Now that you move the printk()s, I also wouldn't mind if you pruned them
some: Un-wrap the format strings and perhaps purge the full stop.

Jan