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

Teddy Astie posted 1 patch 1 year, 2 months ago
Patches applied successfully (tree, apply log)
git fetch https://gitlab.com/xen-project/patchew/xen tags/patchew/bf7146a8ccbf05ddc74d4f451a5fa586309b9a50.1733132729.git.teddy.astie@vates.tech
There is a newer version of this series
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(-)
[XEN PATCH v2] x86/hvm: Use constants for x86 modes
Posted by Teddy Astie 1 year, 2 months ago
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
Re: [XEN PATCH v2] x86/hvm: Use constants for x86 modes
Posted by Andrew Cooper 1 year, 1 month ago
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

Re: [XEN PATCH v2] x86/hvm: Use constants for x86 modes
Posted by Andrew Cooper 1 year, 1 month ago
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

Re: [XEN PATCH v2] x86/hvm: Use constants for x86 modes
Posted by Andrew Cooper 1 year, 1 month ago
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

Re: [XEN PATCH v2] x86/hvm: Use constants for x86 modes
Posted by Andrew Cooper 1 year, 2 months ago
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

Re: [XEN PATCH v2] x86/hvm: Use constants for x86 modes
Posted by Teddy Astie 1 year, 2 months ago
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
Re: [XEN PATCH v2] x86/hvm: Use constants for x86 modes
Posted by Jan Beulich 1 year, 2 months ago
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
[PATCH v3] x86/hvm: Use constants for x86 modes
Posted by Andrew Cooper 1 year, 1 month ago
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


Re: [PATCH v3] x86/hvm: Use constants for x86 modes
Posted by Teddy Astie 1 year, 1 month ago
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
Re: [PATCH v3] x86/hvm: Use constants for x86 modes
Posted by Andrew Cooper 1 year, 1 month ago
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

Re: [PATCH v3] x86/hvm: Use constants for x86 modes
Posted by Teddy Astie 1 year, 1 month ago
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
Re: [PATCH v3] x86/hvm: Use constants for x86 modes
Posted by Jan Beulich 1 year, 1 month ago
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