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(-)
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 <teddy.astie@vates.tech>
---
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(-)
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,
--
2.45.2
Teddy Astie | Vates XCP-ng Developer
XCP-ng & Xen Orchestra - Vates solutions
web: https://vates.techOn 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 <teddy.astie@vates.tech> 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
Hello, Le 04/12/2024 à 14:01, Andrew Cooper a écrit : > 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 <teddy.astie@vates.tech> > > 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 I took a look on the page you sent in the past [1], but I am quite confused on how they should be named then. Intel System Programming Guide in 2.2 "Modes of operation" defines these as "operating modes" - Protected mode (which are named in this patch X86_MODE_32BIT and X86_MODE_16BIT) - Real-address mode (which is named in this patch X86_MODE_REAL) - Virtual-8086 mode (X86_MODE_VM86) - IA-32e mode (X86_MODE_64BIT and in some cases X86_MODE_32BIT) The page you sent introduce multiple "processor modes" with variants of real mode (RM16, RM32), VM86 (VM16, VM16E0, VM16E1), Protected mode (PM16, PM32) and IA32-mode "long mode" (CM16, CM32, PM64). I think we can add all these modes as separate defines, while having a way (macro ?) to check some attributes (e.g is it "long mode" ? with a bit indicating a "long mode" mode). [1] https://sandpile.org/x86/mode.htm Teddy Teddy Astie | Vates XCP-ng Developer XCP-ng & Xen Orchestra - Vates solutions web: https://vates.tech
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 <teddy.astie@vates.tech>
Reviewed-by: Jan Beulich <jbeulich@suse.com>
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
© 2016 - 2024 Red Hat, Inc.