[XEN,v2] x86/hvm: Use constants for x86 modes

Commit Message

Teddy Astie Dec. 2, 2024, 9:49 a.m. UTC

In many places of x86 HVM code, constants integer are used to indicate in what mode is
running the CPU (real, vm86, 16-bits, 32-bits, 64-bits). However, these constants are
are written directly as integer which hides the actual meaning of these modes.

This patch introduces X86_MODE_* macros and replace those occurences with it.

Signed-off-by Teddy Astie <[e-mail]>
---
v2:
Formatting changes (alignment, ...)
Renamed v86 to vm86. (Jan)
---
 xen/arch/x86/hvm/emulate.c           | 18 ++++++++++--------
 xen/arch/x86/hvm/hypercall.c         | 13 +++++++------
 xen/arch/x86/hvm/viridian/viridian.c |  9 +++++----
 xen/arch/x86/hvm/vmx/vmx.c           |  9 +++++----
 xen/arch/x86/hvm/vmx/vvmx.c          |  5 +++--
 xen/arch/x86/include/asm/hvm/hvm.h   |  6 ++++++
 6 files changed, 36 insertions(+), 24 deletions(-)

Comments

Jan Beulich Dec. 4, 2024, 11:10 a.m. UTC | #1

On 02.12.2024 10:49, Teddy Astie wrote:
> In many places of x86 HVM code, constants integer are used to indicate in what mode is
> running the CPU (real, vm86, 16-bits, 32-bits, 64-bits). However, these constants are
> are written directly as integer which hides the actual meaning of these modes.
> 
> This patch introduces X86_MODE_* macros and replace those occurences with it.
> 
> Signed-off-by Teddy Astie <[e-mail]>

Reviewed-by: Jan Beulich <[e-mail]>
with further style adjustments (see below) and ideally with ...

> ---
> v2:
> Formatting changes (alignment, ...)
> Renamed v86 to vm86. (Jan)

... this extended to ...

> @@ -2952,11 +2954,11 @@ static const char *guest_x86_mode_to_str(int mode)
>  {
>      switch ( mode )
>      {
> -    case 0:  return "Real";
> -    case 1:  return "v86";
> -    case 2:  return "16bit";
> -    case 4:  return "32bit";
> -    case 8:  return "64bit";
> +    case X86_MODE_REAL:   return "Real";
> +    case X86_MODE_VM86:   return "v86";

... the string literal here.

> @@ -112,23 +113,23 @@ int hvm_hypercall(struct cpu_user_regs *regs)
>  
>      switch ( mode )
>      {
> -    case 8:
> +    case X86_MODE_64BIT:
>          eax = regs->rax;
>          fallthrough;
> -    case 4:
> -    case 2:
> +    case X86_MODE_32BIT:
> +    case X86_MODE_16BIT:
>          if ( currd->arch.monitor.guest_request_userspace_enabled &&
>              eax == __HYPERVISOR_hvm_op &&
> -            (mode == 8 ? regs->rdi : regs->ebx) == HVMOP_guest_request_vm_event )
> +            (mode == X86_MODE_64BIT ? regs->rdi : regs->ebx) == HVMOP_guest_request_vm_event )

This line is too long now.

> @@ -2073,7 +2073,8 @@ int nvmx_handle_vmx_insn(struct cpu_user_regs *regs, unsigned int exit_reason)
>  
>      if ( !(curr->arch.hvm.guest_cr[4] & X86_CR4_VMXE) ||
>           !nestedhvm_enabled(curr->domain) ||
> -         (vmx_guest_x86_mode(curr) < (hvm_long_mode_active(curr) ? 8 : 2)) ||
> +         (vmx_guest_x86_mode(curr) < (hvm_long_mode_active(curr) ? X86_MODE_64BIT
> +                                                                 : X86_MODE_16BIT)) ||

As are these two.

Likely easy enough to adjust while committing.

Jan

Andrew Cooper Dec. 4, 2024, 1:01 p.m. UTC | #2

On 02/12/2024 9:49 am, Teddy Astie wrote:
> In many places of x86 HVM code, constants integer are used to indicate in what mode is
> running the CPU (real, vm86, 16-bits, 32-bits, 64-bits). However, these constants are
> are written directly as integer which hides the actual meaning of these modes.
>
> This patch introduces X86_MODE_* macros and replace those occurences with it.
>
> Signed-off-by Teddy Astie <[e-mail]>

As fed back previously, these are (mostly) not modes.

"Mode" has a specific meaning in the x86 architecture which is not
this.  You are going to have to change your commit message, and choice
of constant names.

~Andrew

Patch

diff --git a/xen/arch/x86/hvm/emulate.c b/xen/arch/x86/hvm/emulate.c
index f2bc6967df..9721fd8d4d 100644
--- a/xen/arch/x86/hvm/emulate.c
+++ b/xen/arch/x86/hvm/emulate.c
@@ -2433,14 +2433,15 @@  static void cf_check hvmemul_put_fpu(
 
         switch ( mode )
         {
-        case 8:
+        case X86_MODE_64BIT:
             fpu_ctxt->fip.addr = aux->ip;
             if ( dval )
                 fpu_ctxt->fdp.addr = aux->dp;
             fpu_ctxt->x[FPU_WORD_SIZE_OFFSET] = 8;
             break;
 
-        case 4: case 2:
+        case X86_MODE_32BIT:
+        case X86_MODE_16BIT:
             fpu_ctxt->fip.offs = aux->ip;
             fpu_ctxt->fip.sel  = aux->cs;
             if ( dval )
@@ -2451,7 +2452,8 @@  static void cf_check hvmemul_put_fpu(
             fpu_ctxt->x[FPU_WORD_SIZE_OFFSET] = mode;
             break;
 
-        case 0: case 1:
+        case X86_MODE_REAL:
+        case X86_MODE_VM86:
             fpu_ctxt->fip.addr = aux->ip | (aux->cs << 4);
             if ( dval )
                 fpu_ctxt->fdp.addr = aux->dp | (aux->ds << 4);
@@ -2952,11 +2954,11 @@  static const char *guest_x86_mode_to_str(int mode)
 {
     switch ( mode )
     {
-    case 0:  return "Real";
-    case 1:  return "v86";
-    case 2:  return "16bit";
-    case 4:  return "32bit";
-    case 8:  return "64bit";
+    case X86_MODE_REAL:   return "Real";
+    case X86_MODE_VM86:   return "v86";
+    case X86_MODE_16BIT:  return "16bit";
+    case X86_MODE_32BIT:  return "32bit";
+    case X86_MODE_64BIT:  return "64bit";
     default: return "Unknown";
     }
 }
diff --git a/xen/arch/x86/hvm/hypercall.c b/xen/arch/x86/hvm/hypercall.c
index 81883c8d4f..dd5b017fd1 100644
--- a/xen/arch/x86/hvm/hypercall.c
+++ b/xen/arch/x86/hvm/hypercall.c
@@ -11,6 +11,7 @@ 
 #include <xen/ioreq.h>
 #include <xen/nospec.h>
 
+#include <asm/hvm/hvm.h>
 #include <asm/hvm/emulate.h>
 #include <asm/hvm/support.h>
 #include <asm/hvm/viridian.h>
@@ -112,23 +113,23 @@  int hvm_hypercall(struct cpu_user_regs *regs)
 
     switch ( mode )
     {
-    case 8:
+    case X86_MODE_64BIT:
         eax = regs->rax;
         fallthrough;
-    case 4:
-    case 2:
+    case X86_MODE_32BIT:
+    case X86_MODE_16BIT:
         if ( currd->arch.monitor.guest_request_userspace_enabled &&
             eax == __HYPERVISOR_hvm_op &&
-            (mode == 8 ? regs->rdi : regs->ebx) == HVMOP_guest_request_vm_event )
+            (mode == X86_MODE_64BIT ? regs->rdi : regs->ebx) == HVMOP_guest_request_vm_event )
             break;
 
         if ( likely(!hvm_get_cpl(curr)) )
             break;
         fallthrough;
-    default:
+    case X86_MODE_VM86:
         regs->rax = -EPERM;
         return HVM_HCALL_completed;
-    case 0:
+    case X86_MODE_REAL:
         break;
     }
 
diff --git a/xen/arch/x86/hvm/viridian/viridian.c b/xen/arch/x86/hvm/viridian/viridian.c
index 21480d9ee7..0e3b824bf0 100644
--- a/xen/arch/x86/hvm/viridian/viridian.c
+++ b/xen/arch/x86/hvm/viridian/viridian.c
@@ -16,6 +16,7 @@ 
 #include <asm/paging.h>
 #include <asm/p2m.h>
 #include <asm/apic.h>
+#include <asm/hvm/hvm.h>
 #include <public/sched.h>
 #include <public/hvm/hvm_op.h>
 
@@ -933,13 +934,13 @@  int viridian_hypercall(struct cpu_user_regs *regs)
 
     switch ( mode )
     {
-    case 8:
+    case X86_MODE_64BIT:
         input.raw = regs->rcx;
         input_params_gpa = regs->rdx;
         output_params_gpa = regs->r8;
         break;
 
-    case 4:
+    case X86_MODE_32BIT:
         input.raw = (regs->rdx << 32) | regs->eax;
         input_params_gpa = (regs->rbx << 32) | regs->ecx;
         output_params_gpa = (regs->rdi << 32) | regs->esi;
@@ -1038,11 +1039,11 @@  int viridian_hypercall(struct cpu_user_regs *regs)
 
     switch ( mode )
     {
-    case 8:
+    case X86_MODE_64BIT:
         regs->rax = output.raw;
         break;
 
-    case 4:
+    case X86_MODE_32BIT:
         regs->rdx = output.raw >> 32;
         regs->rax = (uint32_t)output.raw;
         break;
diff --git a/xen/arch/x86/hvm/vmx/vmx.c b/xen/arch/x86/hvm/vmx/vmx.c
index b6885d0e27..eee1d4b47a 100644
--- a/xen/arch/x86/hvm/vmx/vmx.c
+++ b/xen/arch/x86/hvm/vmx/vmx.c
@@ -886,14 +886,15 @@  int cf_check vmx_guest_x86_mode(struct vcpu *v)
     unsigned long cs_ar_bytes;
 
     if ( unlikely(!(v->arch.hvm.guest_cr[0] & X86_CR0_PE)) )
-        return 0;
+        return X86_MODE_REAL;
     if ( unlikely(guest_cpu_user_regs()->eflags & X86_EFLAGS_VM) )
-        return 1;
+        return X86_MODE_VM86;
     __vmread(GUEST_CS_AR_BYTES, &cs_ar_bytes);
     if ( hvm_long_mode_active(v) &&
          likely(cs_ar_bytes & X86_SEG_AR_CS_LM_ACTIVE) )
-        return 8;
-    return (likely(cs_ar_bytes & X86_SEG_AR_DEF_OP_SIZE) ? 4 : 2);
+        return X86_MODE_64BIT;
+    return (likely(cs_ar_bytes & X86_SEG_AR_DEF_OP_SIZE)
+            ? X86_MODE_32BIT : X86_MODE_16BIT);
 }
 
 static void vmx_save_dr(struct vcpu *v)
diff --git a/xen/arch/x86/hvm/vmx/vvmx.c b/xen/arch/x86/hvm/vmx/vvmx.c
index 78135ca23b..c267b101d1 100644
--- a/xen/arch/x86/hvm/vmx/vvmx.c
+++ b/xen/arch/x86/hvm/vmx/vvmx.c
@@ -411,7 +411,7 @@  static int decode_vmx_inst(struct cpu_user_regs *regs,
     }
     else
     {
-        bool mode_64bit = (vmx_guest_x86_mode(v) == 8);
+        bool mode_64bit = (vmx_guest_x86_mode(v) == X86_MODE_64BIT);
 
         decode->type = VMX_INST_MEMREG_TYPE_MEMORY;
 
@@ -2073,7 +2073,8 @@  int nvmx_handle_vmx_insn(struct cpu_user_regs *regs, unsigned int exit_reason)
 
     if ( !(curr->arch.hvm.guest_cr[4] & X86_CR4_VMXE) ||
          !nestedhvm_enabled(curr->domain) ||
-         (vmx_guest_x86_mode(curr) < (hvm_long_mode_active(curr) ? 8 : 2)) ||
+         (vmx_guest_x86_mode(curr) < (hvm_long_mode_active(curr) ? X86_MODE_64BIT
+                                                                 : X86_MODE_16BIT)) ||
          (exit_reason != EXIT_REASON_VMXON && !nvmx_vcpu_in_vmx(curr)) )
     {
         hvm_inject_hw_exception(X86_EXC_UD, X86_EVENT_NO_EC);
diff --git a/xen/arch/x86/include/asm/hvm/hvm.h b/xen/arch/x86/include/asm/hvm/hvm.h
index 02de18c7d4..dbc37e8b75 100644
--- a/xen/arch/x86/include/asm/hvm/hvm.h
+++ b/xen/arch/x86/include/asm/hvm/hvm.h
@@ -26,6 +26,12 @@  extern bool opt_hvm_fep;
 #define opt_hvm_fep 0
 #endif
 
+#define X86_MODE_REAL  0
+#define X86_MODE_VM86  1
+#define X86_MODE_16BIT 2
+#define X86_MODE_32BIT 4
+#define X86_MODE_64BIT 8
+
 /* Interrupt acknowledgement sources. */
 enum hvm_intsrc {
     hvm_intsrc_none,