* [XEN PATCH v2] x86/hvm: Use constants for x86 modes
@ 2024-12-02 9:49 Teddy Astie
2024-12-04 11:10 ` Jan Beulich
` (3 more replies)
0 siblings, 4 replies; 12+ messages in thread
From: Teddy Astie @ 2024-12-02 9:49 UTC (permalink / raw)
To: xen-devel
Cc: Teddy Astie, Jan Beulich, Andrew Cooper, Roger Pau Monné,
Paul Durrant
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.tech
^ permalink raw reply related [flat|nested] 12+ messages in thread* Re: [XEN PATCH v2] x86/hvm: Use constants for x86 modes
2024-12-02 9:49 [XEN PATCH v2] x86/hvm: Use constants for x86 modes Teddy Astie
@ 2024-12-04 11:10 ` Jan Beulich
2024-12-04 13:01 ` Andrew Cooper
` (2 subsequent siblings)
3 siblings, 0 replies; 12+ messages in thread
From: Jan Beulich @ 2024-12-04 11:10 UTC (permalink / raw)
To: Teddy Astie; +Cc: Andrew Cooper, Roger Pau Monné, Paul Durrant, xen-devel
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
^ permalink raw reply [flat|nested] 12+ messages in thread* Re: [XEN PATCH v2] x86/hvm: Use constants for x86 modes
2024-12-02 9:49 [XEN PATCH v2] x86/hvm: Use constants for x86 modes Teddy Astie
2024-12-04 11:10 ` Jan Beulich
@ 2024-12-04 13:01 ` Andrew Cooper
2024-12-05 18:02 ` Teddy Astie
2024-12-18 15:54 ` Andrew Cooper
2024-12-18 17:08 ` [PATCH v3] " Andrew Cooper
3 siblings, 1 reply; 12+ messages in thread
From: Andrew Cooper @ 2024-12-04 13:01 UTC (permalink / raw)
To: Teddy Astie, xen-devel; +Cc: Jan Beulich, Roger Pau Monné, Paul Durrant
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
^ permalink raw reply [flat|nested] 12+ messages in thread
* Re: [XEN PATCH v2] x86/hvm: Use constants for x86 modes
2024-12-04 13:01 ` Andrew Cooper
@ 2024-12-05 18:02 ` Teddy Astie
0 siblings, 0 replies; 12+ messages in thread
From: Teddy Astie @ 2024-12-05 18:02 UTC (permalink / raw)
To: Andrew Cooper, xen-devel; +Cc: Jan Beulich, Roger Pau Monné, Paul Durrant
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
^ permalink raw reply [flat|nested] 12+ messages in thread
* Re: [XEN PATCH v2] x86/hvm: Use constants for x86 modes
2024-12-02 9:49 [XEN PATCH v2] x86/hvm: Use constants for x86 modes Teddy Astie
2024-12-04 11:10 ` Jan Beulich
2024-12-04 13:01 ` Andrew Cooper
@ 2024-12-18 15:54 ` Andrew Cooper
2024-12-18 16:23 ` Andrew Cooper
2024-12-18 16:56 ` Andrew Cooper
2024-12-18 17:08 ` [PATCH v3] " Andrew Cooper
3 siblings, 2 replies; 12+ messages in thread
From: Andrew Cooper @ 2024-12-18 15:54 UTC (permalink / raw)
To: Teddy Astie, xen-devel; +Cc: Jan Beulich, Roger Pau Monné, Paul Durrant
On 02/12/2024 9:49 am, Teddy Astie wrote:
> 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,
We discussed this in the x86 meeting. These want to live next to
hvm_guest_x86_mode() with a comment stating that they're not
architectural modes.
During your work, you seem to have only looked at the the VMX side of
things.
There are several callers of hvm_guest_x86_mode() and
svm_guest_x86_mode() missed. Also an unnecessary include, and a couple
of overly long lines.
https://xenbits.xen.org/gitweb/?p=people/andrewcoop/xen.git;a=commitdiff;h=4f8f63d49776d69ed9a978b6601c13c54c579a98
is the incremental fix on top of this patch.
Does this look reasonable?
I've just realised that the check in nvmx_handle_vmx_insn() is an
incredibly complicated way of expressing guest_cr[0].PE, and we've got
the same opencoded elsewhere, so I'll also prepare a patch prerequisite
patch to sort that out, then rebase this over it.
~Andrew
^ permalink raw reply [flat|nested] 12+ messages in thread* Re: [XEN PATCH v2] x86/hvm: Use constants for x86 modes
2024-12-18 15:54 ` Andrew Cooper
@ 2024-12-18 16:23 ` Andrew Cooper
2024-12-18 16:56 ` Andrew Cooper
1 sibling, 0 replies; 12+ messages in thread
From: Andrew Cooper @ 2024-12-18 16:23 UTC (permalink / raw)
To: Teddy Astie, xen-devel; +Cc: Jan Beulich, Roger Pau Monné, Paul Durrant
On 18/12/2024 3:54 pm, Andrew Cooper wrote:
> On 02/12/2024 9:49 am, Teddy Astie wrote:
>> 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,
> We discussed this in the x86 meeting. These want to live next to
> hvm_guest_x86_mode() with a comment stating that they're not
> architectural modes.
>
> During your work, you seem to have only looked at the the VMX side of
> things.
>
> There are several callers of hvm_guest_x86_mode() and
> svm_guest_x86_mode() missed. Also an unnecessary include, and a couple
> of overly long lines.
>
> https://xenbits.xen.org/gitweb/?p=people/andrewcoop/xen.git;a=commitdiff;h=4f8f63d49776d69ed9a978b6601c13c54c579a98
> is the incremental fix on top of this patch.
>
> Does this look reasonable?
Bah, randconfig says this doesn't quite work.
https://gitlab.com/xen-project/people/andyhhp/xen/-/jobs/8677688167 The
constants need to be available in the !HVM case too.
That will involve moving them higher into hvm.h and where they are in
this hunk is probably ok. I'll move the comment too.
~Andrew
^ permalink raw reply [flat|nested] 12+ messages in thread* Re: [XEN PATCH v2] x86/hvm: Use constants for x86 modes
2024-12-18 15:54 ` Andrew Cooper
2024-12-18 16:23 ` Andrew Cooper
@ 2024-12-18 16:56 ` Andrew Cooper
1 sibling, 0 replies; 12+ messages in thread
From: Andrew Cooper @ 2024-12-18 16:56 UTC (permalink / raw)
To: Teddy Astie, xen-devel; +Cc: Jan Beulich, Roger Pau Monné, Paul Durrant
On 18/12/2024 3:54 pm, Andrew Cooper wrote:
> On 02/12/2024 9:49 am, Teddy Astie wrote:
>> 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,
> We discussed this in the x86 meeting. These want to live next to
> hvm_guest_x86_mode() with a comment stating that they're not
> architectural modes.
>
> During your work, you seem to have only looked at the the VMX side of
> things.
>
> There are several callers of hvm_guest_x86_mode() and
> svm_guest_x86_mode() missed. Also an unnecessary include, and a couple
> of overly long lines.
>
> https://xenbits.xen.org/gitweb/?p=people/andrewcoop/xen.git;a=commitdiff;h=4f8f63d49776d69ed9a978b6601c13c54c579a98
> is the incremental fix on top of this patch.
>
> Does this look reasonable?
>
>
> I've just realised that the check in nvmx_handle_vmx_insn() is an
> incredibly complicated way of expressing guest_cr[0].PE, and we've got
> the same opencoded elsewhere, so I'll also prepare a patch prerequisite
> patch to sort that out, then rebase this over it.
No. It's subtly not (only) a PE check, so lets leave it as is for now.
I'll update the comment to say that some users rely on the order of
constants.
~Andrew
^ permalink raw reply [flat|nested] 12+ messages in thread
* [PATCH v3] x86/hvm: Use constants for x86 modes
2024-12-02 9:49 [XEN PATCH v2] x86/hvm: Use constants for x86 modes Teddy Astie
` (2 preceding siblings ...)
2024-12-18 15:54 ` Andrew Cooper
@ 2024-12-18 17:08 ` Andrew Cooper
2024-12-19 8:18 ` Jan Beulich
2024-12-19 10:46 ` Teddy Astie
3 siblings, 2 replies; 12+ messages in thread
From: Andrew Cooper @ 2024-12-18 17:08 UTC (permalink / raw)
To: Xen-devel; +Cc: Teddy Astie, Andrew Cooper, Jan Beulich, Roger Pau Monné
From: Teddy Astie <teddy.astie@vates.tech>
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>
Signed-off-by: Andrew Cooper <andrew.cooper3@citrix.com>
---
CC: Jan Beulich <JBeulich@suse.com>
CC: Roger Pau Monné <roger.pau@citrix.com>
CC: Teddy Astie <teddy.astie@vates.tech>
v3:
* Leave a comment behind explaining why these aren't all modes
* Fix long lines
* Convert more instances (svm_guest_x86_mode, hvm_latch_shinfo_size, xenoprof)
---
xen/arch/x86/hvm/emulate.c | 18 ++++++++++--------
xen/arch/x86/hvm/hvm.c | 4 +++-
xen/arch/x86/hvm/hypercall.c | 17 +++++++++--------
xen/arch/x86/hvm/svm/svm.c | 8 ++++----
xen/arch/x86/hvm/viridian/viridian.c | 8 ++++----
xen/arch/x86/hvm/vmx/vmx.c | 9 +++++----
xen/arch/x86/hvm/vmx/vvmx.c | 5 +++--
xen/arch/x86/include/asm/hvm/hvm.h | 14 ++++++++++++++
xen/arch/x86/oprofile/xenoprof.c | 6 +++---
9 files changed, 55 insertions(+), 34 deletions(-)
diff --git a/xen/arch/x86/hvm/emulate.c b/xen/arch/x86/hvm/emulate.c
index d3006f094a69..76467b76c047 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/hvm.c b/xen/arch/x86/hvm/hvm.c
index 74e58c653e6f..922c9b3af64d 100644
--- a/xen/arch/x86/hvm/hvm.c
+++ b/xen/arch/x86/hvm/hvm.c
@@ -3974,7 +3974,9 @@ static void hvm_latch_shinfo_size(struct domain *d)
*/
if ( current->domain == d )
{
- d->arch.has_32bit_shinfo = (hvm_guest_x86_mode(current) != 8);
+ d->arch.has_32bit_shinfo =
+ hvm_guest_x86_mode(current) != X86_MODE_64BIT;
+
/*
* Make sure that the timebase in the shared info structure is correct.
*
diff --git a/xen/arch/x86/hvm/hypercall.c b/xen/arch/x86/hvm/hypercall.c
index 81883c8d4f60..6f8dfdff4ac6 100644
--- a/xen/arch/x86/hvm/hypercall.c
+++ b/xen/arch/x86/hvm/hypercall.c
@@ -112,23 +112,24 @@ 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 )
+ eax == __HYPERVISOR_hvm_op &&
+ (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;
}
@@ -198,7 +199,7 @@ enum mc_disposition hvm_do_multicall_call(struct mc_state *state)
{
struct vcpu *curr = current;
- if ( hvm_guest_x86_mode(curr) == 8 )
+ if ( hvm_guest_x86_mode(curr) == X86_MODE_64BIT )
{
struct multicall_entry *call = &state->call;
diff --git a/xen/arch/x86/hvm/svm/svm.c b/xen/arch/x86/hvm/svm/svm.c
index b8f87aa1ed08..62905c2c7acd 100644
--- a/xen/arch/x86/hvm/svm/svm.c
+++ b/xen/arch/x86/hvm/svm/svm.c
@@ -571,12 +571,12 @@ static int cf_check svm_guest_x86_mode(struct vcpu *v)
struct vmcb_struct *vmcb = v->arch.hvm.svm.vmcb;
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;
if ( hvm_long_mode_active(v) && likely(vmcb->cs.l) )
- return 8;
- return likely(vmcb->cs.db) ? 4 : 2;
+ return X86_MODE_64BIT;
+ return vmcb->cs.db ? X86_MODE_32BIT : X86_MODE_16BIT;
}
static void cf_check svm_cpuid_policy_changed(struct vcpu *v)
diff --git a/xen/arch/x86/hvm/viridian/viridian.c b/xen/arch/x86/hvm/viridian/viridian.c
index 21480d9ee700..33d54e587edf 100644
--- a/xen/arch/x86/hvm/viridian/viridian.c
+++ b/xen/arch/x86/hvm/viridian/viridian.c
@@ -933,13 +933,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 +1038,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 b6885d0e2764..eee1d4b47a13 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 78135ca23be8..cf47d61b1473 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 02de18c7d4a8..124906a548da 100644
--- a/xen/arch/x86/include/asm/hvm/hvm.h
+++ b/xen/arch/x86/include/asm/hvm/hvm.h
@@ -26,6 +26,20 @@ extern bool opt_hvm_fep;
#define opt_hvm_fep 0
#endif
+/*
+ * Results for hvm_guest_x86_mode().
+ *
+ * Note, some callers depend on the order of these constants.
+ *
+ * TODO: Rework this helper to avoid implying mixing the architectural
+ * concepts of mode and operand size.
+ */
+#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,
diff --git a/xen/arch/x86/oprofile/xenoprof.c b/xen/arch/x86/oprofile/xenoprof.c
index 247a0deca822..7f2525bfb4d6 100644
--- a/xen/arch/x86/oprofile/xenoprof.c
+++ b/xen/arch/x86/oprofile/xenoprof.c
@@ -86,11 +86,11 @@ int xenoprofile_get_mode(struct vcpu *curr, const struct cpu_user_regs *regs)
switch ( hvm_guest_x86_mode(curr) )
{
- case 0: /* real mode */
+ case X86_MODE_REAL:
return 1;
- case 1: /* vm86 mode */
+ case X86_MODE_VM86:
return 0;
- default:
+ default: /* 16BIT | 32BIT | 64BIT */
return hvm_get_cpl(curr) != 3;
}
}
base-commit: 826a9eb072d449cb777d71f52923e6f5f20cefbe
--
2.39.5
^ permalink raw reply related [flat|nested] 12+ messages in thread* Re: [PATCH v3] x86/hvm: Use constants for x86 modes
2024-12-18 17:08 ` [PATCH v3] " Andrew Cooper
@ 2024-12-19 8:18 ` Jan Beulich
2024-12-19 10:46 ` Teddy Astie
1 sibling, 0 replies; 12+ messages in thread
From: Jan Beulich @ 2024-12-19 8:18 UTC (permalink / raw)
To: Andrew Cooper; +Cc: Teddy Astie, Roger Pau Monné, Xen-devel
On 18.12.2024 18:08, Andrew Cooper wrote:
> @@ -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";
I did ask for the string literal to also become "vm86". I'm simply
unaware of "v86" being used anywhere in the vendor docs.
> --- a/xen/arch/x86/include/asm/hvm/hvm.h
> +++ b/xen/arch/x86/include/asm/hvm/hvm.h
> @@ -26,6 +26,20 @@ extern bool opt_hvm_fep;
> #define opt_hvm_fep 0
> #endif
>
> +/*
> + * Results for hvm_guest_x86_mode().
> + *
> + * Note, some callers depend on the order of these constants.
> + *
> + * TODO: Rework this helper to avoid implying mixing the architectural
> + * concepts of mode and operand size.
> + */
> +#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
As this block is no longer adjacent to hvm_guest_x86_mode()'s decl, perhaps
s/this/that/ in the comment?
Preferably with these adjustments:
Reviewed-by: Jan Beulich <jbeulich@suse.com>
Jan
^ permalink raw reply [flat|nested] 12+ messages in thread* Re: [PATCH v3] x86/hvm: Use constants for x86 modes
2024-12-18 17:08 ` [PATCH v3] " Andrew Cooper
2024-12-19 8:18 ` Jan Beulich
@ 2024-12-19 10:46 ` Teddy Astie
2024-12-19 11:35 ` Andrew Cooper
1 sibling, 1 reply; 12+ messages in thread
From: Teddy Astie @ 2024-12-19 10:46 UTC (permalink / raw)
To: Andrew Cooper, Xen-devel; +Cc: Jan Beulich, Roger Pau Monné
Hello,
Le 18/12/2024 à 18:08, Andrew Cooper a écrit :
> From: Teddy Astie <teddy.astie@vates.tech>
>
> 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>
> Signed-off-by: Andrew Cooper <andrew.cooper3@citrix.com>
Acked-by: Teddy Astie <teddy.astie@vates.tech>
Thanks
> ---
> CC: Jan Beulich <JBeulich@suse.com>
> CC: Roger Pau Monné <roger.pau@citrix.com>
> CC: Teddy Astie <teddy.astie@vates.tech>
>
> v3:
> * Leave a comment behind explaining why these aren't all modes
> * Fix long lines
> * Convert more instances (svm_guest_x86_mode, hvm_latch_shinfo_size, xenoprof)
> ---
> xen/arch/x86/hvm/emulate.c | 18 ++++++++++--------
> xen/arch/x86/hvm/hvm.c | 4 +++-
> xen/arch/x86/hvm/hypercall.c | 17 +++++++++--------
> xen/arch/x86/hvm/svm/svm.c | 8 ++++----
> xen/arch/x86/hvm/viridian/viridian.c | 8 ++++----
> xen/arch/x86/hvm/vmx/vmx.c | 9 +++++----
> xen/arch/x86/hvm/vmx/vvmx.c | 5 +++--
> xen/arch/x86/include/asm/hvm/hvm.h | 14 ++++++++++++++
> xen/arch/x86/oprofile/xenoprof.c | 6 +++---
> 9 files changed, 55 insertions(+), 34 deletions(-)
>
> diff --git a/xen/arch/x86/hvm/emulate.c b/xen/arch/x86/hvm/emulate.c
> index d3006f094a69..76467b76c047 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/hvm.c b/xen/arch/x86/hvm/hvm.c
> index 74e58c653e6f..922c9b3af64d 100644
> --- a/xen/arch/x86/hvm/hvm.c
> +++ b/xen/arch/x86/hvm/hvm.c
> @@ -3974,7 +3974,9 @@ static void hvm_latch_shinfo_size(struct domain *d)
> */
> if ( current->domain == d )
> {
> - d->arch.has_32bit_shinfo = (hvm_guest_x86_mode(current) != 8);
> + d->arch.has_32bit_shinfo =
> + hvm_guest_x86_mode(current) != X86_MODE_64BIT;
> +
> /*
> * Make sure that the timebase in the shared info structure is correct.
> *
> diff --git a/xen/arch/x86/hvm/hypercall.c b/xen/arch/x86/hvm/hypercall.c
> index 81883c8d4f60..6f8dfdff4ac6 100644
> --- a/xen/arch/x86/hvm/hypercall.c
> +++ b/xen/arch/x86/hvm/hypercall.c
> @@ -112,23 +112,24 @@ 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 )
> + eax == __HYPERVISOR_hvm_op &&
> + (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;
> }
>
> @@ -198,7 +199,7 @@ enum mc_disposition hvm_do_multicall_call(struct mc_state *state)
> {
> struct vcpu *curr = current;
>
> - if ( hvm_guest_x86_mode(curr) == 8 )
> + if ( hvm_guest_x86_mode(curr) == X86_MODE_64BIT )
> {
> struct multicall_entry *call = &state->call;
>
> diff --git a/xen/arch/x86/hvm/svm/svm.c b/xen/arch/x86/hvm/svm/svm.c
> index b8f87aa1ed08..62905c2c7acd 100644
> --- a/xen/arch/x86/hvm/svm/svm.c
> +++ b/xen/arch/x86/hvm/svm/svm.c
> @@ -571,12 +571,12 @@ static int cf_check svm_guest_x86_mode(struct vcpu *v)
> struct vmcb_struct *vmcb = v->arch.hvm.svm.vmcb;
>
> 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;
> if ( hvm_long_mode_active(v) && likely(vmcb->cs.l) )
> - return 8;
> - return likely(vmcb->cs.db) ? 4 : 2;
> + return X86_MODE_64BIT;
> + return vmcb->cs.db ? X86_MODE_32BIT : X86_MODE_16BIT;
> }
>
> static void cf_check svm_cpuid_policy_changed(struct vcpu *v)
> diff --git a/xen/arch/x86/hvm/viridian/viridian.c b/xen/arch/x86/hvm/viridian/viridian.c
> index 21480d9ee700..33d54e587edf 100644
> --- a/xen/arch/x86/hvm/viridian/viridian.c
> +++ b/xen/arch/x86/hvm/viridian/viridian.c
> @@ -933,13 +933,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 +1038,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 b6885d0e2764..eee1d4b47a13 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 78135ca23be8..cf47d61b1473 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 02de18c7d4a8..124906a548da 100644
> --- a/xen/arch/x86/include/asm/hvm/hvm.h
> +++ b/xen/arch/x86/include/asm/hvm/hvm.h
> @@ -26,6 +26,20 @@ extern bool opt_hvm_fep;
> #define opt_hvm_fep 0
> #endif
>
> +/*
> + * Results for hvm_guest_x86_mode().
> + *
> + * Note, some callers depend on the order of these constants.
> + *
> + * TODO: Rework this helper to avoid implying mixing the architectural
> + * concepts of mode and operand size.
> + */
> +#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,
> diff --git a/xen/arch/x86/oprofile/xenoprof.c b/xen/arch/x86/oprofile/xenoprof.c
> index 247a0deca822..7f2525bfb4d6 100644
> --- a/xen/arch/x86/oprofile/xenoprof.c
> +++ b/xen/arch/x86/oprofile/xenoprof.c
> @@ -86,11 +86,11 @@ int xenoprofile_get_mode(struct vcpu *curr, const struct cpu_user_regs *regs)
>
> switch ( hvm_guest_x86_mode(curr) )
> {
> - case 0: /* real mode */
> + case X86_MODE_REAL:
> return 1;
> - case 1: /* vm86 mode */
> + case X86_MODE_VM86:
> return 0;
> - default:
> + default: /* 16BIT | 32BIT | 64BIT */
> return hvm_get_cpl(curr) != 3;
> }
> }
>
> base-commit: 826a9eb072d449cb777d71f52923e6f5f20cefbe
Teddy
| Vates
XCP-ng & Xen Orchestra - Vates solutions
web: https://vates.tech
^ permalink raw reply [flat|nested] 12+ messages in thread* Re: [PATCH v3] x86/hvm: Use constants for x86 modes
2024-12-19 10:46 ` Teddy Astie
@ 2024-12-19 11:35 ` Andrew Cooper
2024-12-19 13:04 ` Teddy Astie
0 siblings, 1 reply; 12+ messages in thread
From: Andrew Cooper @ 2024-12-19 11:35 UTC (permalink / raw)
To: Teddy Astie, Xen-devel; +Cc: Jan Beulich, Roger Pau Monné
On 19/12/2024 10:46 am, Teddy Astie wrote:
> Hello,
>
> Le 18/12/2024 à 18:08, Andrew Cooper a écrit :
>> From: Teddy Astie <teddy.astie@vates.tech>
>>
>> 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>
>> Signed-off-by: Andrew Cooper <andrew.cooper3@citrix.com>
> Acked-by: Teddy Astie <teddy.astie@vates.tech>
Thanks, but as you're not a maintainer of this code, Ack doesn't carry
any weight. Reviewed-by OTOH would, if you're happy with that adjustment?
~Andrew
^ permalink raw reply [flat|nested] 12+ messages in thread
* Re: [PATCH v3] x86/hvm: Use constants for x86 modes
2024-12-19 11:35 ` Andrew Cooper
@ 2024-12-19 13:04 ` Teddy Astie
0 siblings, 0 replies; 12+ messages in thread
From: Teddy Astie @ 2024-12-19 13:04 UTC (permalink / raw)
To: Andrew Cooper, Xen-devel; +Cc: Jan Beulich, Roger Pau Monné
Le 19/12/2024 à 12:35, Andrew Cooper a écrit :
> On 19/12/2024 10:46 am, Teddy Astie wrote:
>> Hello,
>>
>> Le 18/12/2024 à 18:08, Andrew Cooper a écrit :
>>> From: Teddy Astie <teddy.astie@vates.tech>
>>>
>>> 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>
>>> Signed-off-by: Andrew Cooper <andrew.cooper3@citrix.com>
>> Acked-by: Teddy Astie <teddy.astie@vates.tech>
>
> Thanks, but as you're not a maintainer of this code, Ack doesn't carry
> any weight. Reviewed-by OTOH would, if you're happy with that adjustment?
>
> ~Andrew
Yes, I meant Reviewed-by in my Acked-by.
So,
Reviewed-by: Teddy Astie <teddy.astie@vates.tech>
Teddy
Teddy Astie | Vates XCP-ng Developer
XCP-ng & Xen Orchestra - Vates solutions
web: https://vates.tech
^ permalink raw reply [flat|nested] 12+ messages in thread
end of thread, other threads:[~2024-12-19 13:04 UTC | newest]
Thread overview: 12+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2024-12-02 9:49 [XEN PATCH v2] x86/hvm: Use constants for x86 modes Teddy Astie
2024-12-04 11:10 ` Jan Beulich
2024-12-04 13:01 ` Andrew Cooper
2024-12-05 18:02 ` Teddy Astie
2024-12-18 15:54 ` Andrew Cooper
2024-12-18 16:23 ` Andrew Cooper
2024-12-18 16:56 ` Andrew Cooper
2024-12-18 17:08 ` [PATCH v3] " Andrew Cooper
2024-12-19 8:18 ` Jan Beulich
2024-12-19 10:46 ` Teddy Astie
2024-12-19 11:35 ` Andrew Cooper
2024-12-19 13:04 ` Teddy Astie
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.