Re: [XEN PATCH] x86/hvm: Use constants for x86 modes

["Alejandro Vallejo" <alejandro.vallejo@cloud.com>]

On Thu Oct 31, 2024 at 1:27 PM GMT, Teddy Astie wrote:
> In many places of x86 HVM code, constants integer are used to indicate in what mode is
> running the CPU (real, v86, 16-bits, 32-bits, 64-bits). However, these constants are
> are written directly as integer which hides the actual meaning of these modes.

Ew. Good riddance. Just a couple of nits...

>
> This patch introduces X86_MODE_* macros and replace those occurences with it.
>
> Signed-off-by Teddy Astie <teddy.astie@vates.tech>
> ---
> I am not sure of other places that uses these integer constants.
> ---
>  xen/arch/x86/hvm/emulate.c           | 16 ++++++++--------
>  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, 34 insertions(+), 24 deletions(-)
>
> diff --git a/xen/arch/x86/hvm/emulate.c b/xen/arch/x86/hvm/emulate.c
> index ecf83795fa..60a7c15bdc 100644
> --- a/xen/arch/x86/hvm/emulate.c
> +++ b/xen/arch/x86/hvm/emulate.c
> @@ -2433,14 +2433,14 @@ 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:
                               ^
                               |
nit: Good time to add newline here...

>              fpu_ctxt->fip.offs = aux->ip;
>              fpu_ctxt->fip.sel  = aux->cs;
>              if ( dval )
> @@ -2451,7 +2451,7 @@ 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_V86:
                              ^
                              |
          +-------------------+
          |
... and here.

>              fpu_ctxt->fip.addr = aux->ip | (aux->cs << 4);
>              if ( dval )
>                  fpu_ctxt->fdp.addr = aux->dp | (aux->ds << 4);
> @@ -2952,11 +2952,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_V86:  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..e0e9bcd22d 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_V86:
>          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 12f8a66458..b77f135a2d 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_V86;
>      __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 c05e0e9326..5032fc3a45 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 dd7d4872b5..29ae23617e 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_V86   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,
> --
> 2.45.2
>
>
>
> Teddy Astie | Vates XCP-ng Intern
>
> XCP-ng & Xen Orchestra - Vates solutions
>
> web: https://vates.tech

Cheers,
Alejandro