diff mbox series

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

Message ID 20250402095621.1278093-1-andrew.cooper3@citrix.com (mailing list archive)
State New
Headers show
Series [v2] x86/vmx: Use asm goto() in _vmx_cpu_up() | expand

Commit Message

Andrew Cooper April 2, 2025, 9:56 a.m. UTC
With the new toolchain baseline, we can make use of asm goto() in certain
places, and the VMXON invocation is one example.

This removes the logic to set up rc (including a fixup section where bactraces
have no connection to the invoking function), the logic to decode it, and the
default case which was dead but in a way the compiler couldn't prove
previously.

No functional change.

Signed-off-by: Andrew Cooper <andrew.cooper3@citrix.com>
---
CC: Jan Beulich <JBeulich@suse.com>
CC: Roger Pau Monné <roger.pau@citrix.com>
CC: dmkhn@proton.me

v2:
 * Move error block to the end of the function.

Bloat-o-meter:
  add/remove: 0/0 grow/shrink: 1/1 up/down: 13/-32 (-19)
  Function                                     old     new   delta
  _vmx_cpu_up.cold                            2460    2473     +13
  _vmx_cpu_up                                 1803    1771     -32
---
 xen/arch/x86/hvm/vmx/vmcs.c            | 50 +++++++++++++-------------
 xen/arch/x86/include/asm/hvm/vmx/vmx.h | 19 ----------
 2 files changed, 26 insertions(+), 43 deletions(-)

Comments

Jan Beulich April 2, 2025, 10:03 a.m. UTC | #1
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@suse.com>

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
Andrew Cooper April 2, 2025, 10:05 a.m. UTC | #2
On 02/04/2025 10:56 am, Andrew Cooper wrote:
> diff --git a/xen/arch/x86/hvm/vmx/vmcs.c b/xen/arch/x86/hvm/vmx/vmcs.c
> index 0ba65becec1e..8e99e6f73062 100644
> --- a/xen/arch/x86/hvm/vmx/vmcs.c
> +++ b/xen/arch/x86/hvm/vmx/vmcs.c
> @@ -785,6 +770,23 @@ static int _vmx_cpu_up(bool bsp)
>      vmx_pi_per_cpu_init(cpu);
>  
>      return 0;
> +
> + vmxon_fault:
> +    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");

Oh, I should remove the line splits, and put in some XENLOG_*.

~Andrew
Andrew Cooper April 2, 2025, 10:56 a.m. UTC | #3
On 02/04/2025 11:03 am, Jan Beulich wrote:
> On 02.04.2025 11:56, Andrew Cooper wrote:
>> --- 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.

Hmm.  (Honestly, I wrote the asm goto from scratch, rather than copying it).

The VMXON instruction itself will modify the 4k vmxon_region.  It's an
opaque region, and it might (implementation specific) become
non-coherent with main memory.

However, we don't have a 4k object we could pass as a faux memory
operand, nor can we use asm goto with outputs (That requires a newer
toolchain, and some compiler errata workarounds, but we can do it
conditionally I think).

Lets put the memory clobber back in.

>  With that or with it re-added:
> Reviewed-by: Jan Beulich <jbeulich@suse.com>

Thanks.

~Andrew
diff mbox series

Patch

diff --git a/xen/arch/x86/hvm/vmx/vmcs.c b/xen/arch/x86/hvm/vmx/vmcs.c
index 0ba65becec1e..8e99e6f73062 100644
--- 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 );
+
+    this_cpu(vmxon) = 1;
 
     hvm_asid_init(cpu_has_vmx_vpid ? (1u << VMCS_VPID_WIDTH) : 0);
 
@@ -785,6 +770,23 @@  static int _vmx_cpu_up(bool bsp)
     vmx_pi_per_cpu_init(cpu);
 
     return 0;
+
+ vmxon_fault:
+    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;
+    }
+
+ vmxon_fail:
+    printk("CPU%d: unexpected VMXON failure\n", cpu);
+    return -EINVAL;
 }
 
 int cf_check vmx_cpu_up(void)
diff --git a/xen/arch/x86/include/asm/hvm/vmx/vmx.h b/xen/arch/x86/include/asm/hvm/vmx/vmx.h
index 843f8591b9cf..7c6ba7340744 100644
--- 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");
-
-    return rc;
-}
-
 int cf_check vmx_guest_x86_mode(struct vcpu *v);
 unsigned int vmx_get_cpl(void);